Content extensions should apply css selectors

[Alex Christensen]

Here is a proof of concept.

[Alex Christensen]

Created attachment 248472 [details]
Patch

[Alex Christensen]

Created attachment 248473 [details]
Patch

[Darin Adler]

Comment on attachment 248473 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:498
> +    String out;

This needs to use StringBuilder, otherwise it will have terrible performance. Every single call to append allocates a new StringImpl!

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:500
> +        unsigned c = in[i];

Type should be UChar, not unsigned. It’s not so efficient to call in.length() every time through. Maybe this should use the codeUnits() or codePoints() functions and a modern for loop.

But really this needs to handle surrogate pairs, so it needs to go one code point at a time, not just one character at a time. Unless there is some guarantee that the characters are never going to be in the U+10000-U+10FFFF range.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:501
> +        if (isASCII(c))

What about characters like backslash and space, and control characters? Is there some guarantee we won’t have any of those in the input string?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:511
> +        }

This is not properly indented.

[Alex Christensen]

Comment on attachment 248473 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        TERRIBLE HACK.  GET READY TO r-

I'd like to draw attention to this.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:495
> +// Inverse of CSSParser::parseEscape, this is probably written better somewhere else.

This should be in CSSOMUtils.cpp, but I'm pretty sure this is not actually necessary any more.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:545
> +    String css;

This should be a StringBuilder, too, if this is the way we are doing this.  I think there's probably a better way, though, other than generating and parsing css.  What is this way, though?

[Darin Adler]

Comment on attachment 248473 [details]
Patch

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

r- as requested ;)

>> Source/WebCore/ChangeLog:11
>> +        TERRIBLE HACK.  GET READY TO r-
> 
> I'd like to draw attention to this.

LOL

>> Source/WebCore/loader/cache/CachedResourceLoader.cpp:545
>> +    String css;
> 
> This should be a StringBuilder, too, if this is the way we are doing this.  I think there's probably a better way, though, other than generating and parsing css.  What is this way, though?

Yes, it needs to be a StringBuilder.

I have no insight into a non-string alternative.

Once this is a StringBuilder, then we should write appendCSSEncoded instead of cssEncode to avoid creating an unneeded temporary string.

[Benjamin Poulain]

Comment on attachment 248473 [details]
Patch

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

I think it is a clever hack but I doubt that's the right way.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:496
> +static String cssEncode(const String& in)

We should do this at compile time. We should look for a way to use the parser without allocating a string for the input.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:559
> +        userContentController->addUserStyleSheet(DOMWrapperWorld::create(JSDOMWindow::commonVM()), std::make_unique<UserStyleSheet>(css, URL(URL(), "http://contentextensions.net/"), Vector<String>(), Vector<String>(), InjectInAllFrames, UserStyleUserLevel), InjectInExistingDocuments);

Hum, this is interesting.

I have the feeling we should the stylesheets directly on the document stylesheet collection instead of the UserContentController.

Say you have a page X, with an iframe Y. The URL of Y matches the rule 2, you would not want the style sheet of rule 2 to be applied to the main frame.

On the other hand, we want to share a common stylesheet if it is used by multiple frames. At the moment, I think we should have a cache rule->stylesheet for that.

[Brady Eidson]

I'm exploring better ways to do this.

[Brady Eidson]

Working on a patch that creates a single pre-parsed stylesheet for global rules that is cached in the ContentExtensionBackend, and used on each document.

[Brady Eidson]

Created attachment 249037 [details]
Part 1, v1

This adds a new action for applying the global stylesheet.

Actually applying it will come in part 2.

[WebKit Commit Bot]

Comment on attachment 249037 [details]
Part 1, v1

Rejecting attachment 249037 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
tWebKitAPI/Tests/mac/SubresourceErrorCrash.mm -o /Volumes/Data/EWS/WebKit/WebKitBuild/TestWebKitAPI.build/Release/TestWebKitAPILibrary.build/Objects-normal/x86_64/SubresourceErrorCrash.o

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/TestWebKitAPI.build/Release/TestWebKitAPILibrary.build/Objects-normal/x86_64/ContentExtensions.o Tests/WebCore/ContentExtensions.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.appspot.com/results/5976264649211904

[Brady Eidson]

Created attachment 249043 [details]
Patch 1, patch for landing.

[WebKit Commit Bot]

Comment on attachment 249043 [details]
Patch 1, patch for landing.

Clearing flags on attachment: 249043

Committed r181754: <http://trac.webkit.org/changeset/181754>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brady Eidson]

Reopening, more work to be done.

[Alex Christensen]

http://trac.webkit.org/changeset/181755 fixed API tests.

[Brady Eidson]

Created attachment 249145 [details]
Patch 2 v1

[WebKit Commit Bot]

Attachment 249145 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/cache/CachedResourceLoader.cpp:519:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

Created attachment 249147 [details]
Patch 2 v2 (fix style)

[Alex Christensen]

Comment on attachment 249145 [details]
Patch 2 v1

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

Hooray! It works!
Let's change how things are organized a bit.

> LayoutTests/http/tests/contentextensions/css-display-none.html.json:26
> +            "url-filter": ".*"

Shouldn't rules that match everything be at the beginning?  Does this have output when running?
We should find a way to fail in this case.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:6312
> +		E4F9EEF3156DA00700D23E7E /* StyleSheetContents.h in Headers */ = {isa = PBXBuildFile; fileRef = E4F9EEF1156D84C400D23E7E /* StyleSheetContents.h */; settings = {ATTRIBUTES = (Private, ); }; };

I don't think this is needed.

> Source/WebCore/contentextensions/ContentExtension.cpp:64
> +        css.append("{display:none !important;}\n");

Weren't we going to make this more than !important?

> Source/WebCore/contentextensions/ContentExtension.cpp:69
> +    m_globalDisplayNoneStyleSheet->parseString(css.toString());

Can this fail?

> Source/WebCore/contentextensions/ContentExtension.h:43
> +class ContentExtension : public RefCounted<ContentExtension> {

1) I don't think ContentExtension is a good name for this class.  What about ContentExtensionsStyleSheet?
2) What if someone calls WKPageGroupRemoveUserContentFilter? Is there a way to invalidate this?
3) WKPageGroupRemoveUserContentFilter should be renamed to WKPageGroupRemoveUserContentExtension.  Not related to this, but I just noticed it.

> Source/WebCore/contentextensions/ContentExtensionRule.h:57
> +    Action(ActionType type, const String& cssSelector)

This name needs to be changed, but otherwise leave this file alone.

> Source/WebCore/contentextensions/ContentExtensionRule.h:64
> +    Action(ActionType type, const String& extensionIdentifier, StyleSheetContents* styleSheetContents)

I don't think this should be put into Action.  See comment in ContentExtensionsBackend.cpp.

> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:103
> +            if (!sawIgnorePreviousRules && contentExtension->globalDisplayNoneStyleSheet())
> +                finalActions.append(Action(ActionType::CSSDisplayNoneStyleSheet, contentExtension->identifier(), contentExtension->globalDisplayNoneStyleSheet()));

The backend should just return a CSSDisplayNoneStyleSheet action.  The identifier should be used to get the stylesheet from the calling function.

> Source/WebCore/dom/DocumentStyleSheetCollection.cpp:194
> +void DocumentStyleSheetCollection::maybeAddContentExtensionSheet(const String& identifier, StyleSheetContents& sheet)

I think this should be addContentExtensionSheet(const String& identifier) and bool hasContentExtensionSheet(const String& identifier).  Also, is there a check to see if identifiers are unique?  Can two extensions have the same name, or would it overwrite the extension?  This should have an API test.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:521
> +    StringBuilder css;

globalDisplayNoneStyleSheet already makes this css.  Don't do it here, too.

> Source/WebCore/loader/cache/CachedResourceRequest.h:65
> +    void setInitiator(DocumentLoader&);
> +    DocumentLoader* initiatingDocumentLoader() const { return m_initiatingDocumentLoader.get(); }

If setInitiator is in the cpp, put initiatingDocumentLoader there too.  Then we don't need to include DocumentLoader.h.

> Source/WebCore/page/UserContentController.cpp:42
> +#include "ContentExtension.h"

Not needed.

[Build Bot]

Comment on attachment 249145 [details]
Patch 2 v1

Attachment 249145 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4938726178291712

New failing tests:
http/tests/contentextensions/css-display-none.html

[Build Bot]

Created attachment 249149 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Brady Eidson]

(In reply to comment #19)
> Comment on attachment 249145 [details]
> Patch 2 v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249145&action=review
> 
> Hooray! It works!
> Let's change how things are organized a bit.
> 
> > LayoutTests/http/tests/contentextensions/css-display-none.html.json:26
> > +            "url-filter": ".*"
> 
> Shouldn't rules that match everything be at the beginning?  Does this have
> output when running?
> We should find a way to fail in this case.

We discussed such rules being at the beginning, yes.

That said, I get no output and definitely get a global stylesheet.  o_O

> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:6312
> > +		E4F9EEF3156DA00700D23E7E /* StyleSheetContents.h in Headers */ = {isa = PBXBuildFile; fileRef = E4F9EEF1156D84C400D23E7E /* StyleSheetContents.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 
> I don't think this is needed.

It is.

> > Source/WebCore/contentextensions/ContentExtension.cpp:64
> > +        css.append("{display:none !important;}\n");
> 
> Weren't we going to make this more than !important?

I learned by asking CSS experts - !important in a user stylesheet is already the single most important a rule can be, so we're good.

> > Source/WebCore/contentextensions/ContentExtension.cpp:69
> > +    m_globalDisplayNoneStyleSheet->parseString(css.toString());
> 
> Can this fail?

It can, and I should handle the failure. Good catch.
 
> > Source/WebCore/contentextensions/ContentExtension.h:43
> > +class ContentExtension : public RefCounted<ContentExtension> {
> 
> 1) I don't think ContentExtension is a good name for this class.  What about
> ContentExtensionsStyleSheet?

A "ContentExtension" is a collection of objects. In this case it is an identifier, a CompiledContentExtension, and optionally a global style sheet.

"ContentExtensionStyleSheet" is absolutely the wrong name for this.

> 2) What if someone calls WKPageGroupRemoveUserContentFilter? Is there a way
> to invalidate this?

It is already removed from the controller, just like CompiledContentExtensions used to be.

The global stylesheets or any one-off rules are *not* removed from live pages... but I'm not sure that's important to do.

> 
> > Source/WebCore/contentextensions/ContentExtensionRule.h:64
> > +    Action(ActionType type, const String& extensionIdentifier, StyleSheetContents* styleSheetContents)
> 
> I don't think this should be put into Action.  See comment in
> ContentExtensionsBackend.cpp.

Looking forward to it...
 
> > Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:103
> > +            if (!sawIgnorePreviousRules && contentExtension->globalDisplayNoneStyleSheet())
> > +                finalActions.append(Action(ActionType::CSSDisplayNoneStyleSheet, contentExtension->identifier(), contentExtension->globalDisplayNoneStyleSheet()));
> 
> The backend should just return a CSSDisplayNoneStyleSheet action.  The
> identifier should be used to get the stylesheet from the calling function.

I disagree. The action should have the stylesheet attached to it, the same way "css selector" actions have selectors attached to them.

> 
> > Source/WebCore/dom/DocumentStyleSheetCollection.cpp:194
> > +void DocumentStyleSheetCollection::maybeAddContentExtensionSheet(const String& identifier, StyleSheetContents& sheet)
> 
> I think this should be addContentExtensionSheet(const String& identifier)
> and bool hasContentExtensionSheet(const String& identifier).  

There's no need to split this into two methods and divide up that responsibility, because we don't need that flexibility.

There's already well establish precedent for "maybeDoFoo" when "Foo" might already have been done once - I think the pattern fits nicely here.

> Also, is there a check to see if identifiers are unique?  Can two extensions have the same
> name, or would it overwrite the extension?  This should have an API test.

You tell me - The identifiers come from the API. IIRC, setting a new extension over a previously set identifier overwrites the original one.

That behavior should hold throughout here.

> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:521
> > +    StringBuilder css;
> 
> globalDisplayNoneStyleSheet already makes this css.  Don't do it here, too.

That is not true - globalDisplayNoneStyleSheet handles doing this for the global style sheets represented by the CSSDisplayNoneStyleSheet action, but not for the rules accumulated via CSSDisplayNoneSelector actions.

This StringBuilder does the latter.

> 
> > Source/WebCore/loader/cache/CachedResourceRequest.h:65
> > +    void setInitiator(DocumentLoader&);
> > +    DocumentLoader* initiatingDocumentLoader() const { return m_initiatingDocumentLoader.get(); }
> 
> If setInitiator is in the cpp, put initiatingDocumentLoader there too.  Then
> we don't need to include DocumentLoader.h.

Including DocumentLoader.h isn't because of initiatingDocumentLoader - it's because of RefPtr + DocumentLoader.

If we don't include DocumentLoader.h and instead forward declare it, the RefPtr<DocumentLoader> member forces everyone who includes this header to *also* include DocumentLoader.h

> 
> > Source/WebCore/page/UserContentController.cpp:42
> > +#include "ContentExtension.h"
> 
> Not needed.

Truth!

[Alex Christensen]

Comment on attachment 249145 [details]
Patch 2 v1

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

>>> Source/WebCore/contentextensions/ContentExtension.h:43
>>> +class ContentExtension : public RefCounted<ContentExtension> {
>> 
>> 1) I don't think ContentExtension is a good name for this class.  What about ContentExtensionsStyleSheet?
>> 2) What if someone calls WKPageGroupRemoveUserContentFilter? Is there a way to invalidate this?
>> 3) WKPageGroupRemoveUserContentFilter should be renamed to WKPageGroupRemoveUserContentExtension.  Not related to this, but I just noticed it.
> 
> A "ContentExtension" is a collection of objects. In this case it is an identifier, a CompiledContentExtension, and optionally a global style sheet.
> 
> "ContentExtensionStyleSheet" is absolutely the wrong name for this.

The way I see it, a content extension is something that is installed in a browser.  When installing, that makes one CompiledContentExtension and then makes one of these objects in each WebProcess.  I'm not sure what it should be called, but I don't think it should be ContentExtension.

>>> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:103
>>> +                finalActions.append(Action(ActionType::CSSDisplayNoneStyleSheet, contentExtension->identifier(), contentExtension->globalDisplayNoneStyleSheet()));
>> 
>> The backend should just return a CSSDisplayNoneStyleSheet action.  The identifier should be used to get the stylesheet from the calling function.
> 
> I disagree. The action should have the stylesheet attached to it, the same way "css selector" actions have selectors attached to them.

The backend should not depend on structures outside of it, like a ContentExtension or whatever we end up calling it.  The backend should just be given a string (the URL) and return a small description of actions that can be used to get the compiled stylesheet from a cache of compiled style sheets.

>>> Source/WebCore/dom/DocumentStyleSheetCollection.cpp:194
>>> +void DocumentStyleSheetCollection::maybeAddContentExtensionSheet(const String& identifier, StyleSheetContents& sheet)
>> 
>> I think this should be addContentExtensionSheet(const String& identifier) and bool hasContentExtensionSheet(const String& identifier).  Also, is there a check to see if identifiers are unique?  Can two extensions have the same name, or would it overwrite the extension?  This should have an API test.
> 
> There's no need to split this into two methods and divide up that responsibility, because we don't need that flexibility.
> 
> There's already well establish precedent for "maybeDoFoo" when "Foo" might already have been done once - I think the pattern fits nicely here.

Maybe.

>>> Source/WebCore/loader/cache/CachedResourceLoader.cpp:521
>>> +    StringBuilder css;
>> 
>> globalDisplayNoneStyleSheet already makes this css.  Don't do it here, too.
> 
> That is not true - globalDisplayNoneStyleSheet handles doing this for the global style sheets represented by the CSSDisplayNoneStyleSheet action, but not for the rules accumulated via CSSDisplayNoneSelector actions.
> 
> This StringBuilder does the latter.

You have two places that append the string "{display:none !important;}\n" to a StringBuilder.  That duplicate code should be consolidated.

[Brady Eidson]

(In reply to comment #23)
> Comment on attachment 249145 [details]
> Patch 2 v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249145&action=review
> 
> >>> Source/WebCore/contentextensions/ContentExtension.h:43
> >>> +class ContentExtension : public RefCounted<ContentExtension> {
> >> 
> >> 1) I don't think ContentExtension is a good name for this class.  What about ContentExtensionsStyleSheet?
> >> 2) What if someone calls WKPageGroupRemoveUserContentFilter? Is there a way to invalidate this?
> >> 3) WKPageGroupRemoveUserContentFilter should be renamed to WKPageGroupRemoveUserContentExtension.  Not related to this, but I just noticed it.
> > 
> > A "ContentExtension" is a collection of objects. In this case it is an identifier, a CompiledContentExtension, and optionally a global style sheet.
> > 
> > "ContentExtensionStyleSheet" is absolutely the wrong name for this.
> 
> The way I see it, a content extension is something that is installed in a
> browser.  

Since this is WebKit and not a browser, I'm already dubious.

> When installing, that makes one CompiledContentExtension and then
> makes one of these objects in each WebProcess.  

Being in WebCore, this theoretically applies to WebKit1 process-wide.

> I'm not sure what it should
> be called, but I don't think it should be ContentExtension.

ContentExtensionData? That's wrong too, since it's not just a dumb collection of datum. There's actual logic at play in deciding whether or not to create the StyleSheetContents, and data to support that logic.

> >>> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:103
> >>> +                finalActions.append(Action(ActionType::CSSDisplayNoneStyleSheet, contentExtension->identifier(), contentExtension->globalDisplayNoneStyleSheet()));
> >> 
> >> The backend should just return a CSSDisplayNoneStyleSheet action.  The identifier should be used to get the stylesheet from the calling function.
> > 
> > I disagree. The action should have the stylesheet attached to it, the same way "css selector" actions have selectors attached to them.
> 
> The backend should not depend on structures outside of it, like a
> ContentExtension or whatever we end up calling it.  The backend should just
> be given a string (the URL) and return a small description of actions that
> can be used to get the compiled stylesheet from a cache of compiled style
> sheets.

If we do it your way, we have to expose some "look up stylesheet for identifier" logic somewhere. Where would that be?

Logically it would have to go on the backend, which basically just reintroduces the interface of the backend knowing about plenty outside of itself.

In general, I kind of hate that we're exposing DFA actions out into the loader. But I don't see how this patch changes things; The backend already understands concepts that aren't implemented by the content extension mechanism, such as "CSS selectors"

If you assert that the backend should return an action that says "Apply this CSS selector" and also has the contents of the selector to apply, then I assert that it can also return an action that says "Apply this stylesheet" and also includes the stylesheet.

> 
> >>> Source/WebCore/dom/DocumentStyleSheetCollection.cpp:194
> >>> +void DocumentStyleSheetCollection::maybeAddContentExtensionSheet(const String& identifier, StyleSheetContents& sheet)
> >> 
> >> I think this should be addContentExtensionSheet(const String& identifier) and bool hasContentExtensionSheet(const String& identifier).  Also, is there a check to see if identifiers are unique?  Can two extensions have the same name, or would it overwrite the extension?  This should have an API test.
> > 
> > There's no need to split this into two methods and divide up that responsibility, because we don't need that flexibility.
> > 
> > There's already well establish precedent for "maybeDoFoo" when "Foo" might already have been done once - I think the pattern fits nicely here.
> 
> Maybe.
> 
> >>> Source/WebCore/loader/cache/CachedResourceLoader.cpp:521
> >>> +    StringBuilder css;
> >> 
> >> globalDisplayNoneStyleSheet already makes this css.  Don't do it here, too.
> > 
> > That is not true - globalDisplayNoneStyleSheet handles doing this for the global style sheets represented by the CSSDisplayNoneStyleSheet action, but not for the rules accumulated via CSSDisplayNoneSelector actions.
> > 
> > This StringBuilder does the latter.
> 
> You have two places that append the string "{display:none !important;}\n" to
> a StringBuilder.  That duplicate code should be consolidated.

I misunderstood your feedback. This makes sense.

I'll update the patch with the selector string change and think about naming the object above a little better.

I - so far - definitely still disagree that the fundamental structure of the patch is wrong.

We can talk about it in person Monday, perhaps.

[Alex Christensen]

Also, with this patch, DFABytecodeInterpreter::interpret needs a loop ignoring the actions from the DFA root.  Basically just this where the current FIXME is:

while (static_cast<DFABytecodeInstruction>(m_bytecode[programCounter]) == DFABytecodeInstruction::AppendAction)
    programCounter += instructionSizeWithArguments(DFABytecodeInstruction::AppendAction);

I'll come talk to you in a little bit.

[Alex Christensen]

(In reply to comment #24)
The name of the ContentExtension class isn't the biggest deal right now.  If the purpose of that class becomes confusing we could always rename it later.
> > >>> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:103
> > >>> +                finalActions.append(Action(ActionType::CSSDisplayNoneStyleSheet, contentExtension->identifier(), contentExtension->globalDisplayNoneStyleSheet()));
> > >> 
> > >> The backend should just return a CSSDisplayNoneStyleSheet action.  The identifier should be used to get the stylesheet from the calling function.
> > > 
> > > I disagree. The action should have the stylesheet attached to it, the same way "css selector" actions have selectors attached to them.
> > 
> > The backend should not depend on structures outside of it, like a
> > ContentExtension or whatever we end up calling it.  The backend should just
> > be given a string (the URL) and return a small description of actions that
> > can be used to get the compiled stylesheet from a cache of compiled style
> > sheets.
> 
> If we do it your way, we have to expose some "look up stylesheet for
> identifier" logic somewhere. Where would that be?
> 
> Logically it would have to go on the backend, which basically just
> reintroduces the interface of the backend knowing about plenty outside of
> itself.
> 
> In general, I kind of hate that we're exposing DFA actions out into the
> loader. But I don't see how this patch changes things; The backend already
> understands concepts that aren't implemented by the content extension
> mechanism, such as "CSS selectors"
> 
> If you assert that the backend should return an action that says "Apply this
> CSS selector" and also has the contents of the selector to apply, then I
> assert that it can also return an action that says "Apply this stylesheet"
> and also includes the stylesheet.
> 
I guess ContentExtensionsBackend is the place where we store the mapping of String to CompiledContentExtension, so I guess we need StyleSheetContents* ContentExtensionsBackend:: globalDisplayNoneStyleSheet(const String& identifier).  Then use that from CachedResourceLoader::requestResource to lookup the StyleSheetContents from the identifiers we get from the actions.

[Brady Eidson]

Created attachment 249266 [details]
Patch 2 v3

[Brady Eidson]

In radar as rdar://problem/19978632

[Alex Christensen]

Comment on attachment 249266 [details]
Patch 2 v3

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

Almost there.  A few small changes and put it in! r=me once these issues are addressed.

> Source/WebCore/contentextensions/ContentExtension.cpp:70
> +    m_globalDisplayNoneStyleSheet->parseString(css.toString());

This could fail.

> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:54
> +    RefPtr<ContentExtension> extension = ContentExtension::create(identifier, adoptRef(*compiledContentExtension.leakRef()));
> +    m_contentExtensions.set(identifier, WTF::move(extension));

Do you need WTF::move with a RefPtr?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:559
> +        styleSheet->parseString(css.toString());

This could fail.

[Brady Eidson]

Created attachment 249276 [details]
Patch 2, patch for landing

[WebKit Commit Bot]

Comment on attachment 249276 [details]
Patch 2, patch for landing

Clearing flags on attachment: 249276

Committed r181876: <http://trac.webkit.org/changeset/181876>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This broke 11 API tests: https://build.webkit.org/builders/Apple%20Mavericks%20Debug%20WK1%20%28Tests%29/builds/11377/steps/run-api-tests/logs/stdio

[Alex Christensen]

API tests fixed in http://trac.webkit.org/changeset/181883