Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
885493007:
Refactoring: de-couple Extensions from "script injection System" [render side] : 1 (Closed)
|
Created:
5 years, 10 months ago by Xi Han Modified:
5 years, 10 months ago 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. |
DescriptionA refacotring in the renderer side: introduce InjectionHost to de-couple extensions and to support content script injection out of chrome extensions.
BUG=437566
Committed: https://crrev.com/a5c856cf3ea4f69aa00c6c24676e0bca3f05962a
Cr-Commit-Position: refs/heads/master@{#316283}
Patch Set 1 : Introduce instance_id in the key of IsolatedWorldMap. #Patch Set 2 : Plumb script_injection_instance_id from WebViewGuest to script injection. #
Total comments: 31
Patch Set 3 : Devlin@'s comments. #
Total comments: 18
Patch Set 4 : #Patch Set 5 : Remove Host::IsEmpty() and move ExtensionConsumer to extensions/renderer. #
Total comments: 36
Patch Set 6 : Devlin's comments. #
Total comments: 12
Patch Set 7 : #
Total comments: 27
Patch Set 8 : nits #Patch Set 9 : #Patch Set 10 : Revert key of IsolatedWorldMap as std::string. #
Total comments: 5
Patch Set 11 : nits. #
Total comments: 3
Patch Set 12 : #Messages
Total messages: 47 (15 generated)
Patchset #2 (id:20001) has been deleted
hanxi@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Hi Devlin: the instance_id is stored in ScriptInjection, since I assume ScriptInjection is per ScriptInjector per UserScript. Please take a look at changes in: -extensions/common -extensions/renderer Thanks a lot!
hanxi@chromium.org changed reviewers: + fsamuel@chromium.org
Hi Fady: An script injection instance is is introduced in WebViewGuest, and it will be used when assigning an isolated world number for script injection. With these changes, the injection will be an isolated world per instance. Please review changes in: -extensions/browser.
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
This change seems reasonable to me so lgtm. I defer to Devlin for the final decision though since he understands these concepts better than I do.
One question before I review this. Last week, we were discussing implementing this feature in terms of only using content scripts (which could be dynamically added), rather than also using the declarative content api, and, from what I recall, it sounded like that would work for the use cases. Going forward, is that the direction we are trying to take?
Yes, that is the direction that we are trying to take. On Wed, Feb 4, 2015 at 11:33 AM, <rdevlin.cronin@chromium.org> wrote: > One question before I review this. > > Last week, we were discussing implementing this feature in terms of only > using > content scripts (which could be dynamically added), rather than also using > the > declarative content api, and, from what I recall, it sounded like that > would > work for the use cases. Going forward, is that the direction we are > trying to > take? > > https://codereview.chromium.org/885493007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/02/04 16:35:23, chromium-reviews wrote: > Yes, that is the direction that we are trying to take. > > On Wed, Feb 4, 2015 at 11:33 AM, <mailto:rdevlin.cronin@chromium.org> wrote: > > > One question before I review this. > > > > Last week, we were discussing implementing this feature in terms of only > > using > > content scripts (which could be dynamically added), rather than also using > > the > > declarative content api, and, from what I recall, it sounded like that > > would > > work for the use cases. Going forward, is that the direction we are > > trying to > > take? > > > > https://codereview.chromium.org/885493007/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. In that case, can you make a CL to clean up the changes to the declarative content api in https://codereview.chromium.org/822453002 first? It'll get confusing if we leave those in, even when we're going a different direction. :) (will still review this one now, but will want the cleanup landed before this one.)
First pass. https://codereview.chromium.org/885493007/diff/80001/extensions/browser/guest... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/885493007/diff/80001/extensions/browser/guest... extensions/browser/guest_view/web_view/web_view_guest.cc:218: int WebViewGuest:: GetOrGenerateScriptInjectionInstanceID( Why can't we use the guest_instance_id for the instance id? https://codereview.chromium.org/885493007/diff/80001/extensions/browser/scrip... File extensions/browser/script_executor.cc (right): https://codereview.chromium.org/885493007/diff/80001/extensions/browser/scrip... extensions/browser/script_executor.cc:148: params.instance_id = params.is_web_view ? instance_id : It seems odd that you pass in an instance id from the tabs api, only to ignore it here. I think it'd be cleaner to just assign params.instance_id to the passed in instance id, and add a DCHECK that either it's a webview process or the instance id is 0. https://codereview.chromium.org/885493007/diff/80001/extensions/common/extens... File extensions/common/extension_consumer.cc (right): https://codereview.chromium.org/885493007/diff/80001/extensions/common/extens... extensions/common/extension_consumer.cc:12: extension_(make_scoped_refptr(extension)) { nit: indentation https://codereview.chromium.org/885493007/diff/80001/extensions/common/extens... File extensions/common/extension_consumer.h (right): https://codereview.chromium.org/885493007/diff/80001/extensions/common/extens... extensions/common/extension_consumer.h:15: class ExtensionConsumer : public Host { Does this class (and Host itself) have to be in common? Or can they be in renderer? https://codereview.chromium.org/885493007/diff/80001/extensions/common/extens... extensions/common/extension_consumer.h:17: ExtensionConsumer(Extension* extension, const HostID& host_id); If this is going to store it as a scoped_refptr, it should take in a scoped_refptr (it's fine to do so by const &). https://codereview.chromium.org/885493007/diff/80001/extensions/common/extens... extensions/common/extension_consumer.h:23: scoped_refptr<Extension> extension_; Extensions should be const. https://codereview.chromium.org/885493007/diff/80001/extensions/common/host_id.h File extensions/common/host_id.h (right): https://codereview.chromium.org/885493007/diff/80001/extensions/common/host_i... extensions/common/host_id.h:44: // An interface for all kinds of hosts who own user scripts. nit: extra space between "of" and "hosts" https://codereview.chromium.org/885493007/diff/80001/extensions/renderer/scri... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/885493007/diff/80001/extensions/renderer/scri... extensions/renderer/script_injection.cc:92: static_cast<const ExtensionConsumer*>(host)->extension(); Generally, having something like class AbstractClass { Type type() = 0; } class DerivedClass : public AbstractClass { Type type() { return TYPE_A; } } if (instance->type() == TYPE_A) { DerivedClass* derived = static_cast<DerivedClass*>(instance); derived->... } Is an antipattern. (Said differently, having times when you have to add RTTI to a class and then cast it based on that.) There are some times when we can't really avoid it, but I don't think this is one of those times. Instead, I think we should have methods on Host that handle these things, e.g. Host::GetContentSecurityPolicy(), etc. https://codereview.chromium.org/885493007/diff/80001/extensions/renderer/scri... extensions/renderer/script_injection.cc:120: IsolatedWorldMap& isolated_worlds = g_isolated_worlds.Get(); const & https://codereview.chromium.org/885493007/diff/80001/extensions/renderer/scri... extensions/renderer/script_injection.cc:122: for (auto& kv : isolated_worlds) { const auto& https://codereview.chromium.org/885493007/diff/80001/extensions/renderer/scri... extensions/renderer/script_injection.cc:179: static_cast<const ExtensionConsumer*>(host)->extension(); This feels very unfinished to me. I think I'd rather you just keep passing in the extension until you're ready to convert all/most of the usages. Also, it makes it easier to lead to nasty breakages (see comment in script_injection_manager.cc). https://codereview.chromium.org/885493007/diff/80001/extensions/renderer/scri... File extensions/renderer/script_injection.h (right): https://codereview.chromium.org/885493007/diff/80001/extensions/renderer/scri... extensions/renderer/script_injection.h:87: // If the instance is a <webview>, the |instance_id| is a unique positive nit: lowercase "if" and fix line wrapping. https://codereview.chromium.org/885493007/diff/80001/extensions/renderer/scri... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/885493007/diff/80001/extensions/renderer/scri... extensions/renderer/script_injection_manager.cc:383: new ExtensionConsumer( This breaks. ScriptInjection checks that |extension| is non-null, which you have updated to check that |host| is non-null. But you never pass in a null host here.
I will definitely make a CL to clean up of plumbing HostID in content_action.h(.cc) and declarative_rule.h. I think other changes need to remain and will be used in the following CL. Is there anything that you suggest to revert as well? On Wed, Feb 4, 2015 at 11:37 AM, <rdevlin.cronin@chromium.org> wrote: > On 2015/02/04 16:35:23, chromium-reviews wrote: > >> Yes, that is the direction that we are trying to take. >> > > On Wed, Feb 4, 2015 at 11:33 AM, <mailto:rdevlin.cronin@chromium.org> >> wrote: >> > > > One question before I review this. >> > >> > Last week, we were discussing implementing this feature in terms of only >> > using >> > content scripts (which could be dynamically added), rather than also >> using >> > the >> > declarative content api, and, from what I recall, it sounded like that >> > would >> > work for the use cases. Going forward, is that the direction we are >> > trying to >> > take? >> > >> > https://codereview.chromium.org/885493007/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > In that case, can you make a CL to clean up the changes to the declarative > content api in https://codereview.chromium.org/822453002 first? It'll get > confusing if we leave those in, even when we're going a different > direction. :) > (will still review this one now, but will want the cleanup landed before > this > one.) > > https://codereview.chromium.org/885493007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/02/04 17:07:59, chromium-reviews wrote: > I will definitely make a CL to clean up of plumbing HostID in > content_action.h(.cc) and declarative_rule.h. I think other changes need to > remain and will be used in the following CL. Is there anything that you > suggest to revert as well? I think basically everything in c/b/e/api/declarative_content and e/b/api/declarative should be reverted, right?
Yes, that is as what I thought. On Wed, Feb 4, 2015 at 12:11 PM, <rdevlin.cronin@chromium.org> wrote: > On 2015/02/04 17:07:59, chromium-reviews wrote: > >> I will definitely make a CL to clean up of plumbing HostID in >> content_action.h(.cc) and declarative_rule.h. I think other changes need >> to >> remain and will be used in the following CL. Is there anything that you >> suggest to revert as well? >> > > I think basically everything in c/b/e/api/declarative_content and > e/b/api/declarative should be reverted, right? > > > https://codereview.chromium.org/885493007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
New patchsets have been uploaded after l-g-t-m from fsamuel@chromium.org
Patchset #3 (id:100001) has been deleted
Hi Devlin: PTAL, thanks. https://codereview.chromium.org/885493007/diff/80001/extensions/browser/guest... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/885493007/diff/80001/extensions/browser/guest... extensions/browser/guest_view/web_view/web_view_guest.cc:218: int WebViewGuest:: GetOrGenerateScriptInjectionInstanceID( On 2015/02/04 17:01:14, Devlin wrote: > Why can't we use the guest_instance_id for the instance id? The guest_instance_id isn't a unique id, |embedder_process_id, guest_instance_id| pair is a unique id. I think creating such a unique id is efficient for assigning of isolated world right now, but we can definitely passing in other kinds of unique id pair to replace this one if needed. https://codereview.chromium.org/885493007/diff/80001/extensions/browser/scrip... File extensions/browser/script_executor.cc (right): https://codereview.chromium.org/885493007/diff/80001/extensions/browser/scrip... extensions/browser/script_executor.cc:148: params.instance_id = params.is_web_view ? instance_id : On 2015/02/04 17:01:14, Devlin wrote: > It seems odd that you pass in an instance id from the tabs api, only to ignore > it here. I think it'd be cleaner to just assign params.instance_id to the > passed in instance id, and add a DCHECK that either it's a webview process or > the instance id is 0. The webview.executecode api also needs to call this ExecuteScript function, so the instance_id is passed here to make scripts injection to a separate isolated world. https://codereview.chromium.org/885493007/diff/80001/extensions/common/extens... File extensions/common/extension_consumer.cc (right): https://codereview.chromium.org/885493007/diff/80001/extensions/common/extens... extensions/common/extension_consumer.cc:12: extension_(make_scoped_refptr(extension)) { On 2015/02/04 17:01:14, Devlin wrote: > nit: indentation Done. https://codereview.chromium.org/885493007/diff/80001/extensions/common/extens... File extensions/common/extension_consumer.h (right): https://codereview.chromium.org/885493007/diff/80001/extensions/common/extens... extensions/common/extension_consumer.h:15: class ExtensionConsumer : public Host { On 2015/02/04 17:01:14, Devlin wrote: > Does this class (and Host itself) have to be in common? Or can they be in > renderer? At least now I think both of them should be in common, since on the browser side, they also be used by UserScriptLoader etc.. https://codereview.chromium.org/885493007/diff/80001/extensions/common/extens... extensions/common/extension_consumer.h:17: ExtensionConsumer(Extension* extension, const HostID& host_id); On 2015/02/04 17:01:14, Devlin wrote: > If this is going to store it as a scoped_refptr, it should take in a > scoped_refptr (it's fine to do so by const &). Done. https://codereview.chromium.org/885493007/diff/80001/extensions/common/extens... extensions/common/extension_consumer.h:23: scoped_refptr<Extension> extension_; On 2015/02/04 17:01:14, Devlin wrote: > Extensions should be const. Done. https://codereview.chromium.org/885493007/diff/80001/extensions/common/host_id.h File extensions/common/host_id.h (right): https://codereview.chromium.org/885493007/diff/80001/extensions/common/host_i... extensions/common/host_id.h:44: // An interface for all kinds of hosts who own user scripts. On 2015/02/04 17:01:14, Devlin wrote: > nit: extra space between "of" and "hosts" Done. https://codereview.chromium.org/885493007/diff/80001/extensions/renderer/scri... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/885493007/diff/80001/extensions/renderer/scri... extensions/renderer/script_injection.cc:92: static_cast<const ExtensionConsumer*>(host)->extension(); On 2015/02/04 17:01:14, Devlin wrote: > Generally, having something like > class AbstractClass { > Type type() = 0; > } > > class DerivedClass : public AbstractClass { > Type type() { return TYPE_A; } > } > > if (instance->type() == TYPE_A) { > DerivedClass* derived = static_cast<DerivedClass*>(instance); > derived->... > } > > Is an antipattern. (Said differently, having times when you have to add RTTI to > a class and then cast it based on that.) > > There are some times when we can't really avoid it, but I don't think this is > one of those times. Instead, I think we should have methods on Host that handle > these things, e.g. Host::GetContentSecurityPolicy(), etc. Adds some of these functions in the Host class. https://codereview.chromium.org/885493007/diff/80001/extensions/renderer/scri... extensions/renderer/script_injection.cc:120: IsolatedWorldMap& isolated_worlds = g_isolated_worlds.Get(); On 2015/02/04 17:01:14, Devlin wrote: > const & Done. https://codereview.chromium.org/885493007/diff/80001/extensions/renderer/scri... extensions/renderer/script_injection.cc:122: for (auto& kv : isolated_worlds) { On 2015/02/04 17:01:14, Devlin wrote: > const auto& Done. https://codereview.chromium.org/885493007/diff/80001/extensions/renderer/scri... extensions/renderer/script_injection.cc:179: static_cast<const ExtensionConsumer*>(host)->extension(); On 2015/02/04 17:01:14, Devlin wrote: > This feels very unfinished to me. I think I'd rather you just keep passing in > the extension until you're ready to convert all/most of the usages. Also, it > makes it easier to lead to nasty breakages (see comment in > script_injection_manager.cc). I updated all the if(!host) check, it is a great catch, and I do appreciate it. For passing in host, if we can resolve these hazards, I still want to keep these changes since it makes possible that each CL only contains few files changed. https://codereview.chromium.org/885493007/diff/80001/extensions/renderer/scri... File extensions/renderer/script_injection.h (right): https://codereview.chromium.org/885493007/diff/80001/extensions/renderer/scri... extensions/renderer/script_injection.h:87: // If the instance is a <webview>, the |instance_id| is a unique positive On 2015/02/04 17:01:14, Devlin wrote: > nit: lowercase "if" and fix line wrapping. Done. https://codereview.chromium.org/885493007/diff/80001/extensions/renderer/scri... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/885493007/diff/80001/extensions/renderer/scri... extensions/renderer/script_injection_manager.cc:383: new ExtensionConsumer( On 2015/02/04 17:01:14, Devlin wrote: > This breaks. > > ScriptInjection checks that |extension| is non-null, which you have updated to > check that |host| is non-null. But you never pass in a null host here. You are absolutely right. In ScriptInjection class, what I need to check is whether the pointer of Extension within the host is empty.
https://codereview.chromium.org/885493007/diff/80001/extensions/browser/scrip... File extensions/browser/script_executor.cc (right): https://codereview.chromium.org/885493007/diff/80001/extensions/browser/scrip... extensions/browser/script_executor.cc:148: params.instance_id = params.is_web_view ? instance_id : On 2015/02/05 16:06:20, Xi Han wrote: > On 2015/02/04 17:01:14, Devlin wrote: > > It seems odd that you pass in an instance id from the tabs api, only to ignore > > it here. I think it'd be cleaner to just assign params.instance_id to the > > passed in instance id, and add a DCHECK that either it's a webview process or > > the instance id is 0. > > The webview.executecode api also needs to call this ExecuteScript function, so > the instance_id is passed here to make scripts injection to a separate isolated > world. Right. What I am suggesting is: params.instance_id = instance_id; // Only webviews should have custom instance ids. DCHECK(instance_id == Host::kDefaultInstanceId || params.is_web_view); Since the each execute code function passes in an instance id, this should be correct, and this way we know if it fails. https://codereview.chromium.org/885493007/diff/80001/extensions/common/extens... File extensions/common/extension_consumer.h (right): https://codereview.chromium.org/885493007/diff/80001/extensions/common/extens... extensions/common/extension_consumer.h:15: class ExtensionConsumer : public Host { On 2015/02/05 16:06:20, Xi Han wrote: > On 2015/02/04 17:01:14, Devlin wrote: > > Does this class (and Host itself) have to be in common? Or can they be in > > renderer? > > At least now I think both of them should be in common, since on the browser > side, they also be used by UserScriptLoader etc.. Where are they used by UserScriptLoader, etc? I only see them used on the renderer side in this patch. https://codereview.chromium.org/885493007/diff/120001/extensions/browser/api/... File extensions/browser/api/web_view/web_view_internal_api.h (right): https://codereview.chromium.org/885493007/diff/120001/extensions/browser/api/... extensions/browser/api/web_view/web_view_internal_api.h:80: int script_injection_instance_id_; You need to initialize this in the constructor. https://codereview.chromium.org/885493007/diff/120001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/885493007/diff/120001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:194: static base::LazyInstance<WebViewKeyToIDMap> script_injection_instance_id_map = It bothers me that we have multiple different maps of "webview to unique id". We should only ever need one unique id per webview, right? https://codereview.chromium.org/885493007/diff/120001/extensions/common/host_... File extensions/common/host_id.h (right): https://codereview.chromium.org/885493007/diff/120001/extensions/common/host_... extensions/common/host_id.h:57: // (extension, or WebUI). The idea of a Host being non-null but "empty" is weird. Why make one if it's going to be empty, instead of just not creating it at all (and checking the host for nullness?) https://codereview.chromium.org/885493007/diff/120001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/885493007/diff/120001/extensions/renderer/scr... extensions/renderer/script_injection.cc:88: name = "WebView"; We shouldn't do this here; the host should return its own name. https://codereview.chromium.org/885493007/diff/120001/extensions/renderer/scr... extensions/renderer/script_injection.cc:117: void ScriptInjection::RemoveIsolatedWorld(const std::string& host_id) { This is weird, too. We key isolated worlds based on host ids and ints, but then we remove them based on strings? https://codereview.chromium.org/885493007/diff/120001/extensions/renderer/scr... extensions/renderer/script_injection.cc:166: static_cast<const ExtensionConsumer*>(host)->extension(); again, doing static casts like this is bad.
Patchset #4 (id:140001) has been deleted
https://codereview.chromium.org/885493007/diff/80001/extensions/browser/scrip... File extensions/browser/script_executor.cc (right): https://codereview.chromium.org/885493007/diff/80001/extensions/browser/scrip... extensions/browser/script_executor.cc:148: params.instance_id = params.is_web_view ? instance_id : On 2015/02/06 00:28:40, Devlin wrote: > On 2015/02/05 16:06:20, Xi Han wrote: > > On 2015/02/04 17:01:14, Devlin wrote: > > > It seems odd that you pass in an instance id from the tabs api, only to > ignore > > > it here. I think it'd be cleaner to just assign params.instance_id to the > > > passed in instance id, and add a DCHECK that either it's a webview process > or > > > the instance id is 0. > > > > The webview.executecode api also needs to call this ExecuteScript function, so > > the instance_id is passed here to make scripts injection to a separate > isolated > > world. > > Right. What I am suggesting is: > params.instance_id = instance_id; > // Only webviews should have custom instance ids. > DCHECK(instance_id == Host::kDefaultInstanceId || params.is_web_view); > > Since the each execute code function passes in an instance id, this should be > correct, and this way we know if it fails. Good idea, I misunderstood your last comments. Updated. https://codereview.chromium.org/885493007/diff/120001/extensions/browser/api/... File extensions/browser/api/web_view/web_view_internal_api.h (right): https://codereview.chromium.org/885493007/diff/120001/extensions/browser/api/... extensions/browser/api/web_view/web_view_internal_api.h:80: int script_injection_instance_id_; On 2015/02/06 00:28:41, Devlin wrote: > You need to initialize this in the constructor. Done. https://codereview.chromium.org/885493007/diff/120001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/885493007/diff/120001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:194: static base::LazyInstance<WebViewKeyToIDMap> script_injection_instance_id_map = On 2015/02/06 00:28:41, Devlin wrote: > It bothers me that we have multiple different maps of "webview to unique id". > We should only ever need one unique id per webview, right? We would like to have just one function, but the implementation of the Rule Registry part calls RulesRegistryService::GetNextRulesRegistryID() to generate the rule registry id, since the assignment of id is done by RulesRegistryService. To generate the script_injection_instance_id, however, we don't need the RulesRegistryService to make a decision, so we have to use another map to stored the kv pairs, since the value may be different for the same key in two maps. https://codereview.chromium.org/885493007/diff/120001/extensions/common/host_... File extensions/common/host_id.h (right): https://codereview.chromium.org/885493007/diff/120001/extensions/common/host_... extensions/common/host_id.h:57: // (extension, or WebUI). On 2015/02/06 00:28:41, Devlin wrote: > The idea of a Host being non-null but "empty" is weird. Why make one if it's > going to be empty, instead of just not creating it at all (and checking the host > for nullness?) My initial thoughts was we may have a WebUIConsumer, but it doesn't have such a pointer of an object of the real host(like scoped_ptr<const Extension> in ExtensionConsumer). In many places, we need a check to replace the "if(!extension)" check, and the check will look like: if(host_type == EXTENSION && !extension). We always want to know the type of the host, so we can distinguish whether is the extension is gone, or it is a webUI. https://codereview.chromium.org/885493007/diff/120001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/885493007/diff/120001/extensions/renderer/scr... extensions/renderer/script_injection.cc:88: name = "WebView"; On 2015/02/06 00:28:41, Devlin wrote: > We shouldn't do this here; the host should return its own name. I am just not sure which name should be used here. But if it is fine to use the host name for injections from <webview>, I am totally ok with it. https://codereview.chromium.org/885493007/diff/120001/extensions/renderer/scr... extensions/renderer/script_injection.cc:117: void ScriptInjection::RemoveIsolatedWorld(const std::string& host_id) { On 2015/02/06 00:28:41, Devlin wrote: > This is weird, too. We key isolated worlds based on host ids and ints, but then > we remove them based on strings? You are right. Will pass in const Host&. https://codereview.chromium.org/885493007/diff/120001/extensions/renderer/scr... extensions/renderer/script_injection.cc:166: static_cast<const ExtensionConsumer*>(host)->extension(); On 2015/02/06 00:28:41, Devlin wrote: > again, doing static casts like this is bad. Remove the static casts here. Instead, I pass the "const Host*" to the injector and move the implementation of CanExecuteOnFrame to Host.
https://codereview.chromium.org/885493007/diff/80001/extensions/common/extens... File extensions/common/extension_consumer.h (right): https://codereview.chromium.org/885493007/diff/80001/extensions/common/extens... extensions/common/extension_consumer.h:15: class ExtensionConsumer : public Host { On 2015/02/06 00:28:40, Devlin wrote: > On 2015/02/05 16:06:20, Xi Han wrote: > > On 2015/02/04 17:01:14, Devlin wrote: > > > Does this class (and Host itself) have to be in common? Or can they be in > > > renderer? > > > > At least now I think both of them should be in common, since on the browser > > side, they also be used by UserScriptLoader etc.. > > Where are they used by UserScriptLoader, etc? I only see them used on the > renderer side in this patch. Didn't get an answer to this one. https://codereview.chromium.org/885493007/diff/120001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/885493007/diff/120001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:194: static base::LazyInstance<WebViewKeyToIDMap> script_injection_instance_id_map = On 2015/02/06 17:21:45, Xi Han wrote: > On 2015/02/06 00:28:41, Devlin wrote: > > It bothers me that we have multiple different maps of "webview to unique id". > > We should only ever need one unique id per webview, right? > > We would like to have just one function, but the implementation of the Rule > Registry part calls RulesRegistryService::GetNextRulesRegistryID() to generate > the rule registry id, since the assignment of id is done by > RulesRegistryService. To generate the script_injection_instance_id, however, we > don't need the RulesRegistryService to make a decision, so we have to use > another map to stored the kv pairs, since the value may be different for the > same key in two maps. But the RulesRegistry one is going away, right? https://codereview.chromium.org/885493007/diff/120001/extensions/common/host_... File extensions/common/host_id.h (right): https://codereview.chromium.org/885493007/diff/120001/extensions/common/host_... extensions/common/host_id.h:57: // (extension, or WebUI). On 2015/02/06 17:21:45, Xi Han wrote: > On 2015/02/06 00:28:41, Devlin wrote: > > The idea of a Host being non-null but "empty" is weird. Why make one if it's > > going to be empty, instead of just not creating it at all (and checking the > host > > for nullness?) > > My initial thoughts was we may have a WebUIConsumer, but it doesn't have such a > pointer of an object of the real host(like scoped_ptr<const Extension> in > ExtensionConsumer). In many places, we need a check to replace the > "if(!extension)" check, and the check will look like: if(host_type == EXTENSION > && !extension). We always want to know the type of the host, so we can > distinguish whether is the extension is gone, or it is a webUI. Well, all interactions from the script injection should go through the Host, not through the host's backing object. So a _host_ should always be able to answer things like "GetContentSecurityPolicy". If it's a webui and able to answer these questions without a backing pointer (which it should be able to), that's fine - but having to always check that !host->IsEmpty() seems cumbersome, when if the host is empty (and needs to not be empty), there's nothing we can do. Instead, we should only create valid hosts (for extensions, this means that we should only pass in hosts that have a backing extension).
Patchset #5 (id:170001) has been deleted
https://codereview.chromium.org/885493007/diff/80001/extensions/common/extens... File extensions/common/extension_consumer.h (right): https://codereview.chromium.org/885493007/diff/80001/extensions/common/extens... extensions/common/extension_consumer.h:15: class ExtensionConsumer : public Host { Sorry for missing this comment. Yes, it seems this class in only used the render side, since we always pass hosd_id along with the ipc from browser to render, and reconstruct the ExtensionConsumer object in the render. Moved. https://codereview.chromium.org/885493007/diff/120001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/885493007/diff/120001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:194: static base::LazyInstance<WebViewKeyToIDMap> script_injection_instance_id_map = Hmmm, I don't think so, because the declarative webrequest still needs this map to get an rule registry id for <webview>. https://codereview.chromium.org/885493007/diff/120001/extensions/common/host_... File extensions/common/host_id.h (right): https://codereview.chromium.org/885493007/diff/120001/extensions/common/host_... extensions/common/host_id.h:57: // (extension, or WebUI). On 2015/02/06 18:48:36, Devlin wrote: > On 2015/02/06 17:21:45, Xi Han wrote: > > On 2015/02/06 00:28:41, Devlin wrote: > > > The idea of a Host being non-null but "empty" is weird. Why make one if > it's > > > going to be empty, instead of just not creating it at all (and checking the > > host > > > for nullness?) > > > > My initial thoughts was we may have a WebUIConsumer, but it doesn't have such > a > > pointer of an object of the real host(like scoped_ptr<const Extension> in > > ExtensionConsumer). In many places, we need a check to replace the > > "if(!extension)" check, and the check will look like: if(host_type == > EXTENSION > > && !extension). We always want to know the type of the host, so we can > > distinguish whether is the extension is gone, or it is a webUI. > > Well, all interactions from the script injection should go through the Host, not > through the host's backing object. So a _host_ should always be able to answer > things like "GetContentSecurityPolicy". If it's a webui and able to answer > these questions without a backing pointer (which it should be able to), that's > fine - but having to always check that !host->IsEmpty() seems cumbersome, when > if the host is empty (and needs to not be empty), there's nothing we can do. > Instead, we should only create valid hosts (for extensions, this means that we > should only pass in hosts that have a backing extension). That is fair. We can create a Host object for webUI even with an null pointer inside; while create a null ptr if the extension is gone. In this way, "!host" will always means the host is gone.
https://codereview.chromium.org/885493007/diff/120001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/885493007/diff/120001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:194: static base::LazyInstance<WebViewKeyToIDMap> script_injection_instance_id_map = On 2015/02/06 19:45:33, Xi Han wrote: > Hmmm, I don't think so, because the declarative webrequest still needs this map > to get an rule registry id for <webview>. > Ah, right. In that case, can we make GetRulesRegistryId more generic, and just say GetUniqueWebviewId, and use that for both rules registry and script injection? It just seems silly to have two maps used for generating a unique id for a webview. https://codereview.chromium.org/885493007/diff/190001/extensions/common/exten... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/885493007/diff/190001/extensions/common/exten... extensions/common/extension_messages.h:139: // from a <webview>. add an "else, the default instance id (0)." https://codereview.chromium.org/885493007/diff/190001/extensions/common/host_... File extensions/common/host_id.h (right): https://codereview.chromium.org/885493007/diff/190001/extensions/common/host_... extensions/common/host_id.h:51: class Host { Can't this also be moved to renderer? And just put kDefaultInstanceId in HostId or something? https://codereview.chromium.org/885493007/diff/190001/extensions/common/host_... extensions/common/host_id.h:51: class Host { Having a class named "Host" in the global namespace seems... a bit wrong. In the long run, we may want a namespace like "script_injection", but in the short-term, we should probably just have this be ScriptInjectionHost or InjectionHost (probably the latter, for brevity). Also, since everything else is in the extensions namespace for the time being, this probably should be, too. https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/ext... File extensions/renderer/extension_consumer.cc (right): https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/ext... extensions/renderer/extension_consumer.cc:24: DCHECK(extension_.get()); Probably better to just move the DCHECK into the constructor. Maybe even make it a CHECK, since it's pretty important. https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/ext... extensions/renderer/extension_consumer.cc:48: NULL /* ignore error */); nit: prefer nullptr. https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/ext... File extensions/renderer/extension_consumer.h (right): https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/ext... extensions/renderer/extension_consumer.h:18: const HostID& host_id); Why do we need to pass in a host id? Can't an ExtensionConsumer know how to create it's own id (since it will always be of type extension with the id being the extension id)? https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/pro... File extensions/renderer/programmatic_script_injector.cc (right): https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/pro... extensions/renderer/programmatic_script_injector.cc:75: if (params_->is_web_view) { Since we're abstracting out the idea of a host, it might be nice to put these in the host as well, and a webui host can just return it. Doesn't need to happen in this patch, but please put a TODO. https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/pro... File extensions/renderer/programmatic_script_injector.h (right): https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/pro... extensions/renderer/programmatic_script_injector.h:24: class Extension; Shouldn't need this. https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/scr... extensions/renderer/script_injection.cc:72: const HostID& host_id = host->id(); This cached variable is kinda silly. Just call host->id() when you initialize the key. https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/scr... extensions/renderer/script_injection.cc:85: const GURL& origin = host->url(); Same here - just use host->url() on line 91 and host->name() on line 95. https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/scr... extensions/renderer/script_injection.cc:117: for (auto& kv: isolated_worlds) { const auto& https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/scr... extensions/renderer/script_injection.cc:280: DOMActivityLogger::AttachToWorld(world_id, host->id().id()); We almost certainly don't want activity logger to have anything to do with webuis' injections. https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:391: scoped_ptr<ExtensionConsumer> extension_consumer = GetExtensionConsumer( Not really a big deal, since constructing an ExtensionConsumer is super cheap, but it'd be nice to avoid it at some point, and just have the injection own one (and be able to know when the backing extension is removed). Not in this patch, though. https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:435: HostID(HostID::EXTENSIONS, params.extension_id), Hmm... should the type be HostID::Extensions if it's a webview? https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:436: params.is_web_view ? params.instance_id : Host::kDefaultInstanceId, didn't we ensure on the browser side that params.instance_id is always correct (i.e., it will only be different from kDefaultInstanceId if it's a webview)? https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/scr... File extensions/renderer/script_injector.h (right): https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/scr... extensions/renderer/script_injector.h:23: class Extension; Don't need this, right? https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:63: extension_id_(script_->extension_id()), Things like this will need to be updated soon. https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/use... File extensions/renderer/user_script_injector.h (right): https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/use... extensions/renderer/user_script_injector.h:23: class Extension; Here too
Patchset #6 (id:210001) has been deleted
Hi Devlin: please take another look, thanks! https://codereview.chromium.org/885493007/diff/120001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/885493007/diff/120001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:194: static base::LazyInstance<WebViewKeyToIDMap> script_injection_instance_id_map = On 2015/02/09 17:40:24, Devlin wrote: > On 2015/02/06 19:45:33, Xi Han wrote: > > Hmmm, I don't think so, because the declarative webrequest still needs this > map > > to get an rule registry id for <webview>. > > > > Ah, right. In that case, can we make GetRulesRegistryId more generic, and just > say GetUniqueWebviewId, and use that for both rules registry and script > injection? It just seems silly to have two maps used for generating a unique id > for a webview. As discussed offline, we will still keeps two maps but will reorganize the code and make it more generic. https://codereview.chromium.org/885493007/diff/190001/extensions/common/exten... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/885493007/diff/190001/extensions/common/exten... extensions/common/extension_messages.h:139: // from a <webview>. On 2015/02/09 17:40:24, Devlin wrote: > add an "else, the default instance id (0)." Done. https://codereview.chromium.org/885493007/diff/190001/extensions/common/host_... File extensions/common/host_id.h (right): https://codereview.chromium.org/885493007/diff/190001/extensions/common/host_... extensions/common/host_id.h:51: class Host { On 2015/02/09 17:40:24, Devlin wrote: > Having a class named "Host" in the global namespace seems... a bit wrong. In > the long run, we may want a namespace like "script_injection", but in the > short-term, we should probably just have this be ScriptInjectionHost or > InjectionHost (probably the latter, for brevity). Also, since everything else > is in the extensions namespace for the time being, this probably should be, too. That is true, I will rename it as InjectionHost. I would still want to avoid namespace extensions, since this interface is introduced to decouple extension from the code. https://codereview.chromium.org/885493007/diff/190001/extensions/common/host_... extensions/common/host_id.h:51: class Host { On 2015/02/09 17:40:24, Devlin wrote: > Can't this also be moved to renderer? And just put kDefaultInstanceId in HostId > or something? Moved to renderer. Just put kDefaultInstanceId in HostId for now, and will move it to ConsumerInstanceID later. https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/ext... File extensions/renderer/extension_consumer.cc (right): https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/ext... extensions/renderer/extension_consumer.cc:24: DCHECK(extension_.get()); On 2015/02/09 17:40:24, Devlin wrote: > Probably better to just move the DCHECK into the constructor. Maybe even make > it a CHECK, since it's pretty important. Done. https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/ext... extensions/renderer/extension_consumer.cc:48: NULL /* ignore error */); On 2015/02/09 17:40:24, Devlin wrote: > nit: prefer nullptr. Done. https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/ext... File extensions/renderer/extension_consumer.h (right): https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/ext... extensions/renderer/extension_consumer.h:18: const HostID& host_id); On 2015/02/09 17:40:24, Devlin wrote: > Why do we need to pass in a host id? Can't an ExtensionConsumer know how to > create it's own id (since it will always be of type extension with the id being > the extension id)? That makes sense. Updated. https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/pro... File extensions/renderer/programmatic_script_injector.cc (right): https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/pro... extensions/renderer/programmatic_script_injector.cc:75: if (params_->is_web_view) { On 2015/02/09 17:40:24, Devlin wrote: > Since we're abstracting out the idea of a host, it might be nice to put these in > the host as well, and a webui host can just return it. Doesn't need to happen > in this patch, but please put a TODO. The is_web_view param belongs to the concept of "instance type", while the Host shouldn't contain the instance type that it will inject script on. It because we want to keep the 1:1 association of host and host object (extension or WebUI). We could reorganize the params here, but is_web_view should be in some other class that represents the instance, rather than the Host. https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/pro... File extensions/renderer/programmatic_script_injector.h (right): https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/pro... extensions/renderer/programmatic_script_injector.h:24: class Extension; On 2015/02/09 17:40:24, Devlin wrote: > Shouldn't need this. Removed. https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/scr... extensions/renderer/script_injection.cc:72: const HostID& host_id = host->id(); On 2015/02/09 17:40:25, Devlin wrote: > This cached variable is kinda silly. Just call host->id() when you initialize > the key. Done. https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/scr... extensions/renderer/script_injection.cc:85: const GURL& origin = host->url(); On 2015/02/09 17:40:25, Devlin wrote: > Same here - just use host->url() on line 91 and host->name() on line 95. Legacy code, forgot to clean up:( https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/scr... extensions/renderer/script_injection.cc:117: for (auto& kv: isolated_worlds) { On 2015/02/09 17:40:25, Devlin wrote: > const auto& Done. https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/scr... extensions/renderer/script_injection.cc:280: DOMActivityLogger::AttachToWorld(world_id, host->id().id()); On 2015/02/09 17:40:25, Devlin wrote: > We almost certainly don't want activity logger to have anything to do with > webuis' injections. Add a check for host_type == EXTENSIONS. https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:391: scoped_ptr<ExtensionConsumer> extension_consumer = GetExtensionConsumer( On 2015/02/09 17:40:25, Devlin wrote: > Not really a big deal, since constructing an ExtensionConsumer is super cheap, > but it'd be nice to avoid it at some point, and just have the injection own one > (and be able to know when the backing extension is removed). > > Not in this patch, though. Add a TODO here. https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:435: HostID(HostID::EXTENSIONS, params.extension_id), On 2015/02/09 17:40:25, Devlin wrote: > Hmm... should the type be HostID::Extensions if it's a webview? The HostID::Extensions is a host type, WEBVIEW vs TAB is a instance type. I am trying to keep the 1:1 relation between HostID and Host, so Host won't contain any information of which kind of instance it will inject scripts on. https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:436: params.is_web_view ? params.instance_id : Host::kDefaultInstanceId, On 2015/02/09 17:40:25, Devlin wrote: > didn't we ensure on the browser side that params.instance_id is always correct > (i.e., it will only be different from kDefaultInstanceId if it's a webview)? Well, in browser side, if it is not <webview>, we didn't explicit set param.instance_id when sending IPC ExtensionMsg_ExecuteCode(see https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...). So I add the check here to make sure a correct default instance is is used for non-webview. Yes, for non-webview(regular-tab), the instance id is always kDefaultInstanceId. The purpose of this id is for assigning isolated world in teh render side. So for tabs from the same host, user scripts will always be injected on the same isolated world, which remains the same behavior as it was. https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/scr... File extensions/renderer/script_injector.h (right): https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/scr... extensions/renderer/script_injector.h:23: class Extension; On 2015/02/09 17:40:25, Devlin wrote: > Don't need this, right? Done. https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:63: extension_id_(script_->extension_id()), On 2015/02/09 17:40:25, Devlin wrote: > Things like this will need to be updated soon. You are absolutely right. I will have a patch to de-couple extensions in the render soon. https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/use... File extensions/renderer/user_script_injector.h (right): https://codereview.chromium.org/885493007/diff/190001/extensions/renderer/use... extensions/renderer/user_script_injector.h:23: class Extension; On 2015/02/09 17:40:25, Devlin wrote: > Here too Done.
https://codereview.chromium.org/885493007/diff/230001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/885493007/diff/230001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:123: const GetNextUniqueIDFunction& function) { This is really weird. I don't know if I like this approach. You could end up with colliding IDs...
https://codereview.chromium.org/885493007/diff/230001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/885493007/diff/230001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:123: const GetNextUniqueIDFunction& function) { On 2015/02/10 06:04:12, Fady Samuel wrote: > This is really weird. I don't know if I like this approach. You could end up > with colliding IDs... Nevermind, I didn't notice that you're passing in the map. This style is a little confusing to read.
https://codereview.chromium.org/885493007/diff/230001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/885493007/diff/230001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:123: const GetNextUniqueIDFunction& function) { On 2015/02/10 15:46:47, Fady Samuel wrote: > On 2015/02/10 06:04:12, Fady Samuel wrote: > > This is really weird. I don't know if I like this approach. You could end up > > with colliding IDs... > > Nevermind, I didn't notice that you're passing in the map. This style is a > little confusing to read. I agree - it's a little over-verbose and hard to parse, particularly with the bind statements in the callers (it's probably bad that to call this function uses almost as many lines of code as the function itself ;)). How about simplifying this to something like: bool GetIdFromMap(const WebViewKey& key, const WebViewKeyToIDMap& map, int default_id, int* result) { WebViewKeyToIDMap::const_iterator iter = map.end(); if (!key.first || !key.second) *result = default_id; else if ((iter = map.find(key)) != map.end()) *result = iter->second; else return false; return true; } and then, e.g., int GetOrGenerateScriptInjection() { WebViewKey key(embedder_process_id, webview_instance_id); WebViewKeyToIDMap& map = script_injection_instance_id_map.Get(); int id = 0; if (!GetIdFromMap(key, map, HostID::kDefaultInstanceId, &id)) { id = ++current_script_injection_instance_id; map[key] = id; } return id; } https://codereview.chromium.org/885493007/diff/230001/extensions/common/host_... File extensions/common/host_id.h (right): https://codereview.chromium.org/885493007/diff/230001/extensions/common/host_... extensions/common/host_id.h:10: #include "base/macros.h" Cleanup. https://codereview.chromium.org/885493007/diff/230001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/885493007/diff/230001/extensions/renderer/scr... extensions/renderer/script_injection.cc:15: #include "extensions/common/extension.h" I would hope we can get rid of some of these extension includes. https://codereview.chromium.org/885493007/diff/230001/extensions/renderer/scr... extensions/renderer/script_injection.cc:304: UMA_HISTOGRAM_TIMES("Extensions.InjectScriptTime", exec_timer.Elapsed()); We should probably avoid recording these metrics for webviews (though it might be worth adding new ones). https://codereview.chromium.org/885493007/diff/230001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/885493007/diff/230001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:57: // TODO(hanxi): let ScriptInjection own an InjectionHost to avoid construct an nit: constructing
Patchset #7 (id:250001) has been deleted
I update the GetOrGenerate...ID() function according to Devlin@'s comment, PTAL, thanks! https://codereview.chromium.org/885493007/diff/230001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/885493007/diff/230001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:123: const GetNextUniqueIDFunction& function) { On 2015/02/10 20:59:53, Devlin wrote: > On 2015/02/10 15:46:47, Fady Samuel wrote: > > On 2015/02/10 06:04:12, Fady Samuel wrote: > > > This is really weird. I don't know if I like this approach. You could end up > > > with colliding IDs... > > > > Nevermind, I didn't notice that you're passing in the map. This style is a > > little confusing to read. > > I agree - it's a little over-verbose and hard to parse, particularly with the > bind statements in the callers (it's probably bad that to call this function > uses almost as many lines of code as the function itself ;)). > > How about simplifying this to something like: > bool GetIdFromMap(const WebViewKey& key, > const WebViewKeyToIDMap& map, > int default_id, > int* result) { > WebViewKeyToIDMap::const_iterator iter = map.end(); > if (!key.first || !key.second) > *result = default_id; > else if ((iter = map.find(key)) != map.end()) > *result = iter->second; > else > return false; > return true; > } > > and then, e.g., > > int GetOrGenerateScriptInjection() { > WebViewKey key(embedder_process_id, webview_instance_id); > WebViewKeyToIDMap& map = script_injection_instance_id_map.Get(); > int id = 0; > if (!GetIdFromMap(key, map, HostID::kDefaultInstanceId, &id)) { > id = ++current_script_injection_instance_id; > map[key] = id; > } > return id; > } I am happy to remove the bind statement as well. This solution sounds good to me. https://codereview.chromium.org/885493007/diff/230001/extensions/common/host_... File extensions/common/host_id.h (right): https://codereview.chromium.org/885493007/diff/230001/extensions/common/host_... extensions/common/host_id.h:10: #include "base/macros.h" On 2015/02/10 20:59:53, Devlin wrote: > Cleanup. Done. https://codereview.chromium.org/885493007/diff/230001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/885493007/diff/230001/extensions/renderer/scr... extensions/renderer/script_injection.cc:15: #include "extensions/common/extension.h" On 2015/02/10 20:59:53, Devlin wrote: > I would hope we can get rid of some of these extension includes. extension.h can be removed from here definitely. Others might need to stay here for a while unless we want to further de-couple extensions from this class. https://codereview.chromium.org/885493007/diff/230001/extensions/renderer/scr... extensions/renderer/script_injection.cc:304: UMA_HISTOGRAM_TIMES("Extensions.InjectScriptTime", exec_timer.Elapsed()); On 2015/02/10 20:59:53, Devlin wrote: > We should probably avoid recording these metrics for webviews (though it might > be worth adding new ones). Add a check for now. https://codereview.chromium.org/885493007/diff/230001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/885493007/diff/230001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:57: // TODO(hanxi): let ScriptInjection own an InjectionHost to avoid construct an On 2015/02/10 20:59:53, Devlin wrote: > nit: constructing Done.
Okay, down to the small stuff :) https://codereview.chromium.org/885493007/diff/270001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/885493007/diff/270001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:63: using GetNextUniqueIDFunction = base::Callback<int(void)>; remove https://codereview.chromium.org/885493007/diff/270001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:115: bool GetIDFromMap(const WebViewKey& key, Add a comment - something like "Looks up the id for the given |key| in |map|, setting |result| with the value. If |key| is invalid, |result| is set to |default_id|. Returns true if |result| was set." https://codereview.chromium.org/885493007/diff/270001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:116: const WebViewKeyToIDMap& map, indentation https://codereview.chromium.org/885493007/diff/270001/extensions/common/host_... File extensions/common/host_id.h (right): https://codereview.chromium.org/885493007/diff/270001/extensions/common/host_... extensions/common/host_id.h:22: : type_(type), id_(id) {} Since we need a .cc for the definition of kDefaultInstanceId, may as well move these there, too. https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/ext... File extensions/renderer/extension_injection_host.cc (right): https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/ext... extensions/renderer/extension_injection_host.cc:13: extension_(extension) { CHECK(extension_) https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/ext... File extensions/renderer/extension_injection_host.h (right): https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/ext... extensions/renderer/extension_injection_host.h:14: // A wrapper class which holds an extension and implements the InjectionHost nit of nits: s/which/that https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/ext... extensions/renderer/extension_injection_host.h:24: const std::string name() const override; It's kinda silly to return a const std::string. Pick either const std::string& or std::string. https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/pro... File extensions/renderer/programmatic_script_injector.cc (right): https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/pro... extensions/renderer/programmatic_script_injector.cc:12: #include "extensions/common/extension.h" Prune https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/scr... extensions/renderer/script_injection.cc:34: using IsolatedWorldKey = std::pair<HostID, int>; Can webuis not share isolated world ids, as extensions do? No javascript executed in the context of one webview should affect javascript executed in the context of another, correct? https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/scr... extensions/renderer/script_injection.cc:260: scripts_run_info, We likely don't to log these metrics, either. A TODO will suffice for now. https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:441: params.is_web_view ? params.instance_id : HostID::kDefaultInstanceId, I think there was a thread on this part that I lost somewhere. Anyway.... We can make this just params.instance_id, since instance_id is always set in ScriptExecutor (to either be a unique instance id or HostID::kDefaultInstanceId). Right?
https://codereview.chromium.org/885493007/diff/270001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/885493007/diff/270001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:63: using GetNextUniqueIDFunction = base::Callback<int(void)>; On 2015/02/11 00:00:48, Devlin wrote: > remove Done. https://codereview.chromium.org/885493007/diff/270001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:115: bool GetIDFromMap(const WebViewKey& key, On 2015/02/11 00:00:48, Devlin wrote: > Add a comment - something like "Looks up the id for the given |key| in |map|, > setting |result| with the value. If |key| is invalid, |result| is set to > |default_id|. Returns true if |result| was set." Thanks:) https://codereview.chromium.org/885493007/diff/270001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:116: const WebViewKeyToIDMap& map, On 2015/02/11 00:00:48, Devlin wrote: > indentation Done. https://codereview.chromium.org/885493007/diff/270001/extensions/common/host_... File extensions/common/host_id.h (right): https://codereview.chromium.org/885493007/diff/270001/extensions/common/host_... extensions/common/host_id.h:22: : type_(type), id_(id) {} On 2015/02/11 00:00:48, Devlin wrote: > Since we need a .cc for the definition of kDefaultInstanceId, may as well move > these there, too. Moved. https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/ext... File extensions/renderer/extension_injection_host.cc (right): https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/ext... extensions/renderer/extension_injection_host.cc:13: extension_(extension) { On 2015/02/11 00:00:48, Devlin wrote: > CHECK(extension_) Do you mean CHECK(extension_.get())? Since we already call extension->id() in line12, it will break if the extension is nullptr. Is this CHECK really necessary here? https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/ext... File extensions/renderer/extension_injection_host.h (right): https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/ext... extensions/renderer/extension_injection_host.h:14: // A wrapper class which holds an extension and implements the InjectionHost On 2015/02/11 00:00:48, Devlin wrote: > nit of nits: s/which/that Done. https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/ext... extensions/renderer/extension_injection_host.h:24: const std::string name() const override; On 2015/02/11 00:00:48, Devlin wrote: > It's kinda silly to return a const std::string. Pick either const std::string& > or std::string. Sorry for the typo, should be const std::string&. https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/pro... File extensions/renderer/programmatic_script_injector.cc (right): https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/pro... extensions/renderer/programmatic_script_injector.cc:12: #include "extensions/common/extension.h" On 2015/02/11 00:00:48, Devlin wrote: > Prune Done. https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/scr... extensions/renderer/script_injection.cc:34: using IsolatedWorldKey = std::pair<HostID, int>; On 2015/02/11 00:00:48, Devlin wrote: > Can webuis not share isolated world ids, as extensions do? No javascript > executed in the context of one webview should affect javascript executed in the > context of another, correct? I believe webUIs don't share isolated world ids, each webUI should have its own isolated world id, as extensions do. Yes, for webviews, javascript executed in one webview shouldn't affect javascript executed on another webview. https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/scr... extensions/renderer/script_injection.cc:260: scripts_run_info, On 2015/02/11 00:00:48, Devlin wrote: > We likely don't to log these metrics, either. A TODO will suffice for now. Added. https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:441: params.is_web_view ? params.instance_id : HostID::kDefaultInstanceId, On 2015/02/11 00:00:48, Devlin wrote: > I think there was a thread on this part that I lost somewhere. Anyway.... > We can make this just params.instance_id, since instance_id is always set in > ScriptExecutor (to either be a unique instance id or > HostID::kDefaultInstanceId). Right? Sorry I might not make myself clear. I add this check just in case HostID::kDefaultInstanceId is assigned to a different value (not 0) in the future, the instance_id for non-webview can always sync with HostID::kDefaultInstanceId and be set a correctly value. This is because we didn't explicitly set instance_id for non-webview, so its value is always 0, but maybe different from HostID::kDefaultInstanceId.
https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/ext... File extensions/renderer/extension_injection_host.cc (right): https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/ext... extensions/renderer/extension_injection_host.cc:13: extension_(extension) { On 2015/02/11 16:00:23, Xi Han wrote: > On 2015/02/11 00:00:48, Devlin wrote: > > CHECK(extension_) > > Do you mean CHECK(extension_.get())? Since we already call extension->id() in > line12, it will break if the extension is nullptr. Is this CHECK really > necessary here? Hm. Good point. https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/scr... extensions/renderer/script_injection.cc:34: using IsolatedWorldKey = std::pair<HostID, int>; On 2015/02/11 16:00:23, Xi Han wrote: > On 2015/02/11 00:00:48, Devlin wrote: > > Can webuis not share isolated world ids, as extensions do? No javascript > > executed in the context of one webview should affect javascript executed in > the > > context of another, correct? > > I believe webUIs don't share isolated world ids, each webUI should have its own > isolated world id, as extensions do. Yes, for webviews, javascript executed in > one webview shouldn't affect javascript executed on another webview. Yeah, each webui should have its own unique isolated world id, but if that's all (which I think it should be), then we can keep the IsoaltedWorldMap keyed on strings (extension or webui id), instead of string/int pairs, right? https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:441: params.is_web_view ? params.instance_id : HostID::kDefaultInstanceId, On 2015/02/11 16:00:24, Xi Han wrote: > On 2015/02/11 00:00:48, Devlin wrote: > > I think there was a thread on this part that I lost somewhere. Anyway.... > > We can make this just params.instance_id, since instance_id is always set in > > ScriptExecutor (to either be a unique instance id or > > HostID::kDefaultInstanceId). Right? > > Sorry I might not make myself clear. I add this check just in case > HostID::kDefaultInstanceId is assigned to a different value (not 0) in the > future, the instance_id for non-webview can always sync with > HostID::kDefaultInstanceId and be set a correctly value. This is because we > didn't explicitly set instance_id for non-webview, so its value is always 0, but > maybe different from HostID::kDefaultInstanceId. But we already explicitly set params.instance_id to be HostID::kDefaultInstanceId (whether or not it's 0) on the browser side (in ScriptExecutor, passed in the value from the tabs api and execute code function). So we already know that params.instance_id will either be HostID::kDefaultInstanceId or will be a unique webview id. We don't need this check here. Right?
Hi Devlin: the point of this CL is to make the isolated world map is based on a key of <string, int> pair. The int is used to identify <webview> and non-<webview>. Without this int, user scripts of <webview> will be injected on the same isolated world as its embedder host, say extensions. The is the current behavior and is wrong, so this CL aims to fix this issue. PTAL, thanks. https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/scr... extensions/renderer/script_injection.cc:34: using IsolatedWorldKey = std::pair<HostID, int>; On 2015/02/11 20:19:44, Devlin wrote: > On 2015/02/11 16:00:23, Xi Han wrote: > > On 2015/02/11 00:00:48, Devlin wrote: > > > Can webuis not share isolated world ids, as extensions do? No javascript > > > executed in the context of one webview should affect javascript executed in > > the > > > context of another, correct? > > > > I believe webUIs don't share isolated world ids, each webUI should have its > own > > isolated world id, as extensions do. Yes, for webviews, javascript executed in > > one webview shouldn't affect javascript executed on another webview. > > Yeah, each webui should have its own unique isolated world id, but if that's all > (which I think it should be), then we can keep the IsoaltedWorldMap keyed on > strings (extension or webui id), instead of string/int pairs, right? I am confused here. <webview> in webui should also have its own isolated world, as <webview> does in extensions. So the string/int pair is still necessary to identify <webview> and non-<webview>, right? https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/scr... File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/885493007/diff/270001/extensions/renderer/scr... extensions/renderer/script_injection_manager.cc:441: params.is_web_view ? params.instance_id : HostID::kDefaultInstanceId, On 2015/02/11 20:19:44, Devlin wrote: > On 2015/02/11 16:00:24, Xi Han wrote: > > On 2015/02/11 00:00:48, Devlin wrote: > > > I think there was a thread on this part that I lost somewhere. Anyway.... > > > We can make this just params.instance_id, since instance_id is always set in > > > ScriptExecutor (to either be a unique instance id or > > > HostID::kDefaultInstanceId). Right? > > > > Sorry I might not make myself clear. I add this check just in case > > HostID::kDefaultInstanceId is assigned to a different value (not 0) in the > > future, the instance_id for non-webview can always sync with > > HostID::kDefaultInstanceId and be set a correctly value. This is because we > > didn't explicitly set instance_id for non-webview, so its value is always 0, > but > > maybe different from HostID::kDefaultInstanceId. > > But we already explicitly set params.instance_id to be > HostID::kDefaultInstanceId (whether or not it's 0) on the browser side (in > ScriptExecutor, passed in the value from the tabs api and execute code > function). So we already know that params.instance_id will either be > HostID::kDefaultInstanceId or will be a unique webview id. We don't need this > check here. Right? As discussed offline, we will remove this check here. Instead, explicitly set instance_id in accessibility_manager.cc.
Patchset #10 (id:330001) has been deleted
Hi Devlin: as discussed offline, a key of std::string for IsolatedWoldMap is enough. I revert unnecessary changes back, PTAL. Thanks!
Super close! :) Please also update the CL description. https://codereview.chromium.org/885493007/diff/350001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_guest.cc (left): https://codereview.chromium.org/885493007/diff/350001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:186: static base::LazyInstance<WebViewKeyToIDMap> web_view_key_to_id_map = Since we're not really adding anything to these files, it's not worth the noise in the git history to change them. https://codereview.chromium.org/885493007/diff/350001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/885493007/diff/350001/extensions/renderer/scr... extensions/renderer/script_injection.cc:204: RequestPermission(); We shouldn't ever call this for webui stuff. There's a bigger problem with the logic here, but for now, let's just make the if: if (ShouldNotifyBrowserOfInjections() && injection_host->id().type() == HostID::EXTENSIONS)
https://codereview.chromium.org/885493007/diff/350001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_guest.cc (left): https://codereview.chromium.org/885493007/diff/350001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_guest.cc:186: static base::LazyInstance<WebViewKeyToIDMap> web_view_key_to_id_map = On 2015/02/13 18:27:08, Devlin wrote: > Since we're not really adding anything to these files, it's not worth the noise > in the git history to change them. Ok, revert all the changes. https://codereview.chromium.org/885493007/diff/350001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/885493007/diff/350001/extensions/renderer/scr... extensions/renderer/script_injection.cc:204: RequestPermission(); On 2015/02/13 18:27:08, Devlin wrote: > We shouldn't ever call this for webui stuff. There's a bigger problem with the > logic here, but for now, let's just make the if: > if (ShouldNotifyBrowserOfInjections() && > injection_host->id().type() == HostID::EXTENSIONS) Added. Also, in my next patch (https://codereview.chromium.org/906493004/patch/200001/210004), I added a similar check in the RequestPersmission() to send a notification to browser with request_id = -1 for webui. Which one is preferred?
lgtm with nits https://codereview.chromium.org/885493007/diff/350001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/885493007/diff/350001/extensions/renderer/scr... extensions/renderer/script_injection.cc:204: RequestPermission(); On 2015/02/13 19:11:29, Xi Han wrote: > On 2015/02/13 18:27:08, Devlin wrote: > > We shouldn't ever call this for webui stuff. There's a bigger problem with > the > > logic here, but for now, let's just make the if: > > if (ShouldNotifyBrowserOfInjections() && > > injection_host->id().type() == HostID::EXTENSIONS) > > Added. Also, in my next patch > (https://codereview.chromium.org/906493004/patch/200001/210004), I added a > similar check in the RequestPersmission() to send a notification to browser with > request_id = -1 for webui. Which one is preferred? In the long run, something different - this will need to be reworked for a couple different reasons. But I can handle that once this patch lands. :) https://codereview.chromium.org/885493007/diff/370001/extensions/common/host_... File extensions/common/host_id.h (right): https://codereview.chromium.org/885493007/diff/370001/extensions/common/host_... extensions/common/host_id.h:15: HostID() {} nit: May as well give a destructor, too. https://codereview.chromium.org/885493007/diff/370001/extensions/renderer/ext... File extensions/renderer/extension_injection_host.h (right): https://codereview.chromium.org/885493007/diff/370001/extensions/renderer/ext... extensions/renderer/extension_injection_host.h:21: // InjectionHost: nit: Let's make these private. https://codereview.chromium.org/885493007/diff/370001/extensions/renderer/ext... extensions/renderer/extension_injection_host.h:30: const Extension* extension() const { return extension_.get(); } nit: needed?
New patchsets have been uploaded after l-g-t-m from rdevlin.cronin@chromium.org
Patchset #12 (id:390001) has been deleted
The CQ bit was checked by hanxi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/885493007/410001
Message was sent while issue was closed.
Committed patchset #12 (id:410001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/a5c856cf3ea4f69aa00c6c24676e0bca3f05962a Cr-Commit-Position: refs/heads/master@{#316283} |
