Bug 180292 – [Web App Manifest] Support fetching the app manifest

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[David Quesada]

WebCore needs to know how to look for the manifest URL on a page and fetch it

[David Quesada]

Created attachment 328176 [details]
WIP Patch

My WIP patch for review. This builds on 177973, so it doesn't apply right now, but I thought I'd post the patch for feedback. Particularly with the architecture of representing outstanding requests for the app manifest as ApplicationManifestLoaders attached to the DocumentLoader. I didn't see any precedent for a way to load subresources of a document on demand in a way they could be exposed to an embedder, other than IconLoader, so I used that as inspiration.

[Geoffrey Garen]

Comment on attachment 328176 [details]
WIP Patch

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

r=me with some comments

> Source/WebCore/loader/DocumentLoader.cpp:1087
> +    for (const auto& link : childrenOfType<HTMLLinkElement>(*head)) {
> +        if (link.isApplicationManifest()) {
> +            manifestURL = link.href();
> +            useCredentials = equalIgnoringASCIICase(link.attributeWithoutSynchronization(HTMLNames::crossoriginAttr), "use-credentials");
> +            break;
> +        }
> +    }

Is it specified that multiple manifest links should only result in loading the first link? If so, can you add a test for this?

> Source/WebCore/loader/DocumentLoader.cpp:1089
> +    if (manifestURL.isNull() || manifestURL.isEmpty() || !manifestURL.isValid())

isEmpty() is a superset of isNull() so you should remove the check for isNull().

[David Quesada]

(In reply to Geoffrey Garen from comment #2)
> Comment on attachment 328176 [details]
> WIP Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328176&action=review
> 
> r=me with some comments
> 
> > Source/WebCore/loader/DocumentLoader.cpp:1087
> > +    for (const auto& link : childrenOfType<HTMLLinkElement>(*head)) {
> > +        if (link.isApplicationManifest()) {
> > +            manifestURL = link.href();
> > +            useCredentials = equalIgnoringASCIICase(link.attributeWithoutSynchronization(HTMLNames::crossoriginAttr), "use-credentials");
> > +            break;
> > +        }
> > +    }
> 
> Is it specified that multiple manifest links should only result in loading
> the first link? If so, can you add a test for this?

Yes (https://www.w3.org/TR/appmanifest/#h-note-5) and yes. Since I can't test it with only the changes in this patch, I'll add it to 180294.
> 
> > Source/WebCore/loader/DocumentLoader.cpp:1089
> > +    if (manifestURL.isNull() || manifestURL.isEmpty() || !manifestURL.isValid())
> 
> isEmpty() is a superset of isNull() so you should remove the check for
> isNull().

Will do.

[David Quesada]

Created attachment 328461 [details]
Patch for landing

[Joseph Pecoraro]

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?

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

[David Quesada]

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

[Joseph Pecoraro]

> Can I investigate this and file a followup bug, or do I need to fix
> this before landing this?

Sure. There is nothing incorrect about the current code, it just seems like it may be inefficient if its not necessary.

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

Excellent! Thanks!

[David Quesada]

Created attachment 328530 [details]
Patch for landing (v2)

[WebKit Commit Bot]

Comment on attachment 328530 [details]
Patch for landing (v2)

Clearing flags on attachment: 328530

Committed r225564: <https://trac.webkit.org/changeset/225564>

[Radar WebKit Bug Importer]

<rdar://problem/35895937>

[Radar WebKit Bug Importer]

<rdar://problem/35895938>