Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
952473002:
Add frameId to executeScript/insertCSS (Closed)
|
Created:
5 years, 10 months ago by robwu Modified:
5 years ago CC:
aboxhall+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, davemoore+watch_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, extensions-reviews_chromium.org, je_julie(Not used), nektar+watch_chromium.org, oshima+watch_chromium.org, plundblad+watch_chromium.org, stevenjb+watch_chromium.org, yuzo+watch_chromium.org, Charlie Reis, dmazzoni, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd frameId to executeScript/insertCSS
Depends on https://codereview.chromium.org/945743003/ for the updated error message in the unit tests.
BUG=63979
Patch Set 1 #Patch Set 2 : Add insertCSS tests #Patch Set 3 : Revert change to error message, moved to https://codereview.chromium.org/945743003/ #
Total comments: 35
Patch Set 4 : Fix a few nits (review up to comment 16) #Messages
Total messages: 24 (3 generated)
rob@robwu.nl changed reviewers: + dcheng@chromium.org, jam@chromium.org, kalman@chromium.org
kalman: PTAL at every file. jam: chrome/browser/chromeos/accessibility/accessibility_manager.cc dcheng: extensions/common/extension_messages.h
Is there a particular reason the about: URL error handling needs to be included in this patch? It's pretty minor, but in general, it'd be nice to have each patch be about fixing one issue.
On 2015/02/23 15:29:06, dcheng wrote: > Is there a particular reason the about: URL error handling needs to be included > in this patch? It's pretty minor, but in general, it'd be nice to have each > patch be about fixing one issue. The about: error is currently very rare, but when specific frames (including about:blank) can be targetted by frameId, a proper error message is much desired. The error message improvement is just a few lines, and also referenced in the added unit tests that test the behavior of executeScript/insertCSS with frameId.
On 2015/02/23 at 15:39:15, rob wrote: > On 2015/02/23 15:29:06, dcheng wrote: > > Is there a particular reason the about: URL error handling needs to be included > > in this patch? It's pretty minor, but in general, it'd be nice to have each > > patch be about fixing one issue. > > The about: error is currently very rare, but when specific frames (including about:blank) can be targetted by frameId, a proper error message is much desired. The error message improvement is just a few lines, and also referenced in the added unit tests that test the behavior of executeScript/insertCSS with frameId. I still think it's better to have it in a separate patch. It's easy enough to create a small patch for this and land it first.
+creis if he has any thoughts. https://codereview.chromium.org/952473002/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/952473002/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_api.cc:1762: int frame_id = details_->frame_id.get() ? *details_->frame_id : 0; details_->frame_id ? *details_->frame_id : 0 https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... extensions/renderer/script_injection_manager.cc:79: frame = frame->nextSibling()) { We definitely shouldn't be looping through all the WebFrames to try to find the one with the matching routing ID. We should probably fix ScriptInjectionManager so we can just route the IPC correctly.
Responding to a comment I noticed. Haven't reviewed the whole change. https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... extensions/renderer/script_injection_manager.cc:79: frame = frame->nextSibling()) { On 2015/02/23 16:16:01, dcheng wrote: > We definitely shouldn't be looping through all the WebFrames to try to find the > one with the matching routing ID. > > We should probably fix ScriptInjectionManager so we can just route the IPC > correctly. This is a project unto itself. I just filed crbug.com/460950 for this, you can reference it here - it'd be good not to entrench that bug any further than it is, if possible.
On 2015/02/23 15:09:55, robwu wrote: > kalman: PTAL at every file. > > jam: chrome/browser/chromeos/accessibility/accessibility_manager.cc lgtm > > dcheng: extensions/common/extension_messages.h
https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... extensions/renderer/script_injection_manager.cc:79: frame = frame->nextSibling()) { On 2015/02/23 16:50:17, kalman wrote: > On 2015/02/23 16:16:01, dcheng wrote: > > We definitely shouldn't be looping through all the WebFrames to try to find > the > > one with the matching routing ID. > > > > We should probably fix ScriptInjectionManager so we can just route the IPC > > correctly. > > This is a project unto itself. I just filed crbug.com/460950 for this, you can > reference it here - it'd be good not to entrench that bug any further than it > is, if possible. I realize this is going to be a non-trivial amount of work, but taking this route is just deferring even more work to later. Given that the out-of-process iframe work is pretty far along, I think it's a mistake to keep writing code like this.
https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... extensions/renderer/script_injection_manager.cc:79: frame = frame->nextSibling()) { On 2015/02/23 16:56:42, dcheng wrote: > On 2015/02/23 16:50:17, kalman wrote: > > On 2015/02/23 16:16:01, dcheng wrote: > > > We definitely shouldn't be looping through all the WebFrames to try to find > > the > > > one with the matching routing ID. > > > > > > We should probably fix ScriptInjectionManager so we can just route the IPC > > > correctly. > > > > This is a project unto itself. I just filed crbug.com/460950 for this, you can > > reference it here - it'd be good not to entrench that bug any further than it > > is, if possible. > > I realize this is going to be a non-trivial amount of work, but taking this > route is just deferring even more work to later. Given that the out-of-process > iframe work is pretty far along, I think it's a mistake to keep writing code > like this. Even after fixing the IPC, this feature will still not work with OOPI because of crbug.com/432875. Is the absence of a good integration with OOPI really a blocker for landing this CL? I'm not too happy about the need for traversing frames to identify the frameId, but in practice this is not much different from the present state. Currently, extension authors have to insert in every frame and then use JavaScript to filter the result. With frameId, behind the scenes all frames will still be traversed, but at least the code will be inserted in one frame only. The end result is that extensions could become a bit more efficient (or at the very least become less complex due to the ability to target specific frames).
On 2015/02/23 at 17:13:40, rob wrote: > https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... > File extensions/renderer/script_injection_manager.cc (right): > > https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... > extensions/renderer/script_injection_manager.cc:79: frame = frame->nextSibling()) { > On 2015/02/23 16:56:42, dcheng wrote: > > On 2015/02/23 16:50:17, kalman wrote: > > > On 2015/02/23 16:16:01, dcheng wrote: > > > > We definitely shouldn't be looping through all the WebFrames to try to find > > > the > > > > one with the matching routing ID. > > > > > > > > We should probably fix ScriptInjectionManager so we can just route the IPC > > > > correctly. > > > > > > This is a project unto itself. I just filed crbug.com/460950 for this, you can > > > reference it here - it'd be good not to entrench that bug any further than it > > > is, if possible. > > > > I realize this is going to be a non-trivial amount of work, but taking this > > route is just deferring even more work to later. Given that the out-of-process > > iframe work is pretty far along, I think it's a mistake to keep writing code > > like this. > > Even after fixing the IPC, this feature will still not work with OOPI because of crbug.com/432875. > > Is the absence of a good integration with OOPI really a blocker for landing this CL? Do you have a plan for how to move forward with OOPI support after this? Frame IDs are 32-bit and fully representable in JS, but FrameTreeNode IDs are 64-bit. > I'm not too happy about the need for traversing frames to identify the frameId, but in practice this is not much different from the present state. Currently, extension authors have to insert in every frame and then use JavaScript to filter the result. With frameId, behind the scenes all frames will still be traversed, but at least the code will be inserted in one frame only. The end result is that extensions could become a bit more efficient (or at the very least become less complex due to the ability to target specific frames). This bug was filed 4+ years ago. Given that, I think it's worth some time to find cleaner ways to do what this code needs to do.
https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... extensions/renderer/script_injection_manager.cc:79: frame = frame->nextSibling()) { On 2015/02/23 17:25:23, dcheng wrote: > On 2015/02/23 at 17:13:40, rob wrote: > > Even after fixing the IPC, this feature will still not work with OOPI because > of crbug.com/432875. > > > > Is the absence of a good integration with OOPI really a blocker for landing > this CL? > > Do you have a plan for how to move forward with OOPI support after this? Frame > IDs are 32-bit and fully representable in JS, but FrameTreeNode IDs are 64-bit. I'd take the 32 least significant bits. This truncation seems ok, the odds of having an ID collision is very low, especially within the same tab. Last time I checked, these IDs were not easily accessible via RenderFrame(Host)s. I'll take another look. > > I'm not too happy about the need for traversing frames to identify the > frameId, but in practice this is not much different from the present state. > Currently, extension authors have to insert in every frame and then use > JavaScript to filter the result. With frameId, behind the scenes all frames will > still be traversed, but at least the code will be inserted in one frame only. > The end result is that extensions could become a bit more efficient (or at the > very least become less complex due to the ability to target specific frames). > > This bug was filed 4+ years ago. Given that, I think it's worth some time to > find cleaner ways to do what this code needs to do. In my view, the fact that this feature request has been around for so long and the number of stars means that it should get a higher priority than new bugs. Most extensions do not need this feature, but those that do will benefit from this functionality, even if it is implemented in a way that is not perfect. content::RenderFrameImpl::FromID gives the desired answer in a cleaner way, but access to that method was disallowed by the PRESUBMIT. I reckon that publicising the functionality via content::RenderFrame is worse than traversing.
nasko@chromium.org changed reviewers: + nasko@chromium.org
Few comments. https://codereview.chromium.org/952473002/diff/40001/extensions/common/api/ex... File extensions/common/api/extension_types.json (right): https://codereview.chromium.org/952473002/diff/40001/extensions/common/api/ex... extensions/common/api/extension_types.json:46: "description": "The <a href='webNavigation#frame_ids'>frame</a> where the script or CSS should be injected." A frame id on its own is not unique and cannot fully qualify a frame. It should be a pair of process id and frame id. https://codereview.chromium.org/952473002/diff/40001/extensions/common/extens... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/952473002/diff/40001/extensions/common/extens... extensions/common/extension_messages.h:128: IPC_STRUCT_MEMBER(int, frame_id) Since this is routing id, let's name it clearly: s/frame_id/frame_routing_id/. Why use 0 for the root frame, when it also has a routing id? https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... extensions/renderer/script_injection_manager.cc:74: blink::WebLocalFrame* GetFrameByFrameId(blink::WebFrame* parent_frame, Since this is a new function trying to work on the frame tree and using routing ids, it makes more sense to use RenderFrame instead of WebFrame. Also, have you considered exposing RenderFrameImpl::FromRoutingID on the public interface and using that instead of traversing? It seems a bit more clean to me. https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... extensions/renderer/script_injection_manager.cc:446: render_view->GetWebView()->mainFrame()->toWebLocalFrame(); This will definitely fail with --site-per-process. Can we write the code a bit more OOPIF friendly? Why operate on WebLocalFrame instead of RenderFrame here?
https://codereview.chromium.org/952473002/diff/40001/extensions/common/api/ex... File extensions/common/api/extension_types.json (right): https://codereview.chromium.org/952473002/diff/40001/extensions/common/api/ex... extensions/common/api/extension_types.json:46: "description": "The <a href='webNavigation#frame_ids'>frame</a> where the script or CSS should be injected." On 2015/02/23 18:39:02, nasko wrote: > A frame id on its own is not unique and cannot fully qualify a frame. It should > be a pair of process id and frame id. Within a tab, frameIds are supposed to be unique. The fact that this is not always the case (e.g. with OOPIF) is a bug: https://crbug.com/432875. https://codereview.chromium.org/952473002/diff/40001/extensions/common/extens... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/952473002/diff/40001/extensions/common/extens... extensions/common/extension_messages.h:128: IPC_STRUCT_MEMBER(int, frame_id) On 2015/02/23 18:39:02, nasko wrote: > Since this is routing id, let's name it clearly: s/frame_id/frame_routing_id/. > > Why use 0 for the root frame, when it also has a routing id? This is currently a routing ID, but in the future it could be a FrameTreeNode ID (or whatever that is uniquely identifying a frame). It must be zero to be compatible with other extension APIs (webNavigation, webRequest, messaging). https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... extensions/renderer/script_injection_manager.cc:74: blink::WebLocalFrame* GetFrameByFrameId(blink::WebFrame* parent_frame, On 2015/02/23 18:39:02, nasko wrote: > Since this is a new function trying to work on the frame tree and using routing > ids, it makes more sense to use RenderFrame instead of WebFrame. RenderFrames definitely make more sense, but currently IPC is routed via RenderView, not RenderFrame. I plan on fixing the IPC in a separate patch, which will make this function redundant. > Also, have you considered exposing RenderFrameImpl::FromRoutingID on the public > interface and using that instead of traversing? It seems a bit more clean to me. Importing from non-public content is forbidden by the PRESUBMIT. https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... extensions/renderer/script_injection_manager.cc:446: render_view->GetWebView()->mainFrame()->toWebLocalFrame(); On 2015/02/23 18:39:02, nasko wrote: > This will definitely fail with --site-per-process. Can we write the code a bit > more OOPIF friendly? This whole class does not work with OOPIF at the moment. Getting it to be OOPIF friendly requires the IPC to be rewritten (https://crbug.com/460950). > Why operate on WebLocalFrame instead of RenderFrame here? Because everything else in this class uses WebFrame*. I guess that the cast to a WebLocalFrame is here because the existing logic doesn't work with cross-process frames.
https://codereview.chromium.org/952473002/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right): https://codereview.chromium.org/952473002/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/accessibility_manager.cc:143: params.frame_id = 0; Hm, would be good to use ScriptExecutor here. Something for dmazzoni@ to look into at some point perhaps. https://codereview.chromium.org/952473002/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/952473002/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_api.cc:1760: content::RenderProcessHost* process = contents->GetRenderProcessHost(); @anybody on the site isolation team: How can |process| possibly be null? Surely every WebContents has a process? https://codereview.chromium.org/952473002/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_api.cc:1762: int frame_id = details_->frame_id.get() ? *details_->frame_id : 0; On 2015/02/23 16:16:01, dcheng wrote: > details_->frame_id ? *details_->frame_id : 0 I actually find the ternary-if here looks a bit weird anyway. Can you make the condition below (process && details_->frame_id && *details_->frame_id > 0) https://codereview.chromium.org/952473002/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_api.cc:1774: GURL document_url = rfh ? rfh->GetLastCommittedURL() : contents->GetURL(); All of these GetURL() calls should be GetLastCommittedURL(). https://codereview.chromium.org/952473002/diff/40001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/executescript/frame_id/frames.html (right): https://codereview.chromium.org/952473002/diff/40001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/executescript/frame_id/frames.html:9: document.write('<iframe src="http://c.com:' + location.port + '/empty.html"></iframe>'); try to keep to 80 characters https://codereview.chromium.org/952473002/diff/40001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/executescript/frame_id/test.js (right): https://codereview.chromium.org/952473002/diff/40001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/executescript/frame_id/test.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2015? Or is this test copied and modified from somewhere? https://codereview.chromium.org/952473002/diff/40001/extensions/browser/scrip... File extensions/browser/script_executor.h (right): https://codereview.chromium.org/952473002/diff/40001/extensions/browser/scrip... extensions/browser/script_executor.h:48: ONE_FRAME, Why change the order? also I believe "single" is the dominant term for this idiom. https://codereview.chromium.org/952473002/diff/40001/extensions/common/api/ex... File extensions/common/api/extension_types.json (right): https://codereview.chromium.org/952473002/diff/40001/extensions/common/api/ex... extensions/common/api/extension_types.json:46: "description": "The <a href='webNavigation#frame_ids'>frame</a> where the script or CSS should be injected." On 2015/02/23 18:59:52, robwu wrote: > On 2015/02/23 18:39:02, nasko wrote: > > A frame id on its own is not unique and cannot fully qualify a frame. It > should > > be a pair of process id and frame id. > > Within a tab, frameIds are supposed to be unique. > The fact that this is not always the case (e.g. with OOPIF) is a bug: > https://crbug.com/432875. Worst case we can have our own (process id, frame id) --> (extension frame id) mapping for all classes, but it would be nice not to. Requiring both process and frame IDs for all these APIs would be a PITA, like Rob says. In other circumstances I'd include "frame IDs are unique within tabs" in the description, but it sounds like that's not the case. https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... extensions/renderer/script_injection_manager.cc:76: DCHECK(parent_frame); This seems more like a CHECK situation to me. https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... extensions/renderer/script_injection_manager.cc:78: for (blink::WebFrame* frame = parent_frame->firstChild(); !result && frame; Pulling this comment out of the thread below: we already have incorrect traversal logic somewhere. Is it possible to pull the two of these out into a common utility, to contain the damage that introducing more assumption would cause? I'm thinking, to minimise structural changes, a FrameWalker class which you'd use like: FrameWalker frame_walker(parent_frame); while (frame_walker.HasNext()) { content::RenderFrame* rf = content::RenderFrame::FromWebFrame( frame_walker.Current()); if (rf && rf->GetRoutingID() == frame_id) return rf.Current(); } return nullptr; Then in the other place where we do this (I can't remember, sorry) use FrameWalker. FrameWalker can then implement the logic for skipping over remote frames.
https://codereview.chromium.org/952473002/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/952473002/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_api.cc:1762: int frame_id = details_->frame_id.get() ? *details_->frame_id : 0; On 2015/02/23 20:41:23, kalman wrote: > On 2015/02/23 16:16:01, dcheng wrote: > > details_->frame_id ? *details_->frame_id : 0 Done. > > I actually find the ternary-if here looks a bit weird anyway. Can you make the > condition below (process && details_->frame_id && *details_->frame_id > 0) frameId is used three times in the next few lines, so removing the variable will reduce the readability of the code. https://codereview.chromium.org/952473002/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/tabs/tabs_api.cc:1774: GURL document_url = rfh ? rfh->GetLastCommittedURL() : contents->GetURL(); On 2015/02/23 20:41:23, kalman wrote: > All of these GetURL() calls should be GetLastCommittedURL(). Done (also in a few other places within this file). This change will also fix https://crbug.com/396830#c21 https://codereview.chromium.org/952473002/diff/40001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/executescript/frame_id/frames.html (right): https://codereview.chromium.org/952473002/diff/40001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/executescript/frame_id/frames.html:9: document.write('<iframe src="http://c.com:' + location.port + '/empty.html"></iframe>'); On 2015/02/23 20:41:23, kalman wrote: > try to keep to 80 characters Done. https://codereview.chromium.org/952473002/diff/40001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/executescript/frame_id/test.js (right): https://codereview.chromium.org/952473002/diff/40001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/executescript/frame_id/test.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2015/02/23 20:41:24, kalman wrote: > 2015? Or is this test copied and modified from somewhere? Done. I copied the header and forgot to change the date. https://codereview.chromium.org/952473002/diff/40001/extensions/browser/scrip... File extensions/browser/script_executor.h (right): https://codereview.chromium.org/952473002/diff/40001/extensions/browser/scrip... extensions/browser/script_executor.h:48: ONE_FRAME, On 2015/02/23 20:41:24, kalman wrote: > Why change the order? also I believe "single" is the dominant term for this > idiom. Done (rename & re-order) (I re-ordered because of alphabetical order; and I chose "ONE_FRAME" because it had the same length as "ALL_FRAME" ). https://codereview.chromium.org/952473002/diff/40001/extensions/common/api/ex... File extensions/common/api/extension_types.json (right): https://codereview.chromium.org/952473002/diff/40001/extensions/common/api/ex... extensions/common/api/extension_types.json:46: "description": "The <a href='webNavigation#frame_ids'>frame</a> where the script or CSS should be injected." On 2015/02/23 20:41:24, kalman wrote: > On 2015/02/23 18:59:52, robwu wrote: > > On 2015/02/23 18:39:02, nasko wrote: > > > A frame id on its own is not unique and cannot fully qualify a frame. It > > should > > > be a pair of process id and frame id. > > > > Within a tab, frameIds are supposed to be unique. > > The fact that this is not always the case (e.g. with OOPIF) is a bug: > > https://crbug.com/432875. > > Worst case we can have our own (process id, frame id) --> (extension frame id) > mapping for all classes, but it would be nice not to. Requiring both process and > frame IDs for all these APIs would be a PITA, like Rob says. > > In other circumstances I'd include "frame IDs are unique within tabs" in the > description, but it sounds like that's not the case. If I had to create a central place to do this mapping, what would be the best location for that? It must certainly be available in the browser, but having the logic in the renderer would also be nice. https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... extensions/renderer/script_injection_manager.cc:76: DCHECK(parent_frame); On 2015/02/23 20:41:24, kalman wrote: > This seems more like a CHECK situation to me. I've only put the DCHECK over there to document that the precondition is that the pointer exists. In the way that the logic is used now, this DCHECK can never get triggered (main_frame && GetFrameById(main_frame)). https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... extensions/renderer/script_injection_manager.cc:78: for (blink::WebFrame* frame = parent_frame->firstChild(); !result && frame; On 2015/02/23 20:41:24, kalman wrote: > Pulling this comment out of the thread below: we already have incorrect > traversal logic somewhere. Is it possible to pull the two of these out into a > common utility, to contain the damage that introducing more assumption would > cause? > > I'm thinking, to minimise structural changes, a FrameWalker class which you'd > use like: > > FrameWalker frame_walker(parent_frame); > while (frame_walker.HasNext()) { > content::RenderFrame* rf = content::RenderFrame::FromWebFrame( > frame_walker.Current()); > if (rf && rf->GetRoutingID() == frame_id) > return rf.Current(); > } > return nullptr; > > Then in the other place where we do this (I can't remember, sorry) use FrameWalker. This one: https://cs.chromium.org/AppendAllChildFrames? > FrameWalker can then implement the logic for skipping over remote frames. "My" usage here allows stopping whenever the target frame has been found, regardless of whether the frame is remote (on average, visiting 50% of the frames gives the right (remote/local) frame back). The other one has to walk over every frame and skip remote ones. Considering that these two use cases are not identical, and "my" usage is only temporary (until RenderView -> RenderFrame has been completed), do you still want to merge them?
https://codereview.chromium.org/952473002/diff/40001/extensions/common/api/ex... File extensions/common/api/extension_types.json (right): https://codereview.chromium.org/952473002/diff/40001/extensions/common/api/ex... extensions/common/api/extension_types.json:46: "description": "The <a href='webNavigation#frame_ids'>frame</a> where the script or CSS should be injected." On 2015/02/23 21:54:06, robwu wrote: > On 2015/02/23 20:41:24, kalman wrote: > > On 2015/02/23 18:59:52, robwu wrote: > > > On 2015/02/23 18:39:02, nasko wrote: > > > > A frame id on its own is not unique and cannot fully qualify a frame. It > > > should > > > > be a pair of process id and frame id. > > > > > > Within a tab, frameIds are supposed to be unique. > > > The fact that this is not always the case (e.g. with OOPIF) is a bug: > > > https://crbug.com/432875. > > > > Worst case we can have our own (process id, frame id) --> (extension frame id) > > mapping for all classes, but it would be nice not to. Requiring both process > and > > frame IDs for all these APIs would be a PITA, like Rob says. > > > > In other circumstances I'd include "frame IDs are unique within tabs" in the > > description, but it sounds like that's not the case. > > If I had to create a central place to do this mapping, what would be the best > location for that? It must certainly be available in the browser, but having the > logic in the renderer would also be nice. I was thinking of making a BrowserContextKeyedService or whatever. Pretty simple. I can't think of a reason why the renderer would need to do much else than believe what it was told from an API perspective (internally it could still use the frame ID), but if you did, you'd need to IPC it I suppose. https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/952473002/diff/40001/extensions/renderer/scri... extensions/renderer/script_injection_manager.cc:78: for (blink::WebFrame* frame = parent_frame->firstChild(); !result && frame; On 2015/02/23 21:54:06, robwu wrote: > On 2015/02/23 20:41:24, kalman wrote: > > Pulling this comment out of the thread below: we already have incorrect > > traversal logic somewhere. Is it possible to pull the two of these out into a > > common utility, to contain the damage that introducing more assumption would > > cause? > > > > I'm thinking, to minimise structural changes, a FrameWalker class which you'd > > use like: > > > > FrameWalker frame_walker(parent_frame); > > while (frame_walker.HasNext()) { > > content::RenderFrame* rf = content::RenderFrame::FromWebFrame( > > frame_walker.Current()); > > if (rf && rf->GetRoutingID() == frame_id) > > return rf.Current(); > > } > > return nullptr; > > > > Then in the other place where we do this (I can't remember, sorry) use > FrameWalker. > > This one: https://cs.chromium.org/AppendAllChildFrames That's it. Its call site would be nicer with an iterator IMO. > > > FrameWalker can then implement the logic for skipping over remote frames. > > "My" usage here allows stopping whenever the target frame has been found, > regardless of whether the frame is remote (on average, visiting 50% of the > frames gives the right (remote/local) frame back). The other one has to walk > over every frame and skip remote ones. > > Considering that these two use cases are not identical, and "my" usage is only > temporary (until RenderView -> RenderFrame has been completed), do you still > want to merge them? They seem the same to me? On one hand, you iterate over all the frames. On the other, you stop somewhere along the way. I'm imagining that the code, eventually, will look like the browser sending the IPC to topmost RenderFrame in each tree, iterating over all local children, doing <whatever>, stopping at remote frames. - In the content script injection case, <whatever> is injecting a script. - In this case, <whatever> is finding the frame ID, and the browser should know exactly what RenderFrame to send it to.
creis@chromium.org changed reviewers: + creis@chromium.org
There's a lot being discussed here, so I'm trying to catch up. I haven't reviewed the code yet, but I wanted to weigh in on some points in the discussion. 1) Is OOPIF support a blocker? At this point, we cannot add new features and APIs without having a clear way for them to support OOPIF. I understand that there is more work required to get the existing code ready for OOPIF, and I would argue that work is higher priority than this feature. At a minimum, though, we should not proceed with this feature until we have a clear plan for how it will work when the OOPIF support does land. 2) How are frame IDs implemented? It is clear that neither RenderFrame routing IDs or FrameTreeNode IDs are workable solutions for the "frame IDs" that are exposed in extension APIs. RenderFrame routing IDs are not unique within a tab, since frames may exist in multiple processes. FrameTreeNode IDs are 64-bit and cannot safely be truncated to 32-bit values, plus they are non-zero for main frames. For legacy reasons, extension frame IDs must be unique within a tab and zero for the main frame. Thus, we should have extension frame IDs be a browser-allocated ID that is tab-specific. We should probably map either FrameTreeNode IDs or (process ID, routing ID) pairs to these new tab-specific frame IDs. 3) Accessing frames in the renderer process. Rather than traversing WebFrames, we should expose a public RenderFrame::FromID, similar to RenderView::FromID. Also, we can no longer assume that each frame in a renderer process is a RenderFrame + WebLocalFrame, since it might be a RenderFrameProxy + WebRemoteFrame. To restate: it is now the time to make extensions work with OOPIF, not the time to add more technical debt that will make the task harder. For this CL, there's probably a way to make progress on both fronts. Thanks for understanding.
On Mon, Feb 23, 2015 at 2:53 PM, <creis@chromium.org> wrote: > 2) How are frame IDs implemented? > It is clear that neither RenderFrame routing IDs or FrameTreeNode IDs are > workable solutions for the "frame IDs" that are exposed in extension APIs. > RenderFrame routing IDs are not unique within a tab, since frames may > exist in > multiple processes. FrameTreeNode IDs are 64-bit and cannot safely be > truncated > to 32-bit values, plus they are non-zero for main frames. For legacy > reasons, > extension frame IDs must be unique within a tab and zero for the main > frame. > Thus, we should have extension frame IDs be a browser-allocated ID that is > tab-specific. We should probably map either FrameTreeNode IDs or (process > ID, > routing ID) pairs to these new tab-specific frame IDs. > Random drive-by thought: looking at the extension API docs, frameId is new in Chrome 41, which isn't even stable yet. Are we really constrained by "legacy reasons" for an extension API that hasn't even technically shipped yet? It seems like we could un-ship that feature and change the frameId to be a hex string that encodes a 64-bit value, for example. I'm not suggesting that's better, just that I think we should do what actually makes sense and not worry as much about legacy support. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/02/24 16:56:28, dmazzoni wrote: > On Mon, Feb 23, 2015 at 2:53 PM, <mailto:creis@chromium.org> wrote: > > > 2) How are frame IDs implemented? > > It is clear that neither RenderFrame routing IDs or FrameTreeNode IDs are > > workable solutions for the "frame IDs" that are exposed in extension APIs. > > RenderFrame routing IDs are not unique within a tab, since frames may > > exist in > > multiple processes. FrameTreeNode IDs are 64-bit and cannot safely be > > truncated > > to 32-bit values, plus they are non-zero for main frames. For legacy > > reasons, > > extension frame IDs must be unique within a tab and zero for the main > > frame. > > Thus, we should have extension frame IDs be a browser-allocated ID that is > > tab-specific. We should probably map either FrameTreeNode IDs or (process > > ID, > > routing ID) pairs to these new tab-specific frame IDs. > > > > Random drive-by thought: looking at the extension API docs, frameId is new > in Chrome 41, which isn't even stable yet. Are we really constrained by > "legacy reasons" for an extension API that hasn't even technically shipped > yet? It seems like we could un-ship that feature and change the frameId to > be a hex string that encodes a 64-bit value, for example. I'm not > suggesting that's better, just that I think we should do what actually > makes sense and not worry as much about legacy support. frameId is not new, it has been in the ecosystem for a few years (in the webNavigation/webRequest extension APIs), and will be used by more APIs: https://crbug.com/432875#c5 I'll work towards a refactor in IPC to make content scripts more OOPIF friendly. Ben has suggested a possible approach for allocating frame IDs at https://codereview.chromium.org/952473002/patch/40001/50015; after fixing the IPC (in a separate CL) I will take a look at that.
https://codereview.chromium.org/952473002/diff/40001/extensions/common/api/ex... File extensions/common/api/extension_types.json (right): https://codereview.chromium.org/952473002/diff/40001/extensions/common/api/ex... extensions/common/api/extension_types.json:46: "description": "The <a href='webNavigation#frame_ids'>frame</a> where the script or CSS should be injected." On 2015/02/23 20:41:24, kalman wrote: > On 2015/02/23 18:59:52, robwu wrote: > > On 2015/02/23 18:39:02, nasko wrote: > > > A frame id on its own is not unique and cannot fully qualify a frame. It > > should > > > be a pair of process id and frame id. > > > > Within a tab, frameIds are supposed to be unique. > > The fact that this is not always the case (e.g. with OOPIF) is a bug: > > https://crbug.com/432875. > > Worst case we can have our own (process id, frame id) --> (extension frame id) > mapping for all classes, but it would be nice not to. Requiring both process and > frame IDs for all these APIs would be a PITA, like Rob says. > > In other circumstances I'd include "frame IDs are unique within tabs" in the > description, but it sounds like that's not the case. We already have an API (WebNavigation) that uses (processId, frameId). What's the plan for that then? Or is web navigation just going to be the odd one out?
On 2015/02/24 21:54:28, dcheng wrote: > https://codereview.chromium.org/952473002/diff/40001/extensions/common/api/ex... > File extensions/common/api/extension_types.json (right): > > https://codereview.chromium.org/952473002/diff/40001/extensions/common/api/ex... > extensions/common/api/extension_types.json:46: "description": "The <a > href='webNavigation#frame_ids'>frame</a> where the script or CSS should be > injected." > On 2015/02/23 20:41:24, kalman wrote: > > On 2015/02/23 18:59:52, robwu wrote: > > > On 2015/02/23 18:39:02, nasko wrote: > > > > A frame id on its own is not unique and cannot fully qualify a frame. It > > > should > > > > be a pair of process id and frame id. > > > > > > Within a tab, frameIds are supposed to be unique. > > > The fact that this is not always the case (e.g. with OOPIF) is a bug: > > > https://crbug.com/432875. > > > > Worst case we can have our own (process id, frame id) --> (extension frame id) > > mapping for all classes, but it would be nice not to. Requiring both process > and > > frame IDs for all these APIs would be a PITA, like Rob says. > > > > In other circumstances I'd include "frame IDs are unique within tabs" in the > > description, but it sounds like that's not the case. > > We already have an API (WebNavigation) that uses (processId, frameId). What's > the plan for that then? Or is web navigation just going to be the odd one out? processId will be ignored in the future, since tabId + frameId are unique. This will be done by marking processId as an optional parameter (removing it could break extensions that specify processId). (assuming that you're talking about this one: https://developer.chrome.com/extensions/webNavigation#method-getFrame) |
