Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1721103002:
Switch between online and offline version per network connection (Closed)
DescriptionSwitch between online and offline version of saved page per network connection:
If the user is navigating to an online version of saved page and there is no network connection, we should load the offline copy instead.
If the user is loading an offline copy and there is network connection, we should load the online version instead.
BUG=584417
Committed: https://crrev.com/03b1a6f719fc5016260be23a6d8593d89e06da68
Cr-Commit-Position: refs/heads/master@{#378073}
Patch Set 1 : Patch #
Total comments: 3
Patch Set 2 : Patch #
Total comments: 10
Patch Set 3 : Address feedback #
Total comments: 17
Patch Set 4 : Fix #
Total comments: 4
Patch Set 5 : Changes #
Messages
Total messages: 33 (13 generated)
Patchset #1 (id:1) has been deleted
petewil@chromium.org changed reviewers: + petewil@chromium.org
https://codereview.chromium.org/1721103002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/1721103002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:21: return connection != net::NetworkChangeNotifier::CONNECTION_NONE && Let's replace this IsOnline check with NetworkChangeNotifier.isOnline(). It turns out that BLUETOOTH actually means connnected to the internet via BLUETOOTH, not just paired with a watch or earphones. https://codereview.chromium.org/1721103002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:41: if (!IsOnline() || Why return early if we are offline, are we intentionally deferring until the DidFailProvisionalLoad() callback? I wonder if the "!" is incorrect here.
https://codereview.chromium.org/1721103002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/1721103002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:41: if (!IsOnline() || On 2016/02/23 23:13:47, Pete Williamson wrote: > Why return early if we are offline, are we intentionally deferring until the > DidFailProvisionalLoad() callback? I wonder if the "!" is incorrect here. Ah, nevermind, I see what is going on now, this is to load the online version of a page if we enter the offline URL but are online.
Description was changed from ========== Switch between online and offline version per network connection BUG= ========== to ========== Switch between online and offline version of saved page per network connection: If the user is navigating to an online version of saved page and there is no network connection, we should load the offline copy instead. If the user is loading an offline copy and there is network connection, we should load the online version instead. BUG=584417 ==========
jianli@chromium.org changed reviewers: + creis@chromium.org
jianli@chromium.org changed reviewers: + rkaplow@chromium.org
petewil for offline pages creis for web contents rkaplow for UMA
https://codereview.chromium.org/1721103002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/1721103002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:46: net::NetworkChangeNotifier::CONNECTION_NONE) { Why not use if (!NetworkChangeNotifier::IsOnline()) here instead? That would keep us from also depending on the CONNECTION_NONE constant. https://codereview.chromium.org/1721103002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:57: if (last_redirect_from_url_copy == online_url) What if our last succesful transfer just happened to be the same page we are loading now? Will last_redirect_from_url_copy get cleared between uses? https://codereview.chromium.org/1721103002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:61: base::ThreadTaskRunnerHandle::Get()->PostTask( So if we delay the redirect, it lets the existing page load attempt get further. Is that a problem? (I suspect not, I just wanted to double check). https://codereview.chromium.org/1721103002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:86: net::NetworkChangeNotifier::CONNECTION_NONE) { NetworkChangeNotifier::IsOnline() ? https://codereview.chromium.org/1721103002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right): https://codereview.chromium.org/1721103002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:39: void DidFailProvisionalLoad(content::RenderFrameHost* render_frame_host, nit: It might make the file easier to read if the two functions were indented the same way. I suspect the first one has to be indented the way it is because the first argument is too long, it would be nice if we could indent the second one like the first, with the first argument starting on a new line, and the others lined up beneath it.
jianli@chromium.org changed reviewers: + nasko@chromium.org - creis@chromium.org
https://codereview.chromium.org/1721103002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/1721103002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:46: net::NetworkChangeNotifier::CONNECTION_NONE) { On 2016/02/25 18:05:30, Pete Williamson wrote: > Why not use if (!NetworkChangeNotifier::IsOnline()) here instead? That would > keep us from also depending on the CONNECTION_NONE constant. Changed to call IsOffline. https://codereview.chromium.org/1721103002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:57: if (last_redirect_from_url_copy == online_url) On 2016/02/25 18:05:31, Pete Williamson wrote: > What if our last succesful transfer just happened to be the same page we are > loading now? Will last_redirect_from_url_copy get cleared between uses? last_redirect_from_url_copy is just a saved value before last_redirect_from_url_ is cleared (see line 36-37). https://codereview.chromium.org/1721103002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:61: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/02/25 18:05:31, Pete Williamson wrote: > So if we delay the redirect, it lets the existing page load attempt get further. > Is that a problem? (I suspect not, I just wanted to double check). This is to let other observers finish their work. https://codereview.chromium.org/1721103002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:86: net::NetworkChangeNotifier::CONNECTION_NONE) { On 2016/02/25 18:05:30, Pete Williamson wrote: > NetworkChangeNotifier::IsOnline() ? Changed to call IsOffline. https://codereview.chromium.org/1721103002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right): https://codereview.chromium.org/1721103002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.h:39: void DidFailProvisionalLoad(content::RenderFrameHost* render_frame_host, On 2016/02/25 18:05:31, Pete Williamson wrote: > nit: It might make the file easier to read if the two functions were indented > the same way. I suspect the first one has to be indented the way it is because > the first argument is too long, it would be nice if we could indent the second > one like the first, with the first argument starting on a new line, and the > others lined up beneath it. Done.
lgtm
A bunch of comments, mostly on the unit test. https://codereview.chromium.org/1721103002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/1721103002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:50: web_contents()->GetBrowserContext(), validated_url); Is there any way to confirm that validated_url is indeed an offline URL? You mentioned that it is a file:/// based one, so does it make sense to double check that it is at leas the file: scheme? https://codereview.chromium.org/1721103002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc (right): https://codereview.chromium.org/1721103002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:89: OfflinePageTabHelperTest::OfflinePageTabHelperTest() nit: Since this is a test class, you can inline the methods within the class itself. https://codereview.chromium.org/1721103002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:133: RunUntilIdle(); Why do you need to call RunUntilIdle() here? It only sets up a boolean, right? https://codereview.chromium.org/1721103002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:139: offline_page_tab_helper()->DidStartProvisionalLoadForFrame( This will not be compatible with PlzNavigate. You should probably use main_test_rfh()->PrepareForCommit(). https://codereview.chromium.org/1721103002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:141: RunUntilIdle(); Why is this needed? https://codereview.chromium.org/1721103002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:145: offline_page_tab_helper()->DidFailProvisionalLoad( Use main_test_rfh()->SimulateNavigationError instead. https://codereview.chromium.org/1721103002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:152: } Why do we need an empty method? https://codereview.chromium.org/1721103002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:166: } Why do we need an empty method? https://codereview.chromium.org/1721103002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:178: EXPECT_EQ(online_url, controller().GetPendingEntry()->GetURL()); I'd suggest calling main_test_rfh()->SendNavigate() to commit the navigation and ensuring the GetLastCommittedEntry()->GetURL() is your offline URL.
Also switched to using DidStartNavigation and DidFinishNavigation. https://codereview.chromium.org/1721103002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/1721103002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:50: web_contents()->GetBrowserContext(), validated_url); On 2016/02/26 00:12:19, nasko wrote: > Is there any way to confirm that validated_url is indeed an offline URL? You > mentioned that it is a file:/// based one, so does it make sense to double check > that it is at leas the file: scheme? We don't need to do this because GetOfflinePageForOfflineURL (called by GetOnlineURLForOfflineURL) has the check for the provided offline URL, including file scheme check (see https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/and...). https://codereview.chromium.org/1721103002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc (right): https://codereview.chromium.org/1721103002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:89: OfflinePageTabHelperTest::OfflinePageTabHelperTest() On 2016/02/26 00:12:19, nasko wrote: > nit: Since this is a test class, you can inline the methods within the class > itself. Done. https://codereview.chromium.org/1721103002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:133: RunUntilIdle(); On 2016/02/26 00:12:19, nasko wrote: > Why do you need to call RunUntilIdle() here? It only sets up a boolean, right? Not needed. Removed. https://codereview.chromium.org/1721103002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:139: offline_page_tab_helper()->DidStartProvisionalLoadForFrame( On 2016/02/26 00:12:19, nasko wrote: > This will not be compatible with PlzNavigate. You should probably use > main_test_rfh()->PrepareForCommit(). Done. https://codereview.chromium.org/1721103002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:141: RunUntilIdle(); On 2016/02/26 00:12:19, nasko wrote: > Why is this needed? Removed. https://codereview.chromium.org/1721103002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:145: offline_page_tab_helper()->DidFailProvisionalLoad( On 2016/02/26 00:12:19, nasko wrote: > Use main_test_rfh()->SimulateNavigationError instead. Done. https://codereview.chromium.org/1721103002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:152: } On 2016/02/26 00:12:19, nasko wrote: > Why do we need an empty method? This is needed for OfflinePageTestArchiver::Observer which is provided for creating a test offline archive. https://codereview.chromium.org/1721103002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:166: } On 2016/02/26 00:12:19, nasko wrote: > Why do we need an empty method? This is needed to call model->SavePage.
https://codereview.chromium.org/1721103002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc (right): https://codereview.chromium.org/1721103002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:54: //public content::RenderViewHostImplTestHarness, Remove commented out inheritance.
On 2016/02/26 15:42:57, nasko wrote: > https://codereview.chromium.org/1721103002/diff/80001/chrome/browser/android/... > File chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc > (right): > > https://codereview.chromium.org/1721103002/diff/80001/chrome/browser/android/... > chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:54: > //public content::RenderViewHostImplTestHarness, > Remove commented out inheritance. still looks good to me (Modulo Nasko's comments)
lgtm lgtm for histograms ( with unit) https://codereview.chromium.org/1721103002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1721103002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:31778: +<histogram name="OfflinePages.RedirectToOfflineCount"> should add a unit
Changed back to use DidFailProvisionalLoad instead of DidFinishNavigation since SimulateNavigationError does not trigger the latter. I will work on a separate patch later to switch to using DidFinishNavigation. https://codereview.chromium.org/1721103002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc (right): https://codereview.chromium.org/1721103002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:54: //public content::RenderViewHostImplTestHarness, On 2016/02/26 15:42:56, nasko wrote: > Remove commented out inheritance. Done. https://codereview.chromium.org/1721103002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1721103002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:31778: +<histogram name="OfflinePages.RedirectToOfflineCount"> On 2016/02/26 22:08:45, rkaplow wrote: > should add a unit Done.
jianli@chromium.org changed reviewers: + avi@chromium.org
avi, could you please review adding a new tab helper? thanks.
tab_helpers.cc change LGTM
LGTM with the understanding that the code will still have some changes to move to using NavigationHandle for failed navigations.
The CQ bit was checked by jianli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/1721103002/#ps100001 (title: "Changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1721103002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1721103002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jianli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1721103002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1721103002/100001
Message was sent while issue was closed.
Description was changed from ========== Switch between online and offline version of saved page per network connection: If the user is navigating to an online version of saved page and there is no network connection, we should load the offline copy instead. If the user is loading an offline copy and there is network connection, we should load the online version instead. BUG=584417 ========== to ========== Switch between online and offline version of saved page per network connection: If the user is navigating to an online version of saved page and there is no network connection, we should load the offline copy instead. If the user is loading an offline copy and there is network connection, we should load the online version instead. BUG=584417 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Switch between online and offline version of saved page per network connection: If the user is navigating to an online version of saved page and there is no network connection, we should load the offline copy instead. If the user is loading an offline copy and there is network connection, we should load the online version instead. BUG=584417 ========== to ========== Switch between online and offline version of saved page per network connection: If the user is navigating to an online version of saved page and there is no network connection, we should load the offline copy instead. If the user is loading an offline copy and there is network connection, we should load the online version instead. BUG=584417 Committed: https://crrev.com/03b1a6f719fc5016260be23a6d8593d89e06da68 Cr-Commit-Position: refs/heads/master@{#378073} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/03b1a6f719fc5016260be23a6d8593d89e06da68 Cr-Commit-Position: refs/heads/master@{#378073} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
