Bug 191078 – [iOS] Issue initial paint soon after the visuallyNonEmpty milestone is fired.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[zalan]

as opposed to waiting for 500ms with the initial paint.

[zalan]

waiting 500ms that is.

[zalan]

Created attachment 353403 [details]
Patch

[EWS Watchlist]

Attachment 353403 [details] did not pass style-queue:


ERROR: Source/WebCore/page/ChromeClient.h:336:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebCore/page/FrameView.cpp:96:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 353403 [details]
Patch

Attachment 353403 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9783488

New failing tests:
tiled-drawing/simple-document-with-margin-tiles.html
compositing/tiling/non-visible-window-tile-coverage.html

[EWS Watchlist]

Created attachment 353408 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Simon Fraser (smfr)]

Comment on attachment 353403 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353403&action=review

> Source/WebCore/ChangeLog:12
> +        1. Improve visuallyNoneEmpty milestone confidence
> +        2. Issue initial paint soon after the milestone fires
> +        3. Suspend optional style recalcs and layouts while painting is being throttled.

Will you break this into 3 patches?

> Source/WebCore/dom/Document.cpp:1810
> +    const Seconds styleRecalcDelay = 50_ms;

Would be nice to see all the throttling-related code in one place some day.

> Source/WebCore/page/FrameView.cpp:4415
> +        auto hasPendingScriptExecution = frame().document()->scriptRunner() && frame().document()->scriptRunner()->hasPendingScripts();
> +        if (auto* documentLoader = frame().loader().documentLoader()) {
> +            auto& resourceLoader = documentLoader->cachedResourceLoader();
> +            if (!resourceLoader.requestCount())
> +                return hasPendingScriptExecution;
> +
> +            auto& resources = resourceLoader.allCachedResources();
> +            for (auto& resource : resources) {
> +                if (resource.value->isLoaded())
> +                    continue;
> +                if (resource.value->type() == CachedResource::Type::CSSStyleSheet || resource.value->type() == CachedResource::Type::Script || resource.value->type() == CachedResource::Type::FontResource)
> +                    return false;
> +            }
> +        }

Maybe pull this into a separate function.

> Source/WebCore/page/FrameView.h:396
> +    void incrementVisuallyNonEmptyCharacterCount(const String);

const String&

> Source/WebCore/page/FrameView.h:947
> +    auto whitespaceCheck = [](UChar character) {
> +        return character == ' ' || character == '\t' || character == '\n';
> +    };

Can you not share some existing code for this? What about   ?

> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.h:117
> +    void initialDeferredPaint();

Hard to figure out what this means. Does it mean do the paint, or something else?

[Radar WebKit Bug Importer]

<rdar://problem/45736178>

[zalan]

Created attachment 353624 [details]
Patch

[EWS Watchlist]

Attachment 353624 [details] did not pass style-queue:


ERROR: Source/WebCore/page/ChromeClient.h:336:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[zalan]

(In reply to Simon Fraser (smfr) from comment #6)
> Comment on attachment 353403 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353403&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        1. Improve visuallyNoneEmpty milestone confidence
> > +        2. Issue initial paint soon after the milestone fires
> > +        3. Suspend optional style recalcs and layouts while painting is being throttled.
> 
> Will you break this into 3 patches?
Normally I would, but since the additional paint is certainly a perf regression, I'd rather have it together with the potential progression of suspending optional style recalcs/layouts.

> 
> > Source/WebCore/dom/Document.cpp:1810
> > +    const Seconds styleRecalcDelay = 50_ms;
> 
> Would be nice to see all the throttling-related code in one place some day.
> 
> > Source/WebCore/page/FrameView.cpp:4415
> > +        auto hasPendingScriptExecution = frame().document()->scriptRunner() && frame().document()->scriptRunner()->hasPendingScripts();
> > +        if (auto* documentLoader = frame().loader().documentLoader()) {
> > +            auto& resourceLoader = documentLoader->cachedResourceLoader();
> > +            if (!resourceLoader.requestCount())
> > +                return hasPendingScriptExecution;
> > +
> > +            auto& resources = resourceLoader.allCachedResources();
> > +            for (auto& resource : resources) {
> > +                if (resource.value->isLoaded())
> > +                    continue;
> > +                if (resource.value->type() == CachedResource::Type::CSSStyleSheet || resource.value->type() == CachedResource::Type::Script || resource.value->type() == CachedResource::Type::FontResource)
> > +                    return false;
> > +            }
> > +        }
> 
> Maybe pull this into a separate function.
Done.

> 
> > Source/WebCore/page/FrameView.h:396
> > +    void incrementVisuallyNonEmptyCharacterCount(const String);
> 
> const String&
:(

> 
> > Source/WebCore/page/FrameView.h:947
> > +    auto whitespaceCheck = [](UChar character) {
> > +        return character == ' ' || character == '\t' || character == '\n';
> > +    };
> 
> Can you not share some existing code for this? What about   ?
I actually did find something similar in parsing code.

> 
> > Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.h:117
> > +    void initialDeferredPaint();
> 
> Hard to figure out what this means. Does it mean do the paint, or something
> else?
Good point. Renamed.

[Antti Koivisto]

Comment on attachment 353624 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353624&action=review

r=me. Interesting to see how the perf bots like this.

> Source/WebCore/dom/Document.cpp:1810
> +    const auto styleRecalcDelay = 50_ms;

I suppose there is some deep science here?

> Source/WebCore/dom/Document.cpp:1811
> +    m_styleRecalcTimer.startOneShot(throttleStyleRecalc ? styleRecalcDelay : 0_s);

Maybe this change could be done separately?

> Source/WebCore/rendering/RenderText.cpp:203
> +    // FIXME: Find out how to increment the visually non empty character count when the font becomes available.

Maybe this stuff needs to move to RenderText::styleDidChange

[zalan]

(In reply to Antti Koivisto from comment #11)
> Comment on attachment 353624 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353624&action=review
> 
> r=me. Interesting to see how the perf bots like this.
> 
> > Source/WebCore/dom/Document.cpp:1810
> > +    const auto styleRecalcDelay = 50_ms;
> 
> I suppose there is some deep science here?
49ms seemed too short, while 51ms felt way too long.

> 
> > Source/WebCore/dom/Document.cpp:1811
> > +    m_styleRecalcTimer.startOneShot(throttleStyleRecalc ? styleRecalcDelay : 0_s);
> 
> Maybe this change could be done separately?
perf bots would see this patch as a net regression without that change.

> 
> > Source/WebCore/rendering/RenderText.cpp:203
> > +    // FIXME: Find out how to increment the visually non empty character count when the font becomes available.
> 
> Maybe this stuff needs to move to RenderText::styleDidChange
I was thinking about that too. Will probably do it in a a followup patch.

[EWS Watchlist]

Comment on attachment 353624 [details]
Patch

Attachment 353624 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9817559

New failing tests:
tiled-drawing/simple-document-with-margin-tiles.html
compositing/tiling/non-visible-window-tile-coverage.html

[EWS Watchlist]

Created attachment 353636 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Simon Fraser (smfr)]

Also, what's the story on macOS? Do we do similar throttling?

[zalan]

(In reply to Simon Fraser (smfr) from comment #15)
> Also, what's the story on macOS? Do we do similar throttling?
Geoff and I talked about it and agreed that it would be part of a followup patch.

[zalan]

Committed r237785: <https://trac.webkit.org/changeset/237785>

[Ryan Haddad]

(In reply to Build Bot from comment #13)
> Comment on attachment 353624 [details]
> Patch
> 
> Attachment 353624 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: https://webkit-queues.webkit.org/results/9817559
> 
> New failing tests:
> tiled-drawing/simple-document-with-margin-tiles.html
> compositing/tiling/non-visible-window-tile-coverage.html

These tests are failing on the bots after <https://trac.webkit.org/changeset/237785>:
https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r237785%20(12559)/results.html

[zalan]

(In reply to Ryan Haddad from comment #18)
> (In reply to Build Bot from comment #13)
> > Comment on attachment 353624 [details]
> > Patch
> > 
> > Attachment 353624 [details] did not pass mac-wk2-ews (mac-wk2):
> > Output: https://webkit-queues.webkit.org/results/9817559
> > 
> > New failing tests:
> > tiled-drawing/simple-document-with-margin-tiles.html
> > compositing/tiling/non-visible-window-tile-coverage.html
> 
> These tests are failing on the bots after
> <https://trac.webkit.org/changeset/237785>:
> https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/
> r237785%20(12559)/results.html
It looks to me a (In reply to Ryan Haddad from comment #18)
> (In reply to Build Bot from comment #13)
> > Comment on attachment 353624 [details]
> > Patch
> > 
> > Attachment 353624 [details] did not pass mac-wk2-ews (mac-wk2):
> > Output: https://webkit-queues.webkit.org/results/9817559
> > 
> > New failing tests:
> > tiled-drawing/simple-document-with-margin-tiles.html
> > compositing/tiling/non-visible-window-tile-coverage.html
> 
> These tests are failing on the bots after
> <https://trac.webkit.org/changeset/237785>:
> https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/
> r237785%20(12559)/results.html
I assume those tests need rebaselining but not 100% sure what values represent in this context. Let me check with Tim/Simon first.

[Ryan Haddad]

Other issues after this change:

These two API tests are timing out on macOS bots

    TestWebKitAPI.WebKit.LayoutMilestonesWithAllContentInFrame
    TestWebKitAPI.WebKit.FirstVisuallyNonEmptyLayoutAfterPageCacheRestore

https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/12559


Layout tests are exiting early and 5 API tests are failing the following assertion on iOS Simulator Debug bots:

ASSERTION FAILED: !m_layerFlushTimer.isActive()
/Volumes/Data/slave/ios-simulator-12-debug/build/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm(290) : virtual void WebKit::RemoteLayerTreeDrawingArea::scheduleInitialDeferredPaint()

https://build.webkit.org/builders/Apple%20iOS%2012%20Simulator%20Debug%20WK2%20(Tests)/builds/637

[Ryan Haddad]

Reverted r237785 for reason:

Introduced layout test and API test failures on macOS and iOS.

Committed r237813: <https://trac.webkit.org/changeset/237813>

[Simon Fraser (smfr)]

(In reply to zalan from comment #19)
> (In reply to Ryan Haddad from comment #18)
> > 
> > These tests are failing on the bots after
> > <https://trac.webkit.org/changeset/237785>:
> > https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/
> > r237785%20(12559)/results.html
> I assume those tests need rebaselining but not 100% sure what values
> represent in this context. Let me check with Tim/Simon first.

Those seem like real regressions.

[zalan]

(In reply to Ryan Haddad from comment #21)
> Reverted r237785 for reason:
> 
> Introduced layout test and API test failures on macOS and iOS.
> 
> Committed r237813: <https://trac.webkit.org/changeset/237813>
Both the API and the iOS failures are addressed now and I can't repro the mac-wk2 only failures locally. :(

[zalan]

Created attachment 353992 [details]
Patch

[EWS Watchlist]

Attachment 353992 [details] did not pass style-queue:


ERROR: Source/WebCore/page/ChromeClient.h:336:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[zalan]

Created attachment 354380 [details]
Patch

[EWS Watchlist]

Attachment 354380 [details] did not pass style-queue:


ERROR: Source/WebCore/page/ChromeClient.h:336:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 354380 [details]
Patch

Attachment 354380 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9928487

New failing tests:
http/wpt/css/css-animations/start-animation-001.html

[EWS Watchlist]

Created attachment 354386 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[zalan]

Created attachment 354394 [details]
Patch

[EWS Watchlist]

Attachment 354394 [details] did not pass style-queue:


ERROR: Source/WebCore/page/ChromeClient.h:336:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 354394 [details]
Patch

Attachment 354394 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9932804

New failing tests:
accessibility/ios-simulator/form-control-validation-message.html

[EWS Watchlist]

Created attachment 354430 [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.6

[zalan]

Created attachment 354441 [details]
Patch

[EWS Watchlist]

Attachment 354441 [details] did not pass style-queue:


ERROR: Source/WebCore/page/ChromeClient.h:336:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 354441 [details]
Patch

Clearing flags on attachment: 354441

Committed r238064: <https://trac.webkit.org/changeset/238064>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.