Handling original and redirect links to offline pages |
||||||||||||||||||
Issue descriptionThe OfflinePageModelImpl ensures that the URL associated with an offline page is the same as the last committed URL from the WebContents. How should we handle redirected links? Do we want to be able to call up the redirected saved page for the original link (for effective offline caching)? I encountered this issue when triggering an async load request off search page results for wikipedia that redirects to a *.m.* link. When offline I can't open the offline page from the SRP because the original link is not associated with it (it seems I need to be online to get the redirect to resolve it).
,
Jun 13 2016
,
Jun 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e759fe98d6b70f487d5f5474b00703d1ba492d53 commit e759fe98d6b70f487d5f5474b00703d1ba492d53 Author: dougarnett <dougarnett@chromium.org> Date: Thu Jun 09 20:45:16 2016 For Offliner, pass in the URL from WebContents to SavePage request instead of the request URL since redirect link will fail a URL check in OfflinePageModelImpl. This is workaround until better understand how to deal with redirected URLs for async loading. BUG= 618716 Review-Url: https://codereview.chromium.org/2052913002 Cr-Commit-Position: refs/heads/master@{#399001} [modify] https://crrev.com/e759fe98d6b70f487d5f5474b00703d1ba492d53/chrome/browser/android/offline_pages/prerendering_offliner.cc
,
Jun 15 2016
Discuss this with jianli@
,
Jun 29 2016
Indeed, for now GetLastCommittedURL() is the right way to handle that, at least consistent across features. Keeping this bug open for design tracking purposes.
,
Aug 30 2016
,
Aug 31 2016
[pke@ opened dupe 642277 on this issue wrt Zine experimenting with context menu option. I'm not sure how we deal with dupes in chromium so marked as blocking for now.] Dmitry, was your proposal that we will still only store one URL for the page but make it an option as to which endpoint URL to store with it? With per namespace config being one option?
,
Aug 31 2016
Issue 642277 has been merged into this issue.
,
Aug 31 2016
,
Aug 31 2016
The way we handle it for now is we store final, redirected url in the OfflinePageModel, and original URL in client_id.id for the namespaces that are concerned with the original URL (CCT API). While a bit asymmetrical, this seems to work ok for now. Having said that, I wonder if the bug we just resolved as dupe is actually that issue. It looks to me that we are snapshotting the page that is a redirect request. I played with results of Google Search and sometimes I get the final redirected page but often the redirect page itself (empty body, the reload actually redirects to the target). Here are my steps: 1. open www.google.com 2. search for term "search" 3. Long-press, "save later" on entry for Bing - observe actual Bing page to be loaded 4. Long-press, "save later" on other entries - observe the redirect page to be saved.
,
Aug 31 2016
Oh yes, this does look like different behavior than I originally saw for this bug. Will undupe pke's bug.
,
Oct 12 2016
,
Oct 19 2016
Jianli@: please work with fgorski on actual change, this is part of Async Load.
,
Nov 15 2016
,
Nov 15 2016
This is p1 now, since we want to launch async right?
,
Nov 15 2016
,
Nov 15 2016
This happened to block NTP. Sorry for the late notice. For non offline page related sections (e.g. bookmarks or articles for you) we cannot enforce offline version for redirecting URL, because we have only initial URL and you AFAIK have final URL.
,
Nov 16 2016
Thanks for increasing priority!
,
Nov 16 2016
,
Nov 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/093a2a9dd37c96fb55776c5415c8dd768328e4c2 commit 093a2a9dd37c96fb55776c5415c8dd768328e4c2 Author: jianli <jianli@chromium.org> Date: Wed Nov 16 21:30:25 2016 Store original request URL in offline page metadata table This will be used to support triggering redirect from original request URL to last committed URL which is used to identify the offline page. BUG= 618716 TBR=bauerb@chromium.org Review-Url: https://codereview.chromium.org/2484223005 Cr-Commit-Position: refs/heads/master@{#432632} [modify] https://crrev.com/093a2a9dd37c96fb55776c5415c8dd768328e4c2/chrome/browser/android/offline_pages/offline_page_bridge.cc [modify] https://crrev.com/093a2a9dd37c96fb55776c5415c8dd768328e4c2/chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc [modify] https://crrev.com/093a2a9dd37c96fb55776c5415c8dd768328e4c2/chrome/browser/android/offline_pages/offline_page_utils_unittest.cc [modify] https://crrev.com/093a2a9dd37c96fb55776c5415c8dd768328e4c2/chrome/browser/android/offline_pages/prerendering_offliner.cc [modify] https://crrev.com/093a2a9dd37c96fb55776c5415c8dd768328e4c2/chrome/browser/android/offline_pages/prerendering_offliner.h [modify] https://crrev.com/093a2a9dd37c96fb55776c5415c8dd768328e4c2/chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc [modify] https://crrev.com/093a2a9dd37c96fb55776c5415c8dd768328e4c2/chrome/browser/android/offline_pages/recent_tab_helper.cc [modify] https://crrev.com/093a2a9dd37c96fb55776c5415c8dd768328e4c2/chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc [modify] https://crrev.com/093a2a9dd37c96fb55776c5415c8dd768328e4c2/components/offline_pages/offline_page_item.cc [modify] https://crrev.com/093a2a9dd37c96fb55776c5415c8dd768328e4c2/components/offline_pages/offline_page_item.h [modify] https://crrev.com/093a2a9dd37c96fb55776c5415c8dd768328e4c2/components/offline_pages/offline_page_metadata_store_impl_unittest.cc [modify] https://crrev.com/093a2a9dd37c96fb55776c5415c8dd768328e4c2/components/offline_pages/offline_page_metadata_store_sql.cc [modify] https://crrev.com/093a2a9dd37c96fb55776c5415c8dd768328e4c2/components/offline_pages/offline_page_metadata_store_sql.h [modify] https://crrev.com/093a2a9dd37c96fb55776c5415c8dd768328e4c2/components/offline_pages/offline_page_model.cc [modify] https://crrev.com/093a2a9dd37c96fb55776c5415c8dd768328e4c2/components/offline_pages/offline_page_model.h [modify] https://crrev.com/093a2a9dd37c96fb55776c5415c8dd768328e4c2/components/offline_pages/offline_page_model_impl.cc [modify] https://crrev.com/093a2a9dd37c96fb55776c5415c8dd768328e4c2/components/offline_pages/offline_page_model_impl.h [modify] https://crrev.com/093a2a9dd37c96fb55776c5415c8dd768328e4c2/components/offline_pages/offline_page_model_impl_unittest.cc [modify] https://crrev.com/093a2a9dd37c96fb55776c5415c8dd768328e4c2/components/offline_pages/stub_offline_page_model.cc [modify] https://crrev.com/093a2a9dd37c96fb55776c5415c8dd768328e4c2/components/offline_pages/stub_offline_page_model.h
,
Nov 17 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/610b1fdb73c904e2629f8730b55909c06fd85cf7 commit 610b1fdb73c904e2629f8730b55909c06fd85cf7 Author: jianli <jianli@chromium.org> Date: Thu Nov 17 23:17:52 2016 Support getting offline pages also by original URL BUG= 618716 Review-Url: https://codereview.chromium.org/2503853004 Cr-Commit-Position: refs/heads/master@{#433013} [modify] https://crrev.com/610b1fdb73c904e2629f8730b55909c06fd85cf7/chrome/browser/android/offline_pages/offline_page_bridge.cc [modify] https://crrev.com/610b1fdb73c904e2629f8730b55909c06fd85cf7/chrome/browser/android/offline_pages/offline_page_request_job.cc [modify] https://crrev.com/610b1fdb73c904e2629f8730b55909c06fd85cf7/chrome/browser/android/offline_pages/offline_page_request_job.h [modify] https://crrev.com/610b1fdb73c904e2629f8730b55909c06fd85cf7/chrome/browser/android/offline_pages/offline_page_tab_helper.cc [modify] https://crrev.com/610b1fdb73c904e2629f8730b55909c06fd85cf7/chrome/browser/android/offline_pages/offline_page_tab_helper.h [modify] https://crrev.com/610b1fdb73c904e2629f8730b55909c06fd85cf7/chrome/browser/android/offline_pages/offline_page_utils.cc [modify] https://crrev.com/610b1fdb73c904e2629f8730b55909c06fd85cf7/chrome/browser/android/offline_pages/offline_page_utils.h [modify] https://crrev.com/610b1fdb73c904e2629f8730b55909c06fd85cf7/components/offline_pages/offline_page_metadata_store_impl_unittest.cc [modify] https://crrev.com/610b1fdb73c904e2629f8730b55909c06fd85cf7/components/offline_pages/offline_page_model.h [modify] https://crrev.com/610b1fdb73c904e2629f8730b55909c06fd85cf7/components/offline_pages/offline_page_model_impl.cc [modify] https://crrev.com/610b1fdb73c904e2629f8730b55909c06fd85cf7/components/offline_pages/offline_page_model_impl.h [modify] https://crrev.com/610b1fdb73c904e2629f8730b55909c06fd85cf7/components/offline_pages/offline_page_model_impl_unittest.cc [modify] https://crrev.com/610b1fdb73c904e2629f8730b55909c06fd85cf7/components/offline_pages/stub_offline_page_model.cc [modify] https://crrev.com/610b1fdb73c904e2629f8730b55909c06fd85cf7/components/offline_pages/stub_offline_page_model.h
,
Nov 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/55994375eb39e2965999474fb3612737626bed13 commit 55994375eb39e2965999474fb3612737626bed13 Author: jianli <jianli@chromium.org> Date: Fri Nov 18 22:41:34 2016 Trigger redirect for offline pages If original request URL is found, we will use it to trigger redirect such that offline page for final redirected URL could be served. BUG= 618716 Review-Url: https://codereview.chromium.org/2506293002 Cr-Commit-Position: refs/heads/master@{#433318} [modify] https://crrev.com/55994375eb39e2965999474fb3612737626bed13/chrome/browser/android/offline_pages/offline_page_request_job.cc [modify] https://crrev.com/55994375eb39e2965999474fb3612737626bed13/chrome/browser/android/offline_pages/offline_page_request_job.h [modify] https://crrev.com/55994375eb39e2965999474fb3612737626bed13/chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc
,
Nov 21 2016
This change landed after BP. I guess we have to merge it back to M56, right?
,
Nov 21 2016
Before BP Offline Pages team ensured me that they would merge this. Thus, what is the expected timeline? Without this merge, NTP is in a fairly broken state, when offline badges are shown, but, when clicked, *online* version is opened.
,
Nov 21 2016
,
Nov 21 2016
I think jianli@ already landed most of the patches on trunk and he should start merging it today, unless he is OOF. Based on his code reviews, there is one outstanding patch that need landing on trunk, related to capturing UMA for the redirection. https://codereview.chromium.org/user/jianli
,
Nov 22 2016
The last patch 433318 needs to be merged to M56.
,
Nov 22 2016
Your change meets the bar and is auto-approved for M56 (branch: 2924)
,
Nov 22 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1210efb30b84501b546b16a45a3a8c544ebd7f3e commit 1210efb30b84501b546b16a45a3a8c544ebd7f3e Author: Jian Li <jianli@chromium.org> Date: Tue Nov 22 01:05:16 2016 Merge to M56: Trigger redirect for offline pages If original request URL is found, we will use it to trigger redirect such that offline page for final redirected URL could be served. BUG= 618716 Review-Url: https://codereview.chromium.org/2506293002 Cr-Commit-Position: refs/heads/master@{#433318} (cherry picked from commit 55994375eb39e2965999474fb3612737626bed13) Review URL: https://codereview.chromium.org/2523603002 . Cr-Commit-Position: refs/branch-heads/2924@{#48} Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059} [modify] https://crrev.com/1210efb30b84501b546b16a45a3a8c544ebd7f3e/chrome/browser/android/offline_pages/offline_page_request_job.cc [modify] https://crrev.com/1210efb30b84501b546b16a45a3a8c544ebd7f3e/chrome/browser/android/offline_pages/offline_page_request_job.h [modify] https://crrev.com/1210efb30b84501b546b16a45a3a8c544ebd7f3e/chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc
,
Nov 22 2016
|
||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||
Comment 1 by bugdroid1@chromium.org
, Jun 9 2016