Split RootScrollerController into top-document and child-document classes

Split RootScrollerController into top-document and child-document classes

Work on making RootScrollerController work across iframes revealed that there's
two responsibilities that need management. All frames need a
RootScrollerController to manage the Element currently set as root scroller and
determine which Element should eventually become the effective root scroller.
In addition, we need the RootScrollerController to manage the
ViewportApplyScroll and set it on the appropriate element. Since only one
Element on the whole page needs this callback set, it makes sense that only the
top document should have this responsibility. Experience with classes like
FrameView and EventHandler shows that this may be better served by subclassing
the top level object rather than having isMainFrame() checks intermingled with
the other code.

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

[bokan, 2016-08-24 23:38:56]

Description was changed from

==========
Split RootScrollerController into top-document and child-document classes

Work on making RootScrollerController work across iframes revealed that there's
two responsibilities that need management. All iframes need a
RootScrollerController to manage the Element currently set as root scroller and
determining which Element should eventually become the effective root scroller.
In addition, we need the RootScrollerController to manage the
ViewportApplyScroll and set it on the appropriate element. Since only one
Element on the whole page needs this callback set, it makes sense that only the
top document should have this responsibility. Experience with classes like
FrameView and EventHandler shows that this may be better served by subclassing
the top level object rather than having isMainFrame() checks intermingled with
the other code.

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

to

==========
Split RootScrollerController into top-document and child-document classes

Work on making RootScrollerController work across iframes revealed that there's
two responsibilities that need management. All frames need a
RootScrollerController to manage the Element currently set as root scroller and
determine which Element should eventually become the effective root scroller.
In addition, we need the RootScrollerController to manage the
ViewportApplyScroll and set it on the appropriate element. Since only one
Element on the whole page needs this callback set, it makes sense that only the
top document should have this responsibility. Experience with classes like
FrameView and EventHandler shows that this may be better served by subclassing
the top level object rather than having isMainFrame() checks intermingled with
the other code.

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

[bokan, 2016-08-24 23:38:57]

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

[bokan, 2016-08-24 23:39:18]

part 2 of the cleanup

[bokan, 2016-08-25 14:52:04]

Actually, I think TopDocumentRootScroller should actually be a separate class
and that'll move onto Page or some other page-global place. This'll make the
move to OOPIF more natural and works better conceptually but I think this CL is
still worth landing as a stepping stone to that place.

[tdresser, 2016-08-25 14:59:21]

LGTM

https://codereview.chromium.org/2280453002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2280453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:477: // in checks to
isInMainFrame.
I don't quite follow this TODO.

https://codereview.chromium.org/2280453002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp
(right):

https://codereview.chromium.org/2280453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp:165:
DCHECK(!m_document->isInMainFrame());
Maybe indicate that the top documents controller will be a
TopDocumentRootScrollerController, which is why this DCHECK makes sense.

[bokan, 2016-08-25 15:14:13]

https://codereview.chromium.org/2280453002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2280453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:477: // in checks to
isInMainFrame.
On 2016/08/25 14:59:21, tdresser wrote:
> I don't quite follow this TODO.

Ah, yah, this was added pre-split so I've updated it to make more sense. The
issue is that we can actually initialize a Document without an m_frame which
will now assume that we're in a child document which isn't necessarily true.
This should become moot when I move make TopDocumentRootScroller owned by a
page-level object.

https://codereview.chromium.org/2280453002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp
(right):

https://codereview.chromium.org/2280453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp:165:
DCHECK(!m_document->isInMainFrame());
On 2016/08/25 14:59:21, tdresser wrote:
> Maybe indicate that the top documents controller will be a
> TopDocumentRootScrollerController, which is why this DCHECK makes sense.

Done.

[bokan, 2016-08-25 15:14:27]

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

[commit-bot: I haz the power, 2016-08-25 15:14:53]

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

[tdresser, 2016-08-25 15:18:12]

https://codereview.chromium.org/2280453002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2280453002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.cpp:477: // in checks to
isInMainFrame.
On 2016/08/25 15:14:13, bokan wrote:
> On 2016/08/25 14:59:21, tdresser wrote:
> > I don't quite follow this TODO.
> 
> Ah, yah, this was added pre-split so I've updated it to make more sense. The
> issue is that we can actually initialize a Document without an m_frame which
> will now assume that we're in a child document which isn't necessarily true.
> This should become moot when I move make TopDocumentRootScroller owned by a
> page-level object.

Should this TODO have a bug?

[bokan, 2016-08-25 15:21:57]

On 2016/08/25 15:18:12, tdresser wrote:
>
https://codereview.chromium.org/2280453002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/2280453002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/Document.cpp:477: // in checks to
> isInMainFrame.
> On 2016/08/25 15:14:13, bokan wrote:
> > On 2016/08/25 14:59:21, tdresser wrote:
> > > I don't quite follow this TODO.
> > 
> > Ah, yah, this was added pre-split so I've updated it to make more sense. The
> > issue is that we can actually initialize a Document without an m_frame which
> > will now assume that we're in a child document which isn't necessarily true.
> > This should become moot when I move make TopDocumentRootScroller owned by a
> > page-level object.
> 
> Should this TODO have a bug?

Linked it to the main implementation bug.

[bokan, 2016-08-25 15:22:10]

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

[commit-bot: I haz the power, 2016-08-25 15:22:29]

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-08-25 17:44:51]

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

[commit-bot: I haz the power, 2016-08-25 17:44:52]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[bokan, 2016-08-25 18:53:45]

The CQ bit was checked by bokan@chromium.org

[bokan, 2016-08-25 18:53:46]

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/2280453002/#ps40001
(title: "Linked TODO to bug")

[commit-bot: I haz the power, 2016-08-25 18:54:24]

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-08-25 20:55:54]

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

[commit-bot: I haz the power, 2016-08-25 20:55:55]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[bokan, 2016-08-26 15:12:54]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2016-08-26 15:13:11]

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-08-26 20:27:48]

Description was changed from

==========
Split RootScrollerController into top-document and child-document classes

Work on making RootScrollerController work across iframes revealed that there's
two responsibilities that need management. All frames need a
RootScrollerController to manage the Element currently set as root scroller and
determine which Element should eventually become the effective root scroller.
In addition, we need the RootScrollerController to manage the
ViewportApplyScroll and set it on the appropriate element. Since only one
Element on the whole page needs this callback set, it makes sense that only the
top document should have this responsibility. Experience with classes like
FrameView and EventHandler shows that this may be better served by subclassing
the top level object rather than having isMainFrame() checks intermingled with
the other code.

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

to

==========
Split RootScrollerController into top-document and child-document classes

Work on making RootScrollerController work across iframes revealed that there's
two responsibilities that need management. All frames need a
RootScrollerController to manage the Element currently set as root scroller and
determine which Element should eventually become the effective root scroller.
In addition, we need the RootScrollerController to manage the
ViewportApplyScroll and set it on the appropriate element. Since only one
Element on the whole page needs this callback set, it makes sense that only the
top document should have this responsibility. Experience with classes like
FrameView and EventHandler shows that this may be better served by subclassing
the top level object rather than having isMainFrame() checks intermingled with
the other code.

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

[commit-bot: I haz the power, 2016-08-26 20:27:49]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-26 20:30:20]

Description was changed from

==========
Split RootScrollerController into top-document and child-document classes

Work on making RootScrollerController work across iframes revealed that there's
two responsibilities that need management. All frames need a
RootScrollerController to manage the Element currently set as root scroller and
determine which Element should eventually become the effective root scroller.
In addition, we need the RootScrollerController to manage the
ViewportApplyScroll and set it on the appropriate element. Since only one
Element on the whole page needs this callback set, it makes sense that only the
top document should have this responsibility. Experience with classes like
FrameView and EventHandler shows that this may be better served by subclassing
the top level object rather than having isMainFrame() checks intermingled with
the other code.

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

to

==========
Split RootScrollerController into top-document and child-document classes

Work on making RootScrollerController work across iframes revealed that there's
two responsibilities that need management. All frames need a
RootScrollerController to manage the Element currently set as root scroller and
determine which Element should eventually become the effective root scroller.
In addition, we need the RootScrollerController to manage the
ViewportApplyScroll and set it on the appropriate element. Since only one
Element on the whole page needs this callback set, it makes sense that only the
top document should have this responsibility. Experience with classes like
FrameView and EventHandler shows that this may be better served by subclassing
the top level object rather than having isMainFrame() checks intermingled with
the other code.

BUG=505516

Committed: https://crrev.com/f8bc5669c399f88dc57d6a8832bc233db306c3c7
Cr-Commit-Position: refs/heads/master@{#414796}
==========

[commit-bot: I haz the power, 2016-08-26 20:30:21]

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

[lfg, 2016-08-30 14:06:47]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2293903002/ by lfg@chromium.org.

The reason for reverting is: Causes oopif renderers to die. See
https://crbug.com/641775.