[NTP] Remove the ScrollView version of the NTP.

[NTP] Remove the ScrollView version of the NTP.

BUG=¯\_(ツ)_/¯
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2655263003
Cr-Commit-Position: refs/heads/master@{#446684}
Committed: https://chromium.googlesource.com/chromium/src/+/45c1e695513a13455323fbb639a15276d52b5c2d

[PEConn, 2017-01-26 12:04:00]

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

[PEConn, 2017-01-26 12:04:01]

PTAL, I'm hoping to get this done quickly before the inevitable merge conflicts
start piling up.

[PEConn, 2017-01-26 12:05:37]

https://codereview.chromium.org/2655263003/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/new_tab_page_scroll_view.xml (left):

https://codereview.chromium.org/2655263003/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/new_tab_page_scroll_view.xml:16: <include
layout="@layout/new_tab_page_layout" />
I suppose now I can just merge new_tab_page_recycler_view and
new_tab_page_layout.

[Bernhard Bauer, 2017-01-26 12:10:14]

Can we also remove ChromeFeatureList.NTP_SNIPPETS (in Java) and
ntp_snippets::kContentSuggestionsFeature (in C++)?

Also, we should update Finch configs to remove the holdback on Stable.

[Marc Treib, 2017-01-26 12:12:21]

On 2017/01/26 12:10:14, Bernhard Bauer wrote:
> Can we also remove ChromeFeatureList.NTP_SNIPPETS (in Java) and
> ntp_snippets::kContentSuggestionsFeature (in C++)?
> 
> Also, we should update Finch configs to remove the holdback on Stable.

Wait, have we decided to get rid of the holdback now?
(I'm all for it, but my understanding was that we're keeping it for the
foreseeable future)

[Marc Treib, 2017-01-26 12:13:08]

On 2017/01/26 12:12:21, Marc Treib wrote:
> On 2017/01/26 12:10:14, Bernhard Bauer wrote:
> > Can we also remove ChromeFeatureList.NTP_SNIPPETS (in Java) and
> > ntp_snippets::kContentSuggestionsFeature (in C++)?
> > 
> > Also, we should update Finch configs to remove the holdback on Stable.
> 
> Wait, have we decided to get rid of the holdback now?
> (I'm all for it, but my understanding was that we're keeping it for the
> foreseeable future)

Also, we can't immediately get rid of the feature anyway, because several
parameters are keyed to it.

[Bernhard Bauer, 2017-01-26 12:13:38]

https://codereview.chromium.org/2655263003/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/new_tab_page_scroll_view.xml (left):

https://codereview.chromium.org/2655263003/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/new_tab_page_scroll_view.xml:16: <include
layout="@layout/new_tab_page_layout" />
On 2017/01/26 12:05:37, PEConn wrote:
> I suppose now I can just merge new_tab_page_recycler_view and
> new_tab_page_layout.

Hm, new_tab_page_recycler_view is still used e.g. in the SuggestionsBottomSheet,
but we can probably simplify some intialization in NewTabPageView, like inlining
the RecyclerView in the new_tab_page_view.xml instead of manually inflating it
and replacing the stub.

[Bernhard Bauer, 2017-01-26 12:23:52]

On 2017/01/26 12:13:08, Marc Treib wrote:
> On 2017/01/26 12:12:21, Marc Treib wrote:
> > On 2017/01/26 12:10:14, Bernhard Bauer wrote:
> > > Can we also remove ChromeFeatureList.NTP_SNIPPETS (in Java) and
> > > ntp_snippets::kContentSuggestionsFeature (in C++)?
> > > 
> > > Also, we should update Finch configs to remove the holdback on Stable.
> > 
> > Wait, have we decided to get rid of the holdback now?
> > (I'm all for it, but my understanding was that we're keeping it for the
> > foreseeable future)

Yes, I discussed that with Patrick yesterday. +nepper to keep me honest :)

> Also, we can't immediately get rid of the feature anyway, because several
> parameters are keyed to it.

Really? The only references I've found were to turn everything off (there are
feature params configurable in chrome://flags, but those should be configurable
without the feature itself, right? And there is no code that reads the params
keyed on the feature itself).

[Marc Treib, 2017-01-26 12:30:08]

On 2017/01/26 12:23:52, Bernhard Bauer wrote:
> On 2017/01/26 12:13:08, Marc Treib wrote:
> > On 2017/01/26 12:12:21, Marc Treib wrote:
> > > On 2017/01/26 12:10:14, Bernhard Bauer wrote:
> > > > Can we also remove ChromeFeatureList.NTP_SNIPPETS (in Java) and
> > > > ntp_snippets::kContentSuggestionsFeature (in C++)?
> > > > 
> > > > Also, we should update Finch configs to remove the holdback on Stable.
> > > 
> > > Wait, have we decided to get rid of the holdback now?
> > > (I'm all for it, but my understanding was that we're keeping it for the
> > > foreseeable future)
> 
> Yes, I discussed that with Patrick yesterday. +nepper to keep me honest :)
> 
> > Also, we can't immediately get rid of the feature anyway, because several
> > parameters are keyed to it.
> 
> Really? The only references I've found were to turn everything off (there are
> feature params configurable in chrome://flags, but those should be
configurable
> without the feature itself, right? And there is no code that reads the params
> keyed on the feature itself).

Ah, you're right - we have several params keyed to subfeatures like
NTPArticleSuggestions, as well as a few remaining ones keyed to the study name,
but nothing on the main feature. So removing the feature is yes :)

[PEConn, 2017-01-26 12:37:13]

- Removed the pre-cards toolbar.
- Included the RecyclerView directly, no stubs.
- Removed SnippetsConfig.isEnabled() and simplified code that called it.

[dgn, 2017-01-26 13:47:04]

And yes please on removing the flag :)

https://codereview.chromium.org/2655263003/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(right):

https://codereview.chromium.org/2655263003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:412:
public ViewGroup getWrapperView() {
change to getRecyclerView() and change the return type? so we avoid casting it
in tests. Also replace local usage with mRecyclerView

https://codereview.chromium.org/2655263003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:1017:
private int getVerticalScroll() {
remove this?

[Bernhard Bauer, 2017-01-26 14:05:01]

\m/

LGTM w/ Nicolas' additional suggestions.

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

https://codereview.chromium.org/2655263003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:594: //
TODO(peconn): Can remove this?
I think the tablet UI still uses this... That should get overhauled anyway
though.

[Bernhard Bauer, 2017-01-26 14:05:41]

On 2017/01/26 12:23:52, Bernhard Bauer wrote:
> On 2017/01/26 12:13:08, Marc Treib wrote:
> > On 2017/01/26 12:12:21, Marc Treib wrote:
> > > On 2017/01/26 12:10:14, Bernhard Bauer wrote:
> > > > Can we also remove ChromeFeatureList.NTP_SNIPPETS (in Java) and
> > > > ntp_snippets::kContentSuggestionsFeature (in C++)?
> > > > 
> > > > Also, we should update Finch configs to remove the holdback on Stable.
> > > 
> > > Wait, have we decided to get rid of the holdback now?
> > > (I'm all for it, but my understanding was that we're keeping it for the
> > > foreseeable future)
> 
> Yes, I discussed that with Patrick yesterday. +nepper to keep me honest :)

Argh, actually +nepper

[PEConn, 2017-01-26 15:56:58]

Description was changed from

==========
[NTP] Remove the ScrollView version of the NTP.

BUG=¯\_(ツ)_/¯
==========

to

==========
[NTP] Remove the ScrollView version of the NTP.

BUG=¯\_(ツ)_/¯
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[PEConn, 2017-01-26 15:59:59]

noyau@ please look at:
- ios_chrome_content_suggestions_service_factory.cc

bauerb@, could you please have a look over the C++ side, I left the flag in
about_flags but changed its meaning (as I'd prefer to get this CL done quickly).

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

https://codereview.chromium.org/2655263003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:594: //
TODO(peconn): Can remove this?
On 2017/01/26 14:05:01, Bernhard Bauer wrote:
> I think the tablet UI still uses this... That should get overhauled anyway
> though.

Acknowledged.

https://codereview.chromium.org/2655263003/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(right):

https://codereview.chromium.org/2655263003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:412:
public ViewGroup getWrapperView() {
On 2017/01/26 13:47:04, dgn wrote:
> change to getRecyclerView() and change the return type? so we avoid casting it
> in tests. Also replace local usage with mRecyclerView

Done.

https://codereview.chromium.org/2655263003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:1017:
private int getVerticalScroll() {
On 2017/01/26 13:47:04, dgn wrote:
> remove this?

Done.

[Bernhard Bauer, 2017-01-26 16:03:35]

lgtm

[PEConn, 2017-01-26 16:05:16]

peconn@chromium.org changed reviewers:
+ noyau@chromium.org

[PEConn, 2017-01-26 16:05:17]

noyau@, could you please review the single ios file?

[PEConn, 2017-01-26 16:34:04]

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

[commit-bot: I haz the power, 2017-01-26 16:34: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, 2017-01-26 18:32:54]

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

[commit-bot: I haz the power, 2017-01-26 18:32:55]

Dry run: This issue passed the CQ dry run.

[noyau (Ping after 24h), 2017-01-27 15:39:52]

lgtm

ios lgtm

[PEConn, 2017-01-27 16:29:06]

The CQ bit was checked by peconn@chromium.org

[commit-bot: I haz the power, 2017-01-27 16:29:30]

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-27 16:35:59]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485534546679760,
"parent_rev": "d3688ba478b1992affc20ec3209efe7fef9577be", "commit_rev":
"45c1e695513a13455323fbb639a15276d52b5c2d"}

[commit-bot: I haz the power, 2017-01-27 16:36:32]

Description was changed from

==========
[NTP] Remove the ScrollView version of the NTP.

BUG=¯\_(ツ)_/¯
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[NTP] Remove the ScrollView version of the NTP.

BUG=¯\_(ツ)_/¯
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2655263003
Cr-Commit-Position: refs/heads/master@{#446684}
Committed:
https://chromium.googlesource.com/chromium/src/+/45c1e695513a13455323fbb639a1...
==========

[commit-bot: I haz the power, 2017-01-27 16:36:33]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/45c1e695513a13455323fbb639a1...