Move GetDocument method from WebFrame to WebLocalFrame.

Move GetDocument method from WebFrame to WebLocalFrame.

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

[Łukasz Anforowicz, 2017-06-14 23:42:41]

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

[commit-bot: I haz the power, 2017-06-14 23:43:08]

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-14 23:58:32]

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

[commit-bot: I haz the power, 2017-06-14 23:58:45]

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 00:25:57]

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

[commit-bot: I haz the power, 2017-06-15 00:25:58]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Łukasz Anforowicz, 2017-06-15 03:45:32]

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

[commit-bot: I haz the power, 2017-06-15 03:45:41]

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 04:17:52]

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

[commit-bot: I haz the power, 2017-06-15 04:17:54]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

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

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

[commit-bot: I haz the power, 2017-06-15 13:54:35]

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 15:48:08]

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

[commit-bot: I haz the power, 2017-06-15 15:48:09]

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

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

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

[commit-bot: I haz the power, 2017-06-15 16:32:52]

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 16:36:50]

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

[commit-bot: I haz the power, 2017-06-15 16:36:51]

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

[Łukasz Anforowicz, 2017-06-15 16:53:50]

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

[commit-bot: I haz the power, 2017-06-15 16:54:13]

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 16:57:54]

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

[commit-bot: I haz the power, 2017-06-15 16:57:55]

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

[Łukasz Anforowicz, 2017-06-15 17:42:12]

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

[commit-bot: I haz the power, 2017-06-15 17:42:43]

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 20:09:59]

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

[commit-bot: I haz the power, 2017-06-15 20:10:00]

Dry run: This issue passed the CQ dry run.

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

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

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

dcheng@, could you PTAL?

https://codereview.chromium.org/2928033002/diff/160001/android_webview/render...
File android_webview/renderer/aw_render_frame_ext.cc (right):

https://codereview.chromium.org/2928033002/diff/160001/android_webview/render...
android_webview/renderer/aw_render_frame_ext.cc:192: DCHECK(frame &&
!frame->Parent());
1) AwRenderFrameExt is a RenderFrameObserver
2) AwViewMsg_DocumentHasImages is only sent to the main frame (see
AwRenderViewHostExt::DocumentHasImages)

Therefore, rather than look at GetWebView->MainFrame(), after this CL we just
look at render_frame()->GetWebFrame() and assert that this is a main frame.

PS. I guess the IPC name should be spelled Av*Frame*Msg_DocumentHasImages.  I'd
rather not do it in this CL (because this is slightly complicated -
AwRenderFrameExt is a RenderFrameObserver, *but* some of the IPCs have clearly a
view scope [e.g. AwViewMsg_SetTextZoomFactor]).

https://codereview.chromium.org/2928033002/diff/160001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2928033002/diff/160001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:787: app_url =
frame->Top()->ToWebLocalFrame()->GetDocument().Url();
This isn't worse than the current code, right?

https://codereview.chromium.org/2928033002/diff/160001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.h (right):

https://codereview.chromium.org/2928033002/diff/160001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.h:163: content::RenderFrame*
render_frame) override;
In theory, I could change the type of the 2nd parameter to WebLocalFrame, but
implementations can always get the 2nd argument from the 1st argument:
|render_frame->GetWebFrame()|.  This is safe/valid, because the only caller of
this virtual method is RenderFrameImpl::CreateWorkerContentSettingsClientProxy,
which (before this CL) would always pass equivalent of |this->GetWebFrame()| as
the last argument.

https://codereview.chromium.org/2928033002/diff/160001/chrome/renderer/safe_b...
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc (right):

https://codereview.chromium.org/2928033002/diff/160001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc:395: return
frame->ToWebLocalFrame()->GetDocument();
Skipping remote frames seems okay here?

https://codereview.chromium.org/2928033002/diff/160001/components/autofill/co...
File components/autofill/content/renderer/form_autofill_util.cc (right):

https://codereview.chromium.org/2928033002/diff/160001/components/autofill/co...
components/autofill/content/renderer/form_autofill_util.cc:8: #include <limits>
This CL contains some opportunistic fixes for issues found by "git cl lint"
(IWYU, unnecessary whitespace or semicolons, DCHECK_GT instead of DCHECK(a > b),
etc.).

https://codereview.chromium.org/2928033002/diff/160001/content/renderer/dom_s...
File content/renderer/dom_serializer_browsertest.cc (right):

https://codereview.chromium.org/2928033002/diff/160001/content/renderer/dom_s...
content/renderer/dom_serializer_browsertest.cc:116:
DCHECK(frame->IsWebLocalFrame());
Seems okay to do in a test.

https://codereview.chromium.org/2928033002/diff/160001/content/renderer/savab...
File content/renderer/savable_resources.cc (right):

https://codereview.chromium.org/2928033002/diff/160001/content/renderer/savab...
content/renderer/savable_resources.cc:40: bool
DoesFrameContainHtmlDocument(WebFrame* web_frame,
There is no const version of ToWebLocalFrame, so this CL works around this by
instead removing the constness of the parameter here.

https://codereview.chromium.org/2928033002/diff/160001/extensions/renderer/sc...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/2928033002/diff/160001/extensions/renderer/sc...
extensions/renderer/script_context.cc:315: blink::WebFrame* parent = frame;
The is no const-aware version of ToWebLocalFrame method, so I had to change
constness of the |parent| variable (and it cascaded into changing constness of
parameters in ScriptContext::GetEffectiveDocumentURL above and also in
extensions/renderer/script_context_set.cc and
extensions/renderer/script_context_browsertest.cc).

https://codereview.chromium.org/2928033002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/exported/WebAXObject.cpp (left):

https://codereview.chromium.org/2928033002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/exported/WebAXObject.cpp:1570: }
This is now inlined into content/shell/test_runner/accessibility_controller.cc

[dcheng, 2017-06-15 23:17:00]

https://codereview.chromium.org/2928033002/diff/160001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2928033002/diff/160001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:787: app_url =
frame->Top()->ToWebLocalFrame()->GetDocument().Url();
On 2017/06/15 20:15:59, Łukasz A. wrote:
> This isn't worse than the current code, right?

Doesn't seem worse =/

Didn't we have to "fix" this by passing around top document URL / origin for
plugins or something?

https://codereview.chromium.org/2928033002/diff/160001/chrome/renderer/safe_b...
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc (right):

https://codereview.chromium.org/2928033002/diff/160001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc:395: return
frame->ToWebLocalFrame()->GetDocument();
On 2017/06/15 20:15:59, Łukasz A. wrote:
> Skipping remote frames seems okay here?

Yeah. That being said, I'm quite skeptical of the IsNull() check on WebDocument
here--this shouldn't really be a state that's visible to consumers of the Blink
public API, unless they're doing things at weird times.

You can add a TODO(dcheng): to play around with this.

https://codereview.chromium.org/2928033002/diff/160001/content/renderer/savab...
File content/renderer/savable_resources.cc (right):

https://codereview.chromium.org/2928033002/diff/160001/content/renderer/savab...
content/renderer/savable_resources.cc:40: bool
DoesFrameContainHtmlDocument(WebFrame* web_frame,
On 2017/06/15 20:15:59, Łukasz A. wrote:
> There is no const version of ToWebLocalFrame, so this CL works around this by
> instead removing the constness of the parameter here.

;-)

https://codereview.chromium.org/2928033002/diff/160001/content/shell/renderer...
File content/shell/renderer/layout_test/blink_test_runner.cc (right):

https://codereview.chromium.org/2928033002/diff/160001/content/shell/renderer...
content/shell/renderer/layout_test/blink_test_runner.cc:1051:
url::Origin(main_frame->GetSecurityOrigin()).GetURL());
I'm a little surprised this works -- wouldn't the origin be unique here
normally?

https://codereview.chromium.org/2928033002/diff/160001/extensions/renderer/sc...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/2928033002/diff/160001/extensions/renderer/sc...
extensions/renderer/script_context.cc:325: parent_document =
blink::WebDocument();
Nit: no need to set parent_document here, it's already default constructed on
line 316. Maybe move the declaration down here for clarity though?

[Łukasz Anforowicz, 2017-06-16 17:44:02]

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

[commit-bot: I haz the power, 2017-06-16 17:44:22]

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-16 17:48:21]

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

[commit-bot: I haz the power, 2017-06-16 17:48:22]

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

[Łukasz Anforowicz, 2017-06-16 18:38:42]

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

[commit-bot: I haz the power, 2017-06-16 18:39: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-16 19:39:24]

Thanks for reviewing - can you take another look please?

https://codereview.chromium.org/2928033002/diff/160001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2928033002/diff/160001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:787: app_url =
frame->Top()->ToWebLocalFrame()->GetDocument().Url();
On 2017/06/15 23:17:00, dcheng wrote:
> On 2017/06/15 20:15:59, Łukasz A. wrote:
> > This isn't worse than the current code, right?
> 
> Doesn't seem worse =/

Ack.
> 
> Didn't we have to "fix" this by passing around top document URL / origin for
> plugins or something?

I don't know.

- The issue with the code above has been "known" for a while. (e.g. see
https://crbug.com/466297#c11 from July 2016 which predates
https://crbug.com/728295 by a bit).

- I've tried to look at other callers of WebFrame::Top and I don't see anything
obviously applicable to the code above. 
ChromeContentRendererClient::OverrideCreatePlugin sends the top security origin
to the browser when asking for ChromeViewHostMsg_GetPluginInfo.  Simular
situation happens in ChromePluginPlaceholder::PluginListChanged and
RenderFrameImpl::CreatePlugin.

- Also note that |app_url|'s path gets later inspected by
AppCategorizer::IsHangoutsUrl.

https://codereview.chromium.org/2928033002/diff/160001/chrome/renderer/safe_b...
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc (right):

https://codereview.chromium.org/2928033002/diff/160001/chrome/renderer/safe_b...
chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc:395: return
frame->ToWebLocalFrame()->GetDocument();
On 2017/06/15 23:17:00, dcheng wrote:
> On 2017/06/15 20:15:59, Łukasz A. wrote:
> > Skipping remote frames seems okay here?
> 
> Yeah. That being said, I'm quite skeptical of the IsNull() check on
WebDocument
> here--this shouldn't really be a state that's visible to consumers of the
Blink
> public API, unless they're doing things at weird times.
> 
> You can add a TODO(dcheng): to play around with this.

Done.

https://codereview.chromium.org/2928033002/diff/160001/content/shell/renderer...
File content/shell/renderer/layout_test/blink_test_runner.cc (right):

https://codereview.chromium.org/2928033002/diff/160001/content/shell/renderer...
content/shell/renderer/layout_test/blink_test_runner.cc:1051:
url::Origin(main_frame->GetSecurityOrigin()).GetURL());
On 2017/06/15 23:17:00, dcheng wrote:
> I'm a little surprised this works -- wouldn't the origin be unique here
> normally?

Oops - thanks for catching this.  I didn't catch this because I haven't run
third_party/WebKit/Tools/Scripts/run-webkit-tests with --enable-leak-detection
flag (and apparently CQ also doesn't do this).

I've rewritten the DCHECK to only trigger if the main frame is local.

https://codereview.chromium.org/2928033002/diff/160001/content/shell/renderer...
content/shell/renderer/layout_test/blink_test_runner.cc:1054:
leak_detector_->TryLeakDetection(main_frame);
FWIW, the argument (i.e. |main_frame|) above is only ever used in
WebLeakDetectorImpl::PrepareForLeakDetection to stop the spellchecker if the
frame is local.

- I think this means that OnTryLeakDetection is applicable also in cases when
the main frame is remote.

- There might be something fishy about only stopping the spellchecker in the
main/local frame.  Hmmm...

https://codereview.chromium.org/2928033002/diff/160001/extensions/renderer/sc...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/2928033002/diff/160001/extensions/renderer/sc...
extensions/renderer/script_context.cc:325: parent_document =
blink::WebDocument();
On 2017/06/15 23:17:00, dcheng wrote:
> Nit: no need to set parent_document here, it's already default constructed on
> line 316. Maybe move the declaration down here for clarity though?

We might need to reset |parent_document| on subsequent iterations.  And moving
the declaration here won't work because this variable is used from 1) the loop
condition and 2) from the if below (on line 331).

I've tweaked the code slightly, so maybe it looks a bit nicer now.

[commit-bot: I haz the power, 2017-06-16 20:49:14]

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

[commit-bot: I haz the power, 2017-06-16 20:49:15]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-06-20 09:02:21]

lgtm

https://codereview.chromium.org/2928033002/diff/160001/content/shell/renderer...
File content/shell/renderer/layout_test/blink_test_runner.cc (right):

https://codereview.chromium.org/2928033002/diff/160001/content/shell/renderer...
content/shell/renderer/layout_test/blink_test_runner.cc:1054:
leak_detector_->TryLeakDetection(main_frame);
On 2017/06/16 19:39:24, Łukasz A. wrote:
> FWIW, the argument (i.e. |main_frame|) above is only ever used in
> WebLeakDetectorImpl::PrepareForLeakDetection to stop the spellchecker if the
> frame is local.
> 
> - I think this means that OnTryLeakDetection is applicable also in cases when
> the main frame is remote.
> 
> - There might be something fishy about only stopping the spellchecker in the
> main/local frame.  Hmmm...

Yeah, seems odd. We probably need to go through and audit our various usage of
IsWebLocalFrame / ToWebLocalFrame at some point...

https://codereview.chromium.org/2928033002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2928033002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:7731: WebLocalFrame* frame
= web_view_helper.WebView()->MainFrameImpl();
Nit: this can just be web_view_helper.LocalMainFrame()

(Similar for other changes that introduce MainFrameImpl in this CL)

[dcheng, 2017-06-20 09:03:13]

https://codereview.chromium.org/2928033002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2928033002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:7731: WebLocalFrame* frame
= web_view_helper.WebView()->MainFrameImpl();
On 2017/06/20 09:02:21, dcheng wrote:
> Nit: this can just be web_view_helper.LocalMainFrame()
> 
> (Similar for other changes that introduce MainFrameImpl in this CL)

(Of course this only applies when there's an accessible WebViewHelper)

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

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

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

alexmos@, could you PTAL at //content?

https://codereview.chromium.org/2928033002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2928033002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:7731: WebLocalFrame* frame
= web_view_helper.WebView()->MainFrameImpl();
On 2017/06/20 09:03:13, dcheng wrote:
> On 2017/06/20 09:02:21, dcheng wrote:
> > Nit: this can just be web_view_helper.LocalMainFrame()
> > 
> > (Similar for other changes that introduce MainFrameImpl in this CL)
> 
> (Of course this only applies when there's an accessible WebViewHelper)

This cleanup opportunity impacts more lines than the ones touched in the CL
under review, so I've spun off a separate CL for getting rid of unnecessary
helper.WebView()->MainFrameImpl() calls: 
https://codereview.chromium.org/2945093002.

[Łukasz Anforowicz, 2017-06-20 18:36:59]

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

[commit-bot: I haz the power, 2017-06-20 18:37:20]

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-20 20:56:22]

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

[commit-bot: I haz the power, 2017-06-20 20:56:23]

Dry run: This issue passed the CQ dry run.

[alexmos, 2017-06-20 22:44:11]

Awesome cleanup, content/ LGTM!

https://codereview.chromium.org/2928033002/diff/220001/chrome/browser/web_app...
File chrome/browser/web_applications/web_app_unittest.cc (right):

https://codereview.chromium.org/2928033002/diff/220001/chrome/browser/web_app...
chrome/browser/web_applications/web_app_unittest.cc:24: using
content::RenderViewHostTester;
nit: not needed anymore?

https://codereview.chromium.org/2928033002/diff/220001/chrome/browser/web_app...
chrome/browser/web_applications/web_app_unittest.cc:50: rvh()->GetMainFrame(),
nit: can shorten to main_rfh() (like above)

https://codereview.chromium.org/2928033002/diff/220001/content/renderer/peppe...
File content/renderer/pepper/pepper_file_system_host.cc (left):

https://codereview.chromium.org/2928033002/diff/220001/content/renderer/peppe...
content/renderer/pepper/pepper_file_system_host.cc:135: const GURL& url =
view->GetWebView()->MainFrame()->GetDocument().Url();
Wow, this seems like a bug we hadn't noticed before, though it doesn't look like
we observed any crashes for it.  Thanks for fixing it (and the one below)!

https://codereview.chromium.org/2928033002/diff/220001/content/shell/test_run...
File content/shell/test_runner/test_runner.cc (right):

https://codereview.chromium.org/2928033002/diff/220001/content/shell/test_run...
content/shell/test_runner/test_runner.cc:2109:
CHECK(main_view_->MainFrame()->IsWebLocalFrame())
Is this something that really shouldn't work outside of local main frames, or
have we just not needed to call testRunner.queueLoad() in a cross-site subframe?
 (If the latter, it might be worthwhile to add a TODO to fix it if any layout
tests ever need this in an OOPIF)

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

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

[commit-bot: I haz the power, 2017-06-20 23:15:49]

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 23:19:15]

https://codereview.chromium.org/2928033002/diff/220001/chrome/browser/web_app...
File chrome/browser/web_applications/web_app_unittest.cc (right):

https://codereview.chromium.org/2928033002/diff/220001/chrome/browser/web_app...
chrome/browser/web_applications/web_app_unittest.cc:24: using
content::RenderViewHostTester;
On 2017/06/20 22:44:11, alexmos wrote:
> nit: not needed anymore?

Done.

https://codereview.chromium.org/2928033002/diff/220001/chrome/browser/web_app...
chrome/browser/web_applications/web_app_unittest.cc:50: rvh()->GetMainFrame(),
On 2017/06/20 22:44:11, alexmos wrote:
> nit: can shorten to main_rfh() (like above)

Done.

https://codereview.chromium.org/2928033002/diff/220001/content/shell/test_run...
File content/shell/test_runner/test_runner.cc (right):

https://codereview.chromium.org/2928033002/diff/220001/content/shell/test_run...
content/shell/test_runner/test_runner.cc:2109:
CHECK(main_view_->MainFrame()->IsWebLocalFrame())
On 2017/06/20 22:44:11, alexmos wrote:
> Is this something that really shouldn't work outside of local main frames, or
> have we just not needed to call testRunner.queueLoad() in a cross-site
subframe?
>  (If the latter, it might be worthwhile to add a TODO to fix it if any layout
> tests ever need this in an OOPIF)

It is the latter (this just wasn't needed yet).  Note that under
content/shell/test_runner there are quite a few remaining assumptions that the
main frame is local:
https://cs.chromium.org/search/?q=ToWebLocalFrame+file:%5Esrc/content/shell/t...

Done (here and in 6 other locations under content/shell/test_runner/).  In most
places, I've just added a TODO, without opening a new, specific bug (except for
https://crbug.com/667551 reference in pixel_dump.cc).

[Łukasz Anforowicz, 2017-06-20 23:25:47]

lukasza@chromium.org changed reviewers:
+ michaelbai@chromium.org, rdevlin.cronin@chromium.org, sebsg@chromium.org,
thakis@chromium.org

[Łukasz Anforowicz, 2017-06-20 23:25:48]

+rdevlin.cronin@ for extensions/renderer/
+sebsg@ for components/autofill/content/renderer/
+thakis@ for chrome/
+michaelbai@ for android_webview/renderer/
+jialiul@ for components/safe_browsing/renderer/

[Łukasz Anforowicz, 2017-06-20 23:26:40]

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

[Łukasz Anforowicz, 2017-06-20 23:26:41]

Really adding jialiul@ this time... :-)

[commit-bot: I haz the power, 2017-06-20 23:35:11]

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

[commit-bot: I haz the power, 2017-06-20 23:35:13]

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

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

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

[commit-bot: I haz the power, 2017-06-20 23:39:36]

Dry run: CQ is trying da patch.

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

[michaelbai, 2017-06-21 00:28:00]

michaelbai@chromium.org changed reviewers:
+ boliu@chromium.org

[michaelbai, 2017-06-21 00:28:02]

boliu@ would you like to review webview change?

[boliu, 2017-06-21 00:49:56]

android_webview lgtm

https://codereview.chromium.org/2928033002/diff/240001/android_webview/render...
File android_webview/renderer/aw_render_frame_ext.cc (right):

https://codereview.chromium.org/2928033002/diff/240001/android_webview/render...
android_webview/renderer/aw_render_frame_ext.cc:192: DCHECK(frame &&
!frame->Parent());
nit: split into two DCHECKs

[Devlin, 2017-06-21 00:57:55]

https://codereview.chromium.org/2928033002/diff/240001/extensions/renderer/sc...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/2928033002/diff/240001/extensions/renderer/sc...
extensions/renderer/script_context.cc:325: parent_document = parent &&
parent->IsWebLocalFrame()
This actually seems like it would be a behavior change for the case of an OOPIF
with an accessible parent document, right?

[commit-bot: I haz the power, 2017-06-21 02:03:35]

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

[commit-bot: I haz the power, 2017-06-21 02:03:37]

Dry run: This issue passed the CQ dry run.

[Jialiu Lin, 2017-06-21 02:20:44]

Thanks!
chrome/renderer/safe_browsing/* lgtm

[Łukasz Anforowicz, 2017-06-21 16:53:03]

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

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

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-21 16:54:35]

Devlin - I tried to address your concerns below - PTAL?

https://codereview.chromium.org/2928033002/diff/240001/android_webview/render...
File android_webview/renderer/aw_render_frame_ext.cc (right):

https://codereview.chromium.org/2928033002/diff/240001/android_webview/render...
android_webview/renderer/aw_render_frame_ext.cc:192: DCHECK(frame &&
!frame->Parent());
On 2017/06/21 00:49:56, boliu wrote:
> nit: split into two DCHECKs

Done.

https://codereview.chromium.org/2928033002/diff/240001/extensions/renderer/sc...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/2928033002/diff/240001/extensions/renderer/sc...
extensions/renderer/script_context.cc:325: parent_document = parent &&
parent->IsWebLocalFrame()
On 2017/06/21 00:57:55, Devlin wrote:
> This actually seems like it would be a behavior change for the case of an
OOPIF
> with an accessible parent document, right?

I might be missing something, but I still don't think this CL changes the
behavior here.

1. Before and after this CL the |frame| argument is a *local* frame.

2. Before and after this CL the code walks the chain of *local* parents/openers
until it finds a non-kAboutScheme URL.  Before this CL, only local
parents/openers were considered because of |!parent->GetDocument().IsNull()|
check.  Notice how before this CL WebRemoteFrameImpl::GetDocument returns a null
document and how after this CL there is no WebRemoteFrameImpl::GetDocument (to
some extent the parent_document assignment in the loop here "inlines" the old
WebRemoteFrameImpl::GetDocument's behavior).

Walking only *local* frames makes sense here - we want to find the "real"
security origin of a frame (i.e. if foo.com opened about:blank, then we want to
find foo.com) and so we don't have to consider *remote* frames (because remote
frames are always cross-site).

Does that make sense?

[commit-bot: I haz the power, 2017-06-21 17:20:25]

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

[commit-bot: I haz the power, 2017-06-21 17:20:26]

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

[Łukasz Anforowicz, 2017-06-21 18:41:44]

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

[commit-bot: I haz the power, 2017-06-21 18:42:06]

Dry run: CQ is trying da patch.

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

[Devlin, 2017-06-21 19:24:51]

extensions lgtm

https://codereview.chromium.org/2928033002/diff/240001/extensions/renderer/sc...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/2928033002/diff/240001/extensions/renderer/sc...
extensions/renderer/script_context.cc:325: parent_document = parent &&
parent->IsWebLocalFrame()
On 2017/06/21 16:54:35, Łukasz A. wrote:
> On 2017/06/21 00:57:55, Devlin wrote:
> > This actually seems like it would be a behavior change for the case of an
> OOPIF
> > with an accessible parent document, right?
> 
> I might be missing something, but I still don't think this CL changes the
> behavior here.
> 
> 1. Before and after this CL the |frame| argument is a *local* frame.
> 
> 2. Before and after this CL the code walks the chain of *local*
parents/openers
> until it finds a non-kAboutScheme URL.  Before this CL, only local
> parents/openers were considered because of |!parent->GetDocument().IsNull()|
> check.  Notice how before this CL WebRemoteFrameImpl::GetDocument returns a
null
> document and how after this CL there is no WebRemoteFrameImpl::GetDocument (to
> some extent the parent_document assignment in the loop here "inlines" the old
> WebRemoteFrameImpl::GetDocument's behavior).
> 
> Walking only *local* frames makes sense here - we want to find the "real"
> security origin of a frame (i.e. if http://foo.com opened about:blank, then we
want to
> find http://foo.com) and so we don't have to consider *remote* frames (because
remote
> frames are always cross-site).
> 
> Does that make sense?

Ah, thanks for the explanation!  This is fine then. :)  I missed that
WebRemoteFrameImpl returned an empty document.

[commit-bot: I haz the power, 2017-06-21 20:03:45]

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

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

Dry run: This issue passed the CQ dry run.

[sebsg, 2017-06-21 21:10:40]

autofill LGTM

[Nico, 2017-06-22 19:19:44]

chrome lgtm

[Łukasz Anforowicz, 2017-06-22 19:39:33]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2017-06-22 19:39:34]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
alexmos@chromium.org, boliu@chromium.org, jialiul@chromium.org
Link to the patchset: https://codereview.chromium.org/2928033002/#ps280001
(title: "Split a DCHECK in two as suggested by boliu@.")

[commit-bot: I haz the power, 2017-06-22 19:39:59]

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-23 00:00:20]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1498160373277760,
"parent_rev": "ffc80b382dda2bee490e0fd8ad0b15ea091fba74", "commit_rev":
"bedb4b202b4b909e7e1f79439cadd2842f105470"}

[commit-bot: I haz the power, 2017-06-23 00:00:37]

Description was changed from

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

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

to

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

BUG=416660

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

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

Committed patchset #15 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/bedb4b202b4b909e7e1f79439cad...