Bug 187375 - [iOS] When bringing MobileSafari to the foreground, images, which are pending decoding, won't be drawn into the immediate-paint transaction
Summary: [iOS] When bringing MobileSafari to the foreground, images, which are pending...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-05 18:05 PDT by Said Abou-Hallawa
Modified: 2018-07-16 17:17 PDT (History)
6 users (show)

See Also:


Attachments
Patch (12.66 KB, patch)
2018-07-10 18:21 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (869.28 KB, application/zip)
2018-07-10 19:46 PDT, Build Bot
no flags Details
Patch (13.56 KB, patch)
2018-07-11 19:48 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews204 for win-future (12.83 MB, application/zip)
2018-07-11 21:36 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.29 MB, application/zip)
2018-07-11 21:43 PDT, Build Bot
no flags Details
Patch (13.57 KB, patch)
2018-07-12 09:23 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (19.68 KB, patch)
2018-07-12 18:26 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.27 MB, application/zip)
2018-07-12 19:29 PDT, Build Bot
no flags Details
Patch (19.69 KB, patch)
2018-07-13 08:44 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (26.59 KB, patch)
2018-07-13 13:14 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (32.64 KB, patch)
2018-07-16 15:50 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (32.64 KB, patch)
2018-07-16 16:28 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2018-07-05 18:05:34 PDT
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.
Comment 1 Radar WebKit Bug Importer 2018-07-05 18:06:18 PDT
<rdar://problem/41873022>
Comment 2 Said Abou-Hallawa 2018-07-10 18:21:57 PDT
Created attachment 344742 [details]
Patch
Comment 3 Build Bot 2018-07-10 19:45:59 PDT
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.
Comment 4 Build Bot 2018-07-10 19:46:00 PDT
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
Comment 5 Said Abou-Hallawa 2018-07-11 19:48:20 PDT
Created attachment 344810 [details]
Patch
Comment 6 Build Bot 2018-07-11 21:36:26 PDT
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
Comment 7 Build Bot 2018-07-11 21:36:37 PDT
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 8 Build Bot 2018-07-11 21:43:33 PDT
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
Comment 9 Build Bot 2018-07-11 21:43:35 PDT
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
Comment 10 Said Abou-Hallawa 2018-07-12 09:23:33 PDT
Created attachment 344848 [details]
Patch
Comment 11 Tim Horton 2018-07-12 11:41:15 PDT
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”.
Comment 12 Said Abou-Hallawa 2018-07-12 18:26:14 PDT
Created attachment 344911 [details]
Patch
Comment 13 Build Bot 2018-07-12 19:29:48 PDT
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
Comment 14 Build Bot 2018-07-12 19:29:50 PDT
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
Comment 15 Said Abou-Hallawa 2018-07-13 08:44:27 PDT
Created attachment 344947 [details]
Patch
Comment 16 Said Abou-Hallawa 2018-07-13 09:21:32 PDT
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 17 Simon Fraser (smfr) 2018-07-13 10:46:21 PDT
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
Comment 18 Said Abou-Hallawa 2018-07-13 13:14:17 PDT
Created attachment 344970 [details]
Patch
Comment 19 Said Abou-Hallawa 2018-07-16 11:03:23 PDT
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 20 Simon Fraser (smfr) 2018-07-16 11:15:00 PDT
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
Comment 21 Said Abou-Hallawa 2018-07-16 15:50:47 PDT
Created attachment 345126 [details]
Patch
Comment 22 Said Abou-Hallawa 2018-07-16 16:28:20 PDT
Created attachment 345129 [details]
Patch
Comment 23 WebKit Commit Bot 2018-07-16 17:17:52 PDT
Comment on attachment 345129 [details]
Patch

Clearing flags on attachment: 345129

Committed r233872: <https://trac.webkit.org/changeset/233872>
Comment 24 WebKit Commit Bot 2018-07-16 17:17:54 PDT
All reviewed patches have been landed.  Closing bug.