[webkit-reviews] review granted: [Bug 176277] Follow up FrameView::updateLayoutAndStyleIfNeededRecursive changes with related improvements : [Attachment 319832] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Sep 4 12:30:31 PDT 2017


Antti Koivisto <koivisto at iki.fi> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 176277: Follow up FrameView::updateLayoutAndStyleIfNeededRecursive changes
with related improvements
https://bugs.webkit.org/show_bug.cgi?id=176277

Attachment 319832: Patch

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




--- Comment #21 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 319832
  --> https://bugs.webkit.org/attachment.cgi?id=319832
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.


More information about the webkit-reviews mailing list