Add an experimental standalone content suggestions UI.

Add an experimental standalone content suggestions UI.

The UI is hidden behind a feature flag that enables a menu item (which
uses innovative string compression techniques to avoid bloat due to
i18n) to open the standalone content suggestions UI.

While the activity and menu item are not meant to be shipped in an
enabled state, this will allow iterating on a content suggestions UI
outside of the NTP.

Review-Url: https://codereview.chromium.org/2593523005
Cr-Commit-Position: refs/heads/master@{#441980}
Committed: https://chromium.googlesource.com/chromium/src/+/1b34593a53181b7f8a06a97206219c16486b33a1

[Bernhard Bauer, 2017-01-03 16:25:51]

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

[commit-bot: I haz the power, 2017-01-03 16:26:02]

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-03 16:45:42]

Description was changed from

==========
WIP: Add a ContentSuggestionsActivity that shows content suggestions
outside of the New Tab Page.
==========

to

==========
Add an experimental standalone content suggestions UI.

The UI is hidden behind a feature flag that enables a menu item (which uses
innovative string compression techniques to avoid bloat due to i18n) to open the
standalone content suggestions UI.

While the activity and menu item are not meant to be launched, this will allow
iterating on a content suggestions UI outside of the NTP.
==========

[Bernhard Bauer, 2017-01-03 16:47:59]

Description was changed from

==========
Add an experimental standalone content suggestions UI.

The UI is hidden behind a feature flag that enables a menu item (which uses
innovative string compression techniques to avoid bloat due to i18n) to open the
standalone content suggestions UI.

While the activity and menu item are not meant to be launched, this will allow
iterating on a content suggestions UI outside of the NTP.
==========

to

==========
Add an experimental standalone content suggestions UI.

The UI is hidden behind a feature flag that enables a menu item (which
uses innovative string compression techniques to avoid bloat due to
i18n) to open the standalone content suggestions UI.

While the activity and menu item are not meant to be launched, this
will allow iterating on a content suggestions UI outside of the NTP.
==========

[Bernhard Bauer, 2017-01-03 16:48:22]

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

[Bernhard Bauer, 2017-01-03 16:48:23]

Please review. Thanks!

[Bernhard Bauer, 2017-01-03 16:49:19]

Description was changed from

==========
Add an experimental standalone content suggestions UI.

The UI is hidden behind a feature flag that enables a menu item (which
uses innovative string compression techniques to avoid bloat due to
i18n) to open the standalone content suggestions UI.

While the activity and menu item are not meant to be launched, this
will allow iterating on a content suggestions UI outside of the NTP.
==========

to

==========
Add an experimental standalone content suggestions UI.

The UI is hidden behind a feature flag that enables a menu item (which
uses innovative string compression techniques to avoid bloat due to
i18n) to open the standalone content suggestions UI.

While the activity and menu item are not meant to be shipped in an
enabled state, this will allow iterating on a content suggestions UI
outside of the NTP.
==========

[dgn, 2017-01-04 12:19:42]

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

[dgn, 2017-01-04 12:19:43]

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

https://codereview.chromium.org/2593523005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java:119:
mTab.getWindowAndroid().addContextMenuCloseListener(this);
Activity#onContextMenuClosed as a substitute maybe? We could add something to
NTPManager to expose it.

https://codereview.chromium.org/2593523005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java:130:
public void closeContextMenu() {
we also use this method to close the menu when changing orientation. Provide the
ContentSuggestionActivity instead of using the tab to get one?

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

https://codereview.chromium.org/2593523005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:75:
mRoot.addChildren(mSections, mSigninPromo, mAllDismissed, mFooter,
mBottomSpacer);
Is the bottom spacer still necessary? IIRC that was there only because we needed
to push content up enough to have the above/below-the-fold split.

https://codereview.chromium.org/2593523005/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsActivity.java
(right):

https://codereview.chromium.org/2593523005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsActivity.java:54:
private class SuggestionsNewTabPageManager implements NewTabPageManager {
We'll have to split this in another CL. A bunch of methods make no sense here.

https://codereview.chromium.org/2593523005/diff/60001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2593523005/diff/60001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:2880: 🔰🆕👌
awesome

[Michael van Ouwerkerk, 2017-01-04 16:45:36]

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

https://codereview.chromium.org/2593523005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:69:
if (mAboveTheFoldView == null) {
I worry this will lead to many unhandled edge cases, that view being always
present is such a central assumption.

[Bernhard Bauer, 2017-01-05 12:02:15]

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

https://codereview.chromium.org/2593523005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java:119:
mTab.getWindowAndroid().addContextMenuCloseListener(this);
On 2017/01/04 12:19:43, dgn wrote:
> Activity#onContextMenuClosed as a substitute maybe? We could add something to
> NTPManager to expose it.

As discussed offline, changed ContextMenuManager so it will stay registered as a
ContextMenuCloseListener.

https://codereview.chromium.org/2593523005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java:130:
public void closeContextMenu() {
On 2017/01/04 12:19:43, dgn wrote:
> we also use this method to close the menu when changing orientation. Provide
the
> ContentSuggestionActivity instead of using the tab to get one?

Done.

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

https://codereview.chromium.org/2593523005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:69:
if (mAboveTheFoldView == null) {
On 2017/01/04 16:45:35, Michael van Ouwerkerk wrote:
> I worry this will lead to many unhandled edge cases, that view being always
> present is such a central assumption.

Hm, if it is, that assumption is going to be wrong :) And in that case I'd
rather learn about it via a NullPointerException.

(Also, because the above-the-fold view already may or may not be attached to the
RecyclerView, I think in practice the majority of cases already deal with a
potentially missing above-the-fold view)

https://codereview.chromium.org/2593523005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:75:
mRoot.addChildren(mSections, mSigninPromo, mAllDismissed, mFooter,
mBottomSpacer);
On 2017/01/04 12:19:43, dgn wrote:
> Is the bottom spacer still necessary? IIRC that was there only because we
needed
> to push content up enough to have the above/below-the-fold split.

Done. We need to add some checks to allow the adapter to handle the case when
the SPACING item is now missing, but the view code is already handling it
anyway.

https://codereview.chromium.org/2593523005/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsActivity.java
(right):

https://codereview.chromium.org/2593523005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/suggestions/ContentSuggestionsActivity.java:54:
private class SuggestionsNewTabPageManager implements NewTabPageManager {
On 2017/01/04 12:19:43, dgn wrote:
> We'll have to split this in another CL. A bunch of methods make no sense here.

Yesssss

[Michael van Ouwerkerk, 2017-01-05 13:58:35]

lgtm

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

https://codereview.chromium.org/2593523005/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:69:
if (mAboveTheFoldView == null) {
On 2017/01/05 12:02:15, Bernhard Bauer wrote:
> On 2017/01/04 16:45:35, Michael van Ouwerkerk wrote:
> > I worry this will lead to many unhandled edge cases, that view being always
> > present is such a central assumption.
> 
> Hm, if it is, that assumption is going to be wrong :) And in that case I'd
> rather learn about it via a NullPointerException.
> 
> (Also, because the above-the-fold view already may or may not be attached to
the
> RecyclerView, I think in practice the majority of cases already deal with a
> potentially missing above-the-fold view)

I think it may also fail in ways that are not NullPointerExceptions. But, maybe
it's not that bad for the reasons you gave.

[dgn, 2017-01-05 17:16:25]

lgtm

[Bernhard Bauer, 2017-01-05 17:20:02]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2017-01-05 17:20:24]

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-05 17:58:35]

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

[commit-bot: I haz the power, 2017-01-05 17:58:36]

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

[Bernhard Bauer, 2017-01-05 18:46:23]

The CQ bit was checked by bauerb@chromium.org

[Bernhard Bauer, 2017-01-05 18:46:23]

The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org,
mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/2593523005/#ps100001
(title: "fix test compile")

[commit-bot: I haz the power, 2017-01-05 18:46:56]

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

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

[commit-bot: I haz the power, 2017-01-05 20:13:14]

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

[Bernhard Bauer, 2017-01-06 11:36:06]

The CQ bit was checked by bauerb@chromium.org

[Bernhard Bauer, 2017-01-06 11:36:07]

The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org,
mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/2593523005/#ps120001
(title: "fix test")

[commit-bot: I haz the power, 2017-01-06 11:36:21]

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-06 11:38:12]

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

[commit-bot: I haz the power, 2017-01-06 11:38:13]

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-06 14:59:19]

The CQ bit was checked by bauerb@chromium.org

[Bernhard Bauer, 2017-01-06 14:59:20]

The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org,
mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/2593523005/#ps140001
(title: "sync")

[commit-bot: I haz the power, 2017-01-06 14:59:43]

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-06 16:45:49]

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

[commit-bot: I haz the power, 2017-01-06 16:45:50]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Bernhard Bauer, 2017-01-06 17:15:42]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2017-01-06 17:16:13]

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-06 18:21:54]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1483722942773160,
"parent_rev": "27fa89644b9c8ea68e0445ff6f170acdc3eb0933", "commit_rev":
"1b34593a53181b7f8a06a97206219c16486b33a1"}

[commit-bot: I haz the power, 2017-01-06 18:22:25]

Description was changed from

==========
Add an experimental standalone content suggestions UI.

The UI is hidden behind a feature flag that enables a menu item (which
uses innovative string compression techniques to avoid bloat due to
i18n) to open the standalone content suggestions UI.

While the activity and menu item are not meant to be shipped in an
enabled state, this will allow iterating on a content suggestions UI
outside of the NTP.
==========

to

==========
Add an experimental standalone content suggestions UI.

The UI is hidden behind a feature flag that enables a menu item (which
uses innovative string compression techniques to avoid bloat due to
i18n) to open the standalone content suggestions UI.

While the activity and menu item are not meant to be shipped in an
enabled state, this will allow iterating on a content suggestions UI
outside of the NTP.

Review-Url: https://codereview.chromium.org/2593523005
Cr-Commit-Position: refs/heads/master@{#441980}
Committed:
https://chromium.googlesource.com/chromium/src/+/1b34593a53181b7f8a06a9720621...
==========

[commit-bot: I haz the power, 2017-01-06 18:22:27]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/1b34593a53181b7f8a06a9720621...

[dewittj, 2017-01-06 21:53:34]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/2614023005/ by dewittj@chromium.org.

The reason for reverting is: Speculative revert, likely broke:

https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test...

However, there appears to be logdog problems, so I'm not sure if we are getting
realistic error messages.

E 1343.751s run_tests_on_device(07601d750ae534e5)  Error not bootstrapped.
Failed to start logdog: Missing project [LOGDOG_STREAM_PROJECT]
Traceback (most recent call last):
  File "/b/s/w/irq8DwmG/build/android/pylib/android/logdog_logcat_monitor.py",
line 28, in __init__
    self._stream_client = bootstrap.ButlerBootstrap.probe().stream_client()
  File "/b/s/w/irq8DwmG/tools/swarming_client/libs/logdog/bootstrap.py", line
48, in probe
    raise NotBootstrappedError('Missing project [%s]' % (cls._ENV_PROJECT,))
NotBootstrappedError: Missing project [LOGDOG_STREAM_PROJECT].