Bug 97903 – Make RenderLayerCompositor::requiresCompositingForScrollableFrame scrollbars agnostic

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

[Antonio Gomes]

The way it is implemented now, ports that disable scrollbars creation at FrameView creation time, won't benefit from it.

[Antonio Gomes]

Created attachment 166272 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 166272 [details]
patch

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

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1675
> +    // FIXME: Make FrameView::parentFrameView public to simplify this.
> +    if (!view->parent() || !view->parent()->isFrameView())
> +        return false;
> +
> +    m_reevaluateCompositingAfterLayout = true;
> +
> +    return static_cast<FrameView*>(view->parent())->containsScrollableArea(view);

I don't get why this is any better, or why it makes sense to ask our parent if it contains us?

Why not just invent a way to ask about scrollability that doesn't involve scrollbars?

[Antonio Gomes]

> > +    return static_cast<FrameView*>(view->parent())->containsScrollableArea(view);
> 
> I don't get why this is any better, or why it makes sense to ask our parent if it contains us?

Because FrameView caches all ScrollableArea "under" it (including other child frames, block elements (e.g. scrollable divs), etc).
 
> Why not just invent a way to ask about scrollability that doesn't involve scrollbars?

The way this patch provides is a way. Would you have something in mind specifically?

[Simon Fraser (smfr)]

(In reply to comment #3)
> > > +    return static_cast<FrameView*>(view->parent())->containsScrollableArea(view);
> > 
> > I don't get why this is any better, or why it makes sense to ask our parent if it contains us?
> 
> Because FrameView caches all ScrollableArea "under" it (including other child frames, block elements (e.g. scrollable divs), etc).

Why not factor the code in FrameView::updateScrollableAreaSet() so you can just ask a FrameView if it's scrollable?

[Antonio Gomes]

> > > I don't get why this is any better, or why it makes sense to ask our parent if it contains us?
> > 
> > Because FrameView caches all ScrollableArea "under" it (including other child frames, block elements (e.g. scrollable divs), etc).
> 
> Why not factor the code in FrameView::updateScrollableAreaSet() so you can just ask a FrameView if it's scrollable?

A simple bool to FrameView? If so, we would have to do something about the main frame though, since it does not go through updateScrollableArea

[Simon Fraser (smfr)]

Do you need this to be stateful? Can't you just run the same code that updateScrollableAreaSet() does?

[Antonio Gomes]

(In reply to comment #6)
> Do you need this to be stateful? Can't you just run the same code that updateScrollableAreaSet() does?

The long term plan was:

1) fix this bug
2) remove the existing but now unneeded Settings::acceleratedCompositingForScrollableFramesEnabled
3) make requiresCompositingForScrollableFrame a criteria to promote a layer to be composited, just like requiresCompositingForXXX

So _3_ would be a rather hotter path than it is now.

That said, stateful cache would be of more value. What do you think?

[Simon Fraser (smfr)]

The code in updateScrollableAreaSet() looks pretty cheap. You should just call it each time.

[Antonio Gomes]

Created attachment 166310 [details]
patch 2

Simon, does it get closer to what you had in mind?

[Build Bot]

Comment on attachment 166310 [details]
patch 2

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

[Gyuyoung Kim]

Comment on attachment 166310 [details]
patch 2

Attachment 166310 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14070381

[WebKit Review Bot]

Comment on attachment 166310 [details]
patch 2

Attachment 166310 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14064410

[Early Warning System Bot]

Comment on attachment 166310 [details]
patch 2

Attachment 166310 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14068375

[Antonio Gomes]

Created attachment 166312 [details]
patch 2 - actual

[Simon Fraser (smfr)]

Comment on attachment 166312 [details]
patch 2 - actual

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

> Source/WebCore/page/FrameView.cpp:2794
> +bool FrameView::isScrollableChildFrameView()

I don't think this should have the "child frame" part. It's up to the caller to determine whether it's a child frame. Just do the scrolling-related tests.

> Source/WebCore/page/FrameView.cpp:2834
> +    if (!parentFrameView())
> +        return;
> +
> +    if (isScrollableChildFrameView()) {
> +        parentFrameView()->addScrollableArea(this);
>          return;
>      }
>  
> -    parentFrameView()->addScrollableArea(this);
> +    parentFrameView()->removeScrollableArea(this);

This fetches parentFrameView 3 times, and isScrollableChildFrameView() does so once. You should use a local.

[Antonio Gomes]

(In reply to comment #15)
> (From update of attachment 166312 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166312&action=review
> 
> > Source/WebCore/page/FrameView.cpp:2794
> > +bool FrameView::isScrollableChildFrameView()
> 
> I don't think this should have the "child frame" part. It's up to the caller to determine whether it's a child frame. Just do the scrolling-related tests.
> 
> > Source/WebCore/page/FrameView.cpp:2834
> > +    if (!parentFrameView())
> > +        return;
> > +
> > +    if (isScrollableChildFrameView()) {
> > +        parentFrameView()->addScrollableArea(this);
> >          return;
> >      }
> >  
> > -    parentFrameView()->addScrollableArea(this);
> > +    parentFrameView()->removeScrollableArea(this);
> 
> This fetches parentFrameView 3 times, and isScrollableChildFrameView() does so once. You should use a local.

Getting back to this bug.

[Antonio Gomes]

Created attachment 179041 [details]
patch 3 - addressed simon's comments

[WebKit Review Bot]

Comment on attachment 179041 [details]
patch 3 - addressed simon's comments

Attachment 179041 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15282533

[Early Warning System Bot]

Comment on attachment 179041 [details]
patch 3 - addressed simon's comments

Attachment 179041 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15281531

[Early Warning System Bot]

Comment on attachment 179041 [details]
patch 3 - addressed simon's comments

Attachment 179041 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15296150

[Antonio Gomes]

Created attachment 179049 [details]
(landed r138183, r=smfr) patch 3 (actual) - addressed simon's comments

[Antonio Gomes]

Kindly ping reviewer.

[Antonio Gomes]

(In reply to comment #22)
> Kindly ping reviewers.

Added some potential reviewers.

[Simon Fraser (smfr)]

Comment on attachment 179049 [details]
(landed r138183, r=smfr) patch 3 (actual) - addressed simon's comments

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

> Source/WebCore/page/FrameView.h:313
> +    bool isScrollable();

This should be const.

[Antonio Gomes]

(In reply to comment #24)
> (From update of attachment 179049 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179049&action=review
> 
> > Source/WebCore/page/FrameView.h:313
> > +    bool isScrollable();
> 
> This should be const.

Thanks Simon. Since FrameView::isScrollable calls calculateScrollbarModesForLayout, which is non-const, it can not be const. Maybe we can name it differently?

[Simon Fraser (smfr)]

No, you can leave it as is.

[Antonio Gomes]

https://trac.webkit.org/changeset/138183