Bug 180292

Summary: [Web App Manifest] Support fetching the app manifest
Product: WebKit Reporter: David Quesada <david_quesada>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, dbates, esprehn+autocc, ews, ggaren, gyuyoung.kim, japhet, joepeck, mkwst, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=153152
Attachments:
Description Flags
WIP Patch
ggaren: review+
Patch for landing
none
Patch for landing (v2) none

Description David Quesada 2017-12-01 15:53:31 PST
WebCore needs to know how to look for the manifest URL on a page and fetch it
Comment 1 David Quesada 2017-12-01 16:09:47 PST
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.
Comment 2 Geoffrey Garen 2017-12-04 15:30:02 PST
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().
Comment 3 David Quesada 2017-12-04 15:44:40 PST
(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.
Comment 4 David Quesada 2017-12-05 10:18:43 PST
Created attachment 328461 [details]
Patch for landing
Comment 5 Joseph Pecoraro 2017-12-05 11:02:45 PST
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?
Comment 6 David Quesada 2017-12-05 13:42:40 PST
(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.
Comment 7 Joseph Pecoraro 2017-12-05 14:02:58 PST
> 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!
Comment 8 David Quesada 2017-12-05 17:13:25 PST
Created attachment 328530 [details]
Patch for landing (v2)
Comment 9 WebKit Commit Bot 2017-12-05 18:41:24 PST
Comment on attachment 328530 [details]
Patch for landing (v2)

Clearing flags on attachment: 328530

Committed r225564: <https://trac.webkit.org/changeset/225564>
Comment 10 Radar WebKit Bug Importer 2017-12-06 16:25:21 PST
<rdar://problem/35895937>
Comment 11 Radar WebKit Bug Importer 2017-12-06 16:25:21 PST
<rdar://problem/35895938>