RESOLVED FIXED 186614
Make it possible to add a border around loading or failed-to-load images
https://bugs.webkit.org/show_bug.cgi?id=186614
Summary Make it possible to add a border around loading or failed-to-load images
Tim Horton
Reported 2018-06-13 18:22:02 PDT
Make it possible to add a border around loading or failed-to-load images
Attachments
Patch (19.49 KB, patch)
2018-06-13 18:22 PDT, Tim Horton
no flags
Archive of layout-test-results from ews101 for mac-sierra (805.41 KB, application/zip)
2018-06-13 18:59 PDT, EWS Watchlist
no flags
Archive of layout-test-results from ews116 for mac-sierra (544.25 KB, application/zip)
2018-06-13 19:35 PDT, EWS Watchlist
no flags
Archive of layout-test-results from ews201 for win-future (12.76 MB, application/zip)
2018-06-14 15:57 PDT, EWS Watchlist
no flags
Patch (20.31 KB, patch)
2018-06-21 00:01 PDT, Tim Horton
no flags
Archive of layout-test-results from ews100 for mac-sierra (540.67 KB, application/zip)
2018-06-21 00:54 PDT, EWS Watchlist
no flags
Archive of layout-test-results from ews114 for mac-sierra (537.97 KB, application/zip)
2018-06-21 01:05 PDT, EWS Watchlist
no flags
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.83 MB, application/zip)
2018-06-21 05:14 PDT, EWS Watchlist
no flags
Patch (20.28 KB, patch)
2018-06-22 13:34 PDT, Tim Horton
no flags
Patch (23.05 KB, patch)
2018-06-22 16:24 PDT, Tim Horton
no flags
Tim Horton
Comment 1 2018-06-13 18:22:48 PDT
EWS Watchlist
Comment 2 2018-06-13 18:59:55 PDT
Comment on attachment 342717 [details] Patch Attachment 342717 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/8171818 Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 3 2018-06-13 18:59:56 PDT
Created attachment 342718 [details] Archive of layout-test-results from ews101 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
zalan
Comment 4 2018-06-13 19:32:55 PDT
Comment on attachment 342717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342717&action=review > Source/WebCore/rendering/RenderImage.cpp:381 > +void RenderImage::paintIncompleteImageOutline(PaintInfo& paintInfo, const LayoutPoint& paintOffset, const LayoutUnit& borderWidth) void RenderImage::paintIncompleteImageOutline(PaintInfo& paintInfo, LayoutPoint paintOffset, LayoutUnit borderWidth) > Source/WebCore/rendering/RenderImage.cpp:390 > + LayoutUnit leftBorder = borderLeft(); > + LayoutUnit topBorder = borderTop(); > + LayoutUnit leftPad = paddingLeft(); > + LayoutUnit topPad = paddingTop(); auto and please don't abbrev. > Source/WebCore/rendering/RenderImage.cpp:480 > + LayoutUnit incompleteImageBorderWidth = LayoutUnit(2 / deviceScaleFactor); This makes the image borders thicker. is it intentional? > Source/WebCore/rendering/RenderImage.h:110 > + void paintIncompleteImageOutline(PaintInfo&, const LayoutPoint&, const LayoutUnit&); shouldn't this paint function be const?
EWS Watchlist
Comment 5 2018-06-13 19:35:30 PDT
Comment on attachment 342717 [details] Patch Attachment 342717 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/8171988 Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 6 2018-06-13 19:35:31 PDT
Created attachment 342719 [details] Archive of layout-test-results from ews116 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-sierra Platform: Mac OS X 10.12.6
Tim Horton
Comment 7 2018-06-13 21:06:25 PDT
(In reply to zalan from comment #4) > Comment on attachment 342717 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=342717&action=review > > > Source/WebCore/rendering/RenderImage.cpp:381 > > +void RenderImage::paintIncompleteImageOutline(PaintInfo& paintInfo, const LayoutPoint& paintOffset, const LayoutUnit& borderWidth) > > void RenderImage::paintIncompleteImageOutline(PaintInfo& paintInfo, > LayoutPoint paintOffset, LayoutUnit borderWidth) > > > Source/WebCore/rendering/RenderImage.cpp:390 > > + LayoutUnit leftBorder = borderLeft(); > > + LayoutUnit topBorder = borderTop(); > > + LayoutUnit leftPad = paddingLeft(); > > + LayoutUnit topPad = paddingTop(); > > auto > and please don't abbrev. Just moved, but fine :) > > Source/WebCore/rendering/RenderImage.cpp:480 > > + LayoutUnit incompleteImageBorderWidth = LayoutUnit(2 / deviceScaleFactor); > > This makes the image borders thicker. is it intentional? Technically intentional (see radar), but it /is/ kind of weird that the incomplete border is thicker than the failed-to-load border. Maybe we should change both. Or neither. > > Source/WebCore/rendering/RenderImage.h:110 > > + void paintIncompleteImageOutline(PaintInfo&, const LayoutPoint&, const LayoutUnit&); > > shouldn't this paint function be const? Almost certainly.
Tim Horton
Comment 8 2018-06-13 21:08:24 PDT
It sort of looks like the tests got off-by-one?
EWS Watchlist
Comment 9 2018-06-14 15:57:12 PDT
Comment on attachment 342717 [details] Patch Attachment 342717 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/8185401 New failing tests: http/tests/images/loading-image-border.html
EWS Watchlist
Comment 10 2018-06-14 15:57:23 PDT
Created attachment 342771 [details] Archive of layout-test-results from ews201 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews201 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Tim Horton
Comment 11 2018-06-21 00:01:08 PDT
EWS Watchlist
Comment 12 2018-06-21 00:54:26 PDT
Comment on attachment 343218 [details] Patch Attachment 343218 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/8274643 Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 13 2018-06-21 00:54:28 PDT
Created attachment 343222 [details] Archive of layout-test-results from ews100 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-sierra Platform: Mac OS X 10.12.6
EWS Watchlist
Comment 14 2018-06-21 01:05:14 PDT
Comment on attachment 343218 [details] Patch Attachment 343218 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8274630 Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 15 2018-06-21 01:05:16 PDT
Created attachment 343223 [details] Archive of layout-test-results from ews114 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-sierra Platform: Mac OS X 10.12.6
EWS Watchlist
Comment 16 2018-06-21 05:13:58 PDT
Comment on attachment 343218 [details] Patch Attachment 343218 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/8276262 New failing tests: imported/w3c/web-platform-tests/streams/piping/error-propagation-forward.html
EWS Watchlist
Comment 17 2018-06-21 05:14:00 PDT
Created attachment 343237 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Wenson Hsieh
Comment 18 2018-06-21 07:35:47 PDT
Comment on attachment 343218 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343218&action=review > Source/WebCore/page/Settings.yaml:755 > + initial: false Should we trigger a repaint here when the value changes? (Probably not that important though, since our client is only going to enable this from the get-go and not fiddle with it later.)
Anders Carlsson
Comment 19 2018-06-21 08:11:33 PDT
Comment on attachment 343218 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343218&action=review > Source/WebKit/UIProcess/API/Cocoa/WKPreferences.mm:1266 > +- (void)_setIncompleteImageBorderEnabled:(BOOL)incompleteImageBorderEnabled > +{ > + _preferences->setIncompleteImageBorderEnabled(incompleteImageBorderEnabled); > +} > + > +- (BOOL)_incompleteImageBorderEnabled > +{ > + return _preferences->incompleteImageBorderEnabled(); > +} Doesn't seem like this should be a preference.
zalan
Comment 20 2018-06-21 08:13:44 PDT
Comment on attachment 343218 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343218&action=review > Source/WebCore/rendering/RenderImage.cpp:382 > +void RenderImage::paintIncompleteImageOutline(PaintInfo& paintInfo, const LayoutPoint& paintOffset, const LayoutUnit& borderWidth) const Apparently you missed it from my previous review. const LayoutPoint& paintOffset -> LayoutPoint const LayoutUnit& borderWidth -> LayoutUnit > Source/WebCore/rendering/RenderImage.cpp:384 > + LayoutSize contentSize = this->contentSize(); auto
Tim Horton
Comment 21 2018-06-22 13:34:46 PDT
Tim Horton
Comment 22 2018-06-22 16:24:41 PDT
WebKit Commit Bot
Comment 23 2018-06-22 17:53:03 PDT
Comment on attachment 343392 [details] Patch Clearing flags on attachment: 343392 Committed r233115: <https://trac.webkit.org/changeset/233115>
WebKit Commit Bot
Comment 24 2018-06-22 17:53:04 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 25 2018-06-22 17:55:11 PDT
Note You need to log in before you can comment on or make changes to this bug.