[Webkit-unassigned] [Bug 180292] [Web App Manifest] Support fetching the app manifest

[bugzilla-daemon at webkit.org]

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

--- Comment #6 from David Quesada <david_quesada at apple.com> ---
(In reply to Joseph Pecoraro from comment #5)
> Comment on attachment 328461 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328461&action=review
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:1756
> > +		6353E1E61F91743100A34208 /* CachedApplicationManifest.h in Headers */ = {isa = PBXBuildFile; fileRef = 6353E1E41F91743100A34208 /* CachedApplicationManifest.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 
> This doesn't appear to need to be Private. Maybe you plan on exposing it
> outside of WebCore in a later patch?

Actually this doesn't need to be private. I think I needed to expose this in an earlier version of these patches, but it isn't needed anymore. I can make it Project again.

> 
> > Source/WebCore/inspector/agents/InspectorPageAgent.cpp:284
> > +#if ENABLE(APPLICATION_MANIFEST)
> > +    case ApplicationManifestResource:
> > +        break;
> > +#endif
> 
> In Web Inspector land we'd like a bugzilla bug so that we can improve
> manifest support in inspector, since currently this will cause the resource
> to show up as a general document. We can do better!
> https://webkit.org/new-inspector-bug

There's rdar://problem/35160389 with some ideas, I don't think there's a bugzilla bug yet.

> 
> > Source/WebCore/loader/DocumentLoader.cpp:1100
> > +    auto manifestLoader = std::make_unique<ApplicationManifestLoader>(*this, manifestURL, useCredentials);
> > +    auto* rawManifestLoader = manifestLoader.get();
> > +    auto callbackID = nextCallbackID++;
> > +    m_applicationManifestLoaders.set(WTFMove(manifestLoader), callbackID);
> > +
> > +    if (!rawManifestLoader->startLoading()) {
> > +        m_applicationManifestLoaders.remove(rawManifestLoader);
> > +        return 0;
> > +    }
> 
> This adds and then might immediately remove the loader from the map. Can
> this be reordered to avoid the possible double lookup? Is it important that
> the loader be in the list before startLoading?

IIRC the reason I did this was to cover the case where the manifest is loaded from the cache. If the manifest were in the cache, I thought it would be possible for DocumentLoader::finishedLoadingApplicationManifest() to be invoked from within ApplicationManifestLoader::startLoading(), so we would need to have the loader in the loader list at that point. Though I might need to go back and formally test this, since it looks like there would be issues telling the FrameLoaderClient about the manifest since it wouldn't have received 'callbackID' at this point. I think adding the equivalent of dispatch_async in notifyFinishedLoadingApplicationManifest() would fix this, since it would ensure the requester would get the return value from DocumentLoader::loadApplicationManifest() before FrameLoaderClient::finishedLoadingApplicationManifest() is called. Can I investigate this and file a followup bug, or do I need to fix this before landing this?

> 
> > Source/WebCore/loader/cache/CachedApplicationManifest.cpp:39
> > +    , m_decoder(TextResourceDecoder::create("application/manifest+json", UTF8Encoding()))
> 
> Nit: ASCIILiteral("application/manifest+json")
> 
> > Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp:491
> > +#if ENABLE(APPLICATION_MANIFEST)
> > +    else if (equalIgnoringASCIICase(name, ContentSecurityPolicyDirectiveNames::manifestSrc))
> > +        setCSPDirective<ContentSecurityPolicySourceListDirective>(name, value, m_manifestSrc);
> > +#endif
> 
> It seems bad form to add support for a new CSP attribute and not add a test
> for it. If it isn't reached yet, then it also seems weird to add code that
> isn't reachable. Is this covered by web platform tests?

There was already a test (LayoutTests/http/tests/security/contentSecurityPolicy/manifest-src-{allowed,blocked}) for the manifest-src attribute, imported from Blink. In retrospect, the way I split up these patches is pretty awkward since this code isn't reachable yet, but I have tests for this in https://bugs.webkit.org/show_bug.cgi?id=180294, which adds WebKit support for initiating loads of the manifest on demand.

-- 
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/20171205/ef128878/attachment-0001.html>