Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2395203002:
NTP cards: Restructure change notifications. (Closed)
|
Created:
4 years, 2 months ago by Bernhard Bauer Modified:
4 years, 2 months ago CC:
chromium-reviews, ntp-dev+reviews_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNTP cards: Restructure change notifications.
This moves the logic for notifying about SuggestionSection updates into
SuggestionSection, so that the ItemGroup.Observer is agnostic of sections.
Also replaces the NewTabPageAdapter spy() in the NewTabPageAdapterTest with a
mocked AdapterDataObserver.
BUG=616090
Committed: https://crrev.com/1660bf28e21367fddd506158781f0ab190dc292f
Cr-Commit-Position: refs/heads/master@{#423957}
Patch Set 1 #Patch Set 2 : x #
Total comments: 16
Patch Set 3 : review #Patch Set 4 : review #Patch Set 5 : rebase #Patch Set 6 : x #Messages
Total messages: 36 (21 generated)
The CQ bit was checked by bauerb@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...
bauerb@chromium.org changed reviewers: + dgn@chromium.org
Please review.
https://codereview.chromium.org/2395203002/diff/20001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java (right): https://codereview.chromium.org/2395203002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:121: section.setStatus(CategoryStatus.SIGNED_OUT); This is probably also noteworthy -- setStatus() tries to create a new status item, and passing in LOADING_ERROR is actually not allowed (because that should cause the whole section to be cleared). It just triggers a Log.wtf() though (which then is ignored, I guess...).
The CQ bit was checked by mvanouwerkerk@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/2395203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemGroup.java (right): https://codereview.chromium.org/2395203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemGroup.java:22: interface Observer { It would be nice to have special case forms of these methods for when a single item has changed, like in Adapter. It's a common case in our code. https://codereview.chromium.org/2395203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemGroup.java:24: void onItemRangeChanged(ItemGroup group, int index, int count); While normally I also prefer 'index' for this kind of integers, the Adapter calls them 'position' and I think it would help maintain consistency if this interface also did that. Also, this naming allows for ambiguity about whether this is the item or group position, so I'd prefer 'itemPosition' and 'itemCount', again aligning with Adapter naming. https://codereview.chromium.org/2395203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemGroup.java:27: void onItemRangeAdded(ItemGroup group, int index, int count); nit: rename to onItemRangeInserted to align with Adapter#notifyItemRangeInserted
The CQ bit was checked by bauerb@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/2395203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemGroup.java (right): https://codereview.chromium.org/2395203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemGroup.java:22: interface Observer { On 2016/10/07 09:13:05, Michael van Ouwerkerk wrote: > It would be nice to have special case forms of these methods for when a single > item has changed, like in Adapter. It's a common case in our code. What's the advantage? All it would mean is that we'd have to implement twice as many methods... What I could do is add convenience methods into a base class that notify about changes, with a variant that takes an explicit item count and one that uses the default of 1. https://codereview.chromium.org/2395203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemGroup.java:24: void onItemRangeChanged(ItemGroup group, int index, int count); On 2016/10/07 09:13:05, Michael van Ouwerkerk wrote: > While normally I also prefer 'index' for this kind of integers, the Adapter > calls them 'position' and I think it would help maintain consistency if this > interface also did that. Also, this naming allows for ambiguity about whether > this is the item or group position, so I'd prefer 'itemPosition' and > 'itemCount', again aligning with Adapter naming. Done. https://codereview.chromium.org/2395203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemGroup.java:27: void onItemRangeAdded(ItemGroup group, int index, int count); On 2016/10/07 09:13:05, Michael van Ouwerkerk wrote: > nit: rename to onItemRangeInserted to align with Adapter#notifyItemRangeInserted Done.
https://codereview.chromium.org/2395203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemGroup.java (right): https://codereview.chromium.org/2395203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemGroup.java:22: interface Observer { On 2016/10/07 13:56:36, Bernhard Bauer wrote: > On 2016/10/07 09:13:05, Michael van Ouwerkerk wrote: > > It would be nice to have special case forms of these methods for when a single > > item has changed, like in Adapter. It's a common case in our code. > > What's the advantage? All it would mean is that we'd have to implement twice as > many methods... > > What I could do is add convenience methods into a base class that notify about > changes, with a variant that takes an explicit item count and one that uses the > default of 1. I don't feel too strongly about it. Could also go in a later CL once there's more benefit from it. And your idea could help make it cleaner.
lgtm
https://codereview.chromium.org/2395203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2395203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:449: public void onItemRangeAdded(ItemGroup group, int index, int count) { nit: IMO onItemRange*Inserted* would transmit the intent better, because "add" is usually at the end, while here we can notify about insertions in the middle of the list. Also more consistent with the original adapter method. https://codereview.chromium.org/2395203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:603: @VisibleForTesting this annotation can probably be removed now and the method made private. https://codereview.chromium.org/2395203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2395203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:180: private void notifySectionChanged(int itemCountBefore) { Thanks, it makes more sense for it to be here, since it has section specific logic. https://codereview.chromium.org/2395203002/diff/20001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2395203002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:735: adapter.finishInitialization(); not needed anymore, you can use NtpAdapter.create()
Thanks! https://codereview.chromium.org/2395203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemGroup.java (right): https://codereview.chromium.org/2395203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemGroup.java:22: interface Observer { On 2016/10/07 14:03:43, Michael van Ouwerkerk wrote: > On 2016/10/07 13:56:36, Bernhard Bauer wrote: > > On 2016/10/07 09:13:05, Michael van Ouwerkerk wrote: > > > It would be nice to have special case forms of these methods for when a > single > > > item has changed, like in Adapter. It's a common case in our code. > > > > What's the advantage? All it would mean is that we'd have to implement twice > as > > many methods... > > > > What I could do is add convenience methods into a base class that notify about > > changes, with a variant that takes an explicit item count and one that uses > the > > default of 1. > > I don't feel too strongly about it. Could also go in a later CL once there's > more benefit from it. And your idea could help make it cleaner. > Coincidentally, that's my plan anyway :)
https://codereview.chromium.org/2395203002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2395203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:449: public void onItemRangeAdded(ItemGroup group, int index, int count) { On 2016/10/07 14:05:32, dgn wrote: > nit: IMO onItemRange*Inserted* would transmit the intent better, because "add" > is usually at the end, while here we can notify about insertions in the middle > of the list. Also more consistent with the original adapter method. Yup, already changed to ...Inserted. https://codereview.chromium.org/2395203002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:603: @VisibleForTesting On 2016/10/07 14:05:32, dgn wrote: > this annotation can probably be removed now and the method made private. Yay! Done. https://codereview.chromium.org/2395203002/diff/20001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2395203002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:735: adapter.finishInitialization(); On 2016/10/07 14:05:32, dgn wrote: > not needed anymore, you can use NtpAdapter.create() Done, and inlined create() completely, per off-review discussion.
thanks! lgtm :)
The CQ bit was checked by bauerb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2395203002/#ps60001 (title: "review")
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/NewTabPageAdapterTest.java:
While running git apply --index -3 -p1;
error: patch failed:
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:10
Falling back to three-way merge...
Applied patch to
'chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java'
with conflicts.
U
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java
Patch:
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java
Index:
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java
diff --git
a/chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java
b/chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java
index
af1b473682d045a5032f02788a7d7ce209722386..d5974493665603a9c0f91f5bb1efd2ecee98d8f0
100644
---
a/chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java
+++
b/chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java
@@ -10,7 +10,6 @@ import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import static org.mockito.Mockito.any;
-import static org.mockito.Mockito.anyString;
import static org.mockito.Mockito.doNothing;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.spy;
@@ -19,10 +18,10 @@ import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.chromium.base.test.util.Matchers.greaterThanOrEqualTo;
-import static org.chromium.chrome.browser.ntp.cards.ContentSuggestionsTestUtils
- .createDummySuggestions;
+import static
org.chromium.chrome.browser.ntp.cards.ContentSuggestionsTestUtils.createDummySuggestions;
import android.support.annotation.Nullable;
+import android.support.v7.widget.RecyclerView.AdapterDataObserver;
import android.view.ContextMenu;
import android.view.Menu;
import android.view.MenuItem.OnMenuItemClickListener;
@@ -230,7 +229,7 @@ public class NewTabPageAdapterTest {
mSource.setInfoForCategory(KnownCategories.ARTICLES,
new SuggestionsCategoryInfo("Articles for you",
ContentSuggestionsCardLayout.FULL_CARD, false, true));
- mAdapter = NewTabPageAdapter.create(new MockNewTabPageManager(mSource),
null, null);
+ mAdapter = new NewTabPageAdapter(new MockNewTabPageManager(mSource),
null, null);
}
@After
@@ -430,20 +429,20 @@ public class NewTabPageAdapterTest {
assertItemsFor();
// Same when loading a new NTP.
- mAdapter = NewTabPageAdapter.create(new MockNewTabPageManager(mSource),
null, null);
+ mAdapter = new NewTabPageAdapter(new MockNewTabPageManager(mSource),
null, null);
assertItemsFor();
// Same for CATEGORY_EXPLICITLY_DISABLED.
mSource.setStatusForCategory(KnownCategories.ARTICLES,
CategoryStatus.AVAILABLE);
mSource.setSuggestionsForCategory(KnownCategories.ARTICLES, snippets);
- mAdapter = NewTabPageAdapter.create(new MockNewTabPageManager(mSource),
null, null);
+ mAdapter = new NewTabPageAdapter(new MockNewTabPageManager(mSource),
null, null);
assertItemsFor(section(5));
mSource.setStatusForCategory(
KnownCategories.ARTICLES,
CategoryStatus.CATEGORY_EXPLICITLY_DISABLED);
assertItemsFor();
// Same when loading a new NTP.
- mAdapter = NewTabPageAdapter.create(new MockNewTabPageManager(mSource),
null, null);
+ mAdapter = new NewTabPageAdapter(new MockNewTabPageManager(mSource),
null, null);
assertItemsFor();
}
@@ -464,7 +463,7 @@ public class NewTabPageAdapterTest {
assertItemsFor(section(4));
// But it disappears when loading a new NTP.
- mAdapter = NewTabPageAdapter.create(new MockNewTabPageManager(mSource),
null, null);
+ mAdapter = new NewTabPageAdapter(new MockNewTabPageManager(mSource),
null, null);
assertItemsFor();
}
@@ -486,8 +485,7 @@ public class NewTabPageAdapterTest {
"",
ContentSuggestionsCardLayout.MINIMAL_CARD, false, true));
// 1.1 - Initial state
- mAdapter =
- NewTabPageAdapter.create(new
MockNewTabPageManager(suggestionsSource), null, null);
+ mAdapter = new NewTabPageAdapter(new
MockNewTabPageManager(suggestionsSource), null, null);
assertItemsFor(sectionWithStatusCard());
// 1.2 - With suggestions
@@ -512,8 +510,7 @@ public class NewTabPageAdapterTest {
"",
ContentSuggestionsCardLayout.MINIMAL_CARD, false, false));
// 2.1 - Initial state
- mAdapter =
- NewTabPageAdapter.create(new
MockNewTabPageManager(suggestionsSource), null, null);
+ mAdapter = new NewTabPageAdapter(new
MockNewTabPageManager(suggestionsSource), null, null);
assertItemsFor();
// 2.2 - With suggestions
@@ -545,8 +542,7 @@ public class NewTabPageAdapterTest {
"",
ContentSuggestionsCardLayout.MINIMAL_CARD, true, true));
// 1.1 - Initial state.
- mAdapter =
- NewTabPageAdapter.create(new
MockNewTabPageManager(suggestionsSource), null, null);
+ mAdapter = new NewTabPageAdapter(new
MockNewTabPageManager(suggestionsSource), null, null);
assertItemsFor(sectionWithStatusCardAndMoreButton());
// 1.2 - With suggestions.
@@ -571,8 +567,7 @@ public class NewTabPageAdapterTest {
"",
ContentSuggestionsCardLayout.MINIMAL_CARD, false, true));
// 2.1 - Initial state.
- mAdapter =
- NewTabPageAdapter.create(new
MockNewTabPageManager(suggestionsSource), null, null);
+ mAdapter = new NewTabPageAdapter(new
MockNewTabPageManager(suggestionsSource), null, null);
assertItemsFor(sectionWithStatusCard());
// 2.2 - With suggestions.
@@ -632,8 +627,7 @@ public class NewTabPageAdapterTest {
ContentSuggestionsCardLayout.MINIMAL_CARD, true, false));
mSource.setStatusForCategory(dynamicCategory1,
CategoryStatus.AVAILABLE);
mSource.setSuggestionsForCategory(dynamicCategory1, dynamics1);
- mAdapter =
- NewTabPageAdapter.create(new MockNewTabPageManager(mSource),
null, null); // Reload
+ mAdapter = new NewTabPageAdapter(new MockNewTabPageManager(mSource),
null, null); // Reload
assertItemsFor(section(3), sectionWithMoreButton(5));
int dynamicCategory2 = 1011;
@@ -643,8 +637,7 @@ public class NewTabPageAdapterTest {
ContentSuggestionsCardLayout.MINIMAL_CARD, false, false));
mSource.setStatusForCategory(dynamicCategory2,
CategoryStatus.AVAILABLE);
mSource.setSuggestionsForCategory(dynamicCategory2, dynamics2);
- mAdapter =
- NewTabPageAdapter.create(new MockNewTabPageManager(mSource),
null, null); // Reload
+ mAdapter = new NewTabPageAdapter(new MockNewTabPageManager(mSource),
null, null); // Reload
assertItemsFor(section(3), sectionWithMoreButton(5), section(11));
}
@@ -662,7 +655,7 @@ public class NewTabPageAdapterTest {
registerCategory(suggestionsSource, KnownCategories.DOWNLOADS, 0);
NewTabPageAdapter ntpAdapter =
- NewTabPageAdapter.create(new
MockNewTabPageManager(suggestionsSource), null, null);
+ new NewTabPageAdapter(new
MockNewTabPageManager(suggestionsSource), null, null);
List<ItemGroup> groups = ntpAdapter.getGroups();
assertEquals(basicGroupCount + 4, groups.size());
@@ -684,7 +677,7 @@ public class NewTabPageAdapterTest {
registerCategory(suggestionsSource, KnownCategories.BOOKMARKS, 0);
ntpAdapter =
- NewTabPageAdapter.create(new
MockNewTabPageManager(suggestionsSource), null, null);
+ new NewTabPageAdapter(new
MockNewTabPageManager(suggestionsSource), null, null);
groups = ntpAdapter.getGroups();
assertEquals(basicGroupCount + 4, groups.size());
@@ -705,7 +698,7 @@ public class NewTabPageAdapterTest {
registerCategory(suggestionsSource, KnownCategories.DOWNLOADS, 0);
ntpAdapter =
- NewTabPageAdapter.create(new
MockNewTabPageManager(suggestionsSource), null, null);
+ new NewTabPageAdapter(new
MockNewTabPageManager(suggestionsSource), null, null);
// The adapter is already initialised, it will not accept new
categories anymore.
registerCategory(suggestionsSource, 42, 1);
@@ -731,10 +724,10 @@ public class NewTabPageAdapterTest {
doNothing().when(suggestionsSource).dismissSuggestion(any(SnippetArticle.class));
registerCategory(suggestionsSource, KnownCategories.ARTICLES, 3);
- NewTabPageAdapter adapter = spy(
- new NewTabPageAdapter(new
MockNewTabPageManager(suggestionsSource), null, null));
- adapter.finishInitialization();
- doNothing().when(adapter).announceItemRemoved(anyString());
+ NewTabPageAdapter adapter =
+ new NewTabPageAdapter(new
MockNewTabPageManager(suggestionsSource), null, null);
+ AdapterDataObserver dataObserver = mock(AdapterDataObserver.class);
+ adapter.registerAdapterDataObserver(dataObserver);
// Adapter content:
// Idx | Item
@@ -746,17 +739,20 @@ public class NewTabPageAdapterTest {
// 6 | Spacer
adapter.dismissItem(3); // Dismiss the second suggestion of the second
section.
- verify(adapter).notifyItemR…
(message too large)
The CQ bit was checked by bauerb@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 bauerb@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.
The CQ bit was checked by bauerb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org, mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2395203002/#ps100001 (title: "x")
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 #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== NTP cards: Restructure change notifications. This moves the logic for notifying about SuggestionSection updates into SuggestionSection, so that the ItemGroup.Observer is agnostic of sections. Also replaces the NewTabPageAdapter spy() in the NewTabPageAdapterTest with a mocked AdapterDataObserver. BUG=616090 ========== to ========== NTP cards: Restructure change notifications. This moves the logic for notifying about SuggestionSection updates into SuggestionSection, so that the ItemGroup.Observer is agnostic of sections. Also replaces the NewTabPageAdapter spy() in the NewTabPageAdapterTest with a mocked AdapterDataObserver. BUG=616090 Committed: https://crrev.com/1660bf28e21367fddd506158781f0ab190dc292f Cr-Commit-Position: refs/heads/master@{#423957} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/1660bf28e21367fddd506158781f0ab190dc292f Cr-Commit-Position: refs/heads/master@{#423957} |
