Fixed up root scroller API to be more webby

Fixed up root scroller API to be more webby

-Removed document.setRootScroller and replaced it by making the
document.rootScroller attribute writable.

-Moved the RootScroller object from FrameHost into Document. Now each
Document has its own RootScroller object which manages the rootScroller
for that Document. This means each iframe on the page will have its own
rootScroller. I've allowed setting it from iframes now too, making the
API more composable; setting a rootScroller on an iframe's Document
simply doesn't change any behavior.

-Made the rootScroller "sticky". Previously, if the current rootScroller
becomes invalid (for example, the rootScroller Element becomes
display:none) it would reset to the default element. With this CL, we'll
use the default while the rootScroller is invalid, but we keep the
rootScroller set. When it becomes valid again, we'll use it again. This
is done by making RootScroller::m_rootScroller the Element the page has
set as the root scroller. RootScroller::m_effectiveRootScroller is the
Element we're using internally to handle viewport actions.

-Changed the criteria for a valid rootScroller. Elements in an iframe can
now be used. Only elements that exactly fill the viewport are considered
valid.

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

[bokan, 2016-05-11 15:13:27]

Patchset #1 (id:1) has been deleted

[bokan, 2016-05-11 15:23:20]

Description was changed from

==========
Fixed up root scroller APIs to be more webby

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

to

==========
Fixed up root scroller API to be more webby

-Removed document.setRootScroller and replaced it by making the
document.rootScroller attribute writable.

-Moved the RootScroller object from FrameHost into Document. Now each
Document has its own RootScroller object which manages the rootScroller
for that Document. This means each iframe on the page will have its own
rootScroller. I've allowed setting it from iframes now too, making the
API more composable; setting a rootScroller on an iframe's Document
simply doesn't change any behavior.

-Made the rootScroller "sticky". Previously, if the current rootScroller
becomes invalid (for example, the rootScroller Element becomes
display:none) it would reset to the default element. With this CL, we'll
use the default while the rootScroller is invalid, but we keep the
rootScroller set. When it becomes valid again, we'll use it again. This
is done by making RootScroller::m_rootScroller the Element the page has
set as the root scroller. RootScroller::m_effectiveRootScroller is the
Element we're using internally to handle viewport actions.

-Changed the criteria for a valid rootScroller. Elements in an iframe can
now be used. Only elements that exactly fill the viewport are considered
valid.


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

[bokan, 2016-05-11 15:24:13]

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

[bokan, 2016-05-11 15:28:25]

Hi Elliott,

ptal, I made the changes you mentioned in
https://codereview.chromium.org/1913843004/

Some caveats:

-I made RootScroller 1-1 with Document rather than Frame since the API is on the
document object. This avoids a bunch of lifetime issues. Is it ok?

-I kept hooked it to layout rather than Element::removedFrom. I've updated the
validity criteria to only accept elements that fill the viewport exactly. I
think I need layout for that.

Thanks.

[bokan, 2016-05-11 15:41:48]

Oh, and I still need to update/add tests but I'll do that if you think this is
fine.

[esprehn, 2016-05-11 17:37:05]

lgtm w/ nits about refs fixed. I added some other comments about the design, but
those can be fixed in later patches. Thanks for making your API webby!!

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

https://codereview.chromium.org/1970763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:482: m_rootScroller =
RootScroller::create(*this, applyScroll);
we should rename RootScroller to something like RootScrollController or
something. It's confusing that setRootScroller takes an Element, and the
RootScroller object is something else.

https://codereview.chromium.org/1970763002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1970763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:2433: return
m_frame->document()->topDocument().isEffectiveRootScroller(toElement(&node));
your method should take a const Element&, then do toElement(node) here

https://codereview.chromium.org/1970763002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/scrolling/RootScroller.cpp (right):

https://codereview.chromium.org/1970763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/RootScroller.cpp:43: Document&
topDocument = element.document().topDocument();
hmm, this might be broken for OOPIF. I bet it is. topDocument() can't work in
that case. (No need to fix in this patch, just an observation)

https://codereview.chromium.org/1970763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/RootScroller.cpp:94: void
RootScroller::set(Element* newRootScroller)
I think I'd rename this to RootScrollController or something, and then make
these GetElement() and setElement() or something.

(You can do that in future patches)

https://codereview.chromium.org/1970763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/RootScroller.cpp:112:
updateEffectiveRootScroller();
we probably want to only do this for pipeline updates, not every random layout?
Probably good to fix that in future patches.

https://codereview.chromium.org/1970763002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/scrolling/RootScroller.h (left):

https://codereview.chromium.org/1970763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/RootScroller.h:51:
RootScroller(FrameHost&);
fyi for the future, even private constructors need explicit if they have a
single argument, otherwise you can call them by mistake inside your own class.
:)

https://codereview.chromium.org/1970763002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/scrolling/RootScroller.h (right):

https://codereview.chromium.org/1970763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/RootScroller.h:89: void
moveViewportApplyScroll(Element* target);
can this be null? reference?

[bokan, 2016-06-02 15:49:55]

Back from vacation and ready to land. I've also updated tests to match the new
behavior. Will fix the Ack'd comments in follow up patches.

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

https://codereview.chromium.org/1970763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:482: m_rootScroller =
RootScroller::create(*this, applyScroll);
On 2016/05/11 17:37:05, esprehn wrote:
> we should rename RootScroller to something like RootScrollController or
> something. It's confusing that setRootScroller takes an Element, and the
> RootScroller object is something else.

Acknowledged.

https://codereview.chromium.org/1970763002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1970763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:2433: return
m_frame->document()->topDocument().isEffectiveRootScroller(toElement(&node));
On 2016/05/11 17:37:05, esprehn wrote:
> your method should take a const Element&, then do toElement(node) here

Done.

https://codereview.chromium.org/1970763002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/scrolling/RootScroller.cpp (right):

https://codereview.chromium.org/1970763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/RootScroller.cpp:43: Document&
topDocument = element.document().topDocument();
On 2016/05/11 17:37:05, esprehn wrote:
> hmm, this might be broken for OOPIF. I bet it is. topDocument() can't work in
> that case. (No need to fix in this patch, just an observation)

Acknowledged. Added a TODO.

https://codereview.chromium.org/1970763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/RootScroller.cpp:94: void
RootScroller::set(Element* newRootScroller)
On 2016/05/11 17:37:05, esprehn wrote:
> I think I'd rename this to RootScrollController or something, and then make
> these GetElement() and setElement() or something.
> 
> (You can do that in future patches)

Acknowledged.

https://codereview.chromium.org/1970763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/RootScroller.cpp:112:
updateEffectiveRootScroller();
On 2016/05/11 17:37:05, esprehn wrote:
> we probably want to only do this for pipeline updates, not every random
layout?
> Probably good to fix that in future patches.

Acknowledged.

https://codereview.chromium.org/1970763002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/scrolling/RootScroller.h (left):

https://codereview.chromium.org/1970763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/RootScroller.h:51:
RootScroller(FrameHost&);
On 2016/05/11 17:37:05, esprehn wrote:
> fyi for the future, even private constructors need explicit if they have a
> single argument, otherwise you can call them by mistake inside your own class.
> :)

Acknowledged.

https://codereview.chromium.org/1970763002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/scrolling/RootScroller.h (right):

https://codereview.chromium.org/1970763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/RootScroller.h:89: void
moveViewportApplyScroll(Element* target);
On 2016/05/11 17:37:05, esprehn wrote:
> can this be null? reference?

It can be null though, if we're using the defaultEffectiveRootScroller and
there's no documentElement. (The m_document check inside should never be NULL
though so I turned that into a DCHECK instead).

[bokan, 2016-06-02 15:50:09]

The CQ bit was checked by bokan@chromium.org

[bokan, 2016-06-02 15:50:10]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1970763002/#ps60001
(title: "Fixed tests to match new expectations")

[commit-bot: I haz the power, 2016-06-02 15:50:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1970763002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1970763002/60001

[commit-bot: I haz the power, 2016-06-02 15:58:21]

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

[commit-bot: I haz the power, 2016-06-02 15:58:22]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[bokan, 2016-06-02 19:01:15]

The CQ bit was checked by bokan@chromium.org

[bokan, 2016-06-02 19:01:16]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1970763002/#ps80001
(title: "Missed One Test")

[commit-bot: I haz the power, 2016-06-02 19:02:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1970763002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1970763002/80001

[commit-bot: I haz the power, 2016-06-02 19:39:49]

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

[commit-bot: I haz the power, 2016-06-02 19:39:51]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_clang on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[bokan, 2016-06-03 14:37:10]

The CQ bit was checked by bokan@chromium.org

[bokan, 2016-06-03 14:37:11]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1970763002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2016-06-03 14:37:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1970763002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1970763002/100001

[commit-bot: I haz the power, 2016-06-03 16:32:32]

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

[commit-bot: I haz the power, 2016-06-03 16:32:34]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bokan, 2016-06-03 17:27:16]

The CQ bit was checked by bokan@chromium.org

[bokan, 2016-06-03 17:27:16]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1970763002/#ps120001
(title: "Sigh...fix test expectation again, fix crash when there's no renderer")

[commit-bot: I haz the power, 2016-06-03 17:27:43]

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

[commit-bot: I haz the power, 2016-06-03 19:26:55]

Description was changed from

==========
Fixed up root scroller API to be more webby

-Removed document.setRootScroller and replaced it by making the
document.rootScroller attribute writable.

-Moved the RootScroller object from FrameHost into Document. Now each
Document has its own RootScroller object which manages the rootScroller
for that Document. This means each iframe on the page will have its own
rootScroller. I've allowed setting it from iframes now too, making the
API more composable; setting a rootScroller on an iframe's Document
simply doesn't change any behavior.

-Made the rootScroller "sticky". Previously, if the current rootScroller
becomes invalid (for example, the rootScroller Element becomes
display:none) it would reset to the default element. With this CL, we'll
use the default while the rootScroller is invalid, but we keep the
rootScroller set. When it becomes valid again, we'll use it again. This
is done by making RootScroller::m_rootScroller the Element the page has
set as the root scroller. RootScroller::m_effectiveRootScroller is the
Element we're using internally to handle viewport actions.

-Changed the criteria for a valid rootScroller. Elements in an iframe can
now be used. Only elements that exactly fill the viewport are considered
valid.


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

to

==========
Fixed up root scroller API to be more webby

-Removed document.setRootScroller and replaced it by making the
document.rootScroller attribute writable.

-Moved the RootScroller object from FrameHost into Document. Now each
Document has its own RootScroller object which manages the rootScroller
for that Document. This means each iframe on the page will have its own
rootScroller. I've allowed setting it from iframes now too, making the
API more composable; setting a rootScroller on an iframe's Document
simply doesn't change any behavior.

-Made the rootScroller "sticky". Previously, if the current rootScroller
becomes invalid (for example, the rootScroller Element becomes
display:none) it would reset to the default element. With this CL, we'll
use the default while the rootScroller is invalid, but we keep the
rootScroller set. When it becomes valid again, we'll use it again. This
is done by making RootScroller::m_rootScroller the Element the page has
set as the root scroller. RootScroller::m_effectiveRootScroller is the
Element we're using internally to handle viewport actions.

-Changed the criteria for a valid rootScroller. Elements in an iframe can
now be used. Only elements that exactly fill the viewport are considered
valid.


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

[commit-bot: I haz the power, 2016-06-03 19:26:56]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-06-03 19:29:08]

Description was changed from

==========
Fixed up root scroller API to be more webby

-Removed document.setRootScroller and replaced it by making the
document.rootScroller attribute writable.

-Moved the RootScroller object from FrameHost into Document. Now each
Document has its own RootScroller object which manages the rootScroller
for that Document. This means each iframe on the page will have its own
rootScroller. I've allowed setting it from iframes now too, making the
API more composable; setting a rootScroller on an iframe's Document
simply doesn't change any behavior.

-Made the rootScroller "sticky". Previously, if the current rootScroller
becomes invalid (for example, the rootScroller Element becomes
display:none) it would reset to the default element. With this CL, we'll
use the default while the rootScroller is invalid, but we keep the
rootScroller set. When it becomes valid again, we'll use it again. This
is done by making RootScroller::m_rootScroller the Element the page has
set as the root scroller. RootScroller::m_effectiveRootScroller is the
Element we're using internally to handle viewport actions.

-Changed the criteria for a valid rootScroller. Elements in an iframe can
now be used. Only elements that exactly fill the viewport are considered
valid.


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

to

==========
Fixed up root scroller API to be more webby

-Removed document.setRootScroller and replaced it by making the
document.rootScroller attribute writable.

-Moved the RootScroller object from FrameHost into Document. Now each
Document has its own RootScroller object which manages the rootScroller
for that Document. This means each iframe on the page will have its own
rootScroller. I've allowed setting it from iframes now too, making the
API more composable; setting a rootScroller on an iframe's Document
simply doesn't change any behavior.

-Made the rootScroller "sticky". Previously, if the current rootScroller
becomes invalid (for example, the rootScroller Element becomes
display:none) it would reset to the default element. With this CL, we'll
use the default while the rootScroller is invalid, but we keep the
rootScroller set. When it becomes valid again, we'll use it again. This
is done by making RootScroller::m_rootScroller the Element the page has
set as the root scroller. RootScroller::m_effectiveRootScroller is the
Element we're using internally to handle viewport actions.

-Changed the criteria for a valid rootScroller. Elements in an iframe can
now be used. Only elements that exactly fill the viewport are considered
valid.

BUG=505516

Committed: https://crrev.com/16c5f9e182d412046291088ab782579f315bf100
Cr-Commit-Position: refs/heads/master@{#397778}
==========

[commit-bot: I haz the power, 2016-06-03 19:29:09]

Patchset 6 (id:??) landed as
https://crrev.com/16c5f9e182d412046291088ab782579f315bf100
Cr-Commit-Position: refs/heads/master@{#397778}