Move Get/SetScrollOffset methods from WebFrame to WebLocalFrame.

Move Get/SetScrollOffset methods from WebFrame to WebLocalFrame.

Most callers already call through WebLocalFrame.  In the 3 cases where
the caller cannot guarantee that the main frame is local, this CL drops
the NOTREACHED part of WebRemoteFrameImpl implementations, but otherwise
preserves the old behavior of returning zero WebSize in case of
GetScrollOffset and having no-op behavior in case of SetScrollOffset:
- DevToolsEmulator::ApplyViewportOverride
- DevToolsEmulator::HandleInputEvent
- WebViewImpl::WidenRectWithinPageBounds

BUG=416660
Review-Url: https://codereview.chromium.org/2936423003
Cr-Commit-Position: refs/heads/master@{#481196}
Committed: https://chromium.googlesource.com/chromium/src/+/29d4ee41e5b378d6ecac6e4fe7ec2ef84f5ed2b6

[Łukasz Anforowicz, 2017-06-15 21:27:21]

Description was changed from

==========
Move Get/SetScrollOffset methods from WebFrame to WebLocalFrame.

Most callers already call through WebLocalFrame.  In a few cases where
the caller cannot guarantee that the main frame is local, this CL
preserves the old behavior:
- DevToolsEmulator::ApplyViewportOverride
- DevToolsEmulator::HandleInputEvent
- WebViewImpl::WidenRectWithinPageBounds

No intended behavior change.

BUG=416660
==========

to

==========
Move Get/SetScrollOffset methods from WebFrame to WebLocalFrame.

Most callers already call through WebLocalFrame.  In the 3 cases where
the caller cannot guarantee that the main frame is local, this CL
preserves the old behavior (returning zero WebSize in case of
GetScrollOffset or no-op in case of SetScrollOffset, but drops
the NOTREACHED part):
- DevToolsEmulator::ApplyViewportOverride
- DevToolsEmulator::HandleInputEvent
- WebViewImpl::WidenRectWithinPageBounds

BUG=416660
==========

[Łukasz Anforowicz, 2017-06-15 21:28:22]

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

[commit-bot: I haz the power, 2017-06-15 21:28: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, 2017-06-15 23:16:37]

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

[commit-bot: I haz the power, 2017-06-15 23:16:38]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2017-06-15 23:20:52]

Description was changed from

==========
Move Get/SetScrollOffset methods from WebFrame to WebLocalFrame.

Most callers already call through WebLocalFrame.  In the 3 cases where
the caller cannot guarantee that the main frame is local, this CL
preserves the old behavior (returning zero WebSize in case of
GetScrollOffset or no-op in case of SetScrollOffset, but drops
the NOTREACHED part):
- DevToolsEmulator::ApplyViewportOverride
- DevToolsEmulator::HandleInputEvent
- WebViewImpl::WidenRectWithinPageBounds

BUG=416660
==========

to

==========
Move Get/SetScrollOffset methods from WebFrame to WebLocalFrame.

Most callers already call through WebLocalFrame.  In the 3 cases where
the caller cannot guarantee that the main frame is local, this CL drops
the NOTREACHED part of WebRemoteFrameImpl implementations, but otherwise
preserves the old behavior of returning zero WebSize in case of
GetScrollOffset and having no-op behavior in case of SetScrollOffset:
- DevToolsEmulator::ApplyViewportOverride
- DevToolsEmulator::HandleInputEvent
- WebViewImpl::WidenRectWithinPageBounds

BUG=416660
==========

[Łukasz Anforowicz, 2017-06-15 23:21:32]

lukasza@chromium.org changed reviewers:
+ dcheng@chromium.org

[Łukasz Anforowicz, 2017-06-15 23:21:32]

dcheng@, could you PTAL?

[dcheng, 2017-06-16 00:03:41]

https://codereview.chromium.org/2936423003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/DevToolsEmulator.cpp (right):

https://codereview.chromium.org/2936423003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/DevToolsEmulator.cpp:439: : WebSize();
Do we need TODOs here for OOPIFs?

https://codereview.chromium.org/2936423003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2936423003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewImpl.cpp:1300: scroll_offset =
MainFrame()->ToWebLocalFrame()->GetScrollOffset();
Ditto.

[Łukasz Anforowicz, 2017-06-16 21:26:47]

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

[commit-bot: I haz the power, 2017-06-16 21:27:06]

Dry run: CQ is trying da patch.

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

[Łukasz Anforowicz, 2017-06-16 21:28:13]

I've opened bugs and added the suggested TODOs in the latest patchset (which
accidentally also includes a rebase - sorry about this;  the only real changes
are the new comments in WebViewImpl.cpp and DevToolsEmulator.cpp).

https://codereview.chromium.org/2936423003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/DevToolsEmulator.cpp (right):

https://codereview.chromium.org/2936423003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/DevToolsEmulator.cpp:439: : WebSize();
On 2017/06/16 00:03:41, dcheng wrote:
> Do we need TODOs here for OOPIFs?

Done.

https://codereview.chromium.org/2936423003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2936423003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewImpl.cpp:1300: scroll_offset =
MainFrame()->ToWebLocalFrame()->GetScrollOffset();
On 2017/06/16 00:03:41, dcheng wrote:
> Ditto.

Done.

[dcheng, 2017-06-16 23:52:19]

dcheng@chromium.org changed reviewers:
+ alexmos@chromium.org

[dcheng, 2017-06-16 23:52:20]

https://codereview.chromium.org/2936423003/diff/20001/components/printing/ren...
File components/printing/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/2936423003/diff/20001/components/printing/ren...
components/printing/renderer/print_web_view_helper.cc:762: if
(web_frame->IsWebLocalFrame())
I wonder about this code. Isn't it always the case that it's a main frame? See
https://cs.chromium.org/chromium/src/components/printing/renderer/print_web_v...

(I'm not really sure though, as I don't really understand what's going on here)

https://codereview.chromium.org/2936423003/diff/20001/components/printing/ren...
components/printing/renderer/print_web_view_helper.cc:865:
web_frame->ToWebLocalFrame()->SetScrollOffset(prev_scroll_offset_);
Ditto: maybe I am misunderstanding this code?

https://codereview.chromium.org/2936423003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FullscreenController.cpp (right):

https://codereview.chromium.org/2936423003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FullscreenController.cpp:77:
web_view_base_->MainFrame()->ToWebLocalFrame()->SetScrollOffset(WebSize());
+alexmos, is there anything more we need to do here for OOPIF?

[commit-bot: I haz the power, 2017-06-16 23:54:14]

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

[commit-bot: I haz the power, 2017-06-16 23:54:14]

Dry run: This issue passed the CQ dry run.

[alexmos, 2017-06-17 00:04:02]

https://codereview.chromium.org/2936423003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FullscreenController.cpp (right):

https://codereview.chromium.org/2936423003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FullscreenController.cpp:77:
web_view_base_->MainFrame()->ToWebLocalFrame()->SetScrollOffset(WebSize());
On 2017/06/16 23:52:20, dcheng wrote:
> +alexmos, is there anything more we need to do here for OOPIF?

I don't think so - I think these only need to be done when the main frame is
local. 
(With OOPIF fullscreen, this will be called in each renderer, and I think we
only care about clearing and then restoring these to their original values in
the main frame renderer.)

[Łukasz Anforowicz, 2017-06-19 23:56:56]

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

[commit-bot: I haz the power, 2017-06-19 23:57:40]

Dry run: CQ is trying da patch.

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

[Łukasz Anforowicz, 2017-06-19 23:58:08]

Thanks for the review Daniel and making sure we track all the remaining OOPIF
support issues.  Can you take another look please?

https://codereview.chromium.org/2936423003/diff/20001/components/printing/ren...
File components/printing/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/2936423003/diff/20001/components/printing/ren...
components/printing/renderer/print_web_view_helper.cc:762: if
(web_frame->IsWebLocalFrame())
On 2017/06/16 23:52:20, dcheng wrote:
> I wonder about this code. Isn't it always the case that it's a main frame? See
>
https://cs.chromium.org/chromium/src/components/printing/renderer/print_web_v...
> 
> (I'm not really sure though, as I don't really understand what's going on
here)

Agreed that |frame_| is always a local frame.  I think the question is: Is
|frame_->view()->MainFrame()| always a local frame.

I looked for code paths that can set the value of the
PrepareFrameAndViewForPrint::frame_ field.  This can be set via:

1. PrepareFrameAndViewForPrint::CopySelection (in this case |frame_| is indeed
the main frame)

2. PrepareFrameAndViewForPrint's constructor (in this case |frame_| is not
necessarily the main frame, and therefore the main frame can be remote).


I've confirmed in a manual test that CHECK(web_frame->IsWebLocalFrame()) would
fail here.  Repro steps:
1. Add CHECK
2. Launch chrome with --site-per-process
3. Navigate to https://csreis.github.io/tests/cross-site-iframe.html
4. Click "Go cross-site (complex page)" button
5. Inspect the subframe
6. Run window.print() in the subframe

[commit-bot: I haz the power, 2017-06-20 01:27:44]

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

[commit-bot: I haz the power, 2017-06-20 01:27:45]

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

[dcheng, 2017-06-20 21:16:16]

LGTM, thanks for adding the TODOs.

[Łukasz Anforowicz, 2017-06-20 21:33:21]

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

[commit-bot: I haz the power, 2017-06-20 21:33:44]

Dry run: CQ is trying da patch.

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

[Łukasz Anforowicz, 2017-06-20 21:35:36]

lukasza@chromium.org changed reviewers:
+ thestig@chromium.org

[Łukasz Anforowicz, 2017-06-20 21:35:37]

thestig@, could you PTAL @ the changes in
components/printing/renderer/print_web_view_helper.cc?

(I am also adding weili@ to CC as an FYI, since she is working on OOPIF support
for printing and since her name is in the TODOs I am adding)

[commit-bot: I haz the power, 2017-06-21 00:44:28]

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

[commit-bot: I haz the power, 2017-06-21 00:44:29]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2017-06-21 07:42:43]

lgtm

[Łukasz Anforowicz, 2017-06-21 14:06:40]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2017-06-21 14:06:40]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2936423003/#ps60001
(title: "Use WebViewHelper::LocalMainFrame() where possible.")

[commit-bot: I haz the power, 2017-06-21 14:06:54]

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-06-21 14:10:17]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498054000257050,
"parent_rev": "f9481519e9f7666a61f759e34317a5af900b2845", "commit_rev":
"29d4ee41e5b378d6ecac6e4fe7ec2ef84f5ed2b6"}

[commit-bot: I haz the power, 2017-06-21 14:10:30]

Description was changed from

==========
Move Get/SetScrollOffset methods from WebFrame to WebLocalFrame.

Most callers already call through WebLocalFrame.  In the 3 cases where
the caller cannot guarantee that the main frame is local, this CL drops
the NOTREACHED part of WebRemoteFrameImpl implementations, but otherwise
preserves the old behavior of returning zero WebSize in case of
GetScrollOffset and having no-op behavior in case of SetScrollOffset:
- DevToolsEmulator::ApplyViewportOverride
- DevToolsEmulator::HandleInputEvent
- WebViewImpl::WidenRectWithinPageBounds

BUG=416660
==========

to

==========
Move Get/SetScrollOffset methods from WebFrame to WebLocalFrame.

Most callers already call through WebLocalFrame.  In the 3 cases where
the caller cannot guarantee that the main frame is local, this CL drops
the NOTREACHED part of WebRemoteFrameImpl implementations, but otherwise
preserves the old behavior of returning zero WebSize in case of
GetScrollOffset and having no-op behavior in case of SetScrollOffset:
- DevToolsEmulator::ApplyViewportOverride
- DevToolsEmulator::HandleInputEvent
- WebViewImpl::WidenRectWithinPageBounds

BUG=416660

Review-Url: https://codereview.chromium.org/2936423003
Cr-Commit-Position: refs/heads/master@{#481196}
Committed:
https://chromium.googlesource.com/chromium/src/+/29d4ee41e5b378d6ecac6e4fe7ec...
==========

[commit-bot: I haz the power, 2017-06-21 14:10:32]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/29d4ee41e5b378d6ecac6e4fe7ec...