Move ExecuteScript method from WebFrame to WebLocalFrame.

Move ExecuteScript method from WebFrame to WebLocalFrame.

Many callers already were calling through WebLocalFrame.

For 4 callers that can't guarantee at compile-time that the target frame
is a local frame, this CL:
- preserves the NOTREACHED() / no-op behavior (in case of
  components/plugins/renderer/loadable_plugin_placeholder.cc)
- or does the same, but upgrading NOTREACHED() to a CHECK()
  (in case of content/shell/test_runner/test_runner.cc)

No intended behavior change.

BUG=416660
TBR=mattm@chromium.org, dcblack@chromium.org, scottbyer@chromium.org, avi@chromium.org

Review-Url: https://codereview.chromium.org/2923433002
Cr-Commit-Position: refs/heads/master@{#477333}
Committed: https://chromium.googlesource.com/chromium/src/+/bb4fe6d5aa4d9584944c2223dcd7f3b92da42e9a

[Łukasz Anforowicz, 2017-06-02 22:09:52]

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

[commit-bot: I haz the power, 2017-06-02 22:10:19]

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-02 23:45:49]

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

[Łukasz Anforowicz, 2017-06-02 23:45:50]

dcheng@, could you PTAL?

[commit-bot: I haz the power, 2017-06-03 01:38:25]

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

[commit-bot: I haz the power, 2017-06-03 01:38:25]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-06-04 10:37:23]

https://codereview.chromium.org/2923433002/diff/1/components/plugins/renderer...
File components/plugins/renderer/loadable_plugin_placeholder.cc (right):

https://codereview.chromium.org/2923433002/diff/1/components/plugins/renderer...
components/plugins/renderer/loadable_plugin_placeholder.cc:149: if
(!main_frame->IsWebLocalFrame()) {
As this is a WebViewPlugin, which is only used to load very simple content for
the plugin placeholder, we should be able to just
DCHECK(main_frame->IsWebLocalFrame()) here.

(One possibility to make this clear is to directly expose a main_frame() helper
on WebViewPlugin that does the casting internally)

https://codereview.chromium.org/2923433002/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/2923433002/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebLocalFrame.h:583: virtual void
ExecuteScript(const WebScriptSource&) = 0;
Since there's already a scripting section on line 268, let's group them
together.

[Łukasz Anforowicz, 2017-06-05 17:08:43]

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

[commit-bot: I haz the power, 2017-06-05 17:09:03]

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-05 17:10:57]

Daniel - thanks for the review and feedback.

https://codereview.chromium.org/2923433002/diff/1/components/plugins/renderer...
File components/plugins/renderer/loadable_plugin_placeholder.cc (right):

https://codereview.chromium.org/2923433002/diff/1/components/plugins/renderer...
components/plugins/renderer/loadable_plugin_placeholder.cc:149: if
(!main_frame->IsWebLocalFrame()) {
On 2017/06/04 10:37:22, dcheng wrote:
> As this is a WebViewPlugin, which is only used to load very simple content for
> the plugin placeholder, we should be able to just
> DCHECK(main_frame->IsWebLocalFrame()) here.
> 
> (One possibility to make this clear is to directly expose a main_frame()
helper
> on WebViewPlugin that does the casting internally)

Good point - done:

- I've exposed WebViewPlugin::main_frame accessor that returns a local frame. 
This means that WebViewPlugin::web_view accessor can be made private.

- I've changed a few places to use the main_frame accessor, rather than call
ToWebLocalFrame, etc.

https://codereview.chromium.org/2923433002/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/2923433002/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebLocalFrame.h:583: virtual void
ExecuteScript(const WebScriptSource&) = 0;
On 2017/06/04 10:37:22, dcheng wrote:
> Since there's already a scripting section on line 268, let's group them
> together.

Ooops.  Thanks for catching this.

https://codereview.chromium.org/2923433002/diff/20001/components/plugins/rend...
File components/plugins/renderer/webview_plugin.cc (right):

https://codereview.chromium.org/2923433002/diff/20001/components/plugins/rend...
components/plugins/renderer/webview_plugin.cc:276: // - this will always be a
local frame.
I am not sure if the comment above is sufficient - should I also justify that
the main frame will never be swapped for a remote frame?  I don't know enough
about this class to come up with the justification... :-/

[dcheng, 2017-06-05 18:20:51]

LGTM

https://codereview.chromium.org/2923433002/diff/20001/components/plugins/rend...
File components/plugins/renderer/webview_plugin.cc (right):

https://codereview.chromium.org/2923433002/diff/20001/components/plugins/rend...
components/plugins/renderer/webview_plugin.cc:276: // - this will always be a
local frame.
On 2017/06/05 17:10:57, Łukasz A. wrote:
> I am not sure if the comment above is sufficient - should I also justify that
> the main frame will never be swapped for a remote frame?  I don't know enough
> about this class to come up with the justification... :-/

Maybe just say that WebViewHelper doesn't support OOPIF so the main frame will
always be local.

[Łukasz Anforowicz, 2017-06-05 19:02:38]

https://codereview.chromium.org/2923433002/diff/20001/components/plugins/rend...
File components/plugins/renderer/webview_plugin.cc (right):

https://codereview.chromium.org/2923433002/diff/20001/components/plugins/rend...
components/plugins/renderer/webview_plugin.cc:276: // - this will always be a
local frame.
On 2017/06/05 18:20:51, dcheng wrote:
> On 2017/06/05 17:10:57, Łukasz A. wrote:
> > I am not sure if the comment above is sufficient - should I also justify
that
> > the main frame will never be swapped for a remote frame?  I don't know
enough
> > about this class to come up with the justification... :-/
> 
> Maybe just say that WebViewHelper doesn't support OOPIF so the main frame will
> always be local.

Done.

[Łukasz Anforowicz, 2017-06-05 19:02:49]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2017-06-05 19:02:50]

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/2923433002/#ps40001
(title: "Tweaked a comment in WebViewPlugin::WebViewHelper::main_frame [as
suggested in CR feedback]")

[commit-bot: I haz the power, 2017-06-05 19:03:14]

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-05 19:14:10]

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

[commit-bot: I haz the power, 2017-06-05 19:14:11]

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

[Łukasz Anforowicz, 2017-06-05 19:43:10]

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

[Łukasz Anforowicz, 2017-06-05 19:43:11]

tommycli@ can you PTAL at the changes in components/plugins/ ?

[tommycli, 2017-06-06 16:32:10]

plugins changes look harmless lgtm

[Łukasz Anforowicz, 2017-06-06 16:57:00]

Description was changed from

==========
Move ExecuteScript method from WebFrame to WebLocalFrame.

Many callers already were calling through WebLocalFrame.

For 4 callers that can't guarantee at compile-time that the target frame
is a local frame, this CL:
- preserves the NOTREACHED() / no-op behavior (in case of
  components/plugins/renderer/loadable_plugin_placeholder.cc)
- or does the same, but upgrading NOTREACHED() to a CHECK()
  (in case of content/shell/test_runner/test_runner.cc)

No intended behavior change.

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

to

==========
Move ExecuteScript method from WebFrame to WebLocalFrame.

Many callers already were calling through WebLocalFrame.

For 4 callers that can't guarantee at compile-time that the target frame
is a local frame, this CL:
- preserves the NOTREACHED() / no-op behavior (in case of
  components/plugins/renderer/loadable_plugin_placeholder.cc)
- or does the same, but upgrading NOTREACHED() to a CHECK()
  (in case of content/shell/test_runner/test_runner.cc)

No intended behavior change.

BUG=416660
TBR=mattm@chromium.org, dcblack@chromium.org, scottbyer@chromium.org,
avi@chromium.org
==========

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

lukasza@chromium.org changed reviewers:
+ avi@chromium.org, dcblack@chromium.org, mattm@chromium.org,
scottbyer@chromium.org

[Łukasz Anforowicz, 2017-06-06 16:58:20]

I'll TBR the rest of the CL (which just changes WebFrame to WebLocalFrame in a
few places - if such change compiles it should be non-controversial I think; 
the only exception to this pattern is in
content/shell/test_runner/test_runner.cc, but the changes there mimic other
functions in this file so I think dcheng@'s stamp of approval should be
sufficient).

mattm@ for chrome/renderer/safe_browsing/
dcblack@ for chrome/renderer/searchbox/
scottbyer@ for components/printing/
avi@ for content/

[Łukasz Anforowicz, 2017-06-06 16:58:31]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2017-06-06 16:58:56]

CQ is trying da patch.

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

[Avi (use Gerrit), 2017-06-06 17:04:02]

lgtm

stamp

[commit-bot: I haz the power, 2017-06-06 17:46:49]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496768311422970,
"parent_rev": "821895efec24b5b1815b2c0b11a9bed1075b3433", "commit_rev":
"bb4fe6d5aa4d9584944c2223dcd7f3b92da42e9a"}

[commit-bot: I haz the power, 2017-06-06 17:47:00]

Description was changed from

==========
Move ExecuteScript method from WebFrame to WebLocalFrame.

Many callers already were calling through WebLocalFrame.

For 4 callers that can't guarantee at compile-time that the target frame
is a local frame, this CL:
- preserves the NOTREACHED() / no-op behavior (in case of
  components/plugins/renderer/loadable_plugin_placeholder.cc)
- or does the same, but upgrading NOTREACHED() to a CHECK()
  (in case of content/shell/test_runner/test_runner.cc)

No intended behavior change.

BUG=416660
TBR=mattm@chromium.org, dcblack@chromium.org, scottbyer@chromium.org,
avi@chromium.org
==========

to

==========
Move ExecuteScript method from WebFrame to WebLocalFrame.

Many callers already were calling through WebLocalFrame.

For 4 callers that can't guarantee at compile-time that the target frame
is a local frame, this CL:
- preserves the NOTREACHED() / no-op behavior (in case of
  components/plugins/renderer/loadable_plugin_placeholder.cc)
- or does the same, but upgrading NOTREACHED() to a CHECK()
  (in case of content/shell/test_runner/test_runner.cc)

No intended behavior change.

BUG=416660
TBR=mattm@chromium.org, dcblack@chromium.org, scottbyer@chromium.org,
avi@chromium.org

Review-Url: https://codereview.chromium.org/2923433002
Cr-Commit-Position: refs/heads/master@{#477333}
Committed:
https://chromium.googlesource.com/chromium/src/+/bb4fe6d5aa4d9584944c2223dcd7...
==========

[commit-bot: I haz the power, 2017-06-06 17:47:03]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/bb4fe6d5aa4d9584944c2223dcd7...

[mattm, 2017-06-06 19:37:52]

safe_browsing lgtm

[Scott Byer, 2017-06-07 15:48:50]

On 2017/06/06 at 19:37:52, mattm wrote:
> safe_browsing lgtm

printing lgtm