[webkit-reviews] review denied: [Bug 177973] Add a class for parsing application manifests : [Attachment 324726] Patch v2

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 25 15:02:16 PDT 2017


Brady Eidson <beidson at apple.com> has denied David Quesada
<david_quesada at apple.com>'s request for review:
Bug 177973: Add a class for parsing application manifests
https://bugs.webkit.org/show_bug.cgi?id=177973

Attachment 324726: Patch v2

https://bugs.webkit.org/attachment.cgi?id=324726&action=review




--- Comment #5 from Brady Eidson <beidson at apple.com> ---
Comment on attachment 324726
  --> https://bugs.webkit.org/attachment.cgi?id=324726
Patch v2

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

> Source/JavaScriptCore/runtime/StringPrototype.h:75
> +JS_EXPORT_PRIVATE JSValue jsTrimString(ExecState*, JSValue thisValue, int
trimKind);

There's no need to expose this.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:61
> +    m_globalObject = JSC::JSGlobalObject::create(*vm,
JSC::JSGlobalObject::createStructure(*vm, JSC::jsNull()));

We're creating a global object each time parseManifest is called? That's a bit
of an extreme expense. Let's not recreate it each time.

Maybe parseManifest is only called once, so it's not like we're recreating
m_globalObject? If so, you could just make it be a local global object.

An added optimization; Is this all main-thread only code? If so, all
ApplicationManifestParser's could share one global object.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:71
> +	   logDeveloperWarning(ASCIILiteral("The application manifest must be
valid JSON data."));

I think dev error warnings are usually descriptive, not prescriptive.

I'd suggest "The application manifest is not valid JSON data."

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:76
> +	   logDeveloperWarning(ASCIILiteral("The application manifest must be a
JSON object."));

The error condition here isn't that it's not "a JSON object", but that the
result is of JSON parsing is not even a Javascript object. (Is that possible?)

Also, describe the error, not the solution.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:91
> +    logDeveloperWarning("The \"" + propertyName + "\" application manifest
property must be a string.");

"...is not a string"

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:96
> +    logDeveloperWarning("The \"" + propertyName + "\" application manifest
property must be a valid URL.");

"...is not a valid URL"

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:126
> +    auto startURLOrigin = SecurityOrigin::create(startURL);
> +    auto documentOrigin = SecurityOrigin::create(documentURL);
> +
> +    if (!startURLOrigin->isSameOriginAs(documentOrigin.get())) {

Origins are pretty heavyweight objects and should not be used for string origin
comparisons.

For that we have protocolHostAndPortAreEqual()

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:127
> +	   logDeveloperWarning(ASCIILiteral("The \"start_url\" application
manifest property must be the same origin as the document."));

A "URL" is not an "origin", therefore equating a URL value in the message to an
origin is a bit nonsensical.

Perhaps: "The origin of the "start_url" property is not in the same origin as
the document"

Additionally, you have both origins here. The message could read:
"The start_url's origin of "http://www.apple.com/" is different from the
document's origin of "https://google.com""

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:167
> +    auto scopeOrigin = SecurityOrigin::create(scopeURL);
> +    auto targetOrigin = SecurityOrigin::create(targetURL);

See above about SecurityOrigins.

We can just do comparisons on the URLs.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:197
> +    auto scopeURLOrigin = SecurityOrigin::create(scopeURL);
> +    auto documentOrigin = SecurityOrigin::create(documentURL);
> +
> +    if (!scopeURLOrigin->isSameOriginAs(documentOrigin.get())) {

Ditto about origins.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:198
> +	   logDeveloperWarning(ASCIILiteral("The \"scope\" application manifest
property must be same origin as the document."));

See above about formatting these messages.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:203
> +	   logDeveloperWarning(ASCIILiteral("The \"scope\" application manifest
property must be within scope of the start URL."));

See above about formatting these messages.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:221
> +    return asString(JSC::jsTrimString(exec, value, JSC::TrimLeft |
JSC::TrimRight))->tryGetValue();

There's no need to expose the JS internal trimString just to use it here.

Instead I would simply make the WTFString then call stripWhitespace() on it.


More information about the webkit-reviews mailing list