📰 Cleanup ContextMenuManager and add tests

[NTP Client] Cleanup ContextMenuManager

BUG=663747
Committed: https://crrev.com/5bbe938ce72c30eda5484cc6a5b5f2b7265bf91d
Cr-Commit-Position: refs/heads/master@{#432907}

[dgn, 2016-11-16 19:34:01]

Description was changed from

==========
[NTP Client] Cleanup ContextMenuManager

BUG=
==========

to

==========
[NTP Client] Cleanup ContextMenuManager

BUG=
==========

[dgn, 2016-11-17 12:03:08]

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

[dgn, 2016-11-17 12:03:08]

PTAL

[dgn, 2016-11-17 12:03:13]

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

[commit-bot: I haz the power, 2016-11-17 12:03:24]

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-17 12:42:45]

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

[commit-bot: I haz the power, 2016-11-17 12:42:45]

Dry run: This issue passed the CQ dry run.

[dgn, 2016-11-17 13:12:25]

Description was changed from

==========
[NTP Client] Cleanup ContextMenuManager

BUG=
==========

to

==========
[NTP Client] Cleanup ContextMenuManager

BUG=663747
==========

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

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

[commit-bot: I haz the power, 2016-11-17 13:14:28]

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-11-17 13:20:11]

https://codereview.chromium.org/2507173002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java
(right):

https://codereview.chromium.org/2507173002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java:153:
private static Map<Integer, Integer> sStringMap = new TreeMap<Integer,
Integer>() {
final, and then SHOUTING_CASE.

https://codereview.chromium.org/2507173002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java
(right):

https://codereview.chromium.org/2507173002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java:387:
private void assertArrayEquals(int[] expected, int[] actual) {
You could try using hamcrest matchers for this?

[Michael van Ouwerkerk, 2016-11-17 13:29:37]

https://codereview.chromium.org/2507173002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java
(right):

https://codereview.chromium.org/2507173002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java:200:
assertArrayEquals(new int[] {KnownCategories.ARTICLES},
mSource.getCategories());
Ah yes, this is nicer :-)

https://codereview.chromium.org/2507173002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java:202:
// Scroll a suggestion into view.
Update this comment.

https://codereview.chromium.org/2507173002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java:224:
// Scroll a suggestion into view.
Update this comment.

https://codereview.chromium.org/2507173002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java:225:
int cardPosition = getAdapter().getFirstCardPosition() + 1;
Maybe call it "actionItemPosition"?

https://codereview.chromium.org/2507173002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java:273:
public void onItemRangeRemoved(int positionStart, int itemCount) {
I'm not sure this is needed. Yes it might fire, but is it needed for calling
notifyCalled()?

https://codereview.chromium.org/2507173002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java:387:
private void assertArrayEquals(int[] expected, int[] actual) {
This method could be static.

[commit-bot: I haz the power, 2016-11-17 13:48:31]

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

[commit-bot: I haz the power, 2016-11-17 13:48:32]

Dry run: This issue passed the CQ dry run.

[dgn, 2016-11-17 16:10:22]

https://codereview.chromium.org/2507173002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java
(right):

https://codereview.chromium.org/2507173002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java:153:
private static Map<Integer, Integer> sStringMap = new TreeMap<Integer,
Integer>() {
On 2016/11/17 13:20:11, Bernhard Bauer wrote:
> final, and then SHOUTING_CASE.

Done.

https://codereview.chromium.org/2507173002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java
(right):

https://codereview.chromium.org/2507173002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java:202:
// Scroll a suggestion into view.
On 2016/11/17 13:29:37, Michael van Ouwerkerk wrote:
> Update this comment.

Done.

https://codereview.chromium.org/2507173002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java:224:
// Scroll a suggestion into view.
On 2016/11/17 13:29:37, Michael van Ouwerkerk wrote:
> Update this comment.

Done.

https://codereview.chromium.org/2507173002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java:225:
int cardPosition = getAdapter().getFirstCardPosition() + 1;
On 2016/11/17 13:29:37, Michael van Ouwerkerk wrote:
> Maybe call it "actionItemPosition"?

It is also a card, but anyway, renamed :p

https://codereview.chromium.org/2507173002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java:273:
public void onItemRangeRemoved(int positionStart, int itemCount) {
On 2016/11/17 13:29:37, Michael van Ouwerkerk wrote:
> I'm not sure this is needed. Yes it might fire, but is it needed for calling
> notifyCalled()?

Because I call the method with 0 as suggestion count. We don't add any
suggestion, but this ItemRangeRemoved will be called to remove the loading
indicator. For some reason in the test, pressing the item in the context menu
hangs 

But anyway, this callback doesn't seem necessary, as the setting the suggestions
is a synchronous call. removed it.

https://codereview.chromium.org/2507173002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java:387:
private void assertArrayEquals(int[] expected, int[] actual) {
On 2016/11/17 13:20:11, Bernhard Bauer wrote:
> You could try using hamcrest matchers for this?

Instrumentation test with Junit 3, Otherwise Junit 4 itself has
assertArrayEquals.

https://codereview.chromium.org/2507173002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java:387:
private void assertArrayEquals(int[] expected, int[] actual) {
On 2016/11/17 13:29:37, Michael van Ouwerkerk wrote:
> This method could be static.

Done.

[dgn, 2016-11-17 16:11:23]

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

[commit-bot: I haz the power, 2016-11-17 16:11:54]

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-17 16:22:48]

lgtm

[Bernhard Bauer, 2016-11-17 16:23:54]

lgtm

[dgn, 2016-11-17 16:29:51]

The CQ bit was unchecked by dgn@chromium.org

[dgn, 2016-11-17 16:29:51]

The CQ bit was checked by dgn@chromium.org

[commit-bot: I haz the power, 2016-11-17 16:30:05]

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-17 17:54:12]

Description was changed from

==========
[NTP Client] Cleanup ContextMenuManager

BUG=663747
==========

to

==========
[NTP Client] Cleanup ContextMenuManager

BUG=663747
==========

[commit-bot: I haz the power, 2016-11-17 17:54:13]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-17 18:09:01]

Description was changed from

==========
[NTP Client] Cleanup ContextMenuManager

BUG=663747
==========

to

==========
[NTP Client] Cleanup ContextMenuManager

BUG=663747

Committed: https://crrev.com/5bbe938ce72c30eda5484cc6a5b5f2b7265bf91d
Cr-Commit-Position: refs/heads/master@{#432907}
==========

[commit-bot: I haz the power, 2016-11-17 18:09:02]

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