[webkit-reviews] review denied: [Bug 177973] Add a class for parsing application manifests : [Attachment 328171] Patch v4

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Dec 4 14:46:07 PST 2017


Geoffrey Garen <ggaren 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 328171: Patch v4

https://bugs.webkit.org/attachment.cgi?id=328171&action=review




--- Comment #9 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 328171
  --> https://bugs.webkit.org/attachment.cgi?id=328171
Patch v4

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

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:41
> +static JSC::VM* globalVM()

It's overkill to dedicate a JavaScript VM to application manifest parsing --
unless we have a really good reason to do our parsing in parallel with other
WebCore work.

You can use WebCore::commonVM() instead.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:52
> +    static JSC::JSGlobalObject* sharedGlobalObject;

Also feels like overkill to keep a global object around permanently just
because you parsed a manifest once. That's a memory leak. I know that Brady
asked for some caching, but I don't think the speed benefit justifies the
permanent memory cost.

A better option, which resolves all concerns, is to use wtf/JSONValues.h to do
the JSON parsing without a JavaScript environment. Let's make that change. It
will simplify this code.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:118
> +void ApplicationManifestParser::logDeveloperWarning(const String& message)

Some of the messages you pass to this function don't have enough context to
explain themselves. For example, "The start URL is not within scope of the
provided scope URL" doesn't even say that you're parsing an application
manifest. I think you want a helper function that prefixes everything with
"Parsing application manifest <manifestURL>: <message>".


More information about the webkit-reviews mailing list