Content extension with oft-repeated rules can cause slowdown. With a non-trivial extension adding multiple CSS rules per sub resource, we can't afford to create a new stylesheet each time, especially when they can often represent exactly the same rulesets. We should have a single stylesheet per document and track which action-ids have added a display:none selector so we don't duplicate anything. <rdar://problem/20618511>
Created attachment 251269 [details] Patch v1
Comment on attachment 251269 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=251269&action=review Thanks Brady! We desperately need this, but I'm in a crunch right now. r- even though this is very close to what we need. I just think the Actions should be handled a little differently. > Source/WebCore/ChangeLog:3 > + Need a short description (OOPS!). Describe. > Source/WebCore/ChangeLog:8 > + No new tests (No behavior change, behavior covered by existing tests). I wouldn't say "No behavior change", but this is covered by existing tests. > Source/WebCore/contentextensions/ContentExtensionRule.h:59 > + , m_actionID(UINT64_MAX) std::numeric_limits<unsigned>::max() > Source/WebCore/contentextensions/ContentExtensionRule.h:91 > + void setActionID(uint64_t actionID) { m_actionID = actionID; } You could probably get rid of this and setExtensionIdentifier and have the location be an additional constructor parameter once you change Action::deserialize (see later comment) > Source/WebCore/contentextensions/ContentExtensionRule.h:96 > + String m_extensionIdentifier; I'm quite opposed to adding a String here. We need to change the return type of ContentExtensionsBackend::actionsForResourceLoad to return something like a Vector<std::pair<String, Vector<Action>>>. Actually, it should probably be a Vector<ActionsForResourceLoad> because we should put more things in a class there and not here. > Source/WebCore/contentextensions/ContentExtensionRule.h:98 > + uint64_t m_actionID; unsigned m_location. We should use the location (in the serialized actions from Action::deserialize). > Source/WebCore/contentextensions/ContentExtensionStyleSheet.cpp:47 > + ASSERT(selectorID && selectorID != UINT64_MAX); location can be 0. > Source/WebCore/contentextensions/ContentExtensionStyleSheet.h:57 > + HashSet<uint64_t> m_addedSelectorIDs; HashSet<uint64_t, DefaultHash<uint64_t>::Hash, WTF::UnsignedWithZeroKeyHashTraits<uint64_t>> because location can be 0. > Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:109 > + action.setActionID(i); This should be part of Action::deserialize.
We discussed this in person, followup patch on its way.
Comment on attachment 251269 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=251269&action=review >> Source/WebCore/contentextensions/ContentExtensionRule.h:59 >> + , m_actionID(UINT64_MAX) > > std::numeric_limits<unsigned>::max() std::numeric_limits<uint64_t>::max()
(In reply to comment #4) > Comment on attachment 251269 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=251269&action=review > > >> Source/WebCore/contentextensions/ContentExtensionRule.h:59 > >> + , m_actionID(UINT64_MAX) > > > > std::numeric_limits<unsigned>::max() > > std::numeric_limits<uint64_t>::max() I also wanted to change the type to unsigned :)
Created attachment 251275 [details] Patch v2
Comment on attachment 251275 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=251275&action=review > Source/WebCore/contentextensions/ContentExtensionRule.h:95 > + String m_extensionIdentifier; I don't want this here, but we can clean this up later.
Comment on attachment 251275 [details] Patch v2 Attachment 251275 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6077211144617984 New failing tests: http/tests/contentextensions/css-display-none.html
Created attachment 251286 [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
Definitely was not failing locally. Will take a look in the A.M.
Running run-webkit-tests locally I cannot reproduce this. But running WKTR on this one test directly, I can reliably reproduce. This is almost certainly timing related.
Created attachment 251381 [details] Patch for landing
Created attachment 251384 [details] Patch for landing v2
Created attachment 251385 [details] Patch for landing v3
(In reply to comment #2) > Comment on attachment 251269 [details] > > Source/WebCore/contentextensions/ContentExtensionRule.h:98 > > + uint64_t m_actionID; > > unsigned m_location. I thought I'd made it 64 bit for a reason, then the build error on iOS reminded me: /Volumes/Data/EWS/WebKit/Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:119:77: error: implicit conversion loses integer precision: 'unsigned long long' to 'uint32_t' (aka 'unsigned int') [-Werror,-Wshorten-64-to-32] Action action = Action::deserialize(actions, actionsLength, actionLocation); Because while some of the action locations are 32 bits, when we pull from a: HashSet<uint64_t, DefaultHash<uint64_t>::Hash, WTF::UnsignedWithZeroKeyHashTraits<uint64_t>> ... the actions are 64 bits. I'm going to go back to 64 bits for landing, then if you later determine that action location can, in fact, be 32 bits we can take it all down to 32.
Created attachment 251438 [details] Patch for landing v4
Created attachment 251443 [details] Patch for landing v5
(In reply to comment #15) > > I'm going to go back to 64 bits for landing, then if you later determine > that action location can, in fact, be 32 bits we can take it all down to 32. NM, I see what I'd changed that caused this. Back to 32...
Created attachment 251446 [details] Patch for landing
Comment on attachment 251446 [details] Patch for landing Clearing flags on attachment: 251446 Committed r183195: <http://trac.webkit.org/changeset/183195>