Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
344433003:
Prepare declarativeContent API for new script injection feature. Added Javascript types and functio… (Closed)
|
Created:
6 years, 6 months ago by Mark Dittmer Modified:
6 years, 5 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, darin-cc_chromium.org, jam, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionPrepare declarativeContent API for new script injection feature. Added Javascript types and functions, as well as C++ ContentAction that currently does nothing.
BUG=377978
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284049
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Fix regression by setting up a ContentWatcher for every ContentRulesRegistry #Patch Set 4 : Bind ContentScript and RequestContentScript to declarativeContent API #Patch Set 5 : Add WatchedPagesRecipient as indicator for where to send ContentWatcher updates #Patch Set 6 : Add note about what it means to revert RequestContentScript #
Total comments: 12
Patch Set 7 : Address first round review comments and fix test build failure #
Total comments: 4
Patch Set 8 : Get rid of ContentScript Javascript type; only RequestContentScript is needed #
Total comments: 2
Patch Set 9 : Add description to new Javascript events #
Total comments: 15
Patch Set 10 : Address minor issues: temporary collection as array, less typedef #Patch Set 11 : Greatly simplified: Got rid of WatchedPagesRecipient and new event types #
Total comments: 6
Patch Set 12 : Clean up declarative_content.json and store RequestContentScript data in action object #
Total comments: 25
Patch Set 13 : Undo multiplicity of registries and content watchers. Address code review comments. #
Total comments: 14
Patch Set 14 : Implement fixes from comments and add unit tests #
Total comments: 4
Patch Set 15 : Update input validation patterns #
Total comments: 4
Patch Set 16 : Break input verification into if applicable, then validate #Messages
Total messages: 27 (0 generated)
Hey Fady, would you please take a first pass at this CL? I'll add Ben as a reviewer (for owners review) after you've had a chance to look at it.
An initial set of comments. https://codereview.chromium.org/344433003/diff/80002/chrome/browser/extension... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/344433003/diff/80002/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/content_action.cc:98: class RequestContentScript : public ContentAction { I feel like this should be called InjectContentScript, but I don't want to bikeshed. I don't feel too strongly about the naming. https://codereview.chromium.org/344433003/diff/80002/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/content_action.cc:112: } Add empty line after this. https://codereview.chromium.org/344433003/diff/80002/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/content_action.cc:118: } Add empty line after this. https://codereview.chromium.org/344433003/diff/80002/chrome/browser/extension... File chrome/browser/extensions/api/declarative_content/content_rules_registry.cc (right): https://codereview.chromium.org/344433003/diff/80002/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/content_rules_registry.cc:309: it != watched_css_selectors_.end(); ++it) { This is a do-nothing for loop? WhY? https://codereview.chromium.org/344433003/diff/80002/chrome/browser/extension... File chrome/browser/extensions/tab_helper.cc (right): https://codereview.chromium.org/344433003/diff/80002/chrome/browser/extension... chrome/browser/extensions/tab_helper.cc:258: it->second->DidNavigateMainFrame(web_contents(), This seems a bit weird. It seems like it breaks encapsulation. Perhaps you can move this code into RulesRegistryService? https://codereview.chromium.org/344433003/diff/80002/extensions/renderer/disp... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/344433003/diff/80002/extensions/renderer/disp... extensions/renderer/dispatcher.cc:806: content_watchers_.insert(std::make_pair(event_name, Array notation preferred.
Addressed first round comments and fixed test build failure. Ben, could you please review this as owner? https://codereview.chromium.org/344433003/diff/80002/chrome/browser/extension... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/344433003/diff/80002/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/content_action.cc:98: class RequestContentScript : public ContentAction { On 2014/06/23 18:56:28, Fady Samuel wrote: > I feel like this should be called InjectContentScript, but I don't want to > bikeshed. I don't feel too strongly about the naming. I agree, actually. What do you think kalman@? https://codereview.chromium.org/344433003/diff/80002/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/content_action.cc:112: } On 2014/06/23 18:56:28, Fady Samuel wrote: > Add empty line after this. Done. https://codereview.chromium.org/344433003/diff/80002/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/content_action.cc:118: } On 2014/06/23 18:56:28, Fady Samuel wrote: > Add empty line after this. Done. https://codereview.chromium.org/344433003/diff/80002/chrome/browser/extension... File chrome/browser/extensions/api/declarative_content/content_rules_registry.cc (right): https://codereview.chromium.org/344433003/diff/80002/chrome/browser/extension... chrome/browser/extensions/api/declarative_content/content_rules_registry.cc:309: it != watched_css_selectors_.end(); ++it) { On 2014/06/23 18:56:28, Fady Samuel wrote: > This is a do-nothing for loop? WhY? I couldn't tell you. Probably left over from some experimental refactoring of the IPC workflow. Got rid of it. https://codereview.chromium.org/344433003/diff/80002/chrome/browser/extension... File chrome/browser/extensions/tab_helper.cc (right): https://codereview.chromium.org/344433003/diff/80002/chrome/browser/extension... chrome/browser/extensions/tab_helper.cc:258: it->second->DidNavigateMainFrame(web_contents(), On 2014/06/23 18:56:28, Fady Samuel wrote: > This seems a bit weird. It seems like it breaks encapsulation. Perhaps you can > move this code into RulesRegistryService? That would be nice. RulesRegistryService already has a notification system (namely, NotifyRegistriesHelper), but it only supports passing the extension ID. Is this special case really important enough to implement a DidNavigateMainFrame at the registry service level? https://codereview.chromium.org/344433003/diff/80002/extensions/renderer/disp... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/344433003/diff/80002/extensions/renderer/disp... extensions/renderer/dispatcher.cc:806: content_watchers_.insert(std::make_pair(event_name, On 2014/06/23 18:56:28, Fady Samuel wrote: > Array notation preferred. Done.
I made some comments on the schema, but I'd prefer Jeffrey to review this CL. https://codereview.chromium.org/344433003/diff/110001/chrome/common/extension... File chrome/common/extensions/api/declarative_content.json (right): https://codereview.chromium.org/344433003/diff/110001/chrome/common/extension... chrome/common/extensions/api/declarative_content.json:69: "callback": { We weren't going to have a callback, right? https://codereview.chromium.org/344433003/diff/110001/chrome/common/extension... chrome/common/extensions/api/declarative_content.json:81: "id": "RequestContentScript", I don't think we need 2 different types here. If it's actually useful separating the idioms - a content script which always attempts to be injected, or a content script which fails if it can't be injected - then it could be a flag on the [Request]ContentScript type.
Got rid of ContentScript Javascript type; only need RequestContentScript. https://codereview.chromium.org/344433003/diff/110001/chrome/common/extension... File chrome/common/extensions/api/declarative_content.json (right): https://codereview.chromium.org/344433003/diff/110001/chrome/common/extension... chrome/common/extensions/api/declarative_content.json:69: "callback": { On 2014/06/24 15:06:43, kalman wrote: > We weren't going to have a callback, right? Done. https://codereview.chromium.org/344433003/diff/110001/chrome/common/extension... chrome/common/extensions/api/declarative_content.json:81: "id": "RequestContentScript", On 2014/06/24 15:06:43, kalman wrote: > I don't think we need 2 different types here. If it's actually useful separating > the idioms - a content script which always attempts to be injected, or a content > script which fails if it can't be injected - then it could be a flag on the > [Request]ContentScript type. Done.
schema lgtm but run it by Jeffrey too (and the C++ code of course) https://codereview.chromium.org/344433003/diff/130001/chrome/common/extension... File chrome/common/extensions/api/declarative_content.json (right): https://codereview.chromium.org/344433003/diff/130001/chrome/common/extension... chrome/common/extensions/api/declarative_content.json:113: "name": "onDocumentIdle", please document these 3 events (makes sense to use the same text that the content scripts documentation uses)
https://codereview.chromium.org/344433003/diff/130001/chrome/common/extension... File chrome/common/extensions/api/declarative_content.json (right): https://codereview.chromium.org/344433003/diff/130001/chrome/common/extension... chrome/common/extensions/api/declarative_content.json:113: "name": "onDocumentIdle", On 2014/06/25 14:38:57, kalman wrote: > please document these 3 events (makes sense to use the same text that the > content scripts documentation uses) Done.
https://codereview.chromium.org/344433003/diff/150001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative/rules_registry_service.cc (right): https://codereview.chromium.org/344433003/diff/150001/chrome/browser/extensio... chrome/browser/extensions/api/declarative/rules_registry_service.cc:96: typedef std::vector< std::pair<std::string, WatchedPagesRecipient> > For something with a constant set of elements, I'd just use a C array. It's also almost always more readable to use a locally-defined struct type, rather than std::pair. You can't use a local type as a template argument, but that problem goes away with an array. https://codereview.chromium.org/344433003/diff/150001/chrome/browser/extensio... chrome/browser/extensions/api/declarative/rules_registry_service.cc:128: content_rules_registry.get())); This is going to use the registry after it's been freed, since the content_rules_registry_map_ stores a raw pointer. https://codereview.chromium.org/344433003/diff/150001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/344433003/diff/150001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:125: // unload user script. It's impossible to unload a user script. These actions just need to be un-revertable. You might need to add some logic so the Apply isn't called twice for the same page. https://codereview.chromium.org/344433003/diff/150001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_rules_registry.h (right): https://codereview.chromium.org/344433003/diff/150001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_rules_registry.h:70: const WatchedPagesRecipient watched_pages_recipient, Either use const WatchedPagesRecipient& or just WatchedPagesRecipient in the header file. You can still make it a const value in the .cc file. https://codereview.chromium.org/344433003/diff/150001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_rules_registry.h:155: typedef std::map<std::string, ContentRulesRegistry*> ContentRulesRegistryMap; Generally avoid typedefs like this, since they make it harder to tell what's being mapped at the point they're actually used. https://codereview.chromium.org/344433003/diff/150001/chrome/common/extension... File chrome/common/extensions/api/declarative_content.json (right): https://codereview.chromium.org/344433003/diff/150001/chrome/common/extension... chrome/common/extensions/api/declarative_content.json:91: "name": "onDocumentStart", I'm skeptical of adding three new events for this, and especially of treating them all separately in the browser code. Semantically, one way to view the run_at restriction might be to think of it as another predicate on the PageStateMatcher: minLoadPhase ∈ ['document_start', 'document_end', 'document_idle']. We could have this default to 'document_idle' to match run_at, although we wouldn't really want it to slow down the appearance of a page action. https://codereview.chromium.org/344433003/diff/150001/extensions/common/watch... File extensions/common/watched_pages_recipient.h (right): https://codereview.chromium.org/344433003/diff/150001/extensions/common/watch... extensions/common/watched_pages_recipient.h:11: CONTENT_RULES_REGISTRY = 0, Definitely comment what these mean. If possible, though, it'd be nice to avoid the need for this enum.
Added my thoughts to many comments. Any comments without reply will soon be
replied as "Done" (working on that now).
kalman@, please let me know your latest thoughts on
three-events-vs-condition-variable for {"document_start", "document_end",
"document_idle"}.
https://codereview.chromium.org/344433003/diff/150001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative/rules_registry_service.cc
(right):
https://codereview.chromium.org/344433003/diff/150001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative/rules_registry_service.cc:128:
content_rules_registry.get()));
On 2014/06/27 22:03:41, Jeffrey Yasskin wrote:
> This is going to use the registry after it's been freed, since the
> content_rules_registry_map_ stores a raw pointer.
How is this different from how the code was before? I thought the idea was that
we keep a "convenient" raw pointer to the content rules registry/registries that
is only accessed in code paths that are outlived by the scoped (managed)
pointer.
https://codereview.chromium.org/344433003/diff/150001/chrome/browser/extensio...
File chrome/browser/extensions/api/declarative_content/content_action.cc
(right):
https://codereview.chromium.org/344433003/diff/150001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/content_action.cc:125: //
unload user script.
On 2014/06/27 22:03:41, Jeffrey Yasskin wrote:
> It's impossible to unload a user script. These actions just need to be
> un-revertable. You might need to add some logic so the Apply isn't called
twice
> for the same page.
Perhaps this comment should say "stop future loads of user script".
I thought that UserScriptMaster can be told to reload its set of scripts. Why
can't this functionality be extended to remove a declarative script from the
list, then reload its set of scripts? It's not clear to me what happens when I
invoke removeRules(...) on a rule for script injection if it's impossible to ask
UserScriptMaster to stop loading the script.
https://codereview.chromium.org/344433003/diff/150001/chrome/common/extension...
File chrome/common/extensions/api/declarative_content.json (right):
https://codereview.chromium.org/344433003/diff/150001/chrome/common/extension...
chrome/common/extensions/api/declarative_content.json:91: "name":
"onDocumentStart",
On 2014/06/27 22:03:41, Jeffrey Yasskin wrote:
> I'm skeptical of adding three new events for this, and especially of treating
> them all separately in the browser code. Semantically, one way to view the
> run_at restriction might be to think of it as another predicate on the
> PageStateMatcher: minLoadPhase ∈ ['document_start', 'document_end',
> 'document_idle']. We could have this default to 'document_idle' to match
run_at,
> although we wouldn't really want it to slow down the appearance of a page
> action.
Ben and I discussed this and we both felt that the separate events are more in
keeping with an event-based API. Having a predicate that denotes the "when"
(rather than the choice of event denoting this) feels odd in an events-based
approach. Is the concern memory consumption for managing the rules registries,
or do you feel that developers will be confused/overwhelmed by the number of
events?
Aside: I think "minLoadPhase" would be a misleading choice. We skip scripts that
missed document_start or document_end on the basis that multiple scripts with
different insertion times (but from the same extension) may expect that they not
be inserted out of order.
https://codereview.chromium.org/344433003/diff/150001/extensions/common/watch...
File extensions/common/watched_pages_recipient.h (right):
https://codereview.chromium.org/344433003/diff/150001/extensions/common/watch...
extensions/common/watched_pages_recipient.h:11: CONTENT_RULES_REGISTRY = 0,
On 2014/06/27 22:03:41, Jeffrey Yasskin wrote:
> Definitely comment what these mean. If possible, though, it'd be nice to avoid
> the need for this enum.
We could certainly change this to a boolean, but I feel that there are two
drawbacks to that change:
1. What do we name it? replyToBrowser captures the (non-)IPC'ness of the
behavior, but obscures where the data is sent when there is no IPC.
replyToScriptManagerOrRulesRegistry is already too verbose, and it doesn't even
indicate which is true and which is false. I think your point that comments are
needed here highlights why an enum is probably the right choice.
2. If we switch to a boolean and later end up routing data to a third component,
we'll have to change the type in the message rather than simply augment it.
If by your comment you mean that it would be nice to avoid sometimes sending to
the browser process and sometimes not, I agree. Unfortunately, it doesn't look
like we can achieve this (based on our recent email conversations about timely
browser notification of page loads).
Addressed remaining comments: "Done". https://codereview.chromium.org/344433003/diff/150001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative/rules_registry_service.cc (right): https://codereview.chromium.org/344433003/diff/150001/chrome/browser/extensio... chrome/browser/extensions/api/declarative/rules_registry_service.cc:96: typedef std::vector< std::pair<std::string, WatchedPagesRecipient> > On 2014/06/27 22:03:41, Jeffrey Yasskin wrote: > For something with a constant set of elements, I'd just use a C array. It's also > almost always more readable to use a locally-defined struct type, rather than > std::pair. You can't use a local type as a template argument, but that problem > goes away with an array. Done. https://codereview.chromium.org/344433003/diff/150001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_rules_registry.h (right): https://codereview.chromium.org/344433003/diff/150001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_rules_registry.h:70: const WatchedPagesRecipient watched_pages_recipient, On 2014/06/27 22:03:41, Jeffrey Yasskin wrote: > Either use const WatchedPagesRecipient& or just WatchedPagesRecipient in the > header file. You can still make it a const value in the .cc file. Done. https://codereview.chromium.org/344433003/diff/150001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_rules_registry.h:155: typedef std::map<std::string, ContentRulesRegistry*> ContentRulesRegistryMap; On 2014/06/27 22:03:41, Jeffrey Yasskin wrote: > Generally avoid typedefs like this, since they make it harder to tell what's > being mapped at the point they're actually used. Done. Still use typedef within method in another context where typename is required multiple times.
Just uploaded simplified implementation without WatchedPagesRecipient and new event objects. kalman@ and/or jyasskin@, still need final review/LGTM on bulk of C++. kenrb@, would you please do an owners review for the extensions messages?
ipc lgtm
yeah Jeffrey is certainly the owner of the C++ code here so I'd rather he review that than me. I have a nit and a couple of comments on the JSON. https://codereview.chromium.org/344433003/diff/190001/chrome/common/extension... File chrome/common/extensions/api/declarative_content.json (right): https://codereview.chromium.org/344433003/diff/190001/chrome/common/extension... chrome/common/extensions/api/declarative_content.json:60: "description": "Names of CSS files to be injected as a part of content script.", "...part of the content script" same below https://codereview.chromium.org/344433003/diff/190001/chrome/common/extension... chrome/common/extensions/api/declarative_content.json:68: }, can we also support "allFrames" and "matchAboutBlank" in here? not all of the tabs.executeScript arguments make sense (https://developer.chrome.com/extensions/tabs#type-InjectDetails), but those do. https://codereview.chromium.org/344433003/diff/190001/chrome/common/extension... chrome/common/extensions/api/declarative_content.json:87: "declarativeContent.RequestContentScript" nit: alphabetic
Made API changes recommended by kalman@, and actually store RequestContentScript parameters in action object. https://codereview.chromium.org/344433003/diff/190001/chrome/common/extension... File chrome/common/extensions/api/declarative_content.json (right): https://codereview.chromium.org/344433003/diff/190001/chrome/common/extension... chrome/common/extensions/api/declarative_content.json:60: "description": "Names of CSS files to be injected as a part of content script.", On 2014/07/08 15:25:59, kalman wrote: > "...part of the content script" same below Done. https://codereview.chromium.org/344433003/diff/190001/chrome/common/extension... chrome/common/extensions/api/declarative_content.json:68: }, On 2014/07/08 15:25:59, kalman wrote: > can we also support "allFrames" and "matchAboutBlank" in here? > > not all of the tabs.executeScript arguments make sense > (https://developer.chrome.com/extensions/tabs#type-InjectDetails), but those do. Done. Also added code to store these in RequestContentScript action objects. https://codereview.chromium.org/344433003/diff/190001/chrome/common/extension... chrome/common/extensions/api/declarative_content.json:87: "declarativeContent.RequestContentScript" On 2014/07/08 15:25:58, kalman wrote: > nit: alphabetic Done.
https://codereview.chromium.org/344433003/diff/150001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative/rules_registry_service.cc (right): https://codereview.chromium.org/344433003/diff/150001/chrome/browser/extensio... chrome/browser/extensions/api/declarative/rules_registry_service.cc:128: content_rules_registry.get())); On 2014/06/30 11:50:35, Mark Dittmer wrote: > On 2014/06/27 22:03:41, Jeffrey Yasskin wrote: > > This is going to use the registry after it's been freed, since the > > content_rules_registry_map_ stores a raw pointer. > > How is this different from how the code was before? I thought the idea was that > we keep a "convenient" raw pointer to the content rules registry/registries that > is only accessed in code paths that are outlived by the scoped (managed) > pointer. Whoops, you're right. Never mind. https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative/rules_registry_service.cc (right): https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... chrome/browser/extensions/api/declarative/rules_registry_service.cc:176: return content_rules_registry_map_[event_name]; If you've already run map::find, you can just return *it, rather than looking up the key again. https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative/rules_registry_service.h (right): https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... chrome/browser/extensions/api/declarative/rules_registry_service.h:143: // Collection of content rule registries to make it easier to handle content You've removed the information about what structure owns these pointers. That was important, so please put it back. 'course, this change might not be necessary anymore since there's only 1 event at the moment. Even when there are multiple events in the API, we might still want to track them with a single registry, since they affect when the actions fire, rather than what information is stored. https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:32: const char kInvalidTypeOfParamter[] = "Attribute '%s' has an invalid type"; sp: "Paramter" It's also more user-friendly to say what the type needs to be. This may mean you need more strings. https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:101: RequestContentScript() {} Give this constructor parameters matching the values that need to be stored, instead of side-loading them after constructing the object. https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:107: friend scoped_refptr<ContentAction> RequestContentScript::Create( You don't need to friend members of the same class. https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:129: // do not load user script in the future. s/in the future/if Apply() runs again on the same page/ https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:134: DISALLOW_COPY_AND_ASSIGN(RequestContentScript); This goes last in the class: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:143: bool AppendJSStringsToCPPStrings(const base::ListValue& append_strings, This should be static to be sure it doesn't collide with a helper in another .cc file. https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:144: std::vector<std::string>& append_to) { Chrome doesn't use non-const reference arguments: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Refere... https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:170: if (attribute_name == "css" || attribute_name == "js") { I would probably look up the attributes I want instead of iterating all attributes, but this isn't terrible. https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:199: delete request_content_script; Try never to have to write "delete foo" at the end of a function: it makes the function more fragile. Instead, capture the pointer into a scoped_refptr or scoped_ptr as soon as you create it. https://codereview.chromium.org/344433003/diff/210001/extensions/renderer/con... File extensions/renderer/content_watcher.h (right): https://codereview.chromium.org/344433003/diff/210001/extensions/renderer/con... extensions/renderer/content_watcher.h:34: ContentWatcher(const std::string& event_name); I don't think we want 3 different ContentWatchers in each renderer even if we have multiple events. The renderers should be told what to watch for, but they don't need to know why they're watching for it.
Implemented most comments from jyasskin@. A few comments back and one implementation deferred until I can rebase to a fresher view of the world. https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative/rules_registry_service.cc (right): https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... chrome/browser/extensions/api/declarative/rules_registry_service.cc:176: return content_rules_registry_map_[event_name]; On 2014/07/15 17:12:59, Jeffrey Yasskin wrote: > If you've already run map::find, you can just return *it, rather than looking up > the key again. Done. https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative/rules_registry_service.h (right): https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... chrome/browser/extensions/api/declarative/rules_registry_service.h:143: // Collection of content rule registries to make it easier to handle content On 2014/07/15 17:12:59, Jeffrey Yasskin wrote: > You've removed the information about what structure owns these pointers. That > was important, so please put it back. > > 'course, this change might not be necessary anymore since there's only 1 event > at the moment. Even when there are multiple events in the API, we might still > want to track them with a single registry, since they affect when the actions > fire, rather than what information is stored. Fixed comment to refer to weak pointers. When/if we add multiple events, we would need to rethink binding event names if we want to keep a single registry. Right now, one (and only one) event name is tied to each registry. This seemed like a simpler pattern of composition than refactoring registry code to track and properly dispatch on multiple event names. For this CL, our options are (1) revert this part of the change since there's only one event so far, (2) keep the change to avoid refactoring event name binding, or (3) refactor event name binding as a part of this CL. What's your preference? https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:32: const char kInvalidTypeOfParamter[] = "Attribute '%s' has an invalid type"; On 2014/07/15 17:12:59, Jeffrey Yasskin wrote: > sp: "Paramter" > > It's also more user-friendly to say what the type needs to be. This may mean you > need more strings. Done. https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:101: RequestContentScript() {} On 2014/07/15 17:12:59, Jeffrey Yasskin wrote: > Give this constructor parameters matching the values that need to be stored, > instead of side-loading them after constructing the object. Done. https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:107: friend scoped_refptr<ContentAction> RequestContentScript::Create( On 2014/07/15 17:12:59, Jeffrey Yasskin wrote: > You don't need to friend members of the same class. I hit a compiler error without this. I think the friendship is needed because Create is static. https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:129: // do not load user script in the future. On 2014/07/15 17:13:00, Jeffrey Yasskin wrote: > s/in the future/if Apply() runs again on the same page/ Done. https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:134: DISALLOW_COPY_AND_ASSIGN(RequestContentScript); On 2014/07/15 17:13:00, Jeffrey Yasskin wrote: > This goes last in the class: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... Done. https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:143: bool AppendJSStringsToCPPStrings(const base::ListValue& append_strings, On 2014/07/15 17:12:59, Jeffrey Yasskin wrote: > This should be static to be sure it doesn't collide with a helper in another .cc > file. Done. I'm curious, doesn't living in an anonymous namespace (which it does) also achieve this? https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:144: std::vector<std::string>& append_to) { On 2014/07/15 17:12:59, Jeffrey Yasskin wrote: > Chrome doesn't use non-const reference arguments: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Refere... Done. https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:170: if (attribute_name == "css" || attribute_name == "js") { On 2014/07/15 17:12:59, Jeffrey Yasskin wrote: > I would probably look up the attributes I want instead of iterating all > attributes, but this isn't terrible. Done. Switched to looking up individual members. This didn't account for instanceType anyway, and if we add more metadata in the future, we shouldn't have to update a "skip-property list" in this loop. https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:199: delete request_content_script; On 2014/07/15 17:12:59, Jeffrey Yasskin wrote: > Try never to have to write "delete foo" at the end of a function: it makes the > function more fragile. Instead, capture the pointer into a scoped_refptr or > scoped_ptr as soon as you create it. Done. https://codereview.chromium.org/344433003/diff/210001/extensions/renderer/con... File extensions/renderer/content_watcher.h (right): https://codereview.chromium.org/344433003/diff/210001/extensions/renderer/con... extensions/renderer/content_watcher.h:34: ContentWatcher(const std::string& event_name); On 2014/07/15 17:13:00, Jeffrey Yasskin wrote: > I don't think we want 3 different ContentWatchers in each renderer even if we > have multiple events. The renderers should be told what to watch for, but they > don't need to know why they're watching for it. Acknowledged. I'll fix this once I've merged to a newer master branch.
https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/344433003/diff/210001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:143: bool AppendJSStringsToCPPStrings(const base::ListValue& append_strings, On 2014/07/15 18:32:42, Mark Dittmer wrote: > On 2014/07/15 17:12:59, Jeffrey Yasskin wrote: > > This should be static to be sure it doesn't collide with a helper in another > .cc > > file. > > Done. I'm curious, doesn't living in an anonymous namespace (which it does) also > achieve this? Ah, yes it does. I'd lost the anonymous namespace around nearly everything in the file. https://codereview.chromium.org/344433003/diff/230001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/344433003/diff/230001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:105: const bool& all_frames, You can pass bools by value. https://codereview.chromium.org/344433003/diff/230001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:116: // friend scoped_refptr<ContentAction> RequestContentScript::Create( Remember to delete this before submitting. https://codereview.chromium.org/344433003/diff/230001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:147: virtual ~RequestContentScript() {} The destructor would go first in this private section (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...), but you can probably just remove it. https://codereview.chromium.org/344433003/diff/230001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:176: bool all_frames(false); We usually initialize simple values with = instead of ()s. https://codereview.chromium.org/344433003/diff/230001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:186: AppendJSStringsToCPPStrings(*list_value, &css_file_names); You should use the return value of this function, in order to catch invalid list element types. https://codereview.chromium.org/344433003/diff/230001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:188: *error = base::StringPrintf(kInvalidTypeOfParameter, This looks like it's going to give an error if someone has "js" but not "css", which doesn't seem correct. Please add some tests to content_action_unittest.cc to check this function.
Applied comments, added unit tests, and set bad_message=true for errors. Jeffrey, could you please look over the unit tests and confirm that the errors where I set bad_message=true fit the definition of "syntactically unexpected" input? https://codereview.chromium.org/344433003/diff/230001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/344433003/diff/230001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:105: const bool& all_frames, On 2014/07/15 20:44:59, Jeffrey Yasskin wrote: > You can pass bools by value. Done. https://codereview.chromium.org/344433003/diff/230001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:116: // friend scoped_refptr<ContentAction> RequestContentScript::Create( On 2014/07/15 20:44:59, Jeffrey Yasskin wrote: > Remember to delete this before submitting. Done. Thanks! https://codereview.chromium.org/344433003/diff/230001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:147: virtual ~RequestContentScript() {} On 2014/07/15 20:44:59, Jeffrey Yasskin wrote: > The destructor would go first in this private section > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...), > but you can probably just remove it. Moved it to the beginning of the section. Without it, I get "error: [chromium-style] Classes that are ref-counted should have explicit destructors that are declared protected or private." https://codereview.chromium.org/344433003/diff/230001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:176: bool all_frames(false); On 2014/07/15 20:44:59, Jeffrey Yasskin wrote: > We usually initialize simple values with = instead of ()s. Done. https://codereview.chromium.org/344433003/diff/230001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:186: AppendJSStringsToCPPStrings(*list_value, &css_file_names); On 2014/07/15 20:44:59, Jeffrey Yasskin wrote: > You should use the return value of this function, in order to catch invalid list > element types. Done. https://codereview.chromium.org/344433003/diff/230001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:188: *error = base::StringPrintf(kInvalidTypeOfParameter, On 2014/07/15 20:44:59, Jeffrey Yasskin wrote: > This looks like it's going to give an error if someone has "js" but not "css", > which doesn't seem correct. Please add some tests to content_action_unittest.cc > to check this function. Done.
apologies for the drive-by but I saw "bad_message" in a review comment. https://codereview.chromium.org/344433003/diff/230001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/344433003/diff/230001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:105: const bool& all_frames, On 2014/07/16 12:26:13, Mark Dittmer wrote: > On 2014/07/15 20:44:59, Jeffrey Yasskin wrote: > > You can pass bools by value. > > Done. there's no point having them as "const" when they're passed by value. https://codereview.chromium.org/344433003/diff/250001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/344433003/diff/250001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:177: *bad_message = true; FYI there's not much point setting both bad_message and an error, since bad_message kills the renderer (assuming it behaves the same way as with extension function errors - jeffrey, right?) so the error will never actually reach it. besides which, it looks like the idiom here is to use INPUT_FORMAT_VALIDATE. https://codereview.chromium.org/344433003/diff/250001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:188: return scoped_refptr<ContentAction>(); indentation is off here, you should run "git cl format" before uploading changes.
Fixed issues in drive-by review. Jeffrey, once you review the unit tests this CL should be good to go. https://codereview.chromium.org/344433003/diff/230001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/344433003/diff/230001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:105: const bool& all_frames, On 2014/07/16 16:51:14, kalman wrote: > On 2014/07/16 12:26:13, Mark Dittmer wrote: > > On 2014/07/15 20:44:59, Jeffrey Yasskin wrote: > > > You can pass bools by value. > > > > Done. > > there's no point having them as "const" when they're passed by value. Done. https://codereview.chromium.org/344433003/diff/250001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/344433003/diff/250001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:177: *bad_message = true; On 2014/07/16 16:51:14, kalman wrote: > FYI there's not much point setting both bad_message and an error, since > bad_message kills the renderer (assuming it behaves the same way as with > extension function errors - jeffrey, right?) so the error will never actually > reach it. > > besides which, it looks like the idiom here is to use INPUT_FORMAT_VALIDATE. Done. I use error instead of bad_message for "has css or js" because the checker doesn't support this kind of check. Others now use INPUT_FORMAT_VALIDATE(). https://codereview.chromium.org/344433003/diff/250001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:188: return scoped_refptr<ContentAction>(); On 2014/07/16 16:51:14, kalman wrote: > indentation is off here, you should run "git cl format" before uploading > changes. Done.
https://codereview.chromium.org/344433003/diff/270001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/344433003/diff/270001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:179: !dict->HasKey(keys::kCss) || Use more if()s here. Chains of || and && that rely on short-circuiting tend to be hard to understand. I believe: if (dict->HasKey(keys::kCss)) { INPUT_FORMAT_VALIDATE(dict->GetList(keys::kCss, &list_value)); INPUT_FORMAT_VALIDATE(AppendJSStringsToCPPStrings(*list_value, &css_file_names)); } will work fine in this case. It also makes it easier to set breakpoints in precise places. https://codereview.chromium.org/344433003/diff/270001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action_unittest.cc (right): https://codereview.chromium.org/344433003/diff/270001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:148: ASSERT_TRUE(result.get()); In your next patch, you should extend these to make sure that the contents of |result| are what you expect. It'll be enough to Apply() the actions and assert against the outcomes, but you'll need to be sure that the things Apply() changes are accessible enough to check what effect Apply() had.
Broke input verification into if applicable, then validate. Will add new checks to tests when Apply() is implemented. https://codereview.chromium.org/344433003/diff/270001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action.cc (right): https://codereview.chromium.org/344433003/diff/270001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action.cc:179: !dict->HasKey(keys::kCss) || On 2014/07/17 20:39:14, Jeffrey Yasskin wrote: > Use more if()s here. Chains of || and && that rely on short-circuiting tend to > be hard to understand. I believe: > > if (dict->HasKey(keys::kCss)) { > INPUT_FORMAT_VALIDATE(dict->GetList(keys::kCss, &list_value)); > INPUT_FORMAT_VALIDATE(AppendJSStringsToCPPStrings(*list_value, > &css_file_names)); > } > > will work fine in this case. It also makes it easier to set breakpoints in > precise places. Done. https://codereview.chromium.org/344433003/diff/270001/chrome/browser/extensio... File chrome/browser/extensions/api/declarative_content/content_action_unittest.cc (right): https://codereview.chromium.org/344433003/diff/270001/chrome/browser/extensio... chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:148: ASSERT_TRUE(result.get()); On 2014/07/17 20:39:14, Jeffrey Yasskin wrote: > In your next patch, you should extend these to make sure that the contents of > |result| are what you expect. It'll be enough to Apply() the actions and assert > against the outcomes, but you'll need to be sure that the things Apply() changes > are accessible enough to check what effect Apply() had. Will do this when Apply gets implemented.
Thanks! LGTM.
The CQ bit was checked by markdittmer@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/markdittmer@chromium.org/344433003/290001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
Message was sent while issue was closed.
Change committed as 284049 |
