Don't call into ScrollView from Scrollbar.

Don't call into ScrollView from Scrollbar.

This will be a layering violation when ScrollView code moves into Source/core.

BUG=417774
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183316

[skobes, 2014-10-01 02:46:29]

Patchset #1 (id:1) has been deleted

[skobes, 2014-10-01 02:49:09]

skobes@chromium.org changed reviewers:
+ vollick@chromium.org

[Ian Vollick, 2014-10-04 01:25:45]

https://codereview.chromium.org/621653002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerScrollableArea.cpp (right):

https://codereview.chromium.org/621653002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerScrollableArea.cpp:883:
toScrollView(scrollbar->parent())->removeChild(scrollbar.get());
The fact that we were in RenderLayerScrollableArea but the scrollbar's parent is
a ScrollView kinda blew my mind a bit. It took a bit too long, but I think I
understand it. Please let me know if I've got this right.

First, this matches the code in removeToParent (always a good sign), but we know
that it's safe to call toScrollView here because at the end of
RenderLayerScrollableArea::createScrollbar, we add newly created scrollbar
widget as a child of box().document().view() which is a FrameView and therefore
a non-NULL ScrollView?

https://codereview.chromium.org/621653002/diff/20001/Source/platform/scroll/S...
File Source/platform/scroll/ScrollView.cpp (right):

https://codereview.chromium.org/621653002/diff/20001/Source/platform/scroll/S...
Source/platform/scroll/ScrollView.cpp:78: if
(m_horizontalScrollbar->overlapsResizer())
It's not obvious to me how we'll know that
ScrollView::adjustScrollbarRectForResizer will be called before we get here
ensuring that overlapsResizer is up-to-date. Can you please add a comment?

https://codereview.chromium.org/621653002/diff/20001/Source/platform/scroll/S...
File Source/platform/scroll/Scrollbar.cpp (left):

https://codereview.chromium.org/621653002/diff/20001/Source/platform/scroll/S...
Source/platform/scroll/Scrollbar.cpp:122: return parent() &&
parent()->isFrameView() && toScrollView(parent())->isScrollViewScrollbar(this);
Wouldn't this have previously returned true for RenderLayerScrollableArea's
scrollbars, but will now return false
m_scrollableArea->isScrollViewScrollbar(this) will hit
ScrollableArea::isScrollViewScrollbar(...) which you've made to return false?

[skobes, 2014-10-06 18:18:54]

https://codereview.chromium.org/621653002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerScrollableArea.cpp (right):

https://codereview.chromium.org/621653002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerScrollableArea.cpp:883:
toScrollView(scrollbar->parent())->removeChild(scrollbar.get());
On 2014/10/04 01:25:44, vollick wrote:
> The fact that we were in RenderLayerScrollableArea but the scrollbar's parent
is
> a ScrollView kinda blew my mind a bit. It took a bit too long, but I think I
> understand it. Please let me know if I've got this right.
> 
> First, this matches the code in removeToParent (always a good sign), but we
know
> that it's safe to call toScrollView here because at the end of
> RenderLayerScrollableArea::createScrollbar, we add newly created scrollbar
> widget as a child of box().document().view() which is a FrameView and
therefore
> a non-NULL ScrollView?

Yes, exactly right. :)

https://codereview.chromium.org/621653002/diff/20001/Source/platform/scroll/S...
File Source/platform/scroll/ScrollView.cpp (right):

https://codereview.chromium.org/621653002/diff/20001/Source/platform/scroll/S...
Source/platform/scroll/ScrollView.cpp:78: if
(m_horizontalScrollbar->overlapsResizer())
On 2014/10/04 01:25:44, vollick wrote:
> It's not obvious to me how we'll know that
> ScrollView::adjustScrollbarRectForResizer will be called before we get here
> ensuring that overlapsResizer is up-to-date. Can you please add a comment?

Added a comment.  Note that if adjustScrollbarRectForResizer hasn't been called,
then overlapsResizer() returns the default value of false.

https://codereview.chromium.org/621653002/diff/20001/Source/platform/scroll/S...
File Source/platform/scroll/Scrollbar.cpp (left):

https://codereview.chromium.org/621653002/diff/20001/Source/platform/scroll/S...
Source/platform/scroll/Scrollbar.cpp:122: return parent() &&
parent()->isFrameView() && toScrollView(parent())->isScrollViewScrollbar(this);
On 2014/10/04 01:25:45, vollick wrote:
> Wouldn't this have previously returned true for RenderLayerScrollableArea's
> scrollbars, but will now return false
> m_scrollableArea->isScrollViewScrollbar(this) will hit
> ScrollableArea::isScrollViewScrollbar(...) which you've made to return false?

No, ScrollView::isScrollViewScrollbar returns false if you pass it a scrollbar
that belongs to a RenderLayerScrollableArea.

[Ian Vollick, 2014-10-06 18:50:50]

On 2014/10/06 at 18:18:54, skobes wrote:
>
https://codereview.chromium.org/621653002/diff/20001/Source/core/rendering/Re...
> File Source/core/rendering/RenderLayerScrollableArea.cpp (right):
> 
>
https://codereview.chromium.org/621653002/diff/20001/Source/core/rendering/Re...
> Source/core/rendering/RenderLayerScrollableArea.cpp:883:
toScrollView(scrollbar->parent())->removeChild(scrollbar.get());
> On 2014/10/04 01:25:44, vollick wrote:
> > The fact that we were in RenderLayerScrollableArea but the scrollbar's
parent is
> > a ScrollView kinda blew my mind a bit. It took a bit too long, but I think I
> > understand it. Please let me know if I've got this right.
> > 
> > First, this matches the code in removeToParent (always a good sign), but we
know
> > that it's safe to call toScrollView here because at the end of
> > RenderLayerScrollableArea::createScrollbar, we add newly created scrollbar
> > widget as a child of box().document().view() which is a FrameView and
therefore
> > a non-NULL ScrollView?
> 
> Yes, exactly right. :)
> 
>
https://codereview.chromium.org/621653002/diff/20001/Source/platform/scroll/S...
> File Source/platform/scroll/ScrollView.cpp (right):
> 
>
https://codereview.chromium.org/621653002/diff/20001/Source/platform/scroll/S...
> Source/platform/scroll/ScrollView.cpp:78: if
(m_horizontalScrollbar->overlapsResizer())
> On 2014/10/04 01:25:44, vollick wrote:
> > It's not obvious to me how we'll know that
> > ScrollView::adjustScrollbarRectForResizer will be called before we get here
> > ensuring that overlapsResizer is up-to-date. Can you please add a comment?
> 
> Added a comment.  Note that if adjustScrollbarRectForResizer hasn't been
called, then overlapsResizer() returns the default value of false.
> 
>
https://codereview.chromium.org/621653002/diff/20001/Source/platform/scroll/S...
> File Source/platform/scroll/Scrollbar.cpp (left):
> 
>
https://codereview.chromium.org/621653002/diff/20001/Source/platform/scroll/S...
> Source/platform/scroll/Scrollbar.cpp:122: return parent() &&
parent()->isFrameView() && toScrollView(parent())->isScrollViewScrollbar(this);
> On 2014/10/04 01:25:45, vollick wrote:
> > Wouldn't this have previously returned true for RenderLayerScrollableArea's
> > scrollbars, but will now return false
> > m_scrollableArea->isScrollViewScrollbar(this) will hit
> > ScrollableArea::isScrollViewScrollbar(...) which you've made to return
false?
> 
> No, ScrollView::isScrollViewScrollbar returns false if you pass it a scrollbar
that belongs to a RenderLayerScrollableArea.

Great, thank you for the informative replies. This refactor lgtm.

[skobes, 2014-10-06 18:58:12]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2014-10-06 18:58:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/621653002/40001

[commit-bot: I haz the power, 2014-10-06 22:32:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-06 22:32:48]

Try jobs failed on following builders:
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)

[skobes, 2014-10-06 22:46:09]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2014-10-06 22:48:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/621653002/40001

[commit-bot: I haz the power, 2014-10-06 23:03:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-06 23:03:51]

Try jobs failed on following builders:
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)

[skobes, 2014-10-06 23:12:22]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2014-10-06 23:13:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/621653002/40001

[commit-bot: I haz the power, 2014-10-06 23:26:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-06 23:26:13]

Try jobs failed on following builders:
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)

[skobes, 2014-10-06 23:28:24]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2014-10-06 23:29:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/621653002/40001

[commit-bot: I haz the power, 2014-10-06 23:41:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-06 23:41:58]

Try jobs failed on following builders:
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)

[skobes, 2014-10-07 05:34:32]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2014-10-07 05:35:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/621653002/40001

[commit-bot: I haz the power, 2014-10-07 08:41:13]

Committed patchset #2 (id:40001) as 183316