FrameView now notifies ScrollCoorinator of changes in its scrollable area set

Update FrameView to notify the ScrollCoordinator of changes in its scrollable
area set. This ensures that compositor becomes aware of any scrollable
area changes even if they do not cause a re-layout e.g., a layer
visibility changing from 'hidden' to 'visible'.

Also added a new getter in |FrameView| for the |ScrollCoordinator| 
instance to clean up the code.



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

[majidvp, 2015-01-07 16:58:38]

majidvp@chromium.org changed reviewers:
+ rbyers@chromium.org

[majidvp, 2015-01-07 16:58:39]

+rybers@ for review.

[Rick Byers, 2015-01-07 17:20:16]

Seems reasonable to me, thanks!  We need a test though (I'm not sure offhand
what the existing tests are like for this, let me know if you want help finding
them).

cc vollick/skobes in case they have any input.  This is pretty straight forward
I think.

https://codereview.chromium.org/815363003/diff/20001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/815363003/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:296: if (scrollingCoordinator())
The preferred style in blink is generally something like this:

if (ScrollingCoordinator* scrollingCoordinator = scrollingCoordinator())
  scrollingCoordinator->willDestroyScrollableArea(this);

The extra overhead of asking for it again may be trivial, and it may be
impossible to change value from the first call to the second, but in general
it's hard to know that from the call site.

[Ian Vollick, 2015-01-07 17:58:03]

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

[Ian Vollick, 2015-01-07 17:58:04]

I agree that we need a test. Is it possible to expose the scroll gesture region
in the layer tree dump (if it's not already) and write a layout test that dumps,
scrolls, and dumps and checks that the dumps are correct. The layer tree dumps
are JSON, so that should make it easier to dig into them and check that you're
getting the right values.

https://codereview.chromium.org/815363003/diff/20001/Source/core/page/scrolli...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/815363003/diff/20001/Source/core/page/scrolli...
Source/core/page/scrolling/ScrollingCoordinator.cpp:123: // Wait until after
layout to update.
Why don't we want to set the dirty bit when we need a layout? Will a layout
force this to be updated?

[skobes, 2015-01-07 18:56:28]

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

[skobes, 2015-01-07 18:56:29]

https://codereview.chromium.org/815363003/diff/20001/Source/core/page/scrolli...
File Source/core/page/scrolling/ScrollingCoordinator.h (right):

https://codereview.chromium.org/815363003/diff/20001/Source/core/page/scrolli...
Source/core/page/scrolling/ScrollingCoordinator.h:70: void
scrollAreaSetDidChange();
"scroll area" is slightly confusing, suggest scrollableAreaSetDidChange or
scrollableAreasDidChange for consistency.

[majidvp, 2015-01-07 19:52:07]

On 2015/01/07 17:58:04, vollick wrote:
> I agree that we need a test. Is it possible to expose the scroll gesture
region
> in the layer tree dump (if it's not already) and write a layout test that
dumps,
> scrolls, and dumps and checks that the dumps are correct. The layer tree dumps
> are JSON, so that should make it easier to dig into them and check that you're
> getting the right values.
> 
>
https://codereview.chromium.org/815363003/diff/20001/Source/core/page/scrolli...
> File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):
> 
>
https://codereview.chromium.org/815363003/diff/20001/Source/core/page/scrolli...
> Source/core/page/scrolling/ScrollingCoordinator.cpp:123: // Wait until after
> layout to update.
> Why don't we want to set the dirty bit when we need a layout? Will a layout
> force this to be updated?

There is an existing method to obtain scroll gesture regions and it is exposed
via windows.internals (|Page::nonFastScrollableRects|). However as it stands it
is not very useful for testing this CL because it forces a layout and this CL is
trying to cover cases where a layout is not needed.  
It should not be too difficult to modify |Page::nonFastScrollableRects| and make
the layout optional. That will allow us to use it to create a new test. WDYT?

[majidvp, 2015-01-07 19:52:28]

https://codereview.chromium.org/815363003/diff/20001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/815363003/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:296: if (scrollingCoordinator())
On 2015/01/07 17:20:16, Rick Byers wrote:
> The preferred style in blink is generally something like this:
> 
> if (ScrollingCoordinator* scrollingCoordinator = scrollingCoordinator())
>   scrollingCoordinator->willDestroyScrollableArea(this);
> 
> The extra overhead of asking for it again may be trivial, and it may be
> impossible to change value from the first call to the second, but in general
> it's hard to know that from the call site.

I will update to match the convention.

https://codereview.chromium.org/815363003/diff/20001/Source/core/page/scrolli...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/815363003/diff/20001/Source/core/page/scrolli...
Source/core/page/scrolling/ScrollingCoordinator.cpp:123: // Wait until after
layout to update.
On 2015/01/07 17:58:03, vollick wrote:
> Why don't we want to set the dirty bit when we need a layout? Will a layout
> force this to be updated?

A layout also sets the same dirty flag forcing the same update making this one
redundant. (See |ScrollingCoordinator::notifyLayoutUpdate|)
Since a layout may update these scrollable area bounding boxes, I think it makes
sense to wait for layout completion before computing the non-fast scroll
regions.

https://codereview.chromium.org/815363003/diff/20001/Source/core/page/scrolli...
File Source/core/page/scrolling/ScrollingCoordinator.h (right):

https://codereview.chromium.org/815363003/diff/20001/Source/core/page/scrolli...
Source/core/page/scrolling/ScrollingCoordinator.h:70: void
scrollAreaSetDidChange();
On 2015/01/07 18:56:29, skobes wrote:
> "scroll area" is slightly confusing, suggest scrollableAreaSetDidChange or
> scrollableAreasDidChange for consistency.

Acknowledged.

[Ian Vollick, 2015-01-07 19:54:42]

On 2015/01/07 19:52:07, majidvp wrote:
> On 2015/01/07 17:58:04, vollick wrote:
> > I agree that we need a test. Is it possible to expose the scroll gesture
> region
> > in the layer tree dump (if it's not already) and write a layout test that
> dumps,
> > scrolls, and dumps and checks that the dumps are correct. The layer tree
dumps
> > are JSON, so that should make it easier to dig into them and check that
you're
> > getting the right values.
> > 
> >
>
https://codereview.chromium.org/815363003/diff/20001/Source/core/page/scrolli...
> > File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):
> > 
> >
>
https://codereview.chromium.org/815363003/diff/20001/Source/core/page/scrolli...
> > Source/core/page/scrolling/ScrollingCoordinator.cpp:123: // Wait until after
> > layout to update.
> > Why don't we want to set the dirty bit when we need a layout? Will a layout
> > force this to be updated?
> 
> There is an existing method to obtain scroll gesture regions and it is exposed
> via windows.internals (|Page::nonFastScrollableRects|). However as it stands
it
> is not very useful for testing this CL because it forces a layout and this CL
is
> trying to cover cases where a layout is not needed.  
> It should not be too difficult to modify |Page::nonFastScrollableRects| and
make
> the layout optional. That will allow us to use it to create a new test. WDYT?

I like that. Sounds like a good solution to me.

[Ian Vollick, 2015-01-07 19:56:23]

https://codereview.chromium.org/815363003/diff/20001/Source/core/page/scrolli...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/815363003/diff/20001/Source/core/page/scrolli...
Source/core/page/scrolling/ScrollingCoordinator.cpp:123: // Wait until after
layout to update.
On 2015/01/07 19:52:28, majidvp wrote:
> On 2015/01/07 17:58:03, vollick wrote:
> > Why don't we want to set the dirty bit when we need a layout? Will a layout
> > force this to be updated?
> 
> A layout also sets the same dirty flag forcing the same update making this one
> redundant. (See |ScrollingCoordinator::notifyLayoutUpdate|)
> Since a layout may update these scrollable area bounding boxes, I think it
makes
> sense to wait for layout completion before computing the non-fast scroll
> regions.

Ah, thanks, that makes sense. In that case, could you please update the comment
to reassure the future reader that everything's going to be fine after the
layout? :)

[majidvp, 2015-01-12 16:28:13]

On 2015/01/07 19:56:23, vollick wrote:
>
https://codereview.chromium.org/815363003/diff/20001/Source/core/page/scrolli...
> File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):
> 
>
https://codereview.chromium.org/815363003/diff/20001/Source/core/page/scrolli...
> Source/core/page/scrolling/ScrollingCoordinator.cpp:123: // Wait until after
> layout to update.
> On 2015/01/07 19:52:28, majidvp wrote:
> > On 2015/01/07 17:58:03, vollick wrote:
> > > Why don't we want to set the dirty bit when we need a layout? Will a
layout
> > > force this to be updated?
> > 
> > A layout also sets the same dirty flag forcing the same update making this
one
> > redundant. (See |ScrollingCoordinator::notifyLayoutUpdate|)
> > Since a layout may update these scrollable area bounding boxes, I think it
> makes
> > sense to wait for layout completion before computing the non-fast scroll
> > regions.
> 
> Ah, thanks, that makes sense. In that case, could you please update the
comment
> to reassure the future reader that everything's going to be fine after the
> layout? :)

PTAL. Addressed review feedback and added layout test to cover the change.

[majidvp, 2015-01-12 16:28:27]

https://codereview.chromium.org/815363003/diff/20001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/815363003/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:296: if (scrollingCoordinator())
On 2015/01/07 19:52:28, majidvp wrote:
> On 2015/01/07 17:20:16, Rick Byers wrote:
> > The preferred style in blink is generally something like this:
> > 
> > if (ScrollingCoordinator* scrollingCoordinator = scrollingCoordinator())
> >   scrollingCoordinator->willDestroyScrollableArea(this);
> > 
> > The extra overhead of asking for it again may be trivial, and it may be
> > impossible to change value from the first call to the second, but in general
> > it's hard to know that from the call site.
> 
> I will update to match the convention.

Done.

https://codereview.chromium.org/815363003/diff/20001/Source/core/page/scrolli...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/815363003/diff/20001/Source/core/page/scrolli...
Source/core/page/scrolling/ScrollingCoordinator.cpp:123: // Wait until after
layout to update.
On 2015/01/07 19:56:23, vollick wrote:
> On 2015/01/07 19:52:28, majidvp wrote:
> > On 2015/01/07 17:58:03, vollick wrote:
> > > Why don't we want to set the dirty bit when we need a layout? Will a
layout
> > > force this to be updated?
> > 
> > A layout also sets the same dirty flag forcing the same update making this
one
> > redundant. (See |ScrollingCoordinator::notifyLayoutUpdate|)
> > Since a layout may update these scrollable area bounding boxes, I think it
> makes
> > sense to wait for layout completion before computing the non-fast scroll
> > regions.
> 
> Ah, thanks, that makes sense. In that case, could you please update the
comment
> to reassure the future reader that everything's going to be fine after the
> layout? :)

Done.

https://codereview.chromium.org/815363003/diff/20001/Source/core/page/scrolli...
File Source/core/page/scrolling/ScrollingCoordinator.h (right):

https://codereview.chromium.org/815363003/diff/20001/Source/core/page/scrolli...
Source/core/page/scrolling/ScrollingCoordinator.h:70: void
scrollAreaSetDidChange();
On 2015/01/07 19:52:28, majidvp wrote:
> On 2015/01/07 18:56:29, skobes wrote:
> > "scroll area" is slightly confusing, suggest scrollableAreaSetDidChange or
> > scrollableAreasDidChange for consistency.
> 
> Acknowledged.

Done.

[Rick Byers, 2015-01-12 22:36:27]

Looks great, thanks!  Just a couple minor comments.

https://codereview.chromium.org/815363003/diff/100001/LayoutTests/scrollingco...
File LayoutTests/scrollingcoordinator/non-fast-scrollable-visibility-change.html
(right):

https://codereview.chromium.org/815363003/diff/100001/LayoutTests/scrollingco...
LayoutTests/scrollingcoordinator/non-fast-scrollable-visibility-change.html:47:
'[8, 308, 250, 250]'); // second div
where is the 308 coming from?  You don't want your test behavior to be dependent
on text metrics (since those vary between platforms).

https://codereview.chromium.org/815363003/diff/100001/LayoutTests/scrollingco...
LayoutTests/scrollingcoordinator/non-fast-scrollable-visibility-change.html:54:
// Change visibility (hidden -> visible), which should not cause any layout,
you should probably verify that no layout is necessary here (since it would be
easy to have such a bug mask a regression in your fix).  See I think you can
easily use internals.needsLayoutCount() for this - see
display-none-no-relayout.html.

https://codereview.chromium.org/815363003/diff/100001/LayoutTests/scrollingco...
LayoutTests/scrollingcoordinator/non-fast-scrollable-visibility-change.html:97:
window.cancelAnimationFrame(loop);
I don't think you need this.  RAF fires exactly once - so once you're in the
handler it won't get invoked again unless you explicitly request another frame.

https://codereview.chromium.org/815363003/diff/100001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/815363003/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:454: return p ? p->scrollingCoordinator() :
nullptr;
nit: blink style it to use '0' here, not nullptr.  See
http://www.chromium.org/blink/coding-style#TOC-Null-false-and-0

https://codereview.chromium.org/815363003/diff/100001/Source/core/page/Page.h
File Source/core/page/Page.h (right):

https://codereview.chromium.org/815363003/diff/100001/Source/core/page/Page.h...
Source/core/page/Page.h:56: class GraphicsLayer;
nit: you don't need this here anymore, right?

[majidvp, 2015-01-13 16:01:57]

https://codereview.chromium.org/815363003/diff/100001/LayoutTests/scrollingco...
File LayoutTests/scrollingcoordinator/non-fast-scrollable-visibility-change.html
(right):

https://codereview.chromium.org/815363003/diff/100001/LayoutTests/scrollingco...
LayoutTests/scrollingcoordinator/non-fast-scrollable-visibility-change.html:47:
'[8, 308, 250, 250]'); // second div
On 2015/01/12 22:36:27, Rick Byers wrote:
> where is the 308 coming from?  You don't want your test behavior to be
dependent
> on text metrics (since those vary between platforms).

These values are not dependent on text metrics. The confusing 8px is coming
from default body margin which the test now sets to 0. Test divs dimensions
(250x250) and vertical margin between them (50px) explain 300 used here.

https://codereview.chromium.org/815363003/diff/100001/LayoutTests/scrollingco...
LayoutTests/scrollingcoordinator/non-fast-scrollable-visibility-change.html:54:
// Change visibility (hidden -> visible), which should not cause any layout,
On 2015/01/12 22:36:27, Rick Byers wrote:
> you should probably verify that no layout is necessary here (since it would be
> easy to have such a bug mask a regression in your fix).  See I think you can
> easily use internals.needsLayoutCount() for this - see
> display-none-no-relayout.html.

Done.

https://codereview.chromium.org/815363003/diff/100001/LayoutTests/scrollingco...
LayoutTests/scrollingcoordinator/non-fast-scrollable-visibility-change.html:97:
window.cancelAnimationFrame(loop);
On 2015/01/12 22:36:27, Rick Byers wrote:
> I don't think you need this.  RAF fires exactly once - so once you're in the
> handler it won't get invoked again unless you explicitly request another
frame.

Done.

https://codereview.chromium.org/815363003/diff/100001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/815363003/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:454: return p ? p->scrollingCoordinator() :
nullptr;
On 2015/01/12 22:36:27, Rick Byers wrote:
> nit: blink style it to use '0' here, not nullptr.  See
> http://www.chromium.org/blink/coding-style#TOC-Null-false-and-0

Done.

https://codereview.chromium.org/815363003/diff/100001/Source/core/page/Page.h
File Source/core/page/Page.h (right):

https://codereview.chromium.org/815363003/diff/100001/Source/core/page/Page.h...
Source/core/page/Page.h:56: class GraphicsLayer;
On 2015/01/12 22:36:27, Rick Byers wrote:
> nit: you don't need this here anymore, right?

Done.

[Rick Byers, 2015-01-13 20:12:43]

LGTM with nit

https://codereview.chromium.org/815363003/diff/120001/LayoutTests/scrollingco...
File LayoutTests/scrollingcoordinator/non-fast-scrollable-visibility-change.html
(right):

https://codereview.chromium.org/815363003/diff/120001/LayoutTests/scrollingco...
LayoutTests/scrollingcoordinator/non-fast-scrollable-visibility-change.html:96:
// Waits for one RAF call to ensure compositing update is occurred and invokes
task.
nit: s/is/has/

[majidvp, 2015-01-13 20:40:03]

The CQ bit was checked by majidvp@chromium.org

[majidvp, 2015-01-13 20:40:09]

https://codereview.chromium.org/815363003/diff/120001/LayoutTests/scrollingco...
File LayoutTests/scrollingcoordinator/non-fast-scrollable-visibility-change.html
(right):

https://codereview.chromium.org/815363003/diff/120001/LayoutTests/scrollingco...
LayoutTests/scrollingcoordinator/non-fast-scrollable-visibility-change.html:96:
// Waits for one RAF call to ensure compositing update is occurred and invokes
task.
On 2015/01/13 20:12:43, Rick Byers wrote:
> nit: s/is/has/

Done.

[commit-bot: I haz the power, 2015-01-13 20:41:05]

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

[commit-bot: I haz the power, 2015-01-13 22:24:19]

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

[commit-bot: I haz the power, 2015-01-13 22:24:20]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/46031)

[majidvp, 2015-01-14 16:02:21]

The CQ bit was checked by majidvp@chromium.org

[commit-bot: I haz the power, 2015-01-14 16:02:49]

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

[commit-bot: I haz the power, 2015-01-14 16:03:38]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188391

[Daniel Bratell, 2015-01-22 13:55:07]

Could this have caused the crash in
https://code.google.com/p/chromium/issues/detail?id=450279 ?