📰 Make status cards swipable

[NTP Client] Make status cards swipable

When the status card is swiped away, the entire section is dismissed and
stays that way on NTPs opened afterwards. If the status of the section
changes (new snippets loaded, user logged in, etc.) the section will be
added back.

Preview: https://goo.gl/photos/ooDFBuAVCLauo4bbA

BUG=638580
Committed: https://crrev.com/212feea3b3df08ddb7d25d8b6293344ebc5d5b9b
Cr-Commit-Position: refs/heads/master@{#419170}

[dgn, 2016-09-15 16:01:56]

Description was changed from

==========
[NTP Client] Make status cards swipable

When the status card is swiped away, the entire section is dismissed and
stays that way on NTPs opened afterwards. If the status of the section
changes (new snippets loaded, user logged in, etc.) the section will be
added back.

BUG=638580
==========

to

==========
[NTP Client] Make status cards swipable

When the status card is swiped away, the entire section is dismissed and
stays that way on NTPs opened afterwards. If the status of the section
changes (new snippets loaded, user logged in, etc.) the section will be
added back.

BUG=638580
==========

[dgn, 2016-09-15 16:04:26]

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

[dgn, 2016-09-15 16:04:27]

PTAL. WIP, but I'd like to validate the backend changes. The desired behaviour
is implemented.

I still need to update the UI so that for bookmarks the more button and the
status card move together when either is swiped.

[dgn, 2016-09-15 21:43:47]

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

[commit-bot: I haz the power, 2016-09-15 21:44:39]

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

[dgn, 2016-09-15 21:47:46]

Description was changed from

==========
[NTP Client] Make status cards swipable

When the status card is swiped away, the entire section is dismissed and
stays that way on NTPs opened afterwards. If the status of the section
changes (new snippets loaded, user logged in, etc.) the section will be
added back.

BUG=638580
==========

to

==========
[NTP Client] Make status cards swipable

When the status card is swiped away, the entire section is dismissed and
stays that way on NTPs opened afterwards. If the status of the section
changes (new snippets loaded, user logged in, etc.) the section will be
added back.

Preview: https://goo.gl/photos/ooDFBuAVCLauo4bbA

BUG=638580
==========

[dgn, 2016-09-15 21:48:05]

All is there now. PTAL.

[commit-bot: I haz the power, 2016-09-15 23:00:42]

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

[commit-bot: I haz the power, 2016-09-15 23:00:43]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-09-16 09:56:44]

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

https://codereview.chromium.org/2340333002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionItem.java:27:
/** Whether this item can be dismissed.*/
Maybe move this to setDismissable(), as that needs a Javadoc comment anyway?

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

https://codereview.chromium.org/2340333002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:120:
if (siblingPosDelta != 0) {
Maybe return if the delta is 0.

https://codereview.chromium.org/2340333002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:367:
// TODO(bauerb): Notify about a smaller range: https://crbug.com/627512
Thanks! :)

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

https://codereview.chromium.org/2340333002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:133:
if (item == null) return 0;
When would this happen?

https://codereview.chromium.org/2340333002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:136:
// multiple items at the same time.
I don't quite see how this comment explains the behavior -- the line below
returns no sibling if we do have suggestions.

[Marc Treib, 2016-09-16 10:07:50]

Code mostly LG, but I have some general concerns:

- Dismissing a section has somewhat different semantics from dismissing an
individual suggestion. That seems like it might be confusing for users?

- I'm not completely convinced that "when the status changes" is exactly the
right trigger to make the section reappear - just seems a bit arbitrary. E.g.
each Chrome restart will make it reappear, which on Android roughly translates
to "whenever".

https://codereview.chromium.org/2340333002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java
(right):

https://codereview.chromium.org/2340333002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java:94:
public static final int SNIPPETS_ACTION_DISMISSED_SECTION = 7;
This histogram is deprecated, and I'll probably remove it soon (along with a
bunch of other UMA metrics from before the multi-section world),
crbug.com/639230.

If we care about dismissal metrics, we should add them in
content_suggestions_metrics.h/cc. (Doesn't have to be in this CL; you can just
file a bug and assign it to me.)

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

https://codereview.chromium.org/2340333002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:394:
if (group instanceof SuggestionsSection) dismissSection((SuggestionsSection)
group);
Can it ever *not* be an instance of SuggestionsSection?

[dgn, 2016-09-16 11:20:32]

Moved the discussion to the bug (https://crbug.com/638580), so that Patrick can
contribute.

[dgn, 2016-09-16 11:28:00]

Thanks! PTAL

https://codereview.chromium.org/2340333002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java
(right):

https://codereview.chromium.org/2340333002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java:94:
public static final int SNIPPETS_ACTION_DISMISSED_SECTION = 7;
On 2016/09/16 10:07:50, Marc Treib wrote:
> This histogram is deprecated, and I'll probably remove it soon (along with a
> bunch of other UMA metrics from before the multi-section world),
> crbug.com/639230.
> 
> If we care about dismissal metrics, we should add them in
> content_suggestions_metrics.h/cc. (Doesn't have to be in this CL; you can just
> file a bug and assign it to me.)

All right, I'll remove the UMA code here and add a note on that issue. Thanks!

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

https://codereview.chromium.org/2340333002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionItem.java:27:
/** Whether this item can be dismissed.*/
On 2016/09/16 09:56:43, Bernhard Bauer wrote:
> Maybe move this to setDismissable(), as that needs a Javadoc comment anyway?

Done.

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

https://codereview.chromium.org/2340333002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:120:
if (siblingPosDelta != 0) {
On 2016/09/16 09:56:43, Bernhard Bauer wrote:
> Maybe return if the delta is 0.

Done.

https://codereview.chromium.org/2340333002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:394:
if (group instanceof SuggestionsSection) dismissSection((SuggestionsSection)
group);
On 2016/09/16 10:07:50, Marc Treib wrote:
> Can it ever *not* be an instance of SuggestionsSection?

The above the fold, spacers, etc are not. I think the linter will complain if I
do unchecked casts... replaced with an assert.

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

https://codereview.chromium.org/2340333002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:133:
if (item == null) return 0;
On 2016/09/16 09:56:43, Bernhard Bauer wrote:
> When would this happen?

Can't happen in the current state of the code. I updated it anyway. I also do
another unnecessary check in this function. The more and status card are not
dismissable if there are no suggestions anyway. Not sure if I should skip the
check. But the function on its own is correct and should not let bugs through as
it is...

https://codereview.chromium.org/2340333002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:136:
// multiple items at the same time.
On 2016/09/16 09:56:43, Bernhard Bauer wrote:
> I don't quite see how this comment explains the behavior -- the line below
> returns no sibling if we do have suggestions.

Oops, fixed the comment.

[Marc Treib, 2016-09-16 11:36:44]

Thanks! LGTM if you want to land this now and figure out the exact re-appearing
logic later.

https://codereview.chromium.org/2340333002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java
(right):

https://codereview.chromium.org/2340333002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java:94:
public static final int SNIPPETS_ACTION_DISMISSED_SECTION = 7;
On 2016/09/16 11:28:00, dgn wrote:
> On 2016/09/16 10:07:50, Marc Treib wrote:
> > This histogram is deprecated, and I'll probably remove it soon (along with a
> > bunch of other UMA metrics from before the multi-section world),
> > crbug.com/639230.
> > 
> > If we care about dismissal metrics, we should add them in
> > content_suggestions_metrics.h/cc. (Doesn't have to be in this CL; you can
just
> > file a bug and assign it to me.)
> 
> All right, I'll remove the UMA code here and add a note on that issue. Thanks!

Thanks, and sorry for the confusion! I should at least have added comments to
the old UMA code, but that got lost in the pre-BP hectic :(

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

https://codereview.chromium.org/2340333002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionItem.java:102:
/** Whether this item can be dismissed.*/
nit: *Set* whether... ?

[Marc Treib, 2016-09-16 11:37:56]

Ah, one more thing: We should have tests for the section
dismissing/re-appearing, but probably not worth writing any now while we're not
sure what the exact logic should be.

[Bernhard Bauer, 2016-09-16 12:59:40]

LGTM with Marc's nit addressed.

[dgn, 2016-09-16 14:09:33]

The CQ bit was checked by dgn@chromium.org

[dgn, 2016-09-16 14:09:34]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2340333002/#ps80001
(title: "fix nit")

[commit-bot: I haz the power, 2016-09-16 14:09:57]

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

[dgn, 2016-09-16 14:10:34]

Thanks! I'll add tests in the follow up CL with the logic rework, yes.

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

https://codereview.chromium.org/2340333002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionItem.java:102:
/** Whether this item can be dismissed.*/
On 2016/09/16 11:36:44, Marc Treib wrote:
> nit: *Set* whether... ?

Done.

[commit-bot: I haz the power, 2016-09-16 15:09:06]

Description was changed from

==========
[NTP Client] Make status cards swipable

When the status card is swiped away, the entire section is dismissed and
stays that way on NTPs opened afterwards. If the status of the section
changes (new snippets loaded, user logged in, etc.) the section will be
added back.

Preview: https://goo.gl/photos/ooDFBuAVCLauo4bbA

BUG=638580
==========

to

==========
[NTP Client] Make status cards swipable

When the status card is swiped away, the entire section is dismissed and
stays that way on NTPs opened afterwards. If the status of the section
changes (new snippets loaded, user logged in, etc.) the section will be
added back.

Preview: https://goo.gl/photos/ooDFBuAVCLauo4bbA

BUG=638580
==========

[commit-bot: I haz the power, 2016-09-16 15:09:08]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-16 15:11:07]

Description was changed from

==========
[NTP Client] Make status cards swipable

When the status card is swiped away, the entire section is dismissed and
stays that way on NTPs opened afterwards. If the status of the section
changes (new snippets loaded, user logged in, etc.) the section will be
added back.

Preview: https://goo.gl/photos/ooDFBuAVCLauo4bbA

BUG=638580
==========

to

==========
[NTP Client] Make status cards swipable

When the status card is swiped away, the entire section is dismissed and
stays that way on NTPs opened afterwards. If the status of the section
changes (new snippets loaded, user logged in, etc.) the section will be
added back.

Preview: https://goo.gl/photos/ooDFBuAVCLauo4bbA

BUG=638580

Committed: https://crrev.com/212feea3b3df08ddb7d25d8b6293344ebc5d5b9b
Cr-Commit-Position: refs/heads/master@{#419170}
==========

[commit-bot: I haz the power, 2016-09-16 15:11:08]

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