Update resource requests to remove redundant frame ID.

Update resource requests to remove redundant frame ID.

BUG=314791
TEST=No behavior change.
R=battre@chromium.org, nasko@chromium.org
TBR=dcheng@chromium.org, jochen@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=254566

[Charlie Reis, 2014-02-28 23:26:49]

Nasko: This is the next big step in removing frame IDs.  I took a break when I
hit OnCrossSiteResponse, since that balloons into a big CL of its own.  The plan
is to convert everyone over to using routing IDs (aka render_frame_ids).  Later,
we can look into whether we want to expose browser-global FrameTreeNode IDs to
WebContentsObservers and extension APIs (in a separate bug).

Daniel: Mostly FYI, but please review the restored DCHECKs in
render_frame_impl.cc.

https://codereview.chromium.org/184873004/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/184873004/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.h:331: // TODO(creis):
Change to render_frame_id, then move this to RenderFrameHost.
I'll move this frame_id to render_frame_id in the next CL, along with all that
it implies for RenderFrameHostManager + RequestTransferURL, etc.

https://codereview.chromium.org/184873004/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/184873004/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:858: DCHECK(!frame_ || frame_ == frame);
Daniel: You're right that these two {C|c}reatePlugin checks seem to work now. 
Thanks for fixing it!

[dcheng, 2014-02-28 23:28:21]

https://codereview.chromium.org/184873004/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/184873004/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:858: DCHECK(!frame_ || frame_ == frame);
On 2014/02/28 23:26:49, Charlie Reis wrote:
> Daniel: You're right that these two {C|c}reatePlugin checks seem to work now. 
> Thanks for fixing it!

Random question: Why would frame_ be NULL here? Is that, in fact, a problem?

[Charlie Reis, 2014-02-28 23:31:11]

https://codereview.chromium.org/184873004/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/184873004/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:858: DCHECK(!frame_ || frame_ == frame);
On 2014/02/28 23:28:22, dcheng wrote:
> On 2014/02/28 23:26:49, Charlie Reis wrote:
> > Daniel: You're right that these two {C|c}reatePlugin checks seem to work
now. 
> > Thanks for fixing it!
> 
> Random question: Why would frame_ be NULL here? Is that, in fact, a problem?

I used that in all the other checks because an unpredictably large number of
WebFrameClient methods could be called after the WebFrame was created and before
it was assigned to frame_.  I don't know if this is one of them-- should I
remove that condition?

[dcheng, 2014-02-28 23:32:53]

On 2014/02/28 23:31:11, Charlie Reis wrote:
>
https://codereview.chromium.org/184873004/diff/20001/content/renderer/render_...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/184873004/diff/20001/content/renderer/render_...
> content/renderer/render_frame_impl.cc:858: DCHECK(!frame_ || frame_ == frame);
> On 2014/02/28 23:28:22, dcheng wrote:
> > On 2014/02/28 23:26:49, Charlie Reis wrote:
> > > Daniel: You're right that these two {C|c}reatePlugin checks seem to work
> now. 
> > > Thanks for fixing it!
> > 
> > Random question: Why would frame_ be NULL here? Is that, in fact, a problem?
> 
> I used that in all the other checks because an unpredictably large number of
> WebFrameClient methods could be called after the WebFrame was created and
before
> it was assigned to frame_.  I don't know if this is one of them-- should I
> remove that condition?

Hmm... I don't have a strong opinion one way or another. Of course, if it passes
tests, then I guess it'd be nice to have.

[nasko, 2014-02-28 23:58:37]

LGTM

[Charlie Reis, 2014-03-01 00:12:42]

Thanks.  Daniel, PTAL.

battre: Can you review web_request_api.cc?

jochen: Looks like you're OOO, so I'll TBR you for web_navigation_api.cc.  We
can investigate whether we want to go any deeper into the Web Navigation API
when you return.

[battre, 2014-03-03 11:35:28]

Can you give ma a quick update on the difference between frameID and
renderFrameID? Did they have different lifetimes or anything like that? Do the
values of either get re-used? I just want to see whether I can see any
implications on the web request API.

Thanks,
Dominic

https://codereview.chromium.org/184873004/diff/30001/content/public/browser/r...
File content/public/browser/resource_request_info.h (left):

https://codereview.chromium.org/184873004/diff/30001/content/public/browser/r...
content/public/browser/resource_request_info.h:74: // True if GetFrameID()
represents a main frame in the RenderView.
This comment needs to be updated.

https://codereview.chromium.org/184873004/diff/30001/content/public/browser/r...
content/public/browser/resource_request_info.h:81: virtual bool
ParentIsMainFrame() const = 0;
This comment and function need to be updated.

[Charlie Reis, 2014-03-03 17:14:43]

On 2014/03/03 11:35:28, battre wrote:
> Can you give ma a quick update on the difference between frameID and
> renderFrameID? Did they have different lifetimes or anything like that? Do the
> values of either get re-used? I just want to see whether I can see any
> implications on the web request API.
> 
> Thanks,
> Dominic

Sure.  Frame IDs used to be allocated by Blink (in WebFrameImpl).  Now we use
the routing ID of the RenderFrame{Host}.

In both cases, the ID is specific to a renderer process, and could conflict with
IDs from other renderer processes.  The main observable difference is that
RenderFrameHost routing IDs come from the same space as RenderViewHost routing
IDs, so while you might have seen [1, 2, 3] for consecutive frames before, now
you might see [1, 3, 4] (with 2 allocated for the RenderViewHost).  I don't
think the IDs are otherwise reused.

(We're considering a later shift to using browser-global FrameTreeNode IDs for
observers and APIs, but we'll discuss that with you and other API owners to see
if that's beneficial or not.  The main advantage is that they would never
conflict between renderer processes.)

Hope that helps-- I'll upload a new patch with the changes in a minute.

Charlie

https://codereview.chromium.org/184873004/diff/30001/content/public/browser/r...
File content/public/browser/resource_request_info.h (left):

https://codereview.chromium.org/184873004/diff/30001/content/public/browser/r...
content/public/browser/resource_request_info.h:74: // True if GetFrameID()
represents a main frame in the RenderView.
On 2014/03/03 11:35:28, battre wrote:
> This comment needs to be updated.

Good catch.  Fixing now.

https://codereview.chromium.org/184873004/diff/30001/content/public/browser/r...
content/public/browser/resource_request_info.h:81: virtual bool
ParentIsMainFrame() const = 0;
On 2014/03/03 11:35:28, battre wrote:
> This comment and function need to be updated.

I'll update the comment to say GetParentRenderFrameID(), but
"ParentIsMainFrame()" is still an accurate function name-- it still returns
whether the parent frame is the main frame.

[Charlie Reis, 2014-03-03 17:21:25]

dcheng / battre: PTAL.

https://codereview.chromium.org/184873004/diff/30001/content/public/browser/r...
File content/public/browser/resource_request_info.h (left):

https://codereview.chromium.org/184873004/diff/30001/content/public/browser/r...
content/public/browser/resource_request_info.h:74: // True if GetFrameID()
represents a main frame in the RenderView.
On 2014/03/03 17:14:44, Charlie Reis wrote:
> On 2014/03/03 11:35:28, battre wrote:
> > This comment needs to be updated.
> 
> Good catch.  Fixing now.

Done.

https://codereview.chromium.org/184873004/diff/30001/content/public/browser/r...
content/public/browser/resource_request_info.h:81: virtual bool
ParentIsMainFrame() const = 0;
On 2014/03/03 17:14:44, Charlie Reis wrote:
> On 2014/03/03 11:35:28, battre wrote:
> > This comment and function need to be updated.
> 
> I'll update the comment to say GetParentRenderFrameID(), but
> "ParentIsMainFrame()" is still an accurate function name-- it still returns
> whether the parent frame is the main frame.

Done.

[battre, 2014-03-03 18:02:31]

LGTM @ web_request_api.cc

[Charlie Reis, 2014-03-03 19:07:01]

dcheng: I'll move you to TBR for the DCHECK in render_frame_impl.cc, since it
matches what you requested and passes the tests.

[Charlie Reis, 2014-03-03 19:07:08]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2014-03-03 19:09:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/184873004/50001

[commit-bot: I haz the power, 2014-03-03 19:41:43]

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

[commit-bot: I haz the power, 2014-03-03 19:41:44]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests,
check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, events_unittests, gfx_unittests,
google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests,
jingle_unittests, media_unittests, mini_installer_test, nacl_integration,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Charlie Reis, 2014-03-03 21:29:40]

Committed patchset #3 manually as r254566 (presubmit successful).

[jochen (gone - plz use gerrit), 2014-03-10 12:58:02]

lgtm