Bug 96126 – Appcache fallback URL match should use the longest candidate

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Leo Yang]

http://www.whatwg.org/specs/web-apps/current-work/multipage/offline.html#concept-appcache-matches-fallback states the match as following:

A URL matches a fallback namespace if there exists a relevant application cache whose manifest's URL has the same origin as the URL in question, and that has a fallback namespace that is a prefix match for the URL being examined. If multiple fallback namespaces match the same URL, the longest one is the one that matches. A URL looking for a fallback namespace can match more than one application cache at a time, but only matches one namespace in each cache.

[Leo Yang]

Created attachment 162801 [details]
Patch

[Yong Li]

Comment on attachment 162801 [details]
Patch

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

basically lgtm. but we should get some AppCache experts involved.

> Source/WebCore/loader/appcache/ApplicationCache.cpp:42
> +    bool operator()(const std::pair<KURL, KURL>& lhs, const std::pair<KURL, KURL>& rhs)

static bool operator() is better? If not static, it should be const.

[Yong Li]

+Tony who reviewed r121062.

[Leo Yang]

Created attachment 162812 [details]
Patch v2

Following Yong's comment -- making the operator const.

[Alexey Proskuryakov]

Comment on attachment 162801 [details]
Patch

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

Looks good to me. Since there are quite a few comments for such a small patch, I'd like to do another quick round of review on an updated one.

> LayoutTests/http/tests/appcache/resources/fallback.manifest:4
> +/resources/ empty.txt
>  /resources/network-simulator.php? simple.txt

This is a bit subtle - it would be very hard to tell what exactly failed from an error output of this test. If I try to run this updated test with current trunk, it just says "Loading an URL from fallback namespace when network is disabled returned unexpected response".

Not strictly a requirement for future r+, but please consider separating this new test from testing basic fallback functionality, or perhaps the test to have more informative failure output.

> Source/WebCore/ChangeLog:14
> +        http://www.whatwg.org/specs/web-apps/current-work/multipage/offline.html#concept-appcache-matches-fallback
> +        A URL matches a fallback namespace if there exists a relevant application cache
> +        whose manifest's URL has the same origin as the URL in question, and that has a
> +        fallback namespace that is a prefix match for the URL being examined. If multiple
> +        fallback namespaces match the same URL, the longest one is the one that matches.
> +        A URL looking for a fallback namespace can match more than one application cache
> +        at a time, but only matches one namespace in each cache.

I'd omit this whole explanation, bug title is explanatory enough.

> Source/WebCore/ChangeLog:24
> +        (WebCore):

It's best to remove unhelpful lines like this. There is no way it would ever help anyone reading the ChangeLog.

> Source/WebCore/loader/appcache/ApplicationCache.cpp:34
> +

This blank line shouldn't be here.

> Source/WebCore/loader/appcache/ApplicationCache.cpp:42
> +struct FallbackURLGreaterThan {
> +    bool operator()(const std::pair<KURL, KURL>& lhs, const std::pair<KURL, KURL>& rhs)

Please just use a "static inline bool" function.

Also, the name would be more helpful if it said what the function was actually doing, e.g. "FallbackURLLongerThan".

[Yong Li]

Comment on attachment 162812 [details]
Patch v2

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

> Source/WebCore/loader/appcache/ApplicationCache.cpp:173
>  void ApplicationCache::setFallbackURLs(const FallbackURLVector& fallbackURLs)
>  {
>      ASSERT(m_fallbackURLs.isEmpty());
>      m_fallbackURLs = fallbackURLs;
> +    std::sort(m_fallbackURLs.begin(), m_fallbackURLs.end(), FallbackURLGreaterThan());
>  }
>  

A silly question: would a std::stable_sort be better here? it will be slower, but keep the original order unchanged as long as possible

[Yong Li]

(In reply to comment #5)
> (From update of attachment 162801 [details])

> > Source/WebCore/loader/appcache/ApplicationCache.cpp:42
> > +struct FallbackURLGreaterThan {
> > +    bool operator()(const std::pair<KURL, KURL>& lhs, const std::pair<KURL, KURL>& rhs)
> 
> Please just use a "static inline bool" function.
> 

I would suggest the same, but not sure if it can be built with all STL providers and compilers.

[Leo Yang]

(In reply to comment #6)
> (From update of attachment 162812 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162812&action=review
> 
> > Source/WebCore/loader/appcache/ApplicationCache.cpp:173
> >  void ApplicationCache::setFallbackURLs(const FallbackURLVector& fallbackURLs)
> >  {
> >      ASSERT(m_fallbackURLs.isEmpty());
> >      m_fallbackURLs = fallbackURLs;
> > +    std::sort(m_fallbackURLs.begin(), m_fallbackURLs.end(), FallbackURLGreaterThan());
> >  }
> >  
> 
> A silly question: would a std::stable_sort be better here? it will be slower, but keep the original order unchanged as long as possible

Unless there are 2 same namespaces with different fallback urls.

[Leo Yang]

(In reply to comment #7)
> (In reply to comment #5)
> > (From update of attachment 162801 [details] [details])
> 
> > > Source/WebCore/loader/appcache/ApplicationCache.cpp:42
> > > +struct FallbackURLGreaterThan {
> > > +    bool operator()(const std::pair<KURL, KURL>& lhs, const std::pair<KURL, KURL>& rhs)
> > 
> > Please just use a "static inline bool" function.
> > 
> 
> I would suggest the same, but not sure if it can be built with all STL providers and compilers.

Because we are going to use a function pointer, I think inline wouldn't help. And as Yong said, not sure if some compiler would complain.

[Alexey Proskuryakov]

That's a good idea. We already use std::stable_sort in cross platform code, so it should be fine.

Effect of switching stable sort are worth testing in a regression test, too.

> Because we are going to use a function pointer, I think inline wouldn't help. And as Yong said, not sure if some compiler would complain.

std::sort does not use the function as a pointer, it uses its argument as a generic functor. You might be thinking of qsort() here.

[Leo Yang]

Created attachment 162845 [details]
Patch v3

[Alexey Proskuryakov]

Comment on attachment 162845 [details]
Patch v3

The change from sort to stable_sort is not tested AFAICT. Could you add a test?

In fact, I'm not sure what the right behavior is - is it the first or the last identical entry that wins? If we are doing it wrong, then this patch doesn't change it, and we don't need a test. But we need a FIXME then.

[Leo Yang]

(In reply to comment #12)
> (From update of attachment 162845 [details])
> The change from sort to stable_sort is not tested AFAICT. Could you add a test?
> 
> In fact, I'm not sure what the right behavior is - is it the first or the last identical entry that wins? If we are doing it wrong, then this patch doesn't change it, and we don't need a test. But we need a FIXME then.

Neither me. I'll add a FIXME.

[Leo Yang]

Created attachment 162855 [details]
Patch v4

[Alexey Proskuryakov]

Comment on attachment 162855 [details]
Patch v4

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

Thank you for updating the patch!

> Source/WebCore/loader/appcache/ApplicationCache.cpp:169
> +    std::stable_sort(m_fallbackURLs.begin(), m_fallbackURLs.end(), &fallbackURLLongerThan);

Sorry, I didn't notice this at first. Please remove the "&" sign.

[Leo Yang]

Created attachment 162860 [details]
Patch v5

[WebKit Review Bot]

Comment on attachment 162860 [details]
Patch v5

Clearing flags on attachment: 162860

Committed r127922: <http://trac.webkit.org/changeset/127922>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.