[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

Joseph Pecoraro <joepeck at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |joepeck at webkit.org

--- Comment #2 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 324697
  --> https://bugs.webkit.org/attachment.cgi?id=324697
v1

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

> Source/JavaScriptCore/ChangeLog:9
> +        * Configurations/FeatureDefines.xcconfig: Add ENABLE_APPLICATION_MANIFEST feature flag.

New features like ENABLE_APPLICATION_MANIFEST are typically announced on webkit-dev to let others know it is being worked on.

Likewise, you should probably update the "Web App Manifest" section of Source/WebCore/features.json.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:71
> +        manifest = JSONParse(exec, String("{}"));

It seems a bit overkill to construct an empty object using JSONParse. Maybe this could just be `manifest = constructEmptyObject(exec);`.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:76
> +        manifest = JSONParse(exec, String("{}"));

Ditto.

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

Nit: "must be same origin" => "must be the same origin"

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:135
> +    return parseGenericString(manifest, "name");

Nit: To simplify `const char*` to `WTF::String` conversion here, you can wrap the literal like so:

    return parseGenericString(manifest, ASCIILiteral("name"));

This avoids a copy. You can do the same elsewhere in the patch, for example all calls to `logDeveloperWarning`.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.h:32
> +#include <wtf/Variant.h>

This include doesn't appear to be used.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.h:38
> +class VM;

This forward declaration doesn't appear to be used.

-- 
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/20171024/b74b9281/attachment.html>