Add a WebDocument::watchCssSelectors(selectors)

Add WebDocument::watchCssSelectors(selectors)
which requests callbacks to track whether elements matching |selectors|
exist within the document.  Blink calls the embedder through
WebFrameClient::didMatchCss(frame, newlyMatching, stoppedMatching) to
report changes.

This patch only watches for the binary set "selector matches anything on the page" vs "selector matches nothing on the page". There are some use cases for getting called back when the number of elements matching the selector changes, but I'm not suggesting an interface for that yet. It might also be useful to publish this to the web through MutationObservers. I'm also not proposing that. :)

This continues https://bugs.webkit.org/show_bug.cgi?id=110524.

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

[Jeffrey Yasskin, 2013-07-12 22:40:41]

PTAL.

Eric points out that we might be able to merge this with a fix to
http://crbug.com/259682 (by somehow registering to watch rule matching, maybe on
a subset of rules identified by a filter of some sort?). The right way to do
this isn't immediately obvious, but I figured I'd mention it in case someone has
a good idea.

Lots of the changes here are just threading data between components. The most
interesting files are probably WebFrameTest.cpp, Document.*,
StyleResolverState.h, RenderStyle.*, and StyleRareNonInheritedData.*.

This change depends on https://codereview.chromium.org/5216392399814656/ and
https://codereview.chromium.org/19047002.

https://codereview.chromium.org/18371008/diff/10001/Source/core/features.gypi
File Source/core/features.gypi (right):

https://codereview.chromium.org/18371008/diff/10001/Source/core/features.gypi...
Source/core/features.gypi:36: 'ENABLE_CSS_CALLBACKS=1',
Do I still need this in Blink?

https://codereview.chromium.org/18371008/diff/10001/public/web/WebDocument.h
File public/web/WebDocument.h (right):

https://codereview.chromium.org/18371008/diff/10001/public/web/WebDocument.h#...
public/web/WebDocument.h:129: WEBKIT_EXPORT void watchCssSelectors(const
WebVector<WebString>& selectors);
Used by https://codereview.chromium.org/19045002/.

[abarth-chromium, 2013-07-12 23:44:10]

You're going to need some CSS experts to look at this CL as well.

https://codereview.chromium.org/18371008/diff/10001/Source/WebKit/chromium/te...
File Source/WebKit/chromium/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/18371008/diff/10001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/WebFrameTest.cpp:35: #include <gmock/gmock.h>
I remember someone was telling us to avoid gmock...  Not sure if I understand
why.

https://codereview.chromium.org/18371008/diff/10001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/WebFrameTest.cpp:277: struct
CssCallbackWebFrameClient : public WebFrameClient {
CssCallbackWebFrameClient -> CSSCallbackWebFrameClient

https://codereview.chromium.org/18371008/diff/10001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/WebFrameTest.cpp:279: virtual void
didMatchCss(WebFrame* frame, const WebVector<WebString>& newlyMatchingSelectors,
const WebVector<WebString>& stoppedMatchingSelectors) OVERRIDE
didMatchCss -> didMatchCSS

Basically, we use CSS for CSS.

https://codereview.chromium.org/18371008/diff/10001/Source/core/css/CSSComput...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/18371008/diff/10001/Source/core/css/CSSComput...
Source/core/css/CSSComputedStyleDeclaration.cpp:2697: #if ENABLE(CSS_CALLBACKS)
No need for ENABLE macros.  We're aiming for one compile configuration.

https://codereview.chromium.org/18371008/diff/10001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/18371008/diff/10001/Source/core/dom/Document....
Source/core/dom/Document.cpp:1905: #endif // ENABLE(CSS_CALLBACKS)
This code should move into a supplement too.  It's tempting to make Document an
all-sing, all-dance class, but Document can't know about every feature in the
universe.  It's poor head would explode.

https://codereview.chromium.org/18371008/diff/10001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/18371008/diff/10001/Source/core/dom/Document....
Source/core/dom/Document.h:1368: Timer<Document> m_callbackSelectorChangeTimer;
Please don't spam state onto Document.  If you need to associate state with a
Document, consider using a subclass of Supplement<ScriptExecutionContext>. 
There should be a bunch of examples.

https://codereview.chromium.org/18371008/diff/10001/Source/core/features.gypi
File Source/core/features.gypi (right):

https://codereview.chromium.org/18371008/diff/10001/Source/core/features.gypi...
Source/core/features.gypi:36: 'ENABLE_CSS_CALLBACKS=1',
On 2013/07/12 22:40:42, Jeffrey Yasskin wrote:
> Do I still need this in Blink?

Nope.

https://codereview.chromium.org/18371008/diff/10001/Source/core/rendering/sty...
File Source/core/rendering/style/StyleRareNonInheritedData.cpp (right):

https://codereview.chromium.org/18371008/diff/10001/Source/core/rendering/sty...
Source/core/rendering/style/StyleRareNonInheritedData.cpp:57: , m_document()
Why do you need this explicit call to the constructor?  I'd think the implicit
call would be sufficient.

https://codereview.chromium.org/18371008/diff/10001/Source/web/FrameLoaderCli...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/18371008/diff/10001/Source/web/FrameLoaderCli...
Source/web/FrameLoaderClientImpl.cpp:87: #include "public/platform/WebThread.h"
Why do you need this header?  I don't see anything related to threads here.

https://codereview.chromium.org/18371008/diff/10001/Source/web/WebDocument.cpp
File Source/web/WebDocument.cpp (right):

https://codereview.chromium.org/18371008/diff/10001/Source/web/WebDocument.cp...
Source/web/WebDocument.cpp:211: void WebDocument::watchCssSelectors(const
WebVector<WebString>& selectors)
selectors -> webSelectors

https://codereview.chromium.org/18371008/diff/10001/Source/web/WebDocument.cp...
Source/web/WebDocument.cpp:214: Vector<String> wtfSelectors;
wtfSelectors -> selectors

[abarth-chromium, 2013-07-12 23:46:30]

How many documents will watchCSSSelectors be used on?  If I have an extension
that watches for selectors, does that mean that every document in the universe
is going to have all this state and machinery turned on?  Have you measured the
performance cost (runtime and memory) of this feature?

[esprehn, 2013-07-13 01:43:00]

StyleRareNonInheritedData is huge btw, you're giving extensions a huge memory
footgun if they did something like matchesCss("div") and put this on every
element. Normally we share this thing, but you're adding unique ones all over.

StyleRareNonInheritedData should also not know about Documents, this abstraction
level seems wrong, I think you want to hook Element::recalcStyle.

https://codereview.chromium.org/18371008/diff/10001/Source/core/css/DocumentR...
File Source/core/css/DocumentRuleSets.cpp (right):

https://codereview.chromium.org/18371008/diff/10001/Source/core/css/DocumentR...
Source/core/css/DocumentRuleSets.cpp:106: for (unsigned i = 0; i <
watchedSelectors.size(); ++i) {
Leave off the braces

https://codereview.chromium.org/18371008/diff/10001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/18371008/diff/10001/Source/core/css/resolver/...
Source/core/css/resolver/StyleBuilderCustom.cpp:792: if (primitiveValue) {
if (primitiveValue && primitiveValue->getValueID())
  state.style()->addCallbackSelector( ... );

Since you only allow one value would be simpler

https://codereview.chromium.org/18371008/diff/10001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolverState.h (right):

https://codereview.chromium.org/18371008/diff/10001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolverState.h:131: const StyleRule* rule() const
{ return m_rule; }
currentRule() ?

https://codereview.chromium.org/18371008/diff/10001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/18371008/diff/10001/Source/core/dom/Document....
Source/core/dom/Document.cpp:3105: styleResolverChanged(RecalcStyleImmediately,
FullStyleUpdate);
DeferRecalcStyle, this definitely doesn't need a synchronous update. You can
also omit the second argument.

https://codereview.chromium.org/18371008/diff/10001/Source/core/rendering/sty...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/18371008/diff/10001/Source/core/rendering/sty...
Source/core/rendering/style/RenderStyle.cpp:704:
document->addSelectorMatch(selector, data);
Please do this call outside this function. Return a boolean to signal a change.

https://codereview.chromium.org/18371008/diff/10001/Source/core/rendering/sty...
Source/core/rendering/style/RenderStyle.cpp:714:
document->removeSelectorMatch(selector);
Ditto, mutating the RenderStyle should have no side effects outside the style.

https://codereview.chromium.org/18371008/diff/10001/Source/core/rendering/sty...
File Source/core/rendering/style/StyleRareNonInheritedData.cpp (right):

https://codereview.chromium.org/18371008/diff/10001/Source/core/rendering/sty...
Source/core/rendering/style/StyleRareNonInheritedData.cpp:171:
document->removeSelectorMatch(*it);
No need to cache the ptr, m_document->...() is fine. This also looks suspicous,
I think you want to hook Element::recalcStyle and look at the RenderStyle to see
if it has your magic property and bump the counter up and down.

https://codereview.chromium.org/18371008/diff/10001/Source/core/rendering/sty...
File Source/core/rendering/style/StyleRareNonInheritedData.h (right):

https://codereview.chromium.org/18371008/diff/10001/Source/core/rendering/sty...
Source/core/rendering/style/StyleRareNonInheritedData.h:150: WeakPtr<Document>
m_document;
This doesn't look right, why do you have a document pointer down here? Also
StyleRareNonInheritedData is huge, an extension could easily end up adding it to
every style this way. ex. doing "div" to show up on every page.

Why do you need the m_document ptr here? All RenderStyle objects are tied to a
specific document anyway and so is the SelectorChecker and the StyleResolver. I
don't think you need those document checks

[Jeffrey Yasskin, 2013-07-16 00:38:51]

No need to re-review until I get some benchmark results.

Elliott, I don't intend the current version of these callbacks to make their
RenderStyles unique. Did I miss something?

https://codereview.chromium.org/18371008/diff/10001/Source/WebKit/chromium/te...
File Source/WebKit/chromium/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/18371008/diff/10001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/WebFrameTest.cpp:35: #include <gmock/gmock.h>
On 2013/07/12 23:44:11, abarth wrote:
> I remember someone was telling us to avoid gmock...  Not sure if I understand
> why.

Actual mocks are often a mistake. Here I'm just using gmock for the EXPECT_THAT
support, which makes asserting the contents of a vector more readable.

https://codereview.chromium.org/18371008/diff/10001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/WebFrameTest.cpp:277: struct
CssCallbackWebFrameClient : public WebFrameClient {
On 2013/07/12 23:44:11, abarth wrote:
> CssCallbackWebFrameClient -> CSSCallbackWebFrameClient

Done.

https://codereview.chromium.org/18371008/diff/10001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/WebFrameTest.cpp:279: virtual void
didMatchCss(WebFrame* frame, const WebVector<WebString>& newlyMatchingSelectors,
const WebVector<WebString>& stoppedMatchingSelectors) OVERRIDE
On 2013/07/12 23:44:11, abarth wrote:
> didMatchCss -> didMatchCSS
> 
> Basically, we use CSS for CSS.

Done everywhere.

https://codereview.chromium.org/18371008/diff/10001/Source/core/css/CSSComput...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/18371008/diff/10001/Source/core/css/CSSComput...
Source/core/css/CSSComputedStyleDeclaration.cpp:2697: #if ENABLE(CSS_CALLBACKS)
On 2013/07/12 23:44:11, abarth wrote:
> No need for ENABLE macros.  We're aiming for one compile configuration.

Great! Removed all of them.

https://codereview.chromium.org/18371008/diff/10001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/18371008/diff/10001/Source/core/dom/Document....
Source/core/dom/Document.cpp:1905: #endif // ENABLE(CSS_CALLBACKS)
On 2013/07/12 23:44:11, abarth wrote:
> This code should move into a supplement too.  It's tempting to make Document
an
> all-sing, all-dance class, but Document can't know about every feature in the
> universe.  It's poor head would explode.

Done.

https://codereview.chromium.org/18371008/diff/10001/Source/core/dom/Document....
Source/core/dom/Document.cpp:3105: styleResolverChanged(RecalcStyleImmediately,
FullStyleUpdate);
On 2013/07/13 01:43:01, esprehn wrote:
> DeferRecalcStyle, this definitely doesn't need a synchronous update. You can
> also omit the second argument.

Done.

https://codereview.chromium.org/18371008/diff/10001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/18371008/diff/10001/Source/core/dom/Document....
Source/core/dom/Document.h:1368: Timer<Document> m_callbackSelectorChangeTimer;
On 2013/07/12 23:44:11, abarth wrote:
> Please don't spam state onto Document.  If you need to associate state with a
> Document, consider using a subclass of Supplement<ScriptExecutionContext>. 
> There should be a bunch of examples.

Sounds good. Thanks for pointing that out.

https://codereview.chromium.org/18371008/diff/10001/Source/core/features.gypi
File Source/core/features.gypi (right):

https://codereview.chromium.org/18371008/diff/10001/Source/core/features.gypi...
Source/core/features.gypi:36: 'ENABLE_CSS_CALLBACKS=1',
On 2013/07/12 23:44:11, abarth wrote:
> On 2013/07/12 22:40:42, Jeffrey Yasskin wrote:
> > Do I still need this in Blink?
> 
> Nope.

Removed!

https://codereview.chromium.org/18371008/diff/10001/Source/core/rendering/sty...
File Source/core/rendering/style/StyleRareNonInheritedData.cpp (right):

https://codereview.chromium.org/18371008/diff/10001/Source/core/rendering/sty...
Source/core/rendering/style/StyleRareNonInheritedData.cpp:57: , m_document()
On 2013/07/12 23:44:11, abarth wrote:
> Why do you need this explicit call to the constructor?  I'd think the implicit
> call would be sufficient.

I don't. That's a remnant from an earlier version of the patch. Removed.

https://codereview.chromium.org/18371008/diff/10001/Source/core/rendering/sty...
Source/core/rendering/style/StyleRareNonInheritedData.cpp:171:
document->removeSelectorMatch(*it);
On 2013/07/13 01:43:01, esprehn wrote:
> No need to cache the ptr, m_document->...() is fine.

WTF's WeakPtr doesn't actually have an operator->, but I can switch to
m_document.get()->...()

> This also looks suspicous,
> I think you want to hook Element::recalcStyle and look at the RenderStyle to
see
> if it has your magic property and bump the counter up and down.

I was trying to avoid adding any code to the non-rare path, but I can definitely
switch to 'if's in recalcStyle and probably ~Element. I'll do that after writing
some benchmarks to see if it has any effect on performance.

https://codereview.chromium.org/18371008/diff/10001/Source/web/FrameLoaderCli...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/18371008/diff/10001/Source/web/FrameLoaderCli...
Source/web/FrameLoaderClientImpl.cpp:87: #include "public/platform/WebThread.h"
On 2013/07/12 23:44:11, abarth wrote:
> Why do you need this header?  I don't see anything related to threads here.

I don't. Gone.

https://codereview.chromium.org/18371008/diff/10001/Source/web/WebDocument.cpp
File Source/web/WebDocument.cpp (right):

https://codereview.chromium.org/18371008/diff/10001/Source/web/WebDocument.cp...
Source/web/WebDocument.cpp:211: void WebDocument::watchCssSelectors(const
WebVector<WebString>& selectors)
On 2013/07/12 23:44:11, abarth wrote:
> selectors -> webSelectors

Done.

https://codereview.chromium.org/18371008/diff/10001/Source/web/WebDocument.cp...
Source/web/WebDocument.cpp:214: Vector<String> wtfSelectors;
On 2013/07/12 23:44:11, abarth wrote:
> wtfSelectors -> selectors

Done.

[Jeffrey Yasskin, 2013-07-17 03:42:13]

On 2013/07/12 23:46:30, abarth wrote:
> How many documents will watchCSSSelectors be used on?  If I have an extension
> that watches for selectors, does that mean that every document in the universe
> is going to have all this state and machinery turned on?  Have you measured
the
> performance cost (runtime and memory) of this feature?

I've now run the intl2 page cycler with and without this patch, with no
extensions, and with 3 modified versions of https://code.google.com/p/oplop/.
"oplop_contentScript" is just updated to use manifest v2;
"oplop_declarativeContent" is further updated to use chrome.declarativeContent
to watch "input[type='password']"; and "oplop_declarativeContent_div"
additionally watches "div" (esprehn's concern). I ran:

rm -rf ~/.config/chromium/ && out/Release/chrome --enable-file-cookies
--load-extension=$(echo ~/extensions/oplop_declarativeContent/)
--js-flags="--expose_gc"
'file:///.../page_cycler/intl2/start.html?iterations=10&auto=1'

I only have the summary timings here.
http://www.chromium.org/developers/testing/page-cyclers/analyzing-page-cycler...
discusses using R to analyze the bot output, but I couldn't find how to get the
timings array locally. I also don't see how to measure memory use. Any tips?

pre-change, no extension:
iterations	10
pages	30
milliseconds	32787
mean per set	3278.70
mean per page	109.29
timer lag	4637.00
timer lag per page	15.46

iterations	10
pages	30
milliseconds	33446
mean per set	3344.60
mean per page	111.49
timer lag	4166.00
timer lag per page	13.89


pre-change, oplop_contentScript

iterations	10
pages	30
milliseconds	32386
mean per set	3238.60
mean per page	107.95
timer lag	4166.00
timer lag per page	13.89

iterations	10
pages	30
milliseconds	32906
mean per set	3290.60
mean per page	109.69
timer lag	4255.00
timer lag per page	14.18


pre-change, oplop_declarativeContent:

iterations	10
pages	30
milliseconds	37492
mean per set	3749.20
mean per page	124.97
timer lag	5508.00
timer lag per page	18.36

iterations	10
pages	30
milliseconds	38170
mean per set	3817.00
mean per page	127.23
timer lag	5848.00
timer lag per page	19.49


post-change, no extension:
iterations	10
pages	30
milliseconds	33677
mean per set	3367.70
mean per page	112.26
timer lag	4371.00
timer lag per page	14.57

iterations	10
pages	30
milliseconds	32680
mean per set	3268.00
mean per page	108.93
timer lag	4126.00
timer lag per page	13.75


post-change, oplop_declarativeContent:

iterations	10
pages	30
milliseconds	36016
mean per set	3601.60
mean per page	120.05
timer lag	5067.00
timer lag per page	16.89

iterations	10
pages	30
milliseconds	35774
mean per set	3577.40
mean per page	119.25
timer lag	5147.00
timer lag per page	17.16

post-change, oplop_declarativeContent_div:

iterations	10
pages	30
milliseconds	35004
mean per set	3500.40
mean per page	116.68
timer lag	5612.00
timer lag per page	18.71

iterations	10
pages	30
milliseconds	35235
mean per set	3523.50
mean per page	117.45
timer lag	5185.00
timer lag per page	17.28

[abarth-chromium, 2013-07-17 05:41:14]

From a rough number crunch, the change doesn't have an effect without
extensions, but if an extension watches input[type='password'], page load time
is slowed down by 8%.  How does that scale when watching multiple things?  For
example, if another extension watches form[action='POST'], is that another 8% or
does a single charge of 8% cover all watching?

As another data point, oplop_contentScript seems to have no page load
performance penalty and pre-change oplop_declarativeContent appears to have a
worse PLT penalty.  Is pre-change oplop_declarativeContent using a
MutationObserver?

[Jeffrey Yasskin, 2013-07-17 06:48:00]

On Tue, Jul 16, 2013 at 10:41 PM,  <abarth@chromium.org> wrote:
> From a rough number crunch, the change doesn't have an effect without
> extensions, but if an extension watches input[type='password'], page load
> time
> is slowed down by 8%.  How does that scale when watching multiple things?
> For
> example, if another extension watches form[action='POST'], is that another
> 8% or
> does a single charge of 8% cover all watching?

oplop_declarativeContent_div watches both "input[type='password']" and
"div", and actually seems a bit faster than oplop_declarativeContent,
so it seems like the single charge covers all watching. I don't have
high confidence in that though.

> As another data point, oplop_contentScript seems to have no page load
> performance penalty and pre-change oplop_declarativeContent appears to have
> a
> worse PLT penalty.  Is pre-change oplop_declarativeContent using a
> MutationObserver?

Yes, pre-change oplop_declarativeContent is using my proof-of-concept
implementation, which involves a MutationObserver. I'm not sure why
the contentScript has no penalty, since it's running extra JS in a new
isolated world. Maybe the default run_at of document_idle is saving
the content script here or causing the content script to run after the
page cycler takes its time measurement.

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

[esprehn, 2013-07-17 07:14:31]

You're recreating the string and doing hash table lookups for every match this
is going to make this look nasty on a benchmark. I think we can do better if we
refactor it out or try it at a different phase (like recalcStyle).

Btw we don't selector match elements that are descendants of display none things
where the MutationObserver + querySelectorAll method would work for that. So by
going this route you make this new feature only able to match things that are
not inside a display: none thing.

https://codereview.chromium.org/18371008/diff/24001/Source/core/css/DocumentR...
File Source/core/css/DocumentRuleSets.cpp (right):

https://codereview.chromium.org/18371008/diff/24001/Source/core/css/DocumentR...
Source/core/css/DocumentRuleSets.cpp:108: }
leave off braces

https://codereview.chromium.org/18371008/diff/24001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/18371008/diff/24001/Source/core/css/resolver/...
Source/core/css/resolver/StyleBuilderCustom.cpp:1718:
state.rule()->selectorList().selectorsText(),
This dynamically recreates the text of the selector on every invocation, so for
the "div" case you're creating thousands of strings.

https://codereview.chromium.org/18371008/diff/24001/Source/core/css/resolver/...
Source/core/css/resolver/StyleBuilderCustom.cpp:1719:
CSSSelectorWatch::from(state.element()->document()));
The hash table lookup hiding in here is going to hurt your perf numbers

https://codereview.chromium.org/18371008/diff/24001/Source/core/dom/CSSSelect...
File Source/core/dom/CSSSelectorWatch.cpp (right):

https://codereview.chromium.org/18371008/diff/24001/Source/core/dom/CSSSelect...
Source/core/dom/CSSSelectorWatch.cpp:58: }
We often just leave this in the header like  { }

https://codereview.chromium.org/18371008/diff/24001/Source/core/dom/CSSSelect...
Source/core/dom/CSSSelectorWatch.cpp:77: copyToVector(m_removedSelectors,
removedSelectors);
Use swap() instead.

https://codereview.chromium.org/18371008/diff/24001/Source/core/dom/CSSSelect...
Source/core/dom/CSSSelectorWatch.cpp:81: m_removedSelectors.clear();
remove once you swap()

https://codereview.chromium.org/18371008/diff/24001/Source/core/dom/CSSSelect...
Source/core/dom/CSSSelectorWatch.cpp:135: parser.parseSelector(selectors[i],
selectorList);
What about invalid selectors?

https://codereview.chromium.org/18371008/diff/24001/Source/core/rendering/sty...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/18371008/diff/24001/Source/core/rendering/sty...
Source/core/rendering/style/RenderStyle.cpp:708: void
RenderStyle::removeCallbackSelector(const String& selector)
Nothing ever calls this, can you just remove it?

[abarth-chromium, 2013-07-17 17:45:24]

I'm curious when you're calling watchCSSSelectors (maybe it should just be
called watchSelectors btw---there is no such thing as a non-CSS selector).

Have you considered calling watchSelector at document_idle?  That might reduce
the performance overhead during page load.  You might want to call
querySelectorAll before watchSelector if you take this approach so that you get
all the elements that are already in the document.

[Jeffrey Yasskin, 2013-08-02 01:34:06]

I think not-matching things inside display:none is the right behavior for this
API. I'll need to document that, of course.

Now that I've spent way too long getting solid metrics with comprehensible error
bars for this, I'll go move the tracking into recalcStyle like esprehn suggested
and see how that affects things.

I got better metrics through
http://www.chromium.org/developers/testing/page-cyclers/analyzing-page-cycler...
and the new run_measurement-based instructions at
https://sites.google.com/a/chromium.org/dev/developers/testing/page-cyclers:
I've averaged the percentage slowdown on the 25 sites. With no extension, this
patch speeds things up by .7%. Given this patch, adding the
oplop_declarativeContent extension slows pages down by 2.2%. On top of that,
adding a 'div' watch slows it down by .8% more.
content-script->declarativeContent is a 1.0% slowdown. For comparison, 3
identical runs varied by 1.1%.

The `run_measurement memory` measurement is not very usable at the moment, since
it appears to only count the page's renderer and ignore any extension renderers.
However, looking at RendererUsed, the difference ignoring the extension process
appears to be about .4% less memory use with oplop_declarativeContent and about
2.6% more memory with oplop_declarativeContent_div, both comparing against
no-extension. For comparison, 3 identical runs varied by .7%.

I'm calling watchSelectors from DidCreateDocumentElement
(https://codereview.chromium.org/19045002/diff/1/chrome/renderer/extensions/co...),
called from RenderViewObserver::DidCreateDocumentElement, called from
WebFrameClient::didCreateDocumentElement. Maybe I should inject it earlier or
through the WebFrame, so it doesn't cause a style recalc? documentIdle is 200ms
after DOMContentLoaded or onload, whichever comes first
(https://code.google.com/p/chromium/codesearch/#chromium/src/extensions/common...),
so I could imitate that or just run at onload.

https://codereview.chromium.org/18371008/diff/24001/Source/core/css/DocumentR...
File Source/core/css/DocumentRuleSets.cpp (right):

https://codereview.chromium.org/18371008/diff/24001/Source/core/css/DocumentR...
Source/core/css/DocumentRuleSets.cpp:108: }
On 2013/07/17 07:14:32, esprehn wrote:
> leave off braces

Done.

https://codereview.chromium.org/18371008/diff/24001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/18371008/diff/24001/Source/core/css/resolver/...
Source/core/css/resolver/StyleBuilderCustom.cpp:1718:
state.rule()->selectorList().selectorsText(),
On 2013/07/17 07:14:32, esprehn wrote:
> This dynamically recreates the text of the selector on every invocation, so
for
> the "div" case you're creating thousands of strings.

Right, but the "div" case didn't show up as significantly slower than the
"input[type=password]" case.

https://codereview.chromium.org/18371008/diff/24001/Source/core/css/resolver/...
Source/core/css/resolver/StyleBuilderCustom.cpp:1719:
CSSSelectorWatch::from(state.element()->document()));
On 2013/07/17 07:14:32, esprehn wrote:
> The hash table lookup hiding in here is going to hurt your perf numbers

A hash table lookup is generally cheaper than an allocation. I can try moving
this back to a field on Document to see if that improves anything (after trying
a couple other things), but I'd be surprised if it did.

https://codereview.chromium.org/18371008/diff/24001/Source/core/dom/CSSSelect...
File Source/core/dom/CSSSelectorWatch.cpp (right):

https://codereview.chromium.org/18371008/diff/24001/Source/core/dom/CSSSelect...
Source/core/dom/CSSSelectorWatch.cpp:58: }
On 2013/07/17 07:14:32, esprehn wrote:
> We often just leave this in the header like  { }

'k.

https://codereview.chromium.org/18371008/diff/24001/Source/core/dom/CSSSelect...
Source/core/dom/CSSSelectorWatch.cpp:77: copyToVector(m_removedSelectors,
removedSelectors);
On 2013/07/17 07:14:32, esprehn wrote:
> Use swap() instead.

m_removedSelectors is a HashSet, while removedSelectors is a Vector, making swap
not work.

https://codereview.chromium.org/18371008/diff/24001/Source/core/dom/CSSSelect...
Source/core/dom/CSSSelectorWatch.cpp:135: parser.parseSelector(selectors[i],
selectorList);
On 2013/07/17 07:14:32, esprehn wrote:
> What about invalid selectors?

Fixed this by omitting invalid selectors, and testing that. Is
!selectorList.isValid() the right way to detect this?

https://codereview.chromium.org/18371008/diff/24001/Source/core/rendering/sty...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/18371008/diff/24001/Source/core/rendering/sty...
Source/core/rendering/style/RenderStyle.cpp:708: void
RenderStyle::removeCallbackSelector(const String& selector)
On 2013/07/17 07:14:32, esprehn wrote:
> Nothing ever calls this, can you just remove it?

Done.

[Jeffrey Yasskin, 2013-08-13 00:29:28]

I haven't yet benchmarked this version, but I think this includes all the
changes Elliott asked for. Adam, did you want
s/watchCSSSelectors/watchSelectors/?

https://codereview.chromium.org/18371008/diff/10001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/18371008/diff/10001/Source/core/css/resolver/...
Source/core/css/resolver/StyleBuilderCustom.cpp:792: if (primitiveValue) {
On 2013/07/13 01:43:01, esprehn wrote:
> if (primitiveValue && primitiveValue->getValueID())
>   state.style()->addCallbackSelector( ... );
> 
> Since you only allow one value would be simpler

Done.

https://codereview.chromium.org/18371008/diff/10001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolverState.h (right):

https://codereview.chromium.org/18371008/diff/10001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolverState.h:131: const StyleRule* rule() const
{ return m_rule; }
On 2013/07/13 01:43:01, esprehn wrote:
> currentRule() ?

Done.

https://codereview.chromium.org/18371008/diff/10001/Source/core/rendering/sty...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/18371008/diff/10001/Source/core/rendering/sty...
Source/core/rendering/style/RenderStyle.cpp:704:
document->addSelectorMatch(selector, data);
On 2013/07/13 01:43:01, esprehn wrote:
> Please do this call outside this function. Return a boolean to signal a
change.

This went away with the move to Element.

[esprehn, 2013-08-13 00:34:54]

I need to take some more time to review the whole thing, but what you have does
not appear to be dynamic. You only catch things in ::attach and ::detach, so if
an element changes to start matching one of these rules you'll miss it.

ex. if your selector was [attr=value] and someone does setAttribute("attr",
"value") after the element has already been attached you won't add the selector
match.

[Jeffrey Yasskin, 2013-08-13 00:43:40]

On Mon, Aug 12, 2013 at 5:34 PM,  <esprehn@chromium.org> wrote:
> I need to take some more time to review the whole thing, but what you have
> does
> not appear to be dynamic. You only catch things in ::attach and ::detach, so
> if
> an element changes to start matching one of these rules you'll miss it.
>
> ex. if your selector was [attr=value] and someone does setAttribute("attr",
> "value") after the element has already been attached you won't add the
> selector
> match.

I believe the SelectorWatchUpdater in Element::recalcStyle handles
this:
https://codereview.chromium.org/18371008/diff/66001/Source/core/dom/Element.c....
In any case, it's a good test, so I've added it to WebFrameTest.cpp.
Let me know if I missed anything.

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

[Jeffrey Yasskin, 2013-08-21 23:32:48]

Ok, I've got some benchmarks for the most recent patch. These were run with the
'performance' frequency governor enabled, in addition to other stability things
run_measurements provides.

before-patch->after-patch, no extensions: 1.4% faster (but an earlier benchmark
with the default 'ondemand' governor gave a 1% slowdown, so don't think this is
precise.)
before-patch, no extensions->oplop_contentScript: .26% faster
before-patch, no extensions->oplop_declarativeContent: 4.2% slower
after-patch, no extensions->oplop_contentScript: .82% slower
after-patch, no extensions->oplop_declarativeContent: 2.6% slower
after-patch, oplop_declarativeContent->oplop_declarativeContent_div: 1.2% slower

I tried setting the content script's run_at to document_end, and that slowed it
down by 1.4% before the patch and .9% after the patch (there's no reason for
those to be different), so delaying the watchSelectors call is something to
explore on the Chrome side once this patch is landed.

The memory_top25 measurement still doesn't show a visual memory usage difference
between no-content-script and a content script, or between no-extension and
watching divs, so I'm not sure how to measure the memory use well.

How's the Element-based code look?

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

> The memory_top25 measurement still doesn't show a visual memory usage
difference
> between no-content-script and a content script, or between no-extension and
> watching divs, so I'm not sure how to measure the memory use well.

I looked at the task manager's "private memory" column for a gmail tab. For each
state, I closed the gmail tab, opened a new one, navigated to gmail, switched
back to the chrome://extensions tab, and watched for the lowest memory use it
settled down to:
With no extension, 69348K
With oplop_contentScript, 71068K
With oplop_declarativeContent, 70192K
With oplop_declarativeContent_div, 70656K

[Jeffrey Yasskin, 2013-08-27 21:37:43]

On 2013/08/21 23:32:48, Jeffrey Yasskin wrote:
> Ok, I've got some benchmarks for the most recent patch. These were run with
the
> 'performance' frequency governor enabled, in addition to other stability
things
> run_measurements provides.
> 
> before-patch, no extensions->oplop_declarativeContent: 4.2% slower
> after-patch, no extensions->oplop_declarativeContent: 2.6% slower

https://codereview.chromium.org/22903023/ seems to have helped the after-patch,
no extension->declarativeContent difference a lot. On three runs, I got a .02%
speedup, a .14% slowdown, and a .94% slowdown.

[esprehn, 2013-09-04 06:08:28]

Lots of comments, but architecturally I think this is going in the right
direction. The only major thing that looks wrong is that you call Add and Remove
as recalcStyle walks down the tree even if we know the style is identical for
that node which is not correct.

I think you want recalcOwnStyle and to get rid of the RAII thing and only mess
with Add/Remove when the style is going to be recalced. You might even put the
logic in RenderObject::styleDidChange ?

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/CSSSelect...
File Source/core/dom/CSSSelectorWatch.cpp (right):

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/CSSSelect...
Source/core/dom/CSSSelectorWatch.cpp:85: Vector<String> addedSelectors,
removedSelectors;
We usually do one declaration per line.

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/CSSSelect...
Source/core/dom/CSSSelectorWatch.cpp:167:
m_document->styleResolverChanged(RecalcStyleDeferred);
You should really be calling m_document->modifiedStyleSheet() and modifying your
own StyleSheet instance instead of hooking into the selector like you're doing.

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/CSSSelect...
File Source/core/dom/CSSSelectorWatch.h (right):

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/CSSSelect...
Source/core/dom/CSSSelectorWatch.h:66: // m_callbackSelectorChange.
I'd leave off this comment. You don't need to describe the impl. :)

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/CSSSelect...
Source/core/dom/CSSSelectorWatch.h:69: Document* const m_document;
We almost never do const like this... I suppose it's okay though.

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/Element.c...
Source/core/dom/Element.cpp:1457: class SelectorWatchUpdater {
Don't put classes inside methods. This needs to be at the top of the file or in
its own file.

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/Element.c...
Source/core/dom/Element.cpp:1462: m_element->updateCallbackSelectors(Remove);
This is not right, why are you removing and adding on every recalcStyle? You do
this on every element as it walks down the tree, even if the style is identical.

I think you want recalcOwnStyle() in here and you want to only mess around with
this if the style changed?

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/Element.c...
Source/core/dom/Element.cpp:2463: return;
Don't call RenderStyle repeatedly, you want to call it once and save the value.

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/Element.c...
Source/core/dom/Element.cpp:2465: if (callbackSelectors.isEmpty())
The check should be outside the function call since it'll be inline simple
checks unless you have the "slow stuff"


RenderStyle* style = renderStyle()
if (style && style->hasCallbackSelectors())
  doSomethingHere();

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/Element.c...
Source/core/dom/Element.cpp:2476: break;
These should be two separate methods and get rid of the enum and switch.

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/Element.h...
Source/core/dom/Element.h:754: Remove,
SelectorAdded, SelectorRemoved

and I'd suggest renaming the enum something that's not an "Or" of the values.

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/ElementRa...
File Source/core/dom/ElementRareData.h (right):

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/ElementRa...
Source/core/dom/ElementRareData.h:173: unsigned
m_renderStyleWatchedSelectorsRepresentedInSelectorWatch : 1;
There's no free bitfields on RenderStyle?

https://codereview.chromium.org/18371008/diff/78001/Source/core/page/RuntimeC...
File Source/core/page/RuntimeCSSEnabled.cpp (right):

https://codereview.chromium.org/18371008/diff/78001/Source/core/page/RuntimeC...
Source/core/page/RuntimeCSSEnabled.cpp:116:
RuntimeCSSEnabled::setCSSPropertyEnabled(CSSPropertyInternalCallback, false);
This needs a comment why you pass false and nothing else does.

https://codereview.chromium.org/18371008/diff/78001/Source/web/FrameLoaderCli...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/18371008/diff/78001/Source/web/FrameLoaderCli...
Source/web/FrameLoaderClientImpl.cpp:547: if (m_webFrame->client()) {
no braces and we'd probably write it as:

if (Client* client = m_webFrame->client())
  client->...();

https://codereview.chromium.org/18371008/diff/78001/Source/web/tests/WebFrame...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/18371008/diff/78001/Source/web/tests/WebFrame...
Source/web/tests/WebFrameTest.cpp:282: struct CSSCallbackWebFrameClient : public
WebFrameClient {
class

https://codereview.chromium.org/18371008/diff/78001/Source/web/tests/WebFrame...
Source/web/tests/WebFrameTest.cpp:284: virtual void didMatchCSS(WebFrame* frame,
const WebVector<WebString>& newlyMatchingSelectors, const WebVector<WebString>&
stoppedMatchingSelectors) OVERRIDE
I think we usually put long methods not in the class def.

https://codereview.chromium.org/18371008/diff/78001/public/web/WebFrameClient.h
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/18371008/diff/78001/public/web/WebFrameClient...
public/web/WebFrameClient.h:128: virtual void didMatchCSS(WebFrame*, const
WebVector<WebString>& newlyMatchingSelectors, const WebVector<WebString>&
stoppedMatchingSelectors) { }
Passing the frame into didMatchCSS is really weird, WebDocument already knows
it's frame, you shouldn't pass one since that just means you can potentially get
the wrong one.

[Jeffrey Yasskin, 2013-09-12 22:09:58]

I think I got all your comments.

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/CSSSelect...
File Source/core/dom/CSSSelectorWatch.cpp (right):

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/CSSSelect...
Source/core/dom/CSSSelectorWatch.cpp:85: Vector<String> addedSelectors,
removedSelectors;
On 2013/09/04 06:08:28, esprehn wrote:
> We usually do one declaration per line.

Ah, sure.

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/CSSSelect...
Source/core/dom/CSSSelectorWatch.cpp:167:
m_document->styleResolverChanged(RecalcStyleDeferred);
On 2013/09/04 06:08:28, esprehn wrote:
> You should really be calling m_document->modifiedStyleSheet() and modifying
your
> own StyleSheet instance instead of hooking into the selector like you're
doing.

It turned out to be complicated to create an actual StyleSheet instance. After
an in-person conversation, I added a Document::changedSelectorWatch() so that
this can be cleaned up after the main CL is in.

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/CSSSelect...
File Source/core/dom/CSSSelectorWatch.h (right):

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/CSSSelect...
Source/core/dom/CSSSelectorWatch.h:66: // m_callbackSelectorChange.
On 2013/09/04 06:08:28, esprehn wrote:
> I'd leave off this comment. You don't need to describe the impl. :)

Done.

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/CSSSelect...
Source/core/dom/CSSSelectorWatch.h:69: Document* const m_document;
On 2013/09/04 06:08:28, esprehn wrote:
> We almost never do const like this... I suppose it's okay though.

It looks like in some places you're migrating to Document& instead of Document*
for things you know aren't NULL. I'll use that here too.

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/Element.c...
Source/core/dom/Element.cpp:1457: class SelectorWatchUpdater {
On 2013/09/04 06:08:28, esprehn wrote:
> Don't put classes inside methods. This needs to be at the top of the file or
in
> its own file.

It's no longer necessary. :)

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/Element.c...
Source/core/dom/Element.cpp:1462: m_element->updateCallbackSelectors(Remove);
On 2013/09/04 06:08:28, esprehn wrote:
> This is not right, why are you removing and adding on every recalcStyle? You
do
> this on every element as it walks down the tree, even if the style is
identical.
> 
> I think you want recalcOwnStyle() in here and you want to only mess around
with
> this if the style changed?

Yep, using recalcOwnStyle() cleaned this up a lot.

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/Element.c...
Source/core/dom/Element.cpp:2463: return;
On 2013/09/04 06:08:28, esprehn wrote:
> Don't call RenderStyle repeatedly, you want to call it once and save the
value.

Obsolete. :)

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/Element.c...
Source/core/dom/Element.cpp:2465: if (callbackSelectors.isEmpty())
On 2013/09/04 06:08:28, esprehn wrote:
> The check should be outside the function call since it'll be inline simple
> checks unless you have the "slow stuff"
> 
> 
> RenderStyle* style = renderStyle()
> if (style && style->hasCallbackSelectors())
>   doSomethingHere();

Done.

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/Element.c...
Source/core/dom/Element.cpp:2476: break;
On 2013/09/04 06:08:28, esprehn wrote:
> These should be two separate methods and get rid of the enum and switch.

Refactored away.

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/Element.h...
Source/core/dom/Element.h:754: Remove,
On 2013/09/04 06:08:28, esprehn wrote:
> SelectorAdded, SelectorRemoved
> 
> and I'd suggest renaming the enum something that's not an "Or" of the values.

The enum's gone now.

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/ElementRa...
File Source/core/dom/ElementRareData.h (right):

https://codereview.chromium.org/18371008/diff/78001/Source/core/dom/ElementRa...
Source/core/dom/ElementRareData.h:173: unsigned
m_renderStyleWatchedSelectorsRepresentedInSelectorWatch : 1;
On 2013/09/04 06:08:28, esprehn wrote:
> There's no free bitfields on RenderStyle?

This was tracking whether a RenderStyle had been propagated to *this* element,
which wasn't appropriate to put on RenderStyle. However, with the move to
recalcOwnStyle, it's now unnecessary and removed.

https://codereview.chromium.org/18371008/diff/78001/Source/core/page/RuntimeC...
File Source/core/page/RuntimeCSSEnabled.cpp (right):

https://codereview.chromium.org/18371008/diff/78001/Source/core/page/RuntimeC...
Source/core/page/RuntimeCSSEnabled.cpp:116:
RuntimeCSSEnabled::setCSSPropertyEnabled(CSSPropertyInternalCallback, false);
On 2013/09/04 06:08:28, esprehn wrote:
> This needs a comment why you pass false and nothing else does.

Done; see if you like the wording.

https://codereview.chromium.org/18371008/diff/78001/Source/web/FrameLoaderCli...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/18371008/diff/78001/Source/web/FrameLoaderCli...
Source/web/FrameLoaderClientImpl.cpp:547: if (m_webFrame->client()) {
On 2013/09/04 06:08:28, esprehn wrote:
> no braces and we'd probably write it as:
> 
> if (Client* client = m_webFrame->client())
>   client->...();

Done, but I haven't updated all of the other functions here to be consistent.

https://codereview.chromium.org/18371008/diff/78001/Source/web/tests/WebFrame...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/18371008/diff/78001/Source/web/tests/WebFrame...
Source/web/tests/WebFrameTest.cpp:282: struct CSSCallbackWebFrameClient : public
WebFrameClient {
On 2013/09/04 06:08:28, esprehn wrote:
> class

Done.

https://codereview.chromium.org/18371008/diff/78001/Source/web/tests/WebFrame...
Source/web/tests/WebFrameTest.cpp:284: virtual void didMatchCSS(WebFrame* frame,
const WebVector<WebString>& newlyMatchingSelectors, const WebVector<WebString>&
stoppedMatchingSelectors) OVERRIDE
On 2013/09/04 06:08:28, esprehn wrote:
> I think we usually put long methods not in the class def.

Even in .cpp files? Ok.

https://codereview.chromium.org/18371008/diff/78001/public/web/WebFrameClient.h
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/18371008/diff/78001/public/web/WebFrameClient...
public/web/WebFrameClient.h:128: virtual void didMatchCSS(WebFrame*, const
WebVector<WebString>& newlyMatchingSelectors, const WebVector<WebString>&
stoppedMatchingSelectors) { }
On 2013/09/04 06:08:28, esprehn wrote:
> Passing the frame into didMatchCSS is really weird, WebDocument already knows
> it's frame, you shouldn't pass one since that just means you can potentially
get
> the wrong one.

There's no WebDocument here though, and the WebFrameClient might be a
RenderViewImpl, which is the client for more than one WebFrame.  I need some way
in this callback to figure out which one.

[esprehn, 2013-09-30 17:28:07]

LGTM, awesome.

[abarth-chromium, 2013-09-30 19:17:36]

Given that Elliot is happy, public/web LGTM

[commit-bot: I haz the power, 2013-09-30 20:01:01]

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

[commit-bot: I haz the power, 2013-10-01 03:06:40]

Change committed as 158582