Implement declarativeContent API.

Implement the basic declarativeContent API.

This CL includes the top_url and css conditions and the ShowPageAction
action.  CSS matching is implemented inefficiently using a
MutationObserver, but I plan to push that into WebKit before releasing
this to stable.

BUG=172011
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179051

[Jeffrey Yasskin, 2012-12-13 01:39:04]

This is too big, but I figured I'd send the whole thing out for overall feedback
while I cut pieces out and submit them independently. I'm definitely cutting out
the values_test_util and DeclarativeRule refactorings; are there any other
pieces that look separable to you folks?

[Matt Perry, 2012-12-13 20:27:48]

https://codereview.chromium.org/11547033/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/declarative/declarative_rule.h (right):

https://codereview.chromium.org/11547033/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative/declarative_rule.h:30:
template<typename ConditionT>
Did you consider defining Condition and Action interfaces and using inheritance
instead of templates?

https://codereview.chromium.org/11547033/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/declarative_content/content_rules_registry.cc
(right):

https://codereview.chromium.org/11547033/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative_content/content_rules_registry.cc:86:
// match.
Are you sure this method is guaranteed to be called before an CSS rules start to
match? For example, if CSS rules matched as the page was loaded, and if this
method is called when the page load completes, the rules could match first.

https://codereview.chromium.org/11547033/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative_content/content_rules_registry.cc:254:
// incognito after the rules match.
Where does this filtering happen? How does it handle incognito split-mode?

https://codereview.chromium.org/11547033/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative_content/content_rules_registry.cc:255:
if (process_profile->GetOriginalProfile() == profile_)
nit: IsSameProfile(profile_) does this too

https://codereview.chromium.org/11547033/diff/1/chrome/browser/extensions/api...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc
(right):

https://codereview.chromium.org/11547033/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:280:
found_matching_condition = true;
break;

https://codereview.chromium.org/11547033/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/declarative_content.json (right):

https://codereview.chromium.org/11547033/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/declarative_content.json:15: "top_url": {
topUrl (or maybe pageUrl)

[Jeffrey Yasskin, 2012-12-14 00:38:20]

https://codereview.chromium.org/11547033/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/declarative/declarative_rule.h (right):

https://codereview.chromium.org/11547033/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative/declarative_rule.h:30:
template<typename ConditionT>
On 2012/12/13 20:27:48, Matt Perry wrote:
> Did you consider defining Condition and Action interfaces and using
inheritance
> instead of templates?

Yes. It would be more verbose to implement, and both consumers call methods that
are specific to their implementation, so they'd have to downcast anyway. If I
used inheritance instead, we'd get some reduction in code size and the next
implementation might find it easier to know which methods to define. On the
other hand, if the next implementation is like these two, they're going to have
to modify the generic code anyway, and the required downcasts would make it
harder to figure out what types are supposed to live in here. Using a thin
wrapper template to make the types clear would increase the implementation
complexity again.

https://codereview.chromium.org/11547033/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/declarative_content/content_rules_registry.cc
(right):

https://codereview.chromium.org/11547033/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative_content/content_rules_registry.cc:86:
// match.
On 2012/12/13 20:27:48, Matt Perry wrote:
> Are you sure this method is guaranteed to be called before an CSS rules start
to
> match? For example, if CSS rules matched as the page was loaded, and if this
> method is called when the page load completes, the rules could match first.

In extension_messages.h, I wrote that ExtensionHostMsg_OnWatchedPageChange is
sent after ViewHostMsg_FrameNavigate, and I think I implemented it that way. Is
there a better way to do this? Should I maybe watch the page_id and only update
if I haven't gotten a later page_id in OnWatchedPageChange? Should I rely on
OnWatchedPageChange being sent for all navigations, and just leave the page
action visible until it arrives?

https://codereview.chromium.org/11547033/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative_content/content_rules_registry.cc:254:
// incognito after the rules match.
On 2012/12/13 20:27:48, Matt Perry wrote:
> Where does this filtering happen? How does it handle incognito split-mode?

Either it doesn't yet, or page actions just don't show where the extension can't
run, so it doesn't matter. I was thinking I'd do it in
ContentRulesRegistry::Apply(), but I hadn't considered split-mode extensions
that might register different rules in incognito vs not.

And RulesRegistryService exists separately on normal vs incognito profiles, so
my DCHECK in the constructor would fire anyway. *fixing.*

For spanning extensions, how will it work with two rules registries? Do I need
to do something to get the rules injected into both?

https://codereview.chromium.org/11547033/diff/1/chrome/browser/extensions/api...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc
(right):

https://codereview.chromium.org/11547033/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc:280:
found_matching_condition = true;
On 2012/12/13 20:27:48, Matt Perry wrote:
> break;

This is just moved from WebRequestRule::CheckConsistency. I'd rather fix bugs in
a separate CL.

https://codereview.chromium.org/11547033/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/declarative_content.json (right):

https://codereview.chromium.org/11547033/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/declarative_content.json:15: "top_url": {
On 2012/12/13 20:27:48, Matt Perry wrote:
> topUrl (or maybe pageUrl)

Done. (pageUrl)

[Matt Perry, 2012-12-14 19:49:32]

lgtm

https://codereview.chromium.org/11547033/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/declarative/declarative_rule.h (right):

https://codereview.chromium.org/11547033/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative/declarative_rule.h:30:
template<typename ConditionT>
On 2012/12/14 00:38:20, Jeffrey Yasskin wrote:
> On 2012/12/13 20:27:48, Matt Perry wrote:
> > Did you consider defining Condition and Action interfaces and using
> inheritance
> > instead of templates?
> 
> Yes. It would be more verbose to implement, and both consumers call methods
that
> are specific to their implementation, so they'd have to downcast anyway. If I
> used inheritance instead, we'd get some reduction in code size and the next
> implementation might find it easier to know which methods to define. On the
> other hand, if the next implementation is like these two, they're going to
have
> to modify the generic code anyway, and the required downcasts would make it
> harder to figure out what types are supposed to live in here. Using a thin
> wrapper template to make the types clear would increase the implementation
> complexity again.

Fair enough :)

https://codereview.chromium.org/11547033/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/declarative_content/content_rules_registry.cc
(right):

https://codereview.chromium.org/11547033/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative_content/content_rules_registry.cc:86:
// match.
On 2012/12/14 00:38:20, Jeffrey Yasskin wrote:
> On 2012/12/13 20:27:48, Matt Perry wrote:
> > Are you sure this method is guaranteed to be called before an CSS rules
start
> to
> > match? For example, if CSS rules matched as the page was loaded, and if this
> > method is called when the page load completes, the rules could match first.
> 
> In extension_messages.h, I wrote that ExtensionHostMsg_OnWatchedPageChange is
> sent after ViewHostMsg_FrameNavigate, and I think I implemented it that way.
Is
> there a better way to do this? Should I maybe watch the page_id and only
update
> if I haven't gotten a later page_id in OnWatchedPageChange? Should I rely on
> OnWatchedPageChange being sent for all navigations, and just leave the page
> action visible until it arrives?

I'm not sure. It looks like ExtensionHostMsg_OnWatchedPageChange is sent in 2
cases:
(1) in response to the OnWatchPage message
(2) in response to a DOM mutation

Maybe both are guaranteed to come after a FrameNavigate? I don't know. If it's
not a problem, no need to worry about it. But something to look out for.

https://codereview.chromium.org/11547033/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/declarative_content/content_rules_registry.cc:254:
// incognito after the rules match.
On 2012/12/14 00:38:20, Jeffrey Yasskin wrote:
> On 2012/12/13 20:27:48, Matt Perry wrote:
> > Where does this filtering happen? How does it handle incognito split-mode?
> 
> Either it doesn't yet, or page actions just don't show where the extension
can't
> run, so it doesn't matter. I was thinking I'd do it in
> ContentRulesRegistry::Apply(), but I hadn't considered split-mode extensions
> that might register different rules in incognito vs not.

We could decide not to support this case, and enforce only one set of rules.

> And RulesRegistryService exists separately on normal vs incognito profiles, so
> my DCHECK in the constructor would fire anyway. *fixing.*
> 
> For spanning extensions, how will it work with two rules registries? Do I need
> to do something to get the rules injected into both?

Yes, you probably want the incognito registry to share the normal registry's
rules for spanning mode. For the ExtensionProcessManager, I made an incognito
wrapper that decides the right thing to do depending on spanning vs split.

[battre, 2012-12-15 11:54:10]

I had a first look at chrome/browser/extensions/api/declarative*.

I have learned some cool new stuff! Thank you.

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_action.cc
(right):

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_action.cc:53:
GetPageAction(apply_info.profile, extension_id)->SetDeclarativeAppearance(
should we do this as reference counting? Imagine you have two conditions that
can overlap. (If page has a <h1> element, show the page action. If page has a
<img> element, show the page action.) Now consider a page that has both elements
and the <img> element is deleted.

If we do this, we should ensure that the ContentAction remains immutable and
stateless. This property is important if we want to move some registries into
the renderer processes and allow them to share memory.

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_action.h (right):

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_action.h:11: #include
"base/memory/linked_ptr.h"
nit: a couple of these are not needed

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_action.h:29: // Base
class for all ContentActions of the declarative Web Request API.
nit: Content API

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc
(right):

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:50:
scoped_ptr<base::DictionaryValue> manifest = DictionaryBuilder()
OMG, thanks for showing me this!!! This should be in base/.

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:122:
.Set("default_title", "Extension")));
nit: is this proper formatting?

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:132:
EXPECT_TRUE(page_action->GetIsVisible(tab_id));
Add test for Revert?

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_condition.cc
(right):

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_condition.cc:7:
#include "base/bind.h"
nit: can you check whether all of these are needed

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_condition.h
(right):

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_condition.h:20:
#include "googleurl/src/gurl.h"
nit: some of the includes are not necessary.

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_condition.h:28:
base::hash_set<std::string> css_rules;
are these css selectors? in that case, I think we should call them
css_selectors.

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_condition.h:36: // -
URL Matcher attributes are attributes that test the URL of a page.
does this apply to main_frames or also to other frames?

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_rules_registry.cc
(right):

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_rules_registry.cc:82:
// match.
Is the OnWatchedPageChange a TODO? where do you handle this?

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_rules_registry.cc:240:
watched_css_selectors_.assign(css_selectors.begin(), css_selectors.end());
swap?

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_rules_registry.h
(right):

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_rules_registry.h:75:
// example 'scheme': 'http') are fulfilled.
This is not correct any more (the scheme is processed by the URLMatcher, this
needs to be corrected in webrequest_rules_registry.h as well). I would suggest
to delete lines 56-64 and 68-75.

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_rules_registry.h:77:
public content::NotificationObserver {
nit: indentation

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_rules_registry.h:83:
void InstructRenderProcess(content::RenderProcessHost* process);
private?

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_rules_registry.h:85:
// Applies all content rules given an update from the renderer.
can you explain what "update" means here?

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_rules_registry.h:99:
// case?  For PageActions, doesn't matter.
This looks like a temporary comment that should be moved somewhere else.

[battre, 2012-12-15 11:57:25]

arg. I just noticed that I spent 2 hours reviewing the wrong CL. :-(

[Jeffrey Yasskin, 2012-12-15 19:21:38]

On Sat, Dec 15, 2012 at 3:57 AM,  <battre@chromium.org> wrote:
> arg. I just noticed that I spent 2 hours reviewing the wrong CL. :-(

No, this CL's fine to review. I'll have to submit
https://codereview.chromium.org/11572061/ first, but they're mostly
disjoint, and I need reviews for both. I've closed the CL you
shouldn't review.

> https://codereview.chromium.org/11547033/

[Jeffrey Yasskin, 2013-01-17 09:22:56]

I believe this is now ready for a look. If you can see any way to split it up
into smaller chunks, I'm happy to do that too.

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_action.cc
(right):

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_action.cc:53:
GetPageAction(apply_info.profile, extension_id)->SetDeclarativeAppearance(
On 2012/12/15 11:54:10, battre wrote:
> should we do this as reference counting? Imagine you have two conditions that
> can overlap. (If page has a <h1> element, show the page action. If page has a
> <img> element, show the page action.) Now consider a page that has both
elements
> and the <img> element is deleted.

It's easy right now to refcount Show actions, but if we add Hide, then it has to
take priorities into account. I've added the refcounting for now, and we can
deal with priorities later.

There's a nasty race condition between the PageActionController resetting page
actions and the ContentRulesRegistry Applying the rules to the new page. I'd
like to figure out how to handle that well in a separate change.

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_action.h (right):

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_action.h:11: #include
"base/memory/linked_ptr.h"
On 2012/12/15 11:54:10, battre wrote:
> nit: a couple of these are not needed

Done.

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_action.h:29: // Base
class for all ContentActions of the declarative Web Request API.
On 2012/12/15 11:54:10, battre wrote:
> nit: Content API

Done.

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc
(right):

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:50:
scoped_ptr<base::DictionaryValue> manifest = DictionaryBuilder()
On 2012/12/15 11:54:10, battre wrote:
> OMG, thanks for showing me this!!! This should be in base/.

It's pretty awesome. Many tests are better using base::test::ParseJson
(https://code.google.com/searchframe#OAMlx_jo-ck/src/base/test/values_test_uti...),
but this one wants a DictionaryValue, which requires a cast from ParseJson.

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:122:
.Set("default_title", "Extension")));
On 2012/12/15 11:54:10, battre wrote:
> nit: is this proper formatting?

I think so, because it expresses the nesting with a minimum of lines. I'd be
happy to change to something else if you prefer.

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:132:
EXPECT_TRUE(page_action->GetIsVisible(tab_id));
On 2012/12/15 11:54:10, battre wrote:
> Add test for Revert?

Done.

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_condition.cc
(right):

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_condition.cc:7:
#include "base/bind.h"
On 2012/12/15 11:54:10, battre wrote:
> nit: can you check whether all of these are needed

Done.

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_condition.h
(right):

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_condition.h:20:
#include "googleurl/src/gurl.h"
On 2012/12/15 11:54:10, battre wrote:
> nit: some of the includes are not necessary.

Done.

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_condition.h:28:
base::hash_set<std::string> css_rules;
On 2012/12/15 11:54:10, battre wrote:
> are these css selectors? in that case, I think we should call them
> css_selectors.

Good point. Done.

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_condition.h:36: // -
URL Matcher attributes are attributes that test the URL of a page.
On 2012/12/15 11:54:10, battre wrote:
> does this apply to main_frames or also to other frames?

For now, it's just top-level pages. I think the attribute name is "top_url" to
recognize that.

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_rules_registry.cc
(right):

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_rules_registry.cc:82:
// match.
On 2012/12/15 11:54:10, battre wrote:
> Is the OnWatchedPageChange a TODO? where do you handle this?

https://codereview.chromium.org/11547033/diff/21001/chrome/renderer/extension...
and
https://codereview.chromium.org/11547033/diff/21001/chrome/browser/extensions...

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_rules_registry.cc:240:
watched_css_selectors_.assign(css_selectors.begin(), css_selectors.end());
On 2012/12/15 11:54:10, battre wrote:
> swap?

They're different types. I assume some expense here is ok because it only
happens when the set of rules changes.

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_rules_registry.h
(right):

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_rules_registry.h:75:
// example 'scheme': 'http') are fulfilled.
On 2012/12/15 11:54:10, battre wrote:
> This is not correct any more (the scheme is processed by the URLMatcher, this
> needs to be corrected in webrequest_rules_registry.h as well). I would suggest
> to delete lines 56-64 and 68-75. 

Done.

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_rules_registry.h:77:
public content::NotificationObserver {
On 2012/12/15 11:54:10, battre wrote:
> nit: indentation

Done.

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_rules_registry.h:83:
void InstructRenderProcess(content::RenderProcessHost* process);
On 2012/12/15 11:54:10, battre wrote:
> private?

It was used by something else. I guess not anymore.

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_rules_registry.h:85:
// Applies all content rules given an update from the renderer.
On 2012/12/15 11:54:10, battre wrote:
> can you explain what "update" means here?

Done.

https://codereview.chromium.org/11547033/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_rules_registry.h:99:
// case?  For PageActions, doesn't matter.
On 2012/12/15 11:54:10, battre wrote:
> This looks like a temporary comment that should be moved somewhere else.

Oops.

[battre, 2013-01-18 10:37:28]

Reviewed chrome/browser/extensions/api/ so far (will continue after an on-site
interview). Looking good so far. I wonder, though, whether we have a race
condition in the test. I think this is pretty difficult to argue about.

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_action.cc
(right):

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_action.cc:97: (*
FactoryMethod)(const base::DictionaryValue* /* dict */ ,
nit: additional space after */

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_action.cc:100:
std::map<std::string, FactoryMethod> factory_methods;
comment the std::string in the key represents?

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc
(right):

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:118:
&error, &bad_message);
nit: what is the alignment of this line?

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_condition.h
(right):

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_condition.h:25: GURL
page_url;
add comments on the meaning of page_url and css_selectors?

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_content/content_condition_unittest.cc
(right):

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_condition_unittest.cc:42:
// Test wrong datatype in host_suffix.
This comment is incorrect.

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_rules_registry.cc
(right):

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_rules_registry.cc:128:
if (!error.empty()) {
|| !content_rule

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_rules_registry.h
(right):

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_rules_registry.h:103:
GetMatches(const RendererContentMatchData& renderer_data);
make this function const?

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_rules_registry.h:128:
std::map<int, std::set<ContentRule*> > active_rules_;
how about
typdef int TabId;
std::map<TabId, ...>?

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc
(right):

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc:44:
NavigateInRenderer(tab, GURL("http://test1/"));
Are you sure that this is not racy?

[battre, 2013-01-18 16:13:40]

If would be good if somebody else (Matt?) could review the content_watcher parts
as well.

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/extension_action.h (right):

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/extension_action.h:298: // |declarative_value|),
map[kDefaultTabId], or ValueTraits<T>::CreateEmpty()
I think the "declarative_map[tab_id] (uses |declarative_value|)" is difficult to
grasp.  Can you write more about the concept here? I don't understand why these
maps exist.

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/extension_action.h:344: // Maps tab_id to the number
of times
nit: this sentence seems incomplete and lacks a ".".

https://codereview.chromium.org/11547033/diff/24001/chrome/renderer/extension...
File chrome/renderer/extensions/content_watcher.cc (right):

https://codereview.chromium.org/11547033/diff/24001/chrome/renderer/extension...
chrome/renderer/extensions/content_watcher.cc:122: old_matches = new_matches;
swap?

https://codereview.chromium.org/11547033/diff/24001/chrome/renderer/extension...
chrome/renderer/extensions/content_watcher.cc:208: #if 0
???

[Jeffrey Yasskin, 2013-01-22 08:01:22]

Thanks for slogging through this CL. Matt, could you also review when you get a
chance?

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_action.cc
(right):

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_action.cc:97: (*
FactoryMethod)(const base::DictionaryValue* /* dict */ ,
On 2013/01/18 10:37:29, battre wrote:
> nit: additional space after */

Done.

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_action.cc:100:
std::map<std::string, FactoryMethod> factory_methods;
On 2013/01/18 10:37:29, battre wrote:
> comment the std::string in the key represents?

Done. Remember to do the same in webrequest_action.cc.

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc
(right):

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:118:
&error, &bad_message);
On 2013/01/18 10:37:29, battre wrote:
> nit: what is the alignment of this line?

No idea. Fixed now.

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_condition.h
(right):

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_condition.h:25: GURL
page_url;
On 2013/01/18 10:37:29, battre wrote:
> add comments on the meaning of page_url and css_selectors?

Done. Remember to do the same for webrequest_condition.h.

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_content/content_condition_unittest.cc
(right):

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_condition_unittest.cc:42:
// Test wrong datatype in host_suffix.
On 2013/01/18 10:37:29, battre wrote:
> This comment is incorrect.

Fixed.

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_rules_registry.cc
(right):

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_rules_registry.cc:128:
if (!error.empty()) {
On 2013/01/18 10:37:29, battre wrote:
> || !content_rule

It's supposed to be impossible for DeclarativeRule::Create() to return NULL
without setting error. I've added a DCHECK to catch that case in debug mode,
although it'll still cause a NULL-pointer dereference eventually in releases.

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_rules_registry.h
(right):

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_rules_registry.h:103:
GetMatches(const RendererContentMatchData& renderer_data);
On 2013/01/18 10:37:29, battre wrote:
> make this function const?

I can't, because URLMatcher::MatchURL isn't const. I'd rather not make this
change even bigger by constifying that function here.

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_rules_registry.h:128:
std::map<int, std::set<ContentRule*> > active_rules_;
On 2013/01/18 10:37:29, battre wrote:
> how about
> typdef int TabId;
> std::map<TabId, ...>?

I'd rather not do that in just one place, but I would help with a later
extension-wide change to do it. I'd be even happier with a wrapper type that
would prevent us from accidentally mixing ints and extension::TabIds.

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc
(right):

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc:44:
NavigateInRenderer(tab, GURL("http://test1/"));
On 2013/01/18 10:37:29, battre wrote:
> Are you sure that this is not racy?

I'm never certain. What race are you thinking of? The
ExtensionTestMessageListener |ready| is supposed to handle waiting for the rules
to be registered. I'm assuming that NavigateInRenderer waits until the renderer
is done navigating and sending messages, but I haven't investigated it in
detail.

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/extension_action.h (right):

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/extension_action.h:298: // |declarative_value|),
map[kDefaultTabId], or ValueTraits<T>::CreateEmpty()
On 2013/01/18 16:13:40, battre wrote:
> I think the "declarative_map[tab_id] (uses |declarative_value|)" is difficult
to
> grasp.  Can you write more about the concept here? I don't understand why
these
> maps exist.

This function was too general anyway; I've rewritten it to just cover
appearance_. If more of these attributes acquire declarative control, I can
abstract out the function then.

Hopefully the new comment, and the comment before declarative_show_count_
explain why it exists?

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/extension_action.h:344: // Maps tab_id to the number
of times
On 2013/01/18 16:13:40, battre wrote:
> nit: this sentence seems incomplete and lacks a ".".

That's not a nit. ;) Thanks; fixed.

https://codereview.chromium.org/11547033/diff/24001/chrome/renderer/extension...
File chrome/renderer/extensions/content_watcher.cc (right):

https://codereview.chromium.org/11547033/diff/24001/chrome/renderer/extension...
chrome/renderer/extensions/content_watcher.cc:122: old_matches = new_matches;
On 2013/01/18 16:13:40, battre wrote:
> swap?

Done.

https://codereview.chromium.org/11547033/diff/24001/chrome/renderer/extension...
chrome/renderer/extensions/content_watcher.cc:208: #if 0
On 2013/01/18 16:13:40, battre wrote:
> ???

Oops; old code replaced by content_watcher.js. Removed.

[battre, 2013-01-22 12:54:52]

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_action.cc
(right):

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_action.cc:100:
std::map<std::string, FactoryMethod> factory_methods;
On 2013/01/22 08:01:22, Jeffrey Yasskin wrote:
> On 2013/01/18 10:37:29, battre wrote:
> > comment the std::string in the key represents?
> 
> Done. Remember to do the same in webrequest_action.cc.

Done.

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_condition.h
(right):

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_condition.h:25: GURL
page_url;
On 2013/01/22 08:01:22, Jeffrey Yasskin wrote:
> On 2013/01/18 10:37:29, battre wrote:
> > add comments on the meaning of page_url and css_selectors?
> 
> Done. Remember to do the same for webrequest_condition.h.

Done.

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_rules_registry.h
(right):

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_rules_registry.h:103:
GetMatches(const RendererContentMatchData& renderer_data);
On 2013/01/22 08:01:22, Jeffrey Yasskin wrote:
> On 2013/01/18 10:37:29, battre wrote:
> > make this function const?
> 
> I can't, because URLMatcher::MatchURL isn't const. I'd rather not make this
> change even bigger by constifying that function here.

I've sent you a CL for this.

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/extension_action.h (right):

https://codereview.chromium.org/11547033/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/extension_action.h:298: // |declarative_value|),
map[kDefaultTabId], or ValueTraits<T>::CreateEmpty()
On 2013/01/22 08:01:22, Jeffrey Yasskin wrote:
> On 2013/01/18 16:13:40, battre wrote:
> > I think the "declarative_map[tab_id] (uses |declarative_value|)" is
difficult
> to
> > grasp.  Can you write more about the concept here? I don't understand why
> these
> > maps exist.
> 
> This function was too general anyway; I've rewritten it to just cover
> appearance_. If more of these attributes acquire declarative control, I can
> abstract out the function then.
> 
> Hopefully the new comment, and the comment before declarative_show_count_
> explain why it exists?

Beautiful!

[vabr (Chromium), 2013-01-23 12:32:56]

Hi Jeffrey,

I read this CL through, and did not find anything to comment on, LGTM.

Having said that, checking the linter output, I picked a couple of nits which
you may consider addressing.

Cheers,
Vaclav

https://codereview.chromium.org/11547033/diff/43001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc
(right):

https://codereview.chromium.org/11547033/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:49:
scoped_refptr<Extension> MakeExtension(DictionaryBuilder& manifest_extra) {
Lint flagged this non-const reference. Can you use a pointer?

https://codereview.chromium.org/11547033/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_condition.h
(right):

https://codereview.chromium.org/11547033/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_condition.h:9:
#include <string>
Lint: add #include for set<>.

(It also asked for an #include for hash_set, but you have base/hash_tables.h
here, which should be the one.)

https://codereview.chromium.org/11547033/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_rules_registry.h
(right):

https://codereview.chromium.org/11547033/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_rules_registry.h:9:
#include <set>
Lint: add #include for string.

https://codereview.chromium.org/11547033/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/tab_helper.h (right):

https://codereview.chromium.org/11547033/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/tab_helper.h:9: #include <string>
Lint: add #include for vector<>.

https://codereview.chromium.org/11547033/diff/43001/chrome/common/extensions/...
File chrome/common/extensions/extension_messages.h (right):

https://codereview.chromium.org/11547033/diff/43001/chrome/common/extensions/...
chrome/common/extensions/extension_messages.h:7: 
Lint: add #include for vector<> and string.

https://codereview.chromium.org/11547033/diff/43001/chrome/renderer/extension...
File chrome/renderer/extensions/module_system.h (right):

https://codereview.chromium.org/11547033/diff/43001/chrome/renderer/extension...
chrome/renderer/extensions/module_system.h:7: 
Lint: add #include for vector<>.

[Jeffrey Yasskin, 2013-01-23 22:13:17]

https://codereview.chromium.org/11547033/diff/43001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc
(right):

https://codereview.chromium.org/11547033/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:49:
scoped_refptr<Extension> MakeExtension(DictionaryBuilder& manifest_extra) {
On 2013/01/23 12:32:56, vabr (Chromium) wrote:
> Lint flagged this non-const reference. Can you use a pointer?

I could, or I could move the .Build() call outside the function and have it take
a const DictionaryValue& instead, but it seems like needless verbosity since
DictionaryBuilders are always single-use objects, so there's very little danger
of people storing one in a variable and then being confused that this function
mutates it.

https://codereview.chromium.org/11547033/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_condition.h
(right):

https://codereview.chromium.org/11547033/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_condition.h:9:
#include <string>
On 2013/01/23 12:32:56, vabr (Chromium) wrote:
> Lint: add #include for set<>.
> 
> (It also asked for an #include for hash_set, but you have base/hash_tables.h
> here, which should be the one.)

Done. (How did you run lint? I haven't seen a way to do it besides `git cl
upload`.)

https://codereview.chromium.org/11547033/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_rules_registry.h
(right):

https://codereview.chromium.org/11547033/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_rules_registry.h:9:
#include <set>
On 2013/01/23 12:32:56, vabr (Chromium) wrote:
> Lint: add #include for string.

Done.

https://codereview.chromium.org/11547033/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/tab_helper.h (right):

https://codereview.chromium.org/11547033/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/tab_helper.h:9: #include <string>
On 2013/01/23 12:32:56, vabr (Chromium) wrote:
> Lint: add #include for vector<>.

Done.

https://codereview.chromium.org/11547033/diff/43001/chrome/common/extensions/...
File chrome/common/extensions/extension_messages.h (right):

https://codereview.chromium.org/11547033/diff/43001/chrome/common/extensions/...
chrome/common/extensions/extension_messages.h:7: 
On 2013/01/23 12:32:56, vabr (Chromium) wrote:
> Lint: add #include for vector<> and string.

Done.

https://codereview.chromium.org/11547033/diff/43001/chrome/renderer/extension...
File chrome/renderer/extensions/module_system.h (right):

https://codereview.chromium.org/11547033/diff/43001/chrome/renderer/extension...
chrome/renderer/extensions/module_system.h:7: 
On 2013/01/23 12:32:56, vabr (Chromium) wrote:
> Lint: add #include for vector<>.

Done.

[Matt Perry, 2013-01-24 01:55:16]

Still LGTM.

Minor comments on content_watcher.cc.

https://codereview.chromium.org/11547033/diff/38045/chrome/browser/extensions...
File chrome/browser/extensions/extension_action.h (right):

https://codereview.chromium.org/11547033/diff/38045/chrome/browser/extensions...
chrome/browser/extensions/extension_action.h:305: // leak information about
hosts the extension doesn't have permission to
This means we can't expose whether a page action icon is visible via an API.
e.g. https://code.google.com/p/chromium/issues/detail?id=166904

https://codereview.chromium.org/11547033/diff/38045/chrome/renderer/extension...
File chrome/renderer/extensions/content_watcher.cc (right):

https://codereview.chromium.org/11547033/diff/38045/chrome/renderer/extension...
chrome/renderer/extensions/content_watcher.cc:122: using std::swap;
err... why :)

https://codereview.chromium.org/11547033/diff/38045/chrome/renderer/extension...
chrome/renderer/extensions/content_watcher.cc:140: js_css_selectors->Set(i,
v8::String::New(css_selectors_[i].c_str(),
nit: use data() in conjunction with size() since c_str potentially reallocs the
string to add a null.

https://codereview.chromium.org/11547033/diff/38045/chrome/renderer/extension...
chrome/renderer/extensions/content_watcher.cc:180: // If the changed frame can't
be accessed by the top frame, than
then*

[Jeffrey Yasskin, 2013-01-24 03:10:22]

https://codereview.chromium.org/11547033/diff/38045/chrome/browser/extensions...
File chrome/browser/extensions/extension_action.h (right):

https://codereview.chromium.org/11547033/diff/38045/chrome/browser/extensions...
chrome/browser/extensions/extension_action.h:305: // leak information about
hosts the extension doesn't have permission to
On 2013/01/24 01:55:17, Matt Perry wrote:
> This means we can't expose whether a page action icon is visible via an API.
> e.g. https://code.google.com/p/chromium/issues/detail?id=166904

It means that we have to expose the state the extension set outside of the
declarative API, even if the extension is actually visible because of a
declarative setting.

https://codereview.chromium.org/11547033/diff/38045/chrome/renderer/extension...
File chrome/renderer/extensions/content_watcher.cc (right):

https://codereview.chromium.org/11547033/diff/38045/chrome/renderer/extension...
chrome/renderer/extensions/content_watcher.cc:122: using std::swap;
On 2013/01/24 01:55:17, Matt Perry wrote:
> err... why :)

It has the same effect in this case, because old_matches is in namespace std,
but it's the right habit to be in when calling swap(), since calling std::swap()
directly does the wrong thing for types outside the STL, and unqualified swap()
doesn't work for primitive types.

https://codereview.chromium.org/11547033/diff/38045/chrome/renderer/extension...
chrome/renderer/extensions/content_watcher.cc:140: js_css_selectors->Set(i,
v8::String::New(css_selectors_[i].c_str(),
On 2013/01/24 01:55:17, Matt Perry wrote:
> nit: use data() in conjunction with size() since c_str potentially reallocs
the
> string to add a null.

Done.

https://codereview.chromium.org/11547033/diff/38045/chrome/renderer/extension...
chrome/renderer/extensions/content_watcher.cc:180: // If the changed frame can't
be accessed by the top frame, than
On 2013/01/24 01:55:17, Matt Perry wrote:
> then*

Oops.

[commit-bot: I haz the power, 2013-01-24 03:11:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/11547033/48049

[commit-bot: I haz the power, 2013-01-24 03:11:58]

Presubmit check for 11547033-48049 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
Your #include order seems to be broken. Send mail to
marja@chromium.org if this is not the case.
      chrome/renderer/extensions/module_system.h:17

If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/common/extensions/extension_messages.h

Presubmit checks took 4.1s to calculate.

[Jeffrey Yasskin, 2013-01-24 03:14:32]

+tsepez for extension_messages.h
+thakis for *.gypi, but I'll TBR if tsepez lgtms first.

[vabr (Chromium), 2013-01-24 08:21:00]

https://codereview.chromium.org/11547033/diff/43001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc
(right):

https://codereview.chromium.org/11547033/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:49:
scoped_refptr<Extension> MakeExtension(DictionaryBuilder& manifest_extra) {
On 2013/01/23 22:13:17, Jeffrey Yasskin wrote:
> On 2013/01/23 12:32:56, vabr (Chromium) wrote:
> > Lint flagged this non-const reference. Can you use a pointer?
> 
> I could, or I could move the .Build() call outside the function and have it
take
> a const DictionaryValue& instead, but it seems like needless verbosity since
> DictionaryBuilders are always single-use objects, so there's very little
danger
> of people storing one in a variable and then being confused that this function
> mutates it.

Indeed, people will learn quite fast that manifest_extra has been modified by
the Build() call, because any attempt to call it twice will return NULL.

On the other hand, the style guide is very clear on banning non-const reference
arguments, so I would prefer if manifest_extra were a pointer -- changes in the
code for that are minimal.

It's not critical, because this is a unit test, but if you had a chance of
changing this, it would be good to do.

https://codereview.chromium.org/11547033/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_condition.h
(right):

https://codereview.chromium.org/11547033/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_condition.h:9:
#include <string>
On 2013/01/23 22:13:17, Jeffrey Yasskin wrote:
> On 2013/01/23 12:32:56, vabr (Chromium) wrote:
> > Lint: add #include for set<>.
> > 
> > (It also asked for an #include for hash_set, but you have base/hash_tables.h
> > here, which should be the one.)
> 
> Done. (How did you run lint? I haven't seen a way to do it besides `git cl
> upload`.)

Rietveld does that for me -- the links in the patch set table, column Lint,
although saying "? errors" usually lead to a maningful output from lint.

[Nico, 2013-01-24 18:08:43]

1. Since this is a bigger feature, it should have a tracking bug that describes
the motivation behind the feature etc -- see cpu@'s recent mail on chromium-dev.
The CL description should link to that bug.

2. Is exposing mutations to extensions ok'd by webkit / v8 folks? (Say, jamesr,
haraken, or abarth)

(3. This CL could've been broken up into way smaller chunks. Please try doing
this next time.)

[Tom Sepez, 2013-01-24 18:16:24]

Messages LGTM.

[Jeffrey Yasskin, 2013-01-25 00:08:29]

On 2013/01/24 18:08:43, Nico wrote:
> 1. Since this is a bigger feature, it should have a tracking bug that
describes
> the motivation behind the feature etc -- see cpu@'s recent mail on
chromium-dev.
> The CL description should link to that bug.

Done: http://crbug.com/172011

> 2. Is exposing mutations to extensions ok'd by webkit / v8 folks? (Say,
jamesr,
> haraken, or abarth)

Tony and Ojan were ok with trying it and pointed out a possible technique. 

> (3. This CL could've been broken up into way smaller chunks. Please try doing
> this next time.)

All of the smaller chunks I could have broken it into didn't seem to make sense
without the others. (Except for the 20-line module_system change, I think.) In
theory the extension_action change could have come out, but it was actually
modified in response to comments elsewhere in the change, so it might have had
to change twice if I'd pulled it out. I do try to split changes up into small
pieces when there are independent chunks.

https://codereview.chromium.org/11547033/diff/43001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc
(right):

https://codereview.chromium.org/11547033/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_action_unittest.cc:49:
scoped_refptr<Extension> MakeExtension(DictionaryBuilder& manifest_extra) {
On 2013/01/24 08:21:01, vabr (Chromium) wrote:
> On 2013/01/23 22:13:17, Jeffrey Yasskin wrote:
> > On 2013/01/23 12:32:56, vabr (Chromium) wrote:
> > > Lint flagged this non-const reference. Can you use a pointer?
> > 
> > I could, or I could move the .Build() call outside the function and have it
> take
> > a const DictionaryValue& instead, but it seems like needless verbosity since
> > DictionaryBuilders are always single-use objects, so there's very little
> danger
> > of people storing one in a variable and then being confused that this
function
> > mutates it.
> 
> Indeed, people will learn quite fast that manifest_extra has been modified by
> the Build() call, because any attempt to call it twice will return NULL.
> 
> On the other hand, the style guide is very clear on banning non-const
reference
> arguments, so I would prefer if manifest_extra were a pointer -- changes in
the
> code for that are minimal.
> 
> It's not critical, because this is a unit test, but if you had a chance of
> changing this, it would be good to do.

I changed it to take const DictionaryValue&.

https://codereview.chromium.org/11547033/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_content/content_condition.h
(right):

https://codereview.chromium.org/11547033/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/content_condition.h:9:
#include <string>
On 2013/01/24 08:21:01, vabr (Chromium) wrote:
> On 2013/01/23 22:13:17, Jeffrey Yasskin wrote:
> > On 2013/01/23 12:32:56, vabr (Chromium) wrote:
> > > Lint: add #include for set<>.
> > > 
> > > (It also asked for an #include for hash_set, but you have
base/hash_tables.h
> > > here, which should be the one.)
> > 
> > Done. (How did you run lint? I haven't seen a way to do it besides `git cl
> > upload`.)
> 
> Rietveld does that for me -- the links in the patch set table, column Lint,
> although saying "? errors" usually lead to a maningful output from lint.

Neat. ... unfortunately, that UI is so broken that I'm unlikely to actually use
it. :-/

[Nico, 2013-01-25 00:16:06]

> Done: http://crbug.com/172011

Thanks!


gyp changes lgtm. I'm assuming that the extension OWNERS have checked that this
extension is marked as experimental as mentioned on
http://www.chromium.org/developers/design-documents/extensions/proposed-chang...

[commit-bot: I haz the power, 2013-01-25 16:53:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/11547033/38097

[Jeffrey Yasskin, 2013-01-25 16:55:09]

On 2013/01/25 00:16:06, Nico wrote:
> > Done: http://crbug.com/172011
> 
> Thanks!
> 
> 
> gyp changes lgtm.

Thanks!

> I'm assuming that the extension OWNERS have checked that this
> extension is marked as experimental as mentioned on
>
http://www.chromium.org/developers/design-documents/extensions/proposed-chang...

It's bound to trunk
(https://codereview.chromium.org/11547033/diff/38097/chrome/common/extensions/...),
which is the new way we're doing that. I'll poke people to update that page.

[commit-bot: I haz the power, 2013-01-25 22:40:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/11547033/59001

[commit-bot: I haz the power, 2013-01-26 01:57:22]

Failed to apply patch for chrome/browser/extensions/tab_helper.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/tab_helper.cc
  Hunk #1 succeeded at 5 with fuzz 2.
  Hunk #2 FAILED at 85.
  Hunk #3 succeeded at 191 (offset 2 lines).
  Hunk #4 succeeded at 239 (offset 2 lines).
  Hunk #5 succeeded at 429 (offset 2 lines).
  1 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/extensions/tab_helper.cc.rej

Patch:       chrome/browser/extensions/tab_helper.cc
Index: chrome/browser/extensions/tab_helper.cc
diff --git a/chrome/browser/extensions/tab_helper.cc
b/chrome/browser/extensions/tab_helper.cc
index
88f85508404e5bcb67694bfd86b741c413a27e70..5f00eab2933542d17c90bad16591da9dfe5597d0
100644
--- a/chrome/browser/extensions/tab_helper.cc
+++ b/chrome/browser/extensions/tab_helper.cc
@@ -5,11 +5,14 @@
 #include "chrome/browser/extensions/tab_helper.h"
 
 #include "chrome/browser/extensions/activity_log.h"
+#include "chrome/browser/extensions/api/declarative/rules_registry_service.h"
+#include
"chrome/browser/extensions/api/declarative_content/content_rules_registry.h"
 #include "chrome/browser/extensions/app_notify_channel_ui.h"
 #include "chrome/browser/extensions/crx_installer.h"
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/extension_action_manager.h"
 #include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
 #include "chrome/browser/extensions/page_action_controller.h"
 #include "chrome/browser/extensions/script_badge_controller.h"
@@ -82,7 +85,11 @@ TabHelper::TabHelper(content::WebContents* web_contents)
               this)),
       pending_web_app_action_(NONE),
       script_executor_(new ScriptExecutor(web_contents,
-                                          &script_execution_observers_)) {
+                                          &script_execution_observers_)),
+      rules_registry_service_(
+          ExtensionSystem::Get(
+              Profile::FromBrowserContext(web_contents->GetBrowserContext()))->
+          rules_registry_service()) {
   // The ActiveTabPermissionManager requires a session ID; ensure this
   // WebContents has one.
   SessionTabHelper::CreateForWebContents(web_contents);
@@ -186,6 +193,13 @@ void TabHelper::RenderViewCreated(RenderViewHost*
render_view_host) {
 void TabHelper::DidNavigateMainFrame(
     const content::LoadCommittedDetails& details,
     const content::FrameNavigateParams& params) {
+#if defined(ENABLE_EXTENSIONS)
+  if (rules_registry_service_) {
+    rules_registry_service_->content_rules_registry()->DidNavigateMainFrame(
+        web_contents(), details, params);
+  }
+#endif  // defined(ENABLE_EXTENSIONS)
+
   if (details.is_in_page)
     return;
 
@@ -227,6 +241,8 @@ bool TabHelper::OnMessageReceived(const IPC::Message&
message) {
     IPC_MESSAGE_HANDLER(ExtensionHostMsg_Request, OnRequest)
     IPC_MESSAGE_HANDLER(ExtensionHostMsg_ContentScriptsExecuting,
                         OnContentScriptsExecuting)
+    IPC_MESSAGE_HANDLER(ExtensionHostMsg_OnWatchedPageChange,
+                        OnWatchedPageChange)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
@@ -415,6 +431,16 @@ void TabHelper::OnContentScriptsExecuting(
                                       on_url));
 }
 
+void TabHelper::OnWatchedPageChange(
+    const std::vector<std::string>& css_selectors) {
+#if defined(ENABLE_EXTENSIONS)
+  if (rules_registry_service_) {
+    rules_registry_service_->content_rules_registry()->Apply(
+        web_contents(), css_selectors);
+  }
+#endif  // defined(ENABLE_EXTENSIONS)
+}
+
 const Extension* TabHelper::GetExtension(const std::string& extension_app_id) {
   if (extension_app_id.empty())
     return NULL;

[commit-bot: I haz the power, 2013-01-26 02:39:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/11547033/66002

[commit-bot: I haz the power, 2013-01-26 06:33:15]

Change committed as 179051