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

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


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

Geoffrey Garen <ggaren at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ggaren at apple.com
 Attachment #328171|review?, commit-queue?      |review-, commit-queue-
              Flags|                            |

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

-- 
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/20171204/bf94d911/attachment.html>


More information about the webkit-unassigned mailing list