Bug 182363

Summary: WebAppManifest scope should default to the containing directory of start_url if 'scope' is not specified
Product: WebKit Reporter: David Quesada <david_quesada>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, cdumez, commit-queue, dbates, rniwa, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch v2
rniwa: review+
Patch for landing none

Description David Quesada 2018-01-31 16:47:53 PST
See https://github.com/w3c/manifest/issues/550#issuecomment-361129547

The spec is likely going to be updated to match Chrome's behavior, which some web apps rely on.
Comment 1 Radar WebKit Bug Importer 2018-01-31 16:50:08 PST
<rdar://problem/37093498>
Comment 2 David Quesada 2018-02-01 14:17:17 PST
Created attachment 332910 [details]
Patch
Comment 3 Ryosuke Niwa 2018-02-01 15:38:17 PST
Comment on attachment 332910 [details]
Patch

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

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:211
> +    URL result { url };
> +
> +    result.removeQueryAndFragmentIdentifier();
> +
> +    auto path = result.path();
> +    if (path.endsWith('/'))
> +        return result;
> +
> +    auto lastSlash = path.reverseFind('/');
> +    if (lastSlash == WTF::notFound) {
> +        result.setPath({ });
> +        return result;
> +    }
> +
> +    result.setPath(path.substring(0, lastSlash + 1));
> +    return result;

Just do: URL(url, "./"). What's what service worker code does.
Also, we should this help function scopeURL or computeScopeURL or something.
"containing URL" is not really a nominal terminology.
Comment 4 David Quesada 2018-02-02 08:36:29 PST
(In reply to Ryosuke Niwa from comment #3)
> Comment on attachment 332910 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332910&action=review
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:211
> > +    URL result { url };
> > +
> > +    result.removeQueryAndFragmentIdentifier();
> > +
> > +    auto path = result.path();
> > +    if (path.endsWith('/'))
> > +        return result;
> > +
> > +    auto lastSlash = path.reverseFind('/');
> > +    if (lastSlash == WTF::notFound) {
> > +        result.setPath({ });
> > +        return result;
> > +    }
> > +
> > +    result.setPath(path.substring(0, lastSlash + 1));
> > +    return result;
> 
> Just do: URL(url, "./"). What's what service worker code does.

That's a lot cleaner. I'll use that instead.

> Also, we should this help function scopeURL or computeScopeURL or something.
> "containing URL" is not really a nominal terminology.
Comment 5 David Quesada 2018-02-02 08:41:26 PST
Created attachment 332973 [details]
Patch v2
Comment 6 Ryosuke Niwa 2018-02-02 14:30:03 PST
Comment on attachment 332973 [details]
Patch v2

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

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:198
> -        return { };
> +        return { startURL, "./" };

Hm... I would have preferred to have a helper function like you did earlier.
Comment 7 David Quesada 2018-02-02 14:39:24 PST
(In reply to Ryosuke Niwa from comment #6)
> Comment on attachment 332973 [details]
> Patch v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332973&action=review
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:198
> > -        return { };
> > +        return { startURL, "./" };
> 
> Hm... I would have preferred to have a helper function like you did earlier.

I'm open to adding back a helper function. How does `void defaultScopeURL(const URL& startURL)` sound?
Comment 8 Ryosuke Niwa 2018-02-02 15:02:36 PST
Comment on attachment 332973 [details]
Patch v2

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

>>> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:198
>>> +        return { startURL, "./" };
>> 
>> Hm... I would have preferred to have a helper function like you did earlier.
> 
> I'm open to adding back a helper function. How does `void defaultScopeURL(const URL& startURL)` sound?

Sure that sounds good. Alternatively, you could just compute it once at the top of the function
as defaultScopeURL local variable since this isn't a perf sensitive code.
Comment 9 David Quesada 2018-02-02 15:47:48 PST
Created attachment 333011 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2018-02-02 16:50:00 PST
Comment on attachment 333011 [details]
Patch for landing

Clearing flags on attachment: 333011

Committed r228036: <https://trac.webkit.org/changeset/228036>
Comment 11 WebKit Commit Bot 2018-02-02 16:50:02 PST
All reviewed patches have been landed.  Closing bug.