Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2202423004:
[Offline Page]Enable share offline page from online tab (Closed)
|
Created:
4 years, 4 months ago by Vivian Modified:
4 years, 4 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@newBranchOnTryout Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline Page]Enable share offline page from online tab
The primary implementation in Issue 2081153005 only trigger offline page sharing if user is currently on an offline page. This branch enables sharing offline copy of the page if user is on an online webpage.
BUG=622139
Committed: https://crrev.com/f7dbce5a2c8f157de6f10b9c2249f1c148431027
Cr-Commit-Position: refs/heads/master@{#412971}
Patch Set 1 #Patch Set 2 : Use get best page instead #
Total comments: 17
Patch Set 3 : Change methods to async #
Total comments: 18
Patch Set 4 : Format Callback #Patch Set 5 : Add documentation #
Total comments: 4
Patch Set 6 : Address comments and use async method #
Total comments: 16
Patch Set 7 : Adjust comments, function names, and changes according to recent CL #Patch Set 8 : Merge branch #
Total comments: 1
Patch Set 9 : Patch to land #
Depends on Patchset: Messages
Total messages: 39 (21 generated)
Description was changed from ========== [Offline Page]Enable share offline page from online tab BUG=622139 ========== to ========== [Offline Page]Enable share offline page from online tab The primary implementation in Issue 2081153005 only trigger offline page sharing if user is currently on an offline page. This branch enables sharing offline copy of the page if user is on an online webpage. BUG=622139 ==========
weiran@google.com changed reviewers: + dewittj@chromium.org, fgorski@chromium.org
This is an experimental feature that allows users share an offline copy of the page if they're currently viewing an online webpage. I know we haven't decide if this should happen. Just what to put it there in advance:)
hey. Nice patch. Question: Does it work with no problems? Also, I'll gladly discuss with you how to make the async calls more readable. https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:988: boolean isOnOfflinePage = true; boolean isOfflinePage = (url != null); if (!isOfflinePage) { url = currentTab.getUrl(); } https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/ClientId.java (right): https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/ClientId.java:54: * Create a client id for a tab with no bookmark associated with nit: add a period at the end of the sentence. https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/ClientId.java:59: return new ClientId(OfflinePageBridge.SHARE_NAMESPACE, Integer.toString(id)); could you please create a GUID instead of basing this off the Tab ID? Here is a sample code: // Download UI needs "async_loading" namespace and a random (type 4) GUID. String uuid = UUID.randomUUID().toString(); ClientId clientId = new ClientId("async_loading", uuid); savePageLater(url, clientId); Actually I very much like the idea of having this code here in ClientID. https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:28: public static final String SHARE_NAMESPACE = "share"; I believe this only works, because we never save this type of offline page in the offline page DB. Is that the case? https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:278: public OfflinePageItem getBestPageForOnlineURL(String onlineUrl) { please use asynchronous version. https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:298: public OfflinePageItem getPageByOfflineId(long offlineId) { please use asynchronous version. https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:300: OfflinePageItem item = offlinePageBridge.getBestPageForOnlineURL(onlineUrl); rewrite this part to async. https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:319: offlinePageBridge.getPageByOfflineId(offlineId); rewrite to async.
Comments addressed. Thanks for reviewing this patch! https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:988: boolean isOnOfflinePage = true; On 2016/08/08 17:59:22, fgorski wrote: > boolean isOfflinePage = (url != null); > if (!isOfflinePage) { > url = currentTab.getUrl(); > } Done. https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/ClientId.java (right): https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/ClientId.java:54: * Create a client id for a tab with no bookmark associated with On 2016/08/08 17:59:22, fgorski wrote: > nit: add a period at the end of the sentence. Done. https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/ClientId.java:59: return new ClientId(OfflinePageBridge.SHARE_NAMESPACE, Integer.toString(id)); On 2016/08/08 17:59:22, fgorski wrote: > could you please create a GUID instead of basing this off the Tab ID? > > Here is a sample code: > // Download UI needs "async_loading" namespace and a random (type 4) GUID. > String uuid = UUID.randomUUID().toString(); > ClientId clientId = new ClientId("async_loading", uuid); > savePageLater(url, clientId); > > > Actually I very much like the idea of having this code here in ClientID. Done. https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:28: public static final String SHARE_NAMESPACE = "share"; On 2016/08/08 17:59:22, fgorski wrote: > I believe this only works, because we never save this type of offline page in > the offline page DB. Is that the case? Yes I believe so. Didn't see we use this namespace before. So is it fine that I keep it this way? https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:278: public OfflinePageItem getBestPageForOnlineURL(String onlineUrl) { On 2016/08/08 17:59:22, fgorski wrote: > please use asynchronous version. Done. https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:298: public OfflinePageItem getPageByOfflineId(long offlineId) { On 2016/08/08 17:59:22, fgorski wrote: > please use asynchronous version. Done. https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:300: OfflinePageItem item = offlinePageBridge.getBestPageForOnlineURL(onlineUrl); On 2016/08/08 17:59:22, fgorski wrote: > rewrite this part to async. Done. https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:319: offlinePageBridge.getPageByOfflineId(offlineId); On 2016/08/08 17:59:22, fgorski wrote: > rewrite to async. Done.
some more suggestions + the one I sent in email. https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/2202423004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:28: public static final String SHARE_NAMESPACE = "share"; On 2016/08/08 21:40:36, Vivian wrote: > On 2016/08/08 17:59:22, fgorski wrote: > > I believe this only works, because we never save this type of offline page in > > the offline page DB. Is that the case? > > Yes I believe so. Didn't see we use this namespace before. > So is it fine that I keep it this way? Adding a new flag has some problems here. We should discuss this with Dmitry, before you land it. It will cause problem when we want to clean up the store, which also reports UMA, which in turn uses namespace as a suffix... if it is not created there, it will start crashing. https://codereview.chromium.org/2202423004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/2202423004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:285: * Get the offline page associated with the provided offline id. nit: ID https://codereview.chromium.org/2202423004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:287: * @param offlineId URL pointing to the offline copy of the web page. document the callback please. Also, please read the documentation of offlineId. I am sure it is not a URL https://codereview.chromium.org/2202423004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2202423004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:281: final Tab currentTab, final boolean isOnOfflinePage) { isOfflinePage https://codereview.chromium.org/2202423004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:316: int tabId = currentTab.getId(); remove
jianli@chromium.org changed reviewers: + jianli@chromium.org
https://codereview.chromium.org/2202423004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2202423004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:287: if (offlinePageBridge != null) { nit: do early return to reduce the nest structure, for better readability. Like: if (offlinePageBridge == null) { ... return; } if (isOnOfflinePage) { ... return; } if (currentTab.hasOfflineCopy()) { ... return; } // Save the page offline ... https://codereview.chromium.org/2202423004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:300: } else if (currentTab.hasOfflineCopy()) { This is not an async API which we should try to avoid calling. For now, you can call OfflinePageBridge.getPagesByOnlineUrl and use the best one from the list (see OfflinePageTabHelper::SelectBestPageForRedirectToOffline for how to do this). FYI, I am going to introduce OfflinePageUtils::GetBestPageForOnlineURL in offline_page_utils. This is also going to be exposed to Java side in OfflinePageBridge. Note that this will be an async call which takes a callback. https://codereview.chromium.org/2202423004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:316: int tabId = currentTab.getId(); On 2016/08/08 23:10:35, fgorski wrote: > remove This indeed should be needed in finding the best page for online URL. https://codereview.chromium.org/2202423004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:324: if (savePageResult == SavePageResult.SUCCESS) { nit: do early return https://codereview.chromium.org/2202423004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:329: if (item != null) { nit: do early return
Thanks for reviewing this. Made some changes accordingly. https://codereview.chromium.org/2202423004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/2202423004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:285: * Get the offline page associated with the provided offline id. On 2016/08/08 23:10:35, fgorski wrote: > nit: ID Done. https://codereview.chromium.org/2202423004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:287: * @param offlineId URL pointing to the offline copy of the web page. On 2016/08/08 23:10:35, fgorski wrote: > document the callback please. > > Also, please read the documentation of offlineId. I am sure it is not a URL Done. https://codereview.chromium.org/2202423004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2202423004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:281: final Tab currentTab, final boolean isOnOfflinePage) { On 2016/08/08 23:10:35, fgorski wrote: > isOfflinePage Done. https://codereview.chromium.org/2202423004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:287: if (offlinePageBridge != null) { On 2016/08/08 23:48:49, jianli wrote: > nit: do early return to reduce the nest structure, for better readability. Like: > if (offlinePageBridge == null) { > ... > return; > } > > if (isOnOfflinePage) { > ... > return; > } > > if (currentTab.hasOfflineCopy()) { > ... > return; > } > > // Save the page offline > ... Done. https://codereview.chromium.org/2202423004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:300: } else if (currentTab.hasOfflineCopy()) { On 2016/08/08 23:48:49, jianli wrote: > This is not an async API which we should try to avoid calling. For now, you can > call OfflinePageBridge.getPagesByOnlineUrl and use the best one from the list > (see OfflinePageTabHelper::SelectBestPageForRedirectToOffline for how to do > this). > > FYI, I am going to introduce OfflinePageUtils::GetBestPageForOnlineURL in > offline_page_utils. This is also going to be exposed to Java side in > OfflinePageBridge. Note that this will be an async call which takes a callback. I'm little bit confused here. Is it about "hasOfflineCopy"? So instead of calling it I should discriminate if a online page has an offline copy by calling getPagesByOnlineUrl and check if the list contains at least one item. Am I reading it correctly? https://codereview.chromium.org/2202423004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:316: int tabId = currentTab.getId(); On 2016/08/08 23:48:49, jianli wrote: > On 2016/08/08 23:10:35, fgorski wrote: > > remove > > This indeed should be needed in finding the best page for online URL. This part of code tries to save an offline copy of the page and use the saved result directly. I don't think tabId is needed here. https://codereview.chromium.org/2202423004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:324: if (savePageResult == SavePageResult.SUCCESS) { On 2016/08/08 23:48:49, jianli wrote: > nit: do early return Done. https://codereview.chromium.org/2202423004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:329: if (item != null) { On 2016/08/08 23:48:49, jianli wrote: > nit: do early return Done.
lgtm after you address the comments. https://codereview.chromium.org/2202423004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2202423004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:287: Callback<OfflinePageItem> prepareForSharing = prepareForSharingCallback( this can be done after the if, since you might never need it. https://codereview.chromium.org/2202423004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:299: } nit: be self-consistent. Put an empty line after this }
https://codereview.chromium.org/2202423004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2202423004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:300: } else if (currentTab.hasOfflineCopy()) { On 2016/08/09 17:25:11, Vivian wrote: > On 2016/08/08 23:48:49, jianli wrote: > > This is not an async API which we should try to avoid calling. For now, you > can > > call OfflinePageBridge.getPagesByOnlineUrl and use the best one from the list > > (see OfflinePageTabHelper::SelectBestPageForRedirectToOffline for how to do > > this). > > > > FYI, I am going to introduce OfflinePageUtils::GetBestPageForOnlineURL in > > offline_page_utils. This is also going to be exposed to Java side in > > OfflinePageBridge. Note that this will be an async call which takes a > callback. > > I'm little bit confused here. Is it about "hasOfflineCopy"? So instead of > calling it I should discriminate if a online page has an offline copy by calling > getPagesByOnlineUrl and check if the list contains at least one item. Am I > reading it correctly? I introduce async version in this patch: https://codereview.chromium.org/2225213002/. You can now access it via OfflinePageBridge.getBestPageForOnlineUrl. You don't need to call hasOfflineCopy which is a sync version and will be removed later.
Comment addressed. Changed function calls to async. ptal https://codereview.chromium.org/2202423004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2202423004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:287: Callback<OfflinePageItem> prepareForSharing = prepareForSharingCallback( On 2016/08/09 23:24:32, fgorski wrote: > this can be done after the if, since you might never need it. Done. https://codereview.chromium.org/2202423004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:299: } On 2016/08/09 23:24:32, fgorski wrote: > nit: be self-consistent. Put an empty line after this } Done.
https://codereview.chromium.org/2202423004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2202423004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1012: // tab's URL directly. this comment seems out of date now. https://codereview.chromium.org/2202423004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1026: boolean isOfflinePage = (url != null); I think we need different variable names now, since there are a couple of different "url"s in this function. We have the offlinePageUrl, we have the currentTabUrl, and we have the urlToShare. These are a few suggestions :) but feel free to name them how you like. This will make the boolean logic a little easier to follow. https://codereview.chromium.org/2202423004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/ClientId.java (right): https://codereview.chromium.org/2202423004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/ClientId.java:56: * Create a client id for a tab with no bookmark associated with. This doesn't makes sense. Could you elaborate on the actual use case? Something like "Create a client id for a tab when its contents need to be saved for sharing purposes." Then you wouldn't need the second line about assuming the namespace either. https://codereview.chromium.org/2202423004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2202423004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:280: final Activity mainActivity, final String text, final String onlineUrl, onlineUrl isn't always online anymore, is it? It could be the offline URL. https://codereview.chromium.org/2202423004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:304: ClientId clientId = ClientId.createClientIdForTabSharing(); I think this should be done in selectPageForOnlineUrlCallback, to reduce parameters. https://codereview.chromium.org/2202423004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:324: private static Callback<OfflinePageItem> prepareForSharingCallback(final boolean shareDirectly, nit: rename this to onGotOfflinePageItemToShare https://codereview.chromium.org/2202423004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:331: if (item == null) return; What is the user experience here? Should we just assert item != null? https://codereview.chromium.org/2202423004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:341: * Callback for select an offline page given the online url. Please write about what this function does, rather than where it is invoked. Something like "Takes the offline page item from selectPageForOnlineURL, and if it exists, invokes |prepareForSharing| with it. Otherwise, saves a page for the online URL and invokes |prepareForSharing| with the result when it's ready." Same with SavePageCallback.
Patchset #7 (id:120001) has been deleted
ptal https://codereview.chromium.org/2202423004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2202423004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1012: // tab's URL directly. On 2016/08/15 22:11:20, dewittj wrote: > this comment seems out of date now. Updated. https://codereview.chromium.org/2202423004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1026: boolean isOfflinePage = (url != null); On 2016/08/15 22:11:20, dewittj wrote: > I think we need different variable names now, since there are a couple of > different "url"s in this function. We have the offlinePageUrl, we have the > currentTabUrl, and we have the urlToShare. These are a few suggestions :) but > feel free to name them how you like. This will make the boolean logic a little > easier to follow. Changed url to onlineUrl as that's the name used in OfflinePageUtils. In OfflinePageUtils there're two urls in use: onlineUrl and offlineUrl. OnlineUrl is the one to be shared. I didn't change variable names there. Let me know if any of them need to be changed. https://codereview.chromium.org/2202423004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/ClientId.java (right): https://codereview.chromium.org/2202423004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/ClientId.java:56: * Create a client id for a tab with no bookmark associated with. On 2016/08/15 22:11:20, dewittj wrote: > This doesn't makes sense. Could you elaborate on the actual use case? > Something like "Create a client id for a tab when its contents need to be saved > for sharing purposes." > > Then you wouldn't need the second line about assuming the namespace either. Done. https://codereview.chromium.org/2202423004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2202423004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:280: final Activity mainActivity, final String text, final String onlineUrl, On 2016/08/15 22:11:21, dewittj wrote: > onlineUrl isn't always online anymore, is it? It could be the offline URL. It's still the online url regardless of the tab is online or not. It is gotten from currentTab.getOriginalUrl (which was updated by Jian recently, getOfflinePageOriginalUrl is obsolete). Offline URL is get from tab later in the function. https://codereview.chromium.org/2202423004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:304: ClientId clientId = ClientId.createClientIdForTabSharing(); On 2016/08/15 22:11:21, dewittj wrote: > I think this should be done in selectPageForOnlineUrlCallback, to reduce > parameters. Done. https://codereview.chromium.org/2202423004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:324: private static Callback<OfflinePageItem> prepareForSharingCallback(final boolean shareDirectly, On 2016/08/15 22:11:21, dewittj wrote: > nit: rename this to onGotOfflinePageItemToShare Done. https://codereview.chromium.org/2202423004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:331: if (item == null) return; On 2016/08/15 22:11:21, dewittj wrote: > What is the user experience here? Should we just assert item != null? My understanding is that if the offline sharing process fails at some point, we should share only online url instead. Theresa suggested that it would behave this way when file operation fails for offline page sharing. I edited some code here to reflect this flow. Am I understanding your question correctly? https://codereview.chromium.org/2202423004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:341: * Callback for select an offline page given the online url. On 2016/08/15 22:11:21, dewittj wrote: > Please write about what this function does, rather than where it is invoked. > Something like "Takes the offline page item from selectPageForOnlineURL, and if > it exists, invokes |prepareForSharing| with it. Otherwise, saves a page for the > online URL and invokes |prepareForSharing| with the result when it's ready." > > Same with SavePageCallback. Done.
The CQ bit was checked by weiran@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
weiran@google.com changed reviewers: + dfalcantara@chromium.org
dfalcantara@chromium.org: Please review changes in ChromeActivity.java This branch is depend on the earlier one, which enables share offline page from online tab. ptal.
lgtm
lgtm
The CQ bit was checked by weiran@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm https://codereview.chromium.org/2202423004/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2202423004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:275: * Share a offline copy of the current page. nit: an
The CQ bit was checked by weiran@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by weiran@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org, dfalcantara@chromium.org, jianli@chromium.org, dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/2202423004/#ps180001 (title: "Patch to land")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch. On branch working_branch Your branch is up-to-date with 'origin/refs/pending/heads/master'. nothing to commit, working tree clean
Message was sent while issue was closed.
Description was changed from ========== [Offline Page]Enable share offline page from online tab The primary implementation in Issue 2081153005 only trigger offline page sharing if user is currently on an offline page. This branch enables sharing offline copy of the page if user is on an online webpage. BUG=622139 ========== to ========== [Offline Page]Enable share offline page from online tab The primary implementation in Issue 2081153005 only trigger offline page sharing if user is currently on an offline page. This branch enables sharing offline copy of the page if user is on an online webpage. BUG=622139 Committed: https://crrev.com/f7dbce5a2c8f157de6f10b9c2249f1c148431027 Cr-Commit-Position: refs/heads/master@{#412971} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/f7dbce5a2c8f157de6f10b9c2249f1c148431027 Cr-Commit-Position: refs/heads/master@{#412971} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
