Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1514833002:
[Offline Pages on the NTP] Bypass the network interstitial (Closed)
|
Created:
5 years ago by Marc Treib Modified:
4 years, 11 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@offline_badge Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline Pages on the NTP] Bypass the network interstitial
Go directly to the offline page if it's available and we're offline
BUG=565219
Committed: https://crrev.com/3d14d22a68702b3ec8118f1fe60a3bc2bd002f30
Cr-Commit-Position: refs/heads/master@{#369735}
Patch Set 1 #
Total comments: 2
Patch Set 2 : rebase #Patch Set 3 : review #Patch Set 4 : update after crrev.com/1524293002 #Patch Set 5 : getOfflineUrlForOnlineUrl returns emtpy string, not null #
Total comments: 7
Patch Set 6 : review #Patch Set 7 : rebase #
Total comments: 4
Patch Set 8 : rebase #Patch Set 9 : newt review #
Total comments: 4
Patch Set 10 : review/rebase #
Messages
Total messages: 27 (5 generated)
treib@chromium.org changed reviewers: + fgorski@chromium.org
Next piece - PTAL! Again, I'll add an NTP owner later.
https://codereview.chromium.org/1514833002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1514833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:279: if (!OfflinePageUtils.isConnected(mNewTabPageView.getContext())) { Do we know if the content is related to a bookmark? If so could we use that knowledge? I am working on refactoring getters for offline and online URLs for offline content, but it will take a while. Also, when accessing the offline content, we should mark it as accessed, so that we keep the counter for it's use current. E.g. the way we do here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
https://codereview.chromium.org/1514833002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1514833002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:279: if (!OfflinePageUtils.isConnected(mNewTabPageView.getContext())) { On 2015/12/14 21:01:58, fgorski wrote: > Do we know if the content is related to a bookmark? > If so could we use that knowledge? What exactly do you mean by "related to a bookmark"? AFAIK for now, offline pages always have a corresponding bookmark, right? > I am working on refactoring getters for offline and online URLs for offline > content, but it will take a while. Acknowledged. > Also, when accessing the offline content, we should mark it as accessed, so that > we keep the counter for it's use current. E.g. the way we do here: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... Done.
On 2015/12/15 16:34:13, Marc Treib wrote: > https://codereview.chromium.org/1514833002/diff/1/chrome/android/java/src/org... > File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java > (right): > > https://codereview.chromium.org/1514833002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:279: if > (!OfflinePageUtils.isConnected(mNewTabPageView.getContext())) { > On 2015/12/14 21:01:58, fgorski wrote: > > Do we know if the content is related to a bookmark? > > If so could we use that knowledge? > > What exactly do you mean by "related to a bookmark"? AFAIK for now, offline > pages always have a corresponding bookmark, right? > > > I am working on refactoring getters for offline and online URLs for offline > > content, but it will take a while. > > Acknowledged. > > > Also, when accessing the offline content, we should mark it as accessed, so > that > > we keep the counter for it's use current. E.g. the way we do here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... > > Done. These 2 patches is what will help you: https://codereview.chromium.org/1521193002/ https://codereview.chromium.org/1524293002/
On 2015/12/15 20:02:45, fgorski wrote: > On 2015/12/15 16:34:13, Marc Treib wrote: > > > https://codereview.chromium.org/1514833002/diff/1/chrome/android/java/src/org... > > File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java > > (right): > > > > > https://codereview.chromium.org/1514833002/diff/1/chrome/android/java/src/org... > > chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:279: > if > > (!OfflinePageUtils.isConnected(mNewTabPageView.getContext())) { > > On 2015/12/14 21:01:58, fgorski wrote: > > > Do we know if the content is related to a bookmark? > > > If so could we use that knowledge? > > > > What exactly do you mean by "related to a bookmark"? AFAIK for now, offline > > pages always have a corresponding bookmark, right? > > > > > I am working on refactoring getters for offline and online URLs for offline > > > content, but it will take a while. > > > > Acknowledged. > > > > > Also, when accessing the offline content, we should mark it as accessed, so > > that > > > we keep the counter for it's use current. E.g. the way we do here: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... > > > > Done. > > These 2 patches is what will help you: > https://codereview.chromium.org/1521193002/ > https://codereview.chromium.org/1524293002/ Okay, then I guess I'll wait for those to land, and then update my CL. Thanks!
On 2015/12/16 09:12:04, Marc Treib wrote: > On 2015/12/15 20:02:45, fgorski wrote: > > On 2015/12/15 16:34:13, Marc Treib wrote: > > > > > > https://codereview.chromium.org/1514833002/diff/1/chrome/android/java/src/org... > > > File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java > > > (right): > > > > > > > > > https://codereview.chromium.org/1514833002/diff/1/chrome/android/java/src/org... > > > chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:279: > > if > > > (!OfflinePageUtils.isConnected(mNewTabPageView.getContext())) { > > > On 2015/12/14 21:01:58, fgorski wrote: > > > > Do we know if the content is related to a bookmark? > > > > If so could we use that knowledge? > > > > > > What exactly do you mean by "related to a bookmark"? AFAIK for now, offline > > > pages always have a corresponding bookmark, right? > > > > > > > I am working on refactoring getters for offline and online URLs for > offline > > > > content, but it will take a while. > > > > > > Acknowledged. > > > > > > > Also, when accessing the offline content, we should mark it as accessed, > so > > > that > > > > we keep the counter for it's use current. E.g. the way we do here: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... > > > > > > Done. > > > > These 2 patches is what will help you: > > https://codereview.chromium.org/1521193002/ > > https://codereview.chromium.org/1524293002/ > > Okay, then I guess I'll wait for those to land, and then update my CL. Thanks! Mark, I am still trying to push the relevant changes through. Sorry for the hold up.
On 2016/01/05 21:45:30, fgorski wrote: > On 2015/12/16 09:12:04, Marc Treib wrote: > > On 2015/12/15 20:02:45, fgorski wrote: > > > On 2015/12/15 16:34:13, Marc Treib wrote: > > > > > > > > > > https://codereview.chromium.org/1514833002/diff/1/chrome/android/java/src/org... > > > > File > chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1514833002/diff/1/chrome/android/java/src/org... > > > > > chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:279: > > > if > > > > (!OfflinePageUtils.isConnected(mNewTabPageView.getContext())) { > > > > On 2015/12/14 21:01:58, fgorski wrote: > > > > > Do we know if the content is related to a bookmark? > > > > > If so could we use that knowledge? > > > > > > > > What exactly do you mean by "related to a bookmark"? AFAIK for now, > offline > > > > pages always have a corresponding bookmark, right? > > > > > > > > > I am working on refactoring getters for offline and online URLs for > > offline > > > > > content, but it will take a while. > > > > > > > > Acknowledged. > > > > > > > > > Also, when accessing the offline content, we should mark it as accessed, > > so > > > > that > > > > > we keep the counter for it's use current. E.g. the way we do here: > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... > > > > > > > > Done. > > > > > > These 2 patches is what will help you: > > > https://codereview.chromium.org/1521193002/ > > > https://codereview.chromium.org/1524293002/ > > > > Okay, then I guess I'll wait for those to land, and then update my CL. Thanks! > > Mark, I am still trying to push the relevant changes through. Sorry for the hold > up. Thanks for the update! I also still haven't gotten mocks, so probably this won't make it into M49 after all... :(
Ping! I've updated this to use the new methods from your CL.
https://codereview.chromium.org/1514833002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1514833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:220: private String getOfflineUrl(String pageUrl) { remove and use mOfflinePageBridge.getOfflineUrlForOnlineUrl(pageUrl); Or replace most of code with the above after ensuring offline page bridge is initialized. https://codereview.chromium.org/1514833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:346: if (!OfflinePageUtils.isConnected(mNewTabPageView.getContext())) { This is interesting. Until now we were opening the offline page form the EnhancedBookmarkUtils, where we were able to markPageAccessed, and report UMA (where is the page opened from). I think the markPageAccessed belongs here with openUrl. I think we will have to overhaul this piece soon. For now it is ok, as you are not launching yet, but I'll have to work this out. https://codereview.chromium.org/1514833002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/1514833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:340: * @return URL pointing to the offline copy or the empty string if none exists. Thanks for catching that. I think this should actually be null. I'll consult with clank people. Perhaps you should fix it below in code (replace empty with null). And I can overhaul everything once I get a response.
https://codereview.chromium.org/1514833002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1514833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:220: private String getOfflineUrl(String pageUrl) { On 2016/01/08 17:37:51, fgorski wrote: > remove and use mOfflinePageBridge.getOfflineUrlForOnlineUrl(pageUrl); > > Or replace most of code with the above after ensuring offline page bridge is > initialized. I've removed the helper method, and moved the code into open() below (which was the only call site). I can't just use getOfflineUrlForOnlineUrl though, since I need the bookmarkId to mark the page as accessed. https://codereview.chromium.org/1514833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:346: if (!OfflinePageUtils.isConnected(mNewTabPageView.getContext())) { On 2016/01/08 17:37:51, fgorski wrote: > This is interesting. Until now we were opening the offline page form the > EnhancedBookmarkUtils, where we were able to markPageAccessed, and report UMA > (where is the page opened from). > > I think the markPageAccessed belongs here with openUrl. Done. > I think we will have to overhaul this piece soon. For now it is ok, as you are > not launching yet, but I'll have to work this out. Acknowledged. https://codereview.chromium.org/1514833002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/1514833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:340: * @return URL pointing to the offline copy or the empty string if none exists. On 2016/01/08 17:37:51, fgorski wrote: > Thanks for catching that. I think this should actually be null. I'll consult > with clank people. > > Perhaps you should fix it below in code (replace empty with null). And I can > overhaul everything once I get a response. Done.
lgtm. https://codereview.chromium.org/1514833002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1514833002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:220: private String getOfflineUrl(String pageUrl) { On 2016/01/11 10:32:23, Marc Treib wrote: > On 2016/01/08 17:37:51, fgorski wrote: > > remove and use mOfflinePageBridge.getOfflineUrlForOnlineUrl(pageUrl); > > > > Or replace most of code with the above after ensuring offline page bridge is > > initialized. > > I've removed the helper method, and moved the code into open() below (which was > the only call site). I can't just use getOfflineUrlForOnlineUrl though, since I > need the bookmarkId to mark the page as accessed. Acknowledged.
treib@chromium.org changed reviewers: + newt@chromium.org
+newt for OWNERS approval of NewTabPage.java. PTAL!
https://codereview.chromium.org/1514833002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1514833002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:332: if (!OfflinePageUtils.isConnected(mNewTabPageView.getContext()) && !mIsDestroyed This seems like the wrong layer (too high) to be adding this logic in. Redirecting to the offline version of a page should happen at a lower layer, common to all navigations. One problem with the current solution is that it doesn't handle the myriad other ways that offline pages could be opened: from the bookmarks page, from the recent tabs page, from an add-to-homescreen link, from an omnibox navigation, etc. Here are some options to consider: - BrowserURLHandler. See e.g. ChromeContentBrowserClient::BrowserURLHandlerCreated() and new_tab_page_url_handler.cc - InterceptNavigationDelegateImpl.java - ResourceThrottler/NavigationThrottler Someone who knows the content/ layer well can probably point you in the right direction.
https://codereview.chromium.org/1514833002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1514833002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:332: if (!OfflinePageUtils.isConnected(mNewTabPageView.getContext()) && !mIsDestroyed On 2016/01/12 20:58:55, newt wrote: > This seems like the wrong layer (too high) to be adding this logic in. > Redirecting to the offline version of a page should happen at a lower layer, > common to all navigations. > > One problem with the current solution is that it doesn't handle the myriad other > ways that offline pages could be opened: from the bookmarks page, from the > recent tabs page, from an add-to-homescreen link, from an omnibox navigation, > etc. > > Here are some options to consider: > - BrowserURLHandler. See e.g. > ChromeContentBrowserClient::BrowserURLHandlerCreated() and > new_tab_page_url_handler.cc > - InterceptNavigationDelegateImpl.java > - ResourceThrottler/NavigationThrottler > > Someone who knows the content/ layer well can probably point you in the right > direction. I don't think we want to do the automatic indirection in general. The usual flow is that you follow the online link (from wherever) and get a network interstitial, which tells you you're offline and offers to show the offline copy instead. The NTP is different because it will have special UI to indicate that you'll see an offline copy, so there's no need to also show the interstitial.
https://codereview.chromium.org/1514833002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1514833002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:332: if (!OfflinePageUtils.isConnected(mNewTabPageView.getContext()) && !mIsDestroyed On 2016/01/13 09:38:19, Marc Treib wrote: > On 2016/01/12 20:58:55, newt wrote: > > This seems like the wrong layer (too high) to be adding this logic in. > > Redirecting to the offline version of a page should happen at a lower layer, > > common to all navigations. > > > > One problem with the current solution is that it doesn't handle the myriad > other > > ways that offline pages could be opened: from the bookmarks page, from the > > recent tabs page, from an add-to-homescreen link, from an omnibox navigation, > > etc. > > > > Here are some options to consider: > > - BrowserURLHandler. See e.g. > > ChromeContentBrowserClient::BrowserURLHandlerCreated() and > > new_tab_page_url_handler.cc > > - InterceptNavigationDelegateImpl.java > > - ResourceThrottler/NavigationThrottler > > > > Someone who knows the content/ layer well can probably point you in the right > > direction. > > I don't think we want to do the automatic indirection in general. The usual flow > is that you follow the online link (from wherever) and get a network > interstitial, which tells you you're offline and offers to show the offline copy > instead. > The NTP is different because it will have special UI to indicate that you'll see > an offline copy, so there's no need to also show the interstitial. Ok, I'll buy that distinction between navigation from the NTP and navigation from anywhere else in Chrome :) In that case, could you encapsulate this logic for picking which URL to load inside OfflinePageUtils? e.g. something like OfflinePageUtils.getOfflineUrlIfAvailableAndOffline(String url). It makes more sense for OfflinePageUtils to deal with offline-specific details than for NewTabPage; and this method seems like something that might be reused elsewhere.
Filip, you might want to take a look again - at newt's suggestion, I've added a helper method in OfflinePageUtils. https://codereview.chromium.org/1514833002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1514833002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:332: if (!OfflinePageUtils.isConnected(mNewTabPageView.getContext()) && !mIsDestroyed On 2016/01/13 22:05:12, newt wrote: > On 2016/01/13 09:38:19, Marc Treib wrote: > > On 2016/01/12 20:58:55, newt wrote: > > > This seems like the wrong layer (too high) to be adding this logic in. > > > Redirecting to the offline version of a page should happen at a lower layer, > > > common to all navigations. > > > > > > One problem with the current solution is that it doesn't handle the myriad > > other > > > ways that offline pages could be opened: from the bookmarks page, from the > > > recent tabs page, from an add-to-homescreen link, from an omnibox > navigation, > > > etc. > > > > > > Here are some options to consider: > > > - BrowserURLHandler. See e.g. > > > ChromeContentBrowserClient::BrowserURLHandlerCreated() and > > > new_tab_page_url_handler.cc > > > - InterceptNavigationDelegateImpl.java > > > - ResourceThrottler/NavigationThrottler > > > > > > Someone who knows the content/ layer well can probably point you in the > right > > > direction. > > > > I don't think we want to do the automatic indirection in general. The usual > flow > > is that you follow the online link (from wherever) and get a network > > interstitial, which tells you you're offline and offers to show the offline > copy > > instead. > > The NTP is different because it will have special UI to indicate that you'll > see > > an offline copy, so there's no need to also show the interstitial. > > Ok, I'll buy that distinction between navigation from the NTP and navigation > from anywhere else in Chrome :) > > In that case, could you encapsulate this logic for picking which URL to load > inside OfflinePageUtils? e.g. something like > OfflinePageUtils.getOfflineUrlIfAvailableAndOffline(String url). It makes more > sense for OfflinePageUtils to deal with offline-specific details than for > NewTabPage; and this method seems like something that might be reused elsewhere. Done.
NewTabPage lgtm after comment https://codereview.chromium.org/1514833002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1514833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:339: if (!mIsDestroyed && isNtpOfflinePagesEnabled()) { I'd changed the first line to "if (mIsDestroyed) return;". If this object has been destroyed, it's fine for it to ignore open() requests.
Marc, I sent you a refactoring idea in the email, please let me know if you don't want to take it and I'll do that after your patch. In that case I only ask you to update the method name and it looks good. https://codereview.chromium.org/1514833002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1514833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:343: url = OfflinePageUtils.getOfflineUrlIfNecessary( rename to getLaunchUrlFromOnlineUrl
Thanks all! https://codereview.chromium.org/1514833002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1514833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:339: if (!mIsDestroyed && isNtpOfflinePagesEnabled()) { On 2016/01/14 18:27:11, newt wrote: > I'd changed the first line to "if (mIsDestroyed) return;". If this object has > been destroyed, it's fine for it to ignore open() requests. Done. https://codereview.chromium.org/1514833002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:343: url = OfflinePageUtils.getOfflineUrlIfNecessary( On 2016/01/14 18:44:06, fgorski wrote: > rename to getLaunchUrlFromOnlineUrl Done.
The CQ bit was checked by treib@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1514833002/#ps180001 (title: "review/rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1514833002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1514833002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages on the NTP] Bypass the network interstitial Go directly to the offline page if it's available and we're offline BUG=565219 ========== to ========== [Offline Pages on the NTP] Bypass the network interstitial Go directly to the offline page if it's available and we're offline BUG=565219 Committed: https://crrev.com/3d14d22a68702b3ec8118f1fe60a3bc2bd002f30 Cr-Commit-Position: refs/heads/master@{#369735} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/3d14d22a68702b3ec8118f1fe60a3bc2bd002f30 Cr-Commit-Position: refs/heads/master@{#369735} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
