[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 #3 from David Quesada <david_quesada at apple.com> ---
(In reply to Joseph Pecoraro from comment #2)
> Comment on attachment 324697 [details]
> 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.

I'll do these.

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

Good idea. I didn't notice constructEmptyObject().

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

I'll fix these nits and remove the unused include/declaration. Thanks for the comments.

-- 
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/202a2a33/attachment-0001.html>