Bug 182363 – WebAppManifest scope should default to the containing directory of start_url if 'scope' is not specified

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Quesada]

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.

[Radar WebKit Bug Importer]

<rdar://problem/37093498>

[David Quesada]

Created attachment 332910 [details]
Patch

[Ryosuke Niwa]

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.

[David Quesada]

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

[David Quesada]

Created attachment 332973 [details]
Patch v2

[Ryosuke Niwa]

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.

[David Quesada]

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

[Ryosuke Niwa]

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.

[David Quesada]

Created attachment 333011 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 333011 [details]
Patch for landing

Clearing flags on attachment: 333011

Committed r228036: <https://trac.webkit.org/changeset/228036>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.