Use FrameTreeNode ID as frameId in extension APIs

Use FrameTreeNode ID as frameId in extension APIs

Use FrameTreeNode IDs instead of RenderFrame routing IDs as "frame id"
in the extension APIs (webNavigation, webRequest, extension messaging)
because FTN IDs are globally unique, and RFH IDs are not.

This extends the public content API with:
- RenderFrameHost::FromFrameTreeNodeId
- RenderFrameHost::GetFrameTreeNodeId
- WebContents::FindFrameByFrameTreeNodeId

The extension APIs are modified as follows:
- webRequest:
  * Frame IDs may be unavailable after frame removal (crbug.com/572930).
  * Blocking webRequest handlers may be slower than before due to an
    extra IO->UI->IO hop to determine the frameId (but this happens only
    once per frame load).
- webNavigation:
  * processId is no longer required in chrome.webNavigation.getFrame,
    but marked as optional (deprecated) to make sure that the API is
    backwards-compatible.
  * frameId is constant across navigations.
- Extension messaging (chrome.runtime.connect and friends):
  * Small change for extension developers in the following scenario:
    1. Open port to tab.
    2. Accept the port by listening to onConnect *in a child frame*.
    3. Unload the document in the frame.
    Old behavior: onDisconnect was not triggered in the background until
                  the tab was reloaded or closed.
    New behavior: onDisconnect is called.
  * Extension messaging works correctly in out-of-process frames.
  * Move port lifetime management from renderer to browser.
  * Tie port lifetime to frames instead of processes.
  * Only notify frames in renderers if they have accepted the port.
  * Remove obsolete work-around for crbug.com/520303.
  * Unify open/close/postMessage logic in ChromeExtensionMessageFilter
    (crbug.com/394383#c7)
- The extension frameId logic is no longer scattered over several files,
  but resides in a single class (ExtensionApiFrameIdMap).
- IDs are now guaranteed to be -1 or higher (before this, -2 was
  occasionally seen).

Depends on https://codereview.chromium.org/1413853005/

BUG=432875
TEST=browser_tests --gtest_filter=\
ExtensionWebRequestApiTest.*:\
WebNavigationApiTest.*:\
ExtensionApiTest.Messaging*:\
ExternallyConnectableMessagingTest.*:\
ExternallyConnectableMessagingWithTlsChannelIdTest.*

Committed: https://crrev.com/3e2a0730e0dfb9c4dcbce45eea275270db900e90
Cr-Commit-Position: refs/heads/master@{#367914}

[robwu, 2015-10-29 23:37:47]

Description was changed from

==========
Use FrameTreeNode ID as frameId in extension APIs

Use FrameTreeNode IDs instead of RenderFrame routing IDs as "frame id"
in the extension APIs (webNavigation, webRequest, extension messaging)
because FTN IDs are globally unique, and RFH IDs are not.

This extends the public content API with:
- RenderFrameHost::FromFrameTreeNodeID (static)
- RenderFrameHost::FindByFrameTreeNodeID
- RenderFrameHost::GetFrameTreeNodeID

The extension APIs are modified as follows:
- webRequest:
  * No visible changes for extension developers.
  * Hop from IO to UI and back to IO thread to determine the frameId.
- webNavigation:
  * processId is no longer required in chrome.webNavigation.getFrame,
    but marked as optional (deprecated) to make sure that the API is
    backwards-compatible.
- Extension messaging (chrome.runtime.connect and friends):
  * Small change for extension developers in the following scenario:
    1. Open port to tab.
    2. Accept the port by listening to onConnect *in a child frame*.
    3. Unload the document in the frame.
    Old behavior: onDisconnect was not triggered in the background until
                  the tab was reloaded or closed.
    New behavior: onDisconnect is called.
  * Extension messaging works correctly in out-of-process frames.
  * Move port lifetime management from renderer to browser.
  * Tie port lifetime to frames instead of processes.
  * Only notify frames in renderers if they have accepted the port.
  * Remove obsolete work-around for crbug.com/520303.
  * Unify open/close/postMessage logic in ChromeExtensionMessageFilter
    (crbug.com/394383#c7)

BUG=432875
R=rdevlin.cronin@chromium.org,nasko@chromium.org
TEST=browser_tests, ExtensionWebRequestApiTest.*:WebNavigationApiTest.*
ExtensionApiTest.Messaging*:ExternallyConnectableMessagingTest.*:ExternallyConnectableMessagingWithTlsChannelIdTest.*:ExtensionApiTest.MessagingUserGesture
==========

to

==========
Use FrameTreeNode ID as frameId in extension APIs

Use FrameTreeNode IDs instead of RenderFrame routing IDs as "frame id"
in the extension APIs (webNavigation, webRequest, extension messaging)
because FTN IDs are globally unique, and RFH IDs are not.

This extends the public content API with:
- RenderFrameHost::FromFrameTreeNodeID (static)
- RenderFrameHost::FindByFrameTreeNodeID
- RenderFrameHost::GetFrameTreeNodeID

The extension APIs are modified as follows:
- webRequest:
  * No visible changes for extension developers.
  * Hop from IO to UI and back to IO thread to determine the frameId.
- webNavigation:
  * processId is no longer required in chrome.webNavigation.getFrame,
    but marked as optional (deprecated) to make sure that the API is
    backwards-compatible.
- Extension messaging (chrome.runtime.connect and friends):
  * Small change for extension developers in the following scenario:
    1. Open port to tab.
    2. Accept the port by listening to onConnect *in a child frame*.
    3. Unload the document in the frame.
    Old behavior: onDisconnect was not triggered in the background until
                  the tab was reloaded or closed.
    New behavior: onDisconnect is called.
  * Extension messaging works correctly in out-of-process frames.
  * Move port lifetime management from renderer to browser.
  * Tie port lifetime to frames instead of processes.
  * Only notify frames in renderers if they have accepted the port.
  * Remove obsolete work-around for crbug.com/520303.
  * Unify open/close/postMessage logic in ChromeExtensionMessageFilter
    (crbug.com/394383#c7)

Depends on https://codereview.chromium.org/1413853005/

BUG=432875
R=rdevlin.cronin@chromium.org,nasko@chromium.org
TEST=browser_tests, ExtensionWebRequestApiTest.*:WebNavigationApiTest.*
ExtensionApiTest.Messaging*:ExternallyConnectableMessagingTest.*:ExternallyConnectableMessagingWithTlsChannelIdTest.*
==========

[robwu, 2015-10-29 23:59:34]

rob@robwu.nl changed reviewers:
+ battre@chromium.org, creis@chromium.org

[robwu, 2015-10-29 23:59:34]

This CL has four loosely related changes. They're combined because all extension
APIs must have a common frameId.

I have grouped the changes below, please review the files listed under your
name.

creis: content/ changes:
 content/browser/frame_host/render_frame_host_impl.cc
 content/browser/frame_host/render_frame_host_impl.h
 content/public/browser/render_frame_host.h

nasko: webNavigation API
 chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
 chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc
 chrome/common/extensions/api/web_navigation.json
 chrome/common/extensions/docs/templates/intros/webNavigation.html

rdevlin.cronin: Extension messaging
(depends on https://codereview.chromium.org/1413853005/)
 chrome/browser/extensions/api/messaging/extension_message_port.cc
 chrome/browser/extensions/api/messaging/extension_message_port.h
 chrome/browser/extensions/api/messaging/message_service.cc
 chrome/browser/extensions/api/messaging/message_service.h
 chrome/browser/extensions/api/messaging/native_message_port.cc
 chrome/browser/extensions/api/messaging/native_message_port.h
 chrome/browser/renderer_host/chrome_extension_message_filter.cc
 chrome/browser/renderer_host/chrome_extension_message_filter.h
 chrome/renderer/extensions/tabs_custom_bindings.cc
 chrome/test/data/extensions/api_test/messaging/connect*
 extensions/common/extension_messages.h
 extensions/renderer/messaging_bindings.cc

battre: webRequest API
 content/browser/loader/resource_dispatcher_host_impl.cc
 content/browser/loader/resource_dispatcher_host_unittest.cc
 content/browser/loader/resource_request_info_impl.cc
 content/browser/loader/resource_request_info_impl.h
 content/public/browser/resource_request_info.h
 extensions/browser/api/web_request/web_request_api.cc
 extensions/browser/api/web_request/web_request_api.h
 extensions/browser/api/web_request/web_request_api_constants.cc
 extensions/browser/api/web_request/web_request_api_constants.h

[Devlin, 2015-10-30 01:49:39]

First run-through; haven't gotten to everything.

I actually saw this in my queue before it was ready, and so had started on patch
set 2.  I think the comments there are still relevant, but if you see something
weird, that's why.

https://codereview.chromium.org/1413543005/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/1413543005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:29:
ProcessManager::Get(port_->browser_context_)->AddObserver(this);
nit: prefer scoped observer for usage like this.

https://codereview.chromium.org/1413543005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:34: :
content::WebContentsObserver(tab), port_(port) {
This doesn't observe the process manager.  The comment makes it seem like that's
intentional, but having this behavior in the constructor is pretty weird, and
very opaque from any call site.  I think I'd just prefer to have two methods
here, TrackTabFrames and TrackExtensionProcessFrames or something like that. 
Or, if there's a concrete reason to keep the behavior separate and exclusive, 
make two classes.

https://codereview.chromium.org/1413543005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:92:
frame_tracker_() {
This will default-initialize itself; no need for this line.

https://codereview.chromium.org/1413543005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:105:
frame_tracker_.reset();
Why is the explicit destruction needed?

https://codereview.chromium.org/1413543005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:206: for
(content::RenderFrameHost* rfh : frames_) {
Hmm... This is a bit confusing, because of the way this is laid out, but I
*think* this is all just equivalent to doing WebContents::SendToAllFrames().  If
|extension_process_| is null, it means we're following the tab, in which case
|frames_| is just the set of all frames, right?  This also seems to imply we
don't actually need |frames_| at all, because it doesn't really seem to be used
in the extension_process_ case.

https://codereview.chromium.org/1413543005/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/extension_message_port.h (right):

https://codereview.chromium.org/1413543005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.h:75: void*
background_host_ptr_;  // used in IncrementLazyKeepaliveCount
There's no reason for this to be a void*.  Can you make it an ExtensionHost*?

https://codereview.chromium.org/1413543005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.h:77: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1413543005/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/1413543005/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:39: }
nit: newline

https://codereview.chromium.org/1413543005/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:42: void
RenderFrameDeleted(content::RenderFrameHost* render_frame_host)
I would have thought FrameDeleted here - are you sure RenderFrameDeleted is
correct?

https://codereview.chromium.org/1413543005/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:58: {
bracket

https://codereview.chromium.org/1413543005/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:59: DVLOG(2)
<< "OnExtensionFrameUnregistered " << port_->port_id_ << " for "
We don't usually like to commit logs (even [D]VLOGS).  A few are maybe okay, but
there are many strewn throughout this CL.  Let's trim them a bit.

https://codereview.chromium.org/1413543005/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/message_service.cc (right):

https://codereview.chromium.org/1413543005/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/message_service.cc:669: void
MessageService::ClosePortImpl(int port_id, int process_id, int routing_id,
I think style is to put function parameters in the definition on newlines.  git
cl format will say for sure.

https://codereview.chromium.org/1413543005/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/message_service.h (right):

https://codereview.chromium.org/1413543005/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/message_service.h:64: virtual bool
IsValidPort();
comment and newline, and I think I'd prefer this to be pure virtual.

https://codereview.chromium.org/1413543005/diff/40001/chrome/common/extension...
File chrome/common/extensions/docs/templates/intros/webNavigation.html (left):

https://codereview.chromium.org/1413543005/diff/40001/chrome/common/extension...
chrome/common/extensions/docs/templates/intros/webNavigation.html:109: frame is
always 0, the ID of child frames is a positive number. Once a document
If frame ID uniquely identifies the frame, it implies this needs to be updated.

https://codereview.chromium.org/1413543005/diff/40001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/1413543005/diff/40001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:224:
scoped_ptr<base::DictionaryValue> dict) {
Seems like this should be a void method that takes in a DictionaryValue*?

https://codereview.chromium.org/1413543005/diff/40001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:226: DCHECK(dict.get());
nit: s/.get()//

https://codereview.chromium.org/1413543005/diff/40001/extensions/renderer/mes...
File extensions/renderer/messaging_bindings.cc (right):

https://codereview.chromium.org/1413543005/diff/40001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.cc:208: content::RenderFrame* renderframe
= context()->GetRenderFrame();
nit: I think render_frame is more common.

[Charlie Reis, 2015-10-30 23:25:09]

[+site-isolation-reviews]

Thanks.  No big objections to the content API changes, but a few thoughts below.

https://codereview.chromium.org/1413543005/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1413543005/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:293:
frame_tree_node_->frame_tree()->FindByID(frame_tree_node_id);
This doesn't quite match the documentation, which says that it only returns this
node or one of its children.  This code could return a parent if called on a
subframe.

If you're just looking to return a frame from the current frame tree, I might
suggest putting the method on WebContents instead?

https://codereview.chromium.org/1413543005/diff/40001/content/public/browser/...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/1413543005/diff/40001/content/public/browser/...
content/public/browser/render_frame_host.h:71: // Returns the RenderFrameHost
for the given FrameTreeNode ID if it is the
nit: the current RenderFrameHost
(The same FrameTreeNode ID can refer to different RenderFrameHosts at different
times, due to cross-process navigations.)

https://codereview.chromium.org/1413543005/diff/40001/content/public/browser/...
content/public/browser/render_frame_host.h:72: // frame itself or a child of
this frame.
nit: or an immediate child

(Is this what you intended?  I'm not sure about the use cases.  If you just need
to find any frame in a given FrameTree, the method might belong in WebContents
instead.)

[robwu, 2015-10-31 00:10:45]

Thanks for your review Devlin & Charlie.

I've replied to some comments and will update the CL to address the other
comments.

https://codereview.chromium.org/1413543005/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/1413543005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:206: for
(content::RenderFrameHost* rfh : frames_) {
On 2015/10/30 01:49:39, Devlin wrote:
> Hmm... This is a bit confusing, because of the way this is laid out, but I
> *think* this is all just equivalent to doing WebContents::SendToAllFrames(). 
If
> |extension_process_| is null, it means we're following the tab, in which case
> |frames_| is just the set of all frames, right?  This also seems to imply we
> don't actually need |frames_| at all, because it doesn't really seem to be
used
> in the extension_process_ case.

|frames_| is used to avoid unnecessary IPC, since most frames will not be
interested in the port. The flow is this:
1. Browser: constructor + add all frames
2. Browser: DispatchOnConnect
3. Renderer: Acknowledge or reject port
4. Browser: Based on renderer, either ClosePort (=remove frame) or OpenPort
(=mark port as open).
5. Browser: DispatchOnMessage (zero or more times, depending on the extension
logic).
6. Browser: DispatchOnConnect (if all frames rejected, or if the frames
navigated away).

The |extension_process_| is a small optimization: All extension frames reside in
the same extension process, so a single IPC is sufficient to reach all frames.

https://codereview.chromium.org/1413543005/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/1413543005/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:42: void
RenderFrameDeleted(content::RenderFrameHost* render_frame_host)
On 2015/10/30 01:49:39, Devlin wrote:
> I would have thought FrameDeleted here - are you sure RenderFrameDeleted is
> correct?

I initially used FrameDeleted (as you can see in patch set 2), but that is not
triggered when the tab is killed. I've added a new test
(ExtensionApiTest.MessagingCrash) to catch this use case.
This is also a stepping stone to a fix for https://crbug.com/439780 (the test is
included in this CL because it shows the usefulness of using RenderFrameDeleted
over FrameDeleted, and some stub behavior will be replaced with desired behavior
when I fix this bug).

https://codereview.chromium.org/1413543005/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:59: DVLOG(2)
<< "OnExtensionFrameUnregistered " << port_->port_id_ << " for "
On 2015/10/30 01:49:39, Devlin wrote:
> We don't usually like to commit logs (even [D]VLOGS).  A few are maybe okay,
but
> there are many strewn throughout this CL.  Let's trim them a bit.

Oops, I did not intend to commit this one. I usually do not commit logging, but
I added them now because they helped with debugging. Which DVLOGs do you want to
be removed (all of them?)?

https://codereview.chromium.org/1413543005/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1413543005/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:293:
frame_tree_node_->frame_tree()->FindByID(frame_tree_node_id);
On 2015/10/30 23:25:09, Charlie Reis wrote:
> This doesn't quite match the documentation, which says that it only returns
this
> node or one of its children.  This code could return a parent if called on a
> subframe.
> 
> If you're just looking to return a frame from the current frame tree, I might
> suggest putting the method on WebContents instead?

I was reading frame_tree() as the frame tree rooted at the given node. If that's
incorrect, what do you suggest if I really want to look at the node and its
descendants only?

https://codereview.chromium.org/1413543005/diff/40001/content/public/browser/...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/1413543005/diff/40001/content/public/browser/...
content/public/browser/render_frame_host.h:72: // frame itself or a child of
this frame.
On 2015/10/30 23:25:09, Charlie Reis wrote:
> nit: or an immediate child
> 
> (Is this what you intended?  I'm not sure about the use cases.  If you just
need
> to find any frame in a given FrameTree, the method might belong in WebContents
> instead.)

My use case is finding a frame within a given tab, so moving to a WebContents
sounds good.

[Devlin, 2015-10-31 00:19:56]

https://codereview.chromium.org/1413543005/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/1413543005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:206: for
(content::RenderFrameHost* rfh : frames_) {
On 2015/10/31 00:10:44, robwu wrote:
> The |extension_process_| is a small optimization: All extension frames reside
in
> the same extension process, so a single IPC is sufficient to reach all frames.

What about extension iframes in web pages without site isolation?

https://codereview.chromium.org/1413543005/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/1413543005/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:42: void
RenderFrameDeleted(content::RenderFrameHost* render_frame_host)
On 2015/10/31 00:10:45, robwu wrote:
> On 2015/10/30 01:49:39, Devlin wrote:
> > I would have thought FrameDeleted here - are you sure RenderFrameDeleted is
> > correct?
> 
> I initially used FrameDeleted (as you can see in patch set 2), but that is not
> triggered when the tab is killed. I've added a new test
> (ExtensionApiTest.MessagingCrash) to catch this use case.
> This is also a stepping stone to a fix for https://crbug.com/439780 (the test
is
> included in this CL because it shows the usefulness of using
RenderFrameDeleted
> over FrameDeleted, and some stub behavior will be replaced with desired
behavior
> when I fix this bug).

Interesting.  I defer to Charlie's judgment for what's best here.  IIRC,
sometimes RenderFrameDeleted misses cases that FrameDeleted would have caught.

https://codereview.chromium.org/1413543005/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:59: DVLOG(2)
<< "OnExtensionFrameUnregistered " << port_->port_id_ << " for "
On 2015/10/31 00:10:45, robwu wrote:
> On 2015/10/30 01:49:39, Devlin wrote:
> > We don't usually like to commit logs (even [D]VLOGS).  A few are maybe okay,
> but
> > there are many strewn throughout this CL.  Let's trim them a bit.
> 
> Oops, I did not intend to commit this one. I usually do not commit logging,
but
> I added them now because they helped with debugging. Which DVLOGs do you want
to
> be removed (all of them?)?

Probably all of them, but if there's one or two you found to be invaluable and
will likely need again, it's okay to leave them.

[battre, 2015-11-02 13:54:56]

battre@chromium.org changed reviewers:
+ mpcomplete@chromium.org

[battre, 2015-11-02 13:54:57]

Adding Matt for his work on making the webrequest API work completely on the IO
thread.

I am concerned that congestion on the UI thread might slow down pageload times.

Best regards,
Dominic

https://codereview.chromium.org/1413543005/diff/40001/chrome/common/extension...
File chrome/common/extensions/docs/templates/intros/webNavigation.html (left):

https://codereview.chromium.org/1413543005/diff/40001/chrome/common/extension...
chrome/common/extensions/docs/templates/intros/webNavigation.html:109: frame is
always 0, the ID of child frames is a positive number. Once a document
On 2015/10/30 01:49:39, Devlin wrote:
> If frame ID uniquely identifies the frame, it implies this needs to be
updated.

So this is a non-backwards compatible change, right? Do we have an update
process for that?

https://codereview.chromium.org/1413543005/diff/40001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/1413543005/diff/40001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:786: // find a RFH in
ExtractRequestInfoOnUI, which sets the final frameId.
Can you expand the abbreviation?

https://codereview.chromium.org/1413543005/diff/40001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:1314:
base::Passed(&listeners_to_dispatch)));
I am a little bit concerned about this thread hopping. Matt implemented a lot of
logic for the webrequest API for the single purpose of not hopping between
threads. I wonder whether we need to investigate the performance implications
before launching this.

Do you see a way to work around this frame hopping?

[Devlin, 2015-11-02 17:07:16]

https://codereview.chromium.org/1413543005/diff/40001/chrome/common/extension...
File chrome/common/extensions/docs/templates/intros/webNavigation.html (left):

https://codereview.chromium.org/1413543005/diff/40001/chrome/common/extension...
chrome/common/extensions/docs/templates/intros/webNavigation.html:109: frame is
always 0, the ID of child frames is a positive number. Once a document
On 2015/11/02 13:54:57, battre wrote:
> On 2015/10/30 01:49:39, Devlin wrote:
> > If frame ID uniquely identifies the frame, it implies this needs to be
> updated.
> 
> So this is a non-backwards compatible change, right? Do we have an update
> process for that?

I don't think this should really be a problem.  No one should be persisting
frame ids across sessions or reloads, I think (I'm pretty sure we make no
guarantees that frame ids are persistent at session restore or anything).  And
since frames are now uniquely identified, developers can still use the pair of
<process_id, frame_id> to uniquely identify the frame - it's just extra (but
everything should still work).  I'll let Rob confirm.

[battre, 2015-11-02 17:18:22]

https://codereview.chromium.org/1413543005/diff/40001/chrome/common/extension...
File chrome/common/extensions/docs/templates/intros/webNavigation.html (left):

https://codereview.chromium.org/1413543005/diff/40001/chrome/common/extension...
chrome/common/extensions/docs/templates/intros/webNavigation.html:109: frame is
always 0, the ID of child frames is a positive number. Once a document
On 2015/11/02 17:07:16, Devlin wrote:
> On 2015/11/02 13:54:57, battre wrote:
> > On 2015/10/30 01:49:39, Devlin wrote:
> > > If frame ID uniquely identifies the frame, it implies this needs to be
> > updated.
> > 
> > So this is a non-backwards compatible change, right? Do we have an update
> > process for that?
> 
> I don't think this should really be a problem.  No one should be persisting
> frame ids across sessions or reloads, I think (I'm pretty sure we make no
> guarantees that frame ids are persistent at session restore or anything).  And
> since frames are now uniquely identified, developers can still use the pair of
> <process_id, frame_id> to uniquely identify the frame - it's just extra (but
> everything should still work).  I'll let Rob confirm.

My concern was the "The frame ID of the main frame is always 0" part. Is the
property maintained that the main frame will always carry the id 0?

[Devlin, 2015-11-02 17:28:47]

https://codereview.chromium.org/1413543005/diff/40001/chrome/common/extension...
File chrome/common/extensions/docs/templates/intros/webNavigation.html (left):

https://codereview.chromium.org/1413543005/diff/40001/chrome/common/extension...
chrome/common/extensions/docs/templates/intros/webNavigation.html:109: frame is
always 0, the ID of child frames is a positive number. Once a document
On 2015/11/02 17:18:22, battre wrote:
> On 2015/11/02 17:07:16, Devlin wrote:
> > On 2015/11/02 13:54:57, battre wrote:
> > > On 2015/10/30 01:49:39, Devlin wrote:
> > > > If frame ID uniquely identifies the frame, it implies this needs to be
> > > updated.
> > > 
> > > So this is a non-backwards compatible change, right? Do we have an update
> > > process for that?
> > 
> > I don't think this should really be a problem.  No one should be persisting
> > frame ids across sessions or reloads, I think (I'm pretty sure we make no
> > guarantees that frame ids are persistent at session restore or anything). 
And
> > since frames are now uniquely identified, developers can still use the pair
of
> > <process_id, frame_id> to uniquely identify the frame - it's just extra (but
> > everything should still work).  I'll let Rob confirm.
> 
> My concern was the "The frame ID of the main frame is always 0" part. Is the
> property maintained that the main frame will always carry the id 0?

Oh, whoops (yay for Monday mornings).  Good point.

[robwu, 2015-11-02 19:08:34]

I've addressed all comments made so far, PTAL.

Charlie:
While creating a new webNavigation test, I found some inconsistency in the way
that cross-process navigations are handled. Could you check whether the
expectations are correct (in
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js)?

https://codereview.chromium.org/1413543005/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/1413543005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:29:
ProcessManager::Get(port_->browser_context_)->AddObserver(this);
On 2015/10/30 01:49:39, Devlin wrote:
> nit: prefer scoped observer for usage like this.

Done.

https://codereview.chromium.org/1413543005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:34: :
content::WebContentsObserver(tab), port_(port) {
On 2015/10/30 01:49:38, Devlin wrote:
> This doesn't observe the process manager.  The comment makes it seem like
that's
> intentional, but having this behavior in the constructor is pretty weird, and
> very opaque from any call site.  I think I'd just prefer to have two methods
> here, TrackTabFrames and TrackExtensionProcessFrames or something like that. 
> Or, if there's a concrete reason to keep the behavior separate and exclusive, 
> make two classes.

Done.

https://codereview.chromium.org/1413543005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:92:
frame_tracker_() {
On 2015/10/30 01:49:39, Devlin wrote:
> This will default-initialize itself; no need for this line.

Acknowledged.

https://codereview.chromium.org/1413543005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:105:
frame_tracker_.reset();
On 2015/10/30 01:49:38, Devlin wrote:
> Why is the explicit destruction needed?

It's not needed - removed.

https://codereview.chromium.org/1413543005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:206: for
(content::RenderFrameHost* rfh : frames_) {
On 2015/10/31 00:19:56, Devlin wrote:
> On 2015/10/31 00:10:44, robwu wrote:
> > The |extension_process_| is a small optimization: All extension frames
reside
> in
> > the same extension process, so a single IPC is sufficient to reach all
frames.
> 
> What about extension iframes in web pages without site isolation?

Extension iframes currently don't have any access to extension APIs, except for
those that are already available in content scripts, and origin-specific APIs
such as databases and gUM. Therefore it's totally fine (even desirable for
backwards-compatibility?) that extension frames in web pages are not notified of
extension messages.

https://codereview.chromium.org/1413543005/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/extension_message_port.h (right):

https://codereview.chromium.org/1413543005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.h:75: void*
background_host_ptr_;  // used in IncrementLazyKeepaliveCount
On 2015/10/30 01:49:39, Devlin wrote:
> There's no reason for this to be a void*.  Can you make it an ExtensionHost*?

Done.

https://codereview.chromium.org/1413543005/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.h:77: };
On 2015/10/30 01:49:39, Devlin wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/1413543005/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/1413543005/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:39: }
On 2015/10/30 01:49:39, Devlin wrote:
> nit: newline

Done.

https://codereview.chromium.org/1413543005/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:58: {
On 2015/10/30 01:49:39, Devlin wrote:
> bracket

Done.

https://codereview.chromium.org/1413543005/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:59: DVLOG(2)
<< "OnExtensionFrameUnregistered " << port_->port_id_ << " for "
On 2015/10/31 00:19:56, Devlin wrote:
> On 2015/10/31 00:10:45, robwu wrote:
> > On 2015/10/30 01:49:39, Devlin wrote:
> > > We don't usually like to commit logs (even [D]VLOGS).  A few are maybe
okay,
> > but
> > > there are many strewn throughout this CL.  Let's trim them a bit.
> > 
> > Oops, I did not intend to commit this one. I usually do not commit logging,
> but
> > I added them now because they helped with debugging. Which DVLOGs do you
want
> to
> > be removed (all of them?)?
> 
> Probably all of them, but if there's one or two you found to be invaluable and
> will likely need again, it's okay to leave them.

Done.

https://codereview.chromium.org/1413543005/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/message_service.cc (right):

https://codereview.chromium.org/1413543005/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/message_service.cc:669: void
MessageService::ClosePortImpl(int port_id, int process_id, int routing_id,
On 2015/10/30 01:49:39, Devlin wrote:
> I think style is to put function parameters in the definition on newlines. 
git
> cl format will say for sure.

Done.

https://codereview.chromium.org/1413543005/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/message_service.h (right):

https://codereview.chromium.org/1413543005/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/message_service.h:64: virtual bool
IsValidPort();
On 2015/10/30 01:49:39, Devlin wrote:
> comment and newline, and I think I'd prefer this to be pure virtual.

Done.

https://codereview.chromium.org/1413543005/diff/40001/chrome/common/extension...
File chrome/common/extensions/docs/templates/intros/webNavigation.html (left):

https://codereview.chromium.org/1413543005/diff/40001/chrome/common/extension...
chrome/common/extensions/docs/templates/intros/webNavigation.html:109: frame is
always 0, the ID of child frames is a positive number. Once a document
On 2015/11/02 17:07:16, Devlin wrote:
> On 2015/11/02 13:54:57, battre wrote:
> > On 2015/10/30 01:49:39, Devlin wrote:
> > > If frame ID uniquely identifies the frame, it implies this needs to be
> > updated.

Done. I waited with updating this section until there is a test that actually
guarantees this behavior. I've now added this test
(WebNavigationApiTest.CrossProcessIframe).

> > So this is a non-backwards compatible change, right? Do we have an update
> > process for that?

This is a backwards-compatible change. I took much care to ensure that existing
extensions won't break (see the CL description and linked bug).

> 
> I don't think this should really be a problem.  No one should be persisting
> frame ids across sessions or reloads, I think (I'm pretty sure we make no
> guarantees that frame ids are persistent at session restore or anything).  And
> since frames are now uniquely identified, developers can still use the pair of
> <process_id, frame_id> to uniquely identify the frame - it's just extra (but
> everything should still work).  I'll let Rob confirm.

I've marked processId as deprecated, extension authors should use tabId +
frameId to uniquely identify a frame. We make no guarantees about the usefulness
of frameId across tabs, let alone sessions.

https://codereview.chromium.org/1413543005/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1413543005/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:293:
frame_tree_node_->frame_tree()->FindByID(frame_tree_node_id);
On 2015/10/30 23:25:09, Charlie Reis (slow til 11-16) wrote:
> This doesn't quite match the documentation, which says that it only returns
this
> node or one of its children.  This code could return a parent if called on a
> subframe.
> 
> If you're just looking to return a frame from the current frame tree, I might
> suggest putting the method on WebContents instead?

Done.

https://codereview.chromium.org/1413543005/diff/40001/content/public/browser/...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/1413543005/diff/40001/content/public/browser/...
content/public/browser/render_frame_host.h:71: // Returns the RenderFrameHost
for the given FrameTreeNode ID if it is the
On 2015/10/30 23:25:09, Charlie Reis (slow til 11-16) wrote:
> nit: the current RenderFrameHost
> (The same FrameTreeNode ID can refer to different RenderFrameHosts at
different
> times, due to cross-process navigations.)

Done.

https://codereview.chromium.org/1413543005/diff/40001/content/public/browser/...
content/public/browser/render_frame_host.h:72: // frame itself or a child of
this frame.
On 2015/10/30 23:25:09, Charlie Reis (slow til 11-16) wrote:
> nit: or an immediate child
> 
> (Is this what you intended?  I'm not sure about the use cases.  If you just
need
> to find any frame in a given FrameTree, the method might belong in WebContents
> instead.)

Done.

https://codereview.chromium.org/1413543005/diff/40001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/1413543005/diff/40001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:224:
scoped_ptr<base::DictionaryValue> dict) {
On 2015/10/30 01:49:39, Devlin wrote:
> Seems like this should be a void method that takes in a DictionaryValue*?

Done.

https://codereview.chromium.org/1413543005/diff/40001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:226: DCHECK(dict.get());
On 2015/10/30 01:49:39, Devlin wrote:
> nit: s/.get()//

Done.

https://codereview.chromium.org/1413543005/diff/40001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:786: // find a RFH in
ExtractRequestInfoOnUI, which sets the final frameId.
On 2015/11/02 13:54:57, battre wrote:
> Can you expand the abbreviation?

Done.

https://codereview.chromium.org/1413543005/diff/40001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:1314:
base::Passed(&listeners_to_dispatch)));
On 2015/11/02 13:54:57, battre wrote:
> I am a little bit concerned about this thread hopping. Matt implemented a lot
of
> logic for the webrequest API for the single purpose of not hopping between
> threads. I wonder whether we need to investigate the performance implications
> before launching this.
> 
> Do you see a way to work around this frame hopping?

Unfortunately not, the conclusion of the discussion at
https://groups.google.com/a/chromium.org/d/msg/site-isolation-dev/qlnf2fEAfB8...
is that we have to hop to the UI thread if we want to get a meaningful FTN ID.

I hope to revert the hopping in the webRequest API when every request passes the
UI thread, there is a field trial to investigate the performance impact of doing
so (https://crbug.com/524228).

Matt's work is not completely redundant, the blocking callbacks are directly
handled on the IO thread.

https://codereview.chromium.org/1413543005/diff/40001/extensions/renderer/mes...
File extensions/renderer/messaging_bindings.cc (right):

https://codereview.chromium.org/1413543005/diff/40001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.cc:208: content::RenderFrame* renderframe
= context()->GetRenderFrame();
On 2015/10/30 01:49:39, Devlin wrote:
> nit: I think render_frame is more common.

Indeed, but this file only uses |renderframe| (2x elsewhere), so I've used
renderframe to be consistent within this file.

[Matt Perry, 2015-11-02 21:06:59]

https://codereview.chromium.org/1413543005/diff/40001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/1413543005/diff/40001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:1314:
base::Passed(&listeners_to_dispatch)));
On 2015/11/02 19:08:34, robwu wrote:
> On 2015/11/02 13:54:57, battre wrote:
> > I am a little bit concerned about this thread hopping. Matt implemented a
lot
> of
> > logic for the webrequest API for the single purpose of not hopping between
> > threads. I wonder whether we need to investigate the performance
implications
> > before launching this.
> > 
> > Do you see a way to work around this frame hopping?
> 
> Unfortunately not, the conclusion of the discussion at
>
https://groups.google.com/a/chromium.org/d/msg/site-isolation-dev/qlnf2fEAfB8...
> is that we have to hop to the UI thread if we want to get a meaningful FTN ID.
> 
> I hope to revert the hopping in the webRequest API when every request passes
the
> UI thread, there is a field trial to investigate the performance impact of
doing
> so (https://crbug.com/524228).
> 
> Matt's work is not completely redundant, the blocking callbacks are directly
> handled on the IO thread.

Your last statement doesn't seem to be true. It's been a while since I looked at
this code, but it looks like we still go to the UI thread and back, even for
blocking event handlers. There was a measurable difference in roundtrip time
when I made the change to handle everything on the IO thread.

Note that these event handlers will block every single network request until the
extension responds, so performance is pretty important here.

[robwu, 2015-11-02 21:33:32]

https://codereview.chromium.org/1413543005/diff/40001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/1413543005/diff/40001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:1314:
base::Passed(&listeners_to_dispatch)));
On 2015/11/02 21:06:59, Matt Perry wrote:
> On 2015/11/02 19:08:34, robwu wrote:
> > On 2015/11/02 13:54:57, battre wrote:
> > > I am a little bit concerned about this thread hopping. Matt implemented a
> lot
> > of
> > > logic for the webrequest API for the single purpose of not hopping between
> > > threads. I wonder whether we need to investigate the performance
> implications
> > > before launching this.
> > > 
> > > Do you see a way to work around this frame hopping?
> > 
> > Unfortunately not, the conclusion of the discussion at
> >
>
https://groups.google.com/a/chromium.org/d/msg/site-isolation-dev/qlnf2fEAfB8...
> > is that we have to hop to the UI thread if we want to get a meaningful FTN
ID.
> > 
> > I hope to revert the hopping in the webRequest API when every request passes
> the
> > UI thread, there is a field trial to investigate the performance impact of
> doing
> > so (https://crbug.com/524228).
> > 
> > Matt's work is not completely redundant, the blocking callbacks are directly
> > handled on the IO thread.
> 
> Your last statement doesn't seem to be true. It's been a while since I looked
at
> this code, but it looks like we still go to the UI thread and back, even for
> blocking event handlers. There was a measurable difference in roundtrip time
> when I made the change to handle everything on the IO thread.

By blocking callback, I mean the handling of the webRequest event listener's
return value. This doesn't hop to the UI thread. This patch introduces "only"
one IO->UI->IO hop before every event.


> Note that these event handlers will block every single network request until
the
> extension responds, so performance is pretty important here.

I'll optimize this code a bit by caching the frameId and using a queue to ensure
that the events are not delivered out of order.

[Charlie Reis, 2015-11-03 00:23:49]

render_frame_host{_impl} and web_contents{_impl} LGTM with nits.  Some other
replies below as well.

On 2015/11/02 19:08:34, robwu wrote:
> Charlie:
> While creating a new webNavigation test, I found some inconsistency in the way
> that cross-process navigations are handled. Could you check whether the
> expectations are correct (in
>
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js)?

Sorry, I can't figure out how to read that test.  Maybe you can send me a
separate email with specific questions about the behavior?  Or maybe we can ask
Nasko, since I think he figured out that test syntax when working on the Web
Navigation API?

https://codereview.chromium.org/1413543005/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/1413543005/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:42: void
RenderFrameDeleted(content::RenderFrameHost* render_frame_host)
On 2015/10/31 00:19:56, Devlin wrote:
> On 2015/10/31 00:10:45, robwu wrote:
> > On 2015/10/30 01:49:39, Devlin wrote:
> > > I would have thought FrameDeleted here - are you sure RenderFrameDeleted
is
> > > correct?
> > 
> > I initially used FrameDeleted (as you can see in patch set 2), but that is
not
> > triggered when the tab is killed. I've added a new test
> > (ExtensionApiTest.MessagingCrash) to catch this use case.
> > This is also a stepping stone to a fix for https://crbug.com/439780 (the
test
> is
> > included in this CL because it shows the usefulness of using
> RenderFrameDeleted
> > over FrameDeleted, and some stub behavior will be replaced with desired
> behavior
> > when I fix this bug).
> 
> Interesting.  I defer to Charlie's judgment for what's best here.  IIRC,
> sometimes RenderFrameDeleted misses cases that FrameDeleted would have caught.

RenderFrameDeleted is for cases where you care whether the RenderFrame in the
renderer process has gone away.  It should include all cases in which
FrameDeleted is called (such as closing a tab or detaching a frame), as well as
after cross-process navigations or process crashes.

(If there's cases that FrameDeleted is called and RenderFrameDeleted is not,
that's possibly a bug that we should fix.)

If you're interested in when RenderFrameHosts go away (and not process crashes),
then FrameDeleted and RenderFrameHostChanged are a better fit.  Since it sounds
like you care about process crashes, I'm guessing RenderFrameDeleted is what you
want.

https://codereview.chromium.org/1413543005/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1413543005/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:293:
frame_tree_node_->frame_tree()->FindByID(frame_tree_node_id);
On 2015/10/31 00:10:45, robwu wrote:
> On 2015/10/30 23:25:09, Charlie Reis wrote:
> > This doesn't quite match the documentation, which says that it only returns
> this
> > node or one of its children.  This code could return a parent if called on a
> > subframe.
> > 
> > If you're just looking to return a frame from the current frame tree, I
might
> > suggest putting the method on WebContents instead?
> 
> I was reading frame_tree() as the frame tree rooted at the given node.

No, a FrameTree represents the whole tree, starting from the root (main frame).

> If that's
> incorrect, what do you suggest if I really want to look at the node and its
> descendants only?

Hopefully this isn't needed anymore, but you could recursively traverse the
children of the FrameTreeNode if you needed it.

https://codereview.chromium.org/1413543005/diff/60001/content/public/browser/...
File content/public/browser/resource_request_info.h (right):

https://codereview.chromium.org/1413543005/diff/60001/content/public/browser/...
content/public/browser/resource_request_info.h:79: // True if parent frame of
frame represents a main frame in the RenderView.
nit: True if the parent frame of the frame that sent this request represents a
main frame in the RenderView.

https://codereview.chromium.org/1413543005/diff/60001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1413543005/diff/60001/content/public/browser/...
content/public/browser/web_contents.h:216: virtual RenderFrameHost*
FindByFrameTreeNodeID(int frame_tree_node_id) = 0;
nit: FindFrameByFrameTreeNodeID

(Otherwise FindByX seems to implicitly mean FindWebContentsByX.)

[battre, 2015-11-03 09:07:19]

https://codereview.chromium.org/1413543005/diff/40001/chrome/common/extension...
File chrome/common/extensions/docs/templates/intros/webNavigation.html (left):

https://codereview.chromium.org/1413543005/diff/40001/chrome/common/extension...
chrome/common/extensions/docs/templates/intros/webNavigation.html:109: frame is
always 0, the ID of child frames is a positive number. Once a document
> > > So this is a non-backwards compatible change, right? Do we have an update
> > > process for that?
> 
> This is a backwards-compatible change. I took much care to ensure that
existing
> extensions won't break (see the CL description and linked bug).

My backwards compatibility concern is that in the past, an extension could have
written

if (details.frameId == 0)
  return "main_frame";

That could would break with this change, wouldn't it?

The WebRequest API has the following documentation:
"The value 0 indicates that the request happens in the main frame; a positive
value indicates the ID of a subframe in which the request happens. If the
document of a (sub-)frame is loaded (type is main_frame or sub_frame), frameId
indicates the ID of this frame, not the ID of the outer frame. Frame IDs are
unique within a tab [...]"

That would need to be updated as well.

[robwu, 2015-11-03 10:03:02]

> On 2015/11/02 19:08:34, robwu wrote:
> > Charlie:
> > While creating a new webNavigation test, I found some inconsistency in the
way
> > that cross-process navigations are handled. Could you check whether the
> > expectations are correct (in
> >
>
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js)?
> 
> Sorry, I can't figure out how to read that test.  Maybe you can send me a
> separate email with specific questions about the behavior?  Or maybe we can
ask
> Nasko, since I think he figured out that test syntax when working on the Web
> Navigation API?

That test does:
1. Open an extension page in tab.
2. The extension page embeds a frame from a.com
3. Once a.com is loaded, the extension page navigates the frame to b.com.
4. b.com navigates to c.com via JavaScript.

a.com, b.com and c.com are all loaded in another process.
Step 2 has the following webNavigation event sequence:
onBeforeNavigate(pid=0), onBeforeNavigate(pid=1), onErrorOccurred(pid=0)
Step 4 also has the above events (for processId 2 and processId 3).

But step 3 does NOT have the onBeforeNavigate/onErrorOccurred events for the old
process. This strikes me as inconsistent.

[ I deprecated and processId in the webNavigation API, and deleted the section
on processIds from
https://developer.chrome.com/extensions/webNavigation#frame_ids because I
thought that processIds are no longer relevant. If the above scenario is
intentional, then I'll undeprecate processId. ]

https://codereview.chromium.org/1413543005/diff/40001/chrome/common/extension...
File chrome/common/extensions/docs/templates/intros/webNavigation.html (left):

https://codereview.chromium.org/1413543005/diff/40001/chrome/common/extension...
chrome/common/extensions/docs/templates/intros/webNavigation.html:109: frame is
always 0, the ID of child frames is a positive number. Once a document
On 2015/11/03 09:07:19, battre wrote:
> > > > So this is a non-backwards compatible change, right? Do we have an
update
> > > > process for that?
> > 
> > This is a backwards-compatible change. I took much care to ensure that
> existing
> > extensions won't break (see the CL description and linked bug).
> 
> My backwards compatibility concern is that in the past, an extension could
have
> written
> 
> if (details.frameId == 0)
>   return "main_frame";
> 
> That could would break with this change, wouldn't it?

frameId 0 is still the main frame. The semantics have not changed in that
respect. Here you can see how frameId is set:
https://codereview.chromium.org/1413543005/patch/60001/70042#newcode220
Currently, frames can be uniquely identified only by tabId + frameId +
processId. Now it is already unique for tabId + frameId (but adding processId in
the mix doesn't harm).

> 
> The WebRequest API has the following documentation:
> "The value 0 indicates that the request happens in the main frame; a positive
> value indicates the ID of a subframe in which the request happens. If the
> document of a (sub-)frame is loaded (type is main_frame or sub_frame), frameId
> indicates the ID of this frame, not the ID of the outer frame. Frame IDs are
> unique within a tab [...]"
> 
> That would need to be updated as well.

In patchset 4 I have added "As of Chrome 48, this ID is also constant for the
lifetime of the frame (across multiple navigations).".

https://codereview.chromium.org/1413543005/diff/60001/content/public/browser/...
File content/public/browser/resource_request_info.h (right):

https://codereview.chromium.org/1413543005/diff/60001/content/public/browser/...
content/public/browser/resource_request_info.h:79: // True if parent frame of
frame represents a main frame in the RenderView.
On 2015/11/03 00:23:49, Charlie Reis (slow til 11-16) wrote:
> nit: True if the parent frame of the frame that sent this request represents a
> main frame in the RenderView.

Done (using "the frame's parent" to fit the comment on one line).

https://codereview.chromium.org/1413543005/diff/60001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1413543005/diff/60001/content/public/browser/...
content/public/browser/web_contents.h:216: virtual RenderFrameHost*
FindByFrameTreeNodeID(int frame_tree_node_id) = 0;
On 2015/11/03 00:23:49, Charlie Reis (slow til 11-16) wrote:
> nit: FindFrameByFrameTreeNodeID
> 
> (Otherwise FindByX seems to implicitly mean FindWebContentsByX.)

Done.

[Charlie Reis, 2015-11-04 01:05:04]

On 2015/11/03 10:03:02, robwu wrote:
> > On 2015/11/02 19:08:34, robwu wrote:
> > > Charlie:
> > > While creating a new webNavigation test, I found some inconsistency in the
> way
> > > that cross-process navigations are handled. Could you check whether the
> > > expectations are correct (in
> > >
> >
>
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js)?
> > 
> > Sorry, I can't figure out how to read that test.  Maybe you can send me a
> > separate email with specific questions about the behavior?  Or maybe we can
> ask
> > Nasko, since I think he figured out that test syntax when working on the Web
> > Navigation API?
> 
> That test does:
> 1. Open an extension page in tab.
> 2. The extension page embeds a frame from a.com
> 3. Once a.com is loaded, the extension page navigates the frame to b.com.
> 4. b.com navigates to c.com via JavaScript.
> 
> a.com, b.com and c.com are all loaded in another process.
> Step 2 has the following webNavigation event sequence:
> onBeforeNavigate(pid=0), onBeforeNavigate(pid=1), onErrorOccurred(pid=0)
> Step 4 also has the above events (for processId 2 and processId 3).
> 
> But step 3 does NOT have the onBeforeNavigate/onErrorOccurred events for the
old
> process. This strikes me as inconsistent.
> 
> [ I deprecated and processId in the webNavigation API, and deleted the section
> on processIds from
> https://developer.chrome.com/extensions/webNavigation#frame_ids because I
> thought that processIds are no longer relevant. If the above scenario is
> intentional, then I'll undeprecate processId. ]
> 

Ah, this is a great question.  I'm surprised we're missing those events in step
3, and it sounds like a separate bug.  Would you be able to debug and see why
they're not being called?  We can also ask Nasko (who has worked on the web
navigation API), but I imagine any of us would just be patching in the change
and trying to see how the two cases differ, as a first step.

[robwu, 2015-11-04 11:51:00]

On 2015/11/04 01:05:04, Charlie Reis (slow til 11-16) wrote:
> On 2015/11/03 10:03:02, robwu wrote:
> > > On 2015/11/02 19:08:34, robwu wrote:
> > > > Charlie:
> > > > While creating a new webNavigation test, I found some inconsistency in
the
> > way
> > > > that cross-process navigations are handled. Could you check whether the
> > > > expectations are correct (in
> > > >
> > >
> >
>
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js)?
> > > 
> > > Sorry, I can't figure out how to read that test.  Maybe you can send me a
> > > separate email with specific questions about the behavior?  Or maybe we
can
> > ask
> > > Nasko, since I think he figured out that test syntax when working on the
Web
> > > Navigation API?
> > 
> > That test does:
> > 1. Open an extension page in tab.
> > 2. The extension page embeds a frame from a.com
> > 3. Once a.com is loaded, the extension page navigates the frame to b.com.
> > 4. b.com navigates to c.com via JavaScript.
> > 
> > a.com, b.com and c.com are all loaded in another process.
> > Step 2 has the following webNavigation event sequence:
> > onBeforeNavigate(pid=0), onBeforeNavigate(pid=1), onErrorOccurred(pid=0)
> > Step 4 also has the above events (for processId 2 and processId 3).
> > 
> > But step 3 does NOT have the onBeforeNavigate/onErrorOccurred events for the
> old
> > process. This strikes me as inconsistent.
> > 
> > [ I deprecated and processId in the webNavigation API, and deleted the
section
> > on processIds from
> > https://developer.chrome.com/extensions/webNavigation#frame_ids because I
> > thought that processIds are no longer relevant. If the above scenario is
> > intentional, then I'll undeprecate processId. ]
> > 
> 
> Ah, this is a great question.  I'm surprised we're missing those events in
step
> 3, and it sounds like a separate bug.  Would you be able to debug and see why
> they're not being called?  We can also ask Nasko (who has worked on the web
> navigation API), but I imagine any of us would just be patching in the change
> and trying to see how the two cases differ, as a first step.

I found a possible reason for the inconsistency. Given the scenario as described
before:
Step 2 and 4 start the provisional load in the old renderer process. Shortly
after that, the browser swaps the process.
In step 3, the navigation is not triggered by the renderer process itself, but
by a different process (the frame's parent process). I guess that this means
that the frame swap occurs before the old renderer of the frame has had the
chance to attempt a navigation.

Is this expected? Is it possible to not attempt / defer the provisional load in
the renderer when it's obvious that a process swap is going to occur?

[Charlie Reis, 2015-11-06 23:13:16]

On 2015/11/04 11:51:00, robwu wrote:
> On 2015/11/04 01:05:04, Charlie Reis (slow til 11-16) wrote:
> > On 2015/11/03 10:03:02, robwu wrote:
> > > > On 2015/11/02 19:08:34, robwu wrote:
> > > > > Charlie:
> > > > > While creating a new webNavigation test, I found some inconsistency in
> the
> > > way
> > > > > that cross-process navigations are handled. Could you check whether
the
> > > > > expectations are correct (in
> > > > >
> > > >
> > >
> >
>
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js)?
> > > > 
> > > > Sorry, I can't figure out how to read that test.  Maybe you can send me
a
> > > > separate email with specific questions about the behavior?  Or maybe we
> can
> > > ask
> > > > Nasko, since I think he figured out that test syntax when working on the
> Web
> > > > Navigation API?
> > > 
> > > That test does:
> > > 1. Open an extension page in tab.
> > > 2. The extension page embeds a frame from a.com
> > > 3. Once a.com is loaded, the extension page navigates the frame to b.com.
> > > 4. b.com navigates to c.com via JavaScript.
> > > 
> > > a.com, b.com and c.com are all loaded in another process.
> > > Step 2 has the following webNavigation event sequence:
> > > onBeforeNavigate(pid=0), onBeforeNavigate(pid=1), onErrorOccurred(pid=0)
> > > Step 4 also has the above events (for processId 2 and processId 3).
> > > 
> > > But step 3 does NOT have the onBeforeNavigate/onErrorOccurred events for
the
> > old
> > > process. This strikes me as inconsistent.
> > > 
> > > [ I deprecated and processId in the webNavigation API, and deleted the
> section
> > > on processIds from
> > > https://developer.chrome.com/extensions/webNavigation#frame_ids because I
> > > thought that processIds are no longer relevant. If the above scenario is
> > > intentional, then I'll undeprecate processId. ]
> > > 
> > 
> > Ah, this is a great question.  I'm surprised we're missing those events in
> step
> > 3, and it sounds like a separate bug.  Would you be able to debug and see
why
> > they're not being called?  We can also ask Nasko (who has worked on the web
> > navigation API), but I imagine any of us would just be patching in the
change
> > and trying to see how the two cases differ, as a first step.
> 
> I found a possible reason for the inconsistency. Given the scenario as
described
> before:
> Step 2 and 4 start the provisional load in the old renderer process. Shortly
> after that, the browser swaps the process.
> In step 3, the navigation is not triggered by the renderer process itself, but
> by a different process (the frame's parent process). I guess that this means
> that the frame swap occurs before the old renderer of the frame has had the
> chance to attempt a navigation.
> 
> Is this expected? Is it possible to not attempt / defer the provisional load
in
> the renderer when it's obvious that a process swap is going to occur?

Oh, I see.  Yeah, that makes those cases different.

I think we can punt that until PlzNavigate is finished
(https://crbug.com/368813), which should probably unify those paths.  (All
navigations will be sent to the browser process, and it will choose the right
RenderFrame to send the response to.)  In the meantime, we can probably just put
comments in the test file.

Thanks for looking into it!

https://codereview.chromium.org/1413543005/diff/80001/content/public/browser/...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/1413543005/diff/80001/content/public/browser/...
content/public/browser/render_frame_host.h:67: // Returns the FrameTreeNode ID
for this frame.
Nasko brings up a good point that we may not want to call this
"GetFrameTreeNodeID," since there's no FrameTreeNode in the public API... 
Sadly, "frame ID" is slightly ambiguous since we already have render_frame_id in
some APIs (which confusingly refers to RenderFrame routing ID).

I'm not sure if there's a better option-- GetFrameID (and bite the bullet vs
render_frame_id) vs somehow explaining what GetFrameTreeNodeID means vs
something else?

Either way, we should elaborate on what this concept means here, since there's
no other explanation in the public API.  In other words, that it is a stable
identifier for a frame even after it does cross-process navigations that switch
its RenderFrameHost.

[nasko, 2015-11-06 23:28:27]

I've gone through content/, webnavigation, and some random other files. 

I do feel strongly about keeping the process id as an output value from the
extension API (at least for webNavigation), as I have at least one extension
that relies on it. It makes it very easy to visualize that we have the correct
process allocation without the need to attach a debugger or look at the task
manager. I can imagine other developers using the process id to glean into the
internals of how Chrome navigates around.

As an input parameter, I'm ok ignoring it or even deprecating it, as it doesn't
make sense anymore.

https://codereview.chromium.org/1413543005/diff/80001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js
(right):

https://codereview.chromium.org/1413543005/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js:35:
{ label: "a-onBeforeNavigate",
Shouldn't we use "main-" prefix on these, as we already use a.com as a domain
for the subframe?

https://codereview.chromium.org/1413543005/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js:66:
// Process swap.
Here I would expect to see the load of the iframe with a.com, which includes
onBeforeNavigate, onCommitted, onDOMContentLoaded, though not sure about getting
an onCompleted. I think because of how the code is structured, you will get
onErrorOccurred, since we navigate the frame before finishing the onload event
execution.

https://codereview.chromium.org/1413543005/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js:98:
transitionType: "auto_subframe",
Hmm, this isn't quite expected for me. auto_subframe should be only in the case
where the parent hasn't fired and completed its onload event. For the navigation
to b.com, the parent should've completed the onload event handler, as main.js
uses setTimeout in the handler to defer the navigation to the next iteration of
the event loop.

https://codereview.chromium.org/1413543005/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js:115:
// like above before the process swap?
When you are on b.com, it registers an onload event handler, which has a
setTimeout() call. This will result on the onload event to complete successfully
and we will fire the onCompleted for b.com, which is the event right before your
comment. Because of the setTimeout() handling, the function that will navigate
the frame to c.com will be queued up on the event loop.

https://codereview.chromium.org/1413543005/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js:120:
processId: 2,
Does this pass? I would expect the onBeforeNavigate to come from process 1, the
browser process to do a transfer of the navigation from one process to another,
then the commit to come from process 2. Though this is complex enough that I
can't reason about it entirely in my head and need to probably patch in the CL
and look at the debugger.

https://codereview.chromium.org/1413543005/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js:148:
{ label: "pre-d-onBeforeNavigate",
Urgh, I might have been misreading these steps because of the labelling : (.
Let's use the prefixes to be "main", "a-com", "b-com", "c-com", so they match
the navigations we perform in the JS files.

https://codereview.chromium.org/1413543005/diff/80001/content/public/browser/...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/1413543005/diff/80001/content/public/browser/...
content/public/browser/render_frame_host.h:68: virtual int GetFrameTreeNodeID()
= 0;
Let's call this GetFrameId(). FrameTreeNode is not a concept exposed outside of
content/ and shouldn't be leaked, even through naming of methods.
Also, include another sentence which describes the meaning of this id - a unique
ID for this frame, which will not change even across cross-process navigations.

https://codereview.chromium.org/1413543005/diff/80001/content/public/browser/...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1413543005/diff/80001/content/public/browser/...
content/public/browser/web_contents.h:216: virtual RenderFrameHost*
FindFrameByFrameTreeNodeID(
FindRenderFrameHostByFrameId

https://codereview.chromium.org/1413543005/diff/80001/content/public/browser/...
content/public/browser/web_contents.h:217: int frame_tree_node_id) = 0;
frame_id

https://codereview.chromium.org/1413543005/diff/80001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/1413543005/diff/80001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:232: int render_frame_id =
-1;
What type of id is this? The FTN or the routing id? Let's be very precise.

https://codereview.chromium.org/1413543005/diff/80001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:238:
content::RenderFrameHost::FromID(render_process_id, render_frame_id);
If this comes from caller of the extension API, wouldn't this be a FTN id? If
that's the case, this is the incorrect method to use for look up.

https://codereview.chromium.org/1413543005/diff/80001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:240: if
(content::RenderFrameHost* parent = rfh->GetParent()) {
Assignment should not be done inside an if clause.

https://codereview.chromium.org/1413543005/diff/80001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:251:
dict->Remove(keys::kProcessIdKey, nullptr);
Why are we removing this? I feel strongly about keeping it, as it gives
extension developers good idea of how frames are distributed across processes. I
even use it in an extension of mine to visualize that we do the right thing in
the SiteInstance logic.

https://codereview.chromium.org/1413543005/diff/80001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:771: int frame_id = -1;
I think this is the frame routing id, correct? If yes, let's name it explicitly
so it is clear which id we are using here.

https://codereview.chromium.org/1413543005/diff/80001/extensions/renderer/mes...
File extensions/renderer/messaging_bindings.cc (right):

https://codereview.chromium.org/1413543005/diff/80001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.cc:208: content::RenderFrame* renderframe
= context()->GetRenderFrame();
render_frame

https://codereview.chromium.org/1413543005/diff/80001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.cc:211: renderframe->GetRoutingID(),
port_id, true));
Only one line if statements can skip {}. It is needed here in this case.

https://codereview.chromium.org/1413543005/diff/80001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.cc:239: content::RenderFrame* renderframe
= context()->GetRenderFrame();
render_frame

[robwu, 2015-11-07 00:23:40]

In the next update I'll remove the "deprecate" flag from the event details in
the webNavigation API because it seems useful.

https://codereview.chromium.org/1413543005/diff/80001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js
(right):

https://codereview.chromium.org/1413543005/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js:115:
// like above before the process swap?
On 2015/11/06 23:28:27, nasko (slow to review) wrote:
> When you are on b.com, it registers an onload event handler, which has a
> setTimeout() call. This will result on the onload event to complete
successfully
> and we will fire the onCompleted for b.com, which is the event right before
your
> comment. Because of the setTimeout() handling, the function that will navigate
> the frame to c.com will be queued up on the event loop.

By "like above", I'm referring to the pair of onBeforeNavigate+onErrorOccurred
events that occur at the other navigation. But I think that I've found a
plausible explanation: https://codereview.chromium.org/1413543005/#msg20

https://codereview.chromium.org/1413543005/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js:120:
processId: 2,
On 2015/11/06 23:28:27, nasko (slow to review) wrote:
> Does this pass? I would expect the onBeforeNavigate to come from process 1,
the
> browser process to do a transfer of the navigation from one process to
another,
> then the commit to come from process 2. Though this is complex enough that I
> can't reason about it entirely in my head and need to probably patch in the CL
> and look at the debugger.

The test passes. My TODO comment a few lines back refers to the absence of the
sequence of events that you're describing. See also my reply ("plausible
explanation") that I mentioned in my codereview comment above.

https://codereview.chromium.org/1413543005/diff/80001/content/public/browser/...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/1413543005/diff/80001/content/public/browser/...
content/public/browser/render_frame_host.h:68: virtual int GetFrameTreeNodeID()
= 0;
On 2015/11/06 23:28:27, nasko (slow to review) wrote:
> Let's call this GetFrameId(). FrameTreeNode is not a concept exposed outside
of
> content/ and shouldn't be leaked, even through naming of methods.
> Also, include another sentence which describes the meaning of this id - a
unique
> ID for this frame, which will not change even across cross-process
navigations.

It is very likely that GetFrameId is confused with the RenderFrame routing ID,
so we need a name that is a bit more specific than GetFrameId.

https://codereview.chromium.org/1413543005/diff/80001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/1413543005/diff/80001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:238:
content::RenderFrameHost::FromID(render_process_id, render_frame_id);
On 2015/11/06 23:28:27, nasko (slow to review) wrote:
> If this comes from caller of the extension API, wouldn't this be a FTN id? If
> that's the case, this is the incorrect method to use for look up.

Heh. This ID is correct. I'm abusing kFrameIdKey as a temporary transport for
frameId from the caller to this method.

Before: frameId = RFH routing ID.
After : frameId = RFH FTN ID.

https://codereview.chromium.org/1413543005/diff/80001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:251:
dict->Remove(keys::kProcessIdKey, nullptr);
On 2015/11/06 23:28:27, nasko (slow to review) wrote:
> Why are we removing this? I feel strongly about keeping it, as it gives
> extension developers good idea of how frames are distributed across processes.
I
> even use it in an extension of mine to visualize that we do the right thing in
> the SiteInstance logic.

This is the webRequest API. processId was never included in its public extension
API, and I'm only using the dict to propagate the frameId from
ExtractRequestInfo to this method.

[Devlin, 2015-11-13 21:16:28]

https://codereview.chromium.org/1413543005/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/1413543005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:55: void
OnExtensionFrameUnregistered(
I would hope that doing the lifetime management here would have meant that we
could eliminate some of the crazy lifetime on the renderer; do we still need
that (just for garbage collection)?

https://codereview.chromium.org/1413543005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:90:
ExtensionMessagePort::ExtensionMessagePort(
nit: put two constructors next to each other.

https://codereview.chromium.org/1413543005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:212: void
ExtensionMessagePort::SendToPort(IPC::Message* msg) {
nit: let's make this take a scoped_ptr (and just release it in the
extension_process_->Send case).

https://codereview.chromium.org/1413543005/diff/80001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/messaging/connect_crash/page.js
(right):

https://codereview.chromium.org/1413543005/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/messaging/connect_crash/page.js:1: //
Copyright (c) 2015 The Chromium Authors. All rights reserved.
nit: no (c) (everywhere)

https://codereview.chromium.org/1413543005/diff/80001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/frame.js
(right):

https://codereview.chromium.org/1413543005/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/frame.js:7:
if (location.hostname === "a.com") {
nitty nit: prefer single-quotes in js (except to avoid escapes)

[Devlin, 2015-12-03 20:26:29]

Just confirming, this is blocked on https://codereview.chromium.org/1413853005/?

[robwu, 2015-12-03 21:54:21]

On 2015/12/03 20:26:29, Devlin wrote:
> Just confirming, this is blocked on
https://codereview.chromium.org/1413853005/

Yes. I'm waiting until that patch lands, and then I'll rebase this and address
your review.

[robwu, 2015-12-06 22:00:10]

Patchset #8 (id:140001) has been deleted

[robwu, 2015-12-07 23:44:22]

I've addressed all comments, please review again:

Devlin:
- chrome/browser/extensions/api/messaging/*
- extensions/browser/extension_api_frame_id_map.*

Dominic / Matt:
- extensions/browser/api/web_request/*
- extensions/browser/extension_api_frame_id_map.*

nasko:
- chrome/browser/extensions/api/web_navigation/*
- content/public/browser/render_frame_host.h
- content/public/browser/web_contents.h
- & bikeshedding about FTN ID.


Changes:
- Patch set 7 = small nits / not interesting.

- Patch set 8 = documenting the FrameTreeNode ID. I considered other names (such
as UniqueFrameId and ContentFrameId), but decided against it and stick to
FrameTreeNodeID because my names are lame, and "frame_tree_node_id" is already
used outside of content/, and inventing a completely new name that means the
same thing is just confusing. (see next)

- Patch set 9 = I created a single class that is responsible for mapping
(RenderProcessHost ID, RenderFrameHost ID) pairs to an "extension frame ID"
(=FrameTreeNodeID). This class can be used on the UI thread and IO thread, and
the result is cached to minimize the number of thread hops in the webRequest
API.

https://codereview.chromium.org/1413543005/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/1413543005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:55: void
OnExtensionFrameUnregistered(
On 2015/11/13 21:16:27, Devlin wrote:
> I would hope that doing the lifetime management here would have meant that we
> could eliminate some of the crazy lifetime on the renderer; do we still need
> that (just for garbage collection)?

The logic at the renderer's side is not required any more. In the design of this
class, I'm only relying on the browser-side view of the frame tree to manager
the ports. The renderer is only involved in the lifetime when the port can be
closed earlier, e.g. because the port is no longer used (garbage-collected).

I won't tackle the cleanup of the renderer code in this CL though, because the
logic is quite complex and it's not required for this CL.

https://codereview.chromium.org/1413543005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:90:
ExtensionMessagePort::ExtensionMessagePort(
On 2015/11/13 21:16:27, Devlin wrote:
> nit: put two constructors next to each other.

Done.

https://codereview.chromium.org/1413543005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:212: void
ExtensionMessagePort::SendToPort(IPC::Message* msg) {
On 2015/11/13 21:16:27, Devlin wrote:
> nit: let's make this take a scoped_ptr (and just release it in the
> extension_process_->Send case).

Why do you want a scoped_ptr here? This method is only used internally, and
wrapping the three calls in make_scoped_ptr(new ExtensionMsg_XXX(...)) is not
more readable IMO.

https://codereview.chromium.org/1413543005/diff/80001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/messaging/connect_crash/page.js
(right):

https://codereview.chromium.org/1413543005/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/messaging/connect_crash/page.js:1: //
Copyright (c) 2015 The Chromium Authors. All rights reserved.
On 2015/11/13 21:16:27, Devlin wrote:
> nit: no (c) (everywhere)

Done.

https://codereview.chromium.org/1413543005/diff/80001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/frame.js
(right):

https://codereview.chromium.org/1413543005/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/frame.js:7:
if (location.hostname === "a.com") {
On 2015/11/13 21:16:27, Devlin wrote:
> nitty nit: prefer single-quotes in js (except to avoid escapes)

Done.

https://codereview.chromium.org/1413543005/diff/80001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js
(right):

https://codereview.chromium.org/1413543005/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js:148:
{ label: "pre-d-onBeforeNavigate",
On 2015/11/06 23:28:27, nasko wrote:
> Urgh, I might have been misreading these steps because of the labelling : (.
> Let's use the prefixes to be "main", "a-com", "b-com", "c-com", so they match
> the navigations we perform in the JS files.

Done.

https://codereview.chromium.org/1413543005/diff/80001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/1413543005/diff/80001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:771: int frame_id = -1;
On 2015/11/06 23:28:27, nasko wrote:
> I think this is the frame routing id, correct? If yes, let's name it
explicitly
> so it is clear which id we are using here.

Done.

https://codereview.chromium.org/1413543005/diff/80001/extensions/renderer/mes...
File extensions/renderer/messaging_bindings.cc (right):

https://codereview.chromium.org/1413543005/diff/80001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.cc:208: content::RenderFrame* renderframe
= context()->GetRenderFrame();
On 2015/11/06 23:28:27, nasko wrote:
> render_frame

Done.

https://codereview.chromium.org/1413543005/diff/80001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.cc:211: renderframe->GetRoutingID(),
port_id, true));
On 2015/11/06 23:28:27, nasko wrote:
> Only one line if statements can skip {}. It is needed here in this case.

Done.

https://codereview.chromium.org/1413543005/diff/80001/extensions/renderer/mes...
extensions/renderer/messaging_bindings.cc:239: content::RenderFrame* renderframe
= context()->GetRenderFrame();
On 2015/11/06 23:28:27, nasko wrote:
> render_frame

Done.

[Matt Perry, 2015-12-08 17:53:57]

I skimmed, but general approach LGTM

[nasko, 2015-12-09 23:06:10]

A bunch of comments. The main concern is the global map that we never delete
from.

https://codereview.chromium.org/1413543005/diff/180001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/main.js
(right):

https://codereview.chromium.org/1413543005/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/main.js:12:
// but this doesn't seem to work with OOP extensions.
Does https://codereview.chromium.org/1495993002/ help with this comment?

https://codereview.chromium.org/1413543005/diff/180001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js
(right):

https://codereview.chromium.org/1413543005/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js:28:
// embeds another frame navigates three times (cross-process):
nit: "frame that navigates"?

https://codereview.chromium.org/1413543005/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js:203:
['pre-a.com-onBeforeNavigate', 'pre-a.com-onErrorOccurred'],
Don't we want to ensure that pre-a.com-onErrorOccurred is followed by the
a.com-onBeforeNavigate? Same for pre-c.com.

https://codereview.chromium.org/1413543005/diff/180001/content/public/browser...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/1413543005/diff/180001/content/public/browser...
content/public/browser/render_frame_host.h:39: // Returns the RenderFrameHost
that is hosted by the frame identified by the
nit: s/hosted/associated/

https://codereview.chromium.org/1413543005/diff/180001/content/public/browser...
content/public/browser/render_frame_host.h:45: // Returns nullptr if the frame
does not exist (any more).
nit: You can drop the "(any more)".

https://codereview.chromium.org/1413543005/diff/180001/content/public/browser...
content/public/browser/render_frame_host.h:46: static RenderFrameHost*
FromFrameTreeNodeID(int frame_tree_node_id);
The conclusion on capitalization is using Id instead of ID.

https://codereview.chromium.org/1413543005/diff/180001/content/public/browser...
content/public/browser/render_frame_host.h:80: // uniquely identifies a frame
(including main frames) that hosts content. The
nit: Drop the "(including main frames)" as this is a bit of context specific to
the extension APIs.

https://codereview.chromium.org/1413543005/diff/180001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1413543005/diff/180001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:35:
base::LazyInstance<FrameIdMap> g_frame_id_map = LAZY_INSTANCE_INITIALIZER;
Why is this map needed and must grow unbound? Why not observe
WebContentsObserver::FrameDeleted and then you know that FTN is gone forever and
the ID will never be used again?

https://codereview.chromium.org/1413543005/diff/180001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:39: RenderFrameIdKey(int
process_id, int routing_id)
nit: frame_routing_id

https://codereview.chromium.org/1413543005/diff/180001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:68: if (rfh) {
We usually use early returns. Example pattern:

if (condition)
  return;

// more work

https://codereview.chromium.org/1413543005/diff/180001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.h (right):

https://codereview.chromium.org/1413543005/diff/180001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:37: // identifies a RenderFrame
to an extension frame ID.
nit: s/identifies/maps/? Also RenderFrameHost to be consistent with the rest of
the file.

https://codereview.chromium.org/1413543005/diff/180001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:48: int routing_id,
nit: frame_routing_id or render_frame_routing_id

https://codereview.chromium.org/1413543005/diff/180001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:62: content::WebContents*
web_contents);
WebContents should be the first parameter, as it is the main scope we are
searching in. Similarly to the "process_id, routing_id", the latter is scoped
within the first parameter.

https://codereview.chromium.org/1413543005/diff/180001/extensions/common/exte...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/1413543005/diff/180001/extensions/common/exte...
extensions/common/extension_messages.h:650: int /* routing_id */,
nit: frame_routing_id?

[robwu, 2015-12-10 00:00:51]

https://codereview.chromium.org/1413543005/diff/180001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/main.js
(right):

https://codereview.chromium.org/1413543005/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/main.js:12:
// but this doesn't seem to work with OOP extensions.
On 2015/12/09 23:06:09, nasko wrote:
> Does https://codereview.chromium.org/1495993002/ help with this comment?

Yes. Thanks!

https://codereview.chromium.org/1413543005/diff/180001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js
(right):

https://codereview.chromium.org/1413543005/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js:28:
// embeds another frame navigates three times (cross-process):
On 2015/12/09 23:06:09, nasko wrote:
> nit: "frame that navigates"?

Done.

https://codereview.chromium.org/1413543005/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js:203:
['pre-a.com-onBeforeNavigate', 'pre-a.com-onErrorOccurred'],
On 2015/12/09 23:06:09, nasko wrote:
> Don't we want to ensure that pre-a.com-onErrorOccurred is followed by the
> a.com-onBeforeNavigate? Same for http://pre-c.com.

That's what I want as well, but unfortunately the sequence is
pre-a.com-onBeforeNavigate
a.com-onBeforeNavigate
pre-a.com-onErrorOccurred

I say "unfortunate", because this makes frame tracking much harder for extension
devs; they have to keep track of processIds as well to deal with OOPIF.

(I can't remember whether I omitted a.com-onBeforeNavigate for this reason, or
because of flakiness)

https://codereview.chromium.org/1413543005/diff/180001/content/public/browser...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/1413543005/diff/180001/content/public/browser...
content/public/browser/render_frame_host.h:39: // Returns the RenderFrameHost
that is hosted by the frame identified by the
On 2015/12/09 23:06:09, nasko wrote:
> nit: s/hosted/associated/

Done.

https://codereview.chromium.org/1413543005/diff/180001/content/public/browser...
content/public/browser/render_frame_host.h:45: // Returns nullptr if the frame
does not exist (any more).
On 2015/12/09 23:06:09, nasko wrote:
> nit: You can drop the "(any more)".

Done.

https://codereview.chromium.org/1413543005/diff/180001/content/public/browser...
content/public/browser/render_frame_host.h:46: static RenderFrameHost*
FromFrameTreeNodeID(int frame_tree_node_id);
On 2015/12/09 23:06:09, nasko wrote:
> The conclusion on capitalization is using Id instead of ID.

Done.

https://codereview.chromium.org/1413543005/diff/180001/content/public/browser...
content/public/browser/render_frame_host.h:80: // uniquely identifies a frame
(including main frames) that hosts content. The
On 2015/12/09 23:06:09, nasko wrote:
> nit: Drop the "(including main frames)" as this is a bit of context specific
to
> the extension APIs.

Done.

https://codereview.chromium.org/1413543005/diff/180001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1413543005/diff/180001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:35:
base::LazyInstance<FrameIdMap> g_frame_id_map = LAZY_INSTANCE_INITIALIZER;
On 2015/12/09 23:06:09, nasko wrote:
> Why is this map needed and must grow unbound? Why not observe
> WebContentsObserver::FrameDeleted and then you know that FTN is gone forever
and
> the ID will never be used again?

Because I think that there is no guarantee that all network requests are
immediately dropped before FrameDeleted, or that there are no webNavigation
events after FrameDeleted (e.g. is there a defined order in RenderFrameDeleted
and FrameDeleted? The former triggers the webNavigation.onErrorOccurred event).

Do you think that it's worthwhile to use a timer or some form of a LRU queue to
bound the map size?

https://codereview.chromium.org/1413543005/diff/180001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:39: RenderFrameIdKey(int
process_id, int routing_id)
On 2015/12/09 23:06:09, nasko wrote:
> nit: frame_routing_id

Done. (everywhere, and also changed process_id to render_process_id).

https://codereview.chromium.org/1413543005/diff/180001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:68: if (rfh) {
On 2015/12/09 23:06:09, nasko wrote:
> We usually use early returns. Example pattern:
> 
> if (condition)
>   return;
> 
> // more work

Done.

https://codereview.chromium.org/1413543005/diff/180001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.h (right):

https://codereview.chromium.org/1413543005/diff/180001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:37: // identifies a RenderFrame
to an extension frame ID.
On 2015/12/09 23:06:09, nasko wrote:
> nit: s/identifies/maps/? Also RenderFrameHost to be consistent with the rest
of
> the file.

Done.

https://codereview.chromium.org/1413543005/diff/180001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:48: int routing_id,
On 2015/12/09 23:06:10, nasko wrote:
> nit: frame_routing_id or render_frame_routing_id

Done.

https://codereview.chromium.org/1413543005/diff/180001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:62: content::WebContents*
web_contents);
On 2015/12/09 23:06:09, nasko wrote:
> WebContents should be the first parameter, as it is the main scope we are
> searching in. Similarly to the "process_id, routing_id", the latter is scoped
> within the first parameter.

Done.

https://codereview.chromium.org/1413543005/diff/180001/extensions/common/exte...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/1413543005/diff/180001/extensions/common/exte...
extensions/common/extension_messages.h:650: int /* routing_id */,
On 2015/12/09 23:06:10, nasko wrote:
> nit: frame_routing_id?

Done. (everywhere)

[robwu, 2015-12-10 00:08:01]

Description was changed from

==========
Use FrameTreeNode ID as frameId in extension APIs

Use FrameTreeNode IDs instead of RenderFrame routing IDs as "frame id"
in the extension APIs (webNavigation, webRequest, extension messaging)
because FTN IDs are globally unique, and RFH IDs are not.

This extends the public content API with:
- RenderFrameHost::GetFrameTreeNodeID
- WebContents::FindByFrameTreeNodeID

The extension APIs are modified as follows:
- webRequest:
  * No visible changes for extension developers.
  * Hop from IO to UI and back to IO thread to determine the frameId.
- webNavigation:
  * processId is no longer required in chrome.webNavigation.getFrame,
    but marked as optional (deprecated) to make sure that the API is
    backwards-compatible.
  * frameId is constant across navigations.
- Extension messaging (chrome.runtime.connect and friends):
  * Small change for extension developers in the following scenario:
    1. Open port to tab.
    2. Accept the port by listening to onConnect *in a child frame*.
    3. Unload the document in the frame.
    Old behavior: onDisconnect was not triggered in the background until
                  the tab was reloaded or closed.
    New behavior: onDisconnect is called.
  * Extension messaging works correctly in out-of-process frames.
  * Move port lifetime management from renderer to browser.
  * Tie port lifetime to frames instead of processes.
  * Only notify frames in renderers if they have accepted the port.
  * Remove obsolete work-around for crbug.com/520303.
  * Unify open/close/postMessage logic in ChromeExtensionMessageFilter
    (crbug.com/394383#c7)

Depends on https://codereview.chromium.org/1413853005/

BUG=432875
TEST=browser_tests --gtest_filter=\
ExtensionWebRequestApiTest.*:\
WebNavigationApiTest.*:\
ExtensionApiTest.Messaging*:\
ExternallyConnectableMessagingTest.*:\
ExternallyConnectableMessagingWithTlsChannelIdTest.*
==========

to

==========
Use FrameTreeNode ID as frameId in extension APIs

Use FrameTreeNode IDs instead of RenderFrame routing IDs as "frame id"
in the extension APIs (webNavigation, webRequest, extension messaging)
because FTN IDs are globally unique, and RFH IDs are not.

This extends the public content API with:
- RenderFrameHost::GetFrameTreeNodeId
- WebContents::FindByFrameTreeNodeId

The extension APIs are modified as follows:
- webRequest:
  * No visible changes for extension developers.
  * Hop from IO to UI and back to IO thread to determine the frameId.
- webNavigation:
  * processId is no longer required in chrome.webNavigation.getFrame,
    but marked as optional (deprecated) to make sure that the API is
    backwards-compatible.
  * frameId is constant across navigations.
- Extension messaging (chrome.runtime.connect and friends):
  * Small change for extension developers in the following scenario:
    1. Open port to tab.
    2. Accept the port by listening to onConnect *in a child frame*.
    3. Unload the document in the frame.
    Old behavior: onDisconnect was not triggered in the background until
                  the tab was reloaded or closed.
    New behavior: onDisconnect is called.
  * Extension messaging works correctly in out-of-process frames.
  * Move port lifetime management from renderer to browser.
  * Tie port lifetime to frames instead of processes.
  * Only notify frames in renderers if they have accepted the port.
  * Remove obsolete work-around for crbug.com/520303.
  * Unify open/close/postMessage logic in ChromeExtensionMessageFilter
    (crbug.com/394383#c7)
- The extension frameId logic is no longer scattered over several files,
  but resides now in a single class (ExtensionApiFrameIdMap).

Depends on https://codereview.chromium.org/1413853005/

BUG=432875
TEST=browser_tests --gtest_filter=\
ExtensionWebRequestApiTest.*:\
WebNavigationApiTest.*:\
ExtensionApiTest.Messaging*:\
ExternallyConnectableMessagingTest.*:\
ExternallyConnectableMessagingWithTlsChannelIdTest.*
==========

[Charlie Reis, 2015-12-10 07:20:21]

content/ LGTM with nits.

https://codereview.chromium.org/1413543005/diff/200001/content/public/browser...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/1413543005/diff/200001/content/public/browser...
content/public/browser/render_frame_host.h:39: // Returns the RenderFrameHost
that is associated with the frame identified by
nit: Returns the current RenderFrameHost associated with the frame identified by
the given FrameTreeNode ID, in any WebContents.

https://codereview.chromium.org/1413543005/diff/200001/content/public/browser...
content/public/browser/render_frame_host.h:40: // the given FrameTreeNode ID.
The returned RenderFrameHost can be different
nit: The frame may change its current RenderFrameHost over time, so the
returned...

https://codereview.chromium.org/1413543005/diff/200001/content/public/browser...
content/public/browser/render_frame_host.h:42: // See the documentation at
GetFrameTreeNodeId for more details.
nit: See GetFrameTreeNodeId for more details.

https://codereview.chromium.org/1413543005/diff/200001/content/public/browser...
content/public/browser/render_frame_host.h:86: // the FrameTreeNode ID may refer
to a different RenderFrameHost after a
nit: the same FrameTreeNode ID

[robwu, 2015-12-10 10:47:12]

Description was changed from

==========
Use FrameTreeNode ID as frameId in extension APIs

Use FrameTreeNode IDs instead of RenderFrame routing IDs as "frame id"
in the extension APIs (webNavigation, webRequest, extension messaging)
because FTN IDs are globally unique, and RFH IDs are not.

This extends the public content API with:
- RenderFrameHost::GetFrameTreeNodeId
- WebContents::FindByFrameTreeNodeId

The extension APIs are modified as follows:
- webRequest:
  * No visible changes for extension developers.
  * Hop from IO to UI and back to IO thread to determine the frameId.
- webNavigation:
  * processId is no longer required in chrome.webNavigation.getFrame,
    but marked as optional (deprecated) to make sure that the API is
    backwards-compatible.
  * frameId is constant across navigations.
- Extension messaging (chrome.runtime.connect and friends):
  * Small change for extension developers in the following scenario:
    1. Open port to tab.
    2. Accept the port by listening to onConnect *in a child frame*.
    3. Unload the document in the frame.
    Old behavior: onDisconnect was not triggered in the background until
                  the tab was reloaded or closed.
    New behavior: onDisconnect is called.
  * Extension messaging works correctly in out-of-process frames.
  * Move port lifetime management from renderer to browser.
  * Tie port lifetime to frames instead of processes.
  * Only notify frames in renderers if they have accepted the port.
  * Remove obsolete work-around for crbug.com/520303.
  * Unify open/close/postMessage logic in ChromeExtensionMessageFilter
    (crbug.com/394383#c7)
- The extension frameId logic is no longer scattered over several files,
  but resides now in a single class (ExtensionApiFrameIdMap).

Depends on https://codereview.chromium.org/1413853005/

BUG=432875
TEST=browser_tests --gtest_filter=\
ExtensionWebRequestApiTest.*:\
WebNavigationApiTest.*:\
ExtensionApiTest.Messaging*:\
ExternallyConnectableMessagingTest.*:\
ExternallyConnectableMessagingWithTlsChannelIdTest.*
==========

to

==========
Use FrameTreeNode ID as frameId in extension APIs

Use FrameTreeNode IDs instead of RenderFrame routing IDs as "frame id"
in the extension APIs (webNavigation, webRequest, extension messaging)
because FTN IDs are globally unique, and RFH IDs are not.

This extends the public content API with:
- RenderFrameHost::FromFrameTreeNodeId
- RenderFrameHost::GetFrameTreeNodeId
- WebContents::FindByFrameTreeNodeId

The extension APIs are modified as follows:
- webRequest:
  * No visible changes for extension developers.
  * Hop from IO to UI and back to IO thread to determine the frameId.
- webNavigation:
  * processId is no longer required in chrome.webNavigation.getFrame,
    but marked as optional (deprecated) to make sure that the API is
    backwards-compatible.
  * frameId is constant across navigations.
- Extension messaging (chrome.runtime.connect and friends):
  * Small change for extension developers in the following scenario:
    1. Open port to tab.
    2. Accept the port by listening to onConnect *in a child frame*.
    3. Unload the document in the frame.
    Old behavior: onDisconnect was not triggered in the background until
                  the tab was reloaded or closed.
    New behavior: onDisconnect is called.
  * Extension messaging works correctly in out-of-process frames.
  * Move port lifetime management from renderer to browser.
  * Tie port lifetime to frames instead of processes.
  * Only notify frames in renderers if they have accepted the port.
  * Remove obsolete work-around for crbug.com/520303.
  * Unify open/close/postMessage logic in ChromeExtensionMessageFilter
    (crbug.com/394383#c7)
- The extension frameId logic is no longer scattered over several files,
  but resides in a single class (ExtensionApiFrameIdMap).

Depends on https://codereview.chromium.org/1413853005/

BUG=432875
TEST=browser_tests --gtest_filter=\
ExtensionWebRequestApiTest.*:\
WebNavigationApiTest.*:\
ExtensionApiTest.Messaging*:\
ExternallyConnectableMessagingTest.*:\
ExternallyConnectableMessagingWithTlsChannelIdTest.*
==========

[battre, 2015-12-10 15:31:39]

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:231: // ExtractFrameIdInfo
will overwrite kFrameIdKey, so it is not removed here.
nit: ExtractApiFrameFrameIdInfo?

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:232:
dict->Remove(keys::kProcessIdKey, nullptr);
It feels weird to me that the extract function has the side effect of modifying
dict.

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:236: const
ExtensionApiFrameId& extension_api_frame_id) {
nit: do you want to put the output parameter last?
https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:550: }
nit: no {}?

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:1299:
base::Passed(&listeners_to_dispatch), base::Owned(args_copy)));
Just checking: Can this asynchronicity rearrange events in such a way that a
subresource is reported before its frame has been reported?

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:1313: void
ExtensionWebRequestEventRouter::DispatchEventOnIO(
I think this naming is confusing because the other DispatchEvent is also
executed on the IO thread.

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:35:
base::LazyInstance<FrameIdMap> g_frame_id_map = LAZY_INSTANCE_INITIALIZER;
Is it ok that data in this map is never removed? Thinking of memory consumption
and collision of IDs due to the re-use of process IDs.

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:67: void
InitializeFromFrame(ExtensionApiFrameId& extension_api_frame_id,
https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering

Parameter order and output parameter should be a pointer.

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:96: FrameIdMap& frame_id_map =
g_frame_id_map.Get();
In LookupFrameIdOnUI you are accessing this on the UI thread. Isn't that a
recipe for disaster?

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.h (right):

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:42: // Runs |callback| with the
result of calling GetFrameIdOnUI(). If the result
nit: GetFrameIdOnUI() is not described here.

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:55: static const
ExtensionApiFrameId& GetFrameId(int render_process_id,
is it guaranteed that proccess ids are never reused?

[robwu, 2015-12-10 16:39:30]

https://codereview.chromium.org/1413543005/diff/200001/content/public/browser...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/1413543005/diff/200001/content/public/browser...
content/public/browser/render_frame_host.h:39: // Returns the RenderFrameHost
that is associated with the frame identified by
On 2015/12/10 07:20:20, Charlie Reis wrote:
> nit: Returns the current RenderFrameHost associated with the frame identified
by
> the given FrameTreeNode ID, in any WebContents.

Done.

https://codereview.chromium.org/1413543005/diff/200001/content/public/browser...
content/public/browser/render_frame_host.h:40: // the given FrameTreeNode ID.
The returned RenderFrameHost can be different
On 2015/12/10 07:20:20, Charlie Reis wrote:
> nit: The frame may change its current RenderFrameHost over time, so the
> returned...

Done.

https://codereview.chromium.org/1413543005/diff/200001/content/public/browser...
content/public/browser/render_frame_host.h:42: // See the documentation at
GetFrameTreeNodeId for more details.
On 2015/12/10 07:20:20, Charlie Reis wrote:
> nit: See GetFrameTreeNodeId for more details.

Done.

https://codereview.chromium.org/1413543005/diff/200001/content/public/browser...
content/public/browser/render_frame_host.h:86: // the FrameTreeNode ID may refer
to a different RenderFrameHost after a
On 2015/12/10 07:20:20, Charlie Reis wrote:
> nit: the same FrameTreeNode ID

Done.

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:231: // ExtractFrameIdInfo
will overwrite kFrameIdKey, so it is not removed here.
On 2015/12/10 15:31:39, battre wrote:
> nit: ExtractApiFrameFrameIdInfo?

Done (using 1x Frame though: ExtractApiFrameIdInfo)

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:232:
dict->Remove(keys::kProcessIdKey, nullptr);
On 2015/12/10 15:31:39, battre wrote:
> It feels weird to me that the extract function has the side effect of
modifying
> dict.

Indeed. That's why I document that processId is modified. I'd like to submit
another CL that refactors this class (using a struct to hold the DictionaryValue
plus frame routing id / process routing id) instead of passing a ListValue
around that has only one DictionaryValue...

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:236: const
ExtensionApiFrameId& extension_api_frame_id) {
On 2015/12/10 15:31:39, battre wrote:
> nit: do you want to put the output parameter last?
> https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering

Done.

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:550: }
On 2015/12/10 15:31:39, battre wrote:
> nit: no {}?

Done.

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:1299:
base::Passed(&listeners_to_dispatch), base::Owned(args_copy)));
On 2015/12/10 15:31:39, battre wrote:
> Just checking: Can this asynchronicity rearrange events in such a way that a
> subresource is reported before its frame has been reported?

Good point. In theory, this is certainly possible because PostTask does not
guarantee the order in which the tasks are run.

My ExtensionApiFrameId class only guarantees that lookup requests for the same
ID as processed in order. If this theoretical possiblity turns out to be a real
issue, then I can send another update that synchronizes parent/child frame IDs,
e.g. queuing callbacks when the parent frame ID is not known, and replying with
the whole path to the root when a frame is looked up. This has some overhead, so
I'd like to wait and see whether it is a real problem.

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:1313: void
ExtensionWebRequestEventRouter::DispatchEventOnIO(
On 2015/12/10 15:31:39, battre wrote:
> I think this naming is confusing because the other DispatchEvent is also
> executed on the IO thread.

I've renamed it to DispatchEventToListeners.

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:35:
base::LazyInstance<FrameIdMap> g_frame_id_map = LAZY_INSTANCE_INITIALIZER;
On 2015/12/10 15:31:39, battre wrote:
> Is it ok that data in this map is never removed? Thinking of memory
consumption
> and collision of IDs due to the re-use of process IDs.

Collisions are not probably because those process IDs are not OS process IDs,
but RenderProcessHost IDs (32-bit unsigned integers).

I'm currently not sure when it is safe to remove items from the map. Nasko did
already mention that the map is unbounded, we can continue the discussion at
https://codereview.chromium.org/1413543005/diff/180001/extensions/browser/ext....

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:67: void
InitializeFromFrame(ExtensionApiFrameId& extension_api_frame_id,
On 2015/12/10 15:31:39, battre wrote:
> https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering
> 
> Parameter order and output parameter should be a pointer.

Done.

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:96: FrameIdMap& frame_id_map =
g_frame_id_map.Get();
On 2015/12/10 15:31:39, battre wrote:
> In LookupFrameIdOnUI you are accessing this on the UI thread. Isn't that a
> recipe for disaster?

This is the sequence of actions:

GetFrameIdOnIO:
- If list of callbacks is non-empty, queue callback and return.
- If frame_id is found, run callback with frame_id and return.
- Otherwise queue callback and PostTask to UI thread.

LookupFrameIdOnUI:
- Store frame_id in map

GotFrameId (on IO, posted back after LookupFrameIdOnUI returned):
- Run every callback with the frame_id
- Remove callbacks.

This seems safe, but it is not because I did not account for the fact that
LookupFrameIdOnUI can be called on the UI thread (I added this later). Would a
lock for accessing the map a good solution, or can you think of something
better?

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.h (right):

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:42: // Runs |callback| with the
result of calling GetFrameIdOnUI(). If the result
On 2015/12/10 15:31:39, battre wrote:
> nit: GetFrameIdOnUI() is not described here.

Done.

https://codereview.chromium.org/1413543005/diff/220001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:55: static const
ExtensionApiFrameId& GetFrameId(int render_process_id,
On 2015/12/10 15:31:39, battre wrote:
> is it guaranteed that proccess ids are never reused?

Yes, see RenderProcessHost::GetID.

[Devlin, 2015-12-10 21:53:07]

FYI: I'm pretty much leaving the following extension-y files to other reviewers:
WebNavigation stuff - Nasko
WebRequest stuff - Matt and Dominic

https://codereview.chromium.org/1413543005/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/1413543005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:55: void
OnExtensionFrameUnregistered(
On 2015/12/07 23:44:21, robwu wrote:
> On 2015/11/13 21:16:27, Devlin wrote:
> > I would hope that doing the lifetime management here would have meant that
we
> > could eliminate some of the crazy lifetime on the renderer; do we still need
> > that (just for garbage collection)?
> 
> The logic at the renderer's side is not required any more. In the design of
this
> class, I'm only relying on the browser-side view of the frame tree to manager
> the ports. The renderer is only involved in the lifetime when the port can be
> closed earlier, e.g. because the port is no longer used (garbage-collected).
> 
> I won't tackle the cleanup of the renderer code in this CL though, because the
> logic is quite complex and it's not required for this CL.

Can you add a TODO to clean it up?  I'm also a little nervous that we'll miss
bugs in the browser implementation because the renderer-side logic will
compensate... but I also don't want to see this patch get larger. :/

https://codereview.chromium.org/1413543005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:212: void
ExtensionMessagePort::SendToPort(IPC::Message* msg) {
On 2015/12/07 23:44:21, robwu wrote:
> On 2015/11/13 21:16:27, Devlin wrote:
> > nit: let's make this take a scoped_ptr (and just release it in the
> > extension_process_->Send case).
> 
> Why do you want a scoped_ptr here? This method is only used internally, and
> wrapping the three calls in make_scoped_ptr(new ExtensionMsg_XXX(...)) is not
> more readable IMO.

because code is read, and comments aren't.  Passing a scoped_ptr is
auto-documentation for the ownership.

https://codereview.chromium.org/1413543005/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/1413543005/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/extension_message_port.cc:30: :
pm_observer_(this), port_(port) {}
Add an explicit destructor.

https://codereview.chromium.org/1413543005/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/messaging/extension_message_port.h (right):

https://codereview.chromium.org/1413543005/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/extension_message_port.h:64: void
RegisterFrame(content::RenderFrameHost* rfh);
Most of these should have some kind of comment (though Register/UnregisterFrame
can share one).

https://codereview.chromium.org/1413543005/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/extension_message_port.h:69:
base::WeakPtr<MessageService> weak_message_service_;
here, too, many of these should have a comment.

https://codereview.chromium.org/1413543005/diff/180001/chrome/common/extensio...
File chrome/common/extensions/docs/templates/intros/webNavigation.html (right):

https://codereview.chromium.org/1413543005/diff/180001/chrome/common/extensio...
chrome/common/extensions/docs/templates/intros/webNavigation.html:111: the
document. As of Chrome 48, this ID is also constant for the lifetime of the
Chrome 49?

https://codereview.chromium.org/1413543005/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/api/messaging/extension_message_port.h (right):

https://codereview.chromium.org/1413543005/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/extension_message_port.h:44: bool
IsValidPort() override;
// MessageService::MessagePort:

https://codereview.chromium.org/1413543005/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/api/messaging/message_service.cc (right):

https://codereview.chromium.org/1413543005/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/message_service.cc:565: // TODO(robwu):
Could |guest_process_id| and |guest_render_frame_routing_id|
I think they can, yes (but let's save it for another CL).

https://codereview.chromium.org/1413543005/diff/240001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/messaging/connect_crash/test.js
(right):

https://codereview.chromium.org/1413543005/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/messaging/connect_crash/test.js:17:
console.log('port.onDisconnect was triggered.');
chrome.test.log

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:222: void
ExtractRenderFrameInfo(base::DictionaryValue* dict,
Function comment.

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:225: if
(dict->GetInteger(keys::kFrameIdKey, render_frame_id) &&
I'd much rather have this be
if (!dict->GetInt ||
    !dict->GetInt) {
  // set to -1
}

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:228: *render_process_id =
-1;
Should we reset both if either isn't present in the dict?

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:236: void
ExtractApiFrameIdInfo(const ExtensionApiFrameId& extension_api_frame_id,
function comment

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:21: struct RenderFrameIdKey;
Can we just defined RenderFrameIdKey here?

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:22: typedef
std::list<ExtensionApiFrameIdMap::FrameIdCallback> FrameIdCallbackList;
prefer using

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:67: void
InitializeFromFrame(content::RenderFrameHost* rfh,
Seems like this could just be a constructor of ExtensionApiFrameId?

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:85: ExtensionApiFrameId&
extension_api_frame_id = frame_id_map[key];
and then this set would become something like:
// Insert and return a new entry for the |rfh|.
return frame_id_map.insert(ExtensionApiFrameId(rfh)).first->second;

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:92: void GotFrameId(const
RenderFrameIdKey& key) {
put this in the anonymous namespace?

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:103: // appended to the list.
So we cannot use a fancy range-based for loop.
Heh.  Tricky.

Might be worth a comment on FrameIdCallbackList that it needs to be a std::list
so that iterators aren't invalidated in push_back().

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:128: FrameIdMap& frame_id_map =
g_frame_id_map.Get();
Is this safe?  Map lookup operations aren't purely atomic, so if it's also being
modified on the UI thread, won't we have a big problem?

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.h (right):

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:17: // Extension frame IDs are
exposed through the extension API and have the
"the extension API"?  That sounds like chrome.extension.  Maybe just "through
APIs" (extension can be implied).

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:38: class ExtensionApiFrameIdMap
{
Does this class warrant a few small unit tests?

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:40: typedef
base::Callback<void(const ExtensionApiFrameId&)> FrameIdCallback;
nit: Prefer using.

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:49: FrameIdCallback callback);
const &

[robwu, 2015-12-11 00:48:03]

Patchset #13 (id:260001) has been deleted

[robwu, 2015-12-11 00:53:48]

https://codereview.chromium.org/1413543005/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/1413543005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:55: void
OnExtensionFrameUnregistered(
On 2015/12/10 21:53:06, Devlin wrote:
> On 2015/12/07 23:44:21, robwu wrote:
> > On 2015/11/13 21:16:27, Devlin wrote:
> > > I would hope that doing the lifetime management here would have meant that
> we
> > > could eliminate some of the crazy lifetime on the renderer; do we still
need
> > > that (just for garbage collection)?
> > 
> > The logic at the renderer's side is not required any more. In the design of
> this
> > class, I'm only relying on the browser-side view of the frame tree to
manager
> > the ports. The renderer is only involved in the lifetime when the port can
be
> > closed earlier, e.g. because the port is no longer used (garbage-collected).
> > 
> > I won't tackle the cleanup of the renderer code in this CL though, because
the
> > logic is quite complex and it's not required for this CL.
> 
> Can you add a TODO to clean it up?  I'm also a little nervous that we'll miss
> bugs in the browser implementation because the renderer-side logic will
> compensate... but I also don't want to see this patch get larger. :/

Done.

https://codereview.chromium.org/1413543005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/extension_message_port.cc:212: void
ExtensionMessagePort::SendToPort(IPC::Message* msg) {
On 2015/12/10 21:53:06, Devlin wrote:
> On 2015/12/07 23:44:21, robwu wrote:
> > On 2015/11/13 21:16:27, Devlin wrote:
> > > nit: let's make this take a scoped_ptr (and just release it in the
> > > extension_process_->Send case).
> > 
> > Why do you want a scoped_ptr here? This method is only used internally, and
> > wrapping the three calls in make_scoped_ptr(new ExtensionMsg_XXX(...)) is
not
> > more readable IMO.
> 
> because code is read, and comments aren't.  Passing a scoped_ptr is
> auto-documentation for the ownership.

Done.

https://codereview.chromium.org/1413543005/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/1413543005/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/extension_message_port.cc:30: :
pm_observer_(this), port_(port) {}
On 2015/12/10 21:53:06, Devlin wrote:
> Add an explicit destructor.

Done.

https://codereview.chromium.org/1413543005/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/messaging/extension_message_port.h (right):

https://codereview.chromium.org/1413543005/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/extension_message_port.h:64: void
RegisterFrame(content::RenderFrameHost* rfh);
On 2015/12/10 21:53:06, Devlin wrote:
> Most of these should have some kind of comment (though
Register/UnregisterFrame
> can share one).

Done.

https://codereview.chromium.org/1413543005/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/extension_message_port.h:69:
base::WeakPtr<MessageService> weak_message_service_;
On 2015/12/10 21:53:06, Devlin wrote:
> here, too, many of these should have a comment.

Done.

https://codereview.chromium.org/1413543005/diff/180001/chrome/common/extensio...
File chrome/common/extensions/docs/templates/intros/webNavigation.html (right):

https://codereview.chromium.org/1413543005/diff/180001/chrome/common/extensio...
chrome/common/extensions/docs/templates/intros/webNavigation.html:111: the
document. As of Chrome 48, this ID is also constant for the lifetime of the
On 2015/12/10 21:53:06, Devlin wrote:
> Chrome 49?

Done.

https://codereview.chromium.org/1413543005/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/api/messaging/extension_message_port.h (right):

https://codereview.chromium.org/1413543005/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/extension_message_port.h:44: bool
IsValidPort() override;
On 2015/12/10 21:53:06, Devlin wrote:
> // MessageService::MessagePort:

Done.

https://codereview.chromium.org/1413543005/diff/240001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/messaging/connect_crash/test.js
(right):

https://codereview.chromium.org/1413543005/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/messaging/connect_crash/test.js:17:
console.log('port.onDisconnect was triggered.');
On 2015/12/10 21:53:06, Devlin wrote:
> chrome.test.log

Done.

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:222: void
ExtractRenderFrameInfo(base::DictionaryValue* dict,
On 2015/12/10 21:53:07, Devlin wrote:
> Function comment.

Done.

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:225: if
(dict->GetInteger(keys::kFrameIdKey, render_frame_id) &&
On 2015/12/10 21:53:07, Devlin wrote:
> I'd much rather have this be
> if (!dict->GetInt ||
>     !dict->GetInt) {
>   // set to -1
> }

Done.

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:228: *render_process_id =
-1;
On 2015/12/10 21:53:07, Devlin wrote:
> Should we reset both if either isn't present in the dict?

Yes. A frame can only be identified if both IDs are present.

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:236: void
ExtractApiFrameIdInfo(const ExtensionApiFrameId& extension_api_frame_id,
On 2015/12/10 21:53:07, Devlin wrote:
> function comment

Done.

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:21: struct RenderFrameIdKey;
On 2015/12/10 21:53:07, Devlin wrote:
> Can we just defined RenderFrameIdKey here?

Done.

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:22: typedef
std::list<ExtensionApiFrameIdMap::FrameIdCallback> FrameIdCallbackList;
On 2015/12/10 21:53:07, Devlin wrote:
> prefer using

Done.

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:67: void
InitializeFromFrame(content::RenderFrameHost* rfh,
On 2015/12/10 21:53:07, Devlin wrote:
> Seems like this could just be a constructor of ExtensionApiFrameId?

Done.

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:85: ExtensionApiFrameId&
extension_api_frame_id = frame_id_map[key];
On 2015/12/10 21:53:07, Devlin wrote:
> and then this set would become something like:
> // Insert and return a new entry for the |rfh|.
> return frame_id_map.insert(ExtensionApiFrameId(rfh)).first->second;

Done.

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:92: void GotFrameId(const
RenderFrameIdKey& key) {
On 2015/12/10 21:53:07, Devlin wrote:
> put this in the anonymous namespace?

Done.

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:103: // appended to the list.
So we cannot use a fancy range-based for loop.
On 2015/12/10 21:53:07, Devlin wrote:
> Heh.  Tricky.
> 
> Might be worth a comment on FrameIdCallbackList that it needs to be a
std::list
> so that iterators aren't invalidated in push_back().

Done.

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:128: FrameIdMap& frame_id_map =
g_frame_id_map.Get();
On 2015/12/10 21:53:07, Devlin wrote:
> Is this safe?  Map lookup operations aren't purely atomic, so if it's also
being
> modified on the UI thread, won't we have a big problem?

The initial version was safe, but after adding the ability to trigger a write
from the UI thread, it is no longer safe. I've added a lock for protection.

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.h (right):

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:17: // Extension frame IDs are
exposed through the extension API and have the
On 2015/12/10 21:53:07, Devlin wrote:
> "the extension API"?  That sounds like chrome.extension.  Maybe just "through
> APIs" (extension can be implied).

Changed to "through the chrome.* APIs"

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:38: class ExtensionApiFrameIdMap
{
On 2015/12/10 21:53:07, Devlin wrote:
> Does this class warrant a few small unit tests?

The functionality is already covered by the consumers and their api tests + unit
tests, and I was being lazy here because I did not know whether my approach
would be accepted.

I'll add a few simple tests for the following scenarios:
- Call GetFrameIdOnIO on IO thread.
- Call GetFrameId on UI thread.
- Call both methods concurrently on the IO and IO thread.
- Verify result of GetRenderFrameHostById.

I haven't written tests that ran on both the IO and UI thread before, but I'll
work it out.

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:40: typedef
base::Callback<void(const ExtensionApiFrameId&)> FrameIdCallback;
On 2015/12/10 21:53:07, Devlin wrote:
> nit: Prefer using.

Done.

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:49: FrameIdCallback callback);
On 2015/12/10 21:53:07, Devlin wrote:
> const &

Done.

[Devlin, 2015-12-12 14:25:00]

Getting close!!

Nasko and/or Charlie, please chime in on the comment regarding clearing the map
in extensions/browser/extension_api_frame_id_map.cc.

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:92: void GotFrameId(const
RenderFrameIdKey& key) {
On 2015/12/11 00:53:48, robwu wrote:
> On 2015/12/10 21:53:07, Devlin wrote:
> > put this in the anonymous namespace?
> 
> Done.

Wasn't done?

https://codereview.chromium.org/1413543005/diff/300001/chrome/browser/extensi...
File chrome/browser/extensions/api/messaging/message_service.h (right):

https://codereview.chromium.org/1413543005/diff/300001/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/message_service.h:52: // port: One
sender or receivers tied to one or more RenderFrameHost instances.
nit: receiver (singular)

https://codereview.chromium.org/1413543005/diff/300001/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/message_service.h:157: // Acknowledges
open port in the frame identified by (process_id, routing_id).
"Acknowledges" is a little misleading - it also (usually) opens a port. ;)

https://codereview.chromium.org/1413543005/diff/300001/chrome/common/extensio...
File chrome/common/extensions/api/web_navigation.json (right):

https://codereview.chromium.org/1413543005/diff/300001/chrome/common/extensio...
chrome/common/extensions/api/web_navigation.json:37: "deprecated": "Frames are
now uniquely identified by their frame ID; the process ID is no longer needed
and therefore ignored.",
nitty nit: uniquely identified by tabId + frameID

https://codereview.chromium.org/1413543005/diff/300001/chrome/common/extensio...
File chrome/common/extensions/docs/templates/intros/webNavigation.html (right):

https://codereview.chromium.org/1413543005/diff/300001/chrome/common/extensio...
chrome/common/extensions/docs/templates/intros/webNavigation.html:122:
<code>processId</code> key.
Isn't processId deprecated?

https://codereview.chromium.org/1413543005/diff/300001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1413543005/diff/300001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:47:
base::LazyInstance<CallbackMap> g_callbacks_map = LAZY_INSTANCE_INITIALIZER;
When we have 3 different members, I think it would make sense to just have them
be private fields on ExtensionApiFrameIdMap and have a single lazy instance to
that object.

https://codereview.chromium.org/1413543005/diff/300001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:53: // Items are never removed
from this map.
This actually makes me a little worried.  Frames are pretty common on the web,
so I wouldn't be surprised if this grew to be, well, large enough to matter
(tens of thousands) - especially since people don't restart Chrome anymore. ;)

I'll defer to Nasko or Charlie who will probably have a better insight into
whether or not this is a concern, but my gut tells me we'll probably want to
clean this up occasionally.

https://codereview.chromium.org/1413543005/diff/300001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:95: void GotFrameId(const
RenderFrameIdKey& key) {
nit: let's add "OnIO"

https://codereview.chromium.org/1413543005/diff/300001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:98: // |g_frame_id_map_lock| is
not needed here because GotFrameId is only called
Wow, talk about relying on C++'s underlying guarantees.

I'm pretty sure a map *is* one of those objects that allows concurrent access
and insertion, but this still seems super subtle to me.  Let's see what happens,
but I'm not going to be entirely surprised if we find some bizarre exception as
a result of this.

And, looking at this more, would it be safe to just use
PostTaskAndReplyWithResult() in GetFrameIdOnIO, so that we don't have to do a
lookup at all?

https://codereview.chromium.org/1413543005/diff/300001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:117: callbacks_map.erase(key);
In conjunction with the comment on line 108 and the logic on line 128, doesn't
this mean the callback that could potentially be added will never be called?

https://codereview.chromium.org/1413543005/diff/300001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:136: bool is_frame_id_found;
nit: initialize.

https://codereview.chromium.org/1413543005/diff/300001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:163: const ExtensionApiFrameId&
ExtensionApiFrameIdMap::GetFrameId(
Should this also use LookupFrameIdOnUI?

https://codereview.chromium.org/1413543005/diff/300001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.h (right):

https://codereview.chromium.org/1413543005/diff/300001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:23: // - Multiple
RenderFrameHosts may be mapped to the same ID.
it's probably superfluous with above, but let's be explicit and say "mapped to
the same ID within different tabs".

[Charlie Reis, 2015-12-14 20:45:00]

https://codereview.chromium.org/1413543005/diff/300001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1413543005/diff/300001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:53: // Items are never removed
from this map.
On 2015/12/12 14:25:00, Devlin wrote:
> This actually makes me a little worried.  Frames are pretty common on the web,
> so I wouldn't be surprised if this grew to be, well, large enough to matter
> (tens of thousands) - especially since people don't restart Chrome anymore. ;)
> 
> I'll defer to Nasko or Charlie who will probably have a better insight into
> whether or not this is a concern, but my gut tells me we'll probably want to
> clean this up occasionally.

Yes, this seems like a nontrivial leak to me, since some pages can have a lot of
iframe churn.  And especially so if this is a record of all frames that have
ever existed in this session.

[robwu, 2015-12-14 20:55:42]

(posting my pending comments to get them out of the door - I will continue with
the remaining issues later this week)

The first two of the new patch sets (15 & 16) addresses small nits, the last
patch set (17) contains unit tests for ExtensionApiFrameIdMap.

Next plans:
- Ensure that all IDs of a frame up to the root has been resolved, to guarantee
that the callback for the parent frame is called before the child frame.
- Periodically erase entries in the map.
- (Maybe) change return value from "const ExtensionApiFrameId&" to
ExtensionApiFrameId, to avoid having to look up the value in the map again when
the UI thread replies to the IO thread (in GotFrameIdOnIO).

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1413543005/diff/240001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:92: void GotFrameId(const
RenderFrameIdKey& key) {
On 2015/12/12 14:25:00, Devlin wrote:
> On 2015/12/11 00:53:48, robwu wrote:
> > On 2015/12/10 21:53:07, Devlin wrote:
> > > put this in the anonymous namespace?
> > 
> > Done.
> 
> Wasn't done?

Done in 15, but made redundant by making it a private member of the class.

https://codereview.chromium.org/1413543005/diff/300001/chrome/browser/extensi...
File chrome/browser/extensions/api/messaging/message_service.h (right):

https://codereview.chromium.org/1413543005/diff/300001/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/message_service.h:52: // port: One
sender or receivers tied to one or more RenderFrameHost instances.
On 2015/12/12 14:25:00, Devlin wrote:
> nit: receiver (singular)

Done.

https://codereview.chromium.org/1413543005/diff/300001/chrome/browser/extensi...
chrome/browser/extensions/api/messaging/message_service.h:157: // Acknowledges
open port in the frame identified by (process_id, routing_id).
On 2015/12/12 14:25:00, Devlin wrote:
> "Acknowledges" is a little misleading - it also (usually) opens a port. ;)

I changed this to "Mark the given port as opened by the frame identified by
(process_id, routing_id)." (this is also more in line with the documentation for
MessagePort::OpenPort (not to be confused with MessageService::OpenPort)).

https://codereview.chromium.org/1413543005/diff/300001/chrome/common/extensio...
File chrome/common/extensions/api/web_navigation.json (right):

https://codereview.chromium.org/1413543005/diff/300001/chrome/common/extensio...
chrome/common/extensions/api/web_navigation.json:37: "deprecated": "Frames are
now uniquely identified by their frame ID; the process ID is no longer needed
and therefore ignored.",
On 2015/12/12 14:25:00, Devlin wrote:
> nitty nit: uniquely identified by tabId + frameID

Done.

https://codereview.chromium.org/1413543005/diff/300001/chrome/common/extensio...
File chrome/common/extensions/docs/templates/intros/webNavigation.html (right):

https://codereview.chromium.org/1413543005/diff/300001/chrome/common/extensio...
chrome/common/extensions/docs/templates/intros/webNavigation.html:122:
<code>processId</code> key.
On 2015/12/12 14:25:00, Devlin wrote:
> Isn't processId deprecated?

Nope, only for chrome.webNavigation.getFrame.
I reverted the deprecation of processId upon request of Nasko in comment
https://codereview.chromium.org/1413543005/#msg22.

(and processId appears to be necessary for identifying frames during a process
swap -
https://codereview.chromium.org/1413543005/diff/180001/chrome/test/data/exten...)

https://codereview.chromium.org/1413543005/diff/300001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1413543005/diff/300001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:47:
base::LazyInstance<CallbackMap> g_callbacks_map = LAZY_INSTANCE_INITIALIZER;
On 2015/12/12 14:25:00, Devlin wrote:
> When we have 3 different members, I think it would make sense to just have
them
> be private fields on ExtensionApiFrameIdMap and have a single lazy instance to
> that object.

Done (separately from the other changes, in patch set 16).

https://codereview.chromium.org/1413543005/diff/300001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:95: void GotFrameId(const
RenderFrameIdKey& key) {
On 2015/12/12 14:25:00, Devlin wrote:
> nit: let's add "OnIO"

Done.

https://codereview.chromium.org/1413543005/diff/300001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:98: // |g_frame_id_map_lock| is
not needed here because GotFrameId is only called
On 2015/12/12 14:25:00, Devlin wrote:
> Wow, talk about relying on C++'s underlying guarantees.
> 
> I'm pretty sure a map *is* one of those objects that allows concurrent access
> and insertion, but this still seems super subtle to me.  Let's see what
happens,
> but I'm not going to be entirely surprised if we find some bizarre exception
as
> a result of this.
> 
> And, looking at this more, would it be safe to just use
> PostTaskAndReplyWithResult() in GetFrameIdOnIO, so that we don't have to do a
> lookup at all?

It is indeed safe because the pointers are valid (due to their storage in the
global map), and I tried this at first, but it didn't compile.
Did you ever manage to have a function with return value "const T&" and bind it
using base::Bind?

An alternative is just changing the const ExtensionApiFrameId& signature to
ExtensionApiFrameId, and making copies. The struct consists of two 32-bit
integers, so it isn't that expensive.

WDYT?

https://codereview.chromium.org/1413543005/diff/300001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:117: callbacks_map.erase(key);
On 2015/12/12 14:25:00, Devlin wrote:
> In conjunction with the comment on line 108 and the logic on line 128, doesn't
> this mean the callback that could potentially be added will never be called?

Both methods can only be called on the same thread, so I don't think that it's
possible to loose a callback.

(unless you tell me that there is an interrupt-like thing in Chromium that
suspends the execution at line 114, runs GetFrameIdOnIO, and then resumes the
execution at line 114.).

https://codereview.chromium.org/1413543005/diff/300001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:136: bool is_frame_id_found;
On 2015/12/12 14:25:00, Devlin wrote:
> nit: initialize.

Done.

https://codereview.chromium.org/1413543005/diff/300001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:163: const ExtensionApiFrameId&
ExtensionApiFrameIdMap::GetFrameId(
On 2015/12/12 14:25:00, Devlin wrote:
> Should this also use LookupFrameIdOnUI?

Yes. At first I split up the methods to save a map lookup (RFH->ID->RFH), but
the duplication of logic is not nice, so I'm now using LookupFrameOnUI.

https://codereview.chromium.org/1413543005/diff/300001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.h (right):

https://codereview.chromium.org/1413543005/diff/300001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:23: // - Multiple
RenderFrameHosts may be mapped to the same ID.
On 2015/12/12 14:25:00, Devlin wrote:
> it's probably superfluous with above, but let's be explicit and say "mapped to
> the same ID within different tabs".

This is not what I meant, so I'll clarify it even more. The extension frame ID
is derived from the FrameTreeNode ID , and this ID is constant for the lifetime
of a frame. But the RenderFrameHost for this frame can change over time, and
consequently result in a different RFH for the given FrameTreeNode ID.

[Devlin, 2015-12-14 21:31:55]

https://codereview.chromium.org/1413543005/diff/300001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1413543005/diff/300001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:98: // |g_frame_id_map_lock| is
not needed here because GotFrameId is only called
On 2015/12/14 20:55:42, robwu wrote:
> On 2015/12/12 14:25:00, Devlin wrote:
> > Wow, talk about relying on C++'s underlying guarantees.
> > 
> > I'm pretty sure a map *is* one of those objects that allows concurrent
access
> > and insertion, but this still seems super subtle to me.  Let's see what
> happens,
> > but I'm not going to be entirely surprised if we find some bizarre exception
> as
> > a result of this.
> > 
> > And, looking at this more, would it be safe to just use
> > PostTaskAndReplyWithResult() in GetFrameIdOnIO, so that we don't have to do
a
> > lookup at all?
> 
> It is indeed safe because the pointers are valid (due to their storage in the
> global map), and I tried this at first, but it didn't compile.
> Did you ever manage to have a function with return value "const T&" and bind
it
> using base::Bind?
> 
> An alternative is just changing the const ExtensionApiFrameId& signature to
> ExtensionApiFrameId, and making copies. The struct consists of two 32-bit
> integers, so it isn't that expensive.
> 
> WDYT?

Hmm, yeah, passing a const& to a thread hop seems like it could be bad.  Just
copy the object.  It's cleaner, and copying is almost certainly cheaper than the
duplicate lookup we need this way.

https://codereview.chromium.org/1413543005/diff/300001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:117: callbacks_map.erase(key);
On 2015/12/14 20:55:42, robwu wrote:
> On 2015/12/12 14:25:00, Devlin wrote:
> > In conjunction with the comment on line 108 and the logic on line 128,
doesn't
> > this mean the callback that could potentially be added will never be called?
> 
> Both methods can only be called on the same thread, so I don't think that it's
> possible to loose a callback.
> 
> (unless you tell me that there is an interrupt-like thing in Chromium that
> suspends the execution at line 114, runs GetFrameIdOnIO, and then resumes the
> execution at line 114.).

Yeah, you're right, this should be safe.

[nasko, 2015-12-17 01:19:49]

https://codereview.chromium.org/1413543005/diff/180001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js
(right):

https://codereview.chromium.org/1413543005/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webnavigation/crossProcessIframe/test_crossProcessIframe.js:203:
['pre-a.com-onBeforeNavigate', 'pre-a.com-onErrorOccurred'],
On 2015/12/10 00:00:50, robwu wrote:
> On 2015/12/09 23:06:09, nasko wrote:
> > Don't we want to ensure that pre-a.com-onErrorOccurred is followed by the
> > a.com-onBeforeNavigate? Same for http://pre-c.com.
> 
> That's what I want as well, but unfortunately the sequence is
> pre-a.com-onBeforeNavigate
> a.com-onBeforeNavigate
> pre-a.com-onErrorOccurred

I can see why that is, even though I'm not happy about it.
 
> I say "unfortunate", because this makes frame tracking much harder for
extension
> devs; they have to keep track of processIds as well to deal with OOPIF.

Is it worse than before? According to the crossProcessAbort test case, one
should already be able to handle two onBeforeNavigate calls for the same frame.
We will make it much better once we have PlzNavigate, but in the meantime we
might not be worse.

> (I can't remember whether I omitted a.com-onBeforeNavigate for this reason, or
> because of flakiness)

https://codereview.chromium.org/1413543005/diff/180001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1413543005/diff/180001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:35:
base::LazyInstance<FrameIdMap> g_frame_id_map = LAZY_INSTANCE_INITIALIZER;
On 2015/12/10 00:00:51, robwu wrote:
> On 2015/12/09 23:06:09, nasko wrote:
> > Why is this map needed and must grow unbound? Why not observe
> > WebContentsObserver::FrameDeleted and then you know that FTN is gone forever
> and
> > the ID will never be used again?
> 
> Because I think that there is no guarantee that all network requests are
> immediately dropped before FrameDeleted, or that there are no webNavigation
> events after FrameDeleted (e.g. is there a defined order in RenderFrameDeleted
> and FrameDeleted? The former triggers the webNavigation.onErrorOccurred
event).

There should be no events for webNavigation after a frame is gone.
RenderFrameDeleted should always come before FrameDeleted, so we are ok in that
case.

> Do you think that it's worthwhile to use a timer or some form of a LRU queue
to
> bound the map size?

Why not just update the map? If the frame is gone, the requests/responses will
not be processed by any renderer. In such case, do webRequest extensions need to
know about those, as they will likely have no effect to the user.

https://codereview.chromium.org/1413543005/diff/360001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1413543005/diff/360001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:74: FrameIdMap::const_iterator
frame_id_iter = frame_id_map_.find(key);
All access to frame_id_map_ must be locked if it is accessible on multiple
threads.

https://codereview.chromium.org/1413543005/diff/360001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:153: if (!rfh)
If the RFH is null, why not directly return
ExtensionApiFrameId::kInvalidFrameId? There is no other possible value for it,
is there?

https://codereview.chromium.org/1413543005/diff/360001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:173: return nullptr;
I'd move this before the frame_id == 0, so all nullptr returns are grouped
together.

[robwu, 2015-12-22 16:55:46]

I've implemented the last things on my wishlist for this CL:

- Callbacks for GetFrameIdOnIO are now run in order.
- Frame IDs are eventually removed from the map (with a small delay to make sure
that the onErrorOccurred event dispatched by
WebNavigationTabObserver::RenderFrameDeleted has a proper frameId).

Devlin, nasko, battre: PTAL

https://codereview.chromium.org/1413543005/diff/360001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1413543005/diff/360001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:74: FrameIdMap::const_iterator
frame_id_iter = frame_id_map_.find(key);
On 2015/12/17 01:19:48, nasko wrote:
> All access to frame_id_map_ must be locked if it is accessible on multiple
> threads.

The only method that writes to the map is on the UI thread, so it is safe to
access the map on the UI thread without lock. The reads on the IO thread are
protected by a lock.

https://codereview.chromium.org/1413543005/diff/360001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:153: if (!rfh)
On 2015/12/17 01:19:48, nasko wrote:
> If the RFH is null, why not directly return
> ExtensionApiFrameId::kInvalidFrameId? There is no other possible value for it,
> is there?

I used to need a const&, so I took the value from the map. Now I'm returning by
value so I can directly return ExtensionApiFrameId() (which is a pair of
kInvalidFrameIds).

https://codereview.chromium.org/1413543005/diff/360001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:173: return nullptr;
On 2015/12/17 01:19:49, nasko wrote:
> I'd move this before the frame_id == 0, so all nullptr returns are grouped
> together.

Done.

[Devlin, 2015-12-22 22:07:54]

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:20: // delete it.
I thought we had a way of ensuring which thread an object was deleted on...? 
Not that it really matters with this one; I don't think leaking will make
anything break.  Just seems weird.

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:98: const ExtensionApiFrameId&
extension_api_frame_id = KeyToValue(key);
not const&

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:121: while (!callbacks_.empty()
&&
If we don't need to run callbacks synchronously (see below), could we get rid of
|has_value| and just make this (lines 121 - 144):

for (auto iter = callbacks_.begin(); iter != callbacks_.end();) {
  if (callback_info.key == key) {
    callback_info.callback.Run(extension_api_frame_id);
    iter = callbacks_.erase(iter);
  } else {
    ++iter;
  }
}

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:166: // there are no pending
callbacks, otherwise queue it.
Is there a reason we need to run callbacks synchronously?  Why not be as
performant as possible and ensure that if we know the answer, we return it?

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:213: int frame_routing_id) {
DCHECK_CURRENTLY_ON

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:222: const int
kSecondsUntilRemoval = 1;
removal times like this always make me a bit nervous.  What was the reasoning
for 1s?  What happens if we just post a pending task for removal?

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.h (right):

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:74: const ExtensionApiFrameId
GetFrameId(int render_process_id,
We should document that these are UI-only.  Might even be worthwhile to rename
them, but I don't feel strongly.

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:162: };
DISALLOW_COPY_AND_ASSIGN()

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map_unittest.cc (right):

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map_unittest.cc:83:
scoped_ptr<TestExtensionApiFrameIdMap> map(new TestExtensionApiFrameIdMap());
Why not just stack-allocate?

[nasko, 2015-12-22 22:12:10]

https://codereview.chromium.org/1413543005/diff/380001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1413543005/diff/380001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:99: // Don't save invalid
values in the map.
nit: Comment above the variable or empty line between the two.

https://codereview.chromium.org/1413543005/diff/380001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:108: void
ExtensionApiFrameIdMap::GotFrameIdOnIO(
nit: GetFrameIdOnIO and GotFrameIdOnIO differ by only one letter that is close
enough that makes reading the code a bit hard. Why not name it something else,
for example "ReceivedFrameIdOnIO"?

https://codereview.chromium.org/1413543005/diff/380001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:112: //
BrowserThread::PostTaskAndReplyWithResult runs the tasks in order, so the
nit: Empty line before the comment.

https://codereview.chromium.org/1413543005/diff/380001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:138: // the callbacks queue.
Why do we need all of this complexity? Why can't we just run the callback if the
frame has already been added to the map?

https://codereview.chromium.org/1413543005/diff/380001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:221: // Minimum number of
seconds to wait before removing the frame ID.
I found this comment to be confusing. I was expecting to see a wait and there
isn't one. I'd rephrase it to say that it is the minimum amount of time that has
elapsed since the entry was added to the list.

https://codereview.chromium.org/1413543005/diff/380001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:222: const int
kSecondsUntilRemoval = 1;
This should be declared at the top of the file. Also, was there any rationale
behind picking the 1 second value? If yes, please add a comment to describe it,
otherwise it is pretty arbitrary and might as well be 5 seconds.

https://codereview.chromium.org/1413543005/diff/380001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:230: // before
|kSecondsUntilRemoval| seconds have passed. This is extremely
nit: I'd remove the "extremely" qualifier, as you'd be surprised what is likely
to happen on the web platform :).

https://codereview.chromium.org/1413543005/diff/380001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.h (right):

https://codereview.chromium.org/1413543005/diff/380001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:120: bool has_value;
Can you put some comments on these members so their purpose is a bit more clear?

https://codereview.chromium.org/1413543005/diff/380001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:151:
std::deque<FrameIdRemovalTask> pending_deletions_;
Why not a plain std::vector?

https://codereview.chromium.org/1413543005/diff/380001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map_unittest.cc (right):

https://codereview.chromium.org/1413543005/diff/380001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map_unittest.cc:83:
scoped_ptr<TestExtensionApiFrameIdMap> map(new TestExtensionApiFrameIdMap());
Why not move this to be a member of the test class? There are no test cases that
use more than one map.

https://codereview.chromium.org/1413543005/diff/380001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map_unittest.cc:104: EXPECT_EQ(1,
map->GetInternalSize());
If the map moves to be a class member, then you can have a wrapper to
RemoveFrameId which can trigger the cleanup and then the test case code will be
much easier to read:

EXPECT_EQ(2, map->GetInternalSize());
map->Remove(1, 2);
EXPECT_EQ(1, map->GetInternalSize());

https://codereview.chromium.org/1413543005/diff/380001/extensions/browser/ext...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1413543005/diff/380001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:113:
InitializeRenderFrame(render_frame_host);
Since we are trying to optimize thread hops, why not add the frame to the map
here? We know at this point in time all the values that we need. This can avoid
a thread hop even for the first usage of a specific frame.

[robwu, 2015-12-23 00:07:04]

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:20: // delete it.
On 2015/12/22 22:07:53, Devlin wrote:
> I thought we had a way of ensuring which thread an object was deleted on...?

Are you thinking of RefCountedThreadSafe?

> Not that it really matters with this one; I don't think leaking will make
> anything break.  Just seems weird.

I want to keep the class alive for as long as possible to avoid memory safety
issues at termination. I could make this map class refcounted, and use WeakPtr
to manage the callback's lifetime, but that seemed like too much overhead for no
runtime benefit. Hence I decided to use Leaky.

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:98: const ExtensionApiFrameId&
extension_api_frame_id = KeyToValue(key);
On 2015/12/22 22:07:53, Devlin (OOO through dec27) wrote:
> not const&

Done.

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:121: while (!callbacks_.empty()
&&
On 2015/12/22 22:07:53, Devlin wrote:
> If we don't need to run callbacks synchronously (see below), could we get rid
of
> |has_value| and just make this (lines 121 - 144):
> 
> for (auto iter = callbacks_.begin(); iter != callbacks_.end();) {
>   if (callback_info.key == key) {
>     callback_info.callback.Run(extension_api_frame_id);
>     iter = callbacks_.erase(iter);
>   } else {
>     ++iter;
>   }
> }

Queued callbacks are not immediately called when the value becomes available,
because it is possible that there is another call in between. See lines 131 -
139 of my new patch set for a clarifying comment.

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:166: // there are no pending
callbacks, otherwise queue it.
On 2015/12/22 22:07:53, Devlin wrote:
> Is there a reason we need to run callbacks synchronously?  Why not be as
> performant as possible and ensure that if we know the answer, we return it?

Is is usually not possible to synchronously map a key the desired answer,
because the answer can only be determined on the UI thread.

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:213: int frame_routing_id) {
On 2015/12/22 22:07:53, Devlin wrote:
> DCHECK_CURRENTLY_ON

Done.

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:222: const int
kSecondsUntilRemoval = 1;
On 2015/12/22 22:07:54, Devlin wrote:
> removal times like this always make me a bit nervous.  What was the reasoning
> for 1s?  What happens if we just post a pending task for removal?

1s is an arbitrarily chosen non-zero value.

Initially, I didn't use a time out at all. This caused e.g.
WebNavigationApiTest.CrossProcess to fail at crossProcessRedirect because the
chrome.webNavigation.onErrorOccurred event was triggered with frameId -1.
RemoveFrameId and this onErrorOccurred are both triggered by the
RenderFrameDeleted notification, so using PostTask can fix this particular
problem because RenderFrameId is then guaranteed to be executed after
webNavigation.onErrorOccurred.

But I also thought of the webRequest API. With a simple test extension, I found
that with --site-per-process, webRequest.onErrorOccurred is triggered after
webNavigation.onErrorOccurred. So PostTask would be insufficient, and that's why
I settled for a queue with timestamps.

If it turns out that one second is too low, we can safely increase it. All that
matters is that the map is eventually cleaned up while the user uses Chrome for
a month straight without restarting.

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.h (right):

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:74: const ExtensionApiFrameId
GetFrameId(int render_process_id,
On 2015/12/22 22:07:54, Devlin (OOO through dec27) wrote:
> We should document that these are UI-only.  Might even be worthwhile to rename
> them, but I don't feel strongly.

Above the class: "Unless stated otherwise, the methods can only be called on the
UI thread."

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:162: };
On 2015/12/22 22:07:54, Devlin (OOO through dec27) wrote:
> DISALLOW_COPY_AND_ASSIGN()

Done.

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map_unittest.cc (right):

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map_unittest.cc:83:
scoped_ptr<TestExtensionApiFrameIdMap> map(new TestExtensionApiFrameIdMap());
On 2015/12/22 22:07:54, Devlin (OOO through dec27) wrote:
> Why not just stack-allocate?

Done. (I initially wanted to also test destruction behavior, but that was
pointless).

[battre, 2015-12-23 14:02:12]

WebRequest API stuff LGTM, just a couple of nits...

Nasko's comment on ExtensionWebContentsObserver::RenderFrameCreated is
interesting, however. Would be great to skip the thread hopping completely as
that could simplify some of the logic.

https://codereview.chromium.org/1413543005/diff/420001/chrome/common/extensio...
File chrome/common/extensions/api/web_navigation.json (right):

https://codereview.chromium.org/1413543005/diff/420001/chrome/common/extensio...
chrome/common/extensions/api/web_navigation.json:37: "deprecated": "Frames are
now uniquely identified by their tab Id and frame ID; the process ID is no
longer needed and therefore ignored.",
nit: s/Id/ID/;

https://codereview.chromium.org/1413543005/diff/420001/chrome/common/extensio...
chrome/common/extensions/api/web_navigation.json:38: "description": "The ID of
the process runs the renderer for this tab."
*that* runs (or "of the process running the ...")

https://codereview.chromium.org/1413543005/diff/420001/chrome/common/extensio...
chrome/common/extensions/api/web_navigation.json:98: "description": "The ID of
the process runs the renderer for this frame."
*that* runs

https://codereview.chromium.org/1413543005/diff/420001/chrome/common/extensio...
chrome/common/extensions/api/web_navigation.json:140: "processId": {"type":
"integer", "description": "The ID of the process runs the renderer for this
frame."},
see above

https://codereview.chromium.org/1413543005/diff/420001/chrome/common/extensio...
chrome/common/extensions/api/web_navigation.json:167: "processId": {"type":
"integer", "description": "The ID of the process runs the renderer for this
frame."},
see above, also several occurrences below.

https://codereview.chromium.org/1413543005/diff/420001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/1413543005/diff/420001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:223: // Extract a pair of
IDs to identify the RenderFrameHost and store it in |dict|.
Comment needs to be updated because this function does not store anything in
dict.

https://codereview.chromium.org/1413543005/diff/420001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:383: const
ExtensionApiFrameId& extension_api_frame_id =
why a reference (const EAFI&)?

https://codereview.chromium.org/1413543005/diff/420001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.h (right):

https://codereview.chromium.org/1413543005/diff/420001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:72: // |frame_routing_id|. These
methods always returns the same value when called
nit: s/returns/return/;

https://codereview.chromium.org/1413543005/diff/420001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map_unittest.cc (right):

https://codereview.chromium.org/1413543005/diff/420001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map_unittest.cc:163: EXPECT_EQ(5U,
results().size());
nit: ASSERT_EQ

https://codereview.chromium.org/1413543005/diff/420001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map_unittest.cc:177: EXPECT_EQ(6U,
results().size());
ASSERT_EQ

https://codereview.chromium.org/1413543005/diff/420001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map_unittest.cc:186: EXPECT_EQ(7U,
results().size());
ASSERT_EQ

https://codereview.chromium.org/1413543005/diff/420001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map_unittest.cc:198: EXPECT_EQ(8U,
results().size());
ASSERT_EQ

[robwu, 2015-12-26 11:05:50]

https://codereview.chromium.org/1413543005/diff/380001/extensions/browser/ext...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1413543005/diff/380001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:113:
InitializeRenderFrame(render_frame_host);
On 2015/12/22 22:12:10, nasko wrote:
> Since we are trying to optimize thread hops, why not add the frame to the map
> here? We know at this point in time all the values that we need. This can
avoid
> a thread hop even for the first usage of a specific frame.

Done.

https://codereview.chromium.org/1413543005/diff/420001/chrome/common/extensio...
File chrome/common/extensions/api/web_navigation.json (right):

https://codereview.chromium.org/1413543005/diff/420001/chrome/common/extensio...
chrome/common/extensions/api/web_navigation.json:37: "deprecated": "Frames are
now uniquely identified by their tab Id and frame ID; the process ID is no
longer needed and therefore ignored.",
On 2015/12/23 14:02:12, battre wrote:
> nit: s/Id/ID/;

Done.

https://codereview.chromium.org/1413543005/diff/420001/chrome/common/extensio...
chrome/common/extensions/api/web_navigation.json:38: "description": "The ID of
the process runs the renderer for this tab."
On 2015/12/23 14:02:12, battre wrote:
> *that* runs (or "of the process running the ...")

Done (also for all 10 other occurrences).

https://codereview.chromium.org/1413543005/diff/420001/chrome/common/extensio...
chrome/common/extensions/api/web_navigation.json:273: "sourceProcessId":
{"type": "integer", "description": "The ID of the process runs the renderer for
the source tab."},
I also changed "source tab" to "source frame" because "source tab" doesn't make
sense with OOPIF.

https://codereview.chromium.org/1413543005/diff/420001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/1413543005/diff/420001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:223: // Extract a pair of
IDs to identify the RenderFrameHost and store it in |dict|.
On 2015/12/23 14:02:12, battre wrote:
> Comment needs to be updated because this function does not store anything in
> dict.

Done.

https://codereview.chromium.org/1413543005/diff/420001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:383: const
ExtensionApiFrameId& extension_api_frame_id =
On 2015/12/23 14:02:12, battre wrote:
> why a reference (const EAFI&)?

Previously GetFrameId returned EAFI&. I've now removed the &, let's hope that
RVO kicks in (and otherwise it's just copying a pair of integers).

https://codereview.chromium.org/1413543005/diff/420001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.h (right):

https://codereview.chromium.org/1413543005/diff/420001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:72: // |frame_routing_id|. These
methods always returns the same value when called
On 2015/12/23 14:02:12, battre wrote:
> nit: s/returns/return/;

Done.

[robwu, 2015-12-26 11:37:51]

Patchset #21 (id:440001) has been deleted

[robwu, 2015-12-26 11:39:47]

Patchset #21 (id:460001) has been deleted

[Devlin, 2015-12-28 17:34:25]

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:121: while (!callbacks_.empty()
&&
On 2015/12/23 00:07:03, robwu wrote:
> On 2015/12/22 22:07:53, Devlin wrote:
> > If we don't need to run callbacks synchronously (see below), could we get
rid
> of
> > |has_value| and just make this (lines 121 - 144):
> > 
> > for (auto iter = callbacks_.begin(); iter != callbacks_.end();) {
> >   if (callback_info.key == key) {
> >     callback_info.callback.Run(extension_api_frame_id);
> >     iter = callbacks_.erase(iter);
> >   } else {
> >     ++iter;
> >   }
> > }
> 
> Queued callbacks are not immediately called when the value becomes available,
> because it is possible that there is another call in between. See lines 131 -
> 139 of my new patch set for a clarifying comment.
> 

I'm still a little confused on this.  I understand *what* is happening (you are
ensuring that each callback is called in the order that it was added), but my
question is *why*.  What's wrong with running the callbacks "out of order" as
the frame ids are available?  What breaks?  If nothing, then I'd say just
document that callbacks may be called in a different order than added, and call
it good.  No reason to add unnecessary delays, since this can be
performance-sensitive code.

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:166: // there are no pending
callbacks, otherwise queue it.
On 2015/12/23 00:07:03, robwu wrote:
> On 2015/12/22 22:07:53, Devlin wrote:
> > Is there a reason we need to run callbacks synchronously?  Why not be as
> > performant as possible and ensure that if we know the answer, we return it?
> 
> Is is usually not possible to synchronously map a key the desired answer,
> because the answer can only be determined on the UI thread.

Sorry, perhaps my comment wasn't clear.  I was asking why, in this particular
case (where we already know the frame id), we don't immediately run the
callback.  Basically the same question as on line 121.

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:222: const int
kSecondsUntilRemoval = 1;
On 2015/12/23 00:07:03, robwu wrote:
> On 2015/12/22 22:07:54, Devlin wrote:
> > removal times like this always make me a bit nervous.  What was the
reasoning
> > for 1s?  What happens if we just post a pending task for removal?
> 
> 1s is an arbitrarily chosen non-zero value.
> 
> Initially, I didn't use a time out at all. This caused e.g.
> WebNavigationApiTest.CrossProcess to fail at crossProcessRedirect because the
> chrome.webNavigation.onErrorOccurred event was triggered with frameId -1.
> RemoveFrameId and this onErrorOccurred are both triggered by the
> RenderFrameDeleted notification, so using PostTask can fix this particular
> problem because RenderFrameId is then guaranteed to be executed after
> webNavigation.onErrorOccurred.
> 
> But I also thought of the webRequest API. With a simple test extension, I
found
> that with --site-per-process, webRequest.onErrorOccurred is triggered after
> webNavigation.onErrorOccurred. So PostTask would be insufficient, and that's
why
> I settled for a queue with timestamps.
> 
> If it turns out that one second is too low, we can safely increase it. All
that
> matters is that the map is eventually cleaned up while the user uses Chrome
for
> a month straight without restarting.

It's always best to try and avoid things like this, because 1 second isn't
really meaningful at all.  What takes 1 second on your machine might take 10,
30, or infinite on another.  Do you know what causes the webRequest
onErrorOccured to differ from the webNavigation version?  And/or is there
another notification that would serve as a better indication that it's safe to
remove the frame id?

If, at the end of the day, we need this, it's not the end of the world.  The
consequence of not removing the frame immediately isn't really that bad.  More
frustrating would be flakiness around a situation like calling an API event with
a garbage frame id.  But even that should be relatively minimal.  All that said,
I'd much rather have this work definitively, rather than hopefully. :)

[Devlin, 2015-12-28 17:39:39]

https://codereview.chromium.org/1413543005/diff/480001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1413543005/diff/480001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:148: void
ExtensionApiFrameIdMap::GetFrameIdOnIO(int render_process_id,
Another thought: with the combination of all the optimizations we're doing here
and the importance that this code be performant, I think it would be good to add
some UMA here for the result of the id fetch in terms of synchronous vs
asynchronous.

[robwu, 2015-12-29 16:52:00]

Description was changed from

==========
Use FrameTreeNode ID as frameId in extension APIs

Use FrameTreeNode IDs instead of RenderFrame routing IDs as "frame id"
in the extension APIs (webNavigation, webRequest, extension messaging)
because FTN IDs are globally unique, and RFH IDs are not.

This extends the public content API with:
- RenderFrameHost::FromFrameTreeNodeId
- RenderFrameHost::GetFrameTreeNodeId
- WebContents::FindByFrameTreeNodeId

The extension APIs are modified as follows:
- webRequest:
  * No visible changes for extension developers.
  * Hop from IO to UI and back to IO thread to determine the frameId.
- webNavigation:
  * processId is no longer required in chrome.webNavigation.getFrame,
    but marked as optional (deprecated) to make sure that the API is
    backwards-compatible.
  * frameId is constant across navigations.
- Extension messaging (chrome.runtime.connect and friends):
  * Small change for extension developers in the following scenario:
    1. Open port to tab.
    2. Accept the port by listening to onConnect *in a child frame*.
    3. Unload the document in the frame.
    Old behavior: onDisconnect was not triggered in the background until
                  the tab was reloaded or closed.
    New behavior: onDisconnect is called.
  * Extension messaging works correctly in out-of-process frames.
  * Move port lifetime management from renderer to browser.
  * Tie port lifetime to frames instead of processes.
  * Only notify frames in renderers if they have accepted the port.
  * Remove obsolete work-around for crbug.com/520303.
  * Unify open/close/postMessage logic in ChromeExtensionMessageFilter
    (crbug.com/394383#c7)
- The extension frameId logic is no longer scattered over several files,
  but resides in a single class (ExtensionApiFrameIdMap).

Depends on https://codereview.chromium.org/1413853005/

BUG=432875
TEST=browser_tests --gtest_filter=\
ExtensionWebRequestApiTest.*:\
WebNavigationApiTest.*:\
ExtensionApiTest.Messaging*:\
ExternallyConnectableMessagingTest.*:\
ExternallyConnectableMessagingWithTlsChannelIdTest.*
==========

to

==========
Use FrameTreeNode ID as frameId in extension APIs

Use FrameTreeNode IDs instead of RenderFrame routing IDs as "frame id"
in the extension APIs (webNavigation, webRequest, extension messaging)
because FTN IDs are globally unique, and RFH IDs are not.

This extends the public content API with:
- RenderFrameHost::FromFrameTreeNodeId
- RenderFrameHost::GetFrameTreeNodeId
- WebContents::FindByFrameTreeNodeId

The extension APIs are modified as follows:
- webRequest:
  * Frame IDs may be unavailable after frame removal (crbug.com/572930).
  * Blocking webRequest handlers may be slower than before due to an
    extra IO->UI->IO hop to determine the frameId (but this happens only
    once per frame load).
- webNavigation:
  * processId is no longer required in chrome.webNavigation.getFrame,
    but marked as optional (deprecated) to make sure that the API is
    backwards-compatible.
  * frameId is constant across navigations.
- Extension messaging (chrome.runtime.connect and friends):
  * Small change for extension developers in the following scenario:
    1. Open port to tab.
    2. Accept the port by listening to onConnect *in a child frame*.
    3. Unload the document in the frame.
    Old behavior: onDisconnect was not triggered in the background until
                  the tab was reloaded or closed.
    New behavior: onDisconnect is called.
  * Extension messaging works correctly in out-of-process frames.
  * Move port lifetime management from renderer to browser.
  * Tie port lifetime to frames instead of processes.
  * Only notify frames in renderers if they have accepted the port.
  * Remove obsolete work-around for crbug.com/520303.
  * Unify open/close/postMessage logic in ChromeExtensionMessageFilter
    (crbug.com/394383#c7)
- The extension frameId logic is no longer scattered over several files,
  but resides in a single class (ExtensionApiFrameIdMap).

Depends on https://codereview.chromium.org/1413853005/

BUG=432875
TEST=browser_tests --gtest_filter=\
ExtensionWebRequestApiTest.*:\
WebNavigationApiTest.*:\
ExtensionApiTest.Messaging*:\
ExternallyConnectableMessagingTest.*:\
ExternallyConnectableMessagingWithTlsChannelIdTest.*
==========

[robwu, 2015-12-29 17:05:46]

Hi Devlin,

I made some changes in response to your feedback.

With the last patch set (22), the frame ID mapping logic for any consumers of
the map on the UI thread is trivial and implemented in the static
ExtensionApiFrameIdMap::GetFrameId / GetParentFrameId methods.

For ID lookups on the IO thread, I'm maintaining a per-frame callback queue
instead of a single global queue. Callbacks are called as soon as the frame ID
is known (either synchronously in a call to GetFrameIdOnIO, or in the reply from
the UI thread in GotFrameIdOnIO).

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:121: while (!callbacks_.empty()
&&
On 2015/12/28 17:34:25, Devlin wrote:
> On 2015/12/23 00:07:03, robwu wrote:
> > On 2015/12/22 22:07:53, Devlin wrote:
> > > If we don't need to run callbacks synchronously (see below), could we get
> rid
> > of
> > > |has_value| and just make this (lines 121 - 144):
> > > 
> > > for (auto iter = callbacks_.begin(); iter != callbacks_.end();) {
> > >   if (callback_info.key == key) {
> > >     callback_info.callback.Run(extension_api_frame_id);
> > >     iter = callbacks_.erase(iter);
> > >   } else {
> > >     ++iter;
> > >   }
> > > }
> > 
> > Queued callbacks are not immediately called when the value becomes
available,
> > because it is possible that there is another call in between. See lines 131
-
> > 139 of my new patch set for a clarifying comment.
> > 
> 
> I'm still a little confused on this.  I understand *what* is happening (you
are
> ensuring that each callback is called in the order that it was added), but my
> question is *why*.  What's wrong with running the callbacks "out of order" as
> the frame ids are available?  What breaks?  If nothing, then I'd say just
> document that callbacks may be called in a different order than added, and
call
> it good.  No reason to add unnecessary delays, since this can be
> performance-sensitive code.

Done.

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:166: // there are no pending
callbacks, otherwise queue it.
On 2015/12/28 17:34:25, Devlin wrote:
> On 2015/12/23 00:07:03, robwu wrote:
> > On 2015/12/22 22:07:53, Devlin wrote:
> > > Is there a reason we need to run callbacks synchronously?  Why not be as
> > > performant as possible and ensure that if we know the answer, we return
it?
> > 
> > Is is usually not possible to synchronously map a key the desired answer,
> > because the answer can only be determined on the UI thread.
> 
> Sorry, perhaps my comment wasn't clear.  I was asking why, in this particular
> case (where we already know the frame id), we don't immediately run the
> callback.  Basically the same question as on line 121.

Unlike the other one, it is critical to queue the event here, because of the
following scenario:

1. webRequest event #1 -> hop to thread
2. webRequest event #2 -> meanwhile cache was filled.
3. webRequest event #1 postback

Without pushing the callback onto the queue, events #1 and #2 for the SAME
request could be reported out of order.

In my last patch, I dropped the guarantee of consistent order and used separate
lists again, so the delay is minimal.

https://codereview.chromium.org/1413543005/diff/400001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:222: const int
kSecondsUntilRemoval = 1;
On 2015/12/28 17:34:25, Devlin wrote:
> On 2015/12/23 00:07:03, robwu wrote:
> > On 2015/12/22 22:07:54, Devlin wrote:
> > > removal times like this always make me a bit nervous.  What was the
> reasoning
> > > for 1s?  What happens if we just post a pending task for removal?
> > 
> > 1s is an arbitrarily chosen non-zero value.
> > 
> > Initially, I didn't use a time out at all. This caused e.g.
> > WebNavigationApiTest.CrossProcess to fail at crossProcessRedirect because
the
> > chrome.webNavigation.onErrorOccurred event was triggered with frameId -1.
> > RemoveFrameId and this onErrorOccurred are both triggered by the
> > RenderFrameDeleted notification, so using PostTask can fix this particular
> > problem because RenderFrameId is then guaranteed to be executed after
> > webNavigation.onErrorOccurred.
> > 
> > But I also thought of the webRequest API. With a simple test extension, I
> found
> > that with --site-per-process, webRequest.onErrorOccurred is triggered after
> > webNavigation.onErrorOccurred. So PostTask would be insufficient, and that's
> why
> > I settled for a queue with timestamps.
> > 
> > If it turns out that one second is too low, we can safely increase it. All
> that
> > matters is that the map is eventually cleaned up while the user uses Chrome
> for
> > a month straight without restarting.
> 
> It's always best to try and avoid things like this, because 1 second isn't
> really meaningful at all.  What takes 1 second on your machine might take 10,
> 30, or infinite on another.  Do you know what causes the webRequest
> onErrorOccured to differ from the webNavigation version?  And/or is there
> another notification that would serve as a better indication that it's safe to
> remove the frame id?
> 
> If, at the end of the day, we need this, it's not the end of the world.  The
> consequence of not removing the frame immediately isn't really that bad.  More
> frustrating would be flakiness around a situation like calling an API event
with
> a garbage frame id.  But even that should be relatively minimal.  All that
said,
> I'd much rather have this work definitively, rather than hopefully. :)

I ditched the goal of having a map that can provide an accurate frame ID even
after frame removal, because it is not feasibly to maintain such a map without
retaining everything in memory. No amount of delay can result in 100%
correctness (e.g. even with a delay of 24 hours, the extension API could still
give an invalid frame ID if the device suspends for a day).

Because the map is automatically filled and cleaned via
ExtensionWebContentsObserver, I've made most of the frame ID logic on the UI
static: The IDs are directly inferred from the RenderFrameHost, and not looked
up and cached in the map.

This is valid because:
- In the webNavigation API, all API calls have a valid RenderFrameHost.
- In the extension messaging API, the frame ID must also be valid, otherwise the
port is useless.

In the webRequest API, this might be problematic when frames are removed
(invalid frameIds?), but there are no breaking tests when I ran them locally. To
avoid inflating this CL even more, I deferred that problem to the future
(https://crbug.com/572930).

https://codereview.chromium.org/1413543005/diff/480001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1413543005/diff/480001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:148: void
ExtensionApiFrameIdMap::GetFrameIdOnIO(int render_process_id,
On 2015/12/28 17:39:39, Devlin wrote:
> Another thought: with the combination of all the optimizations we're doing
here
> and the importance that this code be performant, I think it would be good to
add
> some UMA here for the result of the id fetch in terms of synchronous vs
> asynchronous.

The UMA will basically monitor the cost of hopping from IO to UI and back to IO.
There is already a pending experiment at https://crbug.com/524228.

[Devlin, 2016-01-04 17:50:23]

Assuming the patch resolution is trivial and all tests pass, I think this lgtm
(though I very eagerly await a content-layer FrameIOInfo-type object!).  Thanks
for hanging in there.

That said, it looked like you may have missed a few of battre's comments?

https://codereview.chromium.org/1413543005/diff/500001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.h (right):

https://codereview.chromium.org/1413543005/diff/500001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:166:
DISALLOW_COPY_AND_ASSIGN(ExtensionApiFrameIdMap);
#include "base/macros.h"

[robwu, 2016-01-04 18:18:58]

> That said, it looked like you may have missed a few of battre's comments?

Are you referring to battre's last review in web_navigation.json? I fixed all of
them and shortened it to "Done (also for all 10 other occurrences)."

https://codereview.chromium.org/1413543005/diff/500001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.h (right):

https://codereview.chromium.org/1413543005/diff/500001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:166:
DISALLOW_COPY_AND_ASSIGN(ExtensionApiFrameIdMap);
On 2016/01/04 17:50:23, Devlin wrote:
> #include "base/macros.h"

Done.

[Devlin, 2016-01-04 18:33:25]

https://codereview.chromium.org/1413543005/diff/420001/chrome/common/extensio...
File chrome/common/extensions/api/web_navigation.json (right):

https://codereview.chromium.org/1413543005/diff/420001/chrome/common/extensio...
chrome/common/extensions/api/web_navigation.json:38: "description": "The ID of
the process runs the renderer for this tab."
On 2015/12/26 11:05:50, robwu wrote:
> On 2015/12/23 14:02:12, battre wrote:
> > *that* runs (or "of the process running the ...")
> 
> Done (also for all 10 other occurrences).

I see you said done, but it looks like it wasn't?

https://codereview.chromium.org/1413543005/diff/520001/chrome/common/extensio...
File chrome/common/extensions/api/web_navigation.json (right):

https://codereview.chromium.org/1413543005/diff/520001/chrome/common/extensio...
chrome/common/extensions/api/web_navigation.json:98: "description": "The ID of
the process runs the renderer for this frame."
E.g., here - should still be "the process that runs" or similar.

[robwu, 2016-01-04 20:30:19]

nasko: Only your stamp and we're good to go.

https://codereview.chromium.org/1413543005/diff/520001/chrome/common/extensio...
File chrome/common/extensions/api/web_navigation.json (right):

https://codereview.chromium.org/1413543005/diff/520001/chrome/common/extensio...
chrome/common/extensions/api/web_navigation.json:98: "description": "The ID of
the process runs the renderer for this frame."
On 2016/01/04 18:33:25, Devlin wrote:
> E.g., here - should still be "the process that runs" or similar.

Done for real.

[nasko, 2016-01-05 00:39:58]

Few more comments, mostly style nits.

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:19: // delete it.
Should construction of the map also be protected by the lock as the constructor
is not atomic?

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:36: : render_process_id(-1),
frame_routing_id(-1) {}
MSG_ROUTING_NONE for the routing id. 0 or kInvalidUniqueID for the process.

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:94: if (frame_id == -1)
kInvalidFrameId?

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:129: void
ExtensionApiFrameIdMap::GotFrameIdOnIO(
I thought I put a comment earlier on (but can't find it), Get/Got are very
similarly named. Can we change the Got to something a bit more different? For
example "Received"?

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:164: // frame_routing_id == -1
-> unknown RenderFrameHost
Let's avoid using -1 if possible. MSG_ROUTING_NONE should be used in cases where
we don't know of a RenderFrameHost.

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.h (right):

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:55: // Get the extension API
frame ID for the parent of |rfh|.
nit: Empty line above the comment.

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:73: // Look up the frame ID and
store it in the map. This method should be called
nit: "Looks up", "stores it".

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:78: // Remove the frame ID
mapping for a given frame. This method can be called at
nit: "Removes the"

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:84: protected:
Why are all the members protected instead of private?

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:94: // The routing ID of the
RenderFrame.
nit: Empty line before comment.

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:109: // The extension API frame
ID of the parent of the frame.
nit: Empty line before comment.

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:145: // Implementation of
RemoveFrameId(RenderFrameHost), separated for testing.
nit: Empty line before comment.

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:151: // This frameId map is only
modified on the UI thread and used to minimize the
"frameId" is not used before and it doesn't match any casing in this file. How
about just saying "This map"?

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:155: // This lock protects
|frame_id_map| from being concurrently written on the UI
frame_id_map_

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map_unittest.cc (right):

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map_unittest.cc:44: // in unit tests
:(
You technically can. Using TestRenderFrameHost and the
RenderViewHostTestHarness. I would leave this for another patch though, this one
has already seen enough churn.

[robwu, 2016-01-05 10:54:43]

Description was changed from

==========
Use FrameTreeNode ID as frameId in extension APIs

Use FrameTreeNode IDs instead of RenderFrame routing IDs as "frame id"
in the extension APIs (webNavigation, webRequest, extension messaging)
because FTN IDs are globally unique, and RFH IDs are not.

This extends the public content API with:
- RenderFrameHost::FromFrameTreeNodeId
- RenderFrameHost::GetFrameTreeNodeId
- WebContents::FindByFrameTreeNodeId

The extension APIs are modified as follows:
- webRequest:
  * Frame IDs may be unavailable after frame removal (crbug.com/572930).
  * Blocking webRequest handlers may be slower than before due to an
    extra IO->UI->IO hop to determine the frameId (but this happens only
    once per frame load).
- webNavigation:
  * processId is no longer required in chrome.webNavigation.getFrame,
    but marked as optional (deprecated) to make sure that the API is
    backwards-compatible.
  * frameId is constant across navigations.
- Extension messaging (chrome.runtime.connect and friends):
  * Small change for extension developers in the following scenario:
    1. Open port to tab.
    2. Accept the port by listening to onConnect *in a child frame*.
    3. Unload the document in the frame.
    Old behavior: onDisconnect was not triggered in the background until
                  the tab was reloaded or closed.
    New behavior: onDisconnect is called.
  * Extension messaging works correctly in out-of-process frames.
  * Move port lifetime management from renderer to browser.
  * Tie port lifetime to frames instead of processes.
  * Only notify frames in renderers if they have accepted the port.
  * Remove obsolete work-around for crbug.com/520303.
  * Unify open/close/postMessage logic in ChromeExtensionMessageFilter
    (crbug.com/394383#c7)
- The extension frameId logic is no longer scattered over several files,
  but resides in a single class (ExtensionApiFrameIdMap).

Depends on https://codereview.chromium.org/1413853005/

BUG=432875
TEST=browser_tests --gtest_filter=\
ExtensionWebRequestApiTest.*:\
WebNavigationApiTest.*:\
ExtensionApiTest.Messaging*:\
ExternallyConnectableMessagingTest.*:\
ExternallyConnectableMessagingWithTlsChannelIdTest.*
==========

to

==========
Use FrameTreeNode ID as frameId in extension APIs

Use FrameTreeNode IDs instead of RenderFrame routing IDs as "frame id"
in the extension APIs (webNavigation, webRequest, extension messaging)
because FTN IDs are globally unique, and RFH IDs are not.

This extends the public content API with:
- RenderFrameHost::FromFrameTreeNodeId
- RenderFrameHost::GetFrameTreeNodeId
- WebContents::FindFrameByFrameTreeNodeId

The extension APIs are modified as follows:
- webRequest:
  * Frame IDs may be unavailable after frame removal (crbug.com/572930).
  * Blocking webRequest handlers may be slower than before due to an
    extra IO->UI->IO hop to determine the frameId (but this happens only
    once per frame load).
- webNavigation:
  * processId is no longer required in chrome.webNavigation.getFrame,
    but marked as optional (deprecated) to make sure that the API is
    backwards-compatible.
  * frameId is constant across navigations.
- Extension messaging (chrome.runtime.connect and friends):
  * Small change for extension developers in the following scenario:
    1. Open port to tab.
    2. Accept the port by listening to onConnect *in a child frame*.
    3. Unload the document in the frame.
    Old behavior: onDisconnect was not triggered in the background until
                  the tab was reloaded or closed.
    New behavior: onDisconnect is called.
  * Extension messaging works correctly in out-of-process frames.
  * Move port lifetime management from renderer to browser.
  * Tie port lifetime to frames instead of processes.
  * Only notify frames in renderers if they have accepted the port.
  * Remove obsolete work-around for crbug.com/520303.
  * Unify open/close/postMessage logic in ChromeExtensionMessageFilter
    (crbug.com/394383#c7)
- The extension frameId logic is no longer scattered over several files,
  but resides in a single class (ExtensionApiFrameIdMap).
- IDs are now guaranteed to be -1 or higher (before this, -2 was
  occasionally seen).

Depends on https://codereview.chromium.org/1413853005/

BUG=432875
TEST=browser_tests --gtest_filter=\
ExtensionWebRequestApiTest.*:\
WebNavigationApiTest.*:\
ExtensionApiTest.Messaging*:\
ExternallyConnectableMessagingTest.*:\
ExternallyConnectableMessagingWithTlsChannelIdTest.*
==========

[robwu, 2016-01-05 10:59:03]

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:19: // delete it.
On 2016/01/05 00:39:58, nasko wrote:
> Should construction of the map also be protected by the lock as the
constructor
> is not atomic?

LazyInstance is documented to be thread-safe:
https://chromium.googlesource.com/chromium/src/+/9b6f42934e5a1e65ebfc668d91a2...
"The Type constructor will only ever be called once, even if two threads are
racing to create the object.  Get() and Pointer() will always return the same,
completely initialized instance."

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:36: : render_process_id(-1),
frame_routing_id(-1) {}
On 2016/01/05 00:39:58, nasko wrote:
> MSG_ROUTING_NONE for the routing id. 0 or kInvalidUniqueID for the process.

Done. The IDs have always been -1, but changing it to MSG_ROUTING_NONE (-2)
should not make a difference.

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:94: if (frame_id == -1)
On 2016/01/05 00:39:58, nasko wrote:
> kInvalidFrameId?

Done.

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:129: void
ExtensionApiFrameIdMap::GotFrameIdOnIO(
On 2016/01/05 00:39:58, nasko wrote:
> I thought I put a comment earlier on (but can't find it), Get/Got are very
> similarly named. Can we change the Got to something a bit more different? For
> example "Received"?

Done.

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:164: // frame_routing_id == -1
-> unknown RenderFrameHost
On 2016/01/05 00:39:58, nasko wrote:
> Let's avoid using -1 if possible. MSG_ROUTING_NONE should be used in cases
where
> we don't know of a RenderFrameHost.

Updated comment. I cannot add DCHECK_EQ(frame_routing_id, MSG_ROUTING_NONE)
because there are callers that pass -1 for unknown frames, even in content/.
E.g.
https://chromium.googlesource.com/chromium/src/+/8d34e46c9a294b4d9a5f74e97d9e...

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.h (right):

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:55: // Get the extension API
frame ID for the parent of |rfh|.
On 2016/01/05 00:39:58, nasko wrote:
> nit: Empty line above the comment.

Done.

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:73: // Look up the frame ID and
store it in the map. This method should be called
On 2016/01/05 00:39:58, nasko wrote:
> nit: "Looks up", "stores it".

Done.

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:78: // Remove the frame ID
mapping for a given frame. This method can be called at
On 2016/01/05 00:39:58, nasko wrote:
> nit: "Removes the"

Done.

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:84: protected:
On 2016/01/05 00:39:58, nasko wrote:
> Why are all the members protected instead of private?

For unit tests, so that the internal state of the map can be inspected.

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:94: // The routing ID of the
RenderFrame.
On 2016/01/05 00:39:58, nasko wrote:
> nit: Empty line before comment.

Done.

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:109: // The extension API frame
ID of the parent of the frame.
On 2016/01/05 00:39:58, nasko wrote:
> nit: Empty line before comment.

Done.

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:145: // Implementation of
RemoveFrameId(RenderFrameHost), separated for testing.
On 2016/01/05 00:39:58, nasko wrote:
> nit: Empty line before comment.

Done.

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:151: // This frameId map is only
modified on the UI thread and used to minimize the
On 2016/01/05 00:39:58, nasko wrote:
> "frameId" is not used before and it doesn't match any casing in this file. How
> about just saying "This map"?

Done.

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.h:155: // This lock protects
|frame_id_map| from being concurrently written on the UI
On 2016/01/05 00:39:58, nasko wrote:
> frame_id_map_

Done.

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map_unittest.cc (right):

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map_unittest.cc:44: // in unit tests
:(
On 2016/01/05 00:39:58, nasko wrote:
> You technically can. Using TestRenderFrameHost and the
> RenderViewHostTestHarness. I would leave this for another patch though, this
one
> has already seen enough churn.

Those are not in the public part of content/, including them would violate the
-content/ rule of extensions/DEPS.

[nasko, 2016-01-06 17:19:38]

Thanks a ton for pushing through this CL. LGTM!

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map.cc:164: // frame_routing_id == -1
-> unknown RenderFrameHost
On 2016/01/05 10:59:03, robwu wrote:
> On 2016/01/05 00:39:58, nasko wrote:
> > Let's avoid using -1 if possible. MSG_ROUTING_NONE should be used in cases
> where
> > we don't know of a RenderFrameHost.
> 
> Updated comment. I cannot add DCHECK_EQ(frame_routing_id, MSG_ROUTING_NONE)
> because there are callers that pass -1 for unknown frames, even in content/.
> E.g.
>
https://chromium.googlesource.com/chromium/src/+/8d34e46c9a294b4d9a5f74e97d9e...

Thanks for the TODO.

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map_unittest.cc (right):

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map_unittest.cc:44: // in unit tests
:(
On 2016/01/05 10:59:03, robwu wrote:
> On 2016/01/05 00:39:58, nasko wrote:
> > You technically can. Using TestRenderFrameHost and the
> > RenderViewHostTestHarness. I would leave this for another patch though, this
> one
> > has already seen enough churn.
> 
> Those are not in the public part of content/, including them would violate the
> -content/ rule of extensions/DEPS.

The harness is public: content/public/test/test_renderer_host.h. It is used from
outside of content/.

[nasko, 2016-01-06 17:30:22]

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

[commit-bot: I haz the power, 2016-01-06 17:30:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1413543005/580001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1413543005/580001

[robwu, 2016-01-06 17:48:59]

Thanks all for the reviews.

If the CL does not get reverted due to an unexpected regression, I will follow
up with more CLs to fix existing bugs and clean ups that build upon this CL.

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
File extensions/browser/extension_api_frame_id_map_unittest.cc (right):

https://codereview.chromium.org/1413543005/diff/540001/extensions/browser/ext...
extensions/browser/extension_api_frame_id_map_unittest.cc:44: // in unit tests
:(
On 2016/01/06 17:19:38, nasko wrote:
> On 2016/01/05 10:59:03, robwu wrote:
> > On 2016/01/05 00:39:58, nasko wrote:
> > > You technically can. Using TestRenderFrameHost and the
> > > RenderViewHostTestHarness. I would leave this for another patch though,
this
> > one
> > > has already seen enough churn.
> > 
> > Those are not in the public part of content/, including them would violate
the
> > -content/ rule of extensions/DEPS.
> 
> The harness is public: content/public/test/test_renderer_host.h. It is used
from
> outside of content/.

Thanks for this pointer, that could work indeed. I've added a TODO instead of
this comment.

[robwu, 2016-01-06 21:13:56]

The CQ bit was checked by rob@robwu.nl

[robwu, 2016-01-06 21:13:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpcomplete@chromium.org, creis@chromium.org, battre@chromium.org,
rdevlin.cronin@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1413543005/#ps620001
(title: "s/:/ /")

[commit-bot: I haz the power, 2016-01-06 21:14:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1413543005/620001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1413543005/620001

[commit-bot: I haz the power, 2016-01-06 21:22:17]

Description was changed from

==========
Use FrameTreeNode ID as frameId in extension APIs

Use FrameTreeNode IDs instead of RenderFrame routing IDs as "frame id"
in the extension APIs (webNavigation, webRequest, extension messaging)
because FTN IDs are globally unique, and RFH IDs are not.

This extends the public content API with:
- RenderFrameHost::FromFrameTreeNodeId
- RenderFrameHost::GetFrameTreeNodeId
- WebContents::FindFrameByFrameTreeNodeId

The extension APIs are modified as follows:
- webRequest:
  * Frame IDs may be unavailable after frame removal (crbug.com/572930).
  * Blocking webRequest handlers may be slower than before due to an
    extra IO->UI->IO hop to determine the frameId (but this happens only
    once per frame load).
- webNavigation:
  * processId is no longer required in chrome.webNavigation.getFrame,
    but marked as optional (deprecated) to make sure that the API is
    backwards-compatible.
  * frameId is constant across navigations.
- Extension messaging (chrome.runtime.connect and friends):
  * Small change for extension developers in the following scenario:
    1. Open port to tab.
    2. Accept the port by listening to onConnect *in a child frame*.
    3. Unload the document in the frame.
    Old behavior: onDisconnect was not triggered in the background until
                  the tab was reloaded or closed.
    New behavior: onDisconnect is called.
  * Extension messaging works correctly in out-of-process frames.
  * Move port lifetime management from renderer to browser.
  * Tie port lifetime to frames instead of processes.
  * Only notify frames in renderers if they have accepted the port.
  * Remove obsolete work-around for crbug.com/520303.
  * Unify open/close/postMessage logic in ChromeExtensionMessageFilter
    (crbug.com/394383#c7)
- The extension frameId logic is no longer scattered over several files,
  but resides in a single class (ExtensionApiFrameIdMap).
- IDs are now guaranteed to be -1 or higher (before this, -2 was
  occasionally seen).

Depends on https://codereview.chromium.org/1413853005/

BUG=432875
TEST=browser_tests --gtest_filter=\
ExtensionWebRequestApiTest.*:\
WebNavigationApiTest.*:\
ExtensionApiTest.Messaging*:\
ExternallyConnectableMessagingTest.*:\
ExternallyConnectableMessagingWithTlsChannelIdTest.*
==========

to

==========
Use FrameTreeNode ID as frameId in extension APIs

Use FrameTreeNode IDs instead of RenderFrame routing IDs as "frame id"
in the extension APIs (webNavigation, webRequest, extension messaging)
because FTN IDs are globally unique, and RFH IDs are not.

This extends the public content API with:
- RenderFrameHost::FromFrameTreeNodeId
- RenderFrameHost::GetFrameTreeNodeId
- WebContents::FindFrameByFrameTreeNodeId

The extension APIs are modified as follows:
- webRequest:
  * Frame IDs may be unavailable after frame removal (crbug.com/572930).
  * Blocking webRequest handlers may be slower than before due to an
    extra IO->UI->IO hop to determine the frameId (but this happens only
    once per frame load).
- webNavigation:
  * processId is no longer required in chrome.webNavigation.getFrame,
    but marked as optional (deprecated) to make sure that the API is
    backwards-compatible.
  * frameId is constant across navigations.
- Extension messaging (chrome.runtime.connect and friends):
  * Small change for extension developers in the following scenario:
    1. Open port to tab.
    2. Accept the port by listening to onConnect *in a child frame*.
    3. Unload the document in the frame.
    Old behavior: onDisconnect was not triggered in the background until
                  the tab was reloaded or closed.
    New behavior: onDisconnect is called.
  * Extension messaging works correctly in out-of-process frames.
  * Move port lifetime management from renderer to browser.
  * Tie port lifetime to frames instead of processes.
  * Only notify frames in renderers if they have accepted the port.
  * Remove obsolete work-around for crbug.com/520303.
  * Unify open/close/postMessage logic in ChromeExtensionMessageFilter
    (crbug.com/394383#c7)
- The extension frameId logic is no longer scattered over several files,
  but resides in a single class (ExtensionApiFrameIdMap).
- IDs are now guaranteed to be -1 or higher (before this, -2 was
  occasionally seen).

Depends on https://codereview.chromium.org/1413853005/

BUG=432875
TEST=browser_tests --gtest_filter=\
ExtensionWebRequestApiTest.*:\
WebNavigationApiTest.*:\
ExtensionApiTest.Messaging*:\
ExternallyConnectableMessagingTest.*:\
ExternallyConnectableMessagingWithTlsChannelIdTest.*
==========

[commit-bot: I haz the power, 2016-01-06 21:22:19]

Committed patchset #28 (id:620001)

[commit-bot: I haz the power, 2016-01-06 21:23:09]

Description was changed from

==========
Use FrameTreeNode ID as frameId in extension APIs

Use FrameTreeNode IDs instead of RenderFrame routing IDs as "frame id"
in the extension APIs (webNavigation, webRequest, extension messaging)
because FTN IDs are globally unique, and RFH IDs are not.

This extends the public content API with:
- RenderFrameHost::FromFrameTreeNodeId
- RenderFrameHost::GetFrameTreeNodeId
- WebContents::FindFrameByFrameTreeNodeId

The extension APIs are modified as follows:
- webRequest:
  * Frame IDs may be unavailable after frame removal (crbug.com/572930).
  * Blocking webRequest handlers may be slower than before due to an
    extra IO->UI->IO hop to determine the frameId (but this happens only
    once per frame load).
- webNavigation:
  * processId is no longer required in chrome.webNavigation.getFrame,
    but marked as optional (deprecated) to make sure that the API is
    backwards-compatible.
  * frameId is constant across navigations.
- Extension messaging (chrome.runtime.connect and friends):
  * Small change for extension developers in the following scenario:
    1. Open port to tab.
    2. Accept the port by listening to onConnect *in a child frame*.
    3. Unload the document in the frame.
    Old behavior: onDisconnect was not triggered in the background until
                  the tab was reloaded or closed.
    New behavior: onDisconnect is called.
  * Extension messaging works correctly in out-of-process frames.
  * Move port lifetime management from renderer to browser.
  * Tie port lifetime to frames instead of processes.
  * Only notify frames in renderers if they have accepted the port.
  * Remove obsolete work-around for crbug.com/520303.
  * Unify open/close/postMessage logic in ChromeExtensionMessageFilter
    (crbug.com/394383#c7)
- The extension frameId logic is no longer scattered over several files,
  but resides in a single class (ExtensionApiFrameIdMap).
- IDs are now guaranteed to be -1 or higher (before this, -2 was
  occasionally seen).

Depends on https://codereview.chromium.org/1413853005/

BUG=432875
TEST=browser_tests --gtest_filter=\
ExtensionWebRequestApiTest.*:\
WebNavigationApiTest.*:\
ExtensionApiTest.Messaging*:\
ExternallyConnectableMessagingTest.*:\
ExternallyConnectableMessagingWithTlsChannelIdTest.*
==========

to

==========
Use FrameTreeNode ID as frameId in extension APIs

Use FrameTreeNode IDs instead of RenderFrame routing IDs as "frame id"
in the extension APIs (webNavigation, webRequest, extension messaging)
because FTN IDs are globally unique, and RFH IDs are not.

This extends the public content API with:
- RenderFrameHost::FromFrameTreeNodeId
- RenderFrameHost::GetFrameTreeNodeId
- WebContents::FindFrameByFrameTreeNodeId

The extension APIs are modified as follows:
- webRequest:
  * Frame IDs may be unavailable after frame removal (crbug.com/572930).
  * Blocking webRequest handlers may be slower than before due to an
    extra IO->UI->IO hop to determine the frameId (but this happens only
    once per frame load).
- webNavigation:
  * processId is no longer required in chrome.webNavigation.getFrame,
    but marked as optional (deprecated) to make sure that the API is
    backwards-compatible.
  * frameId is constant across navigations.
- Extension messaging (chrome.runtime.connect and friends):
  * Small change for extension developers in the following scenario:
    1. Open port to tab.
    2. Accept the port by listening to onConnect *in a child frame*.
    3. Unload the document in the frame.
    Old behavior: onDisconnect was not triggered in the background until
                  the tab was reloaded or closed.
    New behavior: onDisconnect is called.
  * Extension messaging works correctly in out-of-process frames.
  * Move port lifetime management from renderer to browser.
  * Tie port lifetime to frames instead of processes.
  * Only notify frames in renderers if they have accepted the port.
  * Remove obsolete work-around for crbug.com/520303.
  * Unify open/close/postMessage logic in ChromeExtensionMessageFilter
    (crbug.com/394383#c7)
- The extension frameId logic is no longer scattered over several files,
  but resides in a single class (ExtensionApiFrameIdMap).
- IDs are now guaranteed to be -1 or higher (before this, -2 was
  occasionally seen).

Depends on https://codereview.chromium.org/1413853005/

BUG=432875
TEST=browser_tests --gtest_filter=\
ExtensionWebRequestApiTest.*:\
WebNavigationApiTest.*:\
ExtensionApiTest.Messaging*:\
ExternallyConnectableMessagingTest.*:\
ExternallyConnectableMessagingWithTlsChannelIdTest.*

Committed: https://crrev.com/3e2a0730e0dfb9c4dcbce45eea275270db900e90
Cr-Commit-Position: refs/heads/master@{#367914}
==========

[commit-bot: I haz the power, 2016-01-06 21:23:10]

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

[avallee, 2016-04-11 18:13:17]

avallee@chromium.org changed reviewers:
+ avallee@chromium.org

[avallee, 2016-04-11 18:13:19]

https://codereview.chromium.org/1413543005/diff/620001/content/public/browser...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/1413543005/diff/620001/content/public/browser...
content/public/browser/render_frame_host.h:48: static RenderFrameHost*
FromFrameTreeNodeId(int frame_tree_node_id);
No implementation nor calls...

[Charlie Reis, 2016-04-11 20:41:27]

https://codereview.chromium.org/1413543005/diff/620001/content/public/browser...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/1413543005/diff/620001/content/public/browser...
content/public/browser/render_frame_host.h:48: static RenderFrameHost*
FromFrameTreeNodeId(int frame_tree_node_id);
On 2016/04/11 18:13:19, avallee wrote:
> No implementation nor calls...

Thanks for noticing, avallee@.  Can you send a CL to remove it?  I'm happy to
approve.