[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 #5 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 328461
  --> https://bugs.webkit.org/attachment.cgi?id=328461
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?

> 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

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

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

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