Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2360813004:
📰 Stop refreshing the whole NTP for every adapter change (Closed)
Description[NTP Client] Stop refreshing the whole NTP for every adapter change
Replace NTPAdapter#updateGroups() with more granular notifications.
Note: We are using SuggestionSections#getItems() quite often,
and that creates new lists every time, so it could still
be improved.
Preview: https://goo.gl/photos/UHWg4pfbkWWPHdGi8
BUG=647304, 643106, 647671
Committed: https://crrev.com/509bf6e55d629763e4ba67a123c575de1d60c686
Cr-Commit-Position: refs/heads/master@{#421806}
Patch Set 1 #
Total comments: 27
Patch Set 2 : ready for review #
Total comments: 9
Patch Set 3 : rebase and address comments #Patch Set 4 : #
Total comments: 5
Patch Set 5 : address comments #Patch Set 6 : rebase #Patch Set 7 : rebase again -_- #Messages
Total messages: 52 (36 generated)
Description was changed from ========== [NTP Client] Stop refreshing the whole NTP for every adapter change Remove NTPAdapter#updateGroups(), make more granular notifications BUG=647304 ========== to ========== [NTP Client] Stop refreshing the whole NTP for every adapter change Remove NTPAdapter#updateGroups(), make more granular notifications BUG=647304 ==========
dgn@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemGroup.java (right): https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemGroup.java:21: void notifyGroupChange(ItemGroup group, int itemCountBefore, int itemCountAfter); s/Change/Changed/ to match the others methods and RecyclerView.Adapter#notifyItemChanged https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemGroup.java:22: void notifyItemRemoved(ItemGroup group, int itemIndex); Rename itemIndex to position or itemPosition to align with RecyclerView.Adapter#notifyItemRemoved(int position) https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemGroup.java:23: void notifyFullGroupRefresh(); This is a strange method as it affects other groups also. Let's start with documenting it to clarify what this is meant to do. https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:227: ItemGroup section = mSections.get(category); How is section ever not null here, as the line above removed it from the map? Anyway, Map#remove returns the removed value, so no need to get it separately. https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:370: public void notifyItemRemoved(ItemGroup group, int itemIndex) { @Override https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:370: public void notifyItemRemoved(ItemGroup group, int itemIndex) { s/itemIndex/position/ https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:378: int pos = getGroupPositionOffset(group); nit: s/pos/positionStart/ https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:379: mGroups.remove(group); If the group is in mSections it needs to be removed there as well. (as discussed offline, all callers separately remove it from mSections, but you already started refactoring this method) https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:381: if (mGroups.size() <= 3 /* above-the-fold + footer + spacer*/) { I'd like to avoid this kind of hard-coded magic numbers. How about: "if mSections.isEmpty()" and a check that mFooter and mBottomSpacer are in mGroups? https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:384: notifyItemRangeRemoved(pos, 2); Is it fine to call notifyItemRangeRemoved twice in quick succession? Would the animated effect be any different if you set up this method so it only calls notifyItemRangeRemoved once? https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:390: int pos = getGroupPositionOffset(group); s/pos/positionStart/ https://codereview.chromium.org/2360813004/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/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:96: int items = getItems().size(); s/items/itemCountBefore/ https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:110: int items = getItems().size(); s/items/itemCountBefore/
PTAL https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemGroup.java (right): https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemGroup.java:21: void notifyGroupChange(ItemGroup group, int itemCountBefore, int itemCountAfter); On 2016/09/26 15:14:46, Michael van Ouwerkerk wrote: > s/Change/Changed/ to match the others methods and > RecyclerView.Adapter#notifyItemChanged Done. https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemGroup.java:22: void notifyItemRemoved(ItemGroup group, int itemIndex); On 2016/09/26 15:14:46, Michael van Ouwerkerk wrote: > Rename itemIndex to position or itemPosition to align with > RecyclerView.Adapter#notifyItemRemoved(int position) Done. https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ItemGroup.java:23: void notifyFullGroupRefresh(); On 2016/09/26 15:14:46, Michael van Ouwerkerk wrote: > This is a strange method as it affects other groups also. Let's start with > documenting it to clarify what this is meant to do. Removed it. https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:227: ItemGroup section = mSections.get(category); On 2016/09/26 15:14:47, Michael van Ouwerkerk wrote: > How is section ever not null here, as the line above removed it from the map? > Anyway, Map#remove returns the removed value, so no need to get it separately. Rewrote this. https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:370: public void notifyItemRemoved(ItemGroup group, int itemIndex) { On 2016/09/26 15:14:47, Michael van Ouwerkerk wrote: > @Override Done. https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:370: public void notifyItemRemoved(ItemGroup group, int itemIndex) { On 2016/09/26 15:14:47, Michael van Ouwerkerk wrote: > s/itemIndex/position/ Done. https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:378: int pos = getGroupPositionOffset(group); On 2016/09/26 15:14:47, Michael van Ouwerkerk wrote: > nit: s/pos/positionStart/ Done. https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:379: mGroups.remove(group); On 2016/09/26 15:14:46, Michael van Ouwerkerk wrote: > If the group is in mSections it needs to be removed there as well. (as discussed > offline, all callers separately remove it from mSections, but you already > started refactoring this method) Done. https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:381: if (mGroups.size() <= 3 /* above-the-fold + footer + spacer*/) { On 2016/09/26 15:14:47, Michael van Ouwerkerk wrote: > I'd like to avoid this kind of hard-coded magic numbers. How about: "if > mSections.isEmpty()" and a check that mFooter and mBottomSpacer are in mGroups? Done. https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:384: notifyItemRangeRemoved(pos, 2); On 2016/09/26 15:14:47, Michael van Ouwerkerk wrote: > Is it fine to call notifyItemRangeRemoved twice in quick succession? Would the > animated effect be any different if you set up this method so it only calls > notifyItemRangeRemoved once? I can't see a visible change, but it's not complicated on our end to simplify it. https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:390: int pos = getGroupPositionOffset(group); On 2016/09/26 15:14:46, Michael van Ouwerkerk wrote: > s/pos/positionStart/ Done. https://codereview.chromium.org/2360813004/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/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:96: int items = getItems().size(); On 2016/09/26 15:14:47, Michael van Ouwerkerk wrote: > s/items/itemCountBefore/ Done. https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:110: int items = getItems().size(); On 2016/09/26 15:14:47, Michael van Ouwerkerk wrote: > s/items/itemCountBefore/ Done.
Description was changed from ========== [NTP Client] Stop refreshing the whole NTP for every adapter change Remove NTPAdapter#updateGroups(), make more granular notifications BUG=647304 ========== to ========== [NTP Client] Stop refreshing the whole NTP for every adapter change Replace NTPAdapter#updateGroups() with more granular notifications. Note: We are using SuggestionSections#getItems() quite often, and that creates new lists every time, so it could still be improved. Preview: https://goo.gl/photos/UHWg4pfbkWWPHdGi8 BUG=647304 ==========
https://codereview.chromium.org/2360813004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2360813004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:151: public void initializeSections() { Had to split the constructor in 2 to be able to configure the adapter spy before it's injected in the suggestion sections.
The CQ bit was checked by dgn@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
bauerb@chromium.org changed reviewers: + bauerb@chromium.org
https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2360813004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:384: notifyItemRangeRemoved(pos, 2); On 2016/09/27 12:58:41, dgn wrote: > On 2016/09/26 15:14:47, Michael van Ouwerkerk wrote: > > Is it fine to call notifyItemRangeRemoved twice in quick succession? Would the > > animated effect be any different if you set up this method so it only calls > > notifyItemRangeRemoved once? > > I can't see a visible change, but it's not complicated on our end to simplify > it. I think the RecyclerView just accumulates the changes and then runs the animation later. https://codereview.chromium.org/2360813004/diff/20001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2360813004/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:683: NewTabPageAdapter ntpa = spy(new NewTabPageAdapter(null, null, suggestionsSource, null)); Can you just use |adapter| instead of this abbreviation? https://codereview.chromium.org/2360813004/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:722: verify(ntpa).notifyItemRangeChanged(1, 3); // Header stays, 2 items eplaced by articles. Nit: "replaced" https://codereview.chromium.org/2360813004/diff/20001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java (right): https://codereview.chromium.org/2360813004/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:32: * Unit tests for {@link NewTabPageAdapter}. Update comment. https://codereview.chromium.org/2360813004/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:92: eq(section), eq(emptySectionCount), eq(suggestionCount + 1 /* header*/)); Space before closing comment
The CQ bit was checked by dgn@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 dgn@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2360813004/diff/20001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2360813004/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:683: NewTabPageAdapter ntpa = spy(new NewTabPageAdapter(null, null, suggestionsSource, null)); On 2016/09/27 14:10:39, Bernhard Bauer wrote: > Can you just use |adapter| instead of this abbreviation? Done. https://codereview.chromium.org/2360813004/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:722: verify(ntpa).notifyItemRangeChanged(1, 3); // Header stays, 2 items eplaced by articles. On 2016/09/27 14:10:39, Bernhard Bauer wrote: > Nit: "replaced" Done. https://codereview.chromium.org/2360813004/diff/20001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java (right): https://codereview.chromium.org/2360813004/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:32: * Unit tests for {@link NewTabPageAdapter}. On 2016/09/27 14:10:39, Bernhard Bauer wrote: > Update comment. Done. https://codereview.chromium.org/2360813004/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java:92: eq(section), eq(emptySectionCount), eq(suggestionCount + 1 /* header*/)); On 2016/09/27 14:10:39, Bernhard Bauer wrote: > Space before closing comment Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
https://codereview.chromium.org/2360813004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2360813004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:147: public void initializeSections() { Why was this split from the constructor? Afaict it's kind of for testing purposes, but this type of two-stage initialization (construct and init) can be fragile and reduce readability of the code. Also, the way this is used in the test, initializeSections() is called multiple times on the same adapter instance, which seems a bit counter to the concept of initialization which should happen only once, asap after construction. This multiple initialization is also something that does not happen in the production code path, leading to production code that is only there for test purposes, such as clearing mSections. https://codereview.chromium.org/2360813004/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java (right): https://codereview.chromium.org/2360813004/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java:14: public class ContentSuggestionsTestUtils { This is a static utility class: make it final and the constructor private, as it should never be instantiated.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [NTP Client] Stop refreshing the whole NTP for every adapter change Replace NTPAdapter#updateGroups() with more granular notifications. Note: We are using SuggestionSections#getItems() quite often, and that creates new lists every time, so it could still be improved. Preview: https://goo.gl/photos/UHWg4pfbkWWPHdGi8 BUG=647304 ========== to ========== [NTP Client] Stop refreshing the whole NTP for every adapter change Replace NTPAdapter#updateGroups() with more granular notifications. Note: We are using SuggestionSections#getItems() quite often, and that creates new lists every time, so it could still be improved. Preview: https://goo.gl/photos/UHWg4pfbkWWPHdGi8 BUG=647304,643106,647671 ==========
PTAL. I fixed the flickering MORE button issue that Patrick mentioned during the meeting. https://codereview.chromium.org/2360813004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2360813004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:147: public void initializeSections() { On 2016/09/28 09:36:23, Michael van Ouwerkerk wrote: > Why was this split from the constructor? Afaict it's kind of for testing > purposes, but this type of two-stage initialization (construct and init) can be > fragile and reduce readability of the code. Also, the way this is used in the > test, initializeSections() is called multiple times on the same adapter > instance, which seems a bit counter to the concept of initialization which > should happen only once, asap after construction. This multiple initialization > is also something that does not happen in the production code path, leading to > production code that is only there for test purposes, such as clearing > mSections. Because of what I suppose is another antipattern: we send the reference to the object being constructed to somewhere else. Because of that wrapping the adapter in Mockito.spy is not possible, so I was not able to verify the calls made on it from inside the SuggestionsSections. I made the 2-stage initialisation an implementation detail and exposed a factory function instead. Is that okay? Also I reverted the the initializeSection calls in the tests when not necessary. https://codereview.chromium.org/2360813004/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java (right): https://codereview.chromium.org/2360813004/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java:14: public class ContentSuggestionsTestUtils { On 2016/09/28 09:36:23, Michael van Ouwerkerk wrote: > This is a static utility class: make it final and the constructor private, as it > should never be instantiated. Done.
The CQ bit was checked by dgn@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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) 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 dgn@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.
https://codereview.chromium.org/2360813004/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2360813004/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:147: public void initializeSections() { On 2016/09/28 21:31:19, dgn wrote: > On 2016/09/28 09:36:23, Michael van Ouwerkerk wrote: > > Why was this split from the constructor? Afaict it's kind of for testing > > purposes, but this type of two-stage initialization (construct and init) can > be > > fragile and reduce readability of the code. Also, the way this is used in the > > test, initializeSections() is called multiple times on the same adapter > > instance, which seems a bit counter to the concept of initialization which > > should happen only once, asap after construction. This multiple initialization > > is also something that does not happen in the production code path, leading to > > production code that is only there for test purposes, such as clearing > > mSections. > > Because of what I suppose is another antipattern: we send the reference to the > object being constructed to somewhere else. Because of that wrapping the adapter > in Mockito.spy is not possible, so I was not able to verify the calls made on it > from inside the SuggestionsSections. > > I made the 2-stage initialisation an implementation detail and exposed a factory > function instead. Is that okay? Also I reverted the the initializeSection calls > in the tests when not necessary. Oh that makes sense. Thanks for clarifying! The create function and the docs also help.
lgtm
The CQ bit was checked by dgn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2360813004/#ps100001 (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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dgn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2360813004/#ps120001 (title: "rebase again -_-")
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 Client] Stop refreshing the whole NTP for every adapter change Replace NTPAdapter#updateGroups() with more granular notifications. Note: We are using SuggestionSections#getItems() quite often, and that creates new lists every time, so it could still be improved. Preview: https://goo.gl/photos/UHWg4pfbkWWPHdGi8 BUG=647304,643106,647671 ========== to ========== [NTP Client] Stop refreshing the whole NTP for every adapter change Replace NTPAdapter#updateGroups() with more granular notifications. Note: We are using SuggestionSections#getItems() quite often, and that creates new lists every time, so it could still be improved. Preview: https://goo.gl/photos/UHWg4pfbkWWPHdGi8 BUG=647304,643106,647671 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [NTP Client] Stop refreshing the whole NTP for every adapter change Replace NTPAdapter#updateGroups() with more granular notifications. Note: We are using SuggestionSections#getItems() quite often, and that creates new lists every time, so it could still be improved. Preview: https://goo.gl/photos/UHWg4pfbkWWPHdGi8 BUG=647304,643106,647671 ========== to ========== [NTP Client] Stop refreshing the whole NTP for every adapter change Replace NTPAdapter#updateGroups() with more granular notifications. Note: We are using SuggestionSections#getItems() quite often, and that creates new lists every time, so it could still be improved. Preview: https://goo.gl/photos/UHWg4pfbkWWPHdGi8 BUG=647304,643106,647671 Committed: https://crrev.com/509bf6e55d629763e4ba67a123c575de1d60c686 Cr-Commit-Position: refs/heads/master@{#421806} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/509bf6e55d629763e4ba67a123c575de1d60c686 Cr-Commit-Position: refs/heads/master@{#421806} |
