Encapsulate CSS selector declarative content condition tracking

Encapsulate CSS selector declarative content condition tracking

This is the first step in a refactoring of the declarativeContent API
implementation. The goal is to facilitate profile state matching by
providing abstractions for conditions and condition state tracking
and evaluation.

This CL consists of steps 1 and 2 described in the associated bug:
by-hand instantiation of DeclarativeRule as DeclarativeContentRule to
decouple changes from the web request declarative API implementation,
and encapsulation of CSS selector condition tracking logic. No
functional change is intended.

BUG=492946
Committed: https://crrev.com/fe76220cf0a1cdcfe74ca7daeae9f1da2b91cc6b
Cr-Commit-Position: refs/heads/master@{#333144}

[Mike Wittman, 2015-05-28 00:41:56]

wittman@chromium.org changed reviewers:
+ kalman@chromium.org

[Mike Wittman, 2015-05-28 01:17:33]

Hi Ben, I'm pausing work on the isBookmarked declarativeContent implementation
to refactor ChromeContentRulesRegistry to use a content condition abstraction.
As is, introducing a ProfileStateMatcher and managing the state tracking and
condition evaluation across the URL matcher, CSS selector, and isBookmarked
condition is just getting too complicated and difficult to adequately test.

Please take a look at this first CL in a series of several changes.

[not at google - send to devlin, 2015-05-28 16:05:45]

kalman@chromium.org changed reviewers:
+ jyasskin@chromium.org

[not at google - send to devlin, 2015-05-28 16:05:45]

Awesome. Fantastic to see this happening.

I'm happy to review this and will get to it ASAP, but fair warning, I have a
backlog of code reviews. It would be good to get some early feedback from
Jeffrey (or Jeffrey, if your comfortable, just do the more exhaustive review and
I can tackle it from the extensions perspective).

[not at google - send to devlin, 2015-06-01 22:51:12]

Ok, that sure is a lot to get through, here is my first pass. Comments are
mostly interface level, am I right in hoping that most of the implementation
level changes are shuffling logic around?

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry.cc
(right):

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry.cc:49:
case content::NOTIFICATION_WEB_CONTENTS_DESTROYED: {
Another case for using a WebContentsObserver here (... another case for a follow
up?)?

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry.h
(right):

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry.h:49:
typedef DeclarativeContentRule ContentRule;
Yeah, again we should try to delete this typedef in some way, whether that's
rename DeclarativeContentRule to ContentRule, or replace instances of
ContentRule with DeclarativeContentRule.

Basically typedefs purely for shorthand in header files are silly because they
apply globally, then everyone uses the shorthand, then there's no point having
the longhand.

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.cc
(right):

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.cc:89:
void DeclarativeContentCssConditionTracker::OnWebContentsNavigation(
Can this be part of your own WebContentsObserver implementation? See comment
below.

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.cc:176:
case content::NOTIFICATION_WEB_CONTENTS_DESTROYED: {
You should be able to include this as part of a WebContentsObserver interface -
for example, MatchedSelectorChangeObserver. That should let you make some more
simplifications: MatchedSelectorChangeObserver would then be able to manage its
own lifetime, so you wouldn't need to hold onto it in |matching_css_selectors_|,
and given that you'd be able to define the entire class within the .cc file
rather than in the header file.

(you'd also want to rename it)

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/declarative_content_rule.h
(right):

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/declarative_content_rule.h:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
(note, maybe of the comments in this file apply to more than just the single
line I made them on)

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/declarative_content_rule.h:35:
class DeclarativeContentConditionSet {
I'm not sure that DeclarativeContentConditionSet/DeclarativeContentActionSet
belong in this file, they seem pretty complex. Possibly worth splitting off.

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/declarative_content_rule.h:37:
typedef std::vector<linked_ptr<base::Value> > Values;
- Use "using Values = ..." rather than "typedef".
- Use ">>" not "> >".

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/declarative_content_rule.h:63:
// of the conditions without URL attributes is satisfied.
Can this be 2 separate interface methods, with an Impl-style private method
(fine to call it IsFulfilled as well) - one without |url_match_trigger|, and one
insisting that |url_match_trigger| is a valid number? I think it's odd than a
single interface method would be overloaded in this way.

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/declarative_content_rule.h:93:
typedef DeclarativeContentConditionSet ContentConditionSet;
Why not just directly call "DeclarativeContentConditionSet"
"ContentConditionSet"? They're both living in the same global namespace.

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/declarative_content_rule.h:120:
const base::Time& extension_install_time,
This method taking the extension's install time seems a bit bizarre. At this
point I'm looking at header files only (a nice way to get a feel for a CL) and
I'd guess it's for ranking extensions or something? Definitely in implementation
detail territory.

DeclarativeContentActionSet is aready constructed with a BrowserContext. It
could hold onto that, then as necessary grab the install time (via
ExtensionPrefs I believe).

Also, when possible, interface methods that deal with extensions should pass in
Extensions not extension IDs.

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/declarative_content_rule.h:152:
typedef std::string ExtensionId;
This typedef already exists in extension.h.

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/declarative_content_rule.h:159:
typedef std::vector<std::string> Tags;
That's a lot of typedefs. Some of them like Priority seem like overkill? And the
value of ConditionSet/ActionSet more confusing than helpful given they're
defined (and concisely enough, I would hope) within this file?

https://codereview.chromium.org/1159733004/diff/40001/extensions/browser/api/...
File extensions/browser/api/declarative_content/content_rules_registry.h (left):

https://codereview.chromium.org/1159733004/diff/40001/extensions/browser/api/...
extensions/browser/api/declarative_content/content_rules_registry.h:46: virtual
void DidNavigateMainFrame(
Writing comment here, though I first noticed it in implementations:

A method like "DidNavigateMainFrame" is odd as an interface method, because it's
not an action, rather a notification - and notifications should be handled
within the implementation of classes. Looking at where it's called, it seems to
just forward this event through to something else that could be listening to
those events.

Would it be easier to do this in a follow-up, though?

[Mike Wittman, 2015-06-05 01:17:45]

Multiple updates here:

- revert the DeclarativeRule => DeclarativeContentRule manual template
instantiation since we're not dealing with multiple condition types for now
- address comments and move all WebContents-specific state tracking into the
former MatchedSelectorChangeObserver
- unit tests for DeclarativeContentCssConditionTracker (requires moving
knowledge of the Browser out of the class)

Please take another look.

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry.cc
(right):

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry.cc:49:
case content::NOTIFICATION_WEB_CONTENTS_DESTROYED: {
On 2015/06/01 22:51:11, kalman wrote:
> Another case for using a WebContentsObserver here (... another case for a
follow
> up?)?

Yes, I'll look at this in a future CL.

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.cc
(right):

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.cc:89:
void DeclarativeContentCssConditionTracker::OnWebContentsNavigation(
On 2015/06/01 22:51:11, kalman wrote:
> Can this be part of your own WebContentsObserver implementation? See comment
> below.

It could as it stands now, but once I encapsulate the URL matcher tracking
ChromeContentRulesRegistry will still need to handle this and call into the
trackers itself to avoid inconsistency in condition evaluation. The problem is
that, if both trackers individually listen to this event, they'll each
individually request rule evaluation. Whichever tracker does this first will
have its conditions evaluated on the new URL, while the other tracker won't have
been notified will have its conditions evaluated on the old URL. If
ChromeContentRulesRegistry coordinates, then we can delay evaluation until all
trackers have had a chance to respond to the notification.

I suppose we could also address this by posting the rule evaluation invocation
back on the event loop. That would scale better to a large number of independent
trackers by eliminating coordination on common events, but I'm not sure if it's
worth doing at this point.

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.cc:176:
case content::NOTIFICATION_WEB_CONTENTS_DESTROYED: {
On 2015/06/01 22:51:11, kalman wrote:
> You should be able to include this as part of a WebContentsObserver interface
-
> for example, MatchedSelectorChangeObserver. That should let you make some more
> simplifications: MatchedSelectorChangeObserver would then be able to manage
its
> own lifetime, so you wouldn't need to hold onto it in
|matching_css_selectors_|,
> and given that you'd be able to define the entire class within the .cc file
> rather than in the header file.
> 
> (you'd also want to rename it)

Good point. I think it makes sense to go further and make object own and manage
all of the per-WebContents state.

https://codereview.chromium.org/1159733004/diff/40001/extensions/browser/api/...
File extensions/browser/api/declarative_content/content_rules_registry.h (left):

https://codereview.chromium.org/1159733004/diff/40001/extensions/browser/api/...
extensions/browser/api/declarative_content/content_rules_registry.h:46: virtual
void DidNavigateMainFrame(
On 2015/06/01 22:51:12, kalman wrote:
> Writing comment here, though I first noticed it in implementations:
> 
> A method like "DidNavigateMainFrame" is odd as an interface method, because
it's
> not an action, rather a notification - and notifications should be handled
> within the implementation of classes. Looking at where it's called, it seems
to
> just forward this event through to something else that could be listening to
> those events.
> 
> Would it be easier to do this in a follow-up, though?

It would. My intention is to move the notification handling from TabHelper to
ChromeContentRulesRegistry with my next CL, which will encapsulate the URL
matcher. I'll need to update the logic in this function then as well.

See also my comment on this function in DeclarativeContentCssConditionTracker.

[not at google - send to devlin, 2015-06-05 17:16:30]

lgtm

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.cc
(right):

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.cc:89:
void DeclarativeContentCssConditionTracker::OnWebContentsNavigation(
On 2015/06/05 01:17:44, Mike Wittman wrote:
> On 2015/06/01 22:51:11, kalman wrote:
> > Can this be part of your own WebContentsObserver implementation? See comment
> > below.
> 
> It could as it stands now, but once I encapsulate the URL matcher tracking
> ChromeContentRulesRegistry will still need to handle this and call into the
> trackers itself to avoid inconsistency in condition evaluation. The problem is
> that, if both trackers individually listen to this event, they'll each
> individually request rule evaluation. Whichever tracker does this first will
> have its conditions evaluated on the new URL, while the other tracker won't
have
> been notified will have its conditions evaluated on the old URL. If
> ChromeContentRulesRegistry coordinates, then we can delay evaluation until all
> trackers have had a chance to respond to the notification.
> 
> I suppose we could also address this by posting the rule evaluation invocation
> back on the event loop. That would scale better to a large number of
independent
> trackers by eliminating coordination on common events, but I'm not sure if
it's
> worth doing at this point.

Ah right. No, what you have sounds right them. I don't think that posting to the
event loop will necessarily work, since you'd have the same problem when the
event loop next runs?

https://codereview.chromium.org/1159733004/diff/80001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.h
(right):

https://codereview.chromium.org/1159733004/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.h:73:
// Handles navigation of |contents|.
You could quickly note here why DeclarativeContentCssConditionTracker can't do
this itself (much as you explained in your comment)?

https://codereview.chromium.org/1159733004/diff/80001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker_unittest.cc
(right):

https://codereview.chromium.org/1159733004/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker_unittest.cc:189:
details.is_in_page = false;
(usually I prefer to scope local variables like this inside blocks so that you
don't need to reset values to their default state, but rather just re-declare
them)

[Mike Wittman, 2015-06-05 19:30:30]

Patchset #6 (id:100001) has been deleted

[Mike Wittman, 2015-06-05 20:21:53]

Patchset #6 (id:120001) has been deleted

[Mike Wittman, 2015-06-05 21:00:46]

One more patch to addresses the previous comments, and fix test failures due to
the CSS tracker being notified of WebContents it wasn't tracking for. I've
updated ChromeContentRulesRegistry to only call
DeclarativeContentCssConditionTracker::OnWebContentsNavigation for WebContents
the DeclarativeContentCssConditionTracker knows about. This resulted in some
follow-on changes due to ChromeContentRulesRegistry needing to initiate the
looping over all WebContents on construction.

Take another look if you'd like.

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.cc
(right):

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.cc:89:
void DeclarativeContentCssConditionTracker::OnWebContentsNavigation(
On 2015/06/05 17:16:30, kalman wrote:
> On 2015/06/05 01:17:44, Mike Wittman wrote:
> > On 2015/06/01 22:51:11, kalman wrote:
> > > Can this be part of your own WebContentsObserver implementation? See
comment
> > > below.
> > 
> > It could as it stands now, but once I encapsulate the URL matcher tracking
> > ChromeContentRulesRegistry will still need to handle this and call into the
> > trackers itself to avoid inconsistency in condition evaluation. The problem
is
> > that, if both trackers individually listen to this event, they'll each
> > individually request rule evaluation. Whichever tracker does this first will
> > have its conditions evaluated on the new URL, while the other tracker won't
> have
> > been notified will have its conditions evaluated on the old URL. If
> > ChromeContentRulesRegistry coordinates, then we can delay evaluation until
all
> > trackers have had a chance to respond to the notification.
> > 
> > I suppose we could also address this by posting the rule evaluation
invocation
> > back on the event loop. That would scale better to a large number of
> independent
> > trackers by eliminating coordination on common events, but I'm not sure if
> it's
> > worth doing at this point.
> 
> Ah right. No, what you have sounds right them. I don't think that posting to
the
> event loop will necessarily work, since you'd have the same problem when the
> event loop next runs?

I think it should work if all the tracker state is updated synchronously from
the notification, and just the evaluation is posted back to the event loop. The
OnWebContentsNavigation notifications for a given navigation occur in a single
invocation from the event loop, right?

https://codereview.chromium.org/1159733004/diff/80001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.h
(right):

https://codereview.chromium.org/1159733004/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.h:73:
// Handles navigation of |contents|.
On 2015/06/05 17:16:30, kalman wrote:
> You could quickly note here why DeclarativeContentCssConditionTracker can't do
> this itself (much as you explained in your comment)?

Done.

https://codereview.chromium.org/1159733004/diff/80001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker_unittest.cc
(right):

https://codereview.chromium.org/1159733004/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker_unittest.cc:189:
details.is_in_page = false;
On 2015/06/05 17:16:30, kalman wrote:
> (usually I prefer to scope local variables like this inside blocks so that you
> don't need to reset values to their default state, but rather just re-declare
> them)

Done.

[not at google - send to devlin, 2015-06-05 21:15:21]

still looks fine

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.cc
(right):

https://codereview.chromium.org/1159733004/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.cc:89:
void DeclarativeContentCssConditionTracker::OnWebContentsNavigation(
On 2015/06/05 21:00:46, Mike Wittman wrote:
> On 2015/06/05 17:16:30, kalman wrote:
> > On 2015/06/05 01:17:44, Mike Wittman wrote:
> > > On 2015/06/01 22:51:11, kalman wrote:
> > > > Can this be part of your own WebContentsObserver implementation? See
> comment
> > > > below.
> > > 
> > > It could as it stands now, but once I encapsulate the URL matcher tracking
> > > ChromeContentRulesRegistry will still need to handle this and call into
the
> > > trackers itself to avoid inconsistency in condition evaluation. The
problem
> is
> > > that, if both trackers individually listen to this event, they'll each
> > > individually request rule evaluation. Whichever tracker does this first
will
> > > have its conditions evaluated on the new URL, while the other tracker
won't
> > have
> > > been notified will have its conditions evaluated on the old URL. If
> > > ChromeContentRulesRegistry coordinates, then we can delay evaluation until
> all
> > > trackers have had a chance to respond to the notification.
> > > 
> > > I suppose we could also address this by posting the rule evaluation
> invocation
> > > back on the event loop. That would scale better to a large number of
> > independent
> > > trackers by eliminating coordination on common events, but I'm not sure if
> > it's
> > > worth doing at this point.
> > 
> > Ah right. No, what you have sounds right them. I don't think that posting to
> the
> > event loop will necessarily work, since you'd have the same problem when the
> > event loop next runs?
> 
> I think it should work if all the tracker state is updated synchronously from
> the notification, and just the evaluation is posted back to the event loop.
The
> OnWebContentsNavigation notifications for a given navigation occur in a single
> invocation from the event loop, right?

not sure, but I don't particularly trust that sort of async code as an
implementation detail? what you have is fine I think.

[Mike Wittman, 2015-06-05 21:16:57]

The CQ bit was checked by wittman@chromium.org

[Mike Wittman, 2015-06-05 21:16:57]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1159733004/#ps140001
(title: "only notify navigation for tracked WebContents, address comments")

[commit-bot: I haz the power, 2015-06-05 21:17:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1159733004/140001

[commit-bot: I haz the power, 2015-06-05 21:51:45]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2015-06-05 21:52:50]

Patchset 6 (id:??) landed as
https://crrev.com/fe76220cf0a1cdcfe74ca7daeae9f1da2b91cc6b
Cr-Commit-Position: refs/heads/master@{#333144}

[dominicc (has gone to gerrit), 2015-06-08 04:14:49]

A revert of this CL (patchset #6 id:140001) has been created in
https://codereview.chromium.org/1159623014/ by dominicc@chromium.org.

The reason for reverting is: Stability sheriff here; I'm speculatively rolling
this out because I suspect it caused Issue 497586..