📰 Refactor SuggestionsSection change detection

[NTP Client] Refactor SuggestionsSection change detection

The change detection and notification used to rely on keeping track
of children sizes, manually notifying on certain changes. This CL
finishes making its children OptionalLeaves, and makes
SuggestionList the only one capable of modifying the list of
suggestions. This allows letting these children notify of changes
themselves.

BUG=616090
Committed: https://crrev.com/4e2a6810fd84198ce9d8062d69393076b48a91e9
Cr-Commit-Position: refs/heads/master@{#430929}

[dgn, 2016-11-04 17:48:16]

Description was changed from

==========
[NTP Client] Refactor SuggestionsSection change detection

The change detection and notification used to rely on keeping track
of children sizes, manually notifying on certain changes. This CL
finishes making its children OptionalLeaves, and makes
SuggestionList the only one capable of modifying the list of
suggestions. This allows letting these children notify of changes
themselves.

Also enables the spinner when a FetchMore is started.

BUG=616090
==========

to

==========
[NTP Client] Refactor SuggestionsSection change detection

The change detection and notification used to rely on keeping track
of children sizes, manually notifying on certain changes. This CL
finishes making its children OptionalLeaves, and makes
SuggestionList the only one capable of modifying the list of
suggestions. This allows letting these children notify of changes
themselves.

Also enables the spinner when a FetchMore is started.

BUG=616090
==========

[dgn, 2016-11-07 17:42:34]

Description was changed from

==========
[NTP Client] Refactor SuggestionsSection change detection

The change detection and notification used to rely on keeping track
of children sizes, manually notifying on certain changes. This CL
finishes making its children OptionalLeaves, and makes
SuggestionList the only one capable of modifying the list of
suggestions. This allows letting these children notify of changes
themselves.

Also enables the spinner when a FetchMore is started.

BUG=616090
==========

to

==========
[NTP Client] Refactor SuggestionsSection change detection

The change detection and notification used to rely on keeping track
of children sizes, manually notifying on certain changes. This CL
finishes making its children OptionalLeaves, and makes
SuggestionList the only one capable of modifying the list of
suggestions. This allows letting these children notify of changes
themselves.

BUG=616090
==========

[dgn, 2016-11-07 17:42:40]

The CQ bit was checked by dgn@chromium.org to run a CQ dry run

[dgn, 2016-11-07 17:42:51]

dgn@chromium.org changed reviewers:
+ mvanouwerkerk@chromium.org

[dgn, 2016-11-07 17:42:51]

PTAL

[commit-bot: I haz the power, 2016-11-07 17:43:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-07 18:34:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-07 18:34:11]

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...)

[Michael van Ouwerkerk, 2016-11-08 11:01:31]

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionItem.java
(right):

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionItem.java:25:
mCategoryInfo = section.getCategoryInfo();
Why keep the mCategoryInfo field at all if it's available on mParentSection?

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/OptionalLeaf.java
(right):

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/OptionalLeaf.java:22:
// We can't use isShown() to initialise visibility here. If the child should be
initialised
I think we discussed this bit offline. Could NodeParent somehow tell
OptionalLeaf true or false?

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ProgressItem.java
(right):

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ProgressItem.java:31:
public boolean isVisible() {
This seems to duplicate isShown(), can it be removed?

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusItem.java
(right):

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusItem.java:17:
protected final SuggestionsSection mParentSection;
Why protected? Member fields should normally be private, and can be exposed with
a getter if needed.

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
(right):

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:98:
if (removedIndex == -1) return false;
When would this happen? Should we throw an exception here?

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:132:
private void initialiseChildren() {
nit: "initialize" is more common in chromium.

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:185:
if (!mSuggestionsList.remove(suggestion)) return;
How about:
if (mSuggestionsList.remove(suggestion)) refreshChildrenVisibility();

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:224:
if (suggestions.removeAll(mSuggestionsList.mSuggestions)) {
Ugh, that's a private field :-)

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java
(right):

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsTestUtils.java:52:
public static SuggestionsSection createSection(SuggestionsCategoryInfo info,
NodeParent parent,
Why do we have this method? It doesn't seem to do anything but pass its
arguments to the constructor.

[dgn, 2016-11-09 12:01:12]

The CQ bit was checked by dgn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 12:01:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dgn, 2016-11-09 12:02:24]

PTAL

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionItem.java
(right):

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionItem.java:25:
mCategoryInfo = section.getCategoryInfo();
On 2016/11/08 11:01:31, Michael van Ouwerkerk wrote:
> Why keep the mCategoryInfo field at all if it's available on mParentSection?

Done.

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/OptionalLeaf.java
(right):

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/OptionalLeaf.java:22:
// We can't use isShown() to initialise visibility here. If the child should be
initialised
On 2016/11/08 11:01:31, Michael van Ouwerkerk wrote:
> I think we discussed this bit offline. Could NodeParent somehow tell
> OptionalLeaf true or false?

I tried a few, things, ends up being quite a big change and still wasn't super
satisfying. I ended up with separating the state that the recyclerView is aware
of (isShown()) and the one that it should be considering the current data
(shouldBeShown). That way we know that visibility changes come with a
notification of the adapter. WDYT?

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ProgressItem.java
(right):

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ProgressItem.java:31:
public boolean isVisible() {
On 2016/11/08 11:01:31, Michael van Ouwerkerk wrote:
> This seems to duplicate isShown(), can it be removed?

Done.

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusItem.java
(right):

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusItem.java:17:
protected final SuggestionsSection mParentSection;
On 2016/11/08 11:01:31, Michael van Ouwerkerk wrote:
> Why protected? Member fields should normally be private, and can be exposed
with
> a getter if needed.

Removed, just keep the description now.

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
(right):

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:98:
if (removedIndex == -1) return false;
On 2016/11/08 11:01:31, Michael van Ouwerkerk wrote:
> When would this happen? Should we throw an exception here?

It would happen if the provided suggestion is not in the list. That's carried
over from the previous code. Throwing.

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:132:
private void initialiseChildren() {
On 2016/11/08 11:01:31, Michael van Ouwerkerk wrote:
> nit: "initialize" is more common in chromium.

Done.

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:185:
if (!mSuggestionsList.remove(suggestion)) return;
On 2016/11/08 11:01:31, Michael van Ouwerkerk wrote:
> How about:
> if (mSuggestionsList.remove(suggestion)) refreshChildrenVisibility();

Obsolete.

https://codereview.chromium.org/2470913009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:224:
if (suggestions.removeAll(mSuggestionsList.mSuggestions)) {
On 2016/11/08 11:01:31, Michael van Ouwerkerk wrote:
> Ugh, that's a private field :-)

Yes, but the plan is to get rid of that bunch, so I didn't want to bother adding
a getter that will be removed right away. The class is inner private anyway :/
Will look into that next. I don't have much success with fetching more
suggestions.

https://codereview.chromium.org/2470913009/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
(right):

https://codereview.chromium.org/2470913009/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:178:
private void refreshChildrenVisibility() {
refreshing them could be done a bit more automatically, by having a
SuggestionsChangeListener on SuggestionsList, but not sure it's worth it. Maybe
if we move SuggestionsList out of this class.

[Michael van Ouwerkerk, 2016-11-09 13:42:01]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 13:42:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Michael van Ouwerkerk, 2016-11-09 13:53:17]

lgtm with nits

https://codereview.chromium.org/2470913009/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/OptionalLeaf.java
(right):

https://codereview.chromium.org/2470913009/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/OptionalLeaf.java:25:
// We can't use isShown() to initialise visibility here. If the child should be
initialised
s/isShown/isVisible/

https://codereview.chromium.org/2470913009/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/OptionalLeaf.java:85:
if (mCurrentVisibility == newVisibility) return;
I'd prefer it if member and argument say "visible" rather than "visibility".
Would it still read ok to you if the argument is just "visible" and the member
"mVisible"?

[dgn, 2016-11-09 14:21:49]

The CQ bit was checked by dgn@chromium.org

[dgn, 2016-11-09 14:21:50]

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/2470913009/#ps100001
(title: "address comments")

[commit-bot: I haz the power, 2016-11-09 14:21:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 14:50:50]

Description was changed from

==========
[NTP Client] Refactor SuggestionsSection change detection

The change detection and notification used to rely on keeping track
of children sizes, manually notifying on certain changes. This CL
finishes making its children OptionalLeaves, and makes
SuggestionList the only one capable of modifying the list of
suggestions. This allows letting these children notify of changes
themselves.

BUG=616090
==========

to

==========
[NTP Client] Refactor SuggestionsSection change detection

The change detection and notification used to rely on keeping track
of children sizes, manually notifying on certain changes. This CL
finishes making its children OptionalLeaves, and makes
SuggestionList the only one capable of modifying the list of
suggestions. This allows letting these children notify of changes
themselves.

BUG=616090
==========

[commit-bot: I haz the power, 2016-11-09 14:50:51]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-09 14:53:07]

Description was changed from

==========
[NTP Client] Refactor SuggestionsSection change detection

The change detection and notification used to rely on keeping track
of children sizes, manually notifying on certain changes. This CL
finishes making its children OptionalLeaves, and makes
SuggestionList the only one capable of modifying the list of
suggestions. This allows letting these children notify of changes
themselves.

BUG=616090
==========

to

==========
[NTP Client] Refactor SuggestionsSection change detection

The change detection and notification used to rely on keeping track
of children sizes, manually notifying on certain changes. This CL
finishes making its children OptionalLeaves, and makes
SuggestionList the only one capable of modifying the list of
suggestions. This allows letting these children notify of changes
themselves.

BUG=616090

Committed: https://crrev.com/4e2a6810fd84198ce9d8062d69393076b48a91e9
Cr-Commit-Position: refs/heads/master@{#430929}
==========

[commit-bot: I haz the power, 2016-11-09 14:53:08]

Patchset 6 (id:??) landed as
https://crrev.com/4e2a6810fd84198ce9d8062d69393076b48a91e9
Cr-Commit-Position: refs/heads/master@{#430929}