Implement coordinated scrollbar for subframes and overflow:scroll

• Component: New Bugs
• Status: RESOLVED FIXED

[Tien-Ren Chen]

Implement coordinated scrollbar for subframes and sublayers

[Tien-Ren Chen]

Created attachment 187822 [details]
Patch

[WebKit Review Bot]

Comment on attachment 187822 [details]
Patch

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

New failing tests:
compositing/iframes/scrolling-iframe.html
compositing/iframes/enter-compositing-iframe.html
compositing/iframes/iframe-resize.html
compositing/overflow/content-gains-scrollbars.html
compositing/iframes/connect-compositing-iframe-delayed.html
compositing/overflow/scrolling-content-clip-to-viewport.html
compositing/iframes/connect-compositing-iframe2.html
compositing/overflow/scrolling-without-painting.html
compositing/overflow/composited-scrolling-creates-a-stacking-container.html
compositing/iframes/overlapped-iframe.html
compositing/geometry/limit-layer-bounds-transformed-overflow.html
compositing/iframes/become-overlapped-iframe.html
compositing/repaint/newly-composited-repaint-rect.html
compositing/overflow/content-loses-scrollbars.html
compositing/iframes/connect-compositing-iframe3.html
compositing/reflections/nested-reflection-on-overflow.html
compositing/overflow/textarea-scroll-touch.html
compositing/overflow/image-load-overflow-scrollbars.html
compositing/overflow/scrollbars-with-clipped-owner.html
compositing/iframes/connect-compositing-iframe.html
compositing/overflow/scrollbar-painting.html
compositing/overflow/updating-scrolling-content.html
compositing/layer-creation/overflow-scroll-overlap.html
compositing/overflow/overflow-scrollbar-layers.html
compositing/overflow/overflow-scroll.html
compositing/iframes/composited-parent-iframe.html
compositing/iframes/invisible-nested-iframe-show.html
compositing/iframes/iframe-size-from-zero.html
compositing/iframes/resizer.html
compositing/rtl/rtl-iframe-absolute-overflow-scrolled.html

[Simon Fraser (smfr)]

Comment on attachment 187822 [details]
Patch

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

r- for abuse of setContentsToMedia.

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:125
> +    scrollbarGraphicsLayer->setContentsToMedia(scrollbarLayer->layer());

This seems like an abuse of setContentsToMedia.

> Source/WebCore/rendering/RenderLayerBacking.cpp:1015
>      if (needsHorizontalScrollbarLayer) {
>          if (!m_layerForHorizontalScrollbar) {
>              m_layerForHorizontalScrollbar = createGraphicsLayer("horizontal scrollbar");
>              layersChanged = true;
> +            if (scrollingCoordinator)
> +                scrollingCoordinator->scrollableAreaScrollbarLayerDidChange(m_owningLayer, HorizontalScrollbar);
>          }
>      } else if (m_layerForHorizontalScrollbar) {
>          m_layerForHorizontalScrollbar.clear();
>          layersChanged = true;
> +        if (scrollingCoordinator)
> +            scrollingCoordinator->scrollableAreaScrollbarLayerDidChange(m_owningLayer, HorizontalScrollbar);

I'd prefer that you change the code so that you only need to call scrollableAreaScrollbarLayerDidChange in one or two places in this function, not 4 times.

[Tien-Ren Chen]

(In reply to comment #3)
> (From update of attachment 187822 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187822&action=review
> 
> r- for abuse of setContentsToMedia.
> 
> > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:125
> > +    scrollbarGraphicsLayer->setContentsToMedia(scrollbarLayer->layer());
> 
> This seems like an abuse of setContentsToMedia.

I consider coordinated scrollbar to be a built-in plug-in provided by chromium compositor. Anyway we need to find some way to attach customized platform layer to a GraphicsLayer. What are the other choices that is less abusive?

> > Source/WebCore/rendering/RenderLayerBacking.cpp:1015
> >      if (needsHorizontalScrollbarLayer) {
> >          if (!m_layerForHorizontalScrollbar) {
> >              m_layerForHorizontalScrollbar = createGraphicsLayer("horizontal scrollbar");
> >              layersChanged = true;
> > +            if (scrollingCoordinator)
> > +                scrollingCoordinator->scrollableAreaScrollbarLayerDidChange(m_owningLayer, HorizontalScrollbar);
> >          }
> >      } else if (m_layerForHorizontalScrollbar) {
> >          m_layerForHorizontalScrollbar.clear();
> >          layersChanged = true;
> > +        if (scrollingCoordinator)
> > +            scrollingCoordinator->scrollableAreaScrollbarLayerDidChange(m_owningLayer, HorizontalScrollbar);
> 
> I'd prefer that you change the code so that you only need to call scrollableAreaScrollbarLayerDidChange in one or two places in this function, not 4 times.

Agree that looks ugly. I'll change that in next patch.

[Tien-Ren Chen]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 187822 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=187822&action=review
> > 
> > r- for abuse of setContentsToMedia.
> > 
> > > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:125
> > > +    scrollbarGraphicsLayer->setContentsToMedia(scrollbarLayer->layer());
> > 
> > This seems like an abuse of setContentsToMedia.
> 
> I consider coordinated scrollbar to be a built-in plug-in provided by chromium compositor. Anyway we need to find some way to attach customized platform layer to a GraphicsLayer. What are the other choices that is less abusive?

Per off-line discussion with aelias@, we think we should make a GraphicsLayerChromium::setContentsToCoordinatedScrollbar extension. Does that sound cleaner?

[Simon Fraser (smfr)]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 187822 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=187822&action=review
> > > 
> > > r- for abuse of setContentsToMedia.
> > > 
> > > > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:125
> > > > +    scrollbarGraphicsLayer->setContentsToMedia(scrollbarLayer->layer());
> > > 
> > > This seems like an abuse of setContentsToMedia.
> > 
> > I consider coordinated scrollbar to be a built-in plug-in provided by chromium compositor. Anyway we need to find some way to attach customized platform layer to a GraphicsLayer. What are the other choices that is less abusive?
> 
> Per off-line discussion with aelias@, we think we should make a GraphicsLayerChromium::setContentsToCoordinatedScrollbar extension. Does that sound cleaner?

You could do that, but it might be better to make a more general setContentsToCustomSomething() function that can live on GraphicsLayer.

[James Robinson]

As far as I can see, implementations of GraphicsLayer do not do anything different for setContentsToMedia(PlatformLayer*) and setContentsToCanvas(PlatformLayer*). GraphicsLayerCA and GraphicsLayerChromium set the contentsLayerPurpose, but this doesn't appear to be used for anything.  I think we should just fold them together into one function that accepts any sort of PlatformLayer.

[Tien-Ren Chen]

Created attachment 188003 [details]
Patch

[Tien-Ren Chen]

Changes from last patch:
1. Fix the crashes (scroll layer dangling pointer) -- now we create WebScrollbarLayer if and only if there is a corresponding scrollbar GraphicsLayer, but only actually attach the layer when a scroll layer is available.
2. Fix the coding style problem smfr@ mentioned
3. Change TestExpectations

(In reply to comment #7)
> As far as I can see, implementations of GraphicsLayer do not do anything different for setContentsToMedia(PlatformLayer*) and setContentsToCanvas(PlatformLayer*). GraphicsLayerCA and GraphicsLayerChromium set the contentsLayerPurpose, but this doesn't appear to be used for anything.  I think we should just fold them together into one function that accepts any sort of PlatformLayer.

I feel this patch is already too big to review. Let's do it in a separate bug: https://bugs.webkit.org/show_bug.cgi?id=109658

[Tien-Ren Chen]

(In reply to comment #9)
> Changes from last patch:
> 1. Fix the crashes (scroll layer dangling pointer)

Sorry, I mean null pointer, not dangling pointer.

[Tien-Ren Chen]

Created attachment 188255 [details]
Patch

[Tien-Ren Chen]

Changes from last patch:
* Rebaseline affected tests, update TestExpectation

[WebKit Review Bot]

Comment on attachment 188255 [details]
Patch

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

New failing tests:
platform/chromium/compositing/force-compositing-mode/overflow-iframe-enter-compositing.html

[Tien-Ren Chen]

Created attachment 188261 [details]
Patch

[Sami Kyöstilä]

Comment on attachment 188261 [details]
Patch

I think overall this is a good simplification, but I'd appreciate some test coverage if possible.

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

> Source/WebCore/ChangeLog:22
> +        No new tests. Don't have a good way to test ScrollingCoordinator at this time.

We've got ScrollingCoordinatorChromiumTest unit tests in Source/WebKit/chromium/tests/. Maybe some of the Chromium parts could be covered there?

> Source/WebCore/page/scrolling/ScrollingCoordinator.h:151
> +    virtual void scrollableAreaDestroyed(ScrollableArea*) { }

I think the role of ScrollingCoordinator is getting a bit weird here since on one hand it interrogates FrameView for the known ScrollableAreas, but here we're explicitly pushing information about the lifetimes of ScrollableArea to it. Maybe we could instead reuse scrollableAreaScrollbarLayerDidChange for this so that it would forget about the ScrollableArea if it didn't have any scrollbars? That would be similar to how scrollableAreaScrollLayerDidChange() works.

> Source/WebCore/page/scrolling/ScrollingCoordinator.h:156
>      virtual void touchEventTargetRectsDidChange(const Document*) { }

Can we get rid of the document argument here since it doesn't look like we're using it any longer?

> Source/WebCore/rendering/RenderLayerBacking.cpp:1037
> +        if (verticalScrollbarLayerChanged && scrollingCoordinator)

No need to check scrollingCoordinator again.

[Tien-Ren Chen]

(In reply to comment #15)
> (From update of attachment 188261 [details])
> I think overall this is a good simplification, but I'd appreciate some test coverage if possible.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=188261&action=review
> 
> > Source/WebCore/ChangeLog:22
> > +        No new tests. Don't have a good way to test ScrollingCoordinator at this time.
> 
> We've got ScrollingCoordinatorChromiumTest unit tests in Source/WebKit/chromium/tests/. Maybe some of the Chromium parts could be covered there?

Ah ha, I'll try that. Great hints!

> > Source/WebCore/page/scrolling/ScrollingCoordinator.h:151
> > +    virtual void scrollableAreaDestroyed(ScrollableArea*) { }
> 
> I think the role of ScrollingCoordinator is getting a bit weird here since on one hand it interrogates FrameView for the known ScrollableAreas, but here we're explicitly pushing information about the lifetimes of ScrollableArea to it. Maybe we could instead reuse scrollableAreaScrollbarLayerDidChange for this so that it would forget about the ScrollableArea if it didn't have any scrollbars? That would be similar to how scrollableAreaScrollLayerDidChange() works.

Agree, and we're calling to ScrollingCoordinator from too many different places: Document, HistoryController, FrameView, Page, RenderLayer, RenderLayerBacking, RenderLayerCompositor :(

I wish we have a cleaner way to do the lifetime management, but it is tricky because FrameView could be destructed when a scrollbar is still alive, calling scrollableAreaScrollbarLayerDidChange in RenderLayerCompositor destructor would be dangerous because the FrameView is already partially deconstructed.

Also I wanted to consolidate all scrollbar-related hook to ScrollableArea, but ScrollableArea doesn't have access to Page(thus no access to ScrollingCoordinator).

Consider the problems, that's why I made the decision to call scrollableAreaDestroyed in FrameView and RenderLayer. Any idea how to remove this clutter is welcome!

> > Source/WebCore/page/scrolling/ScrollingCoordinator.h:156
> >      virtual void touchEventTargetRectsDidChange(const Document*) { }
> 
> Can we get rid of the document argument here since it doesn't look like we're using it any longer?

Technically we can, but I don't know if we have a plan to have per-frame touch event rects. CCed leviw@ and yusufo@ for that matter.

> > Source/WebCore/rendering/RenderLayerBacking.cpp:1037
> > +        if (verticalScrollbarLayerChanged && scrollingCoordinator)
> 
> No need to check scrollingCoordinator again.

Oops, that is a copy & paste mistake. I'll rearrange that.

[Sami Kyöstilä]

(In reply to comment #16)
> Consider the problems, that's why I made the decision to call scrollableAreaDestroyed in FrameView and RenderLayer. Any idea how to remove this clutter is welcome!

Thanks for expanding on that. I don't have a straighforward solution to this in mind. Perhaps -- longer term -- some part of ScrollingCoordinator's interface should be exposed at ScrollableArea's level. I don't think this patch needs to get into that, though.

[Tien-Ren Chen]

(In reply to comment #17)
> (In reply to comment #16)
> > Consider the problems, that's why I made the decision to call scrollableAreaDestroyed in FrameView and RenderLayer. Any idea how to remove this clutter is welcome!
> 
> Thanks for expanding on that. I don't have a straighforward solution to this in mind. Perhaps -- longer term -- some part of ScrollingCoordinator's interface should be exposed at ScrollableArea's level. I don't think this patch needs to get into that, though.

I need to come up with something else in this patch though. Mac layout tests are not doing well. Very likely due to access violation during destruction.

[Tien-Ren Chen]

Created attachment 188658 [details]
Patch

[Tien-Ren Chen]

Changes from last patch:
* Added chromium unit tests
* Fix the nits Sami mentioned
* Fix crash bug due to Frame/FrameView lifetime

[Antonio Gomes]

Comment on attachment 188658 [details]
Patch

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

It looks to me that removing the ScrollCoordinatorChromiumPrivate makes the patch harder to follow.

> Source/WebCore/page/FrameView.cpp:256
> +        if (m_frame->page())
> +            if (ScrollingCoordinator* scrollingCoordinator = m_frame->page()->scrollingCoordinator())
> +                scrollingCoordinator->willDestroyScrollableArea(this);

needs wrapping {}

> Source/WebCore/page/FrameView.cpp:320
> +    // FIXME: Technically the FrameView is not destroyed yet,
> +    // but this is our last chance to access ScrollingCoordinator.
> +    if (m_frame && m_frame->page())
> +        if (ScrollingCoordinator* scrollingCoordinator = m_frame->page()->scrollingCoordinator())
> +            scrollingCoordinator->willDestroyScrollableArea(this);
> +

It looks strange to have this block here and at the dtor.

[Build Bot]

Comment on attachment 188658 [details]
Patch

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

New failing tests:
inspector/styles/stylesheet-tracking.html
media/video-controls-captions-trackmenu.html

[Sami Kyöstilä]

Comment on attachment 188658 [details]
Patch

Thanks for adding the test. I think this looks good modulo Antonio's comments.

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

> Source/WebCore/platform/ScrollableArea.h:205
> +    friend class ScrollingCoordinator;

Could you move this next to the accessor methods so the association is clearer -- like we do for ScrollAnimator below. I can't help but wonder whether we should instead make the accessors public instead, but maybe there's a good reason for keeping them protected.

[Tien-Ren Chen]

Created attachment 189163 [details]
Patch

[Tien-Ren Chen]

Change from last patch:
* Rebased
* Fixed the nits

(In reply to comment #21)
> (From update of attachment 188658 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188658&action=review
> 
> It looks to me that removing the ScrollCoordinatorChromiumPrivate makes the patch harder to follow.

Sorry about it. I understand this patch is doing too many thing at the same time.

> > Source/WebCore/page/FrameView.cpp:256
> > +        if (m_frame->page())
> > +            if (ScrollingCoordinator* scrollingCoordinator = m_frame->page()->scrollingCoordinator())
> > +                scrollingCoordinator->willDestroyScrollableArea(this);
> 
> needs wrapping {}

Done

> > Source/WebCore/page/FrameView.cpp:320
> > +    // FIXME: Technically the FrameView is not destroyed yet,
> > +    // but this is our last chance to access ScrollingCoordinator.
> > +    if (m_frame && m_frame->page())
> > +        if (ScrollingCoordinator* scrollingCoordinator = m_frame->page()->scrollingCoordinator())
> > +            scrollingCoordinator->willDestroyScrollableArea(this);
> > +
> 
> It looks strange to have this block here and at the dtor.

Done. I changed the destructor one to just call clearFrame.

(In reply to comment #22)
> (From update of attachment 188658 [details])
> Attachment 188658 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/16585802
> 
> New failing tests:
> inspector/styles/stylesheet-tracking.html
> media/video-controls-captions-trackmenu.html

Feels like a flake. Let's see if it still happens.

(In reply to comment #23)
> (From update of attachment 188658 [details])
> Thanks for adding the test. I think this looks good modulo Antonio's comments.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=188658&action=review
> 
> > Source/WebCore/platform/ScrollableArea.h:205
> > +    friend class ScrollingCoordinator;
> 
> Could you move this next to the accessor methods so the association is clearer -- like we do for ScrollAnimator below. I can't help but wonder whether we should instead make the accessors public instead, but maybe there's a good reason for keeping them protected.

Done moving the friend clause near the accessors.

I'm thinking the same thing. It feels weird that you can access the layers through frameView->compositor()->layerForXxx() or renderLayer->backing()->layerForXxx() but impossible to do so on ScrollableArea unless downcasting it first.

[James Robinson]

Comment on attachment 189163 [details]
Patch

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

This is a big patch and I'm not quite done digesting it yet, but here are some initial comments.

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:173
> +        // Root layer non-overlay scrollbars should be marked opaque to disable
> +        // blending.
> +        bool isOpaqueRootScrollbar = scrollableArea == static_cast<ScrollableArea*>(m_page->mainFrame()->view()) && !scrollbar->isOverlayScrollbar();

could you break this into two checks (one for root-ness and one for overlay)?

I'm a little worried that enabling this for other platforms (mainly windows) is going to break blending unless we take special care somewhere to fix up the alpha channel.

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.h:80
> +    class ScrollbarLayerManager {

It's useful to wrap this functionality in separate functions, but I don't think this inner class adds much additional value.  It's just making ScrollingCoordinatorChromium.cpp more obscure.  I think you should just make these member functions and variables on ScrollingCoordinatorChromium

> Source/WebCore/rendering/RenderLayer.cpp:244
> +            scrollingCoordinator->willDestroyScrollableArea(this);

hmm - is there any way to combine this logic with FrameView::removeScrollableArea?  It's proven tricky to get that mechanism correct in all corner cases (and it still crashes sometimes, so we have a bit of work to do) so I'd like to share it as much as possible.

[Tien-Ren Chen]

(In reply to comment #26)
> (From update of attachment 189163 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189163&action=review
> 
> This is a big patch and I'm not quite done digesting it yet, but here are some initial comments.

Thank you for the valuable feedback!

> > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:173
> > +        // Root layer non-overlay scrollbars should be marked opaque to disable
> > +        // blending.
> > +        bool isOpaqueRootScrollbar = scrollableArea == static_cast<ScrollableArea*>(m_page->mainFrame()->view()) && !scrollbar->isOverlayScrollbar();
> 
> could you break this into two checks (one for root-ness and one for overlay)?

Done

> I'm a little worried that enabling this for other platforms (mainly windows) is going to break blending unless we take special care somewhere to fix up the alpha channel.

I'm not aware of this problem. Could you elaborate? Thanks!
Anyway, I'm happy to set platformSupported to true only for Android.

> > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.h:80
> > +    class ScrollbarLayerManager {
> 
> It's useful to wrap this functionality in separate functions, but I don't think this inner class adds much additional value.  It's just making ScrollingCoordinatorChromium.cpp more obscure.  I think you should just make these member functions and variables on ScrollingCoordinatorChromium

This container class is to isolate GraphicsLayer contents layer registration. Unfortunately it is difficult to do RAII on individual layers so I have to do it in the container. Making it a nested class is to avoid other ScrollCoordinator method to accidentally free a layer without unregistering it first.

I understand that WebKit style discourages the use of nested classes. I'm happy to remove it if you feel the above reason is not strong enough to make an exception.

> > Source/WebCore/rendering/RenderLayer.cpp:244
> > +            scrollingCoordinator->willDestroyScrollableArea(this);
> 
> hmm - is there any way to combine this logic with FrameView::removeScrollableArea?  It's proven tricky to get that mechanism correct in all corner cases (and it still crashes sometimes, so we have a bit of work to do) so I'd like to share it as much as possible.

I managed to reproduced the crash. It happens about 1 out of 10 trials. It is due to null scrollbarGraphicsLayer pointer for setupScrollbarLayer when calling from scrollableAreaScrollLayerDidChange(). Theoretically a WebScrollbarLayer only exists when the GraphicsLayer gets created first. This can only happen if we have orphaned WebScrollbarLayer from previously destroyed FrameView. I can add a check to avoid the crash but I'm afraid of potential memory leak.

I doubt the use of FrameView::removeScrollableArea cures the problem. Do we always remove each ScrollableArea from its parent when nuking down a FrameView tree? (and is ScrollingCoordinator still accessible during that time?)

[Tien-Ren Chen]

Created attachment 189465 [details]
Patch

[Tien-Ren Chen]

Change from last patch:
* Fixed the nits jamesr@ mentioned.
* Only enable coordinated scrollbars for Android.
* Unwrapped ScrollbarLayerManager.
* Fixed crash bug due to Frame/FrameView lifetime. Now we notify ScrollingCoordinator when either Frame/FrameView is going to be detached from Page/Frame.
* CachedFrame should notify Frame and FrameView before detaching them

I'm not able to reproduce compositing/iframes/iframe-composited-scrolling.html
crash anymore. Started a stress test to run all layout tests 20 times before going home.

[Tien-Ren Chen]

(In reply to comment #29)
> * Only enable coordinated scrollbars for Android.

Ah oh this changes many tests results again. Let's wait for jamesr's opinion about what platforms should we enable coordinated scrollbars.

[James Robinson]

(In reply to comment #30)
> (In reply to comment #29)
> > * Only enable coordinated scrollbars for Android.
> 
> Ah oh this changes many tests results again. Let's wait for jamesr's opinion about what platforms should we enable coordinated scrollbars.

Leave them on for the root layer on all platforms - it's just non-root scrollbars on windows that need special attention.

[James Robinson]

Comment on attachment 189465 [details]
Patch

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

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:165
> +#if OS(ANDROID)
> +        bool platformSupported = true;
> +#else
> +        bool platformSupported = false;
> +#endif

This is way too heavy handed and will be a big performance regression on other platforms.

[James Robinson]

Comment on attachment 189465 [details]
Patch

At this point I feel pretty good about the ScrollingCoordinator* changes except for the platformSupported bit.  I'm still trying to reason my way through the Frame/FrameView/ScrollingCoordinator lifetime changes.

[Simon Fraser (smfr)]

What are the sublayers referenced in the title?

[James Robinson]

(In reply to comment #34)
> What are the sublayers referenced in the title?

Composited layers that aren't the rootmost RenderLayerCompositor's layer.

[Simon Fraser (smfr)]

So composited overflow?

[James Robinson]

(In reply to comment #36)
> So composited overflow?

That's right (more generally, any RenderLayerBacking with scrollbars that happens to be composited. It doesn't really matter why it's composited for the purposed of this bug).

[James Robinson]

Comment on attachment 189465 [details]
Patch

The *Frame* changes seem quite reasonable, but I'm curious as to why you changed CacheFrame.  Those changes look correct, but as far as I know we don't ever use CachedFrame in chromium (since we don't use the bfcache).  Did these changes have an effect on the behavior you were seeing in chromium?  Were you testing with the mac port?

[Tien-Ren Chen]

(In reply to comment #38)
> (From update of attachment 189465 [details])
> The *Frame* changes seem quite reasonable, but I'm curious as to why you changed CacheFrame.  Those changes look correct, but as far as I know we don't ever use CachedFrame in chromium (since we don't use the bfcache).  Did these changes have an effect on the behavior you were seeing in chromium?  Were you testing with the mac port?

It is a drive-by change when I grep through all places we ever detach a Frame/FrameView, and found out there are missing willDetach notification. Probably won't affect us. I'll remove those if it doesn't crash layout tests.

[James Robinson]

I wouldn't trust layout test coverage very much for bfcache + compositing.  The changes look OK, though.

[Tien-Ren Chen]

Created attachment 189644 [details]
Patch

[Tien-Ren Chen]

Changes from last patch:
* Rebased
* Rearranged platformSupported flag. Make it an early exit for unsupported platforms.

Stress test looks fine so far. Not introducing new crashes. (some crashes existed even without this patch)

[James Robinson]

Comment on attachment 189644 [details]
Patch

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

R=me but the conditionals for early out still need a bit of work.

I suspect the lifetime cleanups may fix issues like https://bugs.webkit.org/show_bug.cgi?id=108524 which would be a nice bonus.

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:157
> +    if (!(isMainFrame ? supportWebScrollbarLayerForMainFrame() : supportWebScrollbarLayerForAllLayer()))

this is too tricky to follow.  it also doesn't make much sense that we test for the main frame at all if the platform claims not to support "all layer", although I don't really know what "ForAllLayer" means.

Please expand this out.  I'd expect to see something like

if (can't support coordinated scrollbars on anything)
   return;

if (!isMainFrame && can only support coordinated scrollbars on main frame)
   return;

with better names.  I'd probably skip using functions and just have some locals bols that are initialized inside #ifs

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.h:89
> +#if OS(DARWIN)
> +    static bool supportWebScrollbarLayerForMainFrame() { return false; }
> +    static bool supportWebScrollbarLayerForAllLayer() { return false; }
> +#elif OS(WINDOWS)
> +    static bool supportWebScrollbarLayerForMainFrame() { return true; }
> +    static bool supportWebScrollbarLayerForAllLayer() { return false; }
> +#else
> +    static bool supportWebScrollbarLayerForMainFrame() { return true; }
> +    static bool supportWebScrollbarLayerForAllLayer() { return true; }
> +#endif

Don't put these in the header - the only callers are in the .cpp.  Move to cpp or just ditch completely

> Source/WebKit/chromium/tests/data/iframe-scrolling-inner.html:1
> +<html>

<!DOCTYPE html> too

[Tien-Ren Chen]

Created attachment 189654 [details]
Patch

[Tien-Ren Chen]

Comment on attachment 189654 [details]
Patch

Wait a sec, also need to change WebFrameTest expectations for different platforms.

[Tien-Ren Chen]

Created attachment 189656 [details]
Patch

[Tien-Ren Chen]

Created attachment 189663 [details]
Patch

[Tien-Ren Chen]

Changes from last patch:
* Rebaseline layout test expectations. Non-chromium and chromium-mac and chromium-win should have no changes.

[Tien-Ren Chen]

Created attachment 189828 [details]
Patch

[Tien-Ren Chen]

Comment on attachment 189828 [details]
Patch

Changes from last patch:
* Reverted CacheFrame change.

[Tien-Ren Chen]

Created attachment 189835 [details]
Patch

[James Robinson]

Do you have a bug filed for the RTL issues?

[Tien-Ren Chen]

(In reply to comment #52)
> Do you have a bug filed for the RTL issues?

http://crbug.com/175926

[Tien-Ren Chen]

Created attachment 189875 [details]
Patch

[Sam Weinig]

Comment on attachment 189875 [details]
Patch

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

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.h:82
> +    static WebKit::WebLayer* scrollingWebLayerForScrollableArea(ScrollableArea*);
> +
>      void setNonFastScrollableRegion(const Region&);
>      void setTouchEventTargetRects(const Vector<IntRect>&);
>      void setWheelEventHandlerCount(unsigned);
> -    PassOwnPtr<WebKit::WebScrollbarLayer> createScrollbarLayer(Scrollbar*, WebKit::WebLayer* scrollLayer, GraphicsLayer* scrollbarGraphicsLayer, FrameView*);
>  
> -    ScrollingCoordinatorPrivate* m_private;
> +    WebKit::WebScrollbarLayer* addWebScrollbarLayer(ScrollableArea*, ScrollbarOrientation, PassOwnPtr<WebKit::WebScrollbarLayer>);
> +    WebKit::WebScrollbarLayer* getWebScrollbarLayer(ScrollableArea*, ScrollbarOrientation);
> +    void removeWebScrollbarLayer(ScrollableArea*, ScrollbarOrientation);

I guess this issue existed already, but it seems quite wrong to use WebKit types in WebCore.

> Source/WebCore/rendering/RenderLayer.cpp:244
> +    if (renderer()->frame() && renderer()->frame()->page())
> +        if (ScrollingCoordinator* scrollingCoordinator = renderer()->frame()->page()->scrollingCoordinator())
> +            scrollingCoordinator->willDestroyScrollableArea(this);

This requires braces around the outer if-statement.

[Tien-Ren Chen]

(In reply to comment #55)
> (From update of attachment 189875 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189875&action=review
> 
> > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.h:82
> > +    static WebKit::WebLayer* scrollingWebLayerForScrollableArea(ScrollableArea*);
> > +
> >      void setNonFastScrollableRegion(const Region&);
> >      void setTouchEventTargetRects(const Vector<IntRect>&);
> >      void setWheelEventHandlerCount(unsigned);
> > -    PassOwnPtr<WebKit::WebScrollbarLayer> createScrollbarLayer(Scrollbar*, WebKit::WebLayer* scrollLayer, GraphicsLayer* scrollbarGraphicsLayer, FrameView*);
> >  
> > -    ScrollingCoordinatorPrivate* m_private;
> > +    WebKit::WebScrollbarLayer* addWebScrollbarLayer(ScrollableArea*, ScrollbarOrientation, PassOwnPtr<WebKit::WebScrollbarLayer>);
> > +    WebKit::WebScrollbarLayer* getWebScrollbarLayer(ScrollableArea*, ScrollbarOrientation);
> > +    void removeWebScrollbarLayer(ScrollableArea*, ScrollbarOrientation);
> 
> I guess this issue existed already, but it seems quite wrong to use WebKit types in WebCore.

Back in the day when the compositor was still in WebKit it was cc::ScrollbayLayerChromium.

It is not the only place we use WebKit types in WebCore though. I think it is not that uncommon to use WebKit types in platform-specific code.

> > Source/WebCore/rendering/RenderLayer.cpp:244
> > +    if (renderer()->frame() && renderer()->frame()->page())
> > +        if (ScrollingCoordinator* scrollingCoordinator = renderer()->frame()->page()->scrollingCoordinator())
> > +            scrollingCoordinator->willDestroyScrollableArea(this);
> 
> This requires braces around the outer if-statement.

Done. Thank you!

[Tien-Ren Chen]

Created attachment 190117 [details]
Patch

[WebKit Review Bot]

Comment on attachment 190117 [details]
Patch

Clearing flags on attachment: 190117

Committed r144024: <http://trac.webkit.org/changeset/144024>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.