Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2639933003:
[NTP suggestions UI] Track precise count of suggestions seen. (Closed)
Description[NTP suggestions UI] Track precise count of suggestions seen.
Sometimes, more than the first suggestion is marked as seen before the
user scrolls below the fold.
This CL makes the updates more fine-grained and thus more robust. The
section will keep track of the total count of suggestions shown to the
user and update only the remaining ones.
BUG=674163
Review-Url: https://codereview.chromium.org/2639933003
Cr-Commit-Position: refs/heads/master@{#444879}
Committed: https://chromium.googlesource.com/chromium/src/+/29f7cd135146bf1ac2e4020d04014f69fce261ac
Patch Set 1 #
Total comments: 12
Patch Set 2 : Rebase #Patch Set 3 : Nicolas' comments #
Total comments: 18
Patch Set 4 : Nicolas' comments #2 #
Total comments: 2
Patch Set 5 : Nicolas' comments #3 #Patch Set 6 : Rebase #Patch Set 7 : Fix rebase #Patch Set 8 : Fix rebase #2 #
Dependent Patchsets: Messages
Total messages: 31 (16 generated)
Description was changed from ========== [NTP suggestions UI] Track precise count of suggestions seen. Sometimes, more than the first suggestion is marked as seen before the user scrolls below the fold. This CL makes the updates more fine-grained and thus more robust. The section will keep track of the total count of suggestions shown to the user and update only the remaining ones. BUG= ========== to ========== [NTP suggestions UI] Track precise count of suggestions seen. Sometimes, more than the first suggestion is marked as seen before the user scrolls below the fold. This CL makes the updates more fine-grained and thus more robust. The section will keep track of the total count of suggestions shown to the user and update only the remaining ones. BUG=674163 ==========
jkrcal@chromium.org changed reviewers: + dgn@chromium.org
Nicolas, could you PTAL?
https://codereview.chromium.org/2639933003/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/2639933003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:333: if (mNumberOfSuggestionsSeen == getSuggestionsCount()) { s/==/>= ? after a refresh or a dismiss the current number of suggestions could be lower. https://codereview.chromium.org/2639933003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:338: if (mNumberOfSuggestionsSeen > 0) { looking at the implementation of childSeen you might be missing a -1 somewhere up there https://codereview.chromium.org/2639933003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:347: int targetSize = suggestions.size() - mNumberOfSuggestionsSeen; maybe use a constant instead of suggestions.size() here? what if the fetch returns only 3 suggestions? https://codereview.chromium.org/2639933003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:354: suggestions.subList(targetSize, suggestions.size()).clear(); could you please add a tests that ensures that this works properly? I sense a off-by-one error here.
Patchset #2 (id:20001) has been deleted
Thanks! PTAL, again. https://codereview.chromium.org/2639933003/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/2639933003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:333: if (mNumberOfSuggestionsSeen == getSuggestionsCount()) { On 2017/01/18 18:23:13, dgn wrote: > s/==/>= ? after a refresh or a dismiss the current number of suggestions could > be lower. Sure, good point! Added a unit-test for that. https://codereview.chromium.org/2639933003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:338: if (mNumberOfSuggestionsSeen > 0) { On 2017/01/18 18:23:13, dgn wrote: > looking at the implementation of childSeen you might be missing a -1 somewhere > up there I think it is correct. I tried to clarify the code of childSeen. https://codereview.chromium.org/2639933003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:347: int targetSize = suggestions.size() - mNumberOfSuggestionsSeen; On 2017/01/18 18:23:13, dgn wrote: > maybe use a constant instead of suggestions.size() here? what if the fetch > returns only 3 suggestions? I believe that we should show as many as is returned by the server. Anyway, your comment made me aware of that targetCountToAppend needs to be at least 0 ;) Added a unit-test for that. https://codereview.chromium.org/2639933003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:354: suggestions.subList(targetSize, suggestions.size()).clear(); On 2017/01/18 18:23:13, dgn wrote: > could you please add a tests that ensures that this works properly? I sense a > off-by-one error here. Sure. I hope sizes={1,2,all} is general enough :) When expanding the unit-tests I realized some of the previous checks were not good enough, more changes there.
Nice, thanks! a few more remarks https://codereview.chromium.org/2639933003/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/2639933003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:347: int targetSize = suggestions.size() - mNumberOfSuggestionsSeen; On 2017/01/19 10:48:52, jkrcal wrote: > On 2017/01/18 18:23:13, dgn wrote: > > maybe use a constant instead of suggestions.size() here? what if the fetch > > returns only 3 suggestions? > > I believe that we should show as many as is returned by the server. Discarding the stale unseen ones makes sense, no need to keep them. But what if the server runs out of suggestions, wants to return 10 new things but only finds 3? Should we discard 2 of the new 3 just because the user has seen 2 stale ones? shouldn't we append the new 3 and have 5 total? That would require some change somewhere to let the server specify how many results it wants to show per section. New value in category info maybe? > Anyway, your > comment made me aware of that targetCountToAppend needs to be at least 0 ;) > Added a unit-test for that. https://codereview.chromium.org/2639933003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2639933003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:50: // have not been impressed, yet. I'm still a bit uncomfortable about the use of "impression" here, considering that we define it somewhere else as "2/3 of the item is visible", while here it's just whether it was bound to the recyclerview. https://codereview.chromium.org/2639933003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:270: // As asserted above, first suggestion has position 1, etc., so the position of this ah thanks the comment helps :) the variable just for renaming seems a bit excessive though ^^ I think that would just generate warnings in android studio or other linters. https://codereview.chromium.org/2639933003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:337: if (mNumberOfSuggestionsSeen > 0) { is this branch needed? if mNumberOfSuggestionsSeen == 0, we reach the same result at the end and don't run much more code https://codereview.chromium.org/2639933003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:342: // Make sure that {@code mSuggestionsList} will contain as many elements as newly nit (should have said that on the previous cl, sorry): the {@code } tags are only for javadoc (the comment blocks that start with "/**") https://codereview.chromium.org/2639933003/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java (right): https://codereview.chromium.org/2639933003/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:430: createSectionWithSuggestions(new ArrayList<SnippetArticle>(snippets)); nit: you can use diamond here: "new ArrayList<>(snippets)", the compiler knows the generic type here. Also applies to a few more places below https://codereview.chromium.org/2639933003/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:434: section.childSeen(1); nit: can you use bind here? IMO that is closer to "testing behaviour rather than implementation", as this makes a more legitimate the public interface than childSeen (that could then be made private) We have SectionListTest#bindViewHolders(node, startIndex, endIndex) that could be moved to ContentSuggestionsTestUtils to be used elsewhere. https://codereview.chromium.org/2639933003/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:527: // We do not touch the current list of all has been seen. s/of/if https://codereview.chromium.org/2639933003/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:549: assertEquals(4, section.getSuggestionsCount()); shouldn't we test that we do have the old items in there? Also, your old and new suggestions (well, the first 3 at least) are equal in this test. What should it catch that the previous test would miss?
Thanks! Could you PTAL, again? https://codereview.chromium.org/2639933003/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/2639933003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:347: int targetSize = suggestions.size() - mNumberOfSuggestionsSeen; On 2017/01/19 14:44:36, dgn wrote: > On 2017/01/19 10:48:52, jkrcal wrote: > > On 2017/01/18 18:23:13, dgn wrote: > > > maybe use a constant instead of suggestions.size() here? what if the fetch > > > returns only 3 suggestions? > > > > I believe that we should show as many as is returned by the server. > > Discarding the stale unseen ones makes sense, no need to keep them. But what if > the server runs out of suggestions, wants to return 10 new things but only finds > 3? Should we discard 2 of the new 3 just because the user has seen 2 stale ones? > shouldn't we append the new 3 and have 5 total? > > That would require some change somewhere to let the server specify how many > results it wants to show per section. New value in category info maybe? > > > Anyway, your > > comment made me aware of that targetCountToAppend needs to be at least 0 ;) > > Added a unit-test for that. To me, this is too much added logic with little benefit. What should other providers specify (that never know in advance how many items will they have)? If you insist, can we make it just a TODO in this CL? https://codereview.chromium.org/2639933003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2639933003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:50: // have not been impressed, yet. On 2017/01/19 14:44:37, dgn wrote: > I'm still a bit uncomfortable about the use of "impression" here, considering > that we define it somewhere else as "2/3 of the item is visible", while here > it's just whether it was bound to the recyclerview. Sure, should not be there, rephrased. https://codereview.chromium.org/2639933003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:270: // As asserted above, first suggestion has position 1, etc., so the position of this On 2017/01/19 14:44:37, dgn wrote: > ah thanks the comment helps :) the variable just for renaming seems a bit > excessive though ^^ I think that would just generate warnings in android studio > or other linters. Done. https://codereview.chromium.org/2639933003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:337: if (mNumberOfSuggestionsSeen > 0) { On 2017/01/19 14:44:37, dgn wrote: > is this branch needed? if mNumberOfSuggestionsSeen == 0, we reach the same > result at the end and don't run much more code In my opinion, distinguishing the two cases makes the reasoning way clearer for the "==0" case. Not feeling strongly, though. If you insist, I can remove it. A alternative might be moving the following out of the if: Log.d(TAG, "setSuggestions: keeping the first %d suggestion", mNumberOfSuggestionsSeen); mSuggestionsList.clearAllButFirstN(mNumberOfSuggestionsSeen); This way, we can remove the else branch and only enclose in the if the somewhat hard-to-parse filtering. https://codereview.chromium.org/2639933003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:342: // Make sure that {@code mSuggestionsList} will contain as many elements as newly On 2017/01/19 14:44:37, dgn wrote: > nit (should have said that on the previous cl, sorry): the {@code } tags are > only for javadoc (the comment blocks that start with "/**") Removed. https://codereview.chromium.org/2639933003/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java (right): https://codereview.chromium.org/2639933003/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:430: createSectionWithSuggestions(new ArrayList<SnippetArticle>(snippets)); On 2017/01/19 14:44:37, dgn wrote: > nit: you can use diamond here: "new ArrayList<>(snippets)", the compiler knows > the generic type here. Also applies to a few more places below Done. https://codereview.chromium.org/2639933003/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:434: section.childSeen(1); On 2017/01/19 14:44:37, dgn wrote: > nit: can you use bind here? IMO that is closer to "testing behaviour rather than > implementation", as this makes a more legitimate the public interface than > childSeen (that could then be made private) > > We have SectionListTest#bindViewHolders(node, startIndex, endIndex) that could > be moved to ContentSuggestionsTestUtils to be used elsewhere. Done. When doing it, I had to touch Adapter tests and removed one of them that seemed useless. https://codereview.chromium.org/2639933003/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:527: // We do not touch the current list of all has been seen. On 2017/01/19 14:44:37, dgn wrote: > s/of/if Done. https://codereview.chromium.org/2639933003/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:549: assertEquals(4, section.getSuggestionsCount()); On 2017/01/19 14:44:37, dgn wrote: > shouldn't we test that we do have the old items in there? Also, your old and > new suggestions (well, the first 3 at least) are equal in this test. > > What should it catch that the previous test would miss? Done.
lgtm :) https://codereview.chromium.org/2639933003/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/2639933003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:347: int targetSize = suggestions.size() - mNumberOfSuggestionsSeen; On 2017/01/19 15:39:57, jkrcal wrote: > On 2017/01/19 14:44:36, dgn wrote: > > On 2017/01/19 10:48:52, jkrcal wrote: > > > On 2017/01/18 18:23:13, dgn wrote: > > > > maybe use a constant instead of suggestions.size() here? what if the fetch > > > > returns only 3 suggestions? > > > > > > I believe that we should show as many as is returned by the server. > > > > Discarding the stale unseen ones makes sense, no need to keep them. But what > if > > the server runs out of suggestions, wants to return 10 new things but only > finds > > 3? Should we discard 2 of the new 3 just because the user has seen 2 stale > ones? > > shouldn't we append the new 3 and have 5 total? > > > > That would require some change somewhere to let the server specify how many > > results it wants to show per section. New value in category info maybe? > > > > > Anyway, your > > > comment made me aware of that targetCountToAppend needs to be at least 0 ;) > > > Added a unit-test for that. > > To me, this is too much added logic with little benefit. What should other > providers specify (that never know in advance how many items will they have)? > > If you insist, can we make it just a TODO in this CL? Don't all of them have this info, for example kMaxBookmarks=10 for bookmarks? TODO is probably unnecessary, we can just mention it in the bug and have a follow up CL if needed https://codereview.chromium.org/2639933003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2639933003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:337: if (mNumberOfSuggestionsSeen > 0) { On 2017/01/19 15:39:57, jkrcal wrote: > On 2017/01/19 14:44:37, dgn wrote: > > is this branch needed? if mNumberOfSuggestionsSeen == 0, we reach the same > > result at the end and don't run much more code > > In my opinion, distinguishing the two cases makes the reasoning way clearer for > the "==0" case. > > Not feeling strongly, though. If you insist, I can remove it. > > A alternative might be moving the following out of the if: > Log.d(TAG, "setSuggestions: keeping the first %d suggestion", > mNumberOfSuggestionsSeen); > mSuggestionsList.clearAllButFirstN(mNumberOfSuggestionsSeen); > > This way, we can remove the else branch and only enclose in the if the somewhat > hard-to-parse filtering. keeping the filtering only if we kept some suggestions sounds good, yes https://codereview.chromium.org/2639933003/diff/80001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SectionListTest.java (right): https://codereview.chromium.org/2639933003/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SectionListTest.java:147: ContentSuggestionsTestUtils.bindViewHolders(sectionList); nit: import static?
Thanks! https://codereview.chromium.org/2639933003/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/2639933003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:347: int targetSize = suggestions.size() - mNumberOfSuggestionsSeen; On 2017/01/19 17:15:34, dgn wrote: > On 2017/01/19 15:39:57, jkrcal wrote: > > On 2017/01/19 14:44:36, dgn wrote: > > > On 2017/01/19 10:48:52, jkrcal wrote: > > > > On 2017/01/18 18:23:13, dgn wrote: > > > > > maybe use a constant instead of suggestions.size() here? what if the > fetch > > > > > returns only 3 suggestions? > > > > > > > > I believe that we should show as many as is returned by the server. > > > > > > Discarding the stale unseen ones makes sense, no need to keep them. But what > > if > > > the server runs out of suggestions, wants to return 10 new things but only > > finds > > > 3? Should we discard 2 of the new 3 just because the user has seen 2 stale > > ones? > > > shouldn't we append the new 3 and have 5 total? > > > > > > That would require some change somewhere to let the server specify how many > > > results it wants to show per section. New value in category info maybe? > > > > > > > Anyway, your > > > > comment made me aware of that targetCountToAppend needs to be at least 0 > ;) > > > > Added a unit-test for that. > > > > To me, this is too much added logic with little benefit. What should other > > providers specify (that never know in advance how many items will they have)? > > > > If you insist, can we make it just a TODO in this CL? > > Don't all of them have this info, for example kMaxBookmarks=10 for bookmarks? > > TODO is probably unnecessary, we can just mention it in the bug and have a > follow up CL if needed Added a comment to the bug. https://codereview.chromium.org/2639933003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2639933003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:337: if (mNumberOfSuggestionsSeen > 0) { On 2017/01/19 17:15:34, dgn wrote: > On 2017/01/19 15:39:57, jkrcal wrote: > > On 2017/01/19 14:44:37, dgn wrote: > > > is this branch needed? if mNumberOfSuggestionsSeen == 0, we reach the same > > > result at the end and don't run much more code > > > > In my opinion, distinguishing the two cases makes the reasoning way clearer > for > > the "==0" case. > > > > Not feeling strongly, though. If you insist, I can remove it. > > > > A alternative might be moving the following out of the if: > > Log.d(TAG, "setSuggestions: keeping the first %d suggestion", > > mNumberOfSuggestionsSeen); > > mSuggestionsList.clearAllButFirstN(mNumberOfSuggestionsSeen); > > > > This way, we can remove the else branch and only enclose in the if the > somewhat > > hard-to-parse filtering. > > keeping the filtering only if we kept some suggestions sounds good, yes Done. https://codereview.chromium.org/2639933003/diff/80001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SectionListTest.java (right): https://codereview.chromium.org/2639933003/diff/80001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SectionListTest.java:147: ContentSuggestionsTestUtils.bindViewHolders(sectionList); On 2017/01/19 17:15:34, dgn wrote: > nit: import static? Done.
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org Link to the patchset: https://codereview.chromium.org/2639933003/#ps100001 (title: "Nicolas' comments #3")
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 patch for
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SectionListTest.java:
While running git apply --index -p1;
error: patch failed:
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SectionListTest.java:23
error:
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SectionListTest.java:
patch does not apply
Patch:
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SectionListTest.java
Index:
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SectionListTest.java
diff --git
a/chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SectionListTest.java
b/chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SectionListTest.java
index
452238ffd3a4d5c17d800c7f682cafb976718005..ede8e96d1998894e9c25fdb2f0a50729134a2169
100644
---
a/chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SectionListTest.java
+++
b/chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SectionListTest.java
@@ -9,6 +9,7 @@ import static org.junit.Assert.assertThat;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
+import static
org.chromium.chrome.browser.ntp.cards.ContentSuggestionsTestUtils.bindViewHolders;
import static
org.chromium.chrome.browser.ntp.cards.ContentSuggestionsTestUtils.createDummySuggestions;
import static
org.chromium.chrome.browser.ntp.cards.ContentSuggestionsTestUtils.registerCategory;
@@ -23,17 +24,13 @@ import org.chromium.base.Callback;
import org.chromium.base.test.util.Feature;
import org.chromium.chrome.browser.ntp.NewTabPageView.NewTabPageManager;
import
org.chromium.chrome.browser.ntp.cards.ContentSuggestionsTestUtils.CategoryInfoBuilder;
-import org.chromium.chrome.browser.ntp.snippets.CategoryInt;
import org.chromium.chrome.browser.ntp.snippets.FakeSuggestionsSource;
import org.chromium.chrome.browser.ntp.snippets.KnownCategories;
-import org.chromium.chrome.browser.ntp.snippets.SectionHeaderViewHolder;
import org.chromium.chrome.browser.ntp.snippets.SnippetArticle;
-import org.chromium.chrome.browser.ntp.snippets.SnippetArticleViewHolder;
import org.chromium.chrome.browser.offlinepages.OfflinePageBridge;
import org.chromium.chrome.browser.suggestions.SuggestionsMetricsReporter;
import org.chromium.testing.local.LocalRobolectricTestRunner;
-import java.util.Collections;
import java.util.List;
/**
@@ -159,32 +156,4 @@ public class SectionListTest {
.getPerSectionRank(),
equalTo(3));
}
-
- private static void bindViewHolders(InnerNode node) {
- bindViewHolders(node, 0, node.getItemCount());
- }
-
- private static void bindViewHolders(InnerNode node, int startIndex, int
endIndex) {
- for (int i = startIndex; i < endIndex; ++i) {
- node.onBindViewHolder(
- makeViewHolder(node.getItemViewType(i)), i,
Collections.emptyList());
- }
- }
-
- private static NewTabPageViewHolder makeViewHolder(@CategoryInt int
viewType) {
- switch (viewType) {
- case ItemViewType.SNIPPET:
- return mock(SnippetArticleViewHolder.class);
- case ItemViewType.HEADER:
- return mock(SectionHeaderViewHolder.class);
- case ItemViewType.STATUS:
- return mock(StatusCardViewHolder.class);
- case ItemViewType.ACTION:
- return mock(ActionItem.ViewHolder.class);
- case ItemViewType.PROGRESS:
- return mock(ProgressViewHolder.class);
- default:
- return mock(NewTabPageViewHolder.class);
- }
- }
}
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org Link to the patchset: https://codereview.chromium.org/2639933003/#ps120001 (title: "Rebase")
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
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 jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org Link to the patchset: https://codereview.chromium.org/2639933003/#ps140001 (title: "Fix rebase")
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org Link to the patchset: https://codereview.chromium.org/2639933003/#ps160001 (title: "Fix rebase #2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1484864024248980,
"parent_rev": "71cc0f83777bfa22f2286efd81b297680bfee4bd", "commit_rev":
"29f7cd135146bf1ac2e4020d04014f69fce261ac"}
Message was sent while issue was closed.
Description was changed from ========== [NTP suggestions UI] Track precise count of suggestions seen. Sometimes, more than the first suggestion is marked as seen before the user scrolls below the fold. This CL makes the updates more fine-grained and thus more robust. The section will keep track of the total count of suggestions shown to the user and update only the remaining ones. BUG=674163 ========== to ========== [NTP suggestions UI] Track precise count of suggestions seen. Sometimes, more than the first suggestion is marked as seen before the user scrolls below the fold. This CL makes the updates more fine-grained and thus more robust. The section will keep track of the total count of suggestions shown to the user and update only the remaining ones. BUG=674163 Review-Url: https://codereview.chromium.org/2639933003 Cr-Commit-Position: refs/heads/master@{#444879} Committed: https://chromium.googlesource.com/chromium/src/+/29f7cd135146bf1ac2e4020d0401... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as https://chromium.googlesource.com/chromium/src/+/29f7cd135146bf1ac2e4020d0401... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
