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.
Created attachment 162801 [details] Patch
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.
+Tony who reviewed r121062.
Created attachment 162812 [details] Patch v2 Following Yong's comment -- making the operator const.
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".
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
(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.
(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.
(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.
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.
Created attachment 162845 [details] Patch v3
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.
(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.
Created attachment 162855 [details] Patch v4
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.
Created attachment 162860 [details] Patch v5
Comment on attachment 162860 [details] Patch v5 Clearing flags on attachment: 162860 Committed r127922: <http://trac.webkit.org/changeset/127922>
All reviewed patches have been landed. Closing bug.