Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
492133002:
Renderer changes for wiring up shared memory with declarative injection (Closed)
|
Created:
6 years, 4 months ago by Mark Dittmer Modified:
6 years, 3 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, extensions-reviews_chromium.org, chromium-apps-reviews_chromium.org, not at google - send to devlin, Jeffrey Yasskin Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionRenderer changes for wiring up shared memory with declarative injection
BUG=377978
Committed: https://crrev.com/9ea140f4f6a694b3a65461f4ac41daf94dc2c59c
Cr-Commit-Position: refs/heads/master@{#292558}
Patch Set 1 #
Total comments: 14
Patch Set 2 : IGNORE PATCH (diff against wrong branch) #Patch Set 3 : Address first round of review comments #Patch Set 4 : Add browsertests #
Total comments: 30
Patch Set 5 : Fix tests, revert mistaken changes, address nits #
Total comments: 29
Patch Set 6 : Double-check and/or address missed code review comments #Patch Set 7 : Fixed up tests and responded to other comments #
Total comments: 21
Patch Set 8 : Fix up API tests, int64s, and check URL before declarative injection #
Total comments: 12
Patch Set 9 : Fix final nits #Messages
Total messages: 24 (0 generated)
Hey Devlin, here are the IPC + renderer changes for wiring up RequestContentScript to actual script injection, including IPC message send on the browser side. This CL depends on https://codereview.chromium.org/493633003/
https://codereview.chromium.org/492133002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/492133002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative_content/content_action.cc:261: contents->GetRenderProcessHost()->Send( Why not send this to the RenderViewHost so you don't have to look up the tab? https://codereview.chromium.org/492133002/diff/1/extensions/renderer/programm... File extensions/renderer/programmatic_script_injector.h (right): https://codereview.chromium.org/492133002/diff/1/extensions/renderer/programm... extensions/renderer/programmatic_script_injector.h:34: virtual bool is_declarative() const OVERRIDE; Why? https://codereview.chromium.org/492133002/diff/1/extensions/renderer/script_i... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/492133002/diff/1/extensions/renderer/script_i... extensions/renderer/script_injection_manager.cc:353: user_script_set_manager_->GetInjectionForScript( Since this only ever does declarative script injections, let's make the method say that and not need to pass in BROWSER_DRIVEN. https://codereview.chromium.org/492133002/diff/1/extensions/renderer/script_i... File extensions/renderer/script_injector.h (right): https://codereview.chromium.org/492133002/diff/1/extensions/renderer/script_i... extensions/renderer/script_injector.h:44: virtual bool is_declarative() const = 0; Why? https://codereview.chromium.org/492133002/diff/1/extensions/renderer/user_scr... File extensions/renderer/user_script_injector.h (right): https://codereview.chromium.org/492133002/diff/1/extensions/renderer/user_scr... extensions/renderer/user_script_injector.h:33: virtual bool is_declarative() const OVERRIDE; why? https://codereview.chromium.org/492133002/diff/1/extensions/renderer/user_scr... File extensions/renderer/user_script_set.h (right): https://codereview.chromium.org/492133002/diff/1/extensions/renderer/user_scr... extensions/renderer/user_script_set.h:19: #include "extensions/renderer/script_injection.h" Why include the .h? https://codereview.chromium.org/492133002/diff/1/extensions/renderer/user_scr... File extensions/renderer/user_script_set_manager.h (right): https://codereview.chromium.org/492133002/diff/1/extensions/renderer/user_scr... extensions/renderer/user_script_set_manager.h:59: scoped_ptr<ScriptInjection> GetInjectionForScript( comments https://codereview.chromium.org/492133002/diff/1/extensions/renderer/user_scr... extensions/renderer/user_script_set_manager.h:81: // A RenderViewObserver implementation which watches the various render views Why?
And if this one is the last CL for this part (I think it is?) can we add a few good tests?
Adding Ken for owners review of IPC messages. Addressed first round of comments (no tests yet though). https://codereview.chromium.org/492133002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/492133002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/declarative_content/content_action.cc:261: contents->GetRenderProcessHost()->Send( On 2014/08/21 17:06:16, Devlin wrote: > Why not send this to the RenderViewHost so you don't have to look up the tab? Done. https://codereview.chromium.org/492133002/diff/1/extensions/renderer/programm... File extensions/renderer/programmatic_script_injector.h (right): https://codereview.chromium.org/492133002/diff/1/extensions/renderer/programm... extensions/renderer/programmatic_script_injector.h:34: virtual bool is_declarative() const OVERRIDE; On 2014/08/21 17:06:16, Devlin wrote: > Why? Left over from some debugging. I'll get rid of it. https://codereview.chromium.org/492133002/diff/1/extensions/renderer/script_i... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/492133002/diff/1/extensions/renderer/script_i... extensions/renderer/script_injection_manager.cc:353: user_script_set_manager_->GetInjectionForScript( On 2014/08/21 17:06:16, Devlin wrote: > Since this only ever does declarative script injections, let's make the method > say that and not need to pass in BROWSER_DRIVEN. Done. https://codereview.chromium.org/492133002/diff/1/extensions/renderer/script_i... File extensions/renderer/script_injector.h (right): https://codereview.chromium.org/492133002/diff/1/extensions/renderer/script_i... extensions/renderer/script_injector.h:44: virtual bool is_declarative() const = 0; On 2014/08/21 17:06:16, Devlin wrote: > Why? ditto (comment in programmatic_script_injector.h) https://codereview.chromium.org/492133002/diff/1/extensions/renderer/user_scr... File extensions/renderer/user_script_injector.h (right): https://codereview.chromium.org/492133002/diff/1/extensions/renderer/user_scr... extensions/renderer/user_script_injector.h:33: virtual bool is_declarative() const OVERRIDE; On 2014/08/21 17:06:17, Devlin wrote: > why? ditto (comment in programmatic_script_injector.h) https://codereview.chromium.org/492133002/diff/1/extensions/renderer/user_scr... File extensions/renderer/user_script_set_manager.h (right): https://codereview.chromium.org/492133002/diff/1/extensions/renderer/user_scr... extensions/renderer/user_script_set_manager.h:81: // A RenderViewObserver implementation which watches the various render views On 2014/08/21 17:06:17, Devlin wrote: > Why? More leftovers from a failed/unnecessary pattern I tried. Removed.
On 2014/08/21 17:12:38, Devlin wrote: > And if this one is the last CL for this part (I think it is?) can we add a few > good tests? Maybe we should chat about the best way to go about this. It looks like the affected extensions/renderer components don't have any tests for them yet.
On 2014/08/23 17:03:44, Mark Dittmer wrote: > On 2014/08/21 17:12:38, Devlin wrote: > > And if this one is the last CL for this part (I think it is?) can we add a few > > good tests? > > Maybe we should chat about the best way to go about this. It looks like the > affected extensions/renderer components don't have any tests for them yet. Happy to chat, of course, but here's a bit more info: We don't really have tests for almost anything in extensions/renderer (Why? Good question. Probably because mocking up anything in content/renderer is a right pain, and since we have next-to-zero infrastructure for renderer testing, it's very difficult to start now). Instead, we normally test renderer concepts via end-to-end browsertests, which is why I only mention that it should happen in this CL if this one is the last in the series. So I would expect a test for this stuff to do (at a high level): install an extension that has declarative script requests ensure that the scripts ran repeat in various configurations While it is unfortunately not as precise as other tests we have (it's susceptible to failure in both browser/ and renderer/, if it passes, it should guarantee the renderer is working.
Rebased from new revision of browser-side CL and added browsertests.
https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/content_action.cc:21: #include "content/public/browser/render_process_host.h" still need this? https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc (right): https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:94: bool RunAllPendingInRenderer(content::WebContents* web_contents) { // TODO(devlin): If too many tests start to need this, move it somewhere common. https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:142: permission = kAllPermission; How about making a const char* kPermissions[] { "all", "particular", "nowhere" } and enum ManifestPermissionType { ALL = 0, PARTICULAR, NOWHERE } and then say const char* permission = kPermissions[manifest_permission]? https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:153: const char* matcher; ditto for matcher. Although if ManifestPermissionType and ScriptMatcherType are always going to be the same set, could just say something like MatchType and share them. https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:205: Since we never ever ever expect our Injection Setup listener to fail, it might actually be better to put a WaitUntilSatisified call on it here. Right now, it looks like there's a chance that we might perform RunAllPendingInRenderer() *before* the script injection is posted (since there's no guarantee we've requested injection yet). https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:219: EXPECT_TRUE(RunTest()); This is a *lot* of browsertests. Unfortunately, browser tests are expensive. A lot of that expense, though, is actually just in the set up/tear down of the tests. It looks like we could pretty much just make this *all* one test, and have a RunTest(ManifestPermission, ScriptMatcher) method that loads the extensions, makes the observers, and verifies it runs, and then we can just have a nice long list of EXPECT_TRUE(RunTest(MP_ALL, SM_ALL)) EXPECT_TRUE(RunTest(MP_ALL, SM_PARTICULAR)) etc. https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... File chrome/browser/extensions/test_extension_system.h (right): https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... chrome/browser/extensions/test_extension_system.h:90: LOG(INFO) << "SetReady()"; diediedie https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... File chrome/browser/extensions/user_script_loader.cc (left): https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... chrome/browser/extensions/user_script_loader.cc:350: clear_scripts_(false), livelivelive https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... File chrome/browser/extensions/user_script_loader_unittest.cc (right): https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... chrome/browser/extensions/user_script_loader_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. this too https://codereview.chromium.org/492133002/diff/60001/extensions/common/extens... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/492133002/diff/60001/extensions/common/extens... extensions/common/extension_messages.h:413: int64 /* script identifier */, Didn't we make this *not* an int64? https://codereview.chromium.org/492133002/diff/60001/extensions/renderer/user... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/492133002/diff/60001/extensions/renderer/user... extensions/renderer/user_script_injector.cc:138: // manifest) whereas non-declarative scripts use custom nit: "user scripts" (not to be mistaken with, e.g., chrome.tabs.executeScript(), which again uses page access) https://codereview.chromium.org/492133002/diff/60001/extensions/renderer/user... File extensions/renderer/user_script_set.cc (right): https://codereview.chromium.org/492133002/diff/60001/extensions/renderer/user... extensions/renderer/user_script_set.cc:169: true /* is_declarative */); We should either pass this in as a param or rename the method. My (slight) preference is for the latter. Something like "GetDeclarativeScriptInjection()" is fine - we don't really need the "ById" here https://codereview.chromium.org/492133002/diff/60001/extensions/renderer/user... File extensions/renderer/user_script_set.h (right): https://codereview.chromium.org/492133002/diff/60001/extensions/renderer/user... extensions/renderer/user_script_set.h:19: #include "extensions/renderer/script_injection.h" Don't need this, right? https://codereview.chromium.org/492133002/diff/60001/extensions/renderer/user... File extensions/renderer/user_script_set_manager.cc (left): https://codereview.chromium.org/492133002/diff/60001/extensions/renderer/user... extensions/renderer/user_script_set_manager.cc:97: CHECK(changed_extensions.size() == 1 && We can still have a check that the size is <= 1, right? https://codereview.chromium.org/492133002/diff/60001/extensions/renderer/user... File extensions/renderer/user_script_set_manager.h (right): https://codereview.chromium.org/492133002/diff/60001/extensions/renderer/user... extensions/renderer/user_script_set_manager.h:59: scoped_ptr<ScriptInjection> GetInjectionForDeclarativeScript( function comments
Addressed latest round of comments, including refactoring browser tests code. https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/content_action.cc:21: #include "content/public/browser/render_process_host.h" On 2014/08/25 22:31:05, Devlin wrote: > still need this? Nope. Removed. https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc (right): https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:94: bool RunAllPendingInRenderer(content::WebContents* web_contents) { On 2014/08/25 22:31:05, Devlin wrote: > // TODO(devlin): If too many tests start to need this, move it somewhere common. Done. https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:153: const char* matcher; On 2014/08/25 22:31:05, Devlin wrote: > ditto for matcher. Although if ManifestPermissionType and ScriptMatcherType are > always going to be the same set, could just say something like MatchType and > share them. I was thinking that they may not remain the same set in the future. https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:205: On 2014/08/25 22:31:05, Devlin wrote: > Since we never ever ever expect our Injection Setup listener to fail, it might > actually be better to put a WaitUntilSatisified call on it here. Right now, it > looks like there's a chance that we might perform RunAllPendingInRenderer() > *before* the script injection is posted (since there's no guarantee we've > requested injection yet). Done. https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... File chrome/browser/extensions/test_extension_system.h (right): https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... chrome/browser/extensions/test_extension_system.h:90: LOG(INFO) << "SetReady()"; On 2014/08/25 22:31:06, Devlin wrote: > diediedie Done. https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... File chrome/browser/extensions/user_script_loader.cc (left): https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... chrome/browser/extensions/user_script_loader.cc:350: clear_scripts_(false), On 2014/08/25 22:31:06, Devlin wrote: > livelivelive Done. https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... File chrome/browser/extensions/user_script_loader_unittest.cc (right): https://codereview.chromium.org/492133002/diff/60001/chrome/browser/extension... chrome/browser/extensions/user_script_loader_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/08/25 22:31:06, Devlin wrote: > this too Done. https://codereview.chromium.org/492133002/diff/60001/extensions/common/extens... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/492133002/diff/60001/extensions/common/extens... extensions/common/extension_messages.h:413: int64 /* script identifier */, On 2014/08/25 22:31:06, Devlin wrote: > Didn't we make this *not* an int64? Done. https://codereview.chromium.org/492133002/diff/60001/extensions/renderer/user... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/492133002/diff/60001/extensions/renderer/user... extensions/renderer/user_script_injector.cc:138: // manifest) whereas non-declarative scripts use custom On 2014/08/25 22:31:06, Devlin wrote: > nit: "user scripts" (not to be mistaken with, e.g., chrome.tabs.executeScript(), > which again uses page access) Done. https://codereview.chromium.org/492133002/diff/60001/extensions/renderer/user... File extensions/renderer/user_script_set_manager.cc (left): https://codereview.chromium.org/492133002/diff/60001/extensions/renderer/user... extensions/renderer/user_script_set_manager.cc:97: CHECK(changed_extensions.size() == 1 && On 2014/08/25 22:31:06, Devlin wrote: > We can still have a check that the size is <= 1, right? Sure.
It looks like a few of the comments that were marked as fixed actually weren't... I commented on some, then realized it was a bunch, and stopped mentioning it. https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/content_action.cc:19: #include "chrome/common/extensions/manifest_handlers/content_scripts_handler.h" Do we use this? https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc (right): https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit of nits: This is actually more of an API test (even though it just inherits from ExtensionBrowserTest) https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:76: MP_ALL = 0, If this and ScriptMatcherType are always gonna be the same, make them one enum. Is there a time when they would be different? https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:108: testing::AssertionResult RunTest(ManifestPermissionType manifest_permission, indentation https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:121: scoped_ptr<ExtensionTestMessageListener> injection_setup_listener_; Why do these need to be member variables? https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:133: std::string manifest = base::StringPrintf(kManifest, Pull this part out into a CreatendLoadExtension(type, type) function. https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:149: if (extension_) nit: Cleanup _before_ creating the new stuff. https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:166: if (!embedded_test_server()->InitializeAndWaitUntilReady()) { Since this will only be done once, just do it in the first line of the TEST https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:175: We should also check that injection has *not* occurred at this point. https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:203: ASSERT_TRUE(injection_succeeded_listener()->was_satisfied()); Move this into RunTest as a third param (should_inject) https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:234: // EXPECT_TRUE(Init(MP_PARTICULAR, SM_ALL)); ?? https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/user_script_loader_unittest.cc (right): https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... chrome/browser/extensions/user_script_loader_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. ?!??!?! https://codereview.chromium.org/492133002/diff/80001/extensions/renderer/scri... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/492133002/diff/80001/extensions/renderer/scri... extensions/renderer/script_injection_manager.cc:164: blink::WebFrame* main_frame = render_view()->GetWebView()->mainFrame(); Shouldn't we somewhere check that |url| hasn't changed, since it's what we're using everywhere? Otherwise an extension could run on supersensitivesite.com instead of totallyharmless.com because of a race condition... https://codereview.chromium.org/492133002/diff/80001/extensions/renderer/user... File extensions/renderer/user_script_set.h (right): https://codereview.chromium.org/492133002/diff/80001/extensions/renderer/user... extensions/renderer/user_script_set.h:19: #include "extensions/renderer/script_injection.h" !
Double-check and/or address missed code review comments. Sorry for the confusion! https://codereview.chromium.org/492133002/diff/60001/extensions/renderer/user... File extensions/renderer/user_script_set.cc (right): https://codereview.chromium.org/492133002/diff/60001/extensions/renderer/user... extensions/renderer/user_script_set.cc:169: true /* is_declarative */); On 2014/08/25 22:31:06, Devlin wrote: > We should either pass this in as a param or rename the method. My (slight) > preference is for the latter. Something like "GetDeclarativeScriptInjection()" > is fine - we don't really need the "ById" here Done. https://codereview.chromium.org/492133002/diff/60001/extensions/renderer/user... File extensions/renderer/user_script_set.h (right): https://codereview.chromium.org/492133002/diff/60001/extensions/renderer/user... extensions/renderer/user_script_set.h:19: #include "extensions/renderer/script_injection.h" On 2014/08/25 22:31:06, Devlin wrote: > Don't need this, right? Nope. Use of ScopedVector<ScriptInjection> complains without it, even if I forward-declare it. https://codereview.chromium.org/492133002/diff/60001/extensions/renderer/user... File extensions/renderer/user_script_set_manager.h (right): https://codereview.chromium.org/492133002/diff/60001/extensions/renderer/user... extensions/renderer/user_script_set_manager.h:59: scoped_ptr<ScriptInjection> GetInjectionForDeclarativeScript( On 2014/08/25 22:31:06, Devlin wrote: > function comments Done.
Fixed up tests and responded to other comments. https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/content_action.cc:19: #include "chrome/common/extensions/manifest_handlers/content_scripts_handler.h" On 2014/08/26 21:59:15, Devlin wrote: > Do we use this? We do not. Removed. https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc (right): https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/08/26 21:59:16, Devlin wrote: > nit of nits: This is actually more of an API test (even though it just inherits > from ExtensionBrowserTest) File renamed. https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:76: MP_ALL = 0, On 2014/08/26 21:59:16, Devlin wrote: > If this and ScriptMatcherType are always gonna be the same, make them one enum. > Is there a time when they would be different? Currently, they fit into the same categorization system. Merged them. https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:108: testing::AssertionResult RunTest(ManifestPermissionType manifest_permission, On 2014/08/26 21:59:16, Devlin wrote: > indentation Done. https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:121: scoped_ptr<ExtensionTestMessageListener> injection_setup_listener_; On 2014/08/26 21:59:15, Devlin wrote: > Why do these need to be member variables? This ensures the lifetime of the listeners for use in the RunTest context, as well as the main test function context. https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:166: if (!embedded_test_server()->InitializeAndWaitUntilReady()) { On 2014/08/26 21:59:15, Devlin wrote: > Since this will only be done once, just do it in the first line of the TEST Done. https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:175: On 2014/08/26 21:59:15, Devlin wrote: > We should also check that injection has *not* occurred at this point. Why? Done, just the same. https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:203: ASSERT_TRUE(injection_succeeded_listener()->was_satisfied()); On 2014/08/26 21:59:16, Devlin wrote: > Move this into RunTest as a third param (should_inject) Done. https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:234: // EXPECT_TRUE(Init(MP_PARTICULAR, SM_ALL)); On 2014/08/26 21:59:15, Devlin wrote: > ?? Experimentation leftovers. Removed. https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/user_script_loader_unittest.cc (right): https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... chrome/browser/extensions/user_script_loader_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/08/26 21:59:16, Devlin wrote: > ?!??!?! Was already done on my end. Maybe I uploaded the wrong patch? https://codereview.chromium.org/492133002/diff/80001/extensions/renderer/scri... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/492133002/diff/80001/extensions/renderer/scri... extensions/renderer/script_injection_manager.cc:164: blink::WebFrame* main_frame = render_view()->GetWebView()->mainFrame(); I think this is handled by script_injection_manager:200, yes? https://codereview.chromium.org/492133002/diff/80001/extensions/renderer/user... File extensions/renderer/user_script_set.h (right): https://codereview.chromium.org/492133002/diff/80001/extensions/renderer/user... extensions/renderer/user_script_set.h:19: #include "extensions/renderer/script_injection.h" On 2014/08/26 21:59:16, Devlin wrote: > ! Replied to this in last round. Sorry for the confusion.
https://codereview.chromium.org/492133002/diff/60001/extensions/renderer/user... File extensions/renderer/user_script_set.h (right): https://codereview.chromium.org/492133002/diff/60001/extensions/renderer/user... extensions/renderer/user_script_set.h:19: #include "extensions/renderer/script_injection.h" On 2014/08/27 21:21:48, Mark Dittmer wrote: > On 2014/08/25 22:31:06, Devlin wrote: > > Don't need this, right? > > Nope. Use of ScopedVector<ScriptInjection> complains without it, even if I > forward-declare it. But... huh? We already use ScopedVector without the include in trunk... https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere... https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc (right): https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:121: scoped_ptr<ExtensionTestMessageListener> injection_setup_listener_; On 2014/08/27 22:37:04, Mark Dittmer wrote: > On 2014/08/26 21:59:15, Devlin wrote: > > Why do these need to be member variables? > > This ensures the lifetime of the listeners for use in the RunTest context, as > well as the main test function context. See comment on new patch set. https://codereview.chromium.org/492133002/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc (right): https://codereview.chromium.org/492133002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc:8: #include "chrome/browser/extensions/extension_action.h" don't need? https://codereview.chromium.org/492133002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc:25: "*://*/*", // ALL nit: I think these look better as "*://*/*", // ALL "http://127.0.0.1/*", // PARTICULAR "http://nowhere.com/*" // NOWHERE (note the comment vertical alignment) https://codereview.chromium.org/492133002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc:53: "var ShowPageAction = declarativeContent.ShowPageAction;\n" not used? https://codereview.chromium.org/492133002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc:75: enum PermissionOrMatcherType { nit: move this right under kPermissions/kScriptMatchers https://codereview.chromium.org/492133002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc:96: RequestContentScriptAPITest(); add a virtual destructor. https://codereview.chromium.org/492133002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc:137: injection_succeeded_listener_.reset(new ExtensionTestMessageListener( These should only be used in the context of a single function, so just make them function-scoped (not class variables). https://codereview.chromium.org/492133002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc:199: injection_setup_listener_.reset(new ExtensionTestMessageListener( see above comment on listeners. https://codereview.chromium.org/492133002/diff/120001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/492133002/diff/120001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:164: blink::WebFrame* main_frame = render_view()->GetWebView()->mainFrame(); We should a) NULL-check the main frame. b) check that the urls match. What would be even better would be to check if the page has navigated since, but that might involve a bit more infrastructure. https://codereview.chromium.org/492133002/diff/120001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:349: int64 script_id, these aren't int64s... https://codereview.chromium.org/492133002/diff/120001/extensions/renderer/use... File extensions/renderer/user_script_set_manager.h (right): https://codereview.chromium.org/492133002/diff/120001/extensions/renderer/use... extensions/renderer/user_script_set_manager.h:62: int64 script_id, not int64
Addressed all comments with one potential concern about the URL check before injection (see inline comment). It would be nice to get this into the CQ today or tomorrow, given that I'm done tomorrow. Of course, only after it LGTY. https://codereview.chromium.org/492133002/diff/60001/extensions/renderer/user... File extensions/renderer/user_script_set.h (right): https://codereview.chromium.org/492133002/diff/60001/extensions/renderer/user... extensions/renderer/user_script_set.h:19: #include "extensions/renderer/script_injection.h" On 2014/08/27 23:01:11, Devlin wrote: > On 2014/08/27 21:21:48, Mark Dittmer wrote: > > On 2014/08/25 22:31:06, Devlin wrote: > > > Don't need this, right? > > > > Nope. Use of ScopedVector<ScriptInjection> complains without it, even if I > > forward-declare it. > > But... huh? We already use ScopedVector without the include in trunk... > > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere... I was mistaken. The problem was that the .cc file was relying on the .h's include (which is allowed when they have the same name). When I migrated the include to the .cc file and added the forward declaration, it worked. https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc (right): https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:133: std::string manifest = base::StringPrintf(kManifest, On 2014/08/26 21:59:16, Devlin wrote: > Pull this part out into a CreatendLoadExtension(type, type) function. Done. https://codereview.chromium.org/492133002/diff/80001/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/request_content_script_browsertest.cc:149: if (extension_) On 2014/08/26 21:59:15, Devlin wrote: > nit: Cleanup _before_ creating the new stuff. Done. https://codereview.chromium.org/492133002/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc (right): https://codereview.chromium.org/492133002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc:8: #include "chrome/browser/extensions/extension_action.h" On 2014/08/27 23:01:11, Devlin wrote: > don't need? Done. https://codereview.chromium.org/492133002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc:25: "*://*/*", // ALL On 2014/08/27 23:01:11, Devlin wrote: > nit: I think these look better as > "*://*/*", // ALL > "http://127.0.0.1/*", // PARTICULAR > "http://nowhere.com/*" // NOWHERE > > (note the comment vertical alignment) Agreed. Done. https://codereview.chromium.org/492133002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc:53: "var ShowPageAction = declarativeContent.ShowPageAction;\n" On 2014/08/27 23:01:11, Devlin wrote: > not used? Done. https://codereview.chromium.org/492133002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc:75: enum PermissionOrMatcherType { On 2014/08/27 23:01:11, Devlin wrote: > nit: move this right under kPermissions/kScriptMatchers Done. https://codereview.chromium.org/492133002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc:96: RequestContentScriptAPITest(); On 2014/08/27 23:01:11, Devlin wrote: > add a virtual destructor. Done. https://codereview.chromium.org/492133002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc:137: injection_succeeded_listener_.reset(new ExtensionTestMessageListener( On 2014/08/27 23:01:11, Devlin wrote: > These should only be used in the context of a single function, so just make them > function-scoped (not class variables). Done. https://codereview.chromium.org/492133002/diff/120001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc:199: injection_setup_listener_.reset(new ExtensionTestMessageListener( On 2014/08/27 23:01:11, Devlin wrote: > see above comment on listeners. Done. https://codereview.chromium.org/492133002/diff/120001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/492133002/diff/120001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:164: blink::WebFrame* main_frame = render_view()->GetWebView()->mainFrame(); On 2014/08/27 23:01:11, Devlin wrote: > We should > a) NULL-check the main frame. > b) check that the urls match. What would be even better would be to check if > the page has navigated since, but that might involve a bit more infrastructure. Done. I have one potential concern, but I don't think it applies to declarative script injections. Is this injection, if it happens, guaranteed to inject immediately; i.e., before the renderer may have a chance to navigate elsewhere? If not, this check should probably go closer to the injection point. https://codereview.chromium.org/492133002/diff/120001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:349: int64 script_id, On 2014/08/27 23:01:11, Devlin wrote: > these aren't int64s... Done. Here and user_script_set_manager. https://codereview.chromium.org/492133002/diff/120001/extensions/renderer/use... File extensions/renderer/user_script_set_manager.h (right): https://codereview.chromium.org/492133002/diff/120001/extensions/renderer/use... extensions/renderer/user_script_set_manager.h:62: int64 script_id, On 2014/08/27 23:01:12, Devlin wrote: > not int64 Done.
ipc lgtm
Will review the rest in a couple of hours, but I suspect it will be mostly nits. https://codereview.chromium.org/492133002/diff/120001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/492133002/diff/120001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:164: blink::WebFrame* main_frame = render_view()->GetWebView()->mainFrame(); On 2014/08/28 13:02:42, Mark Dittmer wrote: > On 2014/08/27 23:01:11, Devlin wrote: > > We should > > a) NULL-check the main frame. > > b) check that the urls match. What would be even better would be to check if > > the page has navigated since, but that might involve a bit more > infrastructure. > > Done. I have one potential concern, but I don't think it applies to declarative > script injections. Is this injection, if it happens, guaranteed to inject > immediately; i.e., before the renderer may have a chance to navigate elsewhere? > If not, this check should probably go closer to the injection point. The only time it wouldn't inject before a page change would be if it gets put on hold - which can't happen until the TODO on line 361 is resolved.
On 2014/08/28 17:53:58, Devlin wrote: > Will review the rest in a couple of hours, but I suspect it will be mostly nits. > > https://codereview.chromium.org/492133002/diff/120001/extensions/renderer/scr... > File extensions/renderer/script_injection_manager.cc (right): > > https://codereview.chromium.org/492133002/diff/120001/extensions/renderer/scr... > extensions/renderer/script_injection_manager.cc:164: blink::WebFrame* main_frame > = render_view()->GetWebView()->mainFrame(); > On 2014/08/28 13:02:42, Mark Dittmer wrote: > > On 2014/08/27 23:01:11, Devlin wrote: > > > We should > > > a) NULL-check the main frame. > > > b) check that the urls match. What would be even better would be to check > if > > > the page has navigated since, but that might involve a bit more > > infrastructure. > > > > Done. I have one potential concern, but I don't think it applies to > declarative > > script injections. Is this injection, if it happens, guaranteed to inject > > immediately; i.e., before the renderer may have a chance to navigate > elsewhere? > > If not, this check should probably go closer to the injection point. > > The only time it wouldn't inject before a page change would be if it gets put on > hold - which can't happen until the TODO on line 361 is resolved. Got it. Not an issue here then (at least, not yet).
If all the bots are happy, I'm happy, modulo a few nits. LGTM. https://codereview.chromium.org/492133002/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc (right): https://codereview.chromium.org/492133002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc:122: scoped_ptr<ExtensionTestMessageListener> injection_succeeded_listener; No reason for scoped_ptrs here. https://codereview.chromium.org/492133002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc:126: EXPECT_TRUE(CreateAndLoadExtension(manifest_permission, script_matcher)); This should be something like: if (!CreateAndLoadExtension()) return testing::AssertionFailure() << "Could not load extension"; or, if you prefer not to make CreateAndLoadExtension return a bool: testing::AssertionResult result = CreateAndLoadExtension(); if (!result) return result; https://codereview.chromium.org/492133002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc:129: injection_setup_listener.reset(new ExtensionTestMessageListener( This looks racy to me. We should have the injection set up listener within CreateAndLoadExtension, create it *before* we load the extension, and just not restrict it to an id. https://codereview.chromium.org/492133002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc:213: EXPECT_TRUE(RunTest(PARTICULAR, NOWHERE, false)); I wonder if it would be worthwhile to ever test anything else, like requesting multiple scripts.... maybe put in a TODO with what you can think to test. https://codereview.chromium.org/492133002/diff/140001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/492133002/diff/140001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:170: if (main_frame->top()->document().url() == url) { This should *probably* be done better. This doesn't check, for instance, reloading the page. I think we have page_ids that are renderer-specific, so I could imagine a general flow of: Renderer notifies browser of a potential injection point and records the id. Browser says "Inject!" Renderer compares the page ids. But... again, we can make that a TODO for now. https://codereview.chromium.org/492133002/diff/140001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:372: scripts_run_info.LogRun(web_frame, UserScript::BROWSER_DRIVEN); Even if we don't use the result of TryToInject yet, we definitely shouldn't log if we didn't inject anything.
Fixed final nits. https://codereview.chromium.org/492133002/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc (right): https://codereview.chromium.org/492133002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc:122: scoped_ptr<ExtensionTestMessageListener> injection_succeeded_listener; On 2014/08/28 21:38:10, Devlin wrote: > No reason for scoped_ptrs here. Done. https://codereview.chromium.org/492133002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc:126: EXPECT_TRUE(CreateAndLoadExtension(manifest_permission, script_matcher)); On 2014/08/28 21:38:10, Devlin wrote: > This should be something like: > if (!CreateAndLoadExtension()) > return testing::AssertionFailure() << "Could not load extension"; > or, if you prefer not to make CreateAndLoadExtension return a bool: > testing::AssertionResult result = CreateAndLoadExtension(); > if (!result) > return result; Done. https://codereview.chromium.org/492133002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc:129: injection_setup_listener.reset(new ExtensionTestMessageListener( On 2014/08/28 21:38:10, Devlin wrote: > This looks racy to me. We should have the injection set up listener within > CreateAndLoadExtension, create it *before* we load the extension, and just not > restrict it to an id. Done. https://codereview.chromium.org/492133002/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/request_content_script_apitest.cc:213: EXPECT_TRUE(RunTest(PARTICULAR, NOWHERE, false)); On 2014/08/28 21:38:10, Devlin wrote: > I wonder if it would be worthwhile to ever test anything else, like requesting > multiple scripts.... maybe put in a TODO with what you can think to test. Done. https://codereview.chromium.org/492133002/diff/140001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/492133002/diff/140001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:170: if (main_frame->top()->document().url() == url) { On 2014/08/28 21:38:10, Devlin wrote: > This should *probably* be done better. This doesn't check, for instance, > reloading the page. I think we have page_ids that are renderer-specific, so I > could imagine a general flow of: > Renderer notifies browser of a potential injection point and records the id. > Browser says "Inject!" > Renderer compares the page ids. > > But... again, we can make that a TODO for now. Done. https://codereview.chromium.org/492133002/diff/140001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:372: scripts_run_info.LogRun(web_frame, UserScript::BROWSER_DRIVEN); On 2014/08/28 21:38:10, Devlin wrote: > Even if we don't use the result of TryToInject yet, we definitely shouldn't log > if we didn't inject anything. As per our conversation, ScriptInjection::TryToInject reads: "This returns true if the script has either injected or will never inject (i.e., if the object is done)". As such, there is no way to know the difference, but LogRun shouldn't log anything if no scripts have been run, so we'll leave this as-is.
The CQ bit was checked by markdittmer@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/markdittmer@chromium.org/492133002/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/47896) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as 4f974cc82ccda03ede230f8aded6c8ce0b8d640e
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/9ea140f4f6a694b3a65461f4ac41daf94dc2c59c Cr-Commit-Position: refs/heads/master@{#292558} |
