Summary: | prepare content extensions for css selectors | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||||||||||
Component: | WebCore Misc. | Assignee: | Alex Christensen <achristensen> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | beidson, benjamin, buildbot, commit-queue, japhet, rniwa | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Attachments: |
|
Description
Alex Christensen
2015-03-03 10:46:39 PST
Created attachment 247771 [details]
Patch
Created attachment 247776 [details]
Patch
Created attachment 247781 [details]
Patch
Created attachment 247786 [details]
Patch
Comment on attachment 247786 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247786&action=review > Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:174 > for (auto& ruleListSlot : m_ruleLists) { This would cause problems with multiple extensions installed with ignore-previous-rules, but we can fix that and test it in another patch. > Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:180 > + actionLocations.reserveInitialCapacity(actionLocations.size()); This should be triggeredActions.size() Comment on attachment 247786 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247786&action=review Quick review. > Source/WebCore/contentextensions/ContentExtensionActions.h:39 > +typedef uint8_t ActionDescription; I am a bit unconvinced about the name. > Source/WebCore/contentextensions/ContentExtensionActions.h:45 > + // The enum value contains all the information necessary for applying these actions. > + BlockLoad, Can you please keep the list sorted? Maybe add a line of comment for each action? > Source/WebCore/contentextensions/ContentExtensionActions.h:48 > + BlockLoadIf3rdParty, What the heck? Change of plans? > Source/WebCore/contentextensions/ContentExtensionActions.h:50 > + // When serialized, CSSDisplayNone is followed by the length of the selector (4 bytes) then the selector. This comment is out of context. The enum is too far from the serialization code. > Source/WebCore/contentextensions/ContentExtensionRule.cpp:43 > + : m_type(static_cast<ActionType>(*description)) Please use a switch() for this. Clang can optimize it out, and we can catch incorrect values. > Source/WebCore/contentextensions/ContentExtensionRule.cpp:48 > + unsigned length = *reinterpret_cast<const unsigned*>(description + sizeof(uint8_t)); > + m_cssSelector = String(description + sizeof(uint8_t) + sizeof(unsigned), length); Can we get description + buffer length as input? It would be good to crash if we ever create a String past the buffer. > Source/WebCore/contentextensions/ContentExtensionRule.h:53 > + Action(const ActionDescription*); I think this should not be a constructor. A deserialize() function somewhere seems cleaner to me. > Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:71 > + { I think the WebKit style is case ActionType::CSSDisplayNone: { } I am never sure about the style for switch-case... Created attachment 247804 [details]
Patch
Comment on attachment 247804 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247804&action=review > Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:47 > +Vector<unsigned> ContentExtensionsBackend::serializeActions(const Vector<ContentExtensionRule>& ruleList, Vector<SerializedActionByte>& actions) Could you merge similar actions when they are following eachother? This would help making better state machines. > Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:55 > + for (unsigned ruleIndex = 0; ruleIndex < ruleList.size(); ruleIndex++) { ++ruleIndex > Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:81 > + for (unsigned i = 0; i < selectorLength; i++) > + actions.append(selector[i]); Selectors can be 8 bits or 16 bits. Here you would nuke the 16bits one into 8 bits bufffer. You should probably have "length, character-size, bytes". > Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:179 > + actionLocations.reserveInitialCapacity(actionLocations.size()); actionLocations.size() -> triggeredActions.size()! > Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:186 > + if (action.type() == ActionType::IgnorePreviousRules) Maybe ActionType::Block should be one of those early return too? If we have 1) CSS 2) Whitelist 3) CSS 4) Block No point in having [3] in the return vector. > Source/WebCore/loader/cache/CachedResourceLoader.cpp:473 > + // Start with the last action to properly deal with IgnorePreviousRules. This code does not handle IgnorePreviousRules. It looks like you could execute those actions in any order. > LayoutTests/ChangeLog:10 > + * http/tests/usercontentfilter/css-display-none-expected.txt: Added. > + * http/tests/usercontentfilter/css-display-none.html: Added. > + * http/tests/usercontentfilter/css-display-none.html.json: Added. We should really get proper testing of the backend at some point :) Comment on attachment 247804 [details] Patch Attachment 247804 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5468072710242304 New failing tests: http/tests/usercontentfilter/block-cookies-basic.html http/tests/usercontentfilter/basic-filter.html http/tests/usercontentfilter/block-cookies-send.html Created attachment 247805 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 247807 [details]
Patch
Created attachment 247819 [details]
Patch
Comment on attachment 247819 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247819&action=review > Source/WebCore/contentextensions/ContentExtensionRule.cpp:56 > + unsigned selectorLength = *reinterpret_cast<const unsigned*>(&actions[location + sizeof(ActionType)]); You are shifting characters one by one when serializing, then reading them in one read when deserializing. This only works because you shifted them as little endian. You should use the same method on read and write to make this work everywhere. > Source/WebCore/contentextensions/ContentExtensionRule.cpp:60 > + ASSERT(actions.size() >= stringStartIndex + selectorLength * sizeof(UChar)); This should be a release assert IMHO. > Source/WebCore/contentextensions/ContentExtensionRule.cpp:63 > + ASSERT(actions.size() >= stringStartIndex + selectorLength * sizeof(LChar)); This should be a release assert IMHO. > Source/WebCore/contentextensions/ContentExtensionRule.h:45 > + bool urlFilterIsCaseSensitive = false; Let's change it to urlFilterIsCaseSensitive { false }; People seem to favor this syntax now. > Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:49 > + // This data is interpreted by Action::Action(const SerializedActionByte*). This is no longer true. Probably no need for the comment now that there is a name to the function that deserializes. > Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:86 > + actions.append(wideCharacter); > + actions.append(wideCharacter >> 8); Same problem: this force a write in little-endian. Comments addressed, committed to http://trac.webkit.org/changeset/180978 Windows build fix committed to http://trac.webkit.org/changeset/180979 Will look into duplicate actions and other optimizations. |