Bug 176277 - Follow up FrameView::updateLayoutAndStyleIfNeededRecursive changes with related improvements
Summary: Follow up FrameView::updateLayoutAndStyleIfNeededRecursive changes with relat...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-09-02 11:25 PDT by Darin Adler
Modified: 2017-09-27 12:40 PDT (History)
12 users (show)

See Also:


Attachments
Patch (9.56 KB, patch)
2017-09-02 11:34 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (1.40 MB, application/zip)
2017-09-02 12:27 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-elcapitan (316.94 KB, application/zip)
2017-09-02 12:35 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1.59 MB, application/zip)
2017-09-02 12:53 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (1.46 MB, application/zip)
2017-09-02 13:18 PDT, Build Bot
no flags Details
Patch (9.59 KB, patch)
2017-09-03 15:36 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (3.00 MB, application/zip)
2017-09-03 16:20 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-elcapitan (348.23 KB, application/zip)
2017-09-03 16:34 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.86 MB, application/zip)
2017-09-03 16:35 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (11.80 MB, application/zip)
2017-09-03 17:33 PDT, Build Bot
no flags Details
Patch (9.48 KB, patch)
2017-09-03 20:48 PDT, Darin Adler
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2017-09-02 11:25:46 PDT
Follow-up FrameView::updateLayoutAndStyleIfNeededRecursive changes with related improvements
Comment 1 Darin Adler 2017-09-02 11:34:02 PDT Comment hidden (obsolete)
Comment 2 Build Bot 2017-09-02 12:27:31 PDT Comment hidden (obsolete)
Comment 3 Build Bot 2017-09-02 12:27:33 PDT Comment hidden (obsolete)
Comment 4 Build Bot 2017-09-02 12:35:55 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2017-09-02 12:35:56 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2017-09-02 12:53:13 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2017-09-02 12:53:15 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2017-09-02 13:18:42 PDT Comment hidden (obsolete)
Comment 9 Build Bot 2017-09-02 13:18:44 PDT Comment hidden (obsolete)
Comment 10 Darin Adler 2017-09-03 15:36:09 PDT Comment hidden (obsolete)
Comment 11 Build Bot 2017-09-03 16:19:59 PDT Comment hidden (obsolete)
Comment 12 Build Bot 2017-09-03 16:20:00 PDT Comment hidden (obsolete)
Comment 13 Build Bot 2017-09-03 16:34:37 PDT Comment hidden (obsolete)
Comment 14 Build Bot 2017-09-03 16:34:38 PDT Comment hidden (obsolete)
Comment 15 Build Bot 2017-09-03 16:35:08 PDT Comment hidden (obsolete)
Comment 16 Build Bot 2017-09-03 16:35:10 PDT Comment hidden (obsolete)
Comment 17 Build Bot 2017-09-03 17:33:20 PDT Comment hidden (obsolete)
Comment 18 Build Bot 2017-09-03 17:33:22 PDT Comment hidden (obsolete)
Comment 19 Darin Adler 2017-09-03 20:48:13 PDT
Created attachment 319832 [details]
Patch
Comment 20 Darin Adler 2017-09-03 22:24:11 PDT
Passing all EWS now. Ready for someone to review.
Comment 21 Antti Koivisto 2017-09-04 12:30:31 PDT
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.
Comment 22 Darin Adler 2017-09-04 16:47:50 PDT
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.
Comment 23 Darin Adler 2017-09-04 16:50:08 PDT
Committed r221596: <http://trac.webkit.org/changeset/221596>
Comment 24 Radar WebKit Bug Importer 2017-09-27 12:40:52 PDT
<rdar://problem/34693741>