Don't instantiate ContentSuggestionsService on non-Android

Don't instantiate ContentSuggestionsService on non-Android

BUG=688366
Review-Url: https://codereview.chromium.org/2686053002
Cr-Commit-Position: refs/heads/master@{#449293}
Committed: https://chromium.googlesource.com/chromium/src/+/c18c1ca3ce39b15464102d92a15367b0e5a89ac7

[Marc Treib, 2017-02-09 10:54:06]

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

[commit-bot: I haz the power, 2017-02-09 10:54:16]

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

[Marc Treib, 2017-02-09 10:55:05]

treib@chromium.org changed reviewers:
+ jkrcal@chromium.org

[tschumann, 2017-02-09 11:02:18]

is there a nice way we can test this? E.g. making sure that we get nullptr on
Desktop?

[commit-bot: I haz the power, 2017-02-09 11:04:31]

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

[commit-bot: I haz the power, 2017-02-09 11:04:32]

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

[Marc Treib, 2017-02-09 12:07:32]

On 2017/02/09 11:02:18, tschumann wrote:
> is there a nice way we can test this? E.g. making sure that we get nullptr on
> Desktop?

I suppose we could add a browser test for the factory. I'll look into that.

[jkrcal, 2017-02-09 13:06:30]

Is it intentional that this does not touch iOS?
 (I think it is what we want, just wanted to make sure)

I think we should also return nullptr if the kContentSuggestionsSource feature
(the main Zine feature) is disabled. Strangely, we never use that feature at the
moment.

As regards iOS, I suggest to switch the main feature and the articles feature on
Beta/Stable using field trial for all non-Android platforms, including iOS.

[Marc Treib, 2017-02-09 13:11:33]

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

[commit-bot: I haz the power, 2017-02-09 13:11:41]

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

[Marc Treib, 2017-02-09 13:14:17]

On 2017/02/09 13:06:30, jkrcal wrote:
> Is it intentional that this does not touch iOS?
>  (I think it is what we want, just wanted to make sure)

Yup, that's intentional. iOS is actively being worked on now, so I don't think
we want to disable things there.

> I think we should also return nullptr if the kContentSuggestionsSource feature
> (the main Zine feature) is disabled. Strangely, we never use that feature at
the
> moment.

Hm. I think the plan was to get rid of that feature. Do you think we should keep
it around as an emergency switch? I'm not sure if that's a good idea, since that
code path would get zero test coverage... (so chances are, if we ever need that
emergency switch, then we'd get nullptr crashes somewhere else).

> As regards iOS, I suggest to switch the main feature and the articles feature
on
> Beta/Stable using field trial for all non-Android platforms, including iOS.

...I don't follow?

[jkrcal, 2017-02-09 13:31:26]

On 2017/02/09 at 13:14:17, treib wrote:
> On 2017/02/09 13:06:30, jkrcal wrote:
> > Is it intentional that this does not touch iOS?
> >  (I think it is what we want, just wanted to make sure)
> 
> Yup, that's intentional. iOS is actively being worked on now, so I don't think
we want to disable things there.
> 
> > I think we should also return nullptr if the kContentSuggestionsSource
feature
> > (the main Zine feature) is disabled. Strangely, we never use that feature at
the
> > moment.
> 
> Hm. I think the plan was to get rid of that feature. Do you think we should
keep it around as an emergency switch? I'm not sure if that's a good idea, since
that code path would get zero test coverage... (so chances are, if we ever need
that emergency switch, then we'd get nullptr crashes somewhere else).

Well, we anyway get nullptr for
 - non-Android platform,
 - incognito profiles.

Isn't this fair enough to make sure test-coverage is there?
 
> > As regards iOS, I suggest to switch the main feature and the articles
feature on
> > Beta/Stable using field trial for all non-Android platforms, including iOS.
> 
> ...I don't follow?

Well, we want to make sure we do not fetch suggestions (or do any other funky
background work) on non-Android platforms, incl. iOS, before we truly launch
there.
We cannot use an #ifdef on iOS because this would complicate developments. Using
the main feature switch on other platforms on non-dev channels would address
this issue, IMO.

[Marc Treib, 2017-02-09 13:45:29]

On 2017/02/09 13:31:26, jkrcal wrote:
> On 2017/02/09 at 13:14:17, treib wrote:
> > On 2017/02/09 13:06:30, jkrcal wrote:
> > > Is it intentional that this does not touch iOS?
> > >  (I think it is what we want, just wanted to make sure)
> > 
> > Yup, that's intentional. iOS is actively being worked on now, so I don't
think
> we want to disable things there.
> > 
> > > I think we should also return nullptr if the kContentSuggestionsSource
> feature
> > > (the main Zine feature) is disabled. Strangely, we never use that feature
at
> the
> > > moment.
> > 
> > Hm. I think the plan was to get rid of that feature. Do you think we should
> keep it around as an emergency switch? I'm not sure if that's a good idea,
since
> that code path would get zero test coverage... (so chances are, if we ever
need
> that emergency switch, then we'd get nullptr crashes somewhere else).
> 
> Well, we anyway get nullptr for
>  - non-Android platform,
>  - incognito profiles.
> 
> Isn't this fair enough to make sure test-coverage is there?

The Android, non-incognito UI probably won't have coverage for the
service-is-null case.

> > > As regards iOS, I suggest to switch the main feature and the articles
> feature on
> > > Beta/Stable using field trial for all non-Android platforms, including
iOS.
> > 
> > ...I don't follow?
> 
> Well, we want to make sure we do not fetch suggestions (or do any other funky
> background work) on non-Android platforms, incl. iOS, before we truly launch
> there.
> We cannot use an #ifdef on iOS because this would complicate developments.
Using
> the main feature switch on other platforms on non-dev channels would address
> this issue, IMO.

Ah, now I understand.

iOS will require a feature anyway to turn the new UI on or off. And this main
feature is probably just the right thing for that. But instead of configuring
that via Finch, I'd just make it disabled-by-default on iOS.
Whether the factory should also return nullptr in that case is a separate
discussion; let's keep that for another CL :)


Also: PTAL again. There's even a test now!

[Marc Treib, 2017-02-09 13:45:33]

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

[commit-bot: I haz the power, 2017-02-09 13:45:58]

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

[jkrcal, 2017-02-09 13:55:04]

On 2017/02/09 at 13:45:29, treib wrote:
> On 2017/02/09 13:31:26, jkrcal wrote:
> > On 2017/02/09 at 13:14:17, treib wrote:
> > > On 2017/02/09 13:06:30, jkrcal wrote:
> > > > Is it intentional that this does not touch iOS?
> > > >  (I think it is what we want, just wanted to make sure)
> > > 
> > > Yup, that's intentional. iOS is actively being worked on now, so I don't
think
> > we want to disable things there.
> > > 
> > > > I think we should also return nullptr if the kContentSuggestionsSource
> > feature
> > > > (the main Zine feature) is disabled. Strangely, we never use that
feature at
> > the
> > > > moment.
> > > 
> > > Hm. I think the plan was to get rid of that feature. Do you think we
should
> > keep it around as an emergency switch? I'm not sure if that's a good idea,
since
> > that code path would get zero test coverage... (so chances are, if we ever
need
> > that emergency switch, then we'd get nullptr crashes somewhere else).
> > 
> > Well, we anyway get nullptr for
> >  - non-Android platform,
> >  - incognito profiles.
> > 
> > Isn't this fair enough to make sure test-coverage is there?
> 
> The Android, non-incognito UI probably won't have coverage for the
service-is-null case.
> 
> > > > As regards iOS, I suggest to switch the main feature and the articles
> > feature on
> > > > Beta/Stable using field trial for all non-Android platforms, including
iOS.
> > > 
> > > ...I don't follow?
> > 
> > Well, we want to make sure we do not fetch suggestions (or do any other
funky
> > background work) on non-Android platforms, incl. iOS, before we truly launch
> > there.
> > We cannot use an #ifdef on iOS because this would complicate developments.
Using
> > the main feature switch on other platforms on non-dev channels would address
> > this issue, IMO.
> 
> Ah, now I understand.
> 
> iOS will require a feature anyway to turn the new UI on or off. And this main
feature is probably just the right thing for that. But instead of configuring
that via Finch, I'd just make it disabled-by-default on iOS.
> Whether the factory should also return nullptr in that case is a separate
discussion; let's keep that for another CL :)

Okay.

> 
> Also: PTAL again. There's even a test now!

lgtm

[commit-bot: I haz the power, 2017-02-09 15:12:39]

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

[commit-bot: I haz the power, 2017-02-09 15:12:40]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2017-02-09 15:15:35]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2017-02-09 15:15:49]

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-02-09 15:20:41]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486653335102270,
"parent_rev": "c53be51debc342c6b6cd8a3388fa80bbc472ad6f", "commit_rev":
"c18c1ca3ce39b15464102d92a15367b0e5a89ac7"}

[commit-bot: I haz the power, 2017-02-09 15:21:14]

Description was changed from

==========
Don't instantiate ContentSuggestionsService on non-Android

BUG=688366
==========

to

==========
Don't instantiate ContentSuggestionsService on non-Android

BUG=688366

Review-Url: https://codereview.chromium.org/2686053002
Cr-Commit-Position: refs/heads/master@{#449293}
Committed:
https://chromium.googlesource.com/chromium/src/+/c18c1ca3ce39b15464102d92a153...
==========

[commit-bot: I haz the power, 2017-02-09 15:21:16]

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

[tschumann, 2017-02-09 15:42:29]

nice! I assume you saw the test fail for desktop ;-) [I don't trust those #if
defines...]

[Marc Treib, 2017-02-09 16:02:57]

On 2017/02/09 15:42:29, tschumann wrote:
> nice! I assume you saw the test fail for desktop ;-) [I don't trust those #if
> defines...]

You mean, running the test without the new "#if"s? Yup, that fails.

Interestingly, trying to instantiate ContentSuggestionsService on desktop
crashed anyway, because we're trying to read some prefs that aren't registered.
So the problem of the service getting created on desktop has been fixed;
otherwise we would have gotten crash reports :)