📰 Show the Sign In promo as a separate card from the section

[NTP Client] Show the Sign In promo as a separate card from the section

BUG=649670
Committed: https://crrev.com/fa61ae72913bae84fa7e5fdc0ca6fb4092c858b0
Cr-Commit-Position: refs/heads/master@{#422464}

[dgn, 2016-09-30 11:28:46]

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

[dgn, 2016-09-30 11:28:47]

PTAL

[dgn, 2016-09-30 11:35:32]

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

https://codereview.chromium.org/2378323002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:120:
ViewHolder siblingViewHolder = getDismissSibling(viewHolder);
Added check that the view holder is in a suggestion section and moved it out.

https://codereview.chromium.org/2378323002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:196:
suggestionsPerCategory[i++] = resetSection(category, categoryStatus);
Split out to be able to reset only the section that got the SIGNED_OUT status

https://codereview.chromium.org/2378323002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:447:
if (mGroups.isEmpty()) return; // The sections have not been initialised yet.
SuggestionsSection#setSuggestions calls this before updateGroups() has run.

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

https://codereview.chromium.org/2378323002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusCardViewHolder.java:17:
public class StatusCardViewHolder extends CardViewHolder {
moved out of StatusItem

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

https://codereview.chromium.org/2378323002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusItem.java:111:
private static class SignedOut extends StatusItem {
Removed, since we don't show a signed out status card anymore, the signin promo
replaces it.

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

https://codereview.chromium.org/2378323002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusItem.java:67:
public static StatusItem create(
I'll have a try at refactoring this in the next CL about saving the state of
Dismissed Status Cards

https://codereview.chromium.org/2378323002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/ntp_snippets_service.cc (left):

https://codereview.chromium.org/2378323002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/ntp_snippets_service.cc:789: if (content.status
!= CategoryStatus::CATEGORY_EXPLICITLY_DISABLED) {
That removed entirely the section, and made the NTP ignore further updates about
it.

[Bernhard Bauer, 2016-09-30 14:31:50]

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

https://codereview.chromium.org/2378323002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:156: /**
Field backing the NewTabPageManager. Observer tracked for de-registration
purposes. */
What does the first sentence mean?

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

https://codereview.chromium.org/2378323002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:163:
@Override
Indentation?

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

https://codereview.chromium.org/2378323002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SigninPromoItem.java:26:
public class SigninPromoItem extends StatusItem implements ItemGroup,
NewTabPageItem {
Doesn't StatusItem already implement NewTagPageItem?

[dgn, 2016-09-30 15:16:59]

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

https://codereview.chromium.org/2378323002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:156: /**
Field backing the NewTabPageManager. Observer tracked for de-registration
purposes. */
On 2016/09/30 14:31:50, Bernhard Bauer wrote:
> What does the first sentence mean?

I added that because that's kind of leaking out of NTPManager implementation. If
it was not an anonymous class I would have had that inside of it.

[Bernhard Bauer, 2016-09-30 15:22:02]

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

https://codereview.chromium.org/2378323002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:156: /**
Field backing the NewTabPageManager. Observer tracked for de-registration
purposes. */
On 2016/09/30 15:16:58, dgn wrote:
> On 2016/09/30 14:31:50, Bernhard Bauer wrote:
> > What does the first sentence mean?
> 
> I added that because that's kind of leaking out of NTPManager implementation.
If
> it was not an anonymous class I would have had that inside of it.

Hm... maybe "Used by |mNewTabPageManager|"?

[dgn, 2016-10-03 11:51:55]

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

[commit-bot: I haz the power, 2016-10-03 11:52:10]

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

[dgn, 2016-10-03 11:57:08]

PTAL

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

https://codereview.chromium.org/2378323002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:156: /**
Field backing the NewTabPageManager. Observer tracked for de-registration
purposes. */
On 2016/09/30 15:22:02, Bernhard Bauer wrote:
> On 2016/09/30 15:16:58, dgn wrote:
> > On 2016/09/30 14:31:50, Bernhard Bauer wrote:
> > > What does the first sentence mean?
> > 
> > I added that because that's kind of leaking out of NTPManager
implementation.
> If
> > it was not an anonymous class I would have had that inside of it.
> 
> Hm... maybe "Used by |mNewTabPageManager|"?

Done.

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

https://codereview.chromium.org/2378323002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:163:
@Override
On 2016/09/30 14:31:50, Bernhard Bauer wrote:
> Indentation?

Done.

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

https://codereview.chromium.org/2378323002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SigninPromoItem.java:26:
public class SigninPromoItem extends StatusItem implements ItemGroup,
NewTabPageItem {
On 2016/09/30 14:31:50, Bernhard Bauer wrote:
> Doesn't StatusItem already implement NewTagPageItem?

Done.

[dgn, 2016-10-03 11:57:20]

Description was changed from

==========
[NTP Client] Show the Sign In promo as a separate card from the section

BUG=649670
==========

to

==========
[NTP Client] Show the Sign In promo as a separate card from the section

BUG=649670
==========

[Bernhard Bauer, 2016-10-03 12:14:39]

lgtm

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

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

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

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dgn, 2016-10-03 14:55:52]

The CQ bit was checked by dgn@chromium.org

[dgn, 2016-10-03 14:55:52]

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

[commit-bot: I haz the power, 2016-10-03 14:56:01]

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-10-03 16:57:53]

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

[commit-bot: I haz the power, 2016-10-03 16:57:54]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dgn, 2016-10-03 17:49:10]

The CQ bit was checked by dgn@chromium.org

[commit-bot: I haz the power, 2016-10-03 17:49: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-10-03 18:30:31]

Description was changed from

==========
[NTP Client] Show the Sign In promo as a separate card from the section

BUG=649670
==========

to

==========
[NTP Client] Show the Sign In promo as a separate card from the section

BUG=649670
==========

[commit-bot: I haz the power, 2016-10-03 18:30:32]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-03 18:33:31]

Description was changed from

==========
[NTP Client] Show the Sign In promo as a separate card from the section

BUG=649670
==========

to

==========
[NTP Client] Show the Sign In promo as a separate card from the section

BUG=649670

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

[commit-bot: I haz the power, 2016-10-03 18:33:32]

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