Reland "Encapsulate CSS selector declarative content condition tracking"

Reland "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 step 2 described in the associated bug: encapsulation of CSS
selector condition tracking logic. No functional change is intended.

Originally committed at https://crrev.com/fe76220cf0a1cdcfe74ca7daeae9f1da2b91cc6b
and reverted at https://crrev.com/4fcf9f62d9099af2e55186d9c423e2ded4fdca0d due to crashes.

BUG=492946, 497586
Committed: https://crrev.com/2b0e40655fff162c0fe5752623ceab5ddd79d807
Cr-Commit-Position: refs/heads/master@{#333627}

[Mike Wittman, 2015-06-09 01:48:06]

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

[Mike Wittman, 2015-06-09 02:37:36]

Hi Ben, please take (yet) another look at these changes.

To fix two crashes seen in canary, I've made these changes:

1. Don't assume that the set of WebContents with TabHelpers attached is the same
as those added with NOTIFICATION_TAB_ADDED. Instead, monitor exactly and only
the WebContents that have TabHelpers attached.

This actually fixes an issue with the existing code, where
ChromeContentRulesRegistry never actually knew about the WebContents it was
supposed to monitor until it got a navigation or a CSS selector match IPC.

2. Don't assume that the DeclarativeContentCssConditionTracker outlives all
WebContents associated with its BrowserContext. Based on the failing CHECK in
~DeclarativeContentCssConditionTracker, apparently there are cases where this is
not true.

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

https://codereview.chromium.org/1166393002/diff/20001/extensions/browser/api/...
extensions/browser/api/declarative_content/content_rules_registry.h:53: virtual
void DidNavigateMainFrameOfOriginalContext(
I coalesced this back into DidNavigateMainFrame since all functions on this
interface must be called for both Original and OffTheRecord registries, and it
seems undesirable to duplicate every function for both cases.

This also allows us to encapsulate the non-trivial logic in TabHelper for
deciding which ContentRulesRegistries to use when invoking member functions.

[not at google - send to devlin, 2015-06-09 20:38:11]

lgtm

https://codereview.chromium.org/1166393002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/tab_helper.cc (right):

https://codereview.chromium.org/1166393002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:81: void
TabHelper::InvokeForContentRulesRegistries(const Func& func) {
Usually methods are defined in the .cc file in the order they were declared in
the header file. Do you need to do it this way for the compiler or something?

You could fix this by putting this in an anonymous namespace, then currying
|profile_| into this function, like:

template <typename Func>
void InvokeForContentRulesRegistries(Profile* profile, const Func& func) {...}

?

https://codereview.chromium.org/1166393002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/tab_helper.h (right):

https://codereview.chromium.org/1166393002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.h:69: // App extensions
------------------------------------------------------------
and in fact so does this.

https://codereview.chromium.org/1166393002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.h:218: // Data for app extensions
---------------------------------------------------
This comment seems entirely out of place. Can you delete it, cos, why not?

[Mike Wittman, 2015-06-09 22:05:57]

https://codereview.chromium.org/1166393002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/tab_helper.cc (right):

https://codereview.chromium.org/1166393002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:81: void
TabHelper::InvokeForContentRulesRegistries(const Func& func) {
On 2015/06/09 20:38:11, kalman wrote:
> Usually methods are defined in the .cc file in the order they were declared in
> the header file. Do you need to do it this way for the compiler or something?

I could swear that the template function had to be defined before being
instantiated (in the constructor below), but in trying to move it afterwards
apparently this is not the case. Perhaps instantiation requirements have been
relaxed in C++11, or maybe I'm just misremembering

> You could fix this by putting this in an anonymous namespace, then currying
> |profile_| into this function, like:
> 
> template <typename Func>
> void InvokeForContentRulesRegistries(Profile* profile, const Func& func) {...}
> 
> ?

This is what I had originally, but it makes the callsites less readable due to
the extra line breaks.

https://codereview.chromium.org/1166393002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/tab_helper.h (right):

https://codereview.chromium.org/1166393002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.h:69: // App extensions
------------------------------------------------------------
On 2015/06/09 20:38:11, kalman wrote:
> and in fact so does this.

Done.

https://codereview.chromium.org/1166393002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.h:218: // Data for app extensions
---------------------------------------------------
On 2015/06/09 20:38:11, kalman wrote:
> This comment seems entirely out of place. Can you delete it, cos, why not?

Yes, I found this odd also. Removed.

[Mike Wittman, 2015-06-09 22:07:47]

The CQ bit was checked by wittman@chromium.org

[Mike Wittman, 2015-06-09 22:07:47]

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/1166393002/#ps60001
(title: "address comments")

[commit-bot: I haz the power, 2015-06-09 22:09:45]

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

[commit-bot: I haz the power, 2015-06-10 00:53:16]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-06-10 00:53:55]

Patchset 4 (id:??) landed as
https://crrev.com/2b0e40655fff162c0fe5752623ceab5ddd79d807
Cr-Commit-Position: refs/heads/master@{#333627}