Make TopDocumentRootScrollerController store Page instead of FrameHost

Make TopDocumentRootScrollerController store Page instead of FrameHost

This is pre-work to move FrameHost::m_topDocumentRootScrollerController
to Page.

BUG=691794
Review-Url: https://codereview.chromium.org/2732363003
Cr-Commit-Position: refs/heads/master@{#455980}
Committed: https://chromium.googlesource.com/chromium/src/+/32fbfd186648b908ee1f708b6a95f1308f52e308

[sashab, 2017-03-08 04:14:08]

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

[sashab, 2017-03-08 04:14:17]

sashab@chromium.org changed reviewers:
+ joelhockey@chromium.org

[commit-bot: I haz the power, 2017-03-08 04:14:28]

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, 2017-03-08 04:16:11]

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

[commit-bot: I haz the power, 2017-03-08 04:16:12]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[joelhockey, 2017-03-08 04:36:01]

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

https://codereview.chromium.org/2732363003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/scrolling/TopDocumentRootScrollerController.cpp:177:
return toLocalFrame(m_page->frameHost().page().mainFrame())->document();
I think these references of m_page->frameHost.page() could just be m_page

From what I can see, m_page and m_page->frameHost.page() are always the same.
I see that Page constructs a FrameHost with this
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Page...
And then within FrameHost, page is not modified
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Fra...

Could the whole method be:

return m_page && m_page->mainFrame() && m_page->mainFrame().isLocalFrame()
  ? m_page->mainFrame()->document() : nullptr;

[sashab, 2017-03-08 05:02:37]

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

https://codereview.chromium.org/2732363003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/scrolling/TopDocumentRootScrollerController.cpp:177:
return toLocalFrame(m_page->frameHost().page().mainFrame())->document();
On 2017/03/08 at 04:36:00, joelhockey wrote:
> I think these references of m_page->frameHost.page() could just be m_page
> 
> From what I can see, m_page and m_page->frameHost.page() are always the same.
> I see that Page constructs a FrameHost with this
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Page...
> And then within FrameHost, page is not modified
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Fra...
> 
> Could the whole method be:
> 
> return m_page && m_page->mainFrame() && m_page->mainFrame().isLocalFrame()
>   ? m_page->mainFrame()->document() : nullptr;

Oh yeah, great catch! Fixed here and below, thanks.

[sashab, 2017-03-08 05:02:42]

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

[commit-bot: I haz the power, 2017-03-08 05:03:11]

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, 2017-03-08 05:05:31]

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

[commit-bot: I haz the power, 2017-03-08 05:05:32]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[joelhockey, 2017-03-08 06:02:52]

lgtm

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

https://codereview.chromium.org/2732363003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/TopDocumentRootScrollerController.cpp:174:
return nullptr;
It would make a lot of sense to combine the 2 if-statements above.

if (!m_page || !m_page->mainFrame() || !m_page->mainFrame()->isLocalFrame()
  return nullptr;

Or, I would use a single ternary statement for the whole function.

[haraken, 2017-03-08 06:15:48]

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

[haraken, 2017-03-08 06:15:48]

LGTM

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

https://codereview.chromium.org/2732363003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/TopDocumentRootScrollerController.h:109:
WeakMember<Page> m_page;

Not related to this CL, I don't understand why this pointer needs to be weak.
TopDocumentRootScrollerController and Page should live and die at the same time,
so this could just be a Member<Page>.

[sashab, 2017-03-08 23:37:31]

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

[commit-bot: I haz the power, 2017-03-08 23:38:08]

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

[sashab, 2017-03-08 23:39:31]

Thanks for the great feedback everyone!

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

https://codereview.chromium.org/2732363003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/TopDocumentRootScrollerController.cpp:174:
return nullptr;
On 2017/03/08 at 06:02:52, joelhockey wrote:
> It would make a lot of sense to combine the 2 if-statements above.
> 
> if (!m_page || !m_page->mainFrame() || !m_page->mainFrame()->isLocalFrame()
>   return nullptr;
> 
> Or, I would use a single ternary statement for the whole function.

Thanks, good idea! Combined them.

I tried a ternary but it looked a bit weird, I think the early-exit return
better matches the style of the other methods in the class.

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

https://codereview.chromium.org/2732363003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/TopDocumentRootScrollerController.h:109:
WeakMember<Page> m_page;
On 2017/03/08 at 06:15:48, haraken wrote:
> Not related to this CL, I don't understand why this pointer needs to be weak.
TopDocumentRootScrollerController and Page should live and die at the same time,
so this could just be a Member<Page>.

Haha, I thought the same thing when I saw this! Let me land it in a follow-up
patch, in case it causes test failures.

[commit-bot: I haz the power, 2017-03-09 01:04:09]

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

[commit-bot: I haz the power, 2017-03-09 01:04:10]

Dry run: This issue passed the CQ dry run.

[sashab, 2017-03-09 03:15:25]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-03-09 03:15:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, joelhockey@chromium.org
Link to the patchset: https://codereview.chromium.org/2732363003/#ps40001
(title: "Rebase and review feedback")

[sashab, 2017-03-09 03:15:30]

The CQ bit was unchecked by sashab@chromium.org

[sashab, 2017-03-09 03:15:32]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2017-03-09 03:15:55]

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, 2017-03-09 03:22:50]

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

[commit-bot: I haz the power, 2017-03-09 03:22:51]

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

[sashab, 2017-03-09 03:26:31]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-03-09 03:26:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, joelhockey@chromium.org
Link to the patchset: https://codereview.chromium.org/2732363003/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-09 03:26:58]

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, 2017-03-09 04:18:42]

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

[commit-bot: I haz the power, 2017-03-09 04:18:43]

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_...)

[sashab, 2017-03-10 01:57:12]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-03-10 01:57:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, joelhockey@chromium.org
Link to the patchset: https://codereview.chromium.org/2732363003/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-10 01:57:50]

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, 2017-03-10 03:52:09]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489111032333440,
"parent_rev": "fd6d973ee6a4c853542cf4ec2a78dfd294322402", "commit_rev":
"32fbfd186648b908ee1f708b6a95f1308f52e308"}

[commit-bot: I haz the power, 2017-03-10 03:53:23]

Description was changed from

==========
Make TopDocumentRootScrollerController store Page instead of FrameHost

This is pre-work to move FrameHost::m_topDocumentRootScrollerController
to Page.

BUG=691794
==========

to

==========
Make TopDocumentRootScrollerController store Page instead of FrameHost

This is pre-work to move FrameHost::m_topDocumentRootScrollerController
to Page.

BUG=691794

Review-Url: https://codereview.chromium.org/2732363003
Cr-Commit-Position: refs/heads/master@{#455980}
Committed:
https://chromium.googlesource.com/chromium/src/+/32fbfd186648b908ee1f708b6a95...
==========

[commit-bot: I haz the power, 2017-03-10 03:53:24]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/32fbfd186648b908ee1f708b6a95...