Changeset 213322 in webkit


Ignore:
Timestamp:
Mar 2, 2017 4:18:44 PM (7 years ago)
Author:
achristensen@apple.com
Message:

Modernize ContentExtensionParser
https://bugs.webkit.org/show_bug.cgi?id=169106

Reviewed by Andy Estes.

We were returning std::error_code and passing the return value as a parameter reference.
because we wrote this code before we had WTF::Expected.

No change in behavior, verified by many API tests.

  • contentextensions/ContentExtensionCompiler.cpp:

(WebCore::ContentExtensions::compileRuleList):

  • contentextensions/ContentExtensionParser.cpp:

(WebCore::ContentExtensions::getDomainList):
(WebCore::ContentExtensions::loadTrigger):
(WebCore::ContentExtensions::loadAction):
(WebCore::ContentExtensions::loadRule):
(WebCore::ContentExtensions::loadEncodedRules):
(WebCore::ContentExtensions::parseRuleList):

  • contentextensions/ContentExtensionParser.h:
  • contentextensions/ContentExtensionRule.cpp:

(WebCore::ContentExtensions::ContentExtensionRule::ContentExtensionRule):

  • contentextensions/ContentExtensionRule.h:
Location:
trunk/Source/WebCore
Files:
6 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebCore/ChangeLog

    r213320 r213322  
     12017-03-02  Alex Christensen  <achristensen@webkit.org>
     2
     3        Modernize ContentExtensionParser
     4        https://bugs.webkit.org/show_bug.cgi?id=169106
     5
     6        Reviewed by Andy Estes.
     7
     8        We were returning std::error_code and passing the return value as a parameter reference.
     9        because we wrote this code before we had WTF::Expected.
     10
     11        No change in behavior, verified by many API tests.
     12
     13        * contentextensions/ContentExtensionCompiler.cpp:
     14        (WebCore::ContentExtensions::compileRuleList):
     15        * contentextensions/ContentExtensionParser.cpp:
     16        (WebCore::ContentExtensions::getDomainList):
     17        (WebCore::ContentExtensions::loadTrigger):
     18        (WebCore::ContentExtensions::loadAction):
     19        (WebCore::ContentExtensions::loadRule):
     20        (WebCore::ContentExtensions::loadEncodedRules):
     21        (WebCore::ContentExtensions::parseRuleList):
     22        * contentextensions/ContentExtensionParser.h:
     23        * contentextensions/ContentExtensionRule.cpp:
     24        (WebCore::ContentExtensions::ContentExtensionRule::ContentExtensionRule):
     25        * contentextensions/ContentExtensionRule.h:
     26
    1272017-03-02  Ryan Haddad  <ryanhaddad@apple.com>
    228
  • trunk/Source/WebCore/contentextensions/ContentExtensionCompiler.cpp

    r194496 r213322  
    206206}
    207207
    208 std::error_code compileRuleList(ContentExtensionCompilationClient& client, String&& ruleList)
     208std::error_code compileRuleList(ContentExtensionCompilationClient& client, String&& ruleJSON)
    209209{
    210     Vector<ContentExtensionRule> parsedRuleList;
    211     auto parserError = parseRuleList(ruleList, parsedRuleList);
    212     ruleList = String();
    213     if (parserError)
    214         return parserError;
     210    auto ruleList = parseRuleList(WTFMove(ruleJSON));
     211    if (!ruleList.hasValue())
     212        return ruleList.error();
     213    Vector<ContentExtensionRule> parsedRuleList = WTFMove(ruleList.value());
    215214
    216215#if CONTENT_EXTENSIONS_PERFORMANCE_REPORTING
  • trunk/Source/WebCore/contentextensions/ContentExtensionParser.cpp

    r209906 r213322  
    4141#include <JavaScriptCore/VM.h>
    4242#include <wtf/CurrentTime.h>
     43#include <wtf/Expected.h>
    4344#include <wtf/text/WTFString.h>
    4445
     
    5960}
    6061   
    61 static std::error_code getDomainList(ExecState& exec, const JSObject* arrayObject, Vector<String>& vector)
    62 {
    63     VM& vm = exec.vm();
    64     auto scope = DECLARE_THROW_SCOPE(vm);
    65 
    66     ASSERT(vector.isEmpty());
     62static Expected<Vector<String>, std::error_code> getDomainList(ExecState& exec, const JSObject* arrayObject)
     63{
     64    VM& vm = exec.vm();
     65    auto scope = DECLARE_THROW_SCOPE(vm);
     66
    6767    if (!arrayObject || !isJSArray(arrayObject))
    68         return ContentExtensionError::JSONInvalidDomainList;
     68        return makeUnexpected(ContentExtensionError::JSONInvalidDomainList);
    6969    const JSArray* array = jsCast<const JSArray*>(arrayObject);
    7070   
     71    Vector<String> domains;
    7172    unsigned length = array->length();
    7273    for (unsigned i = 0; i < length; ++i) {
    7374        const JSValue value = array->getIndex(&exec, i);
    7475        if (scope.exception() || !value.isString())
    75             return ContentExtensionError::JSONInvalidDomainList;
     76            return makeUnexpected(ContentExtensionError::JSONInvalidDomainList);
    7677       
    7778        // Domains should be punycode encoded lower case.
    7879        const String& domain = asString(value)->value(&exec);
    7980        if (domain.isEmpty())
    80             return ContentExtensionError::JSONInvalidDomainList;
     81            return makeUnexpected(ContentExtensionError::JSONInvalidDomainList);
    8182        if (!containsOnlyASCIIWithNoUppercase(domain))
    82             return ContentExtensionError::JSONDomainNotLowerCaseASCII;
    83         vector.append(domain);
    84     }
    85     return { };
     83            return makeUnexpected(ContentExtensionError::JSONDomainNotLowerCaseASCII);
     84        domains.append(domain);
     85    }
     86    return WTFMove(domains);
    8687}
    8788
     
    118119}
    119120   
    120 static std::error_code loadTrigger(ExecState& exec, const JSObject& ruleObject, Trigger& trigger)
     121static Expected<Trigger, std::error_code> loadTrigger(ExecState& exec, const JSObject& ruleObject)
    121122{
    122123    VM& vm = exec.vm();
     
    125126    const JSValue triggerObject = ruleObject.get(&exec, Identifier::fromString(&exec, "trigger"));
    126127    if (!triggerObject || scope.exception() || !triggerObject.isObject())
    127         return ContentExtensionError::JSONInvalidTrigger;
     128        return makeUnexpected(ContentExtensionError::JSONInvalidTrigger);
    128129   
    129130    const JSValue urlFilterObject = triggerObject.get(&exec, Identifier::fromString(&exec, "url-filter"));
    130131    if (!urlFilterObject || scope.exception() || !urlFilterObject.isString())
    131         return ContentExtensionError::JSONInvalidURLFilterInTrigger;
     132        return makeUnexpected(ContentExtensionError::JSONInvalidURLFilterInTrigger);
    132133
    133134    String urlFilter = asString(urlFilterObject)->value(&exec);
    134135    if (urlFilter.isEmpty())
    135         return ContentExtensionError::JSONInvalidURLFilterInTrigger;
    136 
     136        return makeUnexpected(ContentExtensionError::JSONInvalidURLFilterInTrigger);
     137
     138    Trigger trigger;
    137139    trigger.urlFilter = urlFilter;
    138140
     
    145147        auto typeFlagsError = getTypeFlags(exec, resourceTypeValue, trigger.flags, readResourceType);
    146148        if (typeFlagsError)
    147             return typeFlagsError;
     149            return makeUnexpected(typeFlagsError);
    148150    } else if (!resourceTypeValue.isUndefined())
    149         return ContentExtensionError::JSONInvalidTriggerFlagsArray;
     151        return makeUnexpected(ContentExtensionError::JSONInvalidTriggerFlagsArray);
    150152
    151153    const JSValue loadTypeValue = triggerObject.get(&exec, Identifier::fromString(&exec, "load-type"));
     
    153155        auto typeFlagsError = getTypeFlags(exec, loadTypeValue, trigger.flags, readLoadType);
    154156        if (typeFlagsError)
    155             return typeFlagsError;
     157            return makeUnexpected(typeFlagsError);
    156158    } else if (!loadTypeValue.isUndefined())
    157         return ContentExtensionError::JSONInvalidTriggerFlagsArray;
    158 
    159     const JSValue ifDomain = triggerObject.get(&exec, Identifier::fromString(&exec, "if-domain"));
    160     if (!scope.exception() && ifDomain.isObject()) {
    161         auto ifDomainError = getDomainList(exec, asObject(ifDomain), trigger.domains);
    162         if (ifDomainError)
    163             return ifDomainError;
     159        return makeUnexpected(ContentExtensionError::JSONInvalidTriggerFlagsArray);
     160
     161    const JSValue ifDomainValue = triggerObject.get(&exec, Identifier::fromString(&exec, "if-domain"));
     162    if (!scope.exception() && ifDomainValue.isObject()) {
     163        auto ifDomain = getDomainList(exec, asObject(ifDomainValue));
     164        if (!ifDomain.hasValue())
     165            return makeUnexpected(ifDomain.error());
     166        trigger.domains = WTFMove(ifDomain.value());
    164167        if (trigger.domains.isEmpty())
    165             return ContentExtensionError::JSONInvalidDomainList;
     168            return makeUnexpected(ContentExtensionError::JSONInvalidDomainList);
    166169        ASSERT(trigger.domainCondition == Trigger::DomainCondition::None);
    167170        trigger.domainCondition = Trigger::DomainCondition::IfDomain;
    168     } else if (!ifDomain.isUndefined())
    169         return ContentExtensionError::JSONInvalidDomainList;
    170    
    171     const JSValue unlessDomain = triggerObject.get(&exec, Identifier::fromString(&exec, "unless-domain"));
    172     if (!scope.exception() && unlessDomain.isObject()) {
     171    } else if (!ifDomainValue.isUndefined())
     172        return makeUnexpected(ContentExtensionError::JSONInvalidDomainList);
     173   
     174    const JSValue unlessDomainValue = triggerObject.get(&exec, Identifier::fromString(&exec, "unless-domain"));
     175    if (!scope.exception() && unlessDomainValue.isObject()) {
    173176        if (trigger.domainCondition != Trigger::DomainCondition::None)
    174             return ContentExtensionError::JSONUnlessAndIfDomain;
    175         auto unlessDomainError = getDomainList(exec, asObject(unlessDomain), trigger.domains);
    176         if (unlessDomainError)
    177             return unlessDomainError;
     177            return makeUnexpected(ContentExtensionError::JSONUnlessAndIfDomain);
     178        auto unlessDomain = getDomainList(exec, asObject(unlessDomainValue));
     179        if (!unlessDomain.hasValue())
     180            return makeUnexpected(unlessDomain.error());
     181        trigger.domains = WTFMove(unlessDomain.value());
    178182        if (trigger.domains.isEmpty())
    179             return ContentExtensionError::JSONInvalidDomainList;
     183            return makeUnexpected(ContentExtensionError::JSONInvalidDomainList);
    180184        trigger.domainCondition = Trigger::DomainCondition::UnlessDomain;
    181     } else if (!unlessDomain.isUndefined())
    182         return ContentExtensionError::JSONInvalidDomainList;
    183 
    184     return { };
     185    } else if (!unlessDomainValue.isUndefined())
     186        return makeUnexpected(ContentExtensionError::JSONInvalidDomainList);
     187
     188    return WTFMove(trigger);
    185189}
    186190
     
    194198}
    195199
    196 static std::error_code loadAction(ExecState& exec, const JSObject& ruleObject, Action& action, bool& validSelector)
    197 {
    198     VM& vm = exec.vm();
    199     auto scope = DECLARE_THROW_SCOPE(vm);
    200 
    201     validSelector = true;
     200static Expected<std::optional<Action>, std::error_code> loadAction(ExecState& exec, const JSObject& ruleObject)
     201{
     202    VM& vm = exec.vm();
     203    auto scope = DECLARE_THROW_SCOPE(vm);
     204
    202205    const JSValue actionObject = ruleObject.get(&exec, Identifier::fromString(&exec, "action"));
    203206    if (!actionObject || scope.exception() || !actionObject.isObject())
    204         return ContentExtensionError::JSONInvalidAction;
     207        return makeUnexpected(ContentExtensionError::JSONInvalidAction);
    205208
    206209    const JSValue typeObject = actionObject.get(&exec, Identifier::fromString(&exec, "type"));
    207210    if (!typeObject || scope.exception() || !typeObject.isString())
    208         return ContentExtensionError::JSONInvalidActionType;
     211        return makeUnexpected(ContentExtensionError::JSONInvalidActionType);
    209212
    210213    String actionType = asString(typeObject)->value(&exec);
    211214
    212215    if (actionType == "block")
    213         action = ActionType::BlockLoad;
    214     else if (actionType == "ignore-previous-rules")
    215         action = ActionType::IgnorePreviousRules;
    216     else if (actionType == "block-cookies")
    217         action = ActionType::BlockCookies;
    218     else if (actionType == "css-display-none") {
     216        return {{ActionType::BlockLoad}};
     217    if (actionType == "ignore-previous-rules")
     218        return {{ActionType::IgnorePreviousRules}};
     219    if (actionType == "block-cookies")
     220        return {{ActionType::BlockCookies}};
     221    if (actionType == "css-display-none") {
    219222        JSValue selector = actionObject.get(&exec, Identifier::fromString(&exec, "selector"));
    220223        if (!selector || scope.exception() || !selector.isString())
    221             return ContentExtensionError::JSONInvalidCSSDisplayNoneActionType;
     224            return makeUnexpected(ContentExtensionError::JSONInvalidCSSDisplayNoneActionType);
    222225
    223226        String selectorString = asString(selector)->value(&exec);
    224227        if (!isValidSelector(selectorString)) {
    225228            // Skip rules with invalid selectors to be backwards-compatible.
    226             validSelector = false;
    227             return { };
     229            return {std::nullopt};
    228230        }
    229         action = Action(ActionType::CSSDisplayNoneSelector, selectorString);
    230     } else if (actionType == "make-https") {
    231         action = ActionType::MakeHTTPS;
    232     } else
    233         return ContentExtensionError::JSONInvalidActionType;
    234 
    235     return { };
    236 }
    237 
    238 static std::error_code loadRule(ExecState& exec, const JSObject& ruleObject, Vector<ContentExtensionRule>& ruleList)
    239 {
    240     Trigger trigger;
    241     auto triggerError = loadTrigger(exec, ruleObject, trigger);
    242     if (triggerError)
    243         return triggerError;
    244 
    245     Action action;
    246     bool validSelector;
    247     auto actionError = loadAction(exec, ruleObject, action, validSelector);
    248     if (actionError)
    249         return actionError;
    250 
    251     if (validSelector)
    252         ruleList.append(ContentExtensionRule(trigger, action));
    253     return { };
    254 }
    255 
    256 static std::error_code loadEncodedRules(ExecState& exec, const String& rules, Vector<ContentExtensionRule>& ruleList)
     231        return {Action(ActionType::CSSDisplayNoneSelector, selectorString)};
     232    }
     233    if (actionType == "make-https")
     234        return {{ActionType::MakeHTTPS}};
     235    return makeUnexpected(ContentExtensionError::JSONInvalidActionType);
     236}
     237
     238static Expected<std::optional<ContentExtensionRule>, std::error_code> loadRule(ExecState& exec, const JSObject& ruleObject)
     239{
     240    auto trigger = loadTrigger(exec, ruleObject);
     241    if (!trigger.hasValue())
     242        return makeUnexpected(trigger.error());
     243
     244    auto action = loadAction(exec, ruleObject);
     245    if (!action.hasValue())
     246        return makeUnexpected(action.error());
     247
     248    if (action.value())
     249        return {{{WTFMove(trigger.value()), WTFMove(action.value().value())}}};
     250
     251    return {std::nullopt};
     252}
     253
     254static Expected<Vector<ContentExtensionRule>, std::error_code> loadEncodedRules(ExecState& exec, String&& ruleJSON)
    257255{
    258256    VM& vm = exec.vm();
     
    260258
    261259    // FIXME: JSONParse should require callbacks instead of an ExecState.
    262     const JSValue decodedRules = JSONParse(&exec, rules);
     260    const JSValue decodedRules = JSONParse(&exec, ruleJSON);
    263261
    264262    if (scope.exception() || !decodedRules)
    265         return ContentExtensionError::JSONInvalid;
     263        return makeUnexpected(ContentExtensionError::JSONInvalid);
    266264
    267265    if (!decodedRules.isObject())
    268         return ContentExtensionError::JSONTopLevelStructureNotAnObject;
     266        return makeUnexpected(ContentExtensionError::JSONTopLevelStructureNotAnObject);
    269267
    270268    const JSObject* topLevelObject = decodedRules.toObject(&exec);
    271269    if (!topLevelObject || scope.exception())
    272         return ContentExtensionError::JSONTopLevelStructureNotAnObject;
     270        return makeUnexpected(ContentExtensionError::JSONTopLevelStructureNotAnObject);
    273271   
    274272    if (!isJSArray(topLevelObject))
    275         return ContentExtensionError::JSONTopLevelStructureNotAnArray;
     273        return makeUnexpected(ContentExtensionError::JSONTopLevelStructureNotAnArray);
    276274
    277275    const JSArray* topLevelArray = jsCast<const JSArray*>(topLevelObject);
    278276
    279     Vector<ContentExtensionRule> localRuleList;
     277    Vector<ContentExtensionRule> ruleList;
    280278
    281279    unsigned length = topLevelArray->length();
    282280    const unsigned maxRuleCount = 50000;
    283281    if (length > maxRuleCount)
    284         return ContentExtensionError::JSONTooManyRules;
     282        return makeUnexpected(ContentExtensionError::JSONTooManyRules);
    285283    for (unsigned i = 0; i < length; ++i) {
    286284        const JSValue value = topLevelArray->getIndex(&exec, i);
    287285        if (scope.exception() || !value)
    288             return ContentExtensionError::JSONInvalidObjectInTopLevelArray;
     286            return makeUnexpected(ContentExtensionError::JSONInvalidObjectInTopLevelArray);
    289287
    290288        const JSObject* ruleObject = value.toObject(&exec);
    291289        if (!ruleObject || scope.exception())
    292             return ContentExtensionError::JSONInvalidRule;
    293 
    294         auto error = loadRule(exec, *ruleObject, localRuleList);
    295         if (error)
    296             return error;
    297     }
    298 
    299     ruleList = WTFMove(localRuleList);
    300     return { };
    301 }
    302 
    303 std::error_code parseRuleList(const String& rules, Vector<ContentExtensionRule>& ruleList)
     290            return makeUnexpected(ContentExtensionError::JSONInvalidRule);
     291
     292        auto rule = loadRule(exec, *ruleObject);
     293        if (!rule.hasValue())
     294            return makeUnexpected(rule.error());
     295        if (rule.value())
     296            ruleList.append(*rule.value());
     297    }
     298
     299    return WTFMove(ruleList);
     300}
     301
     302Expected<Vector<ContentExtensionRule>, std::error_code> parseRuleList(String&& ruleJSON)
    304303{
    305304#if CONTENT_EXTENSIONS_PERFORMANCE_REPORTING
     
    312311
    313312    ExecState* exec = globalObject->globalExec();
    314     auto error = loadEncodedRules(*exec, rules, ruleList);
     313    auto ruleList = loadEncodedRules(*exec, WTFMove(ruleJSON));
    315314
    316315    vm = nullptr;
    317316
    318     if (error)
    319         return error;
    320 
    321     if (ruleList.isEmpty())
    322         return ContentExtensionError::JSONContainsNoRules;
     317    if (!ruleList.hasValue())
     318        return makeUnexpected(ruleList.error());
     319
     320    if (ruleList->isEmpty())
     321        return makeUnexpected(ContentExtensionError::JSONContainsNoRules);
    323322
    324323#if CONTENT_EXTENSIONS_PERFORMANCE_REPORTING
     
    327326#endif
    328327
    329     return { };
     328    return WTFMove(*ruleList);
    330329}
    331330
  • trunk/Source/WebCore/contentextensions/ContentExtensionParser.h

    r208179 r213322  
    2929
    3030#include <system_error>
     31#include <wtf/Expected.h>
    3132#include <wtf/Forward.h>
    3233#include <wtf/Vector.h>
     
    3839class ContentExtensionRule;
    3940
    40 std::error_code parseRuleList(const String& rules, Vector<ContentExtensionRule>&);
     41Expected<Vector<ContentExtensionRule>, std::error_code> parseRuleList(String&&);
    4142
    4243} // namespace ContentExtensions
  • trunk/Source/WebCore/contentextensions/ContentExtensionRule.cpp

    r194386 r213322  
    3333namespace ContentExtensions {
    3434
    35 ContentExtensionRule::ContentExtensionRule(const Trigger& trigger, const Action& action)
    36     : m_trigger(trigger)
    37     , m_action(action)
     35ContentExtensionRule::ContentExtensionRule(Trigger&& trigger, Action&& action)
     36    : m_trigger(WTFMove(trigger))
     37    , m_action(WTFMove(action))
    3838{
    3939    ASSERT(!m_trigger.urlFilter.isEmpty());
  • trunk/Source/WebCore/contentextensions/ContentExtensionRule.h

    r208179 r213322  
    176176class ContentExtensionRule {
    177177public:
    178     ContentExtensionRule(const Trigger&, const Action&);
     178    ContentExtensionRule(Trigger&&, Action&&);
    179179
    180180    const Trigger& trigger() const { return m_trigger; }
Note: See TracChangeset for help on using the changeset viewer.