📰 Split TreeNode#init into SetParent and SetChildren

[NTP Client] Split TreeNode#init into SetParent and SetChildren

Unties Node construction with the initialisation of their parent and
children. This allows controlling when the notifications about child
modifications are propagated, and simplifies the initialisation order.

The InnerNode#setParent() call is now what enables the notifications
and is done as the last step of the NewTabPageAdapter constructor.

ChildNode and InnerNode share almost all of the logic related to
managing the tree and their implementations make use of that without
specific modifications.

This also fixes some bugs related to resetting the section list.

BUG=616090, 674023
Committed: https://crrev.com/d3007605eb84788e2d02efc395509ad449f3c035
Cr-Commit-Position: refs/heads/master@{#439489}

[dgn, 2016-12-15 12:02:56]

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

[commit-bot: I haz the power, 2016-12-15 12:03:03]

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-12-15 12:33:52]

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

[commit-bot: I haz the power, 2016-12-15 12:33:53]

Dry run: This issue passed the CQ dry run.

[dgn, 2016-12-15 13:08:56]

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

[dgn, 2016-12-15 13:08:57]

PTAL

https://codereview.chromium.org/2573173002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
(left):

https://codereview.chromium.org/2573173002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:151:
updateAllDismissedVisibility();
is called as indirectly by init, when the children notify about their addition.
So we can now skip this.

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

https://codereview.chromium.org/2573173002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:154:
//        mChildren = Arrays.asList(
For some reason this is a compilation error (List<ChildNode> incompatible with
List<TreeNode>)while the initialisation of mChildren above works. No idea why.

https://codereview.chromium.org/2573173002/diff/20001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java
(left):

https://codereview.chromium.org/2573173002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java:44:
TreeNode child = mock(TreeNode.class);
swapped this mock for a spy so that ChildNode#init() is exercised. Another
option could be to put back the mock as before and create a new tree with real
objects in #initTest instead

the "when..thenReturn" => "doReturn..when" changes are required by the spy
objects

[dgn, 2016-12-15 14:30:21]

Description was changed from

==========
[NTP Client] Make TreeNode#init responsible for notifying of insertions

The #init(NodeParent) method must now be called before events can start
being sent to the parent node. This allows the children to properly
update their state and minimise the number of updates that end up being
sent when an new node is inserted.

Also fixes some bugs related to resetting the list

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

to

==========
[NTP Client] Make TreeNode#init responsible for notifying of insertions

The #init(NodeParent) method must now be called before events can start
being sent to the parent node. This allows the children to properly
update their state and minimise the number of updates that end up being
sent when an new node is inserted.

Also fixes some bugs related to resetting the list

BUG=616090,674035
==========

[Bernhard Bauer, 2016-12-15 15:40:19]

Nice!

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

https://codereview.chromium.org/2573173002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ChildNode.java:31:
if (itemCount > 0) notifyItemRangeInserted(0, itemCount);
Do we even need this? In all cases except for the root, the notifications would
get swallowed somewhere anyway, because of an ancestor being null. If we move
this to InnerNode.addChild(), it's symmetric with removeChild(), and we don't
get any notifications at construction time (where we don't need them anyway).

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

https://codereview.chromium.org/2573173002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java:16:
protected abstract List<TreeNode> getChildren();
Y'know, I'm thinking... now that we initialize children before initializing the
node itself (including setting the parent), we could replace this with a setter
method that a subclass has to call and that will initialize the children.

Then we could move anything apart from setting the parent out of the current
init() method implementations (because it doesn't actually require the parent),
e.g. into the constructor, so init() would essentially become a straightforward
setParent() setter.

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

https://codereview.chromium.org/2573173002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:154:
//        mChildren = Arrays.asList(
On 2016/12/15 13:08:57, dgn wrote:
> For some reason this is a compilation error (List<ChildNode> incompatible with
> List<TreeNode>)while the initialisation of mChildren above works. No idea why.

As counterintuitive as it might sound at first, a List<ChildNode> is _not_ a
List<TreeNode>, because you can insert any TreeNode into the latter, but only a
ChildNode into the former, so it doesn't fulfill the contract.

(And conversely, a List<TreeNode> is not a List<ChildNode> either, because the
latter _contains_ only ChildNodes, while the former could contain any TreeNode.
In the previous paragraph, we had a stronger precondition, and here we have a
stronger postcondition. If you are willing to have the stricter predicate only
for one of those, you can use co- or contravariance, like this: List<? extends
TreeNode> or List<? super ChildNode>.)

In this case however, it's just the compiler inferring out the wrong type
parameter, so helping it along with Arrays.<TreeNode>asList(...) should work.

Alternatively, if we replace init() with setParent() (see my comment above), it
might make sense to leave it out of the TreeNode interface, and have the
children of an InnerNode be a List<ChildNode>.

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

https://codereview.chromium.org/2573173002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SpacingItem.java:47:
NodeParent parent = getParent();
Wait, if SpacingItem is a Leaf and Leaf now derives from ChildNode, could we
just call notifyItemChanged(0)?

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

https://codereview.chromium.org/2573173002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java:146:
public void removeChildren() {
Can this be protected as well?

[dgn, 2016-12-16 12:19:16]

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

[commit-bot: I haz the power, 2016-12-16 12:19:34]

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

[dgn, 2016-12-16 12:32:22]

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

[commit-bot: I haz the power, 2016-12-16 12:32:35]

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

[dgn, 2016-12-16 13:00:36]

Description was changed from

==========
[NTP Client] Make TreeNode#init responsible for notifying of insertions

The #init(NodeParent) method must now be called before events can start
being sent to the parent node. This allows the children to properly
update their state and minimise the number of updates that end up being
sent when an new node is inserted.

Also fixes some bugs related to resetting the list

BUG=616090,674035
==========

to

==========
[NTP Client] Split TreeNode#init into SetParent and SetChildren

Unties Node construction with the initialisation of their parent and
children. This allows controlling when the notifications about child
modifications are propagated, and simplifies the initialisation order.
    
The InnerNode#setParent() call is now what enables the notifications
and is done as the last step of the NewTabPageAdapter constructor.
    
ChildNode and InnerNode share almost all of the logic related to
managing the tree and their implementations make use of that without
specific modifications.
    
This also fixes some bugs related to resetting the section list.

BUG=616090,674035
==========

[dgn, 2016-12-16 13:05:14]

PTAL

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

https://codereview.chromium.org/2573173002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ChildNode.java:31:
if (itemCount > 0) notifyItemRangeInserted(0, itemCount);
On 2016/12/15 15:40:18, Bernhard Bauer wrote:
> Do we even need this? In all cases except for the root, the notifications
would
> get swallowed somewhere anyway, because of an ancestor being null. If we move
> this to InnerNode.addChild(), it's symmetric with removeChild(), and we don't
> get any notifications at construction time (where we don't need them anyway).

Obsolete

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

https://codereview.chromium.org/2573173002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java:16:
protected abstract List<TreeNode> getChildren();
On 2016/12/15 15:40:18, Bernhard Bauer wrote:
> Y'know, I'm thinking... now that we initialize children before initializing
the
> node itself (including setting the parent), we could replace this with a
setter
> method that a subclass has to call and that will initialize the children.
> 
> Then we could move anything apart from setting the parent out of the current
> init() method implementations (because it doesn't actually require the
parent),
> e.g. into the constructor, so init() would essentially become a
straightforward
> setParent() setter.

Done.

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

https://codereview.chromium.org/2573173002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:154:
//        mChildren = Arrays.asList(
On 2016/12/15 15:40:18, Bernhard Bauer wrote:
> On 2016/12/15 13:08:57, dgn wrote:
> > For some reason this is a compilation error (List<ChildNode> incompatible
with
> > List<TreeNode>)while the initialisation of mChildren above works. No idea
why.
> 
> As counterintuitive as it might sound at first, a List<ChildNode> is _not_ a
> List<TreeNode>, because you can insert any TreeNode into the latter, but only
a
> ChildNode into the former, so it doesn't fulfill the contract.
> 
> (And conversely, a List<TreeNode> is not a List<ChildNode> either, because the
> latter _contains_ only ChildNodes, while the former could contain any
TreeNode.
> In the previous paragraph, we had a stronger precondition, and here we have a
> stronger postcondition. If you are willing to have the stricter predicate only
> for one of those, you can use co- or contravariance, like this: List<? extends
> TreeNode> or List<? super ChildNode>.)
> 
> In this case however, it's just the compiler inferring out the wrong type
> parameter, so helping it along with Arrays.<TreeNode>asList(...) should work.
> 
> Alternatively, if we replace init() with setParent() (see my comment above),
it
> might make sense to leave it out of the TreeNode interface, and have the
> children of an InnerNode be a List<ChildNode>.

Ah yes, I now remember running into the same thing a few months ago! I'll just
blame the compiler for not taking the hint from mChildren declaration

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

https://codereview.chromium.org/2573173002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SpacingItem.java:47:
NodeParent parent = getParent();
On 2016/12/15 15:40:19, Bernhard Bauer wrote:
> Wait, if SpacingItem is a Leaf and Leaf now derives from ChildNode, could we
> just call notifyItemChanged(0)?

Done.

https://codereview.chromium.org/2573173002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SpacingItem.java:47:
NodeParent parent = getParent();
On 2016/12/15 15:40:19, Bernhard Bauer wrote:
> Wait, if SpacingItem is a Leaf and Leaf now derives from ChildNode, could we
> just call notifyItemChanged(0)?

Duh, yes, I added that because I noticed notifyItemChanged was unused. Thanks!

https://codereview.chromium.org/2573173002/diff/20001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java
(left):

https://codereview.chromium.org/2573173002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java:44:
TreeNode child = mock(TreeNode.class);
On 2016/12/15 13:08:57, dgn wrote:
> swapped this mock for a spy so that ChildNode#init() is exercised. Another
> option could be to put back the mock as before and create a new tree with real
> objects in #initTest instead
> 
> the "when..thenReturn" => "doReturn..when" changes are required by the spy
> objects

Obsolete.

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

https://codereview.chromium.org/2573173002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java:146:
public void removeChildren() {
On 2016/12/15 15:40:19, Bernhard Bauer wrote:
> Can this be protected as well?

Done.

https://codereview.chromium.org/2573173002/diff/80001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java
(right):

https://codereview.chromium.org/2573173002/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java:189:
public void testChangeNotificationsTiming2() {
testChangeNotificationsTiming2 and ItemCountCheck added to look at an
alternative way to implement the test. WDYT? I suppose you prefer the first one
though :p

[dgn, 2016-12-16 13:05:34]

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

[commit-bot: I haz the power, 2016-12-16 13:05:47]

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

[dgn, 2016-12-16 13:46:40]

Description was changed from

==========
[NTP Client] Split TreeNode#init into SetParent and SetChildren

Unties Node construction with the initialisation of their parent and
children. This allows controlling when the notifications about child
modifications are propagated, and simplifies the initialisation order.
    
The InnerNode#setParent() call is now what enables the notifications
and is done as the last step of the NewTabPageAdapter constructor.
    
ChildNode and InnerNode share almost all of the logic related to
managing the tree and their implementations make use of that without
specific modifications.
    
This also fixes some bugs related to resetting the section list.

BUG=616090,674035
==========

to

==========
[NTP Client] Split TreeNode#init into SetParent and SetChildren

Unties Node construction with the initialisation of their parent and
children. This allows controlling when the notifications about child
modifications are propagated, and simplifies the initialisation order.
    
The InnerNode#setParent() call is now what enables the notifications
and is done as the last step of the NewTabPageAdapter constructor.
    
ChildNode and InnerNode share almost all of the logic related to
managing the tree and their implementations make use of that without
specific modifications.
    
This also fixes some bugs related to resetting the section list.

BUG=616090,674023
==========

[commit-bot: I haz the power, 2016-12-16 13:57:41]

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

[commit-bot: I haz the power, 2016-12-16 13:57:42]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-12-16 17:27:28]

Nice!

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

https://codereview.chromium.org/2573173002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ChildNode.java:24:
if (mParent == parent) return;
I don't think this is necessary anymore.

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

https://codereview.chromium.org/2573173002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java:20:
public void setChildren(Collection<TreeNode> children) {
Huh… I wonder whether we need this at all, or whether we could just add all the
children one by one. Or even just do TreeNode... to get an array.

https://codereview.chromium.org/2573173002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java:115:
* Helper method that adds and initialises a new child node.
…and notifies about the insertion.

https://codereview.chromium.org/2573173002/diff/100001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java
(right):

https://codereview.chromium.org/2573173002/diff/100001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java:173:
verify(parent).onItemRangeInserted(any(TreeNode.class), eq(8), eq(4));
Verify the exact child that is passed?

https://codereview.chromium.org/2573173002/diff/100001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java:176:
rootNode.removeChild(rootNode.getChildren().get(1));
Do you need getChildren()? You could just access |mChildren|.

https://codereview.chromium.org/2573173002/diff/100001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java:191:
public void expect(int count) {
Nit: expectItemCount()? Also, add a comment, in particular about what -1 means?
(Alternatively, use a @Nullable Integer object to store it).

[dgn, 2016-12-16 19:08:43]

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

[dgn, 2016-12-16 19:08:48]

PTAL

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

https://codereview.chromium.org/2573173002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ChildNode.java:24:
if (mParent == parent) return;
On 2016/12/16 17:27:27, Bernhard Bauer wrote:
> I don't think this is necessary anymore.

Done.

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

https://codereview.chromium.org/2573173002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java:20:
public void setChildren(Collection<TreeNode> children) {
On 2016/12/16 17:27:27, Bernhard Bauer wrote:
> Huh… I wonder whether we need this at all, or whether we could just add all
the
> children one by one. Or even just do TreeNode... to get an array.

Done.

https://codereview.chromium.org/2573173002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java:115:
* Helper method that adds and initialises a new child node.
On 2016/12/16 17:27:27, Bernhard Bauer wrote:
> …and notifies about the insertion.

Done.

https://codereview.chromium.org/2573173002/diff/100001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java
(right):

https://codereview.chromium.org/2573173002/diff/100001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java:173:
verify(parent).onItemRangeInserted(any(TreeNode.class), eq(8), eq(4));
On 2016/12/16 17:27:27, Bernhard Bauer wrote:
> Verify the exact child that is passed?

Done.

https://codereview.chromium.org/2573173002/diff/100001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java:176:
rootNode.removeChild(rootNode.getChildren().get(1));
On 2016/12/16 17:27:27, Bernhard Bauer wrote:
> Do you need getChildren()? You could just access |mChildren|.

I'm trying to restrict the usage of it outside of the class.

https://codereview.chromium.org/2573173002/diff/100001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java:191:
public void expect(int count) {
On 2016/12/16 17:27:27, Bernhard Bauer wrote:
> Nit: expectItemCount()? Also, add a comment, in particular about what -1
means?
> (Alternatively, use a @Nullable Integer object to store it).

Done.

[commit-bot: I haz the power, 2016-12-16 19:09:42]

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

[Bernhard Bauer, 2016-12-16 19:34:17]

https://codereview.chromium.org/2573173002/diff/100001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java
(right):

https://codereview.chromium.org/2573173002/diff/100001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java:176:
rootNode.removeChild(rootNode.getChildren().get(1));
On 2016/12/16 19:08:48, dgn wrote:
> On 2016/12/16 17:27:27, Bernhard Bauer wrote:
> > Do you need getChildren()? You could just access |mChildren|.
> 
> I'm trying to restrict the usage of it outside of the class.

Sorry, I meant |mChildren| in the test fixture (didn't notice that this doesn't
use the node from setUp()). You still can keep a handle to the node you inserted
though.

[dgn, 2016-12-16 19:58:58]

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

[dgn, 2016-12-16 20:00:06]

PTAL. I replaced the setChildren by addChildren() in the end, since we have a
removeChildren() method already. For completeness sake it would have been better
to have removedChildren(TreeNode... children) but we don't need it so far.

https://codereview.chromium.org/2573173002/diff/100001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java
(right):

https://codereview.chromium.org/2573173002/diff/100001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java:176:
rootNode.removeChild(rootNode.getChildren().get(1));
On 2016/12/16 19:34:17, Bernhard Bauer wrote:
> On 2016/12/16 19:08:48, dgn wrote:
> > On 2016/12/16 17:27:27, Bernhard Bauer wrote:
> > > Do you need getChildren()? You could just access |mChildren|.
> > 
> > I'm trying to restrict the usage of it outside of the class.
> 
> Sorry, I meant |mChildren| in the test fixture (didn't notice that this
doesn't
> use the node from setUp()). You still can keep a handle to the node you
inserted
> though.

The nodes in mChildren are already used in mInnerNode and I wanted to have the
initialisation from scratch tested here. Added a new array though, so I removed
the getChildren usage.

[commit-bot: I haz the power, 2016-12-16 20:00:33]

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-12-16 22:26:18]

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

[commit-bot: I haz the power, 2016-12-16 22:26:19]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-12-19 10:28:46]

lgtm

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

https://codereview.chromium.org/2573173002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java:163:
@VisibleForTesting
Can we remove this now?

https://codereview.chromium.org/2573173002/diff/140001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java
(right):

https://codereview.chromium.org/2573173002/diff/140001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java:196:
private Integer mNextExpectedItemCount;
Nit: @Nullable? :)

[dgn, 2016-12-19 13:41:08]

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

[commit-bot: I haz the power, 2016-12-19 13:41:22]

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

[dgn, 2016-12-19 13:44:21]

PTAL

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

https://codereview.chromium.org/2573173002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java:163:
@VisibleForTesting
On 2016/12/19 10:28:46, Bernhard Bauer wrote:
> Can we remove this now?

I had to change a test in NTPAdapterTest, but done.

https://codereview.chromium.org/2573173002/diff/140001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java
(right):

https://codereview.chromium.org/2573173002/diff/140001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java:196:
private Integer mNextExpectedItemCount;
On 2016/12/19 10:28:46, Bernhard Bauer wrote:
> Nit: @Nullable? :)

Done.

https://codereview.chromium.org/2573173002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java
(right):

https://codereview.chromium.org/2573173002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:252:
return new ArrayList<>(mSections.values()).get(sectionIndex);
Went with that instead of something like:


int i = 0;
for (SuggestionsSection section : mSections.values()) {
    if (i == sectionIndex) return section;
    ++i;
}
throw new IndexOutOfBoundsException();

because it's only for testing anyway and I like one liners. Can revert if there
is some advantage to the other I missed.

[commit-bot: I haz the power, 2016-12-19 14:19:01]

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

[commit-bot: I haz the power, 2016-12-19 14:19:01]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-12-19 14:35:35]

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

https://codereview.chromium.org/2573173002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java:163:
@VisibleForTesting
On 2016/12/19 13:44:21, dgn wrote:
> On 2016/12/19 10:28:46, Bernhard Bauer wrote:
> > Can we remove this now?
> 
> I had to change a test in NTPAdapterTest, but done.

Now that I see this, I have to admit I liked the old version better (I forgot
that testCategoryOrder used this)... Would you mind going back to the old
version? Sorry about the churn.

(That test also seem suitable to move to a SectionListTest class instead of
being a part of the NewTabPageAdapterTest

https://codereview.chromium.org/2573173002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java
(right):

https://codereview.chromium.org/2573173002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:32:
private final Map<Integer, SuggestionsSection> mSections = new
LinkedHashMap<>();
Huh. I just realized that the test tests whether this maps returns its values in
order, whereas the contract of the class is that the children are in the defined
order, and in addition the former isn't even necessary to fulfill the contract,
because the children are in a separate object! So, I think we should just make
this a regular HashMap, to avoid giving a false impression about where the
section order comes from.

[dgn, 2016-12-19 15:31:41]

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

[commit-bot: I haz the power, 2016-12-19 15:31:53]

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

[dgn, 2016-12-19 15:33:34]

PTAL

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

https://codereview.chromium.org/2573173002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/InnerNode.java:163:
@VisibleForTesting
On 2016/12/19 14:35:35, Bernhard Bauer wrote:
> On 2016/12/19 13:44:21, dgn wrote:
> > On 2016/12/19 10:28:46, Bernhard Bauer wrote:
> > > Can we remove this now?
> > 
> > I had to change a test in NTPAdapterTest, but done.
> 
> Now that I see this, I have to admit I liked the old version better (I forgot
> that testCategoryOrder used this)... Would you mind going back to the old
> version? Sorry about the churn.
> 
> (That test also seem suitable to move to a SectionListTest class instead of
> being a part of the NewTabPageAdapterTest

Done. And there is no more SectionListTest so I left it there for now.

https://codereview.chromium.org/2573173002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java
(right):

https://codereview.chromium.org/2573173002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java:32:
private final Map<Integer, SuggestionsSection> mSections = new
LinkedHashMap<>();
On 2016/12/19 14:35:35, Bernhard Bauer wrote:
> Huh. I just realized that the test tests whether this maps returns its values
in
> order, whereas the contract of the class is that the children are in the
defined
> order, and in addition the former isn't even necessary to fulfill the
contract,
> because the children are in a separate object! So, I think we should just make
> this a regular HashMap, to avoid giving a false impression about where the
> section order comes from.

It is currently used for UMA things, but that code has issues anyway. Filed
https://crbug.com/675623

[commit-bot: I haz the power, 2016-12-19 16:03:11]

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

[commit-bot: I haz the power, 2016-12-19 16:03:12]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-12-19 16:23:38]

Still LGTM!

[dgn, 2016-12-19 16:30:57]

The CQ bit was checked by dgn@chromium.org

[commit-bot: I haz the power, 2016-12-19 16:31:14]

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-12-19 16:36:05]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1482165057753720,
"parent_rev": "ff46458e0b08c023b72283ce7827792993f2753b", "commit_rev":
"58f8458aa34085da0f51ae9077d1d308839b1ca6"}

[commit-bot: I haz the power, 2016-12-19 16:36:35]

Description was changed from

==========
[NTP Client] Split TreeNode#init into SetParent and SetChildren

Unties Node construction with the initialisation of their parent and
children. This allows controlling when the notifications about child
modifications are propagated, and simplifies the initialisation order.
    
The InnerNode#setParent() call is now what enables the notifications
and is done as the last step of the NewTabPageAdapter constructor.
    
ChildNode and InnerNode share almost all of the logic related to
managing the tree and their implementations make use of that without
specific modifications.
    
This also fixes some bugs related to resetting the section list.

BUG=616090,674023
==========

to

==========
[NTP Client] Split TreeNode#init into SetParent and SetChildren

Unties Node construction with the initialisation of their parent and
children. This allows controlling when the notifications about child
modifications are propagated, and simplifies the initialisation order.
    
The InnerNode#setParent() call is now what enables the notifications
and is done as the last step of the NewTabPageAdapter constructor.
    
ChildNode and InnerNode share almost all of the logic related to
managing the tree and their implementations make use of that without
specific modifications.
    
This also fixes some bugs related to resetting the section list.

BUG=616090,674023

Review-Url: https://codereview.chromium.org/2573173002
==========

[commit-bot: I haz the power, 2016-12-19 16:36:36]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-12-19 16:38:21]

Description was changed from

==========
[NTP Client] Split TreeNode#init into SetParent and SetChildren

Unties Node construction with the initialisation of their parent and
children. This allows controlling when the notifications about child
modifications are propagated, and simplifies the initialisation order.
    
The InnerNode#setParent() call is now what enables the notifications
and is done as the last step of the NewTabPageAdapter constructor.
    
ChildNode and InnerNode share almost all of the logic related to
managing the tree and their implementations make use of that without
specific modifications.
    
This also fixes some bugs related to resetting the section list.

BUG=616090,674023

Review-Url: https://codereview.chromium.org/2573173002
==========

to

==========
[NTP Client] Split TreeNode#init into SetParent and SetChildren

Unties Node construction with the initialisation of their parent and
children. This allows controlling when the notifications about child
modifications are propagated, and simplifies the initialisation order.

The InnerNode#setParent() call is now what enables the notifications
and is done as the last step of the NewTabPageAdapter constructor.

ChildNode and InnerNode share almost all of the logic related to
managing the tree and their implementations make use of that without
specific modifications.

This also fixes some bugs related to resetting the section list.

BUG=616090,674023

Committed: https://crrev.com/d3007605eb84788e2d02efc395509ad449f3c035
Cr-Commit-Position: refs/heads/master@{#439489}
==========

[commit-bot: I haz the power, 2016-12-19 16:38:22]

Patchset 11 (id:??) landed as
https://crrev.com/d3007605eb84788e2d02efc395509ad449f3c035
Cr-Commit-Position: refs/heads/master@{#439489}