Use Blink support to watch CSS selectors directly instead of using a MutationObserver.

Use Blink support to watch CSS selectors directly instead of using a MutationObserver.

WebKit::WebDocument::watchCSSSelectors was added in https://src.chromium.org/viewvc/blink?view=rev&revision=158582.

This CL also threads the WebFrameClient::didMatchCSS call back through the Content API.

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

[Jeffrey Yasskin, 2013-10-02 02:58:43]

Extension folks, could you take an initial look before I send this to the
Content owners?

[Matt Perry, 2013-10-02 22:20:21]

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

https://codereview.chromium.org/19045002/diff/35001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc:102:
ASSERT_TRUE(content::ExecuteScript(tab, std::string()));
This double call looks fishy. Are you just calling it again to force a
RunMessageLoop()?

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

https://codereview.chromium.org/19045002/diff/35001/chrome/renderer/extension...
chrome/renderer/extensions/content_watcher.cc:29: bool unchanged =
new_css_selectors.size() == css_selectors_.size();
nit: negative bools always confuse me. how about "changed"

https://codereview.chromium.org/19045002/diff/35001/chrome/renderer/extension...
chrome/renderer/extensions/content_watcher.cc:40: css_selectors_ =
new_css_selectors;
more efficient to do css_selectors_.swap(new_css_selectors)?

https://codereview.chromium.org/19045002/diff/35001/chrome/renderer/extension...
chrome/renderer/extensions/content_watcher.cc:83: std::set<std::string>
ContentWatcher::FindMatchingSelectors(
I don't see this method called anywhere in this CL, which confuses me how its
signature can change.

[Jeffrey Yasskin, 2013-10-03 00:31:57]

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

https://codereview.chromium.org/19045002/diff/35001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc:102:
ASSERT_TRUE(content::ExecuteScript(tab, std::string()));
On 2013/10/02 22:20:22, Matt Perry wrote:
> This double call looks fishy. Are you just calling it again to force a
> RunMessageLoop()?

It's to force a RunMessageLoop in the Blink process. That's icky, but the
technique using Notifications was flaky.

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

https://codereview.chromium.org/19045002/diff/35001/chrome/renderer/extension...
chrome/renderer/extensions/content_watcher.cc:29: bool unchanged =
new_css_selectors.size() == css_selectors_.size();
On 2013/10/02 22:20:22, Matt Perry wrote:
> nit: negative bools always confuse me. how about "changed"

Sounds good. Done.

https://codereview.chromium.org/19045002/diff/35001/chrome/renderer/extension...
chrome/renderer/extensions/content_watcher.cc:40: css_selectors_ =
new_css_selectors;
On 2013/10/02 22:20:22, Matt Perry wrote:
> more efficient to do css_selectors_.swap(new_css_selectors)?

Ah, it does work now to build new_css_selectors as a WebVector. Done.

https://codereview.chromium.org/19045002/diff/35001/chrome/renderer/extension...
chrome/renderer/extensions/content_watcher.cc:83: std::set<std::string>
ContentWatcher::FindMatchingSelectors(
On 2013/10/02 22:20:22, Matt Perry wrote:
> I don't see this method called anywhere in this CL, which confuses me how its
> signature can change.

You're right, it's obsolete. Deleted.

[Matt Perry, 2013-10-03 00:34:54]

lgtm

[Jeffrey Yasskin, 2013-10-03 01:08:56]

+jam to look at threading WebFrameClient::didMatchCSS through Content. I'm not
certain I got the RenderFrameImpl bit right.
Do you want to approve renderer_resources.grd too, or should I get another
top-level owner to catch that?

[jam, 2013-10-03 17:19:14]

+Charlie for his thoughts on how this will interact with site isolation, in
particular this is an API on WebDocument which leads to callbacks on
WebFrameClient. Charlie: are we going to have to change this in a big way in the
near term, or is this fine? I guess we also will have a lot of fun figuring out
how to split/morph RenderViewObserver.

minor nit: newlyMatchingSelectors should be newly_matching_selectors, as the
variable names switch to google style outside blink

[Charlie Reis, 2013-10-03 17:37:55]

On 2013/10/03 17:19:14, jam wrote:
> +Charlie for his thoughts on how this will interact with site isolation, in
> particular this is an API on WebDocument which leads to callbacks on
> WebFrameClient. Charlie: are we going to have to change this in a big way in
the
> near term, or is this fine? I guess we also will have a lot of fun figuring
out
> how to split/morph RenderViewObserver.
> 
> minor nit: newlyMatchingSelectors should be newly_matching_selectors, as the
> variable names switch to google style outside blink

[CC'ing Nasko as FYI on RenderFrame change.]

This looks fine to me, since WebDocument is per-frame.  WebFrameClient and then
RenderFrame seems like the right way to route it.  (WebCore::Page is the
top-level one, which would have been problematic.)  content/ LGTM.

Also, yes, splitting up RenderViewObserver for site isolation is going to be
fun.  :)

[Jeffrey Yasskin, 2013-10-03 18:41:34]

On 2013/10/03 17:19:14, jam wrote:
> minor nit: newlyMatchingSelectors should be newly_matching_selectors, as the
> variable names switch to google style outside blink

This is fixed; thanks.

[jam, 2013-10-03 21:24:35]

On 2013/10/03 17:37:55, creis wrote:
> On 2013/10/03 17:19:14, jam wrote:
> > +Charlie for his thoughts on how this will interact with site isolation, in
> > particular this is an API on WebDocument which leads to callbacks on
> > WebFrameClient. Charlie: are we going to have to change this in a big way in
> the
> > near term, or is this fine? I guess we also will have a lot of fun figuring
> out
> > how to split/morph RenderViewObserver.
> > 
> > minor nit: newlyMatchingSelectors should be newly_matching_selectors, as the
> > variable names switch to google style outside blink
> 
> [CC'ing Nasko as FYI on RenderFrame change.]
> 
> This looks fine to me, since WebDocument is per-frame.  WebFrameClient and
then
> RenderFrame seems like the right way to route it.  (WebCore::Page is the
> top-level one, which would have been problematic.)  content/ LGTM.
> 
> Also, yes, splitting up RenderViewObserver for site isolation is going to be
> fun.  :)

thanks for the lookover. to be explicit, since Charlie reviewed this is fine by
me, so no need to wait for me.

[commit-bot: I haz the power, 2013-10-03 21:34:18]

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

[Jeffrey Yasskin, 2013-10-03 21:54:10]

I still need an LGTM for chrome/renderer/resources/renderer_resources.grd. Nico,
since you approved the addition of content_watcher.js, want to approve its
removal?

[Nico, 2013-10-03 22:02:21]

grd change / js removal lgtm

[commit-bot: I haz the power, 2013-10-03 22:02:48]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-10-03 22:03:59]

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

[commit-bot: I haz the power, 2013-10-04 02:42:56]

Change committed as 226938