Bug 187375 – [iOS] When bringing MobileSafari to the foreground, images, which are pending decoding, won't be drawn into the immediate-paint transaction

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Said Abou-Hallawa]

FlushImmediately paint should force all the contents of the page to be painted synchronously. But all the images which are pending for asynchronous image decoding will be waiting for the decoding to finish. So they are not going to be painted during this FlushImmediately pass. Instead the background of these images will still be painted. To fix this issue, the renderers of these pending images have to be repainted and their images have to be drawn synchronously.

[Radar WebKit Bug Importer]

<rdar://problem/41873022>

[Said Abou-Hallawa]

Created attachment 344742 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Said Abou-Hallawa]

Created attachment 344810 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Said Abou-Hallawa]

Created attachment 344848 [details]
Patch

[Tim Horton]

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”.

[Said Abou-Hallawa]

Created attachment 344911 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Said Abou-Hallawa]

Created attachment 344947 [details]
Patch

[Said Abou-Hallawa]

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; }

[Simon Fraser (smfr)]

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

[Said Abou-Hallawa]

Created attachment 344970 [details]
Patch

[Said Abou-Hallawa]

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.

[Simon Fraser (smfr)]

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

[Said Abou-Hallawa]

Created attachment 345126 [details]
Patch

[Said Abou-Hallawa]

Created attachment 345129 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 345129 [details]
Patch

Clearing flags on attachment: 345129

Committed r233872: <https://trac.webkit.org/changeset/233872>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.