Bug 176277 – Follow up FrameView::updateLayoutAndStyleIfNeededRecursive changes with related improvements

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Darin Adler]

Follow-up FrameView::updateLayoutAndStyleIfNeededRecursive changes with related improvements

[Darin Adler]

Created attachment 319723 [details]
Patch

[Build Bot]

Comment on attachment 319723 [details]
Patch

Attachment 319723 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4436028

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 319728 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

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

[Build Bot]

Comment on attachment 319723 [details]
Patch

Attachment 319723 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4436010

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 319730 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 319723 [details]
Patch

Attachment 319723 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4436122

New failing tests:
pageoverlay/overlay-small-frame-paints.html
fast/events/mouse-cursor-image-set.html
fast/forms/range/range-type-change-onchange.html
pageoverlay/overlay-large-document-scrolled.html
pageoverlay/overlay-installation.html
pageoverlay/overlay-large-document.html
accessibility/loading-iframe-sends-notification.html
pageoverlay/overlay-small-frame-mouse-events.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 319723 [details]
Patch

Attachment 319723 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4436155

New failing tests:
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-innerheight.html
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-width-height.html
imported/blink/animations/display-none-cancels-nested-animations.html
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-innerwidth.html
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-width.html
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-height.html
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-innerheight-innerwidth.html

[Build Bot]

Created attachment 319736 [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.12.5

[Darin Adler]

Created attachment 319812 [details]
Patch

[Build Bot]

Comment on attachment 319812 [details]
Patch

Attachment 319812 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4444209

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 319814 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

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

[Build Bot]

Comment on attachment 319812 [details]
Patch

Attachment 319812 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4444246

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 319816 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 319812 [details]
Patch

Attachment 319812 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4444215

New failing tests:
pageoverlay/overlay-small-frame-paints.html
accessibility/loading-iframe-sends-notification.html
pageoverlay/overlay-large-document-scrolled.html
pageoverlay/overlay-installation.html
compositing/updates/animation-non-composited.html
fast/images/animated-gif-no-layout.html
pageoverlay/overlay-large-document.html
pageoverlay/overlay-small-frame-mouse-events.html

[Build Bot]

Created attachment 319817 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

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

[Build Bot]

Comment on attachment 319812 [details]
Patch

Attachment 319812 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4444367

New failing tests:
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-innerheight.html
compositing/layer-creation/translate-animation-overlap.html
compositing/updates/animation-non-composited.html
scrollingcoordinator/ios/sync-layer-positions-after-scroll.html
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-width-height.html
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-innerwidth.html
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-width.html
compositing/layer-creation/scale-rotation-animation-overlap.html
compositing/layer-creation/translate-scale-animation-overlap.html
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-non-integer-height.html
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-innerheight-innerwidth.html
compositing/layer-creation/multiple-keyframes-animation-overlap.html

[Build Bot]

Created attachment 319819 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Darin Adler]

Created attachment 319832 [details]
Patch

[Darin Adler]

Passing all EWS now. Ready for someone to review.

[Antti Koivisto]

Comment on attachment 319832 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:1902
> +    bool didWork = styleScope().hasPendingUpdate();
>      styleScope().flushPendingUpdate();
>  
>      if (!needsStyleRecalc())
> -        return false;
> +        return didWork;

Does this change actually fix some problem? The main side effect of style scope update is style invalidation. If the style doesn't get invalidated (that is, the stylesheets haven't changed in a way that would affect current document) then why do we need signal that we "did work"?

If this does makes sense for some reason then an enum return value would be better than a boolean. The name of the function communicates that the boolean is specifically about style update.

> Source/WebCore/page/FrameView.cpp:4577
> +    auto nextRenderedDescendant = [this] (DescendantsDeque& descendantsDeque) -> RefPtr<FrameView> {
> +        if (descendantsDeque.isEmpty())
> +            descendantsDeque.append(*this);

Does this only happen on the first iteration? It might be cleaner if you had DescendantsDeque deque { *this } in the caller and removed the branch from the lambda.

[Darin Adler]

Comment on attachment 319832 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:1902
>> +        return didWork;
> 
> Does this change actually fix some problem? The main side effect of style scope update is style invalidation. If the style doesn't get invalidated (that is, the stylesheets haven't changed in a way that would affect current document) then why do we need signal that we "did work"?
> 
> If this does makes sense for some reason then an enum return value would be better than a boolean. The name of the function communicates that the boolean is specifically about style update.

OK, I won’t make this change.

>> Source/WebCore/page/FrameView.cpp:4577
>> +            descendantsDeque.append(*this);
> 
> Does this only happen on the first iteration? It might be cleaner if you had DescendantsDeque deque { *this } in the caller and removed the branch from the lambda.

Yes.

But it’s trickier than it sounds to change. The challenge is how to make something reusable that keeps the three loops readable.

- I won’t really be able to "remove the branch" from the lambda. If I get rid of the special case, I still need a way to tell it’s the the first time through the function, since the "append children" operation is done after processing the previous. And the first time through there is no previous. That whole thing is carefully constructed so I can use this single "next" function.

- Unfortunately, I can’t write DescendantsDeque deque { *this } because of some subtlety about what Deque constructor arguments are currently supported and the fact that these are Ref. I was able to make it work with a separate append call, but could not make it work as a constructor without something pretty long and ugly. I can keep fiddling with that. Note that whatever it is, I have to repeat it all three times.

I tried a few different ways to make a reusable loop that is easy to use both without early exit (for the main function) and with early exit (for the assertions). This was the best I was able to come up with. Something like a for loop would be more natural but I didn’t manage to come up with anything simple.

I’m happy to keep fiddling with it for clarity or readability, but I don’t think it is trivial to make it better.

[Darin Adler]

Committed r221596: <http://trac.webkit.org/changeset/221596>

[Radar WebKit Bug Importer]

<rdar://problem/34693741>