[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

Brady Eidson <beidson at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |beidson at apple.com
 Attachment #324726|review?                     |review-
              Flags|                            |

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

-- 
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/20171025/fdba296c/attachment-0001.html>