[Android NTP] Cache number of items under child nodes

[Android NTP] Cache number of items under child nodes

For inner nodes this makes getItemCount() more efficient, and for any child node
in general adds assertions that it notifies about the correct number of items
upon modification (which might otherwise only manifest).

BUG=616090
Review-Url: https://codereview.chromium.org/2658163003
Cr-Commit-Position: refs/heads/master@{#446998}
Committed: https://chromium.googlesource.com/chromium/src/+/48859482c4d373854a5d4736ab426fb541b99360

[Bernhard Bauer, 2017-01-27 10:23:01]

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

[commit-bot: I haz the power, 2017-01-27 10:23:54]

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, 2017-01-27 10:56:24]

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

[commit-bot: I haz the power, 2017-01-27 10:56:25]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2017-01-27 13:36:29]

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

[commit-bot: I haz the power, 2017-01-27 13:36:46]

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, 2017-01-27 13:38:28]

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

[commit-bot: I haz the power, 2017-01-27 13:38:29]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Bernhard Bauer, 2017-01-27 18:03:59]

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

[commit-bot: I haz the power, 2017-01-27 18:04:49]

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

[Bernhard Bauer, 2017-01-27 18:34:53]

Description was changed from

==========
[Android NTP] Cache number of items in inner nodes

As a side effect, this now verifies that child nodes notify about the correct
number of items on modification.

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

to

==========
[Android NTP] Cache number of items under child nodes

This makes getItemCount() more efficient for inner nodes, and for any child node
verifies that it notifies about the correct number of items on modification.

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

[Bernhard Bauer, 2017-01-27 18:37:24]

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

[commit-bot: I haz the power, 2017-01-27 18:38:05]

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, 2017-01-27 20:46:05]

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

[commit-bot: I haz the power, 2017-01-27 20:46:06]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2017-01-30 10:08:02]

Description was changed from

==========
[Android NTP] Cache number of items under child nodes

This makes getItemCount() more efficient for inner nodes, and for any child node
verifies that it notifies about the correct number of items on modification.

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

to

==========
[Android NTP] Cache number of items under child nodes

For inner nodes this makes getItemCount() more efficient, and for any child node
in general adds assertions that it notifies about the correct number of items
upon modification (which might otherwise only manifest).

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

[Bernhard Bauer, 2017-01-30 10:14:39]

bauerb@chromium.org changed reviewers:
+ peconn@chromium.org

[Bernhard Bauer, 2017-01-30 10:14:40]

Please review.

You remember your comment, "we should make this more efficient" about 4 months
ago? Finally we can :-D

[PEConn, 2017-01-30 10:42:47]

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

https://codereview.chromium.org/2658163003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ChildNode.java:80:
protected abstract int getItemCountForDebugging();
Can we get Javadoc here? Is the intention to get rid of this function once we
know the caching works?

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

https://codereview.chromium.org/2658163003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java:147:
if (addedCount > 0) notifyItemRangeInserted(initialCount, addedCount);
Can you not just use addedCount.size() (or something equivalent for varargs)?

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

https://codereview.chromium.org/2658163003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/Leaf.java:21:
notifyItemInserted(0);
This calls through to notifyItemRangeInserted(0, 1), right?

[Bernhard Bauer, 2017-01-30 15:41:59]

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

https://codereview.chromium.org/2658163003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ChildNode.java:80:
protected abstract int getItemCountForDebugging();
On 2017/01/30 10:42:47, PEConn wrote:
> Can we get Javadoc here? Is the intention to get rid of this function once we
> know the caching works?

Javadoc done.

No, I'd like to keep the function for assertions, but it should never get
exercised in production code. (And if that is the case, Proguard should remove
it.)

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

https://codereview.chromium.org/2658163003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java:147:
if (addedCount > 0) notifyItemRangeInserted(initialCount, addedCount);
On 2017/01/30 10:42:47, PEConn wrote:
> Can you not just use addedCount.size() (or something equivalent for varargs)?

I'm not sure what you mean... I want to notify about the number of items
inserted, which does not correspond to the number of added children (as a child
can represent any number of items, including 0).

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

https://codereview.chromium.org/2658163003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/Leaf.java:21:
notifyItemInserted(0);
On 2017/01/30 10:42:47, PEConn wrote:
> This calls through to notifyItemRangeInserted(0, 1), right?

Exactly, and then it will stop right before notifying the parent, because that's
still null at that point. I expanded the comment a bit to explain that.

[PEConn, 2017-01-30 15:48:20]

LGTM

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

https://codereview.chromium.org/2658163003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java:147:
if (addedCount > 0) notifyItemRangeInserted(initialCount, addedCount);
On 2017/01/30 15:41:59, Bernhard Bauer wrote:
> On 2017/01/30 10:42:47, PEConn wrote:
> > Can you not just use addedCount.size() (or something equivalent for
varargs)?
> 
> I'm not sure what you mean... I want to notify about the number of items
> inserted, which does not correspond to the number of added children (as a
child
> can represent any number of items, including 0).

Ah, sorry - I misread the code.

[Bernhard Bauer, 2017-01-30 16:02:48]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2017-01-30 16:03:18]

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, 2017-01-30 16:37:10]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485792168810250,
"parent_rev": "8a45849750a8bdffd8a47d76c52af3cdf3e834b6", "commit_rev":
"48859482c4d373854a5d4736ab426fb541b99360"}

[commit-bot: I haz the power, 2017-01-30 16:37:41]

Description was changed from

==========
[Android NTP] Cache number of items under child nodes

For inner nodes this makes getItemCount() more efficient, and for any child node
in general adds assertions that it notifies about the correct number of items
upon modification (which might otherwise only manifest).

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

to

==========
[Android NTP] Cache number of items under child nodes

For inner nodes this makes getItemCount() more efficient, and for any child node
in general adds assertions that it notifies about the correct number of items
upon modification (which might otherwise only manifest).

BUG=616090

Review-Url: https://codereview.chromium.org/2658163003
Cr-Commit-Position: refs/heads/master@{#446998}
Committed:
https://chromium.googlesource.com/chromium/src/+/48859482c4d373854a5d4736ab42...
==========

[commit-bot: I haz the power, 2017-01-30 16:37:41]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/48859482c4d373854a5d4736ab42...