[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