[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 #10 from David Quesada <david_quesada at apple.com> ---
(In reply to Geoffrey Garen from comment #9)
> Comment on attachment 328171 [details]
> 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.

Will do.

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

I'll change to use this wording.

-- 
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/08a9f6a0/attachment-0001.html>