Bug 176277

Summary: Follow up FrameView::updateLayoutAndStyleIfNeededRecursive changes with related improvements
Product: WebKit Reporter: Darin Adler <darin>
Component: Layout and RenderingAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, buildbot, cdumez, dbates, esprehn+autocc, kangil.han, kling, koivisto, rniwa, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews103 for mac-elcapitan
none
Archive of layout-test-results from ews115 for mac-elcapitan
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews102 for mac-elcapitan
none
Archive of layout-test-results from ews114 for mac-elcapitan
none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Patch koivisto: review+

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>