Moved RootFrameViewport creation to happen after document attachment

Moved RootFrameViewport creation to happen after document attachment

I previously placed this to happen just before the first layout but that was
overly conservative; we really just need the document to be attached. This will
allow constructing a ViewportScrollCallback for a FrameView in just one place
for both root and nested frames. This will be needed in the setRootScroller
proposal in future patches.

BUG=505516
Committed: https://crrev.com/3efe8736a302182bff2e9101044fd8e15668bbb6
Cr-Commit-Position: refs/heads/master@{#403973}

[bokan, 2016-07-05 18:44:46]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-05 18:44:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-05 18:58:19]

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

[commit-bot: I haz the power, 2016-07-05 18:58:20]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[bokan, 2016-07-05 19:06:10]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-05 19:06:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-05 19:33:40]

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

[commit-bot: I haz the power, 2016-07-05 19:33:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bokan, 2016-07-05 21:00:35]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-05 21:00:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-05 22:31:42]

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

[commit-bot: I haz the power, 2016-07-05 22:31:43]

Dry run: This issue passed the CQ dry run.

[bokan, 2016-07-06 17:51:07]

Description was changed from

==========
Moved RootFrameViewport creation to happen after document attachment

I previously placed this to happen just before the first layout but that was
overly conservative; we really just need the document to be attached. This will
make it easier to perform actions once we know the FrameView has initialized its
ScrollableArea which will be needed for the setRootScroller proposal.

BUG=505516
==========

to

==========
Moved RootFrameViewport creation to happen after document attachment

I previously placed this to happen just before the first layout but that was
overly conservative; we really just need the document to be attached. This will
make it easier to perform actions once we know the FrameView has initialized its
ScrollableArea which will be needed for the setRootScroller proposal.

BUG=505516
==========

[bokan, 2016-07-06 17:51:07]

bokan@chromium.org changed reviewers:
+ tdresser@chromium.org

[bokan, 2016-07-06 17:53:09]

Tim, ptal. This is the first in a series of 3 patches to make the root scroller
act as the layout viewport. The other two are up as well and marked as dependent
patchsets. The overall motivation is in the last one but I split out the first
two to make it easier to review.

[tdresser, 2016-07-06 18:16:15]

On 2016/07/06 17:53:09, bokan wrote:
> Tim, ptal. This is the first in a series of 3 patches to make the root
scroller
> act as the layout viewport. The other two are up as well and marked as
dependent
> patchsets. The overall motivation is in the last one but I split out the first
> two to make it easier to review.

"This will
make it easier to perform actions once we know the FrameView has initialized its
ScrollableArea which will be needed for the setRootScroller proposal."

I find this sentence a bit confusing.

Am I correct to interpret this as "This allows RootFrameViewport's constructor
to depend on the FrameView having an attached document"?

Code LGTM, just want to make sure I understand the purpose.

[tdresser, 2016-07-06 18:28:33]

https://codereview.chromium.org/2123663003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2123663003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:2126:
frameView->didAttachDocument();
I'd be just as happy with:
if (view())
  view()->didAttachDocument().

Either way.

[bokan, 2016-07-06 18:31:48]

Description was changed from

==========
Moved RootFrameViewport creation to happen after document attachment

I previously placed this to happen just before the first layout but that was
overly conservative; we really just need the document to be attached. This will
make it easier to perform actions once we know the FrameView has initialized its
ScrollableArea which will be needed for the setRootScroller proposal.

BUG=505516
==========

to

==========
Moved RootFrameViewport creation to happen after document attachment

I previously placed this to happen just before the first layout but that was
overly conservative; we really just need the document to be attached. This will
allow constructing a ViewportScrollCallback for a FrameView in just one place
for both root and nested frames. This will be needed in the setRootScroller
proposal in future patches.

BUG=505516
==========

[bokan, 2016-07-06 18:41:04]

On 2016/07/06 18:16:15, tdresser wrote:
> On 2016/07/06 17:53:09, bokan wrote:
> > Tim, ptal. This is the first in a series of 3 patches to make the root
> scroller
> > act as the layout viewport. The other two are up as well and marked as
> dependent
> > patchsets. The overall motivation is in the last one but I split out the
first
> > two to make it easier to review.
> 
> "This will
> make it easier to perform actions once we know the FrameView has initialized
its
> ScrollableArea which will be needed for the setRootScroller proposal."
> 
> I find this sentence a bit confusing.
> 
> Am I correct to interpret this as "This allows RootFrameViewport's constructor
> to depend on the FrameView having an attached document"?

That was already the case.

The reason is that in the dependent patches the ViewportScrollCallback needs to
be constructed after the FrameView has a document since in root-layer-scrolls
the LayoutView provides the ScrollableArea. Before attachment happens FrameView
won't have a ScrollableArea. The change to make this necessary is that the
RootViewportScrollCallback now gets the RootFrameViewport pointer at
construction.

The old code also needed this for RootFrameViewport but it made this happen in
prelayout and short circuited after it's happened once. This can't be easily
used for child frames though without adding a flag in FrameView but this is a
cleaner approach.
 
> Code LGTM, just want to make sure I understand the purpose.

[bokan, 2016-07-06 18:43:45]

On 2016/07/06 18:41:04, bokan wrote:
> On 2016/07/06 18:16:15, tdresser wrote:
> > On 2016/07/06 17:53:09, bokan wrote:
> > > Tim, ptal. This is the first in a series of 3 patches to make the root
> > scroller
> > > act as the layout viewport. The other two are up as well and marked as
> > dependent
> > > patchsets. The overall motivation is in the last one but I split out the
> first
> > > two to make it easier to review.
> > 
> > "This will
> > make it easier to perform actions once we know the FrameView has initialized
> its
> > ScrollableArea which will be needed for the setRootScroller proposal."
> > 
> > I find this sentence a bit confusing.
> > 
> > Am I correct to interpret this as "This allows RootFrameViewport's
constructor
> > to depend on the FrameView having an attached document"?
> 
> That was already the case.
> 
> The reason is that in the dependent patches the ViewportScrollCallback needs
to
> be constructed after the FrameView has a document since in root-layer-scrolls
> the LayoutView provides the ScrollableArea. Before attachment happens
FrameView
> won't have a ScrollableArea. The change to make this necessary is that the
> RootViewportScrollCallback now gets the RootFrameViewport pointer at
> construction.
> 
> The old code also needed this for RootFrameViewport but it made this happen in
> prelayout and short circuited after it's happened once. This can't be easily
> used for child frames though without adding a flag in FrameView but this is a
> cleaner approach.
>  
> > Code LGTM, just want to make sure I understand the purpose.

Updated description to make this more clear.

[bokan, 2016-07-06 18:43:52]

https://codereview.chromium.org/2123663003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2123663003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:2126:
frameView->didAttachDocument();
On 2016/07/06 18:28:33, tdresser wrote:
> I'd be just as happy with:
> if (view())
>   view()->didAttachDocument().
> 
> Either way.

Done.

[bokan, 2016-07-06 18:44:32]

The CQ bit was checked by bokan@chromium.org

[bokan, 2016-07-06 18:44:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/2123663003/#ps60001
(title: "Moved RootFrameViewport creation to happen after document attachment")

[commit-bot: I haz the power, 2016-07-06 18:45:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-06 22:08:43]

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

[commit-bot: I haz the power, 2016-07-06 22:08:44]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[bokan, 2016-07-06 22:09:30]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2016-07-06 22:10:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-06 23:27:58]

Description was changed from

==========
Moved RootFrameViewport creation to happen after document attachment

I previously placed this to happen just before the first layout but that was
overly conservative; we really just need the document to be attached. This will
allow constructing a ViewportScrollCallback for a FrameView in just one place
for both root and nested frames. This will be needed in the setRootScroller
proposal in future patches.

BUG=505516
==========

to

==========
Moved RootFrameViewport creation to happen after document attachment

I previously placed this to happen just before the first layout but that was
overly conservative; we really just need the document to be attached. This will
allow constructing a ViewportScrollCallback for a FrameView in just one place
for both root and nested frames. This will be needed in the setRootScroller
proposal in future patches.

BUG=505516
==========

[commit-bot: I haz the power, 2016-07-06 23:27:59]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-06 23:28:07]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-06 23:30:40]

Description was changed from

==========
Moved RootFrameViewport creation to happen after document attachment

I previously placed this to happen just before the first layout but that was
overly conservative; we really just need the document to be attached. This will
allow constructing a ViewportScrollCallback for a FrameView in just one place
for both root and nested frames. This will be needed in the setRootScroller
proposal in future patches.

BUG=505516
==========

to

==========
Moved RootFrameViewport creation to happen after document attachment

I previously placed this to happen just before the first layout but that was
overly conservative; we really just need the document to be attached. This will
allow constructing a ViewportScrollCallback for a FrameView in just one place
for both root and nested frames. This will be needed in the setRootScroller
proposal in future patches.

BUG=505516

Committed: https://crrev.com/3efe8736a302182bff2e9101044fd8e15668bbb6
Cr-Commit-Position: refs/heads/master@{#403973}
==========

[commit-bot: I haz the power, 2016-07-06 23:30:41]

Patchset 4 (id:??) landed as
https://crrev.com/3efe8736a302182bff2e9101044fd8e15668bbb6
Cr-Commit-Position: refs/heads/master@{#403973}