Ensure scrolling causes updateWidgetPositions even on nested frames.

Ensure scrolling causes updateWidgetPositions even on nested frames.

Previously, updateWidgetPositionsIfNeeded() (ie, the deferred call
caused by scroll updates) was only called on the top-level frame,
causing incorrectly positioned widgets in cases of multiple nested
frames and scrollers.

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

[hartmanng, 2014-11-12 19:53:05]

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

[hartmanng, 2014-11-12 19:53:05]

Ian, please take a look.

In my previous CL (https://codereview.chromium.org/686973002), I deferred the
call to updateWidgetPositions() to happen inside
FrameView::updateLayoutAndStyleForPainting(), if needed. It turns out
updateLayoutAndStyleForPainting() is only called on top-level frames.

The fix is pretty simple - we just move the deferred call to
updateWidgetPositions() into FrameView::updateLayoutAndStyleIfNeededRecursive(),
which recursively calls down into subframes.

The layout test is a bit more involved, let me know if it's unclear.

[Ian Vollick, 2014-11-12 20:07:26]

On 2014/11/12 19:53:05, hartmanng wrote:
> Ian, please take a look.
> 
> In my previous CL (https://codereview.chromium.org/686973002), I deferred the
> call to updateWidgetPositions() to happen inside
> FrameView::updateLayoutAndStyleForPainting(), if needed. It turns out
> updateLayoutAndStyleForPainting() is only called on top-level frames.
> 
> The fix is pretty simple - we just move the deferred call to
> updateWidgetPositions() into
FrameView::updateLayoutAndStyleIfNeededRecursive(),
> which recursively calls down into subframes.
> 
> The layout test is a bit more involved, let me know if it's unclear.

Code change looks good and the test seems reasonable, but can the expectations
for at least linux be added to the cl?

[hartmanng, 2014-11-12 20:17:49]

On 2014/11/12 20:07:26, vollick wrote:
> On 2014/11/12 19:53:05, hartmanng wrote:
> > Ian, please take a look.
> > 
> > In my previous CL (https://codereview.chromium.org/686973002), I deferred
the
> > call to updateWidgetPositions() to happen inside
> > FrameView::updateLayoutAndStyleForPainting(), if needed. It turns out
> > updateLayoutAndStyleForPainting() is only called on top-level frames.
> > 
> > The fix is pretty simple - we just move the deferred call to
> > updateWidgetPositions() into
> FrameView::updateLayoutAndStyleIfNeededRecursive(),
> > which recursively calls down into subframes.
> > 
> > The layout test is a bit more involved, let me know if it's unclear.
> 
> Code change looks good and the test seems reasonable, but can the expectations
> for at least linux be added to the cl?

Done. Also, for reference, here's what it looks like without the change in
FrameView.cpp

pixel result:
https://drive.google.com/a/chromium.org/file/d/0B9W7ar1zSvWFY0htaFI4aEo3OGc/v...
text diff:
https://docs.google.com/a/chromium.org/document/d/1KvLB_9a6VNoezPBFWf0l2yLvYF...

[Ian Vollick, 2014-11-12 20:49:40]

Thank you, that was very helpful. lgtm w/ nit.

https://codereview.chromium.org/719813002/diff/80001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/719813002/diff/80001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:186: Bug(hartmanng)
compositing/overflow/update-widget-positions-on-nested-frames-and-scrollers.html
[ NeedsRebaseline ]
nit: [ Win Mac Android ]

[hartmanng, 2014-11-12 21:15:52]

Patchset #6 (id:100001) has been deleted

[hartmanng, 2014-11-12 21:17:23]

thanks!

https://codereview.chromium.org/719813002/diff/80001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/719813002/diff/80001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:186: Bug(hartmanng)
compositing/overflow/update-widget-positions-on-nested-frames-and-scrollers.html
[ NeedsRebaseline ]
On 2014/11/12 20:49:40, vollick wrote:
> nit: [ Win Mac Android ]

Done.

[hartmanng, 2014-11-12 21:17:30]

The CQ bit was checked by hartmanng@chromium.org

[commit-bot: I haz the power, 2014-11-12 21:18:08]

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

[commit-bot: I haz the power, 2014-11-12 22:43:20]

Committed patchset #6 (id:120001) as 185252