Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1628423002:
Add frameId to chrome.tabs.executeScript/insertCSS (Closed)
|
Created:
4 years, 11 months ago by robwu Modified:
4 years, 10 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, creis+watch_chromium.org, oshima+watch_chromium.org, plundblad+watch_chromium.org, aboxhall+watch_chromium.org, nasko+codewatch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, je_julie, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, chromium-apps-reviews_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@permissiondata-remove-process_id Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd frameId to chrome.tabs.executeScript/insertCSS
- Re-implemented https://codereview.chromium.org/952473002/, with all
checks at the browser side instead of just the renderer.
- Use the last committed URL for checking permissions instead of the
visible URL. As a result, executeScript/insertCSS will now succeed in
frames where the currently committed page is scriptable by extensions,
but the target of the pending navigation is is not.
- ExecuteScript: Do not send IPC to non-live frames (they're not going
to reply anyway).
- Include URL in the error if the extension has the tabs permission
(follow-up to TODO from https://codereview.chromium.org/1414223005).
BUG=63979, 551626
TEST=./browser_tests --gtest_filter=ExecuteScriptApiTest.*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/52277c872b368db45bc6d2f2bd9a4b7a0eb2ce8d
Cr-Commit-Position: refs/heads/master@{#374057}
Patch Set 1 #Patch Set 2 : Revert to wc->GetURL for chrome.tabs.update #
Total comments: 12
Patch Set 3 : Nits + comments #
Total comments: 26
Patch Set 4 : Devlins nits #Patch Set 5 : Remove RenderFrameHost::ForEachChildFrame #
Total comments: 6
Patch Set 6 : Last nits #Patch Set 7 : rebase #Patch Set 8 : Remove unused include from rebase #Messages
Total messages: 46 (17 generated)
Description was changed from ========== Add frameId to chrome.tabs.executeScript/insertCSS - Re-implemented https://codereview.chromium.org/952473002/, with all checks at the browser side instead of just the renderer. - Use the last committed URL for checking permissions instead of the visible URL. As a result, executeScript/insertCSS will now succeed for frames were a new navigation was requested but not committed yet. - Add RenderFrameHost::ForEachChildFrame. - ExecuteScript: Do not send IPC to non-live frames (they're not going to reply anyway). - Include URL in the error if the extension has the tabs permission (follow-up to TODO from https://codereview.chromium.org/1414223005). BUG=63979,551626 TEST=./browser_tests --gtest_filter=ExecuteScriptApiTest.* ========== to ========== Add frameId to chrome.tabs.executeScript/insertCSS - Re-implemented https://codereview.chromium.org/952473002/, with all checks at the browser side instead of just the renderer. - Use the last committed URL for checking permissions instead of the visible URL. As a result, executeScript/insertCSS will now succeed for frames were a new navigation was requested but not committed yet. - Add RenderFrameHost::ForEachChildFrame. - ExecuteScript: Do not send IPC to non-live frames (they're not going to reply anyway). - Include URL in the error if the extension has the tabs permission (follow-up to TODO from https://codereview.chromium.org/1414223005). BUG=63979,551626 TEST=./browser_tests --gtest_filter=ExecuteScriptApiTest.* CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Add frameId to chrome.tabs.executeScript/insertCSS - Re-implemented https://codereview.chromium.org/952473002/, with all checks at the browser side instead of just the renderer. - Use the last committed URL for checking permissions instead of the visible URL. As a result, executeScript/insertCSS will now succeed for frames were a new navigation was requested but not committed yet. - Add RenderFrameHost::ForEachChildFrame. - ExecuteScript: Do not send IPC to non-live frames (they're not going to reply anyway). - Include URL in the error if the extension has the tabs permission (follow-up to TODO from https://codereview.chromium.org/1414223005). BUG=63979,551626 TEST=./browser_tests --gtest_filter=ExecuteScriptApiTest.* CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Add frameId to chrome.tabs.executeScript/insertCSS - Re-implemented https://codereview.chromium.org/952473002/, with all checks at the browser side instead of just the renderer. - Use the last committed URL for checking permissions instead of the visible URL. As a result, executeScript/insertCSS will now succeed in frames where the currently committed page is scriptable by extensions, but the target of the pending navigation is is not. - Add RenderFrameHost::ForEachChildFrame. - ExecuteScript: Do not send IPC to non-live frames (they're not going to reply anyway). - Include URL in the error if the extension has the tabs permission (follow-up to TODO from https://codereview.chromium.org/1414223005). BUG=63979,551626 TEST=./browser_tests --gtest_filter=ExecuteScriptApiTest.* CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
rob@robwu.nl changed reviewers: + jam@chromium.org, rdevlin.cronin@chromium.org
jam: content/ devlin: The rest.
I haven't made it through the tests yet, but this looks generally good. https://codereview.chromium.org/1628423002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/executescript/frame_id/test.js (right): https://codereview.chromium.org/1628423002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/frame_id/test.js:18: var id_frame_top = 0; nit: js style https://codereview.chromium.org/1628423002/diff/20001/extensions/browser/scri... File extensions/browser/script_executor.cc (right): https://codereview.chromium.org/1628423002/diff/20001/extensions/browser/scri... extensions/browser/script_executor.cc:175: // Whether to inject in all frames. nit: "inject in all child frames" https://codereview.chromium.org/1628423002/diff/20001/extensions/browser/scri... File extensions/browser/script_executor.h (right): https://codereview.chromium.org/1628423002/diff/20001/extensions/browser/scri... extensions/browser/script_executor.h:92: int frame_id, Man, this function makes me so sad. At the same time, if we moved it to a Params struct, then no one would ever update the callers because we wouldn't get a compile error. Ah well. Complaints aside, please document the |frame_id| argument briefly (and mention it's behavior with FrameScope). https://codereview.chromium.org/1628423002/diff/20001/extensions/common/api/e... File extensions/common/api/extension_types.json (right): https://codereview.chromium.org/1628423002/diff/20001/extensions/common/api/e... extensions/common/api/extension_types.json:57: "description": "The <a href='webNavigation#frame_ids'>frame</a> where the script or CSS should be injected." Mention behavior if not specified. https://codereview.chromium.org/1628423002/diff/20001/extensions/renderer/pro... File extensions/renderer/programmatic_script_injector.cc (right): https://codereview.chromium.org/1628423002/diff/20001/extensions/renderer/pro... extensions/renderer/programmatic_script_injector.cc:161: return extension->permissions_data()->active_permissions().HasAPIPermission( It might be worth a note that we don't need to account for activeTab here because if the extension has activeTab, it wouldn't have been rejected.
https://codereview.chromium.org/1628423002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/executescript/frame_id/test.js (right): https://codereview.chromium.org/1628423002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/frame_id/test.js:18: var id_frame_top = 0; On 2016/01/25 19:36:40, Devlin (Slow until 1-27) wrote: > nit: js style Done. https://codereview.chromium.org/1628423002/diff/20001/extensions/browser/scri... File extensions/browser/script_executor.cc (right): https://codereview.chromium.org/1628423002/diff/20001/extensions/browser/scri... extensions/browser/script_executor.cc:175: // Whether to inject in all frames. On 2016/01/25 19:36:40, Devlin (Slow until 1-27) wrote: > nit: "inject in all child frames" Changed to "in all descendant frames of |root_rfh|". https://codereview.chromium.org/1628423002/diff/20001/extensions/browser/scri... File extensions/browser/script_executor.h (right): https://codereview.chromium.org/1628423002/diff/20001/extensions/browser/scri... extensions/browser/script_executor.h:92: int frame_id, On 2016/01/25 19:36:40, Devlin (Slow until 1-27) wrote: > Man, this function makes me so sad. At the same time, if we moved it to a > Params struct, then no one would ever update the callers because we wouldn't get > a compile error. Ah well. > > Complaints aside, please document the |frame_id| argument briefly (and mention > it's behavior with FrameScope). Done. https://codereview.chromium.org/1628423002/diff/20001/extensions/common/api/e... File extensions/common/api/extension_types.json (right): https://codereview.chromium.org/1628423002/diff/20001/extensions/common/api/e... extensions/common/api/extension_types.json:57: "description": "The <a href='webNavigation#frame_ids'>frame</a> where the script or CSS should be injected." On 2016/01/25 19:36:40, Devlin (Slow until 1-27) wrote: > Mention behavior if not specified. Done. https://codereview.chromium.org/1628423002/diff/20001/extensions/renderer/pro... File extensions/renderer/programmatic_script_injector.cc (right): https://codereview.chromium.org/1628423002/diff/20001/extensions/renderer/pro... extensions/renderer/programmatic_script_injector.cc:161: return extension->permissions_data()->active_permissions().HasAPIPermission( On 2016/01/25 19:36:40, Devlin (Slow until 1-27) wrote: > It might be worth a note that we don't need to account for activeTab here > because if the extension has activeTab, it wouldn't have been rejected. Even with activeTab, one does not get access to restricted URLs such as chrome://. Should I add a check for activeTab as well?
lgtm with nits. https://codereview.chromium.org/1628423002/diff/20001/extensions/renderer/pro... File extensions/renderer/programmatic_script_injector.cc (right): https://codereview.chromium.org/1628423002/diff/20001/extensions/renderer/pro... extensions/renderer/programmatic_script_injector.cc:161: return extension->permissions_data()->active_permissions().HasAPIPermission( On 2016/01/26 11:03:02, robwu wrote: > On 2016/01/25 19:36:40, Devlin (Slow until 1-27) wrote: > > It might be worth a note that we don't need to account for activeTab here > > because if the extension has activeTab, it wouldn't have been rejected. > > Even with activeTab, one does not get access to restricted URLs such as > chrome://. Should I add a check for activeTab as well? Nah - activeTab shouldn't really grant any access to chrome:// urls, so this is okay. https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/executescript/basic/test.js (right): https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/basic/test.js:91: var doneListening = What's the reason for this change? https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/executescript/frame_id/test.js (right): https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/frame_id/test.js:42: var gCssCounter = 0; nit: \n https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/frame_id/test.js:43: // Calls chrome.tabs.insertCSS and invoke the callback with a list of affected *invokes https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/frame_id/test.js:44: // URLs. This function assumes that the insertCSS call will succeed. insertCSS and also executeScript, right? https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/frame_id/test.js:59: .filter(function(result) { I think it's just the indentation that makes this, well, a bit ugly. Can you pull the filter().map() part out into a helper function below marker called filterResults or something, and maybe add a quick comment on what it's doing? https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/frame_id/test.js:79: function getFrameId(R_URL) { strange to have a function param in CONST_STYLE. https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/frame_id/test.js:101: function runTests(config) { Awesome tests. Unfortunately, I'm worried it's going to be too much to handle on our poor, slow, WindowsDbg bots. Can we split these up without changing too much? E.g. have a function runScriptTests() and another runCssTests() that can be called from two separate browser tests? If not, we can submit, and see what happens - but it'd be a shame to have these disabled for being slow. https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/frame_id/test.js:103: function executeScriptFrameIdTop() { There's enough parameters in these test names that the name itself doesn't make it immediately clear what's happening. Can you add a comment above each test, or add more words in the test name (e.g. executeScriptByFrameIdInTopFrame)? https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/frame_id/test.js:111: function executeScriptFrameIdTopAllFrames() { executeScriptByFrameIdInTopFrameWithSubFrames or something. Characters are cheap, and trying to parse test names isn't (and misunderstanding the motivation of a test, even less so). https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/frame_id/test.js:208: function executeScriptFrameIdNestedFrame() { Is this the same name as line 199? https://codereview.chromium.org/1628423002/diff/40001/extensions/browser/scri... File extensions/browser/script_executor.h (right): https://codereview.chromium.org/1628423002/diff/40001/extensions/browser/scri... extensions/browser/script_executor.h:48: ALL_FRAMES, "ALL_FRAMES" is also wrong, now - it should be CHILD_ or SUB_FRAMES or something along those lines (should just be a find-replace - if for whatever reason it's more complicated, feel free to postpone to a later CL).
jam@chromium.org changed reviewers: + nasko@chromium.org
Nasko: is this public API something we want to support ? https://codereview.chromium.org/1628423002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1628423002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:323: if (this != frame_tree_node_->current_frame_host()) { why is this needed?
https://codereview.chromium.org/1628423002/diff/20001/extensions/renderer/pro... File extensions/renderer/programmatic_script_injector.cc (right): https://codereview.chromium.org/1628423002/diff/20001/extensions/renderer/pro... extensions/renderer/programmatic_script_injector.cc:161: return extension->permissions_data()->active_permissions().HasAPIPermission( On 2016/01/26 20:40:36, Devlin (Slow until 1-27) wrote: > On 2016/01/26 11:03:02, robwu wrote: > > On 2016/01/25 19:36:40, Devlin (Slow until 1-27) wrote: > > > It might be worth a note that we don't need to account for activeTab here > > > because if the extension has activeTab, it wouldn't have been rejected. > > > > Even with activeTab, one does not get access to restricted URLs such as > > chrome://. Should I add a check for activeTab as well? > > Nah - activeTab shouldn't really grant any access to chrome:// urls, so this is > okay. Acknowledged. https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/executescript/basic/test.js (right): https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/basic/test.js:91: var doneListening = On 2016/01/26 20:40:36, Devlin (Slow until 1-27) wrote: > What's the reason for this change? Otherwise the test would fail, because chrome.tabs.executeScript now uses the last committed URL for determining script injection, instead of the visible URL. Calling chrome.tabs.update immediately changes the visible URL, but not the last committed URL. https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/executescript/frame_id/test.js (right): https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/frame_id/test.js:42: var gCssCounter = 0; On 2016/01/26 20:40:36, Devlin (Slow until 1-27) wrote: > nit: \n Done. https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/frame_id/test.js:43: // Calls chrome.tabs.insertCSS and invoke the callback with a list of affected On 2016/01/26 20:40:36, Devlin (Slow until 1-27) wrote: > *invokes Done. https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/frame_id/test.js:44: // URLs. This function assumes that the insertCSS call will succeed. On 2016/01/26 20:40:36, Devlin (Slow until 1-27) wrote: > insertCSS and also executeScript, right? I really meant insertCSS (in relation with the arguments). Updated comment. https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/frame_id/test.js:59: .filter(function(result) { On 2016/01/26 20:40:36, Devlin (Slow until 1-27) wrote: > I think it's just the indentation that makes this, well, a bit ugly. Can you > pull the filter().map() part out into a helper function below marker called > filterResults or something, and maybe add a quick comment on what it's doing? Done. https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/frame_id/test.js:79: function getFrameId(R_URL) { On 2016/01/26 20:40:36, Devlin (Slow until 1-27) wrote: > strange to have a function param in CONST_STYLE. Done. https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/frame_id/test.js:101: function runTests(config) { On 2016/01/26 20:40:36, Devlin (Slow until 1-27) wrote: > Awesome tests. Unfortunately, I'm worried it's going to be too much to handle > on our poor, slow, WindowsDbg bots. Can we split these up without changing too > much? E.g. have a function runScriptTests() and another runCssTests() that can > be called from two separate browser tests? > > If not, we can submit, and see what happens - but it'd be a shame to have these > disabled for being slow. What is the maximum execution time? I'll keep the tests together for now, and split it up when there is evidence that it's too slow on the windows bots. https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/frame_id/test.js:103: function executeScriptFrameIdTop() { On 2016/01/26 20:40:36, Devlin (Slow until 1-27) wrote: > There's enough parameters in these test names that the name itself doesn't make > it immediately clear what's happening. Can you add a comment above each test, > or add more words in the test name (e.g. executeScriptByFrameIdInTopFrame)? Done. https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/frame_id/test.js:111: function executeScriptFrameIdTopAllFrames() { On 2016/01/26 20:40:36, Devlin (Slow until 1-27) wrote: > executeScriptByFrameIdInTopFrameWithSubFrames or something. Characters are > cheap, and trying to parse test names isn't (and misunderstanding the motivation > of a test, even less so). Done. https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/frame_id/test.js:208: function executeScriptFrameIdNestedFrame() { On 2016/01/26 20:40:36, Devlin (Slow until 1-27) wrote: > Is this the same name as line 199? Done. https://codereview.chromium.org/1628423002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1628423002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:323: if (this != frame_tree_node_->current_frame_host()) { On 2016/01/26 23:42:34, jam wrote: > why is this needed? Just in case someone calls ForEachChildFrame on a RenderFrameHost that was swapped. This shouldn't happen, but code will still compile, so it could happen, and I don't want to report child frames which are not childs of the given RFH. https://codereview.chromium.org/1628423002/diff/40001/extensions/browser/scri... File extensions/browser/script_executor.h (right): https://codereview.chromium.org/1628423002/diff/40001/extensions/browser/scri... extensions/browser/script_executor.h:48: ALL_FRAMES, On 2016/01/26 20:40:36, Devlin (Slow until 1-27) wrote: > "ALL_FRAMES" is also wrong, now - it should be CHILD_ or SUB_FRAMES or something > along those lines (should just be a find-replace - if for whatever reason it's > more complicated, feel free to postpone to a later CL). Done. INCLUDE_SUB_FRAMES
On 2016/01/26 23:42:34, jam wrote: > Nasko: is this public API something we want to support ? I'm not thrilled about such an API. Isn't it the case that it will only be needed on the main frame? If that is true, we can just use WebContents::ForEachFrame and send an IPC to each one, sans the main.
https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/executescript/basic/test.js (right): https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/basic/test.js:91: var doneListening = On 2016/01/26 23:57:58, robwu wrote: > On 2016/01/26 20:40:36, Devlin (Slow until 1-27) wrote: > > What's the reason for this change? > > Otherwise the test would fail, because chrome.tabs.executeScript now uses the > last committed URL for determining script injection, instead of the visible URL. > Calling chrome.tabs.update immediately changes the visible URL, but not the last > committed URL. Heh. That's the right choice, but I'm sure this is gonna bite us some day... https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/executescript/frame_id/test.js (right): https://codereview.chromium.org/1628423002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/executescript/frame_id/test.js:101: function runTests(config) { On 2016/01/26 23:57:59, robwu wrote: > On 2016/01/26 20:40:36, Devlin (Slow until 1-27) wrote: > > Awesome tests. Unfortunately, I'm worried it's going to be too much to handle > > on our poor, slow, WindowsDbg bots. Can we split these up without changing > too > > much? E.g. have a function runScriptTests() and another runCssTests() that > can > > be called from two separate browser tests? > > > > If not, we can submit, and see what happens - but it'd be a shame to have > these > > disabled for being slow. > > What is the maximum execution time? > I'll keep the tests together for now, and split it up when there is evidence > that it's too slow on the windows bots. We have different execution times depending on the bot, but assume 45 seconds - I think that's what it is for every "normal" bot, including WinDbg. We have much, *much* higher thresholds for bots like Dr Memory.
On 2016/01/27 18:24:33, nasko wrote: > On 2016/01/26 23:42:34, jam wrote: > > Nasko: is this public API something we want to support ? > > I'm not thrilled about such an API. Isn't it the case that it will only be > needed on the main frame? If that is true, we can just use > WebContents::ForEachFrame and send an IPC to each one, sans the main. It is needed for arbitrary frames. Using ForEachFrame + checking whether the frame is a descendant of the given frame seemed too inefficient to me.
On 2016/01/27 19:15:03, robwu wrote: > On 2016/01/27 18:24:33, nasko wrote: > > On 2016/01/26 23:42:34, jam wrote: > > > Nasko: is this public API something we want to support ? > > > > I'm not thrilled about such an API. Isn't it the case that it will only be > > needed on the main frame? If that is true, we can just use > > WebContents::ForEachFrame and send an IPC to each one, sans the main. > > It is needed for arbitrary frames. Using ForEachFrame + checking whether the > frame is a descendant of the given frame seemed too inefficient to me. I would actually suggest we don't expose this functionality to the extension API to begin with. Looking at the API, I would suggest keeping "allFrames" and "frameId" exclusive. Their naming is also suggestive that either we want to inject in all frames or in a specific frame. I am generally a proponent of starting with smaller API and expanding if needed. Extension developers can get the same effect of "frameId+subframes" by doing minimal amount of extra work. The webNavigation API gives them all the frames on a page and the parenting relationships. If this proves to be too big of a burden and there is enough demand for it, I'm ok expanding it later on, but starting smaller makes a ton of sense to me. What do you think?
On 2016/01/27 19:20:50, nasko wrote: > On 2016/01/27 19:15:03, robwu wrote: > > On 2016/01/27 18:24:33, nasko wrote: > > > On 2016/01/26 23:42:34, jam wrote: > > > > Nasko: is this public API something we want to support ? > > > > > > I'm not thrilled about such an API. Isn't it the case that it will only be > > > needed on the main frame? If that is true, we can just use > > > WebContents::ForEachFrame and send an IPC to each one, sans the main. > > > > It is needed for arbitrary frames. Using ForEachFrame + checking whether the > > frame is a descendant of the given frame seemed too inefficient to me. > > I would actually suggest we don't expose this functionality to the extension API > to begin with. Looking at the API, I would suggest keeping "allFrames" and > "frameId" exclusive. Their naming is also suggestive that either we want to > inject in all frames or in a specific frame. Currently (without this CL), scripts don't always get injected in all frames (e.g. due to lack of permission). And with manifest-declared content scripts, a script can run a subframe without ever running in the main frame (if the URL pattern doesn't match). Their relationship is well-documented, so the risk of confusion is low. > I am generally a proponent of > starting with smaller API and expanding if needed. Extension developers can get > the same effect of "frameId+subframes" by doing minimal amount of extra work. > The webNavigation API gives them all the frames on a page and the parenting > relationships. If this proves to be too big of a burden and there is enough > demand for it, I'm ok expanding it later on, but starting smaller makes a ton of > sense to me. > > What do you think? In general, I agree with that. In this specific case, supporting allFrames for child frames is not more costly than supporting main frame injections. So I vote for keeping if there are any benefits and no significant disadvantages, Here are two to start with: *** Performance frameId has a smaller scope than the current situation. If used correctly, extensions can provide their functionality with better performance (not injecting in all frames, but only a subtree). With only frameId, if an extension wants to inject in multiple frames, there will be IPC for every executeScript call, which is not as performant. *** Permission warnings chrome.tabs.executeScript can be used with host permissions only, whereas using the webNavigation API adds a new permission warning. I've opened a feature request for webNavigation without permissions (https://crbug.com/431108), but it's not implemented yet. If they are not convincing, we can ask users who starred the ticket for feedback, and see whether there is any demand for allFrames + frameId.
On 2016/01/28 00:54:28, robwu wrote: > On 2016/01/27 19:20:50, nasko wrote: > > On 2016/01/27 19:15:03, robwu wrote: > > > On 2016/01/27 18:24:33, nasko wrote: > > > > On 2016/01/26 23:42:34, jam wrote: > > > > > Nasko: is this public API something we want to support ? > > > > > > > > I'm not thrilled about such an API. Isn't it the case that it will only be > > > > needed on the main frame? If that is true, we can just use > > > > WebContents::ForEachFrame and send an IPC to each one, sans the main. > > > > > > It is needed for arbitrary frames. Using ForEachFrame + checking whether the > > > frame is a descendant of the given frame seemed too inefficient to me. > > > > I would actually suggest we don't expose this functionality to the extension > API > > to begin with. Looking at the API, I would suggest keeping "allFrames" and > > "frameId" exclusive. Their naming is also suggestive that either we want to > > inject in all frames or in a specific frame. > > Currently (without this CL), scripts don't always get injected in all frames > (e.g. due to lack of permission). And with manifest-declared content scripts, a > script can run a subframe without ever running in the main frame (if the URL > pattern doesn't match). Their relationship is well-documented, so the risk of > confusion is low. > > > I am generally a proponent of > > starting with smaller API and expanding if needed. Extension developers can > get > > the same effect of "frameId+subframes" by doing minimal amount of extra work. > > The webNavigation API gives them all the frames on a page and the parenting > > relationships. If this proves to be too big of a burden and there is enough > > demand for it, I'm ok expanding it later on, but starting smaller makes a ton > of > > sense to me. > > > > What do you think? > > In general, I agree with that. In this specific case, supporting allFrames for > child frames is not more costly than supporting main frame injections. The current "cost" is a very specific content/public/ API that I don't see as useful beyond this specific case. However, the same can be achieved with WebContents::ForEachFrame and compiling a set of the subframes that have a specific frame as an ancestor. > So I vote > for keeping if there are any benefits and no significant disadvantages, Here are > two to start with: > > *** Performance > frameId has a smaller scope than the current situation. If used correctly, > extensions can provide their functionality with better performance (not > injecting in all frames, but only a subtree). With only frameId, if an extension > wants to inject in multiple frames, there will be IPC for every executeScript > call, which is not as performant. I agree with the performance categorization. However, what is the frequency of the use case of all subframes of a frame vs all frames total? > *** Permission warnings > chrome.tabs.executeScript can be used with host permissions only, whereas using > the webNavigation API adds a new permission warning. I've opened a feature > request for webNavigation without permissions (https://crbug.com/431108), but > it's not implemented yet. The permission is a very good argument! > If they are not convincing, we can ask users who starred the ticket for > feedback, and see whether there is any demand for allFrames + frameId.
On 2016/01/28 23:15:13, nasko wrote: > On 2016/01/28 00:54:28, robwu wrote: > > On 2016/01/27 19:20:50, nasko wrote: > > > On 2016/01/27 19:15:03, robwu wrote: > > > > On 2016/01/27 18:24:33, nasko wrote: > > > > > On 2016/01/26 23:42:34, jam wrote: > > > > > > Nasko: is this public API something we want to support ? > > > > > > > > > > I'm not thrilled about such an API. Isn't it the case that it will only > be > > > > > needed on the main frame? If that is true, we can just use > > > > > WebContents::ForEachFrame and send an IPC to each one, sans the main. > > > > > > > > It is needed for arbitrary frames. Using ForEachFrame + checking whether > the > > > > frame is a descendant of the given frame seemed too inefficient to me. > > > > > > I would actually suggest we don't expose this functionality to the extension > > API > > > to begin with. Looking at the API, I would suggest keeping "allFrames" and > > > "frameId" exclusive. Their naming is also suggestive that either we want to > > > inject in all frames or in a specific frame. > > > > Currently (without this CL), scripts don't always get injected in all frames > > (e.g. due to lack of permission). And with manifest-declared content scripts, > a > > script can run a subframe without ever running in the main frame (if the URL > > pattern doesn't match). Their relationship is well-documented, so the risk of > > confusion is low. > > > > > I am generally a proponent of > > > starting with smaller API and expanding if needed. Extension developers can > > get > > > the same effect of "frameId+subframes" by doing minimal amount of extra > work. > > > The webNavigation API gives them all the frames on a page and the parenting > > > relationships. If this proves to be too big of a burden and there is enough > > > demand for it, I'm ok expanding it later on, but starting smaller makes a > ton > > of > > > sense to me. > > > > > > What do you think? > > > > In general, I agree with that. In this specific case, supporting allFrames for > > child frames is not more costly than supporting main frame injections. > > The current "cost" is a very specific content/public/ API that I don't see as > useful beyond this specific case. However, the same can be achieved with > WebContents::ForEachFrame and compiling a set of the subframes that have a > specific frame as an ancestor. Do you prefer a O(dn) algorithm (WC:ForEachFrame + looking up the parent) over a O(n) algorithm (the new API) (where d is the depth of nested frame tree, and n is the number of frames)? > > So I vote > > for keeping if there are any benefits and no significant disadvantages, Here > are > > two to start with: > > > > *** Performance > > frameId has a smaller scope than the current situation. If used correctly, > > extensions can provide their functionality with better performance (not > > injecting in all frames, but only a subtree). With only frameId, if an > extension > > wants to inject in multiple frames, there will be IPC for every executeScript > > call, which is not as performant. > > I agree with the performance categorization. However, what is the frequency of > the use case of all subframes of a frame vs all frames total? I don't know. If desired, we can poll on the crbug for feedback. A generic use case is injection in all frames of some site, regardless of whether the site is the main frame or a sub frame. E.g. Spotify's web player is composed of frames. If Spotify is embedded in some other site (*), and an extension wants to inject in Spotify only (without permissions for the main frame document), then having allFrames+frameId would help. (*) Idk if Spotify can be embedded in another site, but for the sake of the argument let's assume that it's possible; if there is a X-Frame-Options header, the extension can remove it via webRequest. > > *** Permission warnings > > chrome.tabs.executeScript can be used with host permissions only, whereas > using > > the webNavigation API adds a new permission warning. I've opened a feature > > request for webNavigation without permissions (https://crbug.com/431108), but > > it's not implemented yet. > > The permission is a very good argument! > > > If they are not convincing, we can ask users who starred the ticket for > > feedback, and see whether there is any demand for allFrames + frameId.
On 2016/01/28 23:38:51, robwu wrote: > On 2016/01/28 23:15:13, nasko wrote: > > On 2016/01/28 00:54:28, robwu wrote: > > > On 2016/01/27 19:20:50, nasko wrote: > > > > On 2016/01/27 19:15:03, robwu wrote: > > > > > On 2016/01/27 18:24:33, nasko wrote: > > > > > > On 2016/01/26 23:42:34, jam wrote: > > > > > > > Nasko: is this public API something we want to support ? > > > > > > > > > > > > I'm not thrilled about such an API. Isn't it the case that it will > only > > be > > > > > > needed on the main frame? If that is true, we can just use > > > > > > WebContents::ForEachFrame and send an IPC to each one, sans the main. > > > > > > > > > > It is needed for arbitrary frames. Using ForEachFrame + checking whether > > the > > > > > frame is a descendant of the given frame seemed too inefficient to me. > > > > > > > > I would actually suggest we don't expose this functionality to the > extension > > > API > > > > to begin with. Looking at the API, I would suggest keeping "allFrames" and > > > > "frameId" exclusive. Their naming is also suggestive that either we want > to > > > > inject in all frames or in a specific frame. > > > > > > Currently (without this CL), scripts don't always get injected in all frames > > > (e.g. due to lack of permission). And with manifest-declared content > scripts, > > a > > > script can run a subframe without ever running in the main frame (if the URL > > > pattern doesn't match). Their relationship is well-documented, so the risk > of > > > confusion is low. > > > > > > > I am generally a proponent of > > > > starting with smaller API and expanding if needed. Extension developers > can > > > get > > > > the same effect of "frameId+subframes" by doing minimal amount of extra > > work. > > > > The webNavigation API gives them all the frames on a page and the > parenting > > > > relationships. If this proves to be too big of a burden and there is > enough > > > > demand for it, I'm ok expanding it later on, but starting smaller makes a > > ton > > > of > > > > sense to me. > > > > > > > > What do you think? > > > > > > In general, I agree with that. In this specific case, supporting allFrames > for > > > child frames is not more costly than supporting main frame injections. > > > > The current "cost" is a very specific content/public/ API that I don't see as > > useful beyond this specific case. However, the same can be achieved with > > WebContents::ForEachFrame and compiling a set of the subframes that have a > > specific frame as an ancestor. > > Do you prefer a O(dn) algorithm (WC:ForEachFrame + looking up the parent) over a > O(n) algorithm (the new API) (where d is the depth of nested frame tree, and n > is the number of frames)? With the typical number of frames and depth I don't think there is a significant difference in O(dn) and O(n). I would rather not have an API in content/ that can be abused by others. Also, dcheng@ is working on a range iterator for frames, which might allow us to iterate over subframes only. If/when that makes it to the WebContents public API, we can update the extensions code to use it. > > > So I vote > > > for keeping if there are any benefits and no significant disadvantages, Here > > are > > > two to start with: > > > > > > *** Performance > > > frameId has a smaller scope than the current situation. If used correctly, > > > extensions can provide their functionality with better performance (not > > > injecting in all frames, but only a subtree). With only frameId, if an > > extension > > > wants to inject in multiple frames, there will be IPC for every > executeScript > > > call, which is not as performant. > > > > I agree with the performance categorization. However, what is the frequency of > > the use case of all subframes of a frame vs all frames total? > > I don't know. If desired, we can poll on the crbug for feedback. > A generic use case is injection in all frames of some site, regardless of > whether the site is the main frame or a sub frame. E.g. Spotify's web player is > composed of frames. If Spotify is embedded in some other site (*), and an > extension wants to inject in Spotify only (without permissions for the main > frame document), then having allFrames+frameId would help. > (*) Idk if Spotify can be embedded in another site, but for the sake of the > argument let's assume that it's possible; if there is a X-Frame-Options header, > the extension can remove it via webRequest. > > > > *** Permission warnings > > > chrome.tabs.executeScript can be used with host permissions only, whereas > > using > > > the webNavigation API adds a new permission warning. I've opened a feature > > > request for webNavigation without permissions (https://crbug.com/431108), > but > > > it's not implemented yet. > > > > The permission is a very good argument! > > > > > If they are not convincing, we can ask users who starred the ticket for > > > feedback, and see whether there is any demand for allFrames + frameId.
Description was changed from ========== Add frameId to chrome.tabs.executeScript/insertCSS - Re-implemented https://codereview.chromium.org/952473002/, with all checks at the browser side instead of just the renderer. - Use the last committed URL for checking permissions instead of the visible URL. As a result, executeScript/insertCSS will now succeed in frames where the currently committed page is scriptable by extensions, but the target of the pending navigation is is not. - Add RenderFrameHost::ForEachChildFrame. - ExecuteScript: Do not send IPC to non-live frames (they're not going to reply anyway). - Include URL in the error if the extension has the tabs permission (follow-up to TODO from https://codereview.chromium.org/1414223005). BUG=63979,551626 TEST=./browser_tests --gtest_filter=ExecuteScriptApiTest.* CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Add frameId to chrome.tabs.executeScript/insertCSS - Re-implemented https://codereview.chromium.org/952473002/, with all checks at the browser side instead of just the renderer. - Use the last committed URL for checking permissions instead of the visible URL. As a result, executeScript/insertCSS will now succeed in frames where the currently committed page is scriptable by extensions, but the target of the pending navigation is is not. - ExecuteScript: Do not send IPC to non-live frames (they're not going to reply anyway). - Include URL in the error if the extension has the tabs permission (follow-up to TODO from https://codereview.chromium.org/1414223005). BUG=63979,551626 TEST=./browser_tests --gtest_filter=ExecuteScriptApiTest.* CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
rob@robwu.nl changed reviewers: - jam@chromium.org
Patchset #5 (id:80001) has been deleted
On 2016/01/28 23:54:36, nasko wrote: > On 2016/01/28 23:38:51, robwu wrote: > > Do you prefer a O(dn) algorithm (WC:ForEachFrame + looking up the parent) over > a > > O(n) algorithm (the new API) (where d is the depth of nested frame tree, and n > > is the number of frames)? > > With the typical number of frames and depth I don't think there is a significant > difference in O(dn) and O(n). I would rather not have an API in content/ that > can be abused by others. Also, dcheng@ is working on a range iterator for > frames, which might allow us to iterate over subframes only. If/when that makes > it to the WebContents public API, we can update the extensions code to use it. I reverted the content/ changes. Please review the latest patch set.
s lgtm https://codereview.chromium.org/1628423002/diff/100001/extensions/browser/scr... File extensions/browser/script_executor.cc (right): https://codereview.chromium.org/1628423002/diff/100001/extensions/browser/scr... extensions/browser/script_executor.cc:110: DCHECK(ShouldIncludeFrame(frame)); Ah, DCHECKS. So, I get worried when we have if statements like this, because we do funny compilation stuff (or lack thereof). I'm always worried this will somehow lead to a goto fail style error because we compile out the macro (no matter how unlikely it is). With that in mind, I would prefer: if (!ShouldIncludeFrame(frame)) { DCHECK(!root_is_main_frame_); return; } (I think it's cleaner that way anyways - fewer calls to ShouldIncludeFrame makes more sense to me.) https://codereview.chromium.org/1628423002/diff/100001/extensions/browser/scr... File extensions/browser/script_executor.h (right): https://codereview.chromium.org/1628423002/diff/100001/extensions/browser/scr... extensions/browser/script_executor.h:86: // an extension API frame ID). If |frame_scope| is ALL_FRAMES, then the script nit: s/ALL_FRAMES/INCLUDE_SUB_FRAMES
Since there isn't any content/ code, you don't technically need my review. I read through the new version of the script_executor and looks good! It is actually better than I expected.
https://codereview.chromium.org/1628423002/diff/100001/extensions/browser/scr... File extensions/browser/script_executor.cc (right): https://codereview.chromium.org/1628423002/diff/100001/extensions/browser/scr... extensions/browser/script_executor.cc:110: DCHECK(ShouldIncludeFrame(frame)); On 2016/02/01 19:04:07, Devlin wrote: > Ah, DCHECKS. So, I get worried when we have if statements like this, because we > do funny compilation stuff (or lack thereof). I'm always worried this will > somehow lead to a goto fail style error because we compile out the macro (no > matter how unlikely it is). > > With that in mind, I would prefer: > if (!ShouldIncludeFrame(frame)) { > DCHECK(!root_is_main_frame_); > return; > } > > (I think it's cleaner that way anyways - fewer calls to ShouldIncludeFrame makes > more sense to me.) The current construct is an optimization. In most cases, the script injection is directed at the main frame, which includes all frames, so the ShouldIncludeFrame check is not necessary. The DCHECK documents this precondition. If you're uncomfortable with the current appearance of the code, then I can add braces or change it to: DCHECK(!root_is_main_frame_ || ShouldIncludeFrame(frame)); if (!root_is_main_frame_ && !ShouldIncludeFrame(frame)) return;
https://codereview.chromium.org/1628423002/diff/100001/extensions/browser/scr... File extensions/browser/script_executor.cc (right): https://codereview.chromium.org/1628423002/diff/100001/extensions/browser/scr... extensions/browser/script_executor.cc:110: DCHECK(ShouldIncludeFrame(frame)); On 2016/02/01 23:17:44, robwu wrote: > On 2016/02/01 19:04:07, Devlin wrote: > > Ah, DCHECKS. So, I get worried when we have if statements like this, because > we > > do funny compilation stuff (or lack thereof). I'm always worried this will > > somehow lead to a goto fail style error because we compile out the macro (no > > matter how unlikely it is). > > > > With that in mind, I would prefer: > > if (!ShouldIncludeFrame(frame)) { > > DCHECK(!root_is_main_frame_); > > return; > > } > > > > (I think it's cleaner that way anyways - fewer calls to ShouldIncludeFrame > makes > > more sense to me.) > > The current construct is an optimization. > In most cases, the script injection is directed at the main frame, which > includes all frames, so the ShouldIncludeFrame check is not necessary. The > DCHECK documents this precondition. > > If you're uncomfortable with the current appearance of the code, then I can add > braces or change it to: > > DCHECK(!root_is_main_frame_ || ShouldIncludeFrame(frame)); > if (!root_is_main_frame_ && !ShouldIncludeFrame(frame)) > return; I don't think the performance is all that significant, but I'm okay with your second option.
https://codereview.chromium.org/1628423002/diff/100001/extensions/browser/scr... File extensions/browser/script_executor.cc (right): https://codereview.chromium.org/1628423002/diff/100001/extensions/browser/scr... extensions/browser/script_executor.cc:110: DCHECK(ShouldIncludeFrame(frame)); On 2016/02/01 23:28:13, Devlin wrote: > On 2016/02/01 23:17:44, robwu wrote: > > On 2016/02/01 19:04:07, Devlin wrote: > > > Ah, DCHECKS. So, I get worried when we have if statements like this, > because > > we > > > do funny compilation stuff (or lack thereof). I'm always worried this will > > > somehow lead to a goto fail style error because we compile out the macro (no > > > matter how unlikely it is). > > > > > > With that in mind, I would prefer: > > > if (!ShouldIncludeFrame(frame)) { > > > DCHECK(!root_is_main_frame_); > > > return; > > > } > > > > > > (I think it's cleaner that way anyways - fewer calls to ShouldIncludeFrame > > makes > > > more sense to me.) > > > > The current construct is an optimization. > > In most cases, the script injection is directed at the main frame, which > > includes all frames, so the ShouldIncludeFrame check is not necessary. The > > DCHECK documents this precondition. > > > > If you're uncomfortable with the current appearance of the code, then I can > add > > braces or change it to: > > > > DCHECK(!root_is_main_frame_ || ShouldIncludeFrame(frame)); > > if (!root_is_main_frame_ && !ShouldIncludeFrame(frame)) > > return; > > I don't think the performance is all that significant, but I'm okay with your > second option. Done. It's not a bottleneck, but all small bits add up (O(dn) vs O(n) - d depth of frame tree, n number of frames. d and n are typically low, but still...). https://codereview.chromium.org/1628423002/diff/100001/extensions/browser/scr... File extensions/browser/script_executor.h (right): https://codereview.chromium.org/1628423002/diff/100001/extensions/browser/scr... extensions/browser/script_executor.h:86: // an extension API frame ID). If |frame_scope| is ALL_FRAMES, then the script On 2016/02/01 19:04:07, Devlin wrote: > nit: s/ALL_FRAMES/INCLUDE_SUB_FRAMES Done.
The CQ bit was checked by rob@robwu.nl
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/1628423002/#ps120001 (title: "Last nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1628423002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1628423002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
rob@robwu.nl changed reviewers: + jam@chromium.org
+jam Please rubberstamp chrome/browser/chromeos/accessibility/accessibility_manager.cc
On 2016/02/02 15:55:09, robwu wrote: > +jam > Please rubberstamp > chrome/browser/chromeos/accessibility/accessibility_manager.cc lgtm in the future please pick a more specific owner, i.e. from chrome/browser/chromeos/accessibility/OWNERS or chrome/browser/chromeos/OWNERS
The CQ bit was checked by rob@robwu.nl
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1628423002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1628423002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by rob@robwu.nl
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/1628423002/#ps160001 (title: "Remove unused include from rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1628423002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1628423002/160001
Message was sent while issue was closed.
Description was changed from ========== Add frameId to chrome.tabs.executeScript/insertCSS - Re-implemented https://codereview.chromium.org/952473002/, with all checks at the browser side instead of just the renderer. - Use the last committed URL for checking permissions instead of the visible URL. As a result, executeScript/insertCSS will now succeed in frames where the currently committed page is scriptable by extensions, but the target of the pending navigation is is not. - ExecuteScript: Do not send IPC to non-live frames (they're not going to reply anyway). - Include URL in the error if the extension has the tabs permission (follow-up to TODO from https://codereview.chromium.org/1414223005). BUG=63979,551626 TEST=./browser_tests --gtest_filter=ExecuteScriptApiTest.* CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Add frameId to chrome.tabs.executeScript/insertCSS - Re-implemented https://codereview.chromium.org/952473002/, with all checks at the browser side instead of just the renderer. - Use the last committed URL for checking permissions instead of the visible URL. As a result, executeScript/insertCSS will now succeed in frames where the currently committed page is scriptable by extensions, but the target of the pending navigation is is not. - ExecuteScript: Do not send IPC to non-live frames (they're not going to reply anyway). - Include URL in the error if the extension has the tabs permission (follow-up to TODO from https://codereview.chromium.org/1414223005). BUG=63979,551626 TEST=./browser_tests --gtest_filter=ExecuteScriptApiTest.* CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Add frameId to chrome.tabs.executeScript/insertCSS - Re-implemented https://codereview.chromium.org/952473002/, with all checks at the browser side instead of just the renderer. - Use the last committed URL for checking permissions instead of the visible URL. As a result, executeScript/insertCSS will now succeed in frames where the currently committed page is scriptable by extensions, but the target of the pending navigation is is not. - ExecuteScript: Do not send IPC to non-live frames (they're not going to reply anyway). - Include URL in the error if the extension has the tabs permission (follow-up to TODO from https://codereview.chromium.org/1414223005). BUG=63979,551626 TEST=./browser_tests --gtest_filter=ExecuteScriptApiTest.* CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Add frameId to chrome.tabs.executeScript/insertCSS - Re-implemented https://codereview.chromium.org/952473002/, with all checks at the browser side instead of just the renderer. - Use the last committed URL for checking permissions instead of the visible URL. As a result, executeScript/insertCSS will now succeed in frames where the currently committed page is scriptable by extensions, but the target of the pending navigation is is not. - ExecuteScript: Do not send IPC to non-live frames (they're not going to reply anyway). - Include URL in the error if the extension has the tabs permission (follow-up to TODO from https://codereview.chromium.org/1414223005). BUG=63979,551626 TEST=./browser_tests --gtest_filter=ExecuteScriptApiTest.* CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/52277c872b368db45bc6d2f2bd9a4b7a0eb2ce8d Cr-Commit-Position: refs/heads/master@{#374057} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/52277c872b368db45bc6d2f2bd9a4b7a0eb2ce8d Cr-Commit-Position: refs/heads/master@{#374057} |
