Unify the fragment handling in OfflinePageModelQuery for original/final urls. |
|
Issue descriptionDuring the refactoring of OfflinePageModelImpl, the GetPagesByURLs will be refactored to use OfflinePageModelQuery for the purpose of unifying APIs. However the behavior included some cases that need to be treated with separate codepath: the |original_url| will be compared but only *without* stripping the fragments from the URL, while the |final_url| can be controlled to strip the fragments before looking for matches. The refactoring didn't resolve the issue. So the CountMatchingUrls in offline_page_model_query.cc takes the |strip_frament| as always false from OfflinePageModelQuery::Matches when it's trying to get |original_url_count|. This should be re-investigated and should be unified in API usage, probably original URLs can be matched with controllable |strip_fragment|.
,
Sep 28 2017
I believe this difference in equality comparison of the original versus final URL is indeed an error. One example where this should* cause a failure: https://crbug.com/753609#c1 redirects to https://bugs.chromium.org/p/chromium/issues/detail?id=753609#c1 If the 1st URL above should be offlined a future load attempt of https://crbug.com/753609 would *not* find and load that offline page. When fixing this issue the OnGetPagesByURLDone method in offline_page_utils.cc should also be updated. In fact, all occurrences of OfflinePageUtils::EqualsIgnoringFragment should be investigated to make sure there's no other places that are also affected. *: I haven't tested it but looking at OnGetPagesByURLDone in offline_page_utils.cc it seems to be the case. |
|
►
Sign in to add a comment |
|
Comment 1 by romax@chromium.org
, Aug 9 2017