Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
822453002:
Introduce HostID and de-couple Extensions from "script injection System" [browser side] (Closed)
|
Created:
6 years ago by Xi Han Modified:
5 years, 11 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, extensions-reviews_chromium.org, chromium-apps-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce HostID and de-couple Extensions from "script injection System" [browser side]
The major refactor includes:
- Introduce HostID (a pair of |id, type|) to replace extension_id in browser
side.
- Abstract UserScriptLoader to be a base class, and introduces
a derived class ExtensionUserScriptLoader which is
responsible for loading user scripts for extensions.
- In DeclarativeUserScriptManager, a master is created per
extension/webUI.
- DeclarativeUserScriptManager becomes an
ExtensionRegistryObserver and is responsible for clearing scripts
for master objects when receive OnExensionUnloaded event.
BUG=437566
Committed: https://crrev.com/b88fe3dc1072501bdd105fd95a8b3bc453fd2aa7
Cr-Commit-Position: refs/heads/master@{#313402}
Patch Set 1 : #Patch Set 2 : #
Total comments: 12
Patch Set 3 : gn #
Total comments: 2
Patch Set 4 : nits #
Total comments: 20
Patch Set 5 : Change ConsumerID from struct to class. #Patch Set 6 : format #
Total comments: 18
Patch Set 7 : Fady's comments. #
Total comments: 4
Patch Set 8 : Introduce ConsumerIDFactory #
Total comments: 4
Patch Set 9 : Remove ConsumerIDFactory #
Total comments: 8
Patch Set 10 : Clean up. #Patch Set 11 : Devlin's comments #Patch Set 12 : Fix the test failures. #
Total comments: 76
Patch Set 13 : Change ConsumerID to struct, remove ExtensionDeclarativeUserScriptMaster and avoid passing linked_p… #Patch Set 14 : Change ConsumerID back to class #Patch Set 15 : Change ConsumerID to class; remove usages of linked_ptr, and nits. #
Total comments: 76
Patch Set 16 : Another round of comments of Devlin@. #
Total comments: 28
Patch Set 17 : Update the way to assign instance_id when creating ConsumerID and so on. #Patch Set 18 : Introduce HostID for DeclarativeUserScriptMaser assignment, remove ConsumerID. #
Total comments: 18
Patch Set 19 : Clean up. #
Total comments: 4
Patch Set 20 : nits #Messages
Total messages: 63 (22 generated)
Patchset #1 (id:1) has been deleted
Patchset #5 (id:100001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #2 (id:120001) has been deleted
hanxi@chromium.org changed reviewers: + fsamuel@chromium.org
Hi Fady: please take a look, thanks!
https://codereview.chromium.org/822453002/diff/140001/extensions/common/consu... File extensions/common/consumer.h (right): https://codereview.chromium.org/822453002/diff/140001/extensions/common/consu... extensions/common/consumer.h:5: #ifndef EXTENSIONS_COMMON_CONSUMER_H_ Maybe call this file consumer_id.h? https://codereview.chromium.org/822453002/diff/140001/extensions/common/consu... extensions/common/consumer.h:34: // Similar to extension_id, host_id is a uniquely indentifier for a host, Empty line above please. https://codereview.chromium.org/822453002/diff/140001/extensions/common/consu... extensions/common/consumer.h:37: // A tab or a <webiew> Empty line above please. https://codereview.chromium.org/822453002/diff/140001/extensions/common/consu... extensions/common/consumer.h:39: // For a consumer who owns staticlly-defined user scripts, the Empty line above. https://codereview.chromium.org/822453002/diff/140001/extensions/common/consu... extensions/common/consumer.h:53: void set_id(ConsumerID id) { id_ = id; } const ConsumerID& id https://codereview.chromium.org/822453002/diff/140001/extensions/common/consu... extensions/common/consumer.h:54: ConsumerID id() { return id_; } const ConsumerID& id() const { return id_; } https://codereview.chromium.org/822453002/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/extension_user_script_loader.h (right): https://codereview.chromium.org/822453002/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.h:43: static bool LoadScriptContent(const ConsumerID& consumer_id, Move this above // UserScriptLoader implementation.
Patchset #4 (id:180001) has been deleted
https://codereview.chromium.org/822453002/diff/140001/extensions/common/consu... File extensions/common/consumer.h (right): https://codereview.chromium.org/822453002/diff/140001/extensions/common/consu... extensions/common/consumer.h:5: #ifndef EXTENSIONS_COMMON_CONSUMER_H_ On 2014/12/22 21:48:46, Fady Samuel wrote: > Maybe call this file consumer_id.h? I still feel consumer.h is better, since there is a class Consumer in this file. https://codereview.chromium.org/822453002/diff/140001/extensions/common/consu... extensions/common/consumer.h:34: // Similar to extension_id, host_id is a uniquely indentifier for a host, On 2014/12/22 21:48:46, Fady Samuel wrote: > Empty line above please. Done. https://codereview.chromium.org/822453002/diff/140001/extensions/common/consu... extensions/common/consumer.h:37: // A tab or a <webiew> On 2014/12/22 21:48:46, Fady Samuel wrote: > Empty line above please. Done. https://codereview.chromium.org/822453002/diff/140001/extensions/common/consu... extensions/common/consumer.h:39: // For a consumer who owns staticlly-defined user scripts, the On 2014/12/22 21:48:46, Fady Samuel wrote: > Empty line above. Done. https://codereview.chromium.org/822453002/diff/140001/extensions/common/consu... extensions/common/consumer.h:53: void set_id(ConsumerID id) { id_ = id; } On 2014/12/22 21:48:46, Fady Samuel wrote: > const ConsumerID& id Great catch, thanks! https://codereview.chromium.org/822453002/diff/140001/extensions/common/consu... extensions/common/consumer.h:54: ConsumerID id() { return id_; } On 2014/12/22 21:48:46, Fady Samuel wrote: > const ConsumerID& id() const { return id_; } Done. https://codereview.chromium.org/822453002/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/extension_user_script_loader.h (right): https://codereview.chromium.org/822453002/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.h:43: static bool LoadScriptContent(const ConsumerID& consumer_id, On 2014/12/22 21:48:46, Fady Samuel wrote: > Move this above // UserScriptLoader implementation. Moved.
https://codereview.chromium.org/822453002/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/declarative_user_script_manager.cc (right): https://codereview.chromium.org/822453002/diff/200001/chrome/browser/extensio... chrome/browser/extensions/declarative_user_script_manager.cc:13: current_instance_id_(ConsumerID::kDefaultInstanceID + 1) { Why? https://codereview.chromium.org/822453002/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/declarative_user_script_master.cc (right): https://codereview.chromium.org/822453002/diff/200001/chrome/browser/extensio... chrome/browser/extensions/declarative_user_script_master.cc:18: consumer_id) { nit: This can go on the same line as the above. https://codereview.chromium.org/822453002/diff/200001/extensions/common/consu... File extensions/common/consumer.h (right): https://codereview.chromium.org/822453002/diff/200001/extensions/common/consu... extensions/common/consumer.h:13: enum HostType { EXTENSIONS, WEBUI }; Do we support enum classes in chromium? https://codereview.chromium.org/822453002/diff/200001/extensions/common/consu... extensions/common/consumer.h:25: ConsumerID(HostType host_type, Once we create a ConsumerID, do its fields ever change? If not, maybe it makes sense to enforce that? Maybe we can make this thing a class, make all the fields const privates, and only add getter accessors? https://codereview.chromium.org/822453002/diff/200001/extensions/common/consu... extensions/common/consumer.h:39: // A tab or a <webiew> period at the end of the comment. https://codereview.chromium.org/822453002/diff/200001/extensions/common/consu... extensions/common/consumer.h:42: // For a consumer who owns staticlly-defined user scripts, the For a consumer that owns statically defined user scripts, the |instance_id| is 0. https://codereview.chromium.org/822453002/diff/200001/extensions/common/consu... extensions/common/consumer.h:44: // For a consumer who owns declarative user scripts, the |instance_id| change who to that. https://codereview.chromium.org/822453002/diff/200001/extensions/common/consu... extensions/common/consumer.h:56: void set_id(const ConsumerID& id) { id_ = id; } Why is the ConsumerID not in the constructor? https://codereview.chromium.org/822453002/diff/200001/extensions/common/user_... File extensions/common/user_script.h (right): https://codereview.chromium.org/822453002/diff/200001/extensions/common/user_... extensions/common/user_script.h:199: void set_extension_id(const std::string& id) { extension_id_ = id; } Can this be removed? https://codereview.chromium.org/822453002/diff/200001/extensions/common/user_... extensions/common/user_script.h:282: std::string extension_id_; Can this be removed?
Patchset #5 (id:220001) has been deleted
Hi Fady: PTAL, thanks. https://codereview.chromium.org/822453002/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/declarative_user_script_manager.cc (right): https://codereview.chromium.org/822453002/diff/200001/chrome/browser/extensio... chrome/browser/extensions/declarative_user_script_manager.cc:13: current_instance_id_(ConsumerID::kDefaultInstanceID + 1) { On 2015/01/06 13:55:11, Fady Samuel wrote: > Why? Updated. https://codereview.chromium.org/822453002/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/declarative_user_script_master.cc (right): https://codereview.chromium.org/822453002/diff/200001/chrome/browser/extensio... chrome/browser/extensions/declarative_user_script_master.cc:18: consumer_id) { On 2015/01/06 13:55:11, Fady Samuel wrote: > nit: This can go on the same line as the above. Done. https://codereview.chromium.org/822453002/diff/200001/extensions/common/consu... File extensions/common/consumer.h (right): https://codereview.chromium.org/822453002/diff/200001/extensions/common/consu... extensions/common/consumer.h:13: enum HostType { EXTENSIONS, WEBUI }; On 2015/01/06 13:55:11, Fady Samuel wrote: > Do we support enum classes in chromium? Good point, I think there is example about enum classes, but it is better to define it within ConsumerID. Moved. https://codereview.chromium.org/822453002/diff/200001/extensions/common/consu... extensions/common/consumer.h:25: ConsumerID(HostType host_type, On 2015/01/06 13:55:11, Fady Samuel wrote: > Once we create a ConsumerID, do its fields ever change? If not, maybe it makes > sense to enforce that? > > Maybe we can make this thing a class, make all the fields const privates, and > only add getter accessors? As discussed offline, declare ConsumerID as a class, and only add getters. https://codereview.chromium.org/822453002/diff/200001/extensions/common/consu... extensions/common/consumer.h:39: // A tab or a <webiew> On 2015/01/06 13:55:11, Fady Samuel wrote: > period at the end of the comment. Done. https://codereview.chromium.org/822453002/diff/200001/extensions/common/consu... extensions/common/consumer.h:42: // For a consumer who owns staticlly-defined user scripts, the On 2015/01/06 13:55:11, Fady Samuel wrote: > For a consumer that owns statically defined user scripts, the |instance_id| is > 0. Done. https://codereview.chromium.org/822453002/diff/200001/extensions/common/consu... extensions/common/consumer.h:44: // For a consumer who owns declarative user scripts, the |instance_id| On 2015/01/06 13:55:11, Fady Samuel wrote: > change who to that. Done. https://codereview.chromium.org/822453002/diff/200001/extensions/common/consu... extensions/common/consumer.h:56: void set_id(const ConsumerID& id) { id_ = id; } On 2015/01/06 13:55:11, Fady Samuel wrote: > Why is the ConsumerID not in the constructor? Think about it again, I remove the member variable ConsumerID id_ from class Consumer for now. The Consumer is more likely an embedder that holds the entities who request the API. We can consider extensions is an embedder, and it is created before the a ConsumerID is produced for script injection. ConsumerID will be used to represent the entities (e.g., <webview>, tab,...), and Consumer will just be an interface to represent all kinds of embedders. I will just leave it as it is now, and maybe update it when more functionalities are needed in the following CLs. https://codereview.chromium.org/822453002/diff/200001/extensions/common/user_... File extensions/common/user_script.h (right): https://codereview.chromium.org/822453002/diff/200001/extensions/common/user_... extensions/common/user_script.h:199: void set_extension_id(const std::string& id) { extension_id_ = id; } On 2015/01/06 13:55:11, Fady Samuel wrote: > Can this be removed? Removed. https://codereview.chromium.org/822453002/diff/200001/extensions/common/user_... extensions/common/user_script.h:282: std::string extension_id_; On 2015/01/06 13:55:11, Fady Samuel wrote: > Can this be removed? Removed.
Another set of comments. https://codereview.chromium.org/822453002/diff/260001/chrome/browser/extensio... File chrome/browser/extensions/declarative_user_script_manager.h (right): https://codereview.chromium.org/822453002/diff/260001/chrome/browser/extensio... chrome/browser/extensions/declarative_user_script_manager.h:53: ConsumerID* GenerateConsumerID(const std::string& id); Return scoped_ptr<ConsumerID> to avoid accidentally leaking this thing. https://codereview.chromium.org/822453002/diff/260001/extensions/common/consu... File extensions/common/consumer.h (right): https://codereview.chromium.org/822453002/diff/260001/extensions/common/consu... extensions/common/consumer.h:25: ConsumerID(); Is this necessary? https://codereview.chromium.org/822453002/diff/260001/extensions/common/consu... extensions/common/consumer.h:42: HostType host_type_; Mark as const HostType host_type_. This tells developers and compilers that this field will not change after construction. https://codereview.chromium.org/822453002/diff/260001/extensions/common/consu... extensions/common/consumer.h:46: std::string host_id_; Mark as const. https://codereview.chromium.org/822453002/diff/260001/extensions/common/consu... extensions/common/consumer.h:49: InstanceType instance_type_; Mark as const. https://codereview.chromium.org/822453002/diff/260001/extensions/common/consu... extensions/common/consumer.h:56: int instance_id_; Mark as const. https://codereview.chromium.org/822453002/diff/260001/extensions/common/user_... File extensions/common/user_script.cc (right): https://codereview.chromium.org/822453002/diff/260001/extensions/common/user_... extensions/common/user_script.cc:232: ConsumerID* UserScript::UnpickleConsumerID(const ::Pickle& pickle, Return a scoped_ptr https://codereview.chromium.org/822453002/diff/260001/extensions/common/user_... File extensions/common/user_script.h (right): https://codereview.chromium.org/822453002/diff/260001/extensions/common/user_... extensions/common/user_script.h:199: const std::string& extension_id() const; This is no longer a trivial accessor and so it must be renamed to GetExtensionID https://codereview.chromium.org/822453002/diff/260001/extensions/common/user_... extensions/common/user_script.h:281: linked_ptr<ConsumerID> consumer_id_; Why is this a linked_ptr? Can it be a scoped_ptr?
Hi Fady: I would like to add "const" keyword or use scoped_ptr, but they have their own troubles, please see my comments in details. If you have good suggestions, I would like to hear. Thanks! https://codereview.chromium.org/822453002/diff/260001/chrome/browser/extensio... File chrome/browser/extensions/declarative_user_script_manager.h (right): https://codereview.chromium.org/822453002/diff/260001/chrome/browser/extensio... chrome/browser/extensions/declarative_user_script_manager.h:53: ConsumerID* GenerateConsumerID(const std::string& id); On 2015/01/07 19:19:40, Fady Samuel wrote: > Return scoped_ptr<ConsumerID> to avoid accidentally leaking this thing. Good point, but I realize this function hasn't been used anywhere. Removed. https://codereview.chromium.org/822453002/diff/260001/extensions/common/consu... File extensions/common/consumer.h (right): https://codereview.chromium.org/822453002/diff/260001/extensions/common/consu... extensions/common/consumer.h:25: ConsumerID(); On 2015/01/07 19:19:40, Fady Samuel wrote: > Is this necessary? Yes, the ConsumerID() creates an empty ID for extensions with a empty extension_id. It is used by SharedUserScriptMaster(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/extensions/shared_user_script_master.cc&rcl=1420565673&l=16). https://codereview.chromium.org/822453002/diff/260001/extensions/common/consu... extensions/common/consumer.h:42: HostType host_type_; On 2015/01/07 19:19:40, Fady Samuel wrote: > Mark as const HostType host_type_. > > This tells developers and compilers that this field will not change after > construction. I tried the "const" keyword, but a "const" member variable has its own troubles: the compiler generated assignment operator is disabled. Without an assignment operator a class won't work with STL. https://codereview.chromium.org/822453002/diff/260001/extensions/common/consu... extensions/common/consumer.h:46: std::string host_id_; On 2015/01/07 19:19:40, Fady Samuel wrote: > Mark as const. Same as HostType. https://codereview.chromium.org/822453002/diff/260001/extensions/common/consu... extensions/common/consumer.h:49: InstanceType instance_type_; On 2015/01/07 19:19:40, Fady Samuel wrote: > Mark as const. Same as HostType. https://codereview.chromium.org/822453002/diff/260001/extensions/common/consu... extensions/common/consumer.h:56: int instance_id_; On 2015/01/07 19:19:40, Fady Samuel wrote: > Mark as const. Same as HostType. https://codereview.chromium.org/822453002/diff/260001/extensions/common/user_... File extensions/common/user_script.cc (right): https://codereview.chromium.org/822453002/diff/260001/extensions/common/user_... extensions/common/user_script.cc:232: ConsumerID* UserScript::UnpickleConsumerID(const ::Pickle& pickle, On 2015/01/07 19:19:40, Fady Samuel wrote: > Return a scoped_ptr Change it to linked_ptr, due to the type of the member variable consumer_id_. https://codereview.chromium.org/822453002/diff/260001/extensions/common/user_... File extensions/common/user_script.h (right): https://codereview.chromium.org/822453002/diff/260001/extensions/common/user_... extensions/common/user_script.h:199: const std::string& extension_id() const; On 2015/01/07 19:19:40, Fady Samuel wrote: > This is no longer a trivial accessor and so it must be renamed to GetExtensionID Renamed. https://codereview.chromium.org/822453002/diff/260001/extensions/common/user_... extensions/common/user_script.h:281: linked_ptr<ConsumerID> consumer_id_; On 2015/01/07 19:19:40, Fady Samuel wrote: > Why is this a linked_ptr? Can it be a scoped_ptr? No, it can't be a scoped_ptr, since scoped_ptr is not copyable. There are many places that objects of UserScript be stored in STL containers, for example: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte....
https://codereview.chromium.org/822453002/diff/280001/chrome/browser/extensio... File chrome/browser/extensions/declarative_user_script_manager.cc (left): https://codereview.chromium.org/822453002/diff/280001/chrome/browser/extensio... chrome/browser/extensions/declarative_user_script_manager.cc:19: DeclarativeUserScriptManager::GetDeclarativeUserScriptMasterByID( You shouldn't need this. https://codereview.chromium.org/822453002/diff/280001/chrome/browser/extensio... File chrome/browser/extensions/declarative_user_script_manager.h (right): https://codereview.chromium.org/822453002/diff/280001/chrome/browser/extensio... chrome/browser/extensions/declarative_user_script_manager.h:41: using RulesRegistryKeyToIDMap = std::map<RulesRegistryKey, ConsumerID>; Make this std::map<RulesRegistry, linked_ptr<ConsumerID>>;
Patchset #8 (id:300001) has been deleted
Patchset #8 (id:320001) has been deleted
https://codereview.chromium.org/822453002/diff/280001/chrome/browser/extensio... File chrome/browser/extensions/declarative_user_script_manager.cc (left): https://codereview.chromium.org/822453002/diff/280001/chrome/browser/extensio... chrome/browser/extensions/declarative_user_script_manager.cc:19: DeclarativeUserScriptManager::GetDeclarativeUserScriptMasterByID( On 2015/01/08 16:12:59, Fady Samuel wrote: > You shouldn't need this. Removed. https://codereview.chromium.org/822453002/diff/280001/chrome/browser/extensio... File chrome/browser/extensions/declarative_user_script_manager.h (right): https://codereview.chromium.org/822453002/diff/280001/chrome/browser/extensio... chrome/browser/extensions/declarative_user_script_manager.h:41: using RulesRegistryKeyToIDMap = std::map<RulesRegistryKey, ConsumerID>; On 2015/01/08 16:12:59, Fady Samuel wrote: > Make this std::map<RulesRegistry, linked_ptr<ConsumerID>>; Done. Also move the keys conversion part to ConsumerIDFactory class.
Getting close. https://codereview.chromium.org/822453002/diff/340001/chrome/browser/extensio... File chrome/browser/extensions/consumer_id_factory.h (right): https://codereview.chromium.org/822453002/diff/340001/chrome/browser/extensio... chrome/browser/extensions/consumer_id_factory.h:24: static const ConsumerID& Create(const RequestContentScriptKey& key, If you have a factory, it should not be possible to directly construct the object. https://codereview.chromium.org/822453002/diff/340001/extensions/common/user_... File extensions/common/user_script.h (right): https://codereview.chromium.org/822453002/diff/340001/extensions/common/user_... extensions/common/user_script.h:199: ConsumerID* consumer_id() const { return consumer_id_.get(); } nit: const ConsumerID*
https://codereview.chromium.org/822453002/diff/340001/chrome/browser/extensio... File chrome/browser/extensions/consumer_id_factory.h (right): https://codereview.chromium.org/822453002/diff/340001/chrome/browser/extensio... chrome/browser/extensions/consumer_id_factory.h:24: static const ConsumerID& Create(const RequestContentScriptKey& key, On 2015/01/08 21:03:08, Fady Samuel wrote: > If you have a factory, it should not be possible to directly construct the > object. Remove this class, and move the map to RequstContentScript. https://codereview.chromium.org/822453002/diff/340001/extensions/common/user_... File extensions/common/user_script.h (right): https://codereview.chromium.org/822453002/diff/340001/extensions/common/user_... extensions/common/user_script.h:199: ConsumerID* consumer_id() const { return consumer_id_.get(); } On 2015/01/08 21:03:08, Fady Samuel wrote: > nit: const ConsumerID* Done.
Feel free to add a new reviewer once you address these comments. https://codereview.chromium.org/822453002/diff/360001/extensions/common/consu... File extensions/common/consumer.cc (right): https://codereview.chromium.org/822453002/diff/360001/extensions/common/consu... extensions/common/consumer.cc:17: ConsumerID::ConsumerID(const ConsumerID& other) What about having a single InvalidConsumerID? See https://code.google.com/p/chromium/codesearch#chromium/src/url/gurl.cc&q=GURL... https://codereview.chromium.org/822453002/diff/360001/extensions/common/consu... extensions/common/consumer.cc:27: int instance_id) Get rid of this. https://codereview.chromium.org/822453002/diff/360001/extensions/common/consu... extensions/common/consumer.cc:31: instance_id_(instance_id) { instance_id_(++current_instance_id_) https://codereview.chromium.org/822453002/diff/360001/extensions/common/consu... File extensions/common/consumer.h (right): https://codereview.chromium.org/822453002/diff/360001/extensions/common/consu... extensions/common/consumer.h:22: // Default instance ID of a consumer who owns staticlly-defined user scripts. nit: statically
hanxi@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
rdevlin.cronin@chromium.org: Please review all changes. Thanks a lot! https://codereview.chromium.org/822453002/diff/360001/extensions/common/consu... File extensions/common/consumer.cc (right): https://codereview.chromium.org/822453002/diff/360001/extensions/common/consu... extensions/common/consumer.cc:17: ConsumerID::ConsumerID(const ConsumerID& other) On 2015/01/09 22:07:24, Fady Samuel wrote: > What about having a single InvalidConsumerID? See > > https://code.google.com/p/chromium/codesearch#chromium/src/url/gurl.cc&q=GURL... Good idea, updated. https://codereview.chromium.org/822453002/diff/360001/extensions/common/consu... extensions/common/consumer.cc:27: int instance_id) On 2015/01/09 22:07:24, Fady Samuel wrote: > Get rid of this. As discuss offline, this constructor is still needed when pickle and unpickle user scripts from shared memory regions. https://codereview.chromium.org/822453002/diff/360001/extensions/common/consu... extensions/common/consumer.cc:31: instance_id_(instance_id) { On 2015/01/09 22:07:24, Fady Samuel wrote: > instance_id_(++current_instance_id_) Same reason as above. https://codereview.chromium.org/822453002/diff/360001/extensions/common/consu... File extensions/common/consumer.h (right): https://codereview.chromium.org/822453002/diff/360001/extensions/common/consu... extensions/common/consumer.h:22: // Default instance ID of a consumer who owns staticlly-defined user scripts. On 2015/01/09 22:07:24, Fady Samuel wrote: > nit: statically Done.
Whoa, that's a big patch. First pass. https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:255: const ConsumerID& GetConsumerID( nit: please doc this function briefly. https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:260: request_content_script_key_to_id_map_.Get().find(key); nit: Get()s are cheap, but not free. Might be better to cache a reference to the map here. https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:267: return *(consumer_id.get()); nit: linked_ptr overrides the * operator, so you can just do return *consumer_id; https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.h (right): https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.h:162: linked_ptr<DeclarativeUserScriptMaster> master_; Linked ptrs are typically only used in, e.g., std:: containers. What's the reason for using a linked ptr here? https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... File chrome/browser/extensions/declarative_user_script_manager.h (right): https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/declarative_user_script_manager.h:35: linked_ptr<DeclarativeUserScriptMaster> CreateDeclarativeUserScriptMaster( Again, why returning a linked ptr here? https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/declarative_user_script_manager.h:38: // A map of DeclarativeUserScriptMasters for ConsumerIDs; nit: fix line wrapping. https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... File chrome/browser/extensions/extension_declarative_user_script_master.cc (right): https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/extension_declarative_user_script_master.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. Update. https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... File chrome/browser/extensions/extension_declarative_user_script_master.h (right): https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/extension_declarative_user_script_master.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015 now. (Wow, time flies...) https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... File chrome/browser/extensions/extension_user_script_loader.cc (right): https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:33: using extensions::ExtensionsBrowserClient; You're in the extensions namespace; probably don't need this using :) https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:39: void VerifyContent(scoped_refptr<ContentVerifier> verifier, comment. https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:40: const ConsumerID* consumer_id, Why not const &? https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:111: const ConsumerID* consumer_id, Can this be const &? https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:112: UserScript::File* script_file, Can we DCHECK script_file? https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:120: int resource_id; nit: initialize this to a reasonable value (e.g., 0). https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:153: if (!error.empty()) { nit: no brackets around single-line ifs. https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:162: } else { nit: no brackets. https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:178: if (id.host_id().compare(extension->id()) == 0) why not if id.host_id == extension->id()? https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:192: void ExtensionUserScriptLoader::UpdateConsumersInfo() { Please match function orders to the header's declaration order. https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:198: it->host_id(), ExtensionRegistry::EVERYTHING); Do we want to include extensions that are blacklisted, disabled, etc? https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... File chrome/browser/extensions/extension_user_script_loader.h (right): https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.h:10: #include "content/public/browser/notification_observer.h" Don't need these, right? https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.h:14: #include "extensions/common/extension_set.h" This, too. https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.h:34: bool listen_for_extension_system_loaded); Please document listen_for_extension_system_loaded (what are the implications, and when is it true?) https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.h:45: bool ready() override; virtual shouldn't be unix_hacker_style. https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... File chrome/browser/extensions/user_script_loader.cc (right): https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:39: return NULL; nit: nullptr now. https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:123: UserScriptLoader::LoadUserScriptsFunctionCallback function, why not const&? https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... File chrome/browser/extensions/user_script_loader.h (right): https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:84: // Update |consumers_info_| to contain info for each element of nit: Function descriptions should be descriptive, rather than imperative. That is, this should say "Updates" rather than "Update". This goes for other places in the CL, too. (This one's hard to remember - I often have to double-check myself, too. :)) https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:86: virtual void UpdateConsumersInfo() {} Please declare virtual function bodies in the .cc. https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:89: virtual bool ready(); Ben can evangelize on this way more than I can, but sometimes, subclasses make things much harder to understand and maintain. Many times, a delegate is preferred. Could we do that instead, here? Or is there a strong reason to have these as subclasses? https://codereview.chromium.org/822453002/diff/400001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:101: virtual void SendUpdate(content::RenderProcessHost* process, Why are these empty bodies, rather than pure-virtual? https://codereview.chromium.org/822453002/diff/400001/extensions/common/consu... File extensions/common/consumer.cc (right): https://codereview.chromium.org/822453002/diff/400001/extensions/common/consu... extensions/common/consumer.cc:37: #ifdef WIN32 This whole set definitely needs to be commented. https://codereview.chromium.org/822453002/diff/400001/extensions/common/consu... extensions/common/consumer.cc:61: extra newline. https://codereview.chromium.org/822453002/diff/400001/extensions/common/consu... File extensions/common/consumer.h (right): https://codereview.chromium.org/822453002/diff/400001/extensions/common/consu... extensions/common/consumer.h:14: // a <webview> created by WebUI, ... ...? https://codereview.chromium.org/822453002/diff/400001/extensions/common/consu... extensions/common/consumer.h:14: // a <webview> created by WebUI, ... Maybe mention that this is immutable. https://codereview.chromium.org/822453002/diff/400001/extensions/common/consu... extensions/common/consumer.h:15: class ConsumerID { Why is this a class, instead of an immutable struct? (Not saying it needs to be, just curious of reasoning). https://codereview.chromium.org/822453002/diff/400001/extensions/common/consu... extensions/common/consumer.h:32: // Returns a reference to a singleton empty ConsumerID. Document why we need this (instead of just passing around a constructed empty one). https://codereview.chromium.org/822453002/diff/400001/extensions/common/consu... extensions/common/consumer.h:46: static int current_instance_id_; Could this be in an anonymous namespace in the .cc? https://codereview.chromium.org/822453002/diff/400001/extensions/common/consu... extensions/common/consumer.h:48: // Extension, or WebUI, ... ...? https://codereview.chromium.org/822453002/diff/400001/extensions/common/consu... extensions/common/consumer.h:51: // Similar to extension_id, host_id is a uniquely indentifier for a host, unique, not uniquely. https://codereview.chromium.org/822453002/diff/400001/extensions/common/user_... File extensions/common/user_script.h (right): https://codereview.chromium.org/822453002/diff/400001/extensions/common/user_... extensions/common/user_script.h:282: linked_ptr<ConsumerID> consumer_id_; Under what conditions would ConsumerID be null? Is this just so it can be set after construction?
Patchset #11 (id:400001) has been deleted
Hi Robert, I accidentally delete the patchset that have your comments and my replies, and I can not revert the deletion back:( Sorry about that. There are some important replies: 1) the struct vs class about ConsumerID. Basically, I think they are similar, the only differences is the default type of data of a struct is public, and the default for a class is private. Since we don't want the outside changed the values of the local variables after creation, we use class to enhance this purpose. Do you have strong preference to use struct here? 2) The use of linked_ptr. ConsumerID objects are created in the DeclarativeUserScriptManager (https://codereview.chromium.org/822453002/diff/440001/chrome/browser/extensio...) and stored in a map. Starting from there, we used a linked_ptr to store them in the map. Along with this code path, a linked_ptr<ConsumerID> is used as return type for function like DeclarativeUserScriptManager::CreateDeclarativeUserScriptMaster(). Also, we store a linked_ptr<ConsumerId> in UserScript, since its value will be updated after construction. After all, I believe a smart pointer is better than a raw pointer, since it shows the lifetime of a object has been managed. 3) As discussed offline, we leave the subclass (ExtensionUserScriptLoader) as it is now. Maybe update it later if we find this pattern doesn't clean when we add webUI in the future. Sorry for the inconvenience that let you read my replies in this way again.
Patchset #11 (id:420001) has been deleted
https://codereview.chromium.org/822453002/diff/440001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/822453002/diff/440001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:263: request_content_script_key_to_id_map_.Get(); Store it locally.
Patchset #11 (id:440001) has been deleted
On 2015/01/13 17:26:28, Xi Han wrote: > Hi Robert, I accidentally delete the patchset that have your comments and my > replies, and I can not revert the deletion back:( Sorry about that. It happens, unfortunately. :/ Only thing to do is be more careful in the future - it's usually a mistake we all make around once. ;) > There are some important replies: > 1) the struct vs class about ConsumerID. Basically, I think they are similar, > the only differences is the default type of data of a struct is public, and the > default for a class is private. Since we don't want the outside changed the > values of the local variables after creation, we use class to enhance this > purpose. Do you have strong preference to use struct here? The C++ difference between a struct and a class is, as you point out, the default access level of the object. That said, there is a "conceptual" Google/Chromium difference, which is that structs are basically a collection of relatively simple data, and classes have functionality. Since the ConsumerID seems more akin to the former, I think it would be better as a struct (you can make sure outsiders don't change the values by making each member const). > 2) The use of linked_ptr. ConsumerID objects are created in the > DeclarativeUserScriptManager > (https://codereview.chromium.org/822453002/diff/440001/chrome/browser/extensio...) > and stored in a map. Starting from there, we used a linked_ptr to store them in > the map. Along with this code path, a linked_ptr<ConsumerID> is used as return > type for function like > DeclarativeUserScriptManager::CreateDeclarativeUserScriptMaster(). Also, we > store a linked_ptr<ConsumerId> in UserScript, since its value will be updated > after construction. After all, I believe a smart pointer is better than a raw > pointer, since it shows the lifetime of a object has been managed. linked_ptrs are really designed to be used in STL containers, and not passed around the codebase as linked_ptrs. They are grossly inefficient (the comments on the class say if there's ever more than 2 or 3 pointers, they're a bad choice), and they are a little more fragile than other smart pointers. I think the better solution is, if you need to pass them around, pass them weakly with a well-documented lifespan. That said, ConsumerIDs can maybe just be copied when we need them. I'll keep an eye out in my next review for where these should be changed. > 3) As discussed offline, we leave the subclass (ExtensionUserScriptLoader) as it > is now. Maybe update it later if we find this pattern doesn't clean when we add > webUI in the future. Acknowledged. > Sorry for the inconvenience that let you read my replies in this way again. No worries. :)
https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:247: std::map<RequestContentScriptKey, linked_ptr<ConsumerID>>; ConsumerIDs are cheap enough to copy that it's not worth the potential agony to linked_ptr them, since we won't be doing it a lot (especially since GetConsumerID already returns a const &). Let's just make this a map<RequestContentScriptKey, ConsumerID> https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:252: static base::LazyInstance<RequestContentScriptKeyToIDMap> static inside anonymous namespace is redundant -- remove the static. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:254: extra newline. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.h (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.h:162: linked_ptr<DeclarativeUserScriptMaster> master_; I don't think it's wise to make this a linked_ptr. Instead, let's document why the weak pointer is safe. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/declarative_user_script_manager.h (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/declarative_user_script_manager.h:27: linked_ptr<DeclarativeUserScriptMaster> GetDeclarativeUserScriptMasterByID( Again, no linked_ptr (assume this goes for all DeclarativeUserScriptMaster uses). https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/extension_declarative_user_script_master.cc (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/extension_declarative_user_script_master.cc:14: ExtensionDeclarativeUserScriptMaster::ExtensionDeclarativeUserScriptMaster( Actually, this class makes me sad. Will the DeclarativeUserScriptManager always have knowledge of Extensions, as it does now? If so, could we have it watch OnExtensionUnloaded and delete the corresponding DeclarativeUserScriptMaster, so we don't need this subclass? https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/extension_declarative_user_script_master.h (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/extension_declarative_user_script_master.h:12: #include "extensions/common/consumer.h" Check your includes. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/extension_declarative_user_script_master.h:30: // ExtensionRegistryObserver implementation. nit: prefer // ExtensionRegistryObserver: https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/extension_user_script_loader.cc (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. check year (everywhere) https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:35: class ExtensionsBrowserClient; why do you need this? https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:70: weak_factory_.GetWeakPtr())); indentation https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:71: } else our bracketing rules are complex. if (one_line_if_body) NoBrackets() if (multi_line_if_body) { UseBrackets() ... } if (multi_line_if) { UseBrackets() ... } else if (else_with_previous_multi_line_if) { StillUseBrackets() } I believe the linter calls this rule "If an else has brackets on one side, it must also have brackets on the other side". https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:80: for (std::set<ConsumerID>::const_iterator it = changed_consumers_.begin(); Let's C++11 this: for (const ConsumerID& consumer_id : changed_consumers_) { ... } https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/extension_user_script_loader.h (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. year https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.h:39: static bool LoadScriptContent(const ConsumerID& consumer_id, Can this go in an anonymous namespace in the .cc? https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.h:44: // UserScriptLoader implementation. nit: prefer // UserScriptLoader: (goes for everywhere) https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/user_script_loader.cc (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:405: bool UserScriptLoader::LoadScriptContent( Is this correct for non-extension scripts? If not, can we simplify this whole "LoadScriptContent()" and "GetLoadScriptContentFunction" chain? https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/user_script_loader.h (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:89: virtual bool Ready() = 0; Let's get rid of this one, and instead expose a non-virtual SetReady() function. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:97: virtual ContentVerifier* GetContentVerifier(); Can we remove this and initialize the UserScriptLoader with a ContentVerifier? https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:112: void AttemptLoad(); Make this private and call it from the to-be-created SetReady() https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:115: void OnScriptsLoaded(scoped_ptr<UserScriptList> user_scripts, Private. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:119: scoped_ptr<UserScriptList> user_scripts_; Private https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:122: ConsumersInfo consumers_info_; I think this might be better to have an exposed Add/Remove, and make this private. We try to avoid protected data members whenever possible. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:126: ConsumerIDSet changed_consumers_; Make this private and pass it into UpdateConsumersInfo() https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:129: Profile* profile_; Expose a getter, and make this private. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:133: ConsumerID consumer_id_; Expose a getter, and make this private. https://codereview.chromium.org/822453002/diff/480001/extensions/common/consu... File extensions/common/consumer.h (right): https://codereview.chromium.org/822453002/diff/480001/extensions/common/consu... extensions/common/consumer.h:14: // For example: an extension, a <webview> created by an extension, This needs to be re-worded. Maybe something like: A ConsumerID could be used for an extension... https://codereview.chromium.org/822453002/diff/480001/extensions/common/consu... extensions/common/consumer.h:16: class ConsumerID { See also main comment on making this a struct. https://codereview.chromium.org/822453002/diff/480001/extensions/common/consu... extensions/common/consumer.h:34: static const ConsumerID& EmptyConsumerID(); Why do we need this, instead of just creating a new empty one? https://codereview.chromium.org/822453002/diff/480001/extensions/common/user_... File extensions/common/user_script.h (right): https://codereview.chromium.org/822453002/diff/480001/extensions/common/user_... extensions/common/user_script.h:282: linked_ptr<ConsumerID> consumer_id_; Let's not have these be linked_ptrs.
Hi Robert: thanks a lot for your comments. PTAK. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:247: std::map<RequestContentScriptKey, linked_ptr<ConsumerID>>; On 2015/01/14 16:45:08, Devlin wrote: > ConsumerIDs are cheap enough to copy that it's not worth the potential agony to > linked_ptr them, since we won't be doing it a lot (especially since > GetConsumerID already returns a const &). Let's just make this a > map<RequestContentScriptKey, ConsumerID> I remember why we have to use the linked_ptr here. If you see line 271, it needs to call the operator "=" for assigning values, but the auto generated operator = is deleted since all the fields of ConsumerID are marked as const. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:252: static base::LazyInstance<RequestContentScriptKeyToIDMap> On 2015/01/14 16:45:08, Devlin wrote: > static inside anonymous namespace is redundant -- remove the static. Removed. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:254: On 2015/01/14 16:45:08, Devlin wrote: > extra newline. Oops, removed. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.h (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.h:162: linked_ptr<DeclarativeUserScriptMaster> master_; On 2015/01/14 16:45:08, Devlin wrote: > I don't think it's wise to make this a linked_ptr. Instead, let's document why > the weak pointer is safe. linked_ptr was removed from here. Also I have to do a sacrifice here: use a raw pointer. I tried the WeakPtr, but it is difficult to initialize master_ in the constructor's initialization list, and I believe it is the only way to initialize a WeakPtr:( Please correct me if I am wrong, I would like to make it a weak pointer as well. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/declarative_user_script_manager.h (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/declarative_user_script_manager.h:27: linked_ptr<DeclarativeUserScriptMaster> GetDeclarativeUserScriptMasterByID( On 2015/01/14 16:45:08, Devlin wrote: > Again, no linked_ptr (assume this goes for all DeclarativeUserScriptMaster > uses). Updated. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/extension_declarative_user_script_master.cc (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/extension_declarative_user_script_master.cc:14: ExtensionDeclarativeUserScriptMaster::ExtensionDeclarativeUserScriptMaster( On 2015/01/14 16:45:08, Devlin wrote: > Actually, this class makes me sad. Will the DeclarativeUserScriptManager always > have knowledge of Extensions, as it does now? If so, could we have it watch > OnExtensionUnloaded and delete the corresponding DeclarativeUserScriptMaster, so > we don't need this subclass? This is a great idea! https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/extension_declarative_user_script_master.h (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/extension_declarative_user_script_master.h:12: #include "extensions/common/consumer.h" On 2015/01/14 16:45:08, Devlin wrote: > Check your includes. Done. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/extension_declarative_user_script_master.h:30: // ExtensionRegistryObserver implementation. On 2015/01/14 16:45:09, Devlin wrote: > nit: prefer > // ExtensionRegistryObserver: Really? I thought we always use "XXX implementation.". Both are ok for me. Updated. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/extension_user_script_loader.cc (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/14 16:45:09, Devlin wrote: > check year (everywhere) Thanks. I didn't realize there are so many new files were added. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:35: class ExtensionsBrowserClient; On 2015/01/14 16:45:09, Devlin wrote: > why do you need this? Removed. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:70: weak_factory_.GetWeakPtr())); On 2015/01/14 16:45:09, Devlin wrote: > indentation Done. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:71: } else On 2015/01/14 16:45:09, Devlin wrote: > our bracketing rules are complex. > if (one_line_if_body) > NoBrackets() > > if (multi_line_if_body) { > UseBrackets() > ... > } > > if (multi_line_if) { > UseBrackets() > ... > } else if (else_with_previous_multi_line_if) { > StillUseBrackets() > } > > I believe the linter calls this rule "If an else has brackets on one side, it > must also have brackets on the other side". I must misunderstood your last round of comments and changed it like this. I also believe the else should have {} if the if has one. Thank you for the details:) https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:80: for (std::set<ConsumerID>::const_iterator it = changed_consumers_.begin(); On 2015/01/14 16:45:09, Devlin wrote: > Let's C++11 this: > for (const ConsumerID& consumer_id : changed_consumers_) { > ... > } Done. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/extension_user_script_loader.h (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/14 16:45:09, Devlin wrote: > year Done. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.h:39: static bool LoadScriptContent(const ConsumerID& consumer_id, On 2015/01/14 16:45:09, Devlin wrote: > Can this go in an anonymous namespace in the .cc? No, it can't. This static function is introduced for the purpose of creating "a static virtual function". A pointer of this function is returned in a virtual function LoadUserScriptsFunctionCallback GetLoadUserScriptsFunction() (see line 48). This is because we need a static function which will be called on file thread, and we also want to keep different implementations for extensions (in subclass) and without extensions (in the base class). https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.h:44: // UserScriptLoader implementation. On 2015/01/14 16:45:09, Devlin wrote: > nit: prefer > // UserScriptLoader: > > (goes for everywhere) Done. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/user_script_loader.cc (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:405: bool UserScriptLoader::LoadScriptContent( On 2015/01/14 16:45:09, Devlin wrote: > Is this correct for non-extension scripts? If not, can we simplify this whole > "LoadScriptContent()" and "GetLoadScriptContentFunction" chain? Honestly I am not sure whether this function work for non-extension scripts or not, but we do need a way to load script content from file thread for non-extension scripts. So I adopt the "LoadScriptContent()" and "GetLoadScriptContentFunction" chain to implement a static virtual function. We do need such a static function to work on file thread, and also have different ways to load scripts. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/user_script_loader.h (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:89: virtual bool Ready() = 0; On 2015/01/14 16:45:09, Devlin wrote: > Let's get rid of this one, and instead expose a non-virtual SetReady() function. I still feel this flag is extension-specific, maybe it is better to leave it in the subclass? In case we have another subclass later, we can override this function but with totally different flags. What do you think? https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:97: virtual ContentVerifier* GetContentVerifier(); On 2015/01/14 16:45:09, Devlin wrote: > Can we remove this and initialize the UserScriptLoader with a ContentVerifier? Good idea:) https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:112: void AttemptLoad(); On 2015/01/14 16:45:09, Devlin wrote: > Make this private and call it from the to-be-created SetReady() I am a little confused here, it seems there are two places that extension_system_ready_ is assigned true, but only in the OnExtensionUnloaded() the AttemptLoad() is called. We can't put them together, can we? https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:115: void OnScriptsLoaded(scoped_ptr<UserScriptList> user_scripts, On 2015/01/14 16:45:09, Devlin wrote: > Private. Done. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:119: scoped_ptr<UserScriptList> user_scripts_; On 2015/01/14 16:45:09, Devlin wrote: > Private Done. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:122: ConsumersInfo consumers_info_; On 2015/01/14 16:45:09, Devlin wrote: > I think this might be better to have an exposed Add/Remove, and make this > private. We try to avoid protected data members whenever possible. Sure, I will follow this style. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:126: ConsumerIDSet changed_consumers_; On 2015/01/14 16:45:09, Devlin wrote: > Make this private and pass it into UpdateConsumersInfo() Good idea, thanks. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:129: Profile* profile_; On 2015/01/14 16:45:09, Devlin wrote: > Expose a getter, and make this private. Done. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:133: ConsumerID consumer_id_; On 2015/01/14 16:45:09, Devlin wrote: > Expose a getter, and make this private. Done. https://codereview.chromium.org/822453002/diff/480001/extensions/common/consu... File extensions/common/consumer.h (right): https://codereview.chromium.org/822453002/diff/480001/extensions/common/consu... extensions/common/consumer.h:14: // For example: an extension, a <webview> created by an extension, On 2015/01/14 16:45:10, Devlin wrote: > This needs to be re-worded. Maybe something like: > A ConsumerID could be used for an extension... It sounds better:) https://codereview.chromium.org/822453002/diff/480001/extensions/common/consu... extensions/common/consumer.h:16: class ConsumerID { On 2015/01/14 16:45:10, Devlin wrote: > See also main comment on making this a struct. Done. https://codereview.chromium.org/822453002/diff/480001/extensions/common/consu... extensions/common/consumer.h:34: static const ConsumerID& EmptyConsumerID(); On 2015/01/14 16:45:10, Devlin wrote: > Why do we need this, instead of just creating a new empty one? Hmmm, having many copies of empty one seems wasteful, since a ConsumerID is immutable after creation. So I think we can reuse the same reference of an empty one. Any concern about it? https://codereview.chromium.org/822453002/diff/480001/extensions/common/user_... File extensions/common/user_script.h (right): https://codereview.chromium.org/822453002/diff/480001/extensions/common/user_... extensions/common/user_script.h:282: linked_ptr<ConsumerID> consumer_id_; On 2015/01/14 16:45:10, Devlin wrote: > Let's not have these be linked_ptrs. Well, it has to be a pointer here since the consumer_id needs to be updated after the construction of UserScript. Initially we want to use scoped_ptr, but it makes impossible to store UserScript in STL container since scoped_ptr isn't copyable.
Patchset #13 (id:500001) has been deleted
https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:247: std::map<RequestContentScriptKey, linked_ptr<ConsumerID>>; On 2015/01/14 23:46:02, Xi Han wrote: > On 2015/01/14 16:45:08, Devlin wrote: > > ConsumerIDs are cheap enough to copy that it's not worth the potential agony > to > > linked_ptr them, since we won't be doing it a lot (especially since > > GetConsumerID already returns a const &). Let's just make this a > > map<RequestContentScriptKey, ConsumerID> > > I remember why we have to use the linked_ptr here. If you see line 271, it needs > to call the operator "=" for assigning values, but the auto generated operator = > is deleted since all the fields of ConsumerID are marked as const. Dang. Sorry in advance for this: Let's make ConsumerID a class again. :( From there, provide operator= and copy constructors for it, and only allow access to the members by accessor. This allows for copy/assignment of a consumer id in STL containers and such, but keeps it, essentially, immutable. So sorry for all the back and forth on it :/ The reason to do this is that linked_ptrs really aren't designed to be shared - they're only really for storing things in an STL container. And with something so simple as a ConsumerID, an intricate ownership pattern to avoid copying some data really isn't worthwhile, unless we find a place where we'll be copying them hundreds/thousands of meaningless times. It's much better to just do this safely than to deal with the segfaults/dereferences/uses-after-free later. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.h (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.h:162: linked_ptr<DeclarativeUserScriptMaster> master_; On 2015/01/14 23:46:02, Xi Han wrote: > On 2015/01/14 16:45:08, Devlin wrote: > > I don't think it's wise to make this a linked_ptr. Instead, let's document > why > > the weak pointer is safe. > > linked_ptr was removed from here. Also I have to do a sacrifice here: use a raw > pointer. I tried the WeakPtr, but it is difficult to initialize master_ in the > constructor's initialization list, and I believe it is the only way to > initialize a WeakPtr:( Please correct me if I am wrong, I would like to make it > a weak pointer as well. That's my bad - when I say "weak pointer", I mean c-style raw pointer (i.e., one that is weak and "dumb"). I need to fix that. Raw pointer is fine, as long as we document why it is safe. :) https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/extension_declarative_user_script_master.h (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/extension_declarative_user_script_master.h:30: // ExtensionRegistryObserver implementation. On 2015/01/14 23:46:03, Xi Han wrote: > On 2015/01/14 16:45:09, Devlin wrote: > > nit: prefer > > // ExtensionRegistryObserver: > > Really? I thought we always use "XXX implementation.". Both are ok for me. > Updated. We're woefully inconsistent. I used to use "Foo implementation." too, until I was told to use "Foo:", after which I did a quick check via git grep. It looks like "Foo:" is more commonly used in both chrome code as a whole and extensions code, so I'm trying to slowly make a dent towards consistency. :) https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/user_script_loader.cc (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:405: bool UserScriptLoader::LoadScriptContent( On 2015/01/14 23:46:03, Xi Han wrote: > On 2015/01/14 16:45:09, Devlin wrote: > > Is this correct for non-extension scripts? If not, can we simplify this whole > > "LoadScriptContent()" and "GetLoadScriptContentFunction" chain? > > Honestly I am not sure whether this function work for non-extension scripts or > not, but we do need a way to load script content from file thread for > non-extension scripts. So I adopt the "LoadScriptContent()" and > "GetLoadScriptContentFunction" chain to implement a static virtual function. We > do need such a static function to work on file thread, and also have different > ways to load scripts. The fact that this function contains extension-specific logic (like getting an extension resource path) makes me think it won't right for non-extension scripts. Perhaps better to not have the non-extension case until we know what can work. Also, there's no reason this has to be static - it could be a member function so that it can be truly overridden in the subclass, and then you don't need the GetLoadScriptsFunction(). https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/user_script_loader.h (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:89: virtual bool Ready() = 0; On 2015/01/14 23:46:03, Xi Han wrote: > On 2015/01/14 16:45:09, Devlin wrote: > > Let's get rid of this one, and instead expose a non-virtual SetReady() > function. > > I still feel this flag is extension-specific, maybe it is better to leave it in > the subclass? In case we have another subclass later, we can override this > function but with totally different flags. What do you think? Right now, you have to check Ready() in the super class, and have it implemented in the subclass, which extends the complexity necessary for the subclass (and makes the superclass harder to parse). Instead, if we expose a SetReady(), that SetReady() need only be implemented once (in the superclass), and the sub class need only call it. I think anything we can do to reduce the footprint of the subclass/virtual methods is a win. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:112: void AttemptLoad(); On 2015/01/14 23:46:03, Xi Han wrote: > On 2015/01/14 16:45:09, Devlin wrote: > > Make this private and call it from the to-be-created SetReady() > > I am a little confused here, it seems there are two places that > extension_system_ready_ is assigned true, but only in the OnExtensionUnloaded() > the AttemptLoad() is called. We can't put them together, can we? We call AttemptLoad() in the OnExtensionSystemReady() (I think that's what you mean, not OnExtensionUnloaded) case because we need to bootstrap the system. That can be generically true any time that it's not ready from the start. So how about passing in a |ready| bool to UserScriptLoader, and in SetReady() if it's set to be ready from not ready, then we bootstrap it. That make sense? https://codereview.chromium.org/822453002/diff/480001/extensions/common/consu... File extensions/common/consumer.h (right): https://codereview.chromium.org/822453002/diff/480001/extensions/common/consu... extensions/common/consumer.h:34: static const ConsumerID& EmptyConsumerID(); On 2015/01/14 23:46:03, Xi Han wrote: > On 2015/01/14 16:45:10, Devlin wrote: > > Why do we need this, instead of just creating a new empty one? > > Hmmm, having many copies of empty one seems wasteful, since a ConsumerID is > immutable after creation. So I think we can reuse the same reference of an empty > one. Any concern about it? Yeah -- there's no real cheap way to do it. Your current implementation uses a static scoped_ptr, which is a code violation (only plain-old-data statics are allowed), and things like singletons and lazy instances are deceptively expensive. See also comments above, e.g., base::EmptyString() encouraging the use of a newly-constructed object whenever possible. If we don't need to pass a reference around for memory management reasons, it's best to just create a new empty one each time (it's really cheap for an object like a ConsumerID). https://codereview.chromium.org/822453002/diff/480001/extensions/common/user_... File extensions/common/user_script.h (right): https://codereview.chromium.org/822453002/diff/480001/extensions/common/user_... extensions/common/user_script.h:282: linked_ptr<ConsumerID> consumer_id_; On 2015/01/14 23:46:04, Xi Han wrote: > On 2015/01/14 16:45:10, Devlin wrote: > > Let's not have these be linked_ptrs. > > Well, it has to be a pointer here since the consumer_id needs to be updated > after the construction of UserScript. Initially we want to use scoped_ptr, but > it makes impossible to store UserScript in STL container since scoped_ptr isn't > copyable. This should be fixable once you can just assign ConsumerIDs.
Patchset #14 (id:540001) has been deleted
Hi Devlin@: I updated my CL according our discussion offline, and hopefully includes everything. PTAK, thanks. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:247: std::map<RequestContentScriptKey, linked_ptr<ConsumerID>>; On 2015/01/16 17:05:03, Devlin wrote: > On 2015/01/14 23:46:02, Xi Han wrote: > > On 2015/01/14 16:45:08, Devlin wrote: > > > ConsumerIDs are cheap enough to copy that it's not worth the potential agony > > to > > > linked_ptr them, since we won't be doing it a lot (especially since > > > GetConsumerID already returns a const &). Let's just make this a > > > map<RequestContentScriptKey, ConsumerID> > > > > I remember why we have to use the linked_ptr here. If you see line 271, it > needs > > to call the operator "=" for assigning values, but the auto generated operator > = > > is deleted since all the fields of ConsumerID are marked as const. > > Dang. Sorry in advance for this: Let's make ConsumerID a class again. :( > > From there, provide operator= and copy constructors for it, and only allow > access to the members by accessor. This allows for copy/assignment of a > consumer id in STL containers and such, but keeps it, essentially, immutable. > So sorry for all the back and forth on it :/ > > The reason to do this is that linked_ptrs really aren't designed to be shared - > they're only really for storing things in an STL container. And with something > so simple as a ConsumerID, an intricate ownership pattern to avoid copying some > data really isn't worthwhile, unless we find a place where we'll be copying them > hundreds/thousands of meaningless times. It's much better to just do this > safely than to deal with the segfaults/dereferences/uses-after-free later. Done. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.h (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.h:162: linked_ptr<DeclarativeUserScriptMaster> master_; On 2015/01/16 17:05:03, Devlin wrote: > On 2015/01/14 23:46:02, Xi Han wrote: > > On 2015/01/14 16:45:08, Devlin wrote: > > > I don't think it's wise to make this a linked_ptr. Instead, let's document > > why > > > the weak pointer is safe. > > > > linked_ptr was removed from here. Also I have to do a sacrifice here: use a > raw > > pointer. I tried the WeakPtr, but it is difficult to initialize master_ in the > > constructor's initialization list, and I believe it is the only way to > > initialize a WeakPtr:( Please correct me if I am wrong, I would like to make > it > > a weak pointer as well. > > That's my bad - when I say "weak pointer", I mean c-style raw pointer (i.e., one > that is weak and "dumb"). I need to fix that. Raw pointer is fine, as long as > we document why it is safe. :) Sounds good:) https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/extension_declarative_user_script_master.h (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/extension_declarative_user_script_master.h:30: // ExtensionRegistryObserver implementation. On 2015/01/16 17:05:03, Devlin wrote: > On 2015/01/14 23:46:03, Xi Han wrote: > > On 2015/01/14 16:45:09, Devlin wrote: > > > nit: prefer > > > // ExtensionRegistryObserver: > > > > Really? I thought we always use "XXX implementation.". Both are ok for me. > > Updated. > > We're woefully inconsistent. I used to use "Foo implementation." too, until I > was told to use "Foo:", after which I did a quick check via git grep. It looks > like "Foo:" is more commonly used in both chrome code as a whole and extensions > code, so I'm trying to slowly make a dent towards consistency. :) Acknowledged. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/user_script_loader.cc (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:405: bool UserScriptLoader::LoadScriptContent( On 2015/01/16 17:05:04, Devlin wrote: > On 2015/01/14 23:46:03, Xi Han wrote: > > On 2015/01/14 16:45:09, Devlin wrote: > > > Is this correct for non-extension scripts? If not, can we simplify this > whole > > > "LoadScriptContent()" and "GetLoadScriptContentFunction" chain? > > > > Honestly I am not sure whether this function work for non-extension scripts or > > not, but we do need a way to load script content from file thread for > > non-extension scripts. So I adopt the "LoadScriptContent()" and > > "GetLoadScriptContentFunction" chain to implement a static virtual function. > We > > do need such a static function to work on file thread, and also have different > > ways to load scripts. > > The fact that this function contains extension-specific logic (like getting an > extension resource path) makes me think it won't right for non-extension > scripts. Perhaps better to not have the non-extension case until we know what > can work. Also, there's no reason this has to be static - it could be a member > function so that it can be truly overridden in the subclass, and then you don't > need the GetLoadScriptsFunction(). I will remove the logical in the base class and add only when we need it. As discussed offline, we have to make this function as static to avoid troubles since it is called on file thread. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... File chrome/browser/extensions/user_script_loader.h (right): https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:89: virtual bool Ready() = 0; On 2015/01/16 17:05:04, Devlin wrote: > On 2015/01/14 23:46:03, Xi Han wrote: > > On 2015/01/14 16:45:09, Devlin wrote: > > > Let's get rid of this one, and instead expose a non-virtual SetReady() > > function. > > > > I still feel this flag is extension-specific, maybe it is better to leave it > in > > the subclass? In case we have another subclass later, we can override this > > function but with totally different flags. What do you think? > > Right now, you have to check Ready() in the super class, and have it implemented > in the subclass, which extends the complexity necessary for the subclass (and > makes the superclass harder to parse). Instead, if we expose a SetReady(), that > SetReady() need only be implemented once (in the superclass), and the sub class > need only call it. I think anything we can do to reduce the footprint of the > subclass/virtual methods is a win. Done. https://codereview.chromium.org/822453002/diff/480001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:112: void AttemptLoad(); On 2015/01/16 17:05:04, Devlin wrote: > On 2015/01/14 23:46:03, Xi Han wrote: > > On 2015/01/14 16:45:09, Devlin wrote: > > > Make this private and call it from the to-be-created SetReady() > > > > I am a little confused here, it seems there are two places that > > extension_system_ready_ is assigned true, but only in the > OnExtensionUnloaded() > > the AttemptLoad() is called. We can't put them together, can we? > > We call AttemptLoad() in the OnExtensionSystemReady() (I think that's what you > mean, not OnExtensionUnloaded) case because we need to bootstrap the system. > That can be generically true any time that it's not ready from the start. So > how about passing in a |ready| bool to UserScriptLoader, and in SetReady() if > it's set to be ready from not ready, then we bootstrap it. That make sense? Done. https://codereview.chromium.org/822453002/diff/480001/extensions/common/consu... File extensions/common/consumer.h (right): https://codereview.chromium.org/822453002/diff/480001/extensions/common/consu... extensions/common/consumer.h:34: static const ConsumerID& EmptyConsumerID(); On 2015/01/16 17:05:04, Devlin wrote: > On 2015/01/14 23:46:03, Xi Han wrote: > > On 2015/01/14 16:45:10, Devlin wrote: > > > Why do we need this, instead of just creating a new empty one? > > > > Hmmm, having many copies of empty one seems wasteful, since a ConsumerID is > > immutable after creation. So I think we can reuse the same reference of an > empty > > one. Any concern about it? > > Yeah -- there's no real cheap way to do it. Your current implementation uses a > static scoped_ptr, which is a code violation (only plain-old-data statics are > allowed), and things like singletons and lazy instances are deceptively > expensive. See also comments above, e.g., base::EmptyString() encouraging the > use of a newly-constructed object whenever possible. If we don't need to pass a > reference around for memory management reasons, it's best to just create a new > empty one each time (it's really cheap for an object like a ConsumerID). That is a good point. Since we add the default constructor of ConsumerID back, we don't need this function anymore. https://codereview.chromium.org/822453002/diff/480001/extensions/common/user_... File extensions/common/user_script.h (right): https://codereview.chromium.org/822453002/diff/480001/extensions/common/user_... extensions/common/user_script.h:282: linked_ptr<ConsumerID> consumer_id_; On 2015/01/16 17:05:04, Devlin wrote: > On 2015/01/14 23:46:04, Xi Han wrote: > > On 2015/01/14 16:45:10, Devlin wrote: > > > Let's not have these be linked_ptrs. > > > > Well, it has to be a pointer here since the consumer_id needs to be updated > > after the construction of UserScript. Initially we want to use scoped_ptr, but > > it makes impossible to store UserScript in STL container since scoped_ptr > isn't > > copyable. > > This should be fixable once you can just assign ConsumerIDs. Done.
Lots more comments, but mostly all nits. This is looking good; great job! :) https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:10: #include "base/memory/linked_ptr.h" nit: Don't need this, right? https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:262: RequestContentScriptKeyToIDMap::iterator it = map.find(key); instead of lines 263 - 268, do: if (it == map.end()) { it = map.insert(std::make_pair(key, ConsumerID(host_type, host_id, instance_type, GetNextId()).first; } return it->second; In addition to saving a line or two of code, it saves you from doing a duplicate lookup on line 268. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.h (right): https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.h:99: struct RequestKey { Could we instead just use a std::string for this? https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.h:162: // The lifetime of the master_ is managed by DeclarativeUserScriptManager, nit: newline before this. nit: Document why the lifetime is safe (i.e., show that the DeclarativeUserScriptManager outlives the RequestContentScript). https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... File chrome/browser/extensions/declarative_user_script_manager.cc (right): https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/declarative_user_script_manager.cc:46: for (auto it = declarative_user_script_masters_.begin(); nit: shorter: for (const auto& val : declarative_user_script_masters_) { DeclarativeUserScriptMaster* master = val.second.get(); if (master...) } https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... File chrome/browser/extensions/extension_user_script_loader.cc (right): https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:15: #include "chrome/browser/chrome_notification_types.h" nit: prune includes. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:32: using content::BrowserThread; nit: Even though you have this line, you specify "content::BrowserThread" everywhere. Just delete this one (we have a slight preference for not using too many usings, anyway). https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:46: consumer_id.host_id(), extension_root, relative_path)); nit: since this is in the ExtensionUserScriptLoader, we can probably just pass this function the extension id, instead of the full consumer id. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:67: SetReady(false); UserScriptLoader is, by default, set to false. You can take out this line. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:103: for (std::set<ConsumerID>::iterator it = changed_consumers.begin(); nit: shorter: for (const ConsumerID& id : changed_consumers) changed_extensions.insert(id.host_id()); https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:170: if (index == 0) { nit: no brackets. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... File chrome/browser/extensions/extension_user_script_loader.h (right): https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.h:10: #include "content/public/browser/notification_observer.h" nit: prune includes. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.h:39: static bool LoadScriptContent(const ConsumerID& consumer_id, nit: function comments. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... File chrome/browser/extensions/shared_user_script_master.cc (right): https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/shared_user_script_master.cc:17: ConsumerID() /* owner_extension_id */, update comment https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... File chrome/browser/extensions/user_script_loader.cc (left): https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:571: if (base::SharedMemory::IsHandleValid(handle_for_process)) { Why not keep this if here, so that subclasses don't have to worry about it? https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... File chrome/browser/extensions/user_script_loader.cc (right): https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:48: ContentVerifier* verifier, nit: Please update this to take a const scoped_refptr<ContentVerifier>& (and similarly LoadScriptContent). https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:49: UserScriptLoader::LoadUserScriptsFunctionCallback callback) { nit: I think |load_script_content_function| is more descriptive than |callback|. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:265: ContentVerifier* content_verifier) Please update this to take a const scoped_refptr<ContentVerifier>&. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:404: bool UserScriptLoader::LoadScriptContent( Why can't we remove this method, remove the static method declaration in the .h, move the ExtensionUserScriptLoader::LoadScriptContent into an anonymous namespace in the .cc of that file, and make GetLoadUserScriptsFunction pure-virtual? https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:437: AttemptLoad(); We should either: DCHECK_NE(ready, ready_); or bool was_ready = ready_; ready_ = ready; if (ready_ && !was_ready) AttemptLoad(); i.e., if we were already ready, we shouldn't attempt the load. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:509: for (std::set<UserScript>::const_iterator it = scripts.begin(); let's C++11-ify this: for (const UserScript& script : scripts) changed_consumers_.insert(script.consumer_id()); https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... File chrome/browser/extensions/user_script_loader.h (right): https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:61: ContentVerifier* content_verifier = nullptr); nit: we don't allow default values for arguments. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:94: // Notifies render process and convert |changed_consumers| to specified types nit: Just "Notifies the render process of any updated scripts." The fact that the extension version converts consumer ids is an implementation detail. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:112: // Removes the entries with the given host_id from the consumers_info_ map. nit: It's common practice to wrap variable names in bars, i.e. |consumers_info_|. (Goes for everywhere.) https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:200: ContentVerifier* content_verifier_; Please store this as a scoped_refptr. https://codereview.chromium.org/822453002/diff/580001/extensions/common/consu... File extensions/common/consumer.cc (right): https://codereview.chromium.org/822453002/diff/580001/extensions/common/consu... extensions/common/consumer.cc:8: static int current_instance_id_ = ConsumerID::kDefaultInstanceID; anonymous namespace + static = redundant. https://codereview.chromium.org/822453002/diff/580001/extensions/common/consu... extensions/common/consumer.cc:8: static int current_instance_id_ = ConsumerID::kDefaultInstanceID; We should probably use a StaticAtomicSequenceNumber for this (see user_script.cc for an example). https://codereview.chromium.org/822453002/diff/580001/extensions/common/consu... extensions/common/consumer.cc:40: (host_type_ == id.host_type() && host_id_.compare(id.host_id()) < 0) || yuck :/ Why isn't there a better way to do this yet? I mean, you can cheat with std::tie, but that's just...weird. I think this is easier to read, and not any more lines, as: if (host_type_ != id.host_type_) return host_type_ < id.host_type_; if (host_id_ != id.host_id_) return host_id_ < id.host_id_; ... https://codereview.chromium.org/822453002/diff/580001/extensions/common/consu... File extensions/common/consumer.h (right): https://codereview.chromium.org/822453002/diff/580001/extensions/common/consu... extensions/common/consumer.h:5: #ifndef EXTENSIONS_COMMON_CONSUMER_H_ Given the contents of this file, I think it makes more sense to have it be consumer_id.h/.cc. In case you haven't discovered it: ~ $ cd <SOURCE_DIR> src $ ./tools/git/move_source_file.py extensions/common/consumer.h extensions/common/consumer_id.h src $ ./tools/git/move_source_file.py extensions/common/consumer_id.cc extensions/common/consumer_id.cc That will update all dependents of the file, including the gypis. Might wanna check the gns; I'm not sure the script was updated for those yet. https://codereview.chromium.org/822453002/diff/580001/extensions/common/consu... extensions/common/consumer.h:27: explicit ConsumerID(const ConsumerID& other); Make a note here about why we allow copying/why it's okay. https://codereview.chromium.org/822453002/diff/580001/extensions/common/consu... extensions/common/consumer.h:31: int instance_id); I think I'd slightly prefer an "is_declarative" bool here, and then removing the public access to kDefaultInstanceID and GetNextID, since they are implementation details. https://codereview.chromium.org/822453002/diff/580001/extensions/common/consu... extensions/common/consumer.h:46: // Extension or WebUI. nit: Just say "The host type of the consumer." The enum already says that this is either extension or webui, and there's no point in needing to update this comment, too, when we add host type FluxCapacitor. :) https://codereview.chromium.org/822453002/diff/580001/extensions/common/consu... extensions/common/consumer.h:54: InstanceType instance_type_; Similar comment to above. https://codereview.chromium.org/822453002/diff/580001/extensions/common/consu... extensions/common/consumer.h:57: // |instance_id| is 0; nit: either make the semicolon a period, or fix wrapping and capitalization. https://codereview.chromium.org/822453002/diff/580001/extensions/common/consu... extensions/common/consumer.h:65: class Consumer { When is this used? https://codereview.chromium.org/822453002/diff/580001/extensions/common/user_... File extensions/common/user_script.cc (right): https://codereview.chromium.org/822453002/diff/580001/extensions/common/user_... extensions/common/user_script.cc:234: int host_type; nit: initialize the ints to something reasonable (0). https://codereview.chromium.org/822453002/diff/580001/extensions/common/user_... File extensions/common/user_script.h (right): https://codereview.chromium.org/822453002/diff/580001/extensions/common/user_... extensions/common/user_script.h:13: #include "base/memory/linked_ptr.h" prune https://codereview.chromium.org/822453002/diff/580001/extensions/common/user_... extensions/common/user_script.h:212: const std::string& GetExtensionID() const; it's safe to have this inlined as const std::string& extension_id() const { return consumer_id_.host_id(); }
Patchset #16 (id:600001) has been deleted
Hi Devlin@: PTAL, thanks! https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:10: #include "base/memory/linked_ptr.h" On 2015/01/20 17:51:04, Devlin wrote: > nit: Don't need this, right? Sorry, forget to clean up. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:262: RequestContentScriptKeyToIDMap::iterator it = map.find(key); On 2015/01/20 17:51:05, Devlin wrote: > instead of lines 263 - 268, do: > if (it == map.end()) { > it = map.insert(std::make_pair(key, ConsumerID(host_type, host_id, > instance_type, GetNextId()).first; > } > return it->second; > > In addition to saving a line or two of code, it saves you from doing a duplicate > lookup on line 268. Goods better, updated. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.h (right): https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.h:99: struct RequestKey { On 2015/01/20 17:51:05, Devlin wrote: > Could we instead just use a std::string for this? Ok, I will change it to std::string, and change it when we need to include more information for <webview>. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.h:162: // The lifetime of the master_ is managed by DeclarativeUserScriptManager, On 2015/01/20 17:51:05, Devlin wrote: > nit: newline before this. > nit: Document why the lifetime is safe (i.e., show that the > DeclarativeUserScriptManager outlives the RequestContentScript). Lifetime is hard to explain, so I try my best to add some comments here. Feel free to update if you think they are not enough to explain. Thanks. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... File chrome/browser/extensions/declarative_user_script_manager.cc (right): https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/declarative_user_script_manager.cc:46: for (auto it = declarative_user_script_masters_.begin(); On 2015/01/20 17:51:05, Devlin wrote: > nit: shorter: > for (const auto& val : declarative_user_script_masters_) { > DeclarativeUserScriptMaster* master = val.second.get(); > if (master...) > } Done. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... File chrome/browser/extensions/extension_user_script_loader.cc (right): https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:15: #include "chrome/browser/chrome_notification_types.h" On 2015/01/20 17:51:05, Devlin wrote: > nit: prune includes. Why I cannot find these unnecessary includes:( https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:32: using content::BrowserThread; On 2015/01/20 17:51:05, Devlin wrote: > nit: Even though you have this line, you specify "content::BrowserThread" > everywhere. Just delete this one (we have a slight preference for not using too > many usings, anyway). Removed. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:46: consumer_id.host_id(), extension_root, relative_path)); On 2015/01/20 17:51:05, Devlin wrote: > nit: since this is in the ExtensionUserScriptLoader, we can probably just pass > this function the extension id, instead of the full consumer id. Done. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:67: SetReady(false); On 2015/01/20 17:51:05, Devlin wrote: > UserScriptLoader is, by default, set to false. You can take out this line. Done. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:103: for (std::set<ConsumerID>::iterator it = changed_consumers.begin(); On 2015/01/20 17:51:05, Devlin wrote: > nit: shorter: > for (const ConsumerID& id : changed_consumers) > changed_extensions.insert(id.host_id()); Done. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:170: if (index == 0) { On 2015/01/20 17:51:05, Devlin wrote: > nit: no brackets. Done. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... File chrome/browser/extensions/extension_user_script_loader.h (right): https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.h:10: #include "content/public/browser/notification_observer.h" On 2015/01/20 17:51:05, Devlin wrote: > nit: prune includes. Done. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.h:39: static bool LoadScriptContent(const ConsumerID& consumer_id, On 2015/01/20 17:51:05, Devlin wrote: > nit: function comments. Done. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... File chrome/browser/extensions/shared_user_script_master.cc (right): https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/shared_user_script_master.cc:17: ConsumerID() /* owner_extension_id */, On 2015/01/20 17:51:05, Devlin wrote: > update comment Done. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... File chrome/browser/extensions/user_script_loader.cc (left): https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:571: if (base::SharedMemory::IsHandleValid(handle_for_process)) { On 2015/01/20 17:51:05, Devlin wrote: > Why not keep this if here, so that subclasses don't have to worry about it? Sure, move the code back. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... File chrome/browser/extensions/user_script_loader.cc (right): https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:48: ContentVerifier* verifier, On 2015/01/20 17:51:05, Devlin wrote: > nit: Please update this to take a const scoped_refptr<ContentVerifier>& (and > similarly LoadScriptContent). Updated. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:49: UserScriptLoader::LoadUserScriptsFunctionCallback callback) { On 2015/01/20 17:51:05, Devlin wrote: > nit: I think |load_script_content_function| is more descriptive than |callback|. Done. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:265: ContentVerifier* content_verifier) On 2015/01/20 17:51:05, Devlin wrote: > Please update this to take a const scoped_refptr<ContentVerifier>&. Done. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:404: bool UserScriptLoader::LoadScriptContent( On 2015/01/20 17:51:05, Devlin wrote: > Why can't we remove this method, remove the static method declaration in the .h, > move the ExtensionUserScriptLoader::LoadScriptContent into an anonymous > namespace in the .cc of that file, and make GetLoadUserScriptsFunction > pure-virtual? Done. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:437: AttemptLoad(); On 2015/01/20 17:51:05, Devlin wrote: > We should either: > DCHECK_NE(ready, ready_); > or > bool was_ready = ready_; > ready_ = ready; > if (ready_ && !was_ready) > AttemptLoad(); > > i.e., if we were already ready, we shouldn't attempt the load. Done. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:509: for (std::set<UserScript>::const_iterator it = scripts.begin(); On 2015/01/20 17:51:05, Devlin wrote: > let's C++11-ify this: > for (const UserScript& script : scripts) > changed_consumers_.insert(script.consumer_id()); Done. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... File chrome/browser/extensions/user_script_loader.h (right): https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:61: ContentVerifier* content_verifier = nullptr); On 2015/01/20 17:51:06, Devlin wrote: > nit: we don't allow default values for arguments. Thank for pointing out this. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:94: // Notifies render process and convert |changed_consumers| to specified types On 2015/01/20 17:51:06, Devlin wrote: > nit: Just "Notifies the render process of any updated scripts." The fact that > the extension version converts consumer ids is an implementation detail. Done. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:112: // Removes the entries with the given host_id from the consumers_info_ map. On 2015/01/20 17:51:06, Devlin wrote: > nit: It's common practice to wrap variable names in bars, i.e. > |consumers_info_|. (Goes for everywhere.) Updated. https://codereview.chromium.org/822453002/diff/580001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:200: ContentVerifier* content_verifier_; On 2015/01/20 17:51:06, Devlin wrote: > Please store this as a scoped_refptr. Done. https://codereview.chromium.org/822453002/diff/580001/extensions/common/consu... File extensions/common/consumer.cc (right): https://codereview.chromium.org/822453002/diff/580001/extensions/common/consu... extensions/common/consumer.cc:8: static int current_instance_id_ = ConsumerID::kDefaultInstanceID; On 2015/01/20 17:51:06, Devlin wrote: > anonymous namespace + static = redundant. Removed. https://codereview.chromium.org/822453002/diff/580001/extensions/common/consu... extensions/common/consumer.cc:8: static int current_instance_id_ = ConsumerID::kDefaultInstanceID; On 2015/01/20 17:51:06, Devlin wrote: > We should probably use a StaticAtomicSequenceNumber for this (see user_script.cc > for an example). As discussed offline, will still use int for current_instance_id_. https://codereview.chromium.org/822453002/diff/580001/extensions/common/consu... extensions/common/consumer.cc:40: (host_type_ == id.host_type() && host_id_.compare(id.host_id()) < 0) || On 2015/01/20 17:51:06, Devlin wrote: > yuck :/ Why isn't there a better way to do this yet? I mean, you can cheat > with std::tie, but that's just...weird. > > I think this is easier to read, and not any more lines, as: > if (host_type_ != id.host_type_) > return host_type_ < id.host_type_; > if (host_id_ != id.host_id_) > return host_id_ < id.host_id_; > ... Yes, the code seems easier to understand now. https://codereview.chromium.org/822453002/diff/580001/extensions/common/consu... File extensions/common/consumer.h (right): https://codereview.chromium.org/822453002/diff/580001/extensions/common/consu... extensions/common/consumer.h:5: #ifndef EXTENSIONS_COMMON_CONSUMER_H_ On 2015/01/20 17:51:06, Devlin wrote: > Given the contents of this file, I think it makes more sense to have it be > consumer_id.h/.cc. > > In case you haven't discovered it: > ~ $ cd <SOURCE_DIR> > src $ ./tools/git/move_source_file.py extensions/common/consumer.h > extensions/common/consumer_id.h > src $ ./tools/git/move_source_file.py extensions/common/consumer_id.cc > extensions/common/consumer_id.cc > > That will update all dependents of the file, including the gypis. Might wanna > check the gns; I'm not sure the script was updated for those yet. This git tool really handy for refactoring, thanks. https://codereview.chromium.org/822453002/diff/580001/extensions/common/consu... extensions/common/consumer.h:27: explicit ConsumerID(const ConsumerID& other); On 2015/01/20 17:51:06, Devlin wrote: > Make a note here about why we allow copying/why it's okay. It seems we don't need this copy constructor now, removed. https://codereview.chromium.org/822453002/diff/580001/extensions/common/consu... extensions/common/consumer.h:31: int instance_id); On 2015/01/20 17:51:06, Devlin wrote: > I think I'd slightly prefer an "is_declarative" bool here, and then removing the > public access to kDefaultInstanceID and GetNextID, since they are implementation > details. That is a good point. I won't add the is_declarative in this constructor, since is_declarative is used to hide the details of assignment for instance_id, and it doesn't make sense that we have both instance_id and is_declarative. Instead, I will create another constructor without instance_id but with is_declarative. https://codereview.chromium.org/822453002/diff/580001/extensions/common/consu... extensions/common/consumer.h:46: // Extension or WebUI. On 2015/01/20 17:51:06, Devlin wrote: > nit: Just say "The host type of the consumer." The enum already says that this > is either extension or webui, and there's no point in needing to update this > comment, too, when we add host type FluxCapacitor. :) Done. https://codereview.chromium.org/822453002/diff/580001/extensions/common/consu... extensions/common/consumer.h:54: InstanceType instance_type_; On 2015/01/20 17:51:06, Devlin wrote: > Similar comment to above. Done. https://codereview.chromium.org/822453002/diff/580001/extensions/common/consu... extensions/common/consumer.h:57: // |instance_id| is 0; On 2015/01/20 17:51:06, Devlin wrote: > nit: either make the semicolon a period, or fix wrapping and capitalization. Replace the semicolon with a period:) https://codereview.chromium.org/822453002/diff/580001/extensions/common/consu... extensions/common/consumer.h:65: class Consumer { On 2015/01/20 17:51:06, Devlin wrote: > When is this used? It is not used in this patch, I can just remove it for now. https://codereview.chromium.org/822453002/diff/580001/extensions/common/user_... File extensions/common/user_script.cc (right): https://codereview.chromium.org/822453002/diff/580001/extensions/common/user_... extensions/common/user_script.cc:234: int host_type; On 2015/01/20 17:51:06, Devlin wrote: > nit: initialize the ints to something reasonable (0). Done. https://codereview.chromium.org/822453002/diff/580001/extensions/common/user_... File extensions/common/user_script.h (right): https://codereview.chromium.org/822453002/diff/580001/extensions/common/user_... extensions/common/user_script.h:13: #include "base/memory/linked_ptr.h" On 2015/01/20 17:51:06, Devlin wrote: > prune Done. https://codereview.chromium.org/822453002/diff/580001/extensions/common/user_... extensions/common/user_script.h:212: const std::string& GetExtensionID() const; On 2015/01/20 17:51:06, Devlin wrote: > it's safe to have this inlined as > const std::string& extension_id() const { return consumer_id_.host_id(); } Yes, I forgot to update this the legacy code, change it back to a regular getter function.
https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:381: const RequestKey key(extension->id()); I think the typedef to RequestKey actually makes it harder for me to tell what's going on here, not to mention requiring you to make a copy of extension->id(). Can we just take out the typedef completely? https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.h (right): https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.h:157: // The lifetime of the |master_| is managed by DeclarativeUserScriptManager, How about instead "Since the lifetime of DeclarativeUserScriptMasters is managed by the ExtensionSystem, it is safe to use a raw pointer (as all extension APIs have a dependency on the system)." https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... File chrome/browser/extensions/declarative_user_script_manager.cc (right): https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... chrome/browser/extensions/declarative_user_script_manager.cc:23: const ConsumerID& consumer_id) { Thinking about this more, this means that every time a webui or extension requests a declarative script, we create a new DeclarativeUserScriptMaster, right? Wouldn't it be better to only create one per extension/webui? https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... File chrome/browser/extensions/declarative_user_script_master.h (right): https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... chrome/browser/extensions/declarative_user_script_master.h:25: virtual ~DeclarativeUserScriptMaster(); Nothing inherits this, right? We can make it a non-virtual destructor. https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... File chrome/browser/extensions/extension_user_script_loader.h (right): https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.h:12: #include "extensions/common/extension_set.h" Do we need this? https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... File chrome/browser/extensions/shared_user_script_master.cc (right): https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... chrome/browser/extensions/shared_user_script_master.cc:17: ConsumerID() /* consumer_id*/, nit: probably don't need the annotation now (a string could be anything, a consumer id is probably a consumer id :)) https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... File chrome/browser/extensions/user_script_loader.h (right): https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:45: using PathAndDefaultLocale = std::pair<base::FilePath, std::string>; nit: can we move these usings to the protected section? https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:84: using ConsumerIDSet = std::set<ConsumerID>; nit: I don't think this adds anything for readability. Let's just use std::set<ConsumerID>. https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:91: // Base and derived classes can specify their ways to load. nit: The base class doesn't specify its way to load, so you can take that part out of the comment. nit: Please mention that this has to be safe to call multiple times. https://codereview.chromium.org/822453002/diff/620001/extensions/common/consu... File extensions/common/consumer_id.cc (right): https://codereview.chromium.org/822453002/diff/620001/extensions/common/consu... extensions/common/consumer_id.cc:7: namespace { nit: newline https://codereview.chromium.org/822453002/diff/620001/extensions/common/consu... File extensions/common/consumer_id.h (right): https://codereview.chromium.org/822453002/diff/620001/extensions/common/consu... extensions/common/consumer_id.h:24: ConsumerID(HostType host_type, Hmm... I think we want to make it clear that most callers shouldn't use this constructor. A comment would probably do the trick ("// Only use this constructor for (de)serialization."). Otherwise, you could also make ConsumerID know how to pickle/unpickle itself - which isn't a bad idea, either, and is a little more OO. I'll let you decide. https://codereview.chromium.org/822453002/diff/620001/extensions/common/consu... extensions/common/consumer_id.h:27: int instance_idte); typo: instance_idte https://codereview.chromium.org/822453002/diff/620001/extensions/common/consu... extensions/common/consumer_id.h:57: // For a consumer that owns staticlly defined user scripts, the typo: staticlly -> statically https://codereview.chromium.org/822453002/diff/620001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/822453002/diff/620001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:58: UserScriptSet* script_list, nit: It's best practice to not include changes like this that only affect formatting, since it makes revision history noisier.
Patchset #17 (id:640001) has been deleted
Hi Devlin@: Thank you for point out to create a master per extension/webui. I realize that the current implementation does something different with my initial thoughts. I do plan to create one perextension/<webview>(no matter created by extensions or webUI). This keeps consistent with existing code: all the declarative user scripts created by an extension itself still share the same master, while scripts created by a <webview> will have a master per <webview>. One solution is to update the way of assigning masters, but I think a better way to do this is to change the way when creating new ConsumerID for declarative user scripts: 1) if created by extensions itself: instance_id = a specified id, like kDefaultDeclarativeInstanceID; compared with kDefaultStaticInstanceID for static user scripts. 2) if created by <webview> : instance_id = GetNextId(). Please take another to see whether you are ok with this solution. Thanks! https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:381: const RequestKey key(extension->id()); On 2015/01/21 23:25:20, Devlin wrote: > I think the typedef to RequestKey actually makes it harder for me to tell what's > going on here, not to mention requiring you to make a copy of extension->id(). > Can we just take out the typedef completely? Sorry, but no. To support <webview>, we need to extend the struct RequestKey, and is why I define it as a struct in the first place. I put a TODO on top of where the thatRequestKey is defined (refer to content_action.h line 99), hopefully this commend can explain my purpose to introduce this struct. https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.h (right): https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.h:157: // The lifetime of the |master_| is managed by DeclarativeUserScriptManager, On 2015/01/21 23:25:20, Devlin wrote: > How about instead > "Since the lifetime of DeclarativeUserScriptMasters is managed by the > ExtensionSystem, it is safe to use a raw pointer (as all extension APIs > have a dependency on the system)." Sounds better, thanks! https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... File chrome/browser/extensions/declarative_user_script_manager.cc (right): https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... chrome/browser/extensions/declarative_user_script_manager.cc:23: const ConsumerID& consumer_id) { On 2015/01/21 23:25:20, Devlin wrote: > Thinking about this more, this means that every time a webui or extension > requests a declarative script, we create a new DeclarativeUserScriptMaster, > right? Wouldn't it be better to only create one per extension/webui? This is a good point, and I realize that the current implementation does something different with my initial thoughts. I plan to create one per extension/<webview>(no matter created by extensions or webUI). This keeps consistent with existing code: all the declarative user scripts created by an extension itself still share the same master, while scripts created by a <webview> will have a master per <webview>. One solution is to update the way of assigning masters, but I think a better way to do this is to change the way when creating new ConsumerID for declarative user scripts: 1) if created by extensions itself: instance_id = a specified id, like kDefaultDeclarativeInstanceID; compared with kDefaultStaticInstanceID for static user scripts. 2) if created by <webview> : instance_id = GetNextId(). Using this way to create ConsumerID keeps the behaviors of content scripts injection for extensions unchanged in everywhere. https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... File chrome/browser/extensions/declarative_user_script_master.h (right): https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... chrome/browser/extensions/declarative_user_script_master.h:25: virtual ~DeclarativeUserScriptMaster(); On 2015/01/21 23:25:20, Devlin wrote: > Nothing inherits this, right? We can make it a non-virtual destructor. Done. https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... File chrome/browser/extensions/extension_user_script_loader.h (right): https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.h:12: #include "extensions/common/extension_set.h" On 2015/01/21 23:25:20, Devlin wrote: > Do we need this? Removed. https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... File chrome/browser/extensions/shared_user_script_master.cc (right): https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... chrome/browser/extensions/shared_user_script_master.cc:17: ConsumerID() /* consumer_id*/, On 2015/01/21 23:25:20, Devlin wrote: > nit: probably don't need the annotation now (a string could be anything, a > consumer id is probably a consumer id :)) Removed. https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... File chrome/browser/extensions/user_script_loader.h (right): https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:45: using PathAndDefaultLocale = std::pair<base::FilePath, std::string>; On 2015/01/21 23:25:20, Devlin wrote: > nit: can we move these usings to the protected section? I would love to move them, but they are used by some functions in the anonymous namespace in user_script_loader.cc. https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:84: using ConsumerIDSet = std::set<ConsumerID>; On 2015/01/21 23:25:20, Devlin wrote: > nit: I don't think this adds anything for readability. Let's just use > std::set<ConsumerID>. Done. https://codereview.chromium.org/822453002/diff/620001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.h:91: // Base and derived classes can specify their ways to load. On 2015/01/21 23:25:20, Devlin wrote: > nit: The base class doesn't specify its way to load, so you can take that part > out of the comment. > nit: Please mention that this has to be safe to call multiple times. Done. https://codereview.chromium.org/822453002/diff/620001/extensions/common/consu... File extensions/common/consumer_id.cc (right): https://codereview.chromium.org/822453002/diff/620001/extensions/common/consu... extensions/common/consumer_id.cc:7: namespace { On 2015/01/21 23:25:20, Devlin wrote: > nit: newline Done. https://codereview.chromium.org/822453002/diff/620001/extensions/common/consu... File extensions/common/consumer_id.h (right): https://codereview.chromium.org/822453002/diff/620001/extensions/common/consu... extensions/common/consumer_id.h:24: ConsumerID(HostType host_type, On 2015/01/21 23:25:21, Devlin wrote: > Hmm... I think we want to make it clear that most callers shouldn't use this > constructor. A comment would probably do the trick ("// Only use this > constructor for (de)serialization."). Otherwise, you could also make ConsumerID > know how to pickle/unpickle itself - which isn't a bad idea, either, and is a > little more OO. > > I'll let you decide. I slightly prefer to add the common here. I believe this constructor will be used in the render side when they receive IPC message from the browser and reconstruct ConsumerID. https://codereview.chromium.org/822453002/diff/620001/extensions/common/consu... extensions/common/consumer_id.h:27: int instance_idte); On 2015/01/21 23:25:20, Devlin wrote: > typo: instance_idte Updated. https://codereview.chromium.org/822453002/diff/620001/extensions/common/consu... extensions/common/consumer_id.h:57: // For a consumer that owns staticlly defined user scripts, the On 2015/01/21 23:25:20, Devlin wrote: > typo: staticlly -> statically Done. https://codereview.chromium.org/822453002/diff/620001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/822453002/diff/620001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:58: UserScriptSet* script_list, On 2015/01/21 23:25:21, Devlin wrote: > nit: It's best practice to not include changes like this that only affect > formatting, since it makes revision history noisier. Sorry, git cl format does these unnecessary changes. I will changes them back in the future.
On 2015/01/22 17:19:37, Xi Han wrote: > Hi Devlin@: Thank you for point out to create a master per extension/webui. I > realize that the current implementation does something different with my initial > thoughts. I do plan to create one perextension/<webview>(no matter created by > extensions or webUI). This keeps consistent with existing code: all the > declarative user scripts created by an extension itself still share the same > master, while scripts created by a <webview> will have a master per <webview>. > > One solution is to update the way of assigning masters, but I think a better way > to do this is to change the way when creating new ConsumerID for declarative > user scripts: > 1) if created by extensions itself: instance_id = a specified id, like > kDefaultDeclarativeInstanceID; compared with kDefaultStaticInstanceID for static > user scripts. > 2) if created by <webview> : instance_id = GetNextId(). I'm still not as familiar with all this as I'd like, but I don't think that's quite right either. <webview>s themselves cannot create content scripts, right? It's either an extension or a webui which creates the content script, and is injected into either a tab or a webview, I thought. Hence, the instance_id and host_id. I think that there should be one DeclarativeUserScriptMaster per _host_ (either extension or webui), which manages all scripts for that host. The instance_id is important, and can't simply be kDeclarativeInstanceID, because we may want to differentiate scripts injected by an extension into Tab A versus Tab B versus <webview> 1 (and these can't all have the same instance id for declarative scripts). So isn't the solution to just key the user script masters based on host_id, so that each webui/extension gets one for declarative scripts? Perhaps I'm missing something...
On 2015/01/22 17:40:54, Devlin wrote: > On 2015/01/22 17:19:37, Xi Han wrote: > > Hi Devlin@: Thank you for point out to create a master per extension/webui. I > > realize that the current implementation does something different with my > initial > > thoughts. I do plan to create one perextension/<webview>(no matter created by > > extensions or webUI). This keeps consistent with existing code: all the > > declarative user scripts created by an extension itself still share the same > > master, while scripts created by a <webview> will have a master per <webview>. > > > > > One solution is to update the way of assigning masters, but I think a better > way > > to do this is to change the way when creating new ConsumerID for declarative > > user scripts: > > 1) if created by extensions itself: instance_id = a specified id, like > > kDefaultDeclarativeInstanceID; compared with kDefaultStaticInstanceID for > static > > user scripts. > > 2) if created by <webview> : instance_id = GetNextId(). > > I'm still not as familiar with all this as I'd like, but I don't think that's > quite right either. <webview>s themselves cannot create content scripts, right? > It's either an extension or a webui which creates the content script, and is > injected into either a tab or a webview, I thought. Hence, the instance_id and > host_id. I think that there should be one DeclarativeUserScriptMaster per > _host_ (either extension or webui), which manages all scripts for that host. > The instance_id is important, and can't simply be kDeclarativeInstanceID, > because we may want to differentiate scripts injected by an extension into Tab A > versus Tab B versus <webview> 1 (and these can't all have the same instance id > for declarative scripts). > > So isn't the solution to just key the user script masters based on host_id, so > that each webui/extension gets one for declarative scripts? Perhaps I'm missing > something... No, the current model is each <webview> is isolated from other <webview>s even if they have the same embedder.
On 2015/01/22 17:43:35, Fady Samuel wrote: > On 2015/01/22 17:40:54, Devlin wrote: > > On 2015/01/22 17:19:37, Xi Han wrote: > > > Hi Devlin@: Thank you for point out to create a master per extension/webui. > I > > > realize that the current implementation does something different with my > > initial > > > thoughts. I do plan to create one perextension/<webview>(no matter created > by > > > extensions or webUI). This keeps consistent with existing code: all the > > > declarative user scripts created by an extension itself still share the same > > > master, while scripts created by a <webview> will have a master per > <webview>. > > > > > > > > One solution is to update the way of assigning masters, but I think a better > > way > > > to do this is to change the way when creating new ConsumerID for declarative > > > user scripts: > > > 1) if created by extensions itself: instance_id = a specified id, like > > > kDefaultDeclarativeInstanceID; compared with kDefaultStaticInstanceID for > > static > > > user scripts. > > > 2) if created by <webview> : instance_id = GetNextId(). > > > > I'm still not as familiar with all this as I'd like, but I don't think that's > > quite right either. <webview>s themselves cannot create content scripts, > right? > > It's either an extension or a webui which creates the content script, and is > > injected into either a tab or a webview, I thought. Hence, the instance_id > and > > host_id. I think that there should be one DeclarativeUserScriptMaster per > > _host_ (either extension or webui), which manages all scripts for that host. > > The instance_id is important, and can't simply be kDeclarativeInstanceID, > > because we may want to differentiate scripts injected by an extension into Tab > A > > versus Tab B versus <webview> 1 (and these can't all have the same instance id > > for declarative scripts). > > > > So isn't the solution to just key the user script masters based on host_id, so > > that each webui/extension gets one for declarative scripts? Perhaps I'm > missing > > something... > > No, the current model is each <webview> is isolated from other <webview>s even > if they have the same embedder.
> On 2015/01/22 17:43:35, Fady Samuel wrote: > > No, the current model is each <webview> is isolated from other <webview>s even > > if they have the same embedder. (Sorry for the empty; twitchy fingers). Right, each webview is separate and isolated, just as each tab is separate and isolated. But my understanding is that webviews don't create content scripts - they are scripted upon by either an extension or a webui. Since the DeclarativeUserScriptMaster is in charge of managing all the scripts for a "thing which scripts" (just webui and extension, right?), we shouldn't need a separate one for each webview we script upon - just as we don't need a separate one for each tab we script upon.
On 2015/01/22 17:59:28, Devlin wrote: > > On 2015/01/22 17:43:35, Fady Samuel wrote: > > > No, the current model is each <webview> is isolated from other <webview>s > even > > > if they have the same embedder. > > (Sorry for the empty; twitchy fingers). > > Right, each webview is separate and isolated, just as each tab is separate and > isolated. But my understanding is that webviews don't create content scripts - > they are scripted upon by either an extension or a webui. Since the > DeclarativeUserScriptMaster is in charge of managing all the scripts for a > "thing which scripts" (just webui and extension, right?), we shouldn't need a > separate one for each webview we script upon - just as we don't need a separate > one for each tab we script upon. True, as long as injecting a content script in one <webview> doesn't inject it in another. The memory containing ALL content scripts can be shared.
On 2015/01/22 18:04:13, Fady Samuel wrote: > True, as long as injecting a content script in one <webview> doesn't inject it > in another. The memory containing ALL content scripts can be shared. Right. IIRC, Mark made a DeclarativeUserScriptMaster per extension so that it was easy to manage scripts on a per-extension basis (as the extension is unloaded or the rule is removed, e.g.). It would probably be a good refactor to make just one static master and one declarative master, but we haven't gotten around to it yet. But keeping with the current idiom, each host (extension/webview) should have one master for declarative scripts - there's no reason to have one per script injected anywhere.
Patchset #19 (id:700001) has been deleted
Patchset #18 (id:680001) has been deleted
Hi reviewers: As discussed offline, the isolation of applying content scripts (rules) between <webview>s and tabs should happen on the browser side, more precisely, in ChromeContentRulesRegistry::Apply(...). Beside css selector and url pattern, a rule registry number can be used when choosing matched rules. DeclarativeUserScriptMaster or UserScriptLoader has nothing to do with the isolation. This simplifies the changes needed in the script injection system. So I remove ConsumerID and introduce HostID (type, id). The "id" could be an extension_id, or a url of webUI. Since DeclarativeUserScriptMaster is assigned based on HostId, so it is still one per extension/webUI.
On 2015/01/23 21:13:31, Xi Han wrote: > Hi reviewers: > As discussed offline, the isolation of applying content scripts (rules) between > <webview>s and tabs should happen on the browser side, more precisely, in > ChromeContentRulesRegistry::Apply(...). Beside css selector and url pattern, a > rule registry number can be used when choosing matched rules. > DeclarativeUserScriptMaster or UserScriptLoader has nothing to do with the > isolation. This simplifies the changes needed in the script injection system. So > I remove ConsumerID and introduce HostID (type, id). The "id" could be an > extension_id, or a url of webUI. Since DeclarativeUserScriptMaster is assigned > based on HostId, so it is still one per extension/webUI. Changes to simplify the declarative masters SGTM. Will we still need a consumer id (specifically, instance id) on the renderer side for injection?
> Changes to simplify the declarative masters SGTM. Will we still need a consumer > id (specifically, instance id) on the renderer side for injection? I think we do need some id on the renderer side to choose an isolated world for injection, but it could be other kinds of id. This id will sent out when the declarative rules are applied (i.e., ChromeContentRulesRegistry::Apply() is called), and plumb to RequestContentScript::InstructRenderProcessToInject() where IPC ExtensionMsg_ExecuteDeclarativeScript is sent to renderer. We can use WebContents to obtain the partition id (a string) for <webview>, if the content script will be injected on <webview>; use extension_id if the scripts will be injected on tabs of an extension. The picked string will be sent to render along with the IPC ExtensionMsg_ExecuteDeclarativeScript, so the renderer can use this id to pick up an number of isolated world. I plan to have these changes on a separate patch, probably with the changes on the renderer side, since this patch is already large. Let me know if you have any concern or any thing that I might miss.
You'll also need to update the description of the CL. https://codereview.chromium.org/822453002/diff/720001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.h (right): https://codereview.chromium.org/822453002/diff/720001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.h:71: const Extension* extension, We're already passing in an extension to all of these different methods. Do we really need to pass in a host id, rather than having the action construct a host id from the extension object? https://codereview.chromium.org/822453002/diff/720001/chrome/browser/extensio... File chrome/browser/extensions/extension_user_script_loader.cc (right): https://codereview.chromium.org/822453002/diff/720001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:107: } nit: newline https://codereview.chromium.org/822453002/diff/720001/chrome/browser/extensio... File chrome/browser/extensions/extension_user_script_loader.h (right): https://codereview.chromium.org/822453002/diff/720001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.h:21: class ContentVerifier; Delete. https://codereview.chromium.org/822453002/diff/720001/chrome/browser/extensio... File chrome/browser/extensions/user_script_loader.cc (right): https://codereview.chromium.org/822453002/diff/720001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:479: // TODO(hanxi): update the IPC message to send a set of HostID to render. nit: "... a set of HostIDs to the renderer." https://codereview.chromium.org/822453002/diff/720001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:492: for (const UserScript& script : scripts) I realize that the code was like this before, but let's just inline this where it's called. It's not worth having a separate method for two lines. https://codereview.chromium.org/822453002/diff/720001/chrome/common/extension... File chrome/common/extensions/manifest_handlers/content_scripts_handler.cc (right): https://codereview.chromium.org/822453002/diff/720001/chrome/common/extension... chrome/common/extensions/manifest_handlers/content_scripts_handler.cc:424: user_script.set_host_id(id); nit: inline HostID construction. https://codereview.chromium.org/822453002/diff/720001/extensions/common/BUILD.gn File extensions/common/BUILD.gn (right): https://codereview.chromium.org/822453002/diff/720001/extensions/common/BUILD... extensions/common/BUILD.gn:49: "consumer_id.h", host_id https://codereview.chromium.org/822453002/diff/720001/extensions/common/host_... File extensions/common/host_id.h (right): https://codereview.chromium.org/822453002/diff/720001/extensions/common/host_... extensions/common/host_id.h:30: HostType Type() const { return type; } nit: lowercase these -> type() and id() https://codereview.chromium.org/822453002/diff/720001/extensions/common/host_... extensions/common/host_id.h:35: HostType type; Since these are private, they should have a trailing _
https://codereview.chromium.org/822453002/diff/720001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.h (right): https://codereview.chromium.org/822453002/diff/720001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.h:71: const Extension* extension, On 2015/01/26 20:12:08, Devlin wrote: > We're already passing in an extension to all of these different methods. Do we > really need to pass in a host id, rather than having the action construct a host > id from the extension object? Yes, because for webUI, we want to pass in the id of webUI (e.g., a string of its url). This id can only be created when <webview> is registering the declarative content rules. I think the confusion here is caused by creating a HostID from ChromeContentRulesRegistry, in fact the HostID should be created in declarative_api.cc and plumbed to here. The rule registry part will be updated in a following patch, since de-couple extension is also required. https://codereview.chromium.org/822453002/diff/720001/chrome/browser/extensio... File chrome/browser/extensions/extension_user_script_loader.cc (right): https://codereview.chromium.org/822453002/diff/720001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.cc:107: } On 2015/01/26 20:12:08, Devlin wrote: > nit: newline Done. https://codereview.chromium.org/822453002/diff/720001/chrome/browser/extensio... File chrome/browser/extensions/extension_user_script_loader.h (right): https://codereview.chromium.org/822453002/diff/720001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.h:21: class ContentVerifier; On 2015/01/26 20:12:08, Devlin wrote: > Delete. Done. https://codereview.chromium.org/822453002/diff/720001/chrome/browser/extensio... File chrome/browser/extensions/user_script_loader.cc (right): https://codereview.chromium.org/822453002/diff/720001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:479: // TODO(hanxi): update the IPC message to send a set of HostID to render. On 2015/01/26 20:12:09, Devlin wrote: > nit: "... a set of HostIDs to the renderer." Done. https://codereview.chromium.org/822453002/diff/720001/chrome/browser/extensio... chrome/browser/extensions/user_script_loader.cc:492: for (const UserScript& script : scripts) On 2015/01/26 20:12:08, Devlin wrote: > I realize that the code was like this before, but let's just inline this where > it's called. It's not worth having a separate method for two lines. That is fair:) https://codereview.chromium.org/822453002/diff/720001/chrome/common/extension... File chrome/common/extensions/manifest_handlers/content_scripts_handler.cc (right): https://codereview.chromium.org/822453002/diff/720001/chrome/common/extension... chrome/common/extensions/manifest_handlers/content_scripts_handler.cc:424: user_script.set_host_id(id); On 2015/01/26 20:12:09, Devlin wrote: > nit: inline HostID construction. Done. https://codereview.chromium.org/822453002/diff/720001/extensions/common/BUILD.gn File extensions/common/BUILD.gn (right): https://codereview.chromium.org/822453002/diff/720001/extensions/common/BUILD... extensions/common/BUILD.gn:49: "consumer_id.h", On 2015/01/26 20:12:09, Devlin wrote: > host_id Great catch! How can I forget to update here:( https://codereview.chromium.org/822453002/diff/720001/extensions/common/host_... File extensions/common/host_id.h (right): https://codereview.chromium.org/822453002/diff/720001/extensions/common/host_... extensions/common/host_id.h:30: HostType Type() const { return type; } On 2015/01/26 20:12:09, Devlin wrote: > nit: lowercase these -> type() and id() Done. https://codereview.chromium.org/822453002/diff/720001/extensions/common/host_... extensions/common/host_id.h:35: HostType type; On 2015/01/26 20:12:09, Devlin wrote: > Since these are private, they should have a trailing _ Oh, I thought the member variables of a struct wouldn't have a trailing _. Updated.
lgtm with nits. https://codereview.chromium.org/822453002/diff/740001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.h (right): https://codereview.chromium.org/822453002/diff/740001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.h:100: const HostID& host_id, This still kind of makes me sad that we plumb these through everywhere. Can you at least add a TODO(hanxi) to clean it up in subsequent patches? https://codereview.chromium.org/822453002/diff/740001/chrome/browser/extensio... File chrome/browser/extensions/extension_user_script_loader.h (right): https://codereview.chromium.org/822453002/diff/740001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader.h:28: // the Exttension System, e.g, when constructs SharedUserScriptMaster in typo: Exxtension https://codereview.chromium.org/822453002/diff/740001/extensions/common/host_... File extensions/common/host_id.h (right): https://codereview.chromium.org/822453002/diff/740001/extensions/common/host_... extensions/common/host_id.h:10: #include "base/macros.h" Don't need this, right? https://codereview.chromium.org/822453002/diff/740001/extensions/common/host_... extensions/common/host_id.h:26: else nit: remove this else
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/822453002/760001
Message was sent while issue was closed.
Committed patchset #20 (id:760001)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/b88fe3dc1072501bdd105fd95a8b3bc453fd2aa7 Cr-Commit-Position: refs/heads/master@{#313402}
Message was sent while issue was closed.
A revert of this CL (patchset #20 id:760001) has been created in https://codereview.chromium.org/899983002/ by hanxi@chromium.org. The reason for reverting is: It might cause the crash https://code.google.com/p/chromium/issues/detail?id=454917.. |
