Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2507793002:
[NTP] Propagate OfflineId from C++ for Downloads. (Closed)
Description[NTP] Propagate OfflineId from C++ for Downloads.
This CL propagates OfflineId from C++ for Downloads in order to match
exactly the corresponding offline page (this is important when there
are multiple ones for the same URL). While we are here, RecentTabs are
changed to store OfflineId in a general (non recent tabs) variable.
BUG=664903
Committed: https://crrev.com/08ada313f04c242dfa8f3b71543b039cb99afb2e
Cr-Commit-Position: refs/heads/master@{#432830}
Patch Set 1 #Patch Set 2 : removed TODO. #
Total comments: 10
Patch Set 3 : rebase and treib@ comments. #
Total comments: 6
Patch Set 4 : rebase and added a constructor. #Patch Set 5 : bauerb@ comments. #
Total comments: 8
Patch Set 6 : rebase and bauerb@ nits. #Patch Set 7 : rebase. #Depends on Patchset: Dependent Patchsets: Messages
Total messages: 42 (26 generated)
vitaliii@chromium.org changed reviewers: + treib@chromium.org
Hey Marc, would you mind having a look at C++ bit?
The CQ bit was checked by vitaliii@chromium.org 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 checked by vitaliii@chromium.org to run a CQ dry run
I forgot to remove TODO, now I have removed it.
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2507793002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2507793002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:397: Long.parseLong(recentTabArticle.getRecentTabId())); If OfflinePageId and RecentTabId are always the same, can we get rid of RecentTabId? https://codereview.chromium.org/2507793002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2507793002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:491: extra->offline_page_id = base::Int64ToString(offline_page.offline_id); Er, any reason why we pass this as a string? Seems like the Java side just converts it back to an int? https://codereview.chromium.org/2507793002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestion.h (right): https://codereview.chromium.org/2507793002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestion.h:30: bool is_download_asset = false; Hm, this is now a bit messy... if is_download_asset is true, only offline_page_id will be valid; otherwise only the other two members will be valid. Right? Since I don't really have a better idea: Can we at least add a comment that states this? (I guess the alternative would be to have static factory methods ForAssetDownload and ForOfflinePage that take exactly the required inputs. Not sure if that's worth it though.)
Hi Marc, PTAL. https://codereview.chromium.org/2507793002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2507793002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:397: Long.parseLong(recentTabArticle.getRecentTabId())); On 2016/11/16 15:22:58, Marc Treib wrote: > If OfflinePageId and RecentTabId are always the same, can we get rid of > RecentTabId? I am not sure whether this is guaranteed and whether this is not an implementation detail. Last time I checked there were no comments when they initialize one with another. Moreover, now I am confused how they ensure that recent tab offline pages ids do not collide with others. Thus, since we already passing it anyway, staying safe seems like a good choice :) https://codereview.chromium.org/2507793002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2507793002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:491: extra->offline_page_id = base::Int64ToString(offline_page.offline_id); On 2016/11/16 15:22:58, Marc Treib wrote: > Er, any reason why we pass this as a string? Seems like the Java side just > converts it back to an int? As we discussed it is not clear how to match int64 on Java side, so TODO for now. https://codereview.chromium.org/2507793002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestion.h (right): https://codereview.chromium.org/2507793002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestion.h:30: bool is_download_asset = false; On 2016/11/16 15:22:58, Marc Treib wrote: > Hm, this is now a bit messy... if is_download_asset is true, only > offline_page_id will be valid; otherwise only the other two members will be > valid. Right? > Since I don't really have a better idea: Can we at least add a comment that > states this? > (I guess the alternative would be to have static factory methods > ForAssetDownload and ForOfflinePage that take exactly the required inputs. Not > sure if that's worth it though.) It is other way around. When is_download_asset is false, offline_page_id is valid, otherwise remaining two variables are valid. I added a comment.
lgtm https://codereview.chromium.org/2507793002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2507793002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:397: Long.parseLong(recentTabArticle.getRecentTabId())); On 2016/11/16 15:45:08, vitaliii wrote: > On 2016/11/16 15:22:58, Marc Treib wrote: > > If OfflinePageId and RecentTabId are always the same, can we get rid of > > RecentTabId? > > I am not sure whether this is guaranteed and whether this is not an > implementation detail. Last time I checked there were no comments when they > initialize one with another. Moreover, now I am confused how they ensure that > recent tab offline pages ids do not collide with others. Thus, since we already > passing it anyway, staying safe seems like a good choice :) Aren't offline IDs unique? Well, do we do anything with the recent tab ID anywhere? If not, then I still vote we get rid of it. https://codereview.chromium.org/2507793002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2507793002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:491: extra->offline_page_id = base::Int64ToString(offline_page.offline_id); On 2016/11/16 15:45:08, vitaliii wrote: > On 2016/11/16 15:22:58, Marc Treib wrote: > > Er, any reason why we pass this as a string? Seems like the Java side just > > converts it back to an int? > > As we discussed it is not clear how to match int64 on Java side, so TODO for > now. There's a jlong analogous to jint, I'm pretty sure that does the right thing :) Anyway, this is fine in a followup.
answered comment, no need to look. https://codereview.chromium.org/2507793002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2507793002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:397: Long.parseLong(recentTabArticle.getRecentTabId())); On 2016/11/16 15:55:56, Marc Treib wrote: > On 2016/11/16 15:45:08, vitaliii wrote: > > On 2016/11/16 15:22:58, Marc Treib wrote: > > > If OfflinePageId and RecentTabId are always the same, can we get rid of > > > RecentTabId? > > > > I am not sure whether this is guaranteed and whether this is not an > > implementation detail. Last time I checked there were no comments when they > > initialize one with another. Moreover, now I am confused how they ensure that > > recent tab offline pages ids do not collide with others. Thus, since we > already > > passing it anyway, staying safe seems like a good choice :) > > Aren't offline IDs unique? They must be. That is my concern how they ensure this while having OfflineId = TabId for recent tabs. > > Well, do we do anything with the recent tab ID anywhere? If not, then I still > vote we get rid of it. Yes, we find the tab by tab ID (a few lines above this comment). https://codereview.chromium.org/2507793002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/download_suggestions_provider.cc (right): https://codereview.chromium.org/2507793002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/download_suggestions_provider.cc:491: extra->offline_page_id = base::Int64ToString(offline_page.offline_id); On 2016/11/16 15:55:56, Marc Treib wrote: > On 2016/11/16 15:45:08, vitaliii wrote: > > On 2016/11/16 15:22:58, Marc Treib wrote: > > > Er, any reason why we pass this as a string? Seems like the Java side just > > > converts it back to an int? > > > > As we discussed it is not clear how to match int64 on Java side, so TODO for > > now. > > There's a jlong analogous to jint, I'm pretty sure that does the right thing :) > Anyway, this is fine in a followup. Acknowledged.
vitaliii@chromium.org changed reviewers: + bauerb@google.com
Hi Bernhard, would you mind reviewing my CL or redirecting it to someone else from UI?
vitaliii@chromium.org changed reviewers: + bauerb@chromium.org - bauerb@google.com
Sorry, Bernhard, I sent the review to the wrong account. So could you have a look or redirect?
The CQ bit was checked by vitaliii@chromium.org 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...
https://codereview.chromium.org/2507793002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/2507793002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:93: // TODO(vitaliii): Pass |offline_page_id| as a numeric variable. Any particular reason you're not doing that now? https://codereview.chromium.org/2507793002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:94: Java_SnippetsBridge_setDownloadOfflinePageDataForLastSuggestion( I realize this is just following existing code, but this interface seems a bit more complicated than necessary. Couldn't we have SnippetsBridge.addSuggestion() return the newly added suggestion, and make these methods take just the suggestion as a parameter? https://codereview.chromium.org/2507793002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestion.h (right): https://codereview.chromium.org/2507793002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestion.h:28: std::string offline_page_id; Same here -- why is this a string and not an int64_t?
On 2016/11/16 17:04:02, Bernhard Bauer wrote: > https://codereview.chromium.org/2507793002/diff/40001/chrome/browser/android/... > File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): > > https://codereview.chromium.org/2507793002/diff/40001/chrome/browser/android/... > chrome/browser/android/ntp/ntp_snippets_bridge.cc:93: // TODO(vitaliii): Pass > |offline_page_id| as a numeric variable. > Any particular reason you're not doing that now? > > https://codereview.chromium.org/2507793002/diff/40001/chrome/browser/android/... > chrome/browser/android/ntp/ntp_snippets_bridge.cc:94: > Java_SnippetsBridge_setDownloadOfflinePageDataForLastSuggestion( > I realize this is just following existing code, but this interface seems a bit > more complicated than necessary. Couldn't we have SnippetsBridge.addSuggestion() > return the newly added suggestion, and make these methods take just the > suggestion as a parameter? > > https://codereview.chromium.org/2507793002/diff/40001/components/ntp_snippets... > File components/ntp_snippets/content_suggestion.h (right): > > https://codereview.chromium.org/2507793002/diff/40001/components/ntp_snippets... > components/ntp_snippets/content_suggestion.h:28: std::string offline_page_id; > Same here -- why is this a string and not an int64_t? Oops, missed Marc's comment. Yes, a Java long is guaranteed to be a 64-bit signed integer: http://docs.oracle.com/javase/7/docs/technotes/guides/jni/spec/types.html. This seems like a thing that would not be more difficult to do right in this CL; the interface change to SnippetsBridge.addSuggestion() we could potentially do in a followup.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by vitaliii@chromium.org to run a CQ dry run
Hi Bernhard, I addressed your comments, could you check please? https://codereview.chromium.org/2507793002/diff/40001/chrome/browser/android/... File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/2507793002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:93: // TODO(vitaliii): Pass |offline_page_id| as a numeric variable. On 2016/11/16 17:04:02, Bernhard Bauer wrote: > Any particular reason you're not doing that now? Neither Mark nor I knew whether this was possible :) But now I did it and it was really easy. Thanks! https://codereview.chromium.org/2507793002/diff/40001/chrome/browser/android/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:94: Java_SnippetsBridge_setDownloadOfflinePageDataForLastSuggestion( On 2016/11/16 17:04:02, Bernhard Bauer wrote: > I realize this is just following existing code, but this interface seems a bit > more complicated than necessary. Couldn't we have SnippetsBridge.addSuggestion() > return the newly added suggestion, and make these methods take just the > suggestion as a parameter? I added a TODO (as per your following comment). https://codereview.chromium.org/2507793002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestion.h (right): https://codereview.chromium.org/2507793002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestion.h:28: std::string offline_page_id; On 2016/11/16 17:04:02, Bernhard Bauer wrote: > Same here -- why is this a string and not an int64_t? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM with some remaining nits: https://codereview.chromium.org/2507793002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2507793002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:352: // TODO(vitaliii): Propagate OfflineId for offline page downloads from C++. Should you remove this now? :) https://codereview.chromium.org/2507793002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java (right): https://codereview.chromium.org/2507793002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:186: public boolean requiresExactOfflinePage() { Javadoc please. https://codereview.chromium.org/2507793002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestion.h (right): https://codereview.chromium.org/2507793002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestion.h:23: DownloadSuggestionExtra(); // = default, in .cc I don't think you need to specify how the implementation file chooses to implement these. https://codereview.chromium.org/2507793002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestion.h:31: int64_t offline_page_id; Maybe initialize this to 0 now?
Resolved nits, no need to look. https://codereview.chromium.org/2507793002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2507793002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:352: // TODO(vitaliii): Propagate OfflineId for offline page downloads from C++. On 2016/11/16 17:44:35, Bernhard Bauer wrote: > Should you remove this now? :) Done. https://codereview.chromium.org/2507793002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java (right): https://codereview.chromium.org/2507793002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:186: public boolean requiresExactOfflinePage() { On 2016/11/16 17:44:35, Bernhard Bauer wrote: > Javadoc please. I will do this in the parent CL. https://codereview.chromium.org/2507793002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestion.h (right): https://codereview.chromium.org/2507793002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestion.h:23: DownloadSuggestionExtra(); // = default, in .cc On 2016/11/16 17:44:35, Bernhard Bauer wrote: > I don't think you need to specify how the implementation file chooses to > implement these. Done. https://codereview.chromium.org/2507793002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestion.h:31: int64_t offline_page_id; On 2016/11/16 17:44:35, Bernhard Bauer wrote: > Maybe initialize this to 0 now? Done.
The CQ bit was checked by vitaliii@chromium.org 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.
Patchset #7 (id:120001) has been deleted
The CQ bit was checked by vitaliii@chromium.org 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 vitaliii@chromium.org
The CQ bit was checked by vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2507793002/#ps140001 (title: "rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [NTP] Propagate OfflineId from C++ for Downloads. This CL propagates OfflineId from C++ for Downloads in order to match exactly the corresponding offline page (this is important when there are multiple ones for the same URL). While we are here, RecentTabs are changed to store OfflineId in a general (non recent tabs) variable. BUG=664903 ========== to ========== [NTP] Propagate OfflineId from C++ for Downloads. This CL propagates OfflineId from C++ for Downloads in order to match exactly the corresponding offline page (this is important when there are multiple ones for the same URL). While we are here, RecentTabs are changed to store OfflineId in a general (non recent tabs) variable. BUG=664903 Committed: https://crrev.com/08ada313f04c242dfa8f3b71543b039cb99afb2e Cr-Commit-Position: refs/heads/master@{#432830} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/08ada313f04c242dfa8f3b71543b039cb99afb2e Cr-Commit-Position: refs/heads/master@{#432830} |
