Create ReadingListSuggestionsProvider

components/ntp_snippets/ios/BUILD.gn

+# Copyright 2017 The Chromium Authors. All rights reserved.

[Marc Treib, 2017/03/17 14:44:00]

I thought we agreed on components/ntp_snippets/reading_list/, analogous to the
other providers?

Also, do we really need a separate build file here? All the other providers are
listed in the main ntp_snippets/BUILD.gn.
If the separate build file is indeed needed, then should the folder be
components/ntp_snippets/ios/reading_list/ ?

[gambard, 2017/03/20 08:32:39]

The components/reading_list(/ios) target is not built for other platform. It is
possible (there is only one file to move to a separate target) but as no-one is
using it, we don't see the point of increasing the build time for other
platforms.
As we don't plan to have other iOS specific providers soon, I thought that it
would be OK to create it directly in ios/. But I don't feel strongly about
moving it to ios/reading_list if you think it is better.

[Marc Treib, 2017/03/20 09:55:26]

Hm, so far we've build everything everywhere, including the Android-only parts
(offline_pages/ subfolder), but maybe it's time to split things up by platform.
Let's get some more opinions here - +jkrcal, +bauerb

[jkrcal, 2017/03/20 11:50:00]

Building stuff on desktop allows us to run unit-tests of components/ntp_snippets
on desktop. This makes life so much better for us that I am hesitant to optimize
a little bit for others ;)

How about unit-tests for reading list suggestions provider, can it technically
build and run on desktop?

If not, lets keep it in the ios subfolder (I do not care whether ios/ or
ios/reading_list).
If yes, do you feel strongly about _not_ building it on linux?

For sure you are perfectly able to run your unit-tests on iOS. 
It is more pain for the rest of us: if we make a change that breaks something in
reading_list_provider, we realize that only from build bots failures.

[gambard, 2017/03/23 15:45:52]

The ReadingListProvider is built for iOS only in chrome but build for every
platform in components_unit_tests

[Olivier, 2017/03/24 09:57:03]

I realize that this discussion is already settled, but I think we should
separate the providers by platform.
I am sure this is not a problem on desktop, where size and ram is unlimited, but
on iOS we are struggling to reduce binary size. The provider by itself is not
big, but the dependency induced by building it may make a difference (specially
if the component has assets).

So when I see that ios now builds component/offline_pages because of the
provider, I think this should be avoided.

Of course, this does not prevent use from building everything for the unittests.

[Marc Treib, 2017/03/27 08:19:22]

We're working with Android, where resources tend to be more constrained than on
iOS :)

I'm not sure if compiling this stuff will have any effect on binary size. If the
linker does its job, then all of it should get stripped out anyway.
Could we verify if this actually has an impact on binary size? If it does, then
I agree it would be good to split things up by platform.
Indeed, we'd very much like to keep that. Running unit tests on the workstation
is just *so* much more convenient.

[Olivier, 2017/03/27 15:29:24]

I checked binary size on iOS, and removing the dependency does not seem to
change the binary size at all.
This means that either there is no consequence, or that another components
depends on offline_pages :)

LGTM for this CL, but I agree on filing a bug to fix this.

+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+source_set("ios") {
+  sources = [
+    "reading_list_suggestions_provider.cc",
+    "reading_list_suggestions_provider.h",
+  ]
+  deps = [
+    "//base",
+    "//components/ntp_snippets",
+    "//components/reading_list/ios",
+    "//components/strings",
+  ]
+}