Summary: | [iOS] When bringing MobileSafari to the foreground, images, which are pending decoding, won't be drawn into the immediate-paint transaction | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||||||||||||||||
Component: | Images | Assignee: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, ews-watchlist, rniwa, simon.fraser, thorton, webkit-bug-importer | ||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=187374 | ||||||||||||||||||||||||||||
Attachments: |
|
Description
Said Abou-Hallawa
2018-07-05 18:05:34 PDT
Created attachment 344742 [details]
Patch
Comment on attachment 344742 [details] Patch Attachment 344742 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/8500741 Number of test failures exceeded the failure limit. Created attachment 344747 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Created attachment 344810 [details]
Patch
Comment on attachment 344810 [details] Patch Attachment 344810 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8511579 New failing tests: http/tests/security/video-poster-cross-origin-crash2.html Created attachment 344821 [details]
Archive of layout-test-results from ews204 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 344810 [details] Patch Attachment 344810 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/8511538 New failing tests: fast/inline/out-of-flow-positioned-render-replaced-box-moves.html Created attachment 344823 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Created attachment 344848 [details]
Patch
Comment on attachment 344848 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344848&action=review > Source/WebCore/page/FrameView.h:643 > + WEBCORE_EXPORT void traverseRenderTreeForNonPainting(GraphicsContext::NonPaintingReasons); Something about the parts of speech feels really weird here. Or maybe it’s because “non-painting” is not a common phrase. > Source/WebCore/platform/ScrollView.cpp:1170 > + if (context.paintingDisabled() && !context.updatingNonPainting()) This is just clearly worded wrong, you’re not “updating a non-painting”. Created attachment 344911 [details]
Patch
Comment on attachment 344911 [details] Patch Attachment 344911 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/8522318 New failing tests: accessibility/smart-invert-reference.html Created attachment 344917 [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
Created attachment 344947 [details]
Patch
Comment on attachment 344848 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344848&action=review >> Source/WebCore/page/FrameView.h:643 >> + WEBCORE_EXPORT void traverseRenderTreeForNonPainting(GraphicsContext::NonPaintingReasons); > > Something about the parts of speech feels really weird here. Or maybe it’s because “non-painting” is not a common phrase. I made these function have the following prototypes: void scanRenderTree(GraphicsContext::ScanningFunction); void paintControlTints() { scanRenderTree(GraphicsContext::ScanningFunction::PaintingControlTints); } void repaintImagesPendingDecoding() { scanRenderTree(GraphicsContext::ScanningFunction::RepaintingImagesPendingDecoding); } >> Source/WebCore/platform/ScrollView.cpp:1170 >> + if (context.paintingDisabled() && !context.updatingNonPainting()) > > This is just clearly worded wrong, you’re not “updating a non-painting”. I made the functions in GraphicsContext.h have the following name: bool isScanningForUpdating() const { return m_scanningFunction != ScanningFunction::None; } bool isPaintingControlTints() const { return m_scanningFunction == ScanningFunction::PaintingControlTints; } bool isRepaintingImagesPendingDecoding() const { return m_scanningFunction == ScanningFunction::RepaintingImagesPendingDecoding; } Comment on attachment 344848 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344848&action=review >>> Source/WebCore/page/FrameView.h:643 >>> + WEBCORE_EXPORT void traverseRenderTreeForNonPainting(GraphicsContext::NonPaintingReasons); >> >> Something about the parts of speech feels really weird here. Or maybe it’s because “non-painting” is not a common phrase. > > I made these function have the following prototypes: > > void scanRenderTree(GraphicsContext::ScanningFunction); > void paintControlTints() { scanRenderTree(GraphicsContext::ScanningFunction::PaintingControlTints); } > void repaintImagesPendingDecoding() { scanRenderTree(GraphicsContext::ScanningFunction::RepaintingImagesPendingDecoding); } I don't like "scan": it doesn't tell you that it's doing a pass in painting order, and it sounds like it's looking for something, rather than traversing the entire tree. Maybe "traverseForPaintInvalidation()" or something. > Source/WebCore/platform/graphics/GraphicsContext.h:262 > enum class NonPaintingReasons { I think we can make this less negative, and call them paintInvalidationReasons. > Source/WebCore/platform/graphics/GraphicsContext.h:273 > + bool updatingNonPainting() const { return m_nonPaintingReasons != NonPaintingReasons::NoReasons; } performingPaintInvalidation()? > Source/WebCore/platform/graphics/GraphicsContext.h:274 > bool updatingControlTints() const { return m_nonPaintingReasons == NonPaintingReasons::UpdatingControlTints; } invalidatingControlTints > Source/WebCore/platform/graphics/GraphicsContext.h:275 > + bool updatingPendingImageDecoding() const { return m_nonPaintingReasons == NonPaintingReasons::UpdatingPendingImageDecoding; } invalidatingImagesWithAsyncDecodes Created attachment 344970 [details]
Patch
Comment on attachment 344848 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344848&action=review >>>> Source/WebCore/page/FrameView.h:643 >>>> + WEBCORE_EXPORT void traverseRenderTreeForNonPainting(GraphicsContext::NonPaintingReasons); >>> >>> Something about the parts of speech feels really weird here. Or maybe it’s because “non-painting” is not a common phrase. >> >> I made these function have the following prototypes: >> >> void scanRenderTree(GraphicsContext::ScanningFunction); >> void paintControlTints() { scanRenderTree(GraphicsContext::ScanningFunction::PaintingControlTints); } >> void repaintImagesPendingDecoding() { scanRenderTree(GraphicsContext::ScanningFunction::RepaintingImagesPendingDecoding); } > > I don't like "scan": it doesn't tell you that it's doing a pass in painting order, and it sounds like it's looking for something, rather than traversing the entire tree. > > Maybe "traverseForPaintInvalidation()" or something. Function was renamed to traverseForPaintInvalidation(). >> Source/WebCore/platform/graphics/GraphicsContext.h:262 >> enum class NonPaintingReasons { > > I think we can make this less negative, and call them paintInvalidationReasons. This enum was renamed to enum class PaintInvalidationReasons { None, InvalidatingControlTints, InvalidatingImagesWithAsyncDecodes }; >> Source/WebCore/platform/graphics/GraphicsContext.h:273 >> + bool updatingNonPainting() const { return m_nonPaintingReasons != NonPaintingReasons::NoReasons; } > > performingPaintInvalidation()? Done. >> Source/WebCore/platform/graphics/GraphicsContext.h:274 >> bool updatingControlTints() const { return m_nonPaintingReasons == NonPaintingReasons::UpdatingControlTints; } > > invalidatingControlTints Done. >> Source/WebCore/platform/graphics/GraphicsContext.h:275 >> + bool updatingPendingImageDecoding() const { return m_nonPaintingReasons == NonPaintingReasons::UpdatingPendingImageDecoding; } > > invalidatingImagesWithAsyncDecodes Done. Comment on attachment 344970 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344970&action=review > Source/WebCore/loader/cache/CachedImage.cpp:143 > +bool CachedImage::isPendingImageDrawingClient(CachedImageClient& client) const This function name is hard to parse. Does it mean "Is client of pending image drawing" or "has image drawing client which is pending (something)"? > Source/WebCore/loader/cache/CachedImage.cpp:167 > +void CachedImage::clearPendingImageDrawingClient() Also hard to parse. > Source/WebCore/platform/graphics/GraphicsContext.h:262 > + enum class PaintInvalidationReasons { : unint8_t Created attachment 345126 [details]
Patch
Created attachment 345129 [details]
Patch
Comment on attachment 345129 [details] Patch Clearing flags on attachment: 345129 Committed r233872: <https://trac.webkit.org/changeset/233872> All reviewed patches have been landed. Closing bug. |