Fix ChromeRenderViewObserver::DidStopLoading to be OOPIF compatible.

Fix ChromeRenderViewObserver::DidStopLoading to be OOPIF compatible.

BUG=442955
Committed: https://crrev.com/3c529153c2aecee794884b82933df75cd55a4c6d
Cr-Commit-Position: refs/heads/master@{#308709}

[nasko, 2014-12-16 23:24:20]

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

[nasko, 2014-12-16 23:24:20]

Hi Lei,
Can you review this CL for me? The assumption that the top-level frame has a
document is not true with out-of-process iframes, so this CL fixes this in
ChromeRenderViewObserver::DidStopLoading.
Thanks in advance!
Nasko

[Lei Zhang, 2014-12-16 23:46:58]

https://codereview.chromium.org/807583006/diff/1/chrome/renderer/chrome_rende...
File chrome/renderer/chrome_render_view_observer.cc (right):

https://codereview.chromium.org/807583006/diff/1/chrome/renderer/chrome_rende...
chrome/renderer/chrome_render_view_observer.cc:353: CapturePageInfoLater(
I think you want this to be for WebLocalFrame only as well? This eventually goes
to ChromeRenderViewObserver::CapturePageInfo(), which currently first checks and
ignores WebRemoteFrames.

[nasko, 2014-12-16 23:48:59]

https://codereview.chromium.org/807583006/diff/1/chrome/renderer/chrome_rende...
File chrome/renderer/chrome_render_view_observer.cc (right):

https://codereview.chromium.org/807583006/diff/1/chrome/renderer/chrome_rende...
chrome/renderer/chrome_render_view_observer.cc:353: CapturePageInfoLater(
On 2014/12/16 23:46:57, Lei Zhang wrote:
> I think you want this to be for WebLocalFrame only as well? This eventually
goes
> to ChromeRenderViewObserver::CapturePageInfo(), which currently first checks
and
> ignores WebRemoteFrames.

I'm not certain and the CapturePageInfo already has a check for remote frames,
which is tagged separately. Do you think it is worth just returning early then?

[Lei Zhang, 2014-12-17 00:14:20]

https://codereview.chromium.org/807583006/diff/1/chrome/renderer/chrome_rende...
File chrome/renderer/chrome_render_view_observer.cc (right):

https://codereview.chromium.org/807583006/diff/1/chrome/renderer/chrome_rende...
chrome/renderer/chrome_render_view_observer.cc:353: CapturePageInfoLater(
On 2014/12/16 23:48:59, nasko wrote:
> On 2014/12/16 23:46:57, Lei Zhang wrote:
> > I think you want this to be for WebLocalFrame only as well? This eventually
> goes
> > to ChromeRenderViewObserver::CapturePageInfo(), which currently first checks
> and
> > ignores WebRemoteFrames.
> 
> I'm not certain and the CapturePageInfo already has a check for remote frames,
> which is tagged separately. Do you think it is worth just returning early
then?

It looks like creis@ has plans to move the CapturePageInfo() code to the browser
process eventually, so I think the whole thing can go inside the if
(isWebLocalFrame()) block. The HasRefreshMetaTag() and CapturePageInfoLater()
calls are related, so it feels weird to separate them.

I don't know if the OSDD portion is correct. Do we care about OSDD tags in
remote frames?

[nasko, 2014-12-17 00:25:07]

On 2014/12/17 00:14:20, Lei Zhang wrote:
>
https://codereview.chromium.org/807583006/diff/1/chrome/renderer/chrome_rende...
> File chrome/renderer/chrome_render_view_observer.cc (right):
> 
>
https://codereview.chromium.org/807583006/diff/1/chrome/renderer/chrome_rende...
> chrome/renderer/chrome_render_view_observer.cc:353: CapturePageInfoLater(
> On 2014/12/16 23:48:59, nasko wrote:
> > On 2014/12/16 23:46:57, Lei Zhang wrote:
> > > I think you want this to be for WebLocalFrame only as well? This
eventually
> > goes
> > > to ChromeRenderViewObserver::CapturePageInfo(), which currently first
checks
> > and
> > > ignores WebRemoteFrames.
> > 
> > I'm not certain and the CapturePageInfo already has a check for remote
frames,
> > which is tagged separately. Do you think it is worth just returning early
> then?
> 
> It looks like creis@ has plans to move the CapturePageInfo() code to the
browser
> process eventually, so I think the whole thing can go inside the if
> (isWebLocalFrame()) block. The HasRefreshMetaTag() and CapturePageInfoLater()
> calls are related, so it feels weird to separate them.

Sure. I've changed it to return early then, since the whole method doesn't apply
to remote frames.

> I don't know if the OSDD portion is correct. Do we care about OSDD tags in
> remote frames?

Remote frames don't have a document associated with them, they are just a
placeholder/proxy. The local frame hosting the real document will take care of
processing the OSDD tag.

[Lei Zhang, 2014-12-17 00:30:04]

lgtm

[nasko, 2014-12-17 00:30:34]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-12-17 00:31:21]

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

[commit-bot: I haz the power, 2014-12-17 01:19:52]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-12-17 01:21:30]

Patchset 2 (id:??) landed as
https://crrev.com/3c529153c2aecee794884b82933df75cd55a4c6d
Cr-Commit-Position: refs/heads/master@{#308709}