Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2505763002:
[NTP] Use OfflineId instead of GUID to identify offline pages. (Closed)
Description[NTP] Use OfflineId instead of GUID to identify offline pages.
Previosly we used GUID, because all methods to force opening offline
version of the page required GUID. However, recently we added methods
based on OfflineId and we were advised by Offline Pages team to move to
OfflineId instead.
This CL replaces GUID with OfflineId, usage of OfflinePageDownloadBridge
with OfflinePageBridge.
BUG=665454
Committed: https://crrev.com/3242a750958cd45273d18b22a8de1492f1c76b0d
Cr-Commit-Position: refs/heads/master@{#432820}
Patch Set 1 #
Total comments: 11
Patch Set 2 : rebase + dewittj@ comments + tests. #
Total comments: 8
Patch Set 3 : bauerb@ comments. #
Total comments: 2
Patch Set 4 : bauerb@ comment. #
Total comments: 6
Patch Set 5 : dgn@ comments. #Patch Set 6 : handle NTP descruction in callbacks. #Dependent Patchsets: Messages
Total messages: 60 (40 generated)
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...
vitaliii@chromium.org changed reviewers: + dewittj@chromium.org, dgn@chromium.org, fgorski@chromium.org
Hi folks, Here is my attempt to replace GUID with OfflineId. It seems to work on the device. @dewittj, @fgorski - could you approve usage of OfflinePageBridge? Would you like me to recreate it every time from Profile instead of having as a member? Feel free to ignore strange java usage, since I am a beginner. Feel free to take your time, this will be relevant for me only in at least 9 hours. @dgn - could you look at general java usage? :) I have not changed the tests yet.
On 2016/11/15 21:05:35, vitaliii wrote: > Hi folks, > > Here is my attempt to replace GUID with OfflineId. > It seems to work on the device. > > @dewittj, @fgorski - could you approve usage of OfflinePageBridge? Would you > like me to recreate it every time from Profile instead of having as a member? > Feel free to ignore strange java usage, since I am a beginner. Feel free to take > your time, this will be relevant for me only in at least 9 hours. > > @dgn - could you look at general java usage? :) > > I have not changed the tests yet. BTW, if you are busy with BP, feel free to ignore this altogether.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
this seems better! Still a little bit circuitous with your TODO in there. I imagine that this may not be the end game since there are still too many model queries. https://codereview.chromium.org/2505763002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2505763002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:57: // We need to setup a listener because the OfflinePageBridge won't be available This comment is now out of date. All methods on OfflinePageBridge should be async and therefore wait until the model is loaded before returning. https://codereview.chromium.org/2505763002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:159: updateAllSnippetOfflineAvailability(); This could probably be more efficient https://codereview.chromium.org/2505763002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:238: android.util.Log.i("MYMYMY", "right before the call " + (mOfflinePageBridge == null)); remove? https://codereview.chromium.org/2505763002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:240: mOfflinePageBridge.selectPageForOnlineUrl(article.mUrl, 0, new Callback<OfflinePageItem>() { Possibly need to cancel this if the destruction event occurs. https://codereview.chromium.org/2505763002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:258: updateSnippetOfflineAvailability(article); This feels like a shortcoming of the API; please file a bug to make selectPageForOnlineUrl be batchable.
Description was changed from ========== [NTP] Use OfflineId isntead of GUID to identify offline pages. Previosly we used GUID, because all methods to force opening offline version of the page required GUID. However, recently we added methods based on OfflineId and we were advised by Offline Pages team to move to OfflineId instead. This CL replaces GUID with OfflineId, usage of OfflinePageDownloadBridge with OfflinePageBridge. BUG=665454 ========== to ========== [NTP] Use OfflineId instead of GUID to identify offline pages. Previosly we used GUID, because all methods to force opening offline version of the page required GUID. However, recently we added methods based on OfflineId and we were advised by Offline Pages team to move to OfflineId instead. This CL replaces GUID with OfflineId, usage of OfflinePageDownloadBridge with OfflinePageBridge. BUG=665454 ==========
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: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
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...
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by vitaliii@chromium.org to run a CQ dry run
Patchset #2 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Published comments. https://codereview.chromium.org/2505763002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2505763002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:57: // We need to setup a listener because the OfflinePageBridge won't be available On 2016/11/16 05:03:34, dewittj wrote: > This comment is now out of date. All methods on OfflinePageBridge should be > async and therefore wait until the model is loaded before returning. Done. https://codereview.chromium.org/2505763002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:159: updateAllSnippetOfflineAvailability(); On 2016/11/16 05:03:34, dewittj wrote: > This could probably be more efficient Done. https://codereview.chromium.org/2505763002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:238: android.util.Log.i("MYMYMY", "right before the call " + (mOfflinePageBridge == null)); On 2016/11/16 05:03:34, dewittj wrote: > remove? Done. https://codereview.chromium.org/2505763002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:240: mOfflinePageBridge.selectPageForOnlineUrl(article.mUrl, 0, new Callback<OfflinePageItem>() { On 2016/11/16 05:03:34, dewittj wrote: > Possibly need to cancel this if the destruction event occurs. I do not understand which destruction and how to cancel. dgn@ could you help me with this? https://codereview.chromium.org/2505763002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:258: updateSnippetOfflineAvailability(article); On 2016/11/16 05:03:34, dewittj wrote: > This feels like a shortcoming of the API; please file a bug to make > selectPageForOnlineUrl be batchable. Done. crbug.com/665756
bauerb@chromium.org changed reviewers: + bauerb@chromium.org
https://codereview.chromium.org/2505763002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2505763002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:158: if (article.getOfflinePageOfflineId() != null I would maybe make this a return as well, and store the offline ID in a local variable. https://codereview.chromium.org/2505763002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java (right): https://codereview.chromium.org/2505763002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:216: && ((previous == null) != (mOfflinePageOfflineId == null))) { Would we also want to run the callback if both new and old value are non-null, but different? In that case, you could just use a null-sensitive comparison. https://codereview.chromium.org/2505763002/diff/80001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java (right): https://codereview.chromium.org/2505763002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:209: doAnswer(new Answer<Void>() { At this point, why don't you just create a FakeOfflinePageBridge yourself instead of shoehorning this into Mockito?
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 vitaliii@chromium.org to run a CQ dry run
Hi Bernhard, I addressed your comments. Please have a look. https://codereview.chromium.org/2505763002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2505763002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:158: if (article.getOfflinePageOfflineId() != null On 2016/11/16 13:51:59, Bernhard Bauer wrote: > I would maybe make this a return as well, and store the offline ID in a local > variable. Done. https://codereview.chromium.org/2505763002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java (right): https://codereview.chromium.org/2505763002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:216: && ((previous == null) != (mOfflinePageOfflineId == null))) { On 2016/11/16 13:51:59, Bernhard Bauer wrote: > Would we also want to run the callback if both new and old value are non-null, > but different? In that case, you could just use a null-sensitive comparison. Currently this does not matter, but, yes, we want to run callback for non-null different values. I rewrite the statement accordingly. Note that Objects.equals does not work - lint fails with: SnippetArticle.class Call requires API level 19 (current min is 16): `java.util.Objects#equals`: NewApi [warning] https://codereview.chromium.org/2505763002/diff/80001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java (right): https://codereview.chromium.org/2505763002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:209: doAnswer(new Answer<Void>() { On 2016/11/16 13:51:59, Bernhard Bauer wrote: > At this point, why don't you just create a FakeOfflinePageBridge yourself > instead of shoehorning this into Mockito? I feel like this will take quite some time, while this is only a test. How about a TODO now and FakeOfflinePageBridge after BP?
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.
https://codereview.chromium.org/2505763002/diff/80001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java (right): https://codereview.chromium.org/2505763002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:209: doAnswer(new Answer<Void>() { On 2016/11/16 14:23:36, vitaliii wrote: > On 2016/11/16 13:51:59, Bernhard Bauer wrote: > > At this point, why don't you just create a FakeOfflinePageBridge yourself > > instead of shoehorning this into Mockito? > > I feel like this will take quite some time, while this is only a test. How about > a TODO now and FakeOfflinePageBridge after BP? I don't think it will, because subclassing OfflinePageBridge should be more straightforward than fiddling with Mockito invocations, but yeah, I'm okay with doing this in a followup. https://codereview.chromium.org/2505763002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java (right): https://codereview.chromium.org/2505763002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:218: || previous == null && mOfflinePageOfflineId != null) { How about writing this as: if ((previous == null) ? (mOfflinePageOfflineId != null) : !previous.equals(mOfflinePageOfflineId)) { ... } ?
The CQ bit was checked by vitaliii@chromium.org to run a CQ dry run
Hey Bernhard, I addressed your comments. PTAL. https://codereview.chromium.org/2505763002/diff/80001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java (right): https://codereview.chromium.org/2505763002/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:209: doAnswer(new Answer<Void>() { On 2016/11/16 15:14:59, Bernhard Bauer wrote: > On 2016/11/16 14:23:36, vitaliii wrote: > > On 2016/11/16 13:51:59, Bernhard Bauer wrote: > > > At this point, why don't you just create a FakeOfflinePageBridge yourself > > > instead of shoehorning this into Mockito? > > > > I feel like this will take quite some time, while this is only a test. How > about > > a TODO now and FakeOfflinePageBridge after BP? > > I don't think it will, because subclassing OfflinePageBridge should be more > straightforward than fiddling with Mockito invocations, but yeah, I'm okay with > doing this in a followup. Acknowledged. https://codereview.chromium.org/2505763002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java (right): https://codereview.chromium.org/2505763002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:218: || previous == null && mOfflinePageOfflineId != null) { On 2016/11/16 15:14:59, Bernhard Bauer wrote: > How about writing this as: > > if ((previous == null) ? (mOfflinePageOfflineId != null) : > !previous.equals(mOfflinePageOfflineId)) { ... } > > ? Done. That's a tricky way to write this, but it feels clearer that the previous |if|.
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.
https://codereview.chromium.org/2505763002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2505763002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:232: if (!article.isDownload() && !article.isRecentTab()) { Wrap this check in a method on SnippetArticle? It is used repeatedly around here and would probably make sense to be checked in openSnippets() too? https://codereview.chromium.org/2505763002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java (right): https://codereview.chromium.org/2505763002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:216: if ((previous == null) ? (mOfflinePageOfflineId != null) does it matter that the ID is the same? Don't we just care whether there is an offline page or not? So just check (previous == null) == (mOfflinePageOfflineId == null) ? https://codereview.chromium.org/2505763002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:226: public Long getOfflinePageOfflineId() { add @Nullable
lgtm
Hi dgn@, I addressed your comments, PTAL. https://codereview.chromium.org/2505763002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2505763002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:232: if (!article.isDownload() && !article.isRecentTab()) { On 2016/11/16 16:25:05, dgn wrote: > Wrap this check in a method on SnippetArticle? It is used repeatedly around here > and would probably make sense to be checked in openSnippets() too? Done. Are you ok with article.requiresExactOfflinePage() name? https://codereview.chromium.org/2505763002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java (right): https://codereview.chromium.org/2505763002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:216: if ((previous == null) ? (mOfflinePageOfflineId != null) On 2016/11/16 16:25:05, dgn wrote: > does it matter that the ID is the same? Don't we just care whether there is an > offline page or not? So just check (previous == null) == (mOfflinePageOfflineId > == null) ? Now we do care only about existence of the offline page. In case we decide to care also about the page itself, I would prefer to leave this check as it is. At least |mOfflineStatusChangeRunnable| does not make it clear to me that this is about offline page existence. https://codereview.chromium.org/2505763002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:226: public Long getOfflinePageOfflineId() { On 2016/11/16 16:25:05, dgn wrote: > add @Nullable Done.
lgtm Great job, Vitalii! Thanks for bearing with us through the review of this functionality. I love the outcome: dropping the OPDownloadBridge from a lot of NTP code (all of it?) and putting OPBridge instead (things will be more predictable and cleaner in your code). Also adding Offline ID in the snippet is the right choice. I know the patch is not as small as the previous one, but it eliminates bugs that would have been hard to track. Also, it is impressive that all this was achievable without having to change code on offline side. (Thanks to earlier work by NTP and Paquete, of course.) Thanks. Filip
lgtm, some context in the doc of requiresExactOfflinePage would be great.
+1 to dgn@'s doc comment lgtm https://codereview.chromium.org/2505763002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2505763002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:240: mOfflinePageBridge.selectPageForOnlineUrl(article.mUrl, 0, new Callback<OfflinePageItem>() { On 2016/11/16 13:47:37, vitaliii wrote: > On 2016/11/16 05:03:34, dewittj wrote: > > Possibly need to cancel this if the destruction event occurs. > > I do not understand which destruction and how to cancel. > dgn@ could you help me with this? There are other places where you have DestructionObservers in this file, comments say that the NTP might have gone away at that time. Does executing onResult still make sense after that event? Does |article| still make sense?
Patchset #6 (id:160001) has been deleted
Patchset #6 (id:180001) has been deleted
The CQ bit was checked by vitaliii@chromium.org to run a CQ dry run
vitaliii@chromium.org changed reviewers: + peconn@chromium.org
vitaliii@chromium.org changed reviewers: + peconn@chromium.org
+peconn@ to have a look at NTP desctruction related code.
+peconn@ to have a look at NTP desctruction related code.
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.
On 2016/11/16 18:35:45, vitaliii wrote: > +peconn@ to have a look at NTP desctruction related code. LGTM
The CQ bit was checked by vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, dewittj@chromium.org, dgn@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2505763002/#ps200001 (title: "handle NTP descruction in callbacks.")
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.
Description was changed from ========== [NTP] Use OfflineId instead of GUID to identify offline pages. Previosly we used GUID, because all methods to force opening offline version of the page required GUID. However, recently we added methods based on OfflineId and we were advised by Offline Pages team to move to OfflineId instead. This CL replaces GUID with OfflineId, usage of OfflinePageDownloadBridge with OfflinePageBridge. BUG=665454 ========== to ========== [NTP] Use OfflineId instead of GUID to identify offline pages. Previosly we used GUID, because all methods to force opening offline version of the page required GUID. However, recently we added methods based on OfflineId and we were advised by Offline Pages team to move to OfflineId instead. This CL replaces GUID with OfflineId, usage of OfflinePageDownloadBridge with OfflinePageBridge. BUG=665454 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== [NTP] Use OfflineId instead of GUID to identify offline pages. Previosly we used GUID, because all methods to force opening offline version of the page required GUID. However, recently we added methods based on OfflineId and we were advised by Offline Pages team to move to OfflineId instead. This CL replaces GUID with OfflineId, usage of OfflinePageDownloadBridge with OfflinePageBridge. BUG=665454 ========== to ========== [NTP] Use OfflineId instead of GUID to identify offline pages. Previosly we used GUID, because all methods to force opening offline version of the page required GUID. However, recently we added methods based on OfflineId and we were advised by Offline Pages team to move to OfflineId instead. This CL replaces GUID with OfflineId, usage of OfflinePageDownloadBridge with OfflinePageBridge. BUG=665454 Committed: https://crrev.com/3242a750958cd45273d18b22a8de1492f1c76b0d Cr-Commit-Position: refs/heads/master@{#432820} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3242a750958cd45273d18b22a8de1492f1c76b0d Cr-Commit-Position: refs/heads/master@{#432820} |
