Bug 110524 – Add a way to watch pages for elements matching a CSS selector

• Component: CSS
• Status: RESOLVED WONTFIX

[Jeffrey Yasskin]

Created attachment 189626 [details]
Preliminary patch adding a -webkit-callback CSS property

In order to implement http://developer.chrome.com/trunk/extensions/declarativeContent.html efficiently, I'd like to add a way to cause certain CSS selectors to fire a callback in the WebKit embedder when they match an element on the page.


The attached patch is a possible direction for this: I've added a -webkit-callback CSS property with values of 'none' and '-webkit-presence'. If a rule with -webkit-callback: -webkit-presence matches any element in a frame, WebCore::FrameLoaderClient::selectorMatchChanged(selector, true), where 'selector' is the result of SelectorList::selectorsText() on the matching rule. Unlike most (all?) other CSS properties, if multiple rules match with this property, all of their selectors get passed back to the client.  The WebKit/chromium embedding then batches these within a microtask to present a summary of the changes out to Chromium itself.

If the DOM changes so that a particular rule doesn't match any element on the page anymore, selectorMatchChanged(selector, false) is called.

Setting '-webkit-callback: none' in a rule unregisters interest in the particular selector used for that rule, but not other selectors. 'initial' and 'inherit' have no effect.

I chose "-webkit-presence" because this callback is watching for the presence of a type of element on the page. There's an obvious extension to watch for each element's creation or destruction, but handling that would be more complex and would require me to make this sort of RenderStyle unshareable. Since I don't need to watch particular elements for my use case, I didn't do this.  I also don't see any need to expose these callbacks to Javascript.

A preliminary use of this API is at https://codereview.chromium.org/12326052/.


This patch still lacks tests, and probably does many things in totally the wrong way. I'd appreciate any suggestions for how to do it better, even if that means completely redoing the approach.  One other interface that might work would be to extend mutation observers to filter by css selector. I didn't pick this route because it would seem to require a web platform change, and I don't immediately see a way to do it without notifying on every element change, which, as I said above, is heavier-weight than I actually need.

Let me know what you think. Thanks.

[Adam Klein]

Comment on attachment 189626 [details]
Preliminary patch adding a -webkit-callback CSS property

View in context: https://bugs.webkit.org/attachment.cgi?id=189626&action=review

> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1240
> +class FrameLoaderClientImpl::SelectorMatchAggregator : public WebThread::TaskObserver {

Per our offline discussion, since this doesn't need to run ASAP, it should just be posted as a regular task to the main thread.

[Tony Chang]

Comment on attachment 189626 [details]
Preliminary patch adding a -webkit-callback CSS property

View in context: https://bugs.webkit.org/attachment.cgi?id=189626&action=review

High level questions:
- How hard do we want to work at hiding -webkit-callback from the web?  Should it show up in getComputedStyle or what should happen if the webpage decides to add it themselves (should that trigger the callback)?
- Should we fire a callback if a node that matches is set to display:none?  We don't construct a RenderObject or RenderStyle for nodes that are display:none.

There are a bunch of style violations.  You'll see them when running Tools/Scripts/check-webkit-style.

> Source/WebCore/dom/Document.cpp:2016
> +void Document::removeSelectorMatch(String selector) {

I think if there's a single match and if you move it in the document, we will destroy the RenderStyle and re-create it. That would fire spurious events (maybe that's OK?).

> Source/WebCore/rendering/style/RenderStyle.cpp:172
> +    if (!rareNonInheritedData->m_callbackSelectors.isEmpty()) {

I wonder if this extra check would show up in one of our existing page cyclers.

[Jeffrey Yasskin]

Created attachment 189861 [details]
Preliminary patch adding a -webkit-callback CSS property

This patch implements Adam's suggestion to just post a task to the main thread, and fixes the style problems.

- I don't have any personal preference about how much to hide -webkit-callback from the web. I can look into hiding it from getComputedStyle; not sure how to prevent web pages from setting it. At the moment, if a web page uses it itself, that does trigger (or suppress) the callback, so embedders need to handle this, but it's straightforward to avoid losing callbacks by putting embedder-relevant uses in user- rather than author- stylesheets. (I'll test this.)
- For the declarativeContent use case, I think it's actually better to suppress callbacks for display:none nodes, although clearly I'll have to document that.

- Re "spurious events", because I'm aggregating these across a task, destroying the RenderStyle and recreating it will cause an extra task to run, but won't send an extra callback to the FrameLoaderClient.
- I'll check with you how to test this against a page cycler.

I still need to write tests and a benchmark proving this helps my use case.

[Tony Chang]

(In reply to comment #3)
> Created an attachment (id=189861) [details]
> - Re "spurious events", because I'm aggregating these across a task, destroying the RenderStyle and recreating it will cause an extra task to run, but won't send an extra callback to the FrameLoaderClient.

Oh, I was more concerned about moving a node causing you to fire the callback(false) even though nothing was removed.

I'll do a more thorough style review on Monday.

[Tony Chang]

Comment on attachment 189861 [details]
Preliminary patch adding a -webkit-callback CSS property

View in context: https://bugs.webkit.org/attachment.cgi?id=189861&action=review

> Source/Platform/chromium/public/WebVector.h:115
> +    void resize(size_t newSize)

I don't see any callers of this, so I assume it's used on the Chromium side.

> Source/Platform/chromium/public/WebVector.h:118
> +        for (size_t i = 0, e = std::min(m_size, newSize); i < e; ++i)

Nit: Please use a longer variable name than |e|.  Maybe sizeToCopy?

> Source/WebCore/css/StyleResolver.cpp:3401
> +                state.style()->removeCallbackSelector(
> +                    rule->selectorList().selectorsText());

It might be possible to move removeCallbackSelector/addCallbackSelector to ElementRareData.  Instead of hooking a destructor, you could hook RenderObject::styleDidChange to watch for style removal.  You would still need to add a bit to StyleRareNonInheritedData.cpp, but it would just have a simple setter/getter.  It would also avoid having to add a document pointer.

> Source/WebCore/dom/Document.cpp:2011
> +        OwnPtr<pair<HashSet<String>, HashSet<String> > > callbackSelectorChange =

Maybe use a typedef?

> Source/WebCore/dom/Document.h:1190
> +    void addSelectorMatch(String);
> +    void removeSelectorMatch(String);

Nit: I would name these params to make it clear what the parameter is.

> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1242
> +    if (m_webFrame->client()) {

I'm not sure if you need to check client() for NULL here.  Maybe someone with more knowledge of FrameLoader can comment.

[Jeffrey Yasskin]

Created attachment 192328 [details]
Patch adding a -webkit-callback CSS property

* WebVector::resize() is used by WTF::copyToVector in FrameLoaderClientImpl::selectorMatchChanged().

* I didn't try moving {add,remove}CallbackSelector to ElementRareData because I believe it wouldn't save any storage: since RenderStyles can be shared between elements, I'd have to save the selector list from StyleResolver into the RenderStyle anyway in order to propagate it to the Elements.

* Re checking client() for NULL, everything in FrameLoaderClientImpl does that, so I think it's needed.

* Other style comments are addressed, I think.

* I ran this through a couple pagecyclers and saw a fairly consistent ~3% slowdown. Moving the destruction behavior from ~RenderStyle to ~StyleRareNonInheritedData and moving the current StyleRule* from an argument to applyProperty into m_state resolved that slowdown.

* I've added a test, too.

Is there anything else you'd like me to do before this is ready for review?

[Tony Chang]

Comment on attachment 192328 [details]
Patch adding a -webkit-callback CSS property

View in context: https://bugs.webkit.org/attachment.cgi?id=192328&action=review

We'll want to add a compile time flag for this feature and you'll need to generate a ChangeLog.  I think we can upload for review after that.

> Source/WebCore/dom/Document.h:1176
> +    void addSelectorMatch(String selector);
> +    void removeSelectorMatch(String selector);

Nit: const String& to avoid some refcount churn.

> Source/WebCore/rendering/style/RenderStyle.h:1266
> +    void addCallbackSelector(String selector, Document*);
> +    void removeCallbackSelector(String selector);

Nit: const String&

> Source/WebKit/chromium/tests/WebFrameTest.cpp:298
> +    doc.insertUserStyleSheet("span{-webkit-callback:-webkit-presence !important}",

Is !important necessary in this test case?

[Jeffrey Yasskin]

Created attachment 192855 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Jeffrey Yasskin]

Created attachment 192857 [details]
Patch adding a -webkit-callback CSS property

Now with complete fixes for Tony's comments.

[Jeffrey Yasskin]

Comment on attachment 192328 [details]
Patch adding a -webkit-callback CSS property

View in context: https://bugs.webkit.org/attachment.cgi?id=192328&action=review

>> Source/WebCore/dom/Document.h:1176
>> +    void removeSelectorMatch(String selector);
> 
> Nit: const String& to avoid some refcount churn.

Done

>> Source/WebCore/rendering/style/RenderStyle.h:1266
>> +    void removeCallbackSelector(String selector);
> 
> Nit: const String&

Done

>> Source/WebKit/chromium/tests/WebFrameTest.cpp:298
>> +    doc.insertUserStyleSheet("span{-webkit-callback:-webkit-presence !important}",
> 
> Is !important necessary in this test case?

In this one it's not necessary, although it reflects what I think will be the common use case. I've removed it.

[Simon Fraser (smfr)]

Why does this have to be exposed via a new css property? Why isn't it just native code API (or some special JS bindings or something)?

[Darin Fisher (:fishd, Google)]

Comment on attachment 192857 [details]
Patch adding a -webkit-callback CSS property

View in context: https://bugs.webkit.org/attachment.cgi?id=192857&action=review

> Source/Platform/chromium/public/WebVector.h:115
> +    void resize(size_t newSize)

It was part of the design of WebVector that it is not resizable.  The idea was that people
should primarily be working with WTF::Vector when inside WebKit and std::vector when outside
of WebKit.  WebVector is just a go-between.

Also, this function does not appear to be used as part of this patch.  We should probably
discuss this change separately.

[Jeffrey Yasskin]

Comment on attachment 192857 [details]
Patch adding a -webkit-callback CSS property

@Simon: I would have loved to make this just a native code API, but I couldn't see how to tell the CSS matching system to execute some code when a particular selector matches without creating a property. Could you point out roughly how to do it?

View in context: https://bugs.webkit.org/attachment.cgi?id=192857&action=review
>> Source/Platform/chromium/public/WebVector.h:115
>> +    void resize(size_t newSize)
> 
> It was part of the design of WebVector that it is not resizable.  The idea was that people
> should primarily be working with WTF::Vector when inside WebKit and std::vector when outside
> of WebKit.  WebVector is just a go-between.
> 
> Also, this function does not appear to be used as part of this patch.  We should probably
> discuss this change separately.

This function is called by WTF::copyToVector (http://trac.webkit.org/browser/trunk/Source/WTF/wtf/HashSet.h#L247) in FrameLoaderClientImpl::selectorMatchChanged. Is there some other way you normally transfer WTF::HashSets into WebVectors? Clearly I can just write out a copy, but that seems like a waste.

[Darin Fisher (:fishd, Google)]

(In reply to comment #14)
...
> This function is called by WTF::copyToVector (http://trac.webkit.org/browser/trunk/Source/WTF/wtf/HashSet.h#L247) in FrameLoaderClientImpl::selectorMatchChanged. Is there some other way you normally transfer WTF::HashSets into WebVectors? Clearly I can just write out a copy, but that seems like a waste.

Ah.  I don't think this scenario comes up that often.  I couldn't find any examples while digging through the code.  In most cases where we want a WebVector, we start out with a WTF::Vector.

I'd be tempted to just create an intermediate WTF::Vector in this case.  While that is a bit costly in the case where resize() is not needed, I suspect it won't matter in practice.

[Simon Fraser (smfr)]

(In reply to comment #14)
> (From update of attachment 192857 [details])
> @Simon: I would have loved to make this just a native code API, but I couldn't see how to tell the CSS matching system to execute some code when a particular selector matches without creating a property. Could you point out roughly how to do it?

When a selector matches what? Anything? Does it matter if there are any rules in the selector?

Presumably you could get StyleResolver to track these magic selectors, but I'm not quite sure what the goal is here.

[Simon Fraser (smfr)]

Comment on attachment 192857 [details]
Patch adding a -webkit-callback CSS property

r- under we have a better understanding of whether this new property is necessary, and why it won't leak onto the open web.

[Dean Jackson]

The purpose of this is not clear to me either. I read the linked document, but I'm not sure if that's supposed to explain it or not.

It is risky to expose a feature to the web. If this is something to do with extensions, why not make it a JS API within your extension framework? If you want a callback to be invoked whenever something happens, then the way you do that on the Web is via Javascript, not CSS. If this has no use on the open Web, then it should not be exposed there. If it does have a use on the open Web, then it should be proposed to the W3C/WHATWG.

[Dean Jackson]

Maybe this should be discussed on webkit-dev?

[Dimitri Glazkov (Google)]

This may not need to be a WebExposed feature.

I could be wrong, but maybe we could side-load a vector RuleDatas that passed to collectMatchingRules with some s3kr3t RuleData (maybe two, one for "on" and one for "off" states) that contains the watched selector, then reuse the rest of your plumbing and celebrate by continually pumping fists in the air -- modulo possible performance or memory hits, increased code complexity, yada-yada...

[Jeffrey Yasskin]

@Dean, This is exposed to extensions via a JS API, not by having them insert CSS rules, for the reason you said. My difficulty was that I couldn't find a C++ API to use to tell WebCore to watch for the presence or absence of elements matching the relevant selectors.

I'll try Dmitri's suggestion of adding an ElementRuleCollector::matchCallbackSelectors that pulls the watched selectors off of the Document, and returns matched ones through the MatchResult.

[Tony Chang]

Trying to make this a JS only feature seems worth exploring.

FWIW, I think the current patch is only web exposed in that it will show up in getComputedStyle and will parse in an @supports condition.  I bet we could fix those so the property isn't exposed at all (i.e., would behave the same as adding a CSS property that we don't support like a -moz prefixed property).

[Adam Barth]

Comment on attachment 192857 [details]
Patch adding a -webkit-callback CSS property

View in context: https://bugs.webkit.org/attachment.cgi?id=192857&action=review

> Source/WebCore/dom/Document.cpp:2033
> +class Document::ReportCallbackSelectorChange : public Task {
> +public:
> +    virtual void performTask(ScriptExecutionContext* context)

You can just use wtf/Functional.h to create a closure rather than making yet another task object.  Document has a weak pointer factory if you need to bind to a weak ptr.

> Source/WebCore/dom/Document.cpp:2057
> +        postTask(adoptPtr(new ReportCallbackSelectorChange));

Given that you're on the main thread, it might be easier to just use a Timer though.

> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:160
> +    Document* m_document;

This looks dangerous.  What ensures that this document isn't used after free?

> Source/WebKit/chromium/public/WebFrameClient.h:129
> +    virtual void cssMatches(WebFrame*, const WebVector<WebString>& newlyMatchingSelectors, const WebVector<WebString>& stoppedMatchingSelectors) { }

cssMatches --> we prefer names like didMatchFoo

> Source/WebKit/chromium/tests/WebFrameTest.cpp:223
> +static std::string AsString(const WebString& utf16Input)

Should we add this sort of method to WebString?  I'm surprised it doesn't already have a way to convert to a std::string.