components/ntp_snippets/ios/BUILD.gn - Issue 2755113002: Create ReadingListSuggestionsProvider

Unified Diff: components/ntp_snippets/ios/BUILD.gn

Issue 2755113002: Create ReadingListSuggestionsProvider (Closed)

Patch Set: Created 3 years, 9 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

« components/ntp_snippets/category_rankers/constant_category_ranker.cc ('K') | « components/ntp_snippets/content_suggestions_metrics.cc ('k') | components/ntp_snippets/ios/DEPS » ('j') | components/ntp_snippets/ios/reading_list_suggestions_provider.h » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: components/ntp_snippets/ios/BUILD.gn

diff --git a/components/ntp_snippets/ios/BUILD.gn b/components/ntp_snippets/ios/BUILD.gn

new file mode 100644

index 0000000000000000000000000000000000000000..3728d592d68e24aae21e42daed9337c26507c8e0

--- /dev/null

+++ b/components/ntp_snippets/ios/BUILD.gn

@@ -0,0 +1,16 @@

Marc Treib 2017/03/17 14:44:00 I thought we agreed on components/ntp_snippets/rea

gambard 2017/03/20 08:32:39 The components/reading_list(/ios) target is not bu

Marc Treib 2017/03/20 09:55:26 Hm, so far we've build everything everywhere, incl

jkrcal 2017/03/20 11:50:00 Building stuff on desktop allows us to run unit-te

gambard 2017/03/23 15:45:52 The ReadingListProvider is built for iOS only in c

On 2017/03/20 11:50:00, jkrcal wrote: > On 2017/03/20 09:55:26, Marc Treib wrote: > > On 2017/03/20 08:32:39, gambard wrote: > > > On 2017/03/17 14:44:00, Marc Treib wrote: > > > > 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/ ? > > > > > > 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. > > > > 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 > > 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.

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,

On 2017/03/23 15:45:52, gambard wrote: > On 2017/03/20 11:50:00, jkrcal wrote: > > On 2017/03/20 09:55:26, Marc Treib wrote: > > > On 2017/03/20 08:32:39, gambard wrote: > > > > On 2017/03/17 14:44:00, Marc Treib wrote: > > > > > 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/ ? > > > > > > > > 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. > > > > > > 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 > > > > 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. > > The ReadingListProvider is built for iOS only in chrome but build for every > platform in components_unit_tests

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 t

On 2017/03/24 09:57:03, Olivier Robin wrote: > On 2017/03/23 15:45:52, gambard wrote: > > On 2017/03/20 11:50:00, jkrcal wrote: > > > On 2017/03/20 09:55:26, Marc Treib wrote: > > > > On 2017/03/20 08:32:39, gambard wrote: > > > > > On 2017/03/17 14:44:00, Marc Treib wrote: > > > > > > 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/ ? > > > > > > > > > > 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. > > > > > > > > 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 > > > > > > 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. > > > > The ReadingListProvider is built for iOS only in chrome but build for every > > platform in components_unit_tests > > 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).

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 dep

On 2017/03/27 08:19:22, Marc Treib wrote: > On 2017/03/24 09:57:03, Olivier Robin wrote: > > On 2017/03/23 15:45:52, gambard wrote: > > > On 2017/03/20 11:50:00, jkrcal wrote: > > > > On 2017/03/20 09:55:26, Marc Treib wrote: > > > > > On 2017/03/20 08:32:39, gambard wrote: > > > > > > On 2017/03/17 14:44:00, Marc Treib wrote: > > > > > > > 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/ ? > > > > > > > > > > > > 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. > > > > > > > > > > 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 > > > > > > > > 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. > > > > > > The ReadingListProvider is built for iOS only in chrome but build for every > > > platform in components_unit_tests > > > > 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). > > 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. > > > So when I see that ios now builds component/offline_pages because of the > > provider, I think this should be avoided. > > 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. > > > Of course, this does not prevent use from building everything for the > unittests. > > Indeed, we'd very much like to keep that. Running unit tests on the workstation > is just *so* much more convenient.

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",

+ ]