Bug 97903 - Make RenderLayerCompositor::requiresCompositingForScrollableFrame scrollbars agnostic
Summary: Make RenderLayerCompositor::requiresCompositingForScrollableFrame scrollbars ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antonio Gomes
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-28 06:19 PDT by Antonio Gomes
Modified: 2012-12-19 11:32 PST (History)
12 users (show)

See Also:


Attachments
patch (3.98 KB, patch)
2012-09-28 10:17 PDT, Antonio Gomes
simon.fraser: review-
Details | Formatted Diff | Diff
patch 2 (7.26 KB, patch)
2012-09-28 14:21 PDT, Antonio Gomes
buildbot: commit-queue-
Details | Formatted Diff | Diff
patch 2 - actual (7.26 KB, patch)
2012-09-28 14:37 PDT, Antonio Gomes
simon.fraser: review-
Details | Formatted Diff | Diff
patch 3 - addressed simon's comments (7.19 KB, patch)
2012-12-12 07:44 PST, Antonio Gomes
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
(landed r138183, r=smfr) patch 3 (actual) - addressed simon's comments (7.41 KB, patch)
2012-12-12 08:18 PST, Antonio Gomes
simon.fraser: review+
simon.fraser: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 2012-09-28 06:19:38 PDT
The way it is implemented now, ports that disable scrollbars creation at FrameView creation time, won't benefit from it.
Comment 1 Antonio Gomes 2012-09-28 10:17:42 PDT
Created attachment 166272 [details]
patch
Comment 2 Simon Fraser (smfr) 2012-09-28 10:49:54 PDT
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?
Comment 3 Antonio Gomes 2012-09-28 10:54:45 PDT
> > +    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?
Comment 4 Simon Fraser (smfr) 2012-09-28 11:01:07 PDT
(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?
Comment 5 Antonio Gomes 2012-09-28 11:14:48 PDT
> > > 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
Comment 6 Simon Fraser (smfr) 2012-09-28 11:38:15 PDT
Do you need this to be stateful? Can't you just run the same code that updateScrollableAreaSet() does?
Comment 7 Antonio Gomes 2012-09-28 12:11:48 PDT
(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?
Comment 8 Simon Fraser (smfr) 2012-09-28 12:41:08 PDT
The code in updateScrollableAreaSet() looks pretty cheap. You should just call it each time.
Comment 9 Antonio Gomes 2012-09-28 14:21:52 PDT
Created attachment 166310 [details]
patch 2

Simon, does it get closer to what you had in mind?
Comment 10 Build Bot 2012-09-28 14:30:13 PDT
Comment on attachment 166310 [details]
patch 2

Attachment 166310 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14056550
Comment 11 Gyuyoung Kim 2012-09-28 14:30:46 PDT
Comment on attachment 166310 [details]
patch 2

Attachment 166310 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14070381
Comment 12 WebKit Review Bot 2012-09-28 14:33:14 PDT
Comment on attachment 166310 [details]
patch 2

Attachment 166310 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14064410
Comment 13 Early Warning System Bot 2012-09-28 14:35:56 PDT
Comment on attachment 166310 [details]
patch 2

Attachment 166310 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14068375
Comment 14 Antonio Gomes 2012-09-28 14:37:19 PDT
Created attachment 166312 [details]
patch 2 - actual
Comment 15 Simon Fraser (smfr) 2012-09-28 14:43:32 PDT
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.
Comment 16 Antonio Gomes 2012-12-07 07:34:01 PST
(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.
Comment 17 Antonio Gomes 2012-12-12 07:44:39 PST
Created attachment 179041 [details]
patch 3 - addressed simon's comments
Comment 18 WebKit Review Bot 2012-12-12 08:00:17 PST
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
Comment 19 Early Warning System Bot 2012-12-12 08:00:45 PST
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
Comment 20 Early Warning System Bot 2012-12-12 08:08:49 PST
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
Comment 21 Antonio Gomes 2012-12-12 08:18:57 PST
Created attachment 179049 [details]
(landed r138183, r=smfr) patch 3 (actual) - addressed simon's comments
Comment 22 Antonio Gomes 2012-12-18 14:21:39 PST
Kindly ping reviewer.
Comment 23 Antonio Gomes 2012-12-19 03:56:05 PST
(In reply to comment #22)
> Kindly ping reviewers.

Added some potential reviewers.
Comment 24 Simon Fraser (smfr) 2012-12-19 08:59:58 PST
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.
Comment 25 Antonio Gomes 2012-12-19 09:12:10 PST
(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?
Comment 26 Simon Fraser (smfr) 2012-12-19 09:55:38 PST
No, you can leave it as is.
Comment 27 Antonio Gomes 2012-12-19 11:32:50 PST
https://trac.webkit.org/changeset/138183