[Webkit-unassigned] [Bug 96126] Appcache fallback URL match should use the longest candidate

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=96126


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #162801|1                           |0
        is obsolete|                            |




--- Comment #5 from Alexey Proskuryakov <ap at webkit.org>  2012-09-07 10:46:45 PST ---
(From update of attachment 162801)
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".

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.