Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2116613002:
Prevent duplicate content script injection defined in manifest.json (Closed)
|
Created:
4 years, 5 months ago by catmullings Modified:
4 years, 4 months ago Reviewers:
Devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionKeeps track of scripts injected by extensions installed. Checks if a script is
already injected, and injects a script only if it has not been injected.
BUG=248627
TEST= Run target browser_tests with
--gtest_filter="ExtensionApiTest.ContentScript*"
Committed: https://crrev.com/90d8dd145246616da02f5a54f1da1f5bfe6680e9
Cr-Commit-Position: refs/heads/master@{#407016}
Patch Set 1 #Patch Set 2 : Tests Added #
Total comments: 25
Patch Set 3 : Addressed code review comments #
Total comments: 12
Patch Set 4 : Addressed code review comments on patch 3 #
Total comments: 3
Patch Set 5 : Addressed code review comments from patch 4 #
Total comments: 6
Patch Set 6 : Addressed code review comments on patch 5 #Patch Set 7 : Minor followup fix to patch 5 #
Total comments: 10
Patch Set 8 : Addressed code review comments from patch 7 #
Total comments: 10
Patch Set 9 : Addressed code review on patch 8 #Messages
Total messages: 31 (10 generated)
catmullings@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
I don't see any glaring issues here; it seems like the right solution. :) Go ahead and ping again once you add a test, and I'll take another look and cover all the small things.
Test added. Let me know if this test is sufficient.
Overall, test looks pretty solid! There were also a few comments similar to your first CL, e.g. ending comments in a period, that I've omitted. Also, for your description: - Try to wrap to 72 chars for git-terminal-prettiness. :) Definitely stick below 80 char line length. - IMO, the description is a little more detailed than it needs to be. The description should say at a high level what's happening, but I don't think needs to describe the fine details of the change (in most cases - there are exceptions). For one thing, this makes it easier to read (because we only include important information), and for another, it's harder for it to get out-of-date as the patch evolves (more than once we've accidentally committed blatantly incorrect CL descriptions because we forgot to update them as we added new patch sets). https://codereview.chromium.org/2116613002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/content_script_apitest.cc (right): https://codereview.chromium.org/2116613002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/content_script_apitest.cc:338: IN_PROC_BROWSER_TEST_F(ExtensionApiTest, Though we don't do it uniformly, I think a comment at the top of the test (or comments sprinkled throughout, but that's probably overkill here) is useful for quickly understanding the purpose. So in this case, just something like: // Test that a script that matches two separate match patterns is only // injected once. https://codereview.chromium.org/2116613002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/content_script_apitest.cc:344: GURL url = GURL("http://maps.google.com:" We have handy utility called base::StringPrintf, which would clean this up a little bit. https://codereview.chromium.org/2116613002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/content_script_apitest.cc:349: test_data_dir_.AppendASCII("content_scripts/duplicate_script_injection"))); the indentation here is off (feel free to just run git cl format, which should catch these). https://codereview.chromium.org/2116613002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/content_script_apitest.cc:353: bool scripts_injected_once; All variables should be initialized at creation. https://codereview.chromium.org/2116613002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json (right): https://codereview.chromium.org/2116613002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json:22: "permissions": [ Do we need these permissions? https://codereview.chromium.org/2116613002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/test.css (right): https://codereview.chromium.org/2116613002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/test.css:1: /* Copyright 2016 The Chromium Authors. All rights reserved. Do we need the css injection at all? https://codereview.chromium.org/2116613002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/test.js (right): https://codereview.chromium.org/2116613002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/test.js:7: document.querySelector("body").appendChild(el); nit: prefer single-quotes in js. https://codereview.chromium.org/2116613002/diff/20001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2116613002/diff/20001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:213: GURL scriptUrl = iter->url(); variable names should_be_underscored https://codereview.chromium.org/2116613002/diff/20001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:213: GURL scriptUrl = iter->url(); This creates a copy of GURL - prefer const &. https://codereview.chromium.org/2116613002/diff/20001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:215: if(scripts_run_info->injected_scripts.find(scriptUrl) space between 'if' and '('. Also, since injected_scripts is just a std::set, I think injected_scripts.count(script_url) == 0 is mildly clearer. It might also be worth inverting the logic in the if and making it if (...) continue; to avoid the extra indentation of everything else.
Addressed comments in code review. https://codereview.chromium.org/2116613002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/content_script_apitest.cc (right): https://codereview.chromium.org/2116613002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/content_script_apitest.cc:338: IN_PROC_BROWSER_TEST_F(ExtensionApiTest, On 2016/07/11 23:45:50, Devlin wrote: > Though we don't do it uniformly, I think a comment at the top of the test (or > comments sprinkled throughout, but that's probably overkill here) is useful for > quickly understanding the purpose. So in this case, just something like: > // Test that a script that matches two separate match patterns is only > // injected once. Thanks Devlin. I like the comment you've written. :) I will use that here. https://codereview.chromium.org/2116613002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/content_script_apitest.cc:338: IN_PROC_BROWSER_TEST_F(ExtensionApiTest, On 2016/07/11 23:45:50, Devlin wrote: > Though we don't do it uniformly, I think a comment at the top of the test (or > comments sprinkled throughout, but that's probably overkill here) is useful for > quickly understanding the purpose. So in this case, just something like: > // Test that a script that matches two separate match patterns is only > // injected once. Done. https://codereview.chromium.org/2116613002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/content_script_apitest.cc:344: GURL url = GURL("http://maps.google.com:" On 2016/07/11 23:45:50, Devlin wrote: > We have handy utility called base::StringPrintf, which would clean this up a > little bit. Done. https://codereview.chromium.org/2116613002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/content_script_apitest.cc:349: test_data_dir_.AppendASCII("content_scripts/duplicate_script_injection"))); On 2016/07/11 23:45:50, Devlin wrote: > the indentation here is off (feel free to just run git cl format, which should > catch these). Done. https://codereview.chromium.org/2116613002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/content_script_apitest.cc:353: bool scripts_injected_once; On 2016/07/11 23:45:50, Devlin wrote: > All variables should be initialized at creation. Many variables in this file are left uninitialized, so I was following that style. Though I'll it a practice to initialize variables at creation. https://codereview.chromium.org/2116613002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/content_script_apitest.cc:353: bool scripts_injected_once; On 2016/07/11 23:45:50, Devlin wrote: > All variables should be initialized at creation. Done. https://codereview.chromium.org/2116613002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json (right): https://codereview.chromium.org/2116613002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json:22: "permissions": [ On 2016/07/11 23:45:50, Devlin wrote: > Do we need these permissions? Nope. Removing them. https://codereview.chromium.org/2116613002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json:22: "permissions": [ On 2016/07/11 23:45:50, Devlin wrote: > Do we need these permissions? Done. https://codereview.chromium.org/2116613002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/test.css (right): https://codereview.chromium.org/2116613002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/test.css:1: /* Copyright 2016 The Chromium Authors. All rights reserved. On 2016/07/11 23:45:50, Devlin wrote: > Do we need the css injection at all? Nope. Deleted this file. https://codereview.chromium.org/2116613002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/test.css:1: /* Copyright 2016 The Chromium Authors. All rights reserved. On 2016/07/11 23:45:50, Devlin wrote: > Do we need the css injection at all? Done. https://codereview.chromium.org/2116613002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/test.js (right): https://codereview.chromium.org/2116613002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/test.js:7: document.querySelector("body").appendChild(el); On 2016/07/11 23:45:50, Devlin wrote: > nit: prefer single-quotes in js. Done. https://codereview.chromium.org/2116613002/diff/20001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2116613002/diff/20001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:213: GURL scriptUrl = iter->url(); On 2016/07/11 23:45:50, Devlin wrote: > variable names should_be_underscored Done. https://codereview.chromium.org/2116613002/diff/20001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:213: GURL scriptUrl = iter->url(); On 2016/07/11 23:45:50, Devlin wrote: > This creates a copy of GURL - prefer const &. Done. https://codereview.chromium.org/2116613002/diff/20001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:215: if(scripts_run_info->injected_scripts.find(scriptUrl) On 2016/07/11 23:45:50, Devlin wrote: > space between 'if' and '('. Also, since injected_scripts is just a std::set, I > think injected_scripts.count(script_url) == 0 is mildly clearer. > > It might also be worth inverting the logic in the if and making it > if (...) > continue; > > to avoid the extra indentation of everything else. Done.
A few more comments, but looking pretty good! Also, I left a couple of meta-comments last round; mind addressing those? https://codereview.chromium.org/2116613002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/content_script_apitest.cc (right): https://codereview.chromium.org/2116613002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/content_script_apitest.cc:353: bool scripts_injected_once; On 2016/07/12 22:23:26, catmullings wrote: > On 2016/07/11 23:45:50, Devlin wrote: > > All variables should be initialized at creation. > > Many variables in this file are left uninitialized, so I was following that > style. > Though I'll it a practice to initialize variables at creation. I can't find any examples of uninitialized variables from a quick skim, but if there are some, they should be fixed. :) Note that this is only relevant for POD variables, since anything with a CTOR is automatically initialized via RAII. https://codereview.chromium.org/2116613002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/content_script_apitest.cc (right): https://codereview.chromium.org/2116613002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/content_script_apitest.cc:358: "document.getElementsByClassName('test-duplicate-script-injection')" hmm... git cl format seemed to miss this. I'll take a look with you when we have a moment. https://codereview.chromium.org/2116613002/diff/40001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json (right): https://codereview.chromium.org/2116613002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json:12: "css": ["test.css"], Remove this line. https://codereview.chromium.org/2116613002/diff/40001/extensions/renderer/scr... File extensions/renderer/scripts_run_info.h (right): https://codereview.chromium.org/2116613002/diff/40001/extensions/renderer/scr... extensions/renderer/scripts_run_info.h:43: // A list of pathnames of executing scripts nit: comment needs a period. Also, I'd probably replace pathnames with "extension urls". https://codereview.chromium.org/2116613002/diff/40001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2116613002/diff/40001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:215: if (scripts_run_info->injected_scripts.cout() != 0) cout? ;) Don't forget to recompile (and maybe re-run tests locally or kick off trybots) for each new patch, since it saves you a little bit of time from having to switch back to the branch a bit less. https://codereview.chromium.org/2116613002/diff/40001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:254: GURL scriptUrl = iter->url(); variable naming style.
Description was changed from ========== Prevent duplicate content script injection defined in manifest.json Modified ScriptRunInfo by adding an injected_scripts member, representing a set of js and css script GURLs executing in a page. Methods InjectJs(),revent duplicate content script injection defined in manifest.json Methods UserScriptInjector::GetJsSources() and UserScriptInjector::GetCssSources() are further modified such that scripts are only (later) injected if they are not in the ScriptRunInfo's injected_scripts. BUG=248627 TEST= Run target browser_tests with --gtest_filter="ExtensionApiTest.ContentScript*" ========== to ========== Keeps track of scripts injected by extensions installed. Checks if a script is already injected, and only injects a script only if it has not been injected. BUG=248627 TEST= Run target browser_tests with --gtest_filter="ExtensionApiTest.ContentScript*" ==========
Description was changed from ========== Keeps track of scripts injected by extensions installed. Checks if a script is already injected, and only injects a script only if it has not been injected. BUG=248627 TEST= Run target browser_tests with --gtest_filter="ExtensionApiTest.ContentScript*" ========== to ========== Keeps track of scripts injected by extensions installed. Checks if a script is already injected, and injects a script only if it has not been injected. BUG=248627 TEST= Run target browser_tests with --gtest_filter="ExtensionApiTest.ContentScript*" ==========
On 2016/07/12 23:32:36, Devlin wrote: > A few more comments, but looking pretty good! Also, I left a couple of > meta-comments last round; mind addressing those? > > https://codereview.chromium.org/2116613002/diff/20001/chrome/browser/extensio... > File chrome/browser/extensions/content_script_apitest.cc (right): > > https://codereview.chromium.org/2116613002/diff/20001/chrome/browser/extensio... > chrome/browser/extensions/content_script_apitest.cc:353: bool > scripts_injected_once; > On 2016/07/12 22:23:26, catmullings wrote: > > On 2016/07/11 23:45:50, Devlin wrote: > > > All variables should be initialized at creation. > > > > Many variables in this file are left uninitialized, so I was following that > > style. > > Though I'll it a practice to initialize variables at creation. > > I can't find any examples of uninitialized variables from a quick skim, but if > there are some, they should be fixed. :) > > Note that this is only relevant for POD variables, since anything with a CTOR is > automatically initialized via RAII. > > https://codereview.chromium.org/2116613002/diff/40001/chrome/browser/extensio... > File chrome/browser/extensions/content_script_apitest.cc (right): > > https://codereview.chromium.org/2116613002/diff/40001/chrome/browser/extensio... > chrome/browser/extensions/content_script_apitest.cc:358: > "document.getElementsByClassName('test-duplicate-script-injection')" > hmm... git cl format seemed to miss this. I'll take a look with you when we > have a moment. > > https://codereview.chromium.org/2116613002/diff/40001/chrome/test/data/extens... > File > chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json > (right): > > https://codereview.chromium.org/2116613002/diff/40001/chrome/test/data/extens... > chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json:12: > "css": ["test.css"], > Remove this line. > > https://codereview.chromium.org/2116613002/diff/40001/extensions/renderer/scr... > File extensions/renderer/scripts_run_info.h (right): > > https://codereview.chromium.org/2116613002/diff/40001/extensions/renderer/scr... > extensions/renderer/scripts_run_info.h:43: // A list of pathnames of executing > scripts > nit: comment needs a period. Also, I'd probably replace pathnames with > "extension urls". > > https://codereview.chromium.org/2116613002/diff/40001/extensions/renderer/use... > File extensions/renderer/user_script_injector.cc (right): > > https://codereview.chromium.org/2116613002/diff/40001/extensions/renderer/use... > extensions/renderer/user_script_injector.cc:215: if > (scripts_run_info->injected_scripts.cout() != 0) > cout? ;) Don't forget to recompile (and maybe re-run tests locally or kick off > trybots) for each new patch, since it saves you a little bit of time from having > to switch back to the branch a bit less. > > https://codereview.chromium.org/2116613002/diff/40001/extensions/renderer/use... > extensions/renderer/user_script_injector.cc:254: GURL scriptUrl = iter->url(); > variable naming style. Updated the high-level description of the patch. Let me know if this is still too specific, and I can try to be more general. Even w/o a description, I feel that this patch's title says it all. Still working on addressing your other comments.
Addressed code review comments on patch 3. I'm adding trybots on this patch. https://codereview.chromium.org/2116613002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/content_script_apitest.cc (right): https://codereview.chromium.org/2116613002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/content_script_apitest.cc:358: "document.getElementsByClassName('test-duplicate-script-injection')" On 2016/07/12 23:32:36, Devlin wrote: > hmm... git cl format seemed to miss this. I'll take a look with you when we > have a moment. As discussed, this is the format chosen by git cl format, so keeping it as is. https://codereview.chromium.org/2116613002/diff/40001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json (right): https://codereview.chromium.org/2116613002/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json:12: "css": ["test.css"], On 2016/07/12 23:32:36, Devlin wrote: > Remove this line. Done. https://codereview.chromium.org/2116613002/diff/40001/extensions/renderer/scr... File extensions/renderer/scripts_run_info.h (right): https://codereview.chromium.org/2116613002/diff/40001/extensions/renderer/scr... extensions/renderer/scripts_run_info.h:43: // A list of pathnames of executing scripts On 2016/07/12 23:32:36, Devlin wrote: > nit: comment needs a period. Also, I'd probably replace pathnames with > "extension urls". Done. https://codereview.chromium.org/2116613002/diff/40001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2116613002/diff/40001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:215: if (scripts_run_info->injected_scripts.cout() != 0) On 2016/07/12 23:32:36, Devlin wrote: > cout? ;) Don't forget to recompile (and maybe re-run tests locally or kick off > trybots) for each new patch, since it saves you a little bit of time from having > to switch back to the branch a bit less. Done. https://codereview.chromium.org/2116613002/diff/40001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:215: if (scripts_run_info->injected_scripts.cout() != 0) On 2016/07/12 23:32:36, Devlin wrote: > cout? ;) Don't forget to recompile (and maybe re-run tests locally or kick off > trybots) for each new patch, since it saves you a little bit of time from having > to switch back to the branch a bit less. wow, thanks for the catch! Properly updated this change, re-compiled, and re-ran tests.
https://codereview.chromium.org/2116613002/diff/40001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2116613002/diff/40001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:254: GURL scriptUrl = iter->url(); On 2016/07/12 23:32:36, Devlin wrote: > variable naming style. Looks like you missed this one? (Also please make this a const &.) https://codereview.chromium.org/2116613002/diff/60001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2116613002/diff/60001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:258: std::string content = iter->GetContent().as_string(); is this variable ever used?
Addressed code review comments from patch 4 https://codereview.chromium.org/2116613002/diff/40001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2116613002/diff/40001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:254: GURL scriptUrl = iter->url(); On 2016/07/13 18:58:36, Devlin wrote: > On 2016/07/12 23:32:36, Devlin wrote: > > variable naming style. > > Looks like you missed this one? (Also please make this a const &.) Done. https://codereview.chromium.org/2116613002/diff/60001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2116613002/diff/60001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:258: std::string content = iter->GetContent().as_string(); On 2016/07/13 18:58:36, Devlin wrote: > is this variable ever used? Nope. I had set it up to pass it as a parameter to the following .push_back() call, but I ended up doing it inline below. https://codereview.chromium.org/2116613002/diff/60001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:258: std::string content = iter->GetContent().as_string(); On 2016/07/13 18:58:36, Devlin wrote: > is this variable ever used? Done.
https://codereview.chromium.org/2116613002/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json (right): https://codereview.chromium.org/2116613002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json:6: "background": { nitty nit: "background": {}, (i.e., open and close brace on one line) https://codereview.chromium.org/2116613002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json:18: } I wonder if we should make this a little more intense by having another script that is deliberately injected twice because it's injected at different times in the load process. e.g. adding a rule { "matches": ["*://maps.google.com/*"], "js": ["should_run_twice.js"], "run_at": "document_start" } and then add the "should_run_twice.js" entry to either of the rules above. WDYT? https://codereview.chromium.org/2116613002/diff/80001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2116613002/diff/80001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:255: // Check if the script is already injected nitty nit: technically, this isn't a script, it's a stylesheet.
Addressed patch 5 code review https://codereview.chromium.org/2116613002/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json (right): https://codereview.chromium.org/2116613002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json:6: "background": { On 2016/07/15 22:43:26, Devlin wrote: > nitty nit: > "background": {}, > > (i.e., open and close brace on one line) Done. https://codereview.chromium.org/2116613002/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json:18: } On 2016/07/15 22:43:27, Devlin wrote: > I wonder if we should make this a little more intense by having another script > that is deliberately injected twice because it's injected at different times in > the load process. e.g. adding a rule > { > "matches": ["*://maps.google.com/*"], > "js": ["should_run_twice.js"], > "run_at": "document_start" > } > and then add the "should_run_twice.js" entry to either of the rules above. > > WDYT? Done. https://codereview.chromium.org/2116613002/diff/80001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2116613002/diff/80001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:255: // Check if the script is already injected On 2016/07/15 22:43:27, Devlin wrote: > nitty nit: technically, this isn't a script, it's a stylesheet. Done.
Nice! Just a few last nits https://codereview.chromium.org/2116613002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/content_script_apitest.cc (right): https://codereview.chromium.org/2116613002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/content_script_apitest.cc:345: GURL url = GURL( nit: no reason for assignment instead of just construction here. i.e. GURL url(base::StringPrintf(...)) https://codereview.chromium.org/2116613002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/content_script_apitest.cc:354: bool scripts_injected_once = false; these variables are somewhat opaque from the test alone, and it's not clear why we expect 1 vs 2. I'd suggest adding a few comments to explain what's going on here so the reader doesn't need to bounce to the content script. https://codereview.chromium.org/2116613002/diff/120001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json (right): https://codereview.chromium.org/2116613002/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json:15: "js": ["test.js", "should_run_twice.js"], let's have parity between these script names - so should_run_once and should_run_twice or something. https://codereview.chromium.org/2116613002/diff/120001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/should_run_twice.js (right): https://codereview.chromium.org/2116613002/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/should_run_twice.js:6: elem.className = 'injected-twice'; here, too, let's have the divs have parity and mirror the script names. https://codereview.chromium.org/2116613002/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/should_run_twice.js:7: document.querySelector("body").appendChild(elem); prefer single quotes in js
Addressed code review comments from patch 7 https://codereview.chromium.org/2116613002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/content_script_apitest.cc (right): https://codereview.chromium.org/2116613002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/content_script_apitest.cc:345: GURL url = GURL( On 2016/07/19 16:09:00, Devlin wrote: > nit: no reason for assignment instead of just construction here. i.e. > GURL url(base::StringPrintf(...)) Done. https://codereview.chromium.org/2116613002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/content_script_apitest.cc:354: bool scripts_injected_once = false; On 2016/07/19 16:09:00, Devlin wrote: > these variables are somewhat opaque from the test alone, and it's not clear why > we expect 1 vs 2. I'd suggest adding a few comments to explain what's going on > here so the reader doesn't need to bounce to the content script. Moved the comment describing the entire TEST_F to the sub-test above and added a comment for this new sub-test. Done. https://codereview.chromium.org/2116613002/diff/120001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json (right): https://codereview.chromium.org/2116613002/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json:15: "js": ["test.js", "should_run_twice.js"], On 2016/07/19 16:09:00, Devlin wrote: > let's have parity between these script names - so should_run_once and > should_run_twice or something. Done. https://codereview.chromium.org/2116613002/diff/120001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/should_run_twice.js (right): https://codereview.chromium.org/2116613002/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/should_run_twice.js:6: elem.className = 'injected-twice'; On 2016/07/19 16:09:00, Devlin wrote: > here, too, let's have the divs have parity and mirror the script names. Done. https://codereview.chromium.org/2116613002/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/should_run_twice.js:7: document.querySelector("body").appendChild(elem); On 2016/07/19 16:09:00, Devlin wrote: > prefer single quotes in js Done.
lgtm with one last round of nits. https://codereview.chromium.org/2116613002/diff/140001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/should_run_once.js (right): https://codereview.chromium.org/2116613002/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/should_run_once.js:7: document.querySelector('body').appendChild(should_be_injected_once); sorry, don't know how I missed this before, but this should just be document.body rather than document.querySelector('body'). https://codereview.chromium.org/2116613002/diff/140001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/should_run_twice.js (right): https://codereview.chromium.org/2116613002/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/should_run_twice.js:7: document.querySelector('body').appendChild(should_be_injected_twice); ditto https://codereview.chromium.org/2116613002/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/should_run_twice.js:10: delete extra blank lines https://codereview.chromium.org/2116613002/diff/140001/extensions/renderer/pr... File extensions/renderer/programmatic_script_injector.h (right): https://codereview.chromium.org/2116613002/diff/140001/extensions/renderer/pr... extensions/renderer/programmatic_script_injector.h:45: delete this blank line https://codereview.chromium.org/2116613002/diff/140001/extensions/renderer/us... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2116613002/diff/140001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:255: // Check if the stylesheet is already injected Comments should end in a '.'.
Addressed comments in patch 8 https://codereview.chromium.org/2116613002/diff/140001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/should_run_once.js (right): https://codereview.chromium.org/2116613002/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/should_run_once.js:7: document.querySelector('body').appendChild(should_be_injected_once); On 2016/07/19 23:10:36, Devlin wrote: > sorry, don't know how I missed this before, but this should just be > document.body rather than document.querySelector('body'). Done. https://codereview.chromium.org/2116613002/diff/140001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/should_run_twice.js (right): https://codereview.chromium.org/2116613002/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/should_run_twice.js:7: document.querySelector('body').appendChild(should_be_injected_twice); On 2016/07/19 23:10:36, Devlin wrote: > ditto Done. https://codereview.chromium.org/2116613002/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/should_run_twice.js:10: On 2016/07/19 23:10:36, Devlin wrote: > delete extra blank lines Done. https://codereview.chromium.org/2116613002/diff/140001/extensions/renderer/pr... File extensions/renderer/programmatic_script_injector.h (right): https://codereview.chromium.org/2116613002/diff/140001/extensions/renderer/pr... extensions/renderer/programmatic_script_injector.h:45: On 2016/07/19 23:10:36, Devlin wrote: > delete this blank line Done. https://codereview.chromium.org/2116613002/diff/140001/extensions/renderer/us... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2116613002/diff/140001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:255: // Check if the stylesheet is already injected On 2016/07/19 23:10:36, Devlin wrote: > Comments should end in a '.'. Done.
The CQ bit was checked by catmullings@chromium.org
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/2116613002/#ps160001 (title: "Addressed code review on patch 8")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Keeps track of scripts injected by extensions installed. Checks if a script is already injected, and injects a script only if it has not been injected. BUG=248627 TEST= Run target browser_tests with --gtest_filter="ExtensionApiTest.ContentScript*" ========== to ========== Keeps track of scripts injected by extensions installed. Checks if a script is already injected, and injects a script only if it has not been injected. BUG=248627 TEST= Run target browser_tests with --gtest_filter="ExtensionApiTest.ContentScript*" ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Keeps track of scripts injected by extensions installed. Checks if a script is already injected, and injects a script only if it has not been injected. BUG=248627 TEST= Run target browser_tests with --gtest_filter="ExtensionApiTest.ContentScript*" ========== to ========== Keeps track of scripts injected by extensions installed. Checks if a script is already injected, and injects a script only if it has not been injected. BUG=248627 TEST= Run target browser_tests with --gtest_filter="ExtensionApiTest.ContentScript*" Committed: https://crrev.com/90d8dd145246616da02f5a54f1da1f5bfe6680e9 Cr-Commit-Position: refs/heads/master@{#407016} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/90d8dd145246616da02f5a54f1da1f5bfe6680e9 Cr-Commit-Position: refs/heads/master@{#407016}
Message was sent while issue was closed.
There's a renderer crash on chromeos (http://crbug.com/631247) that I've bisected to this CL.
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.chromium.org/2206463002/ by rdevlin.cronin@chromium.org. The reason for reverting is: Causes a renderer crash - http://crbug.com/631247.
Message was sent while issue was closed.
Description was changed from ========== Keeps track of scripts injected by extensions installed. Checks if a script is already injected, and injects a script only if it has not been injected. BUG=248627 TEST= Run target browser_tests with --gtest_filter="ExtensionApiTest.ContentScript*" Committed: https://crrev.com/90d8dd145246616da02f5a54f1da1f5bfe6680e9 Cr-Commit-Position: refs/heads/master@{#407016} ========== to ========== Keeps track of scripts injected by extensions installed. Checks if a script is already injected, and injects a script only if it has not been injected. BUG=248627 TEST= Run target browser_tests with --gtest_filter="ExtensionApiTest.ContentScript*" Committed: https://crrev.com/90d8dd145246616da02f5a54f1da1f5bfe6680e9 Cr-Commit-Position: refs/heads/master@{#407016} ==========
Patchset #10 (id:180001) has been deleted
Patchset #10 (id:200001) has been deleted |
