[Webkit-unassigned] [Bug 177973] Add a class for parsing application manifests

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=177973

--- Comment #6 from David Quesada <david_quesada at apple.com> ---
(In reply to Brady Eidson from comment #5)
> Comment on attachment 324726 [details]
> 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.

I originally chose to do this based on this wording from the spec: “When instructed to Trim(x), a user agent must behave as if [ECMASCRIPT]'s String.prototype.trim() function had been called on the string x.”

This actually matters in a few weird places where non-string could be passed to Trim(). But I’ll do this by executing the same steps without exposing the function from JavaScriptCore.

> 
> > 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.

I can’t think of any reason this would be called off the main thread, so it seems reasonable to share a global object. I’ll do this.

> 
> > 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."

I’ll change all the messages to be more descriptive.

> 
> > 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?)

The spec says to log a warning “ If Type(manifest) is not "object": “. If the manifest data is a string, bool, or number, we would want to log a warning since we expect the top-level value to be an object.

> 
> 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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20171026/35934c02/attachment-0001.html>