|
|
Created:
3 years, 9 months ago by gambard Modified:
3 years, 9 months ago CC:
asvitkine+watch_chromium.org, blundell+watchlist_chromium.org, chromium-reviews, droger+watchlist_chromium.org, ios-reviews_chromium.org, ios-reviews+chrome_chromium.org, marq+watch_chromium.org, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org, pkl (ping after 24h if needed), sdefresne+watch_chromium.org, sdefresne+watchlist_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate ReadingListSuggestionsProvider
This CL creates the ReadingListSuggestionsProvider.
BUG=702241
Review-Url: https://codereview.chromium.org/2755113002
Cr-Commit-Position: refs/heads/master@{#460039}
Committed: https://chromium.googlesource.com/chromium/src/+/c9bff741d84b56d8f5963a398958fe85cc110fae
Patch Set 1 #
Total comments: 37
Patch Set 2 : Address comments #
Total comments: 6
Patch Set 3 : Address comments #
Total comments: 16
Patch Set 4 : Address comments #Patch Set 5 : Move test to their target #
Total comments: 7
Patch Set 6 : Address comments #
Total comments: 2
Patch Set 7 : Fix strings #
Total comments: 2
Messages
Total messages: 45 (14 generated)
gambard@chromium.org changed reviewers: + treib@chromium.org
PTAL. Feel free to add others if you think a discussion about the place of the reading list provider inside components/ntp_snippets should happen.
https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category.h (right): https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category.h:37: // Reading List entries available. Pages that the user added to their reading list. ? https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/constant_category_ranker.cc (right): https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/constant_category_ranker.cc:84: categories.push_back(KnownCategories::READING_LIST); The "EMERGING_MARKETS_ORIENTED" order explicitly has ARTICLES at the top, so I'd move this below. (Not that it'll matter in practice, but still) https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... File components/ntp_snippets/ios/BUILD.gn (right): https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/BUILD.gn:1: # Copyright 2017 The Chromium Authors. All rights reserved. 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/ ? https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... File components/ntp_snippets/ios/reading_list_suggestions_provider.cc (right): https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/reading_list_suggestions_provider.cc:30: } Do we need to register as a ReadingListObserver or something like that? https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/reading_list_suggestions_provider.cc:54: const ContentSuggestion::ID& suggestion_id){}; This should probably get implemented? (doesn't have to be in this CL, but please add a TODO at least) https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/reading_list_suggestions_provider.cc:57: const ImageFetchedCallback& callback){}; Also here https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/reading_list_suggestions_provider.cc:77: const base::Callback<bool(const GURL& url)>& filter){}; Also here https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/reading_list_suggestions_provider.cc:88: Category category){}; And here :) https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/reading_list_suggestions_provider.cc:92: FetchReadingListInternal(); nit: DCHECK_EQ(reading_list_model_, model); (though I wonder why it's even passed; seems kinda useless...) https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... File components/ntp_snippets/ios/reading_list_suggestions_provider.h (right): https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/reading_list_suggestions_provider.h:5: #ifndef COMPONENTS_NTP_SNIPPETS_READING_LIST_READING_LIST_SUGGESTIONS_PROVIDER_H_ include guard doesn't match the folder https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/reading_list_suggestions_provider.h:8: #include "components/ntp_snippets/content_suggestions_provider.h" nit: Some includes missing, e.g. for ContentSuggestion, Category, std::set https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/reading_list_suggestions_provider.h:9: #include "components/reading_list/ios/reading_list_model_observer.h" Ah, this is probably what makes the ios/ subfolder necessary... https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets_str... File components/ntp_snippets_strings.grdp (right): https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets_str... components/ntp_snippets_strings.grdp:49: <message name="IDS_NTP_READING_LIST_SUGGESTIONS_SECTION_HEADER" desc="Header of the Reading List section. The Reading List section show a list of unread article from the Reading List."> s/show/shows/ s/article/articles/ (or "pages"?) https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets_str... components/ntp_snippets_strings.grdp:50: Reading List Is "Reading List" always capitalized? AFAIK we usually don't capitalize such terms.
Thanks, PTAL. https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category.h (right): https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category.h:37: // Reading List entries available. On 2017/03/17 14:44:00, Marc Treib wrote: > Pages that the user added to their reading list. > ? Done. https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/constant_category_ranker.cc (right): https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/constant_category_ranker.cc:84: categories.push_back(KnownCategories::READING_LIST); On 2017/03/17 14:44:00, Marc Treib wrote: > The "EMERGING_MARKETS_ORIENTED" order explicitly has ARTICLES at the top, so I'd > move this below. (Not that it'll matter in practice, but still) Well, what if we want Reading List on top for emerging market? What is the difference between emerging market and general? https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... File components/ntp_snippets/ios/BUILD.gn (right): https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/BUILD.gn:1: # Copyright 2017 The Chromium Authors. All rights reserved. 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. https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... File components/ntp_snippets/ios/reading_list_suggestions_provider.cc (right): https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/reading_list_suggestions_provider.cc:30: } On 2017/03/17 14:44:01, Marc Treib wrote: > Do we need to register as a ReadingListObserver or something like that? Done. https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/reading_list_suggestions_provider.cc:54: const ContentSuggestion::ID& suggestion_id){}; On 2017/03/17 14:44:01, Marc Treib wrote: > This should probably get implemented? (doesn't have to be in this CL, but please > add a TODO at least) Done. https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/reading_list_suggestions_provider.cc:57: const ImageFetchedCallback& callback){}; On 2017/03/17 14:44:01, Marc Treib wrote: > Also here Done. https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/reading_list_suggestions_provider.cc:77: const base::Callback<bool(const GURL& url)>& filter){}; On 2017/03/17 14:44:01, Marc Treib wrote: > Also here Done. https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/reading_list_suggestions_provider.cc:88: Category category){}; On 2017/03/17 14:44:01, Marc Treib wrote: > And here :) Done. https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/reading_list_suggestions_provider.cc:92: FetchReadingListInternal(); On 2017/03/17 14:44:00, Marc Treib wrote: > nit: DCHECK_EQ(reading_list_model_, model); > (though I wonder why it's even passed; seems kinda useless...) I don't know :) It looks like a standard iOS delegate pattern. Done. https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... File components/ntp_snippets/ios/reading_list_suggestions_provider.h (right): https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/reading_list_suggestions_provider.h:5: #ifndef COMPONENTS_NTP_SNIPPETS_READING_LIST_READING_LIST_SUGGESTIONS_PROVIDER_H_ On 2017/03/17 14:44:01, Marc Treib wrote: > include guard doesn't match the folder Done. https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/reading_list_suggestions_provider.h:8: #include "components/ntp_snippets/content_suggestions_provider.h" On 2017/03/17 14:44:01, Marc Treib wrote: > nit: Some includes missing, e.g. for ContentSuggestion, Category, std::set Done. https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/reading_list_suggestions_provider.h:9: #include "components/reading_list/ios/reading_list_model_observer.h" On 2017/03/17 14:44:01, Marc Treib wrote: > Ah, this is probably what makes the ios/ subfolder necessary... See the other comment. https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets_str... File components/ntp_snippets_strings.grdp (right): https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets_str... components/ntp_snippets_strings.grdp:49: <message name="IDS_NTP_READING_LIST_SUGGESTIONS_SECTION_HEADER" desc="Header of the Reading List section. The Reading List section show a list of unread article from the Reading List."> On 2017/03/17 14:44:01, Marc Treib wrote: > s/show/shows/ > s/article/articles/ (or "pages"?) Done. https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets_str... components/ntp_snippets_strings.grdp:50: Reading List On 2017/03/17 14:44:01, Marc Treib wrote: > Is "Reading List" always capitalized? AFAIK we usually don't capitalize such > terms. It is always capitalized. At least up to now :)
quick drive-by. https://codereview.chromium.org/2755113002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ios/reading_list_suggestions_provider.h (right): https://codereview.chromium.org/2755113002/diff/20001/components/ntp_snippets... components/ntp_snippets/ios/reading_list_suggestions_provider.h:25: public ReadingListModelObserver { I wonder if we can avoid having the provider be too many roles. (just reading the constructor already requires looking up multiple interfaces to figure out what roles 'this' is playing where). Inheritance is quite a strong coupling; wouldn't aggregation suffice for listening to changes on the readinglist model? I wonder if we would make things easier to encapsulate the readinglist handling in a separate class (can be fully implemented in the .cc file) on which the provider registers a simple callback that already get's the updated data to push along. Looking at the ReadingListModelObserver interface, there's a good chance that additional methods will be implemented which would unnecessarily increase the provider's complexity. https://codereview.chromium.org/2755113002/diff/20001/components/ntp_snippets... components/ntp_snippets/ios/reading_list_suggestions_provider.h:32: // ContentSuggestionsProvider implementation. there was some discussion about artificially (and ineffectively) hiding methods from interfaces like this. IIRC, we agreed to not do that anymore and just have them public.
treib@chromium.org changed reviewers: + bauerb@chromium.org, jkrcal@chromium.org
Jan/Bernhard, please chime in on the ios/ subfolder discussion :) https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category_rankers/constant_category_ranker.cc (right): https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category_rankers/constant_category_ranker.cc:84: categories.push_back(KnownCategories::READING_LIST); On 2017/03/20 08:32:39, gambard wrote: > On 2017/03/17 14:44:00, Marc Treib wrote: > > The "EMERGING_MARKETS_ORIENTED" order explicitly has ARTICLES at the top, so > I'd > > move this below. (Not that it'll matter in practice, but still) > > Well, what if we want Reading List on top for emerging market? What is the > difference between emerging market and general? We don't :) I'll provide some more context out-of-band. https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... File components/ntp_snippets/ios/BUILD.gn (right): https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/BUILD.gn:1: # Copyright 2017 The Chromium Authors. All rights reserved. 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 https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets_str... File components/ntp_snippets_strings.grdp (right): https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets_str... components/ntp_snippets_strings.grdp:50: Reading List On 2017/03/20 08:32:40, gambard wrote: > On 2017/03/17 14:44:01, Marc Treib wrote: > > Is "Reading List" always capitalized? AFAIK we usually don't capitalize such > > terms. > > It is always capitalized. At least up to now :) Okay, you'll have to take that up with UI ;) I imagine it'll look a bit weird/inconsistent if some section titles are captialized but others aren't. https://codereview.chromium.org/2755113002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ios/reading_list_suggestions_provider.h (right): https://codereview.chromium.org/2755113002/diff/20001/components/ntp_snippets... components/ntp_snippets/ios/reading_list_suggestions_provider.h:32: // ContentSuggestionsProvider implementation. On 2017/03/20 08:58:13, tschumann wrote: > there was some discussion about artificially (and ineffectively) hiding methods > from interfaces like this. > IIRC, we agreed to not do that anymore and just have them public. It depends - things that aren't conceptually part of the public interface should IMO be "hidden" like this. (Of course, per your other comment, that kind of thing is usually not a good sign.) Here, these methods *are* part of the public interface though, so they should indeed be public.
https://codereview.chromium.org/2755113002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ios/reading_list_suggestions_provider.h (right): https://codereview.chromium.org/2755113002/diff/20001/components/ntp_snippets... components/ntp_snippets/ios/reading_list_suggestions_provider.h:32: // ContentSuggestionsProvider implementation. On 2017/03/20 09:55:26, Marc Treib wrote: > On 2017/03/20 08:58:13, tschumann wrote: > > there was some discussion about artificially (and ineffectively) hiding > methods > > from interfaces like this. > > IIRC, we agreed to not do that anymore and just have them public. > > It depends - things that aren't conceptually part of the public interface should > IMO be "hidden" like this. (Of course, per your other comment, that kind of > thing is usually not a good sign.) > > Here, these methods *are* part of the public interface though, so they should > indeed be public. The problem is that you cannot effecively hide it like that ;-) Given that we have consensus here on this particular case, no need to derail the thread further. Also, there's more discussion documented already here :-) https://groups.google.com/a/google.com/d/msg/c-style/ThZwK66DbOQ/mKAWxCoAEAAJ
https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... File components/ntp_snippets/ios/BUILD.gn (right): https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/BUILD.gn:1: # Copyright 2017 The Chromium Authors. All rights reserved. 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.
On 2017/03/20 11:50:00, jkrcal wrote: > https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... > File components/ntp_snippets/ios/BUILD.gn (right): > > https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... > components/ntp_snippets/ios/BUILD.gn:1: # Copyright 2017 The Chromium Authors. > All rights reserved. > 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. Yes, it makes sense. It should be possible to run the tests on desktop (and build the ReadingListProvider only for the tests). I don't see the benefit of building it in Chrome for all platform. Isn't the tests enough?
On 2017/03/20 13:53:27, gambard wrote: > On 2017/03/20 11:50:00, jkrcal wrote: > > > https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... > > File components/ntp_snippets/ios/BUILD.gn (right): > > > > > https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... > > components/ntp_snippets/ios/BUILD.gn:1: # Copyright 2017 The Chromium Authors. > > All rights reserved. > > 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. > > Yes, it makes sense. > It should be possible to run the tests on desktop (and build the > ReadingListProvider only for the tests). > I don't see the benefit of building it in Chrome for all platform. Isn't the > tests enough? Sure, building only for tests SGTM (but if that's not straightforward, it's IMO not worth spending a lot of effort on it).
Thanks, PTAL. I am still in ios/ but it is possible to build from the components_unit_tests. https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... File components/ntp_snippets/ios/BUILD.gn (right): https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/BUILD.gn:1: # Copyright 2017 The Chromium Authors. All rights reserved. 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 https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets_str... File components/ntp_snippets_strings.grdp (right): https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets_str... components/ntp_snippets_strings.grdp:50: Reading List On 2017/03/20 09:55:26, Marc Treib wrote: > On 2017/03/20 08:32:40, gambard wrote: > > On 2017/03/17 14:44:01, Marc Treib wrote: > > > Is "Reading List" always capitalized? AFAIK we usually don't capitalize such > > > terms. > > > > It is always capitalized. At least up to now :) > > Okay, you'll have to take that up with UI ;) > I imagine it'll look a bit weird/inconsistent if some section titles are > captialized but others aren't. I am landing this string for now, I will change it once I get a feedback from UI. https://codereview.chromium.org/2755113002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ios/reading_list_suggestions_provider.h (right): https://codereview.chromium.org/2755113002/diff/20001/components/ntp_snippets... components/ntp_snippets/ios/reading_list_suggestions_provider.h:25: public ReadingListModelObserver { On 2017/03/20 08:58:13, tschumann wrote: > I wonder if we can avoid having the provider be too many roles. > (just reading the constructor already requires looking up multiple interfaces to > figure out what roles 'this' is playing where). > > Inheritance is quite a strong coupling; wouldn't aggregation suffice for > listening to changes on the readinglist model? > > I wonder if we would make things easier to encapsulate the readinglist handling > in a separate class (can be fully implemented in the .cc file) on which the > provider registers a simple callback that already get's the updated data to push > along. > Looking at the ReadingListModelObserver interface, there's a good chance that > additional methods will be implemented which would unnecessarily increase the > provider's complexity. I planned to have the support of ReadingListModelObserver kept minimal (i.e. observing only a few events, not all of them) and the events would probably only trigger a notification to the service (i.e. no internal logic inside the provider). I fear that the separate class handling the ReadingList observation would only send a notification to the provider which would then directly forward it to the ContentSuggestionsService. I think it is OK to keep it this way for now, then if the provider evolves in something more complex we should do some inheritance. https://codereview.chromium.org/2755113002/diff/20001/components/ntp_snippets... components/ntp_snippets/ios/reading_list_suggestions_provider.h:32: // ContentSuggestionsProvider implementation. On 2017/03/20 09:59:58, tschumann wrote: > On 2017/03/20 09:55:26, Marc Treib wrote: > > On 2017/03/20 08:58:13, tschumann wrote: > > > there was some discussion about artificially (and ineffectively) hiding > > methods > > > from interfaces like this. > > > IIRC, we agreed to not do that anymore and just have them public. > > > > It depends - things that aren't conceptually part of the public interface > should > > IMO be "hidden" like this. (Of course, per your other comment, that kind of > > thing is usually not a good sign.) > > > > Here, these methods *are* part of the public interface though, so they should > > indeed be public. > > The problem is that you cannot effecively hide it like that ;-) Given that we > have consensus here on this particular case, no need to derail the thread > further. Also, there's more discussion documented already here :-) > https://groups.google.com/a/google.com/d/msg/c-style/ThZwK66DbOQ/mKAWxCoAEAAJ I set it public. https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ios/reading_list_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... components/ntp_snippets/ios/reading_list_suggestions_provider_unittest.cc:45: TEST_F(ReadingListSuggestionsProviderTest, testCategoryInfo) { This is not doing much for now, but it allows other platform to build the ReadingListSuggestionProvider.
https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... File components/ntp_snippets/BUILD.gn (right): https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... components/ntp_snippets/BUILD.gn:168: "//components/ntp_snippets/ios", This seems wrong. The general build file shouldn't reference the ios/ subfolder. How about making a "reading_list" folder after all, and having an "ios" subfolder in there, which then contains all the actual ios-only stuff? https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ios/reading_list_suggestions_provider.cc (right): https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... components/ntp_snippets/ios/reading_list_suggestions_provider.cc:42: }; nit: no semicolon after method, also in lots more places below https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... components/ntp_snippets/ios/reading_list_suggestions_provider.cc:61: void ReadingListSuggestionsProvider::FetchSuggestionImage( nit: empty line before https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ios/reading_list_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... components/ntp_snippets/ios/reading_list_suggestions_provider_unittest.cc:22: nullptr, nullptr, base::MakeUnique<base::SimpleTestClock>()); nit: Add /*the_thing=*/nullptr comments? https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... components/ntp_snippets/ios/reading_list_suggestions_provider_unittest.cc:33: ReadingListSuggestionsProvider* Provider() { return provider_.get(); } nit: either "provider" or "GetProvider" https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... components/ntp_snippets/ios/reading_list_suggestions_provider_unittest.cc:45: TEST_F(ReadingListSuggestionsProviderTest, testCategoryInfo) { nitty nit: test names are usually UpperCamel, and the leading "test" is really redundant
Thanks, PTAL. I have moved it to reading_list/ , in a separate target. https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... File components/ntp_snippets/BUILD.gn (right): https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... components/ntp_snippets/BUILD.gn:168: "//components/ntp_snippets/ios", On 2017/03/23 17:12:21, Marc Treib wrote: > This seems wrong. The general build file shouldn't reference the ios/ subfolder. > How about making a "reading_list" folder after all, and having an "ios" > subfolder in there, which then contains all the actual ios-only stuff? Ok, as there is no iOS-only stuff (only things which are not needed on other platform), I am moving it to reading_list. https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ios/reading_list_suggestions_provider.cc (right): https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... components/ntp_snippets/ios/reading_list_suggestions_provider.cc:42: }; On 2017/03/23 17:12:21, Marc Treib wrote: > nit: no semicolon after method, also in lots more places below Done. https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... components/ntp_snippets/ios/reading_list_suggestions_provider.cc:61: void ReadingListSuggestionsProvider::FetchSuggestionImage( On 2017/03/23 17:12:21, Marc Treib wrote: > nit: empty line before Done. https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ios/reading_list_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... components/ntp_snippets/ios/reading_list_suggestions_provider_unittest.cc:22: nullptr, nullptr, base::MakeUnique<base::SimpleTestClock>()); On 2017/03/23 17:12:21, Marc Treib wrote: > nit: Add /*the_thing=*/nullptr comments? Done. https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... components/ntp_snippets/ios/reading_list_suggestions_provider_unittest.cc:33: ReadingListSuggestionsProvider* Provider() { return provider_.get(); } On 2017/03/23 17:12:21, Marc Treib wrote: > nit: either "provider" or "GetProvider" Done. https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... components/ntp_snippets/ios/reading_list_suggestions_provider_unittest.cc:45: TEST_F(ReadingListSuggestionsProviderTest, testCategoryInfo) { On 2017/03/23 17:12:21, Marc Treib wrote: > nitty nit: test names are usually UpperCamel, and the leading "test" is really > redundant Done.
lgtm Some more nits, and a suggestion re build file setup. Please wait for another "okay" about the build files from jkrcal or bauerb before landing, I'd like to get a second opinion :) https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... File components/ntp_snippets/BUILD.gn (right): https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... components/ntp_snippets/BUILD.gn:168: "//components/ntp_snippets/ios", On 2017/03/23 17:49:39, gambard wrote: > On 2017/03/23 17:12:21, Marc Treib wrote: > > This seems wrong. The general build file shouldn't reference the ios/ > subfolder. > > How about making a "reading_list" folder after all, and having an "ios" > > subfolder in there, which then contains all the actual ios-only stuff? > > Ok, as there is no iOS-only stuff (only things which are not needed on other > platform), I am moving it to reading_list. Ah, so we *could* actually build it on all platforms without issues? Then TBH, I'd just do that. The impact on build time should be negligible - through the tests, all the code will get compiled anyway. And the build file setup will be much simpler that way. Am I missing something? Anyway, I'll leave the final decision to you. https://codereview.chromium.org/2755113002/diff/80001/components/BUILD.gn File components/BUILD.gn (right): https://codereview.chromium.org/2755113002/diff/80001/components/BUILD.gn#new... components/BUILD.gn:108: "//components/ntp_snippets/reading_list:unit_tests", Hm, I was about to suggest moving this dependency be moved into the "main" //components/ntp_snippets:unit_tests target, so that we don't have to change the top-level components/ build file. But I guess that would introduce a cyclic dependency? https://codereview.chromium.org/2755113002/diff/80001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2755113002/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:21: model_ = base::MakeUnique<ReadingListModelImpl>( optional: If this is created in the ctor anyway, it doesn't have to be a pointer, it could just be an instance, created in the initializer list. https://codereview.chromium.org/2755113002/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:33: Category Category() { nit: I find it confusing to overload the name between the type and the "getter". Maybe ReadingListCategory(), or just GetCategory()?
lgtm https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... File components/ntp_snippets/BUILD.gn (right): https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... components/ntp_snippets/BUILD.gn:168: "//components/ntp_snippets/ios", On 2017/03/23 19:01:49, Marc Treib wrote: > On 2017/03/23 17:49:39, gambard wrote: > > On 2017/03/23 17:12:21, Marc Treib wrote: > > > This seems wrong. The general build file shouldn't reference the ios/ > > subfolder. > > > How about making a "reading_list" folder after all, and having an "ios" > > > subfolder in there, which then contains all the actual ios-only stuff? > > > > Ok, as there is no iOS-only stuff (only things which are not needed on other > > platform), I am moving it to reading_list. > > Ah, so we *could* actually build it on all platforms without issues? Then TBH, > I'd just do that. The impact on build time should be negligible - through the > tests, all the code will get compiled anyway. And the build file setup will be > much simpler that way. > Am I missing something? > Anyway, I'll leave the final decision to you. I also value simplicity and tend to agree with Marc. However, I am also fine with the proposed separate BUILD setup. https://codereview.chromium.org/2755113002/diff/80001/components/BUILD.gn File components/BUILD.gn (right): https://codereview.chromium.org/2755113002/diff/80001/components/BUILD.gn#new... components/BUILD.gn:108: "//components/ntp_snippets/reading_list:unit_tests", On 2017/03/23 19:01:49, Marc Treib wrote: > Hm, I was about to suggest moving this dependency be moved into the "main" > //components/ntp_snippets:unit_tests target, so that we don't have to change the > top-level components/ build file. But I guess that would introduce a cyclic > dependency? +1 I do not see the cyclicity. Is it really cyclic?
gambard@chromium.org changed reviewers: + jochen@chromium.org, olivierrobin@chromium.org
Thanks! +olivierrobin@ for reading_list include in DEPS +jochen@ for tools/metrics https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... File components/ntp_snippets/BUILD.gn (right): https://codereview.chromium.org/2755113002/diff/40001/components/ntp_snippets... components/ntp_snippets/BUILD.gn:168: "//components/ntp_snippets/ios", On 2017/03/24 08:45:19, jkrcal wrote: > On 2017/03/23 19:01:49, Marc Treib wrote: > > On 2017/03/23 17:49:39, gambard wrote: > > > On 2017/03/23 17:12:21, Marc Treib wrote: > > > > This seems wrong. The general build file shouldn't reference the ios/ > > > subfolder. > > > > How about making a "reading_list" folder after all, and having an "ios" > > > > subfolder in there, which then contains all the actual ios-only stuff? > > > > > > Ok, as there is no iOS-only stuff (only things which are not needed on other > > > platform), I am moving it to reading_list. > > > > Ah, so we *could* actually build it on all platforms without issues? Then TBH, > > I'd just do that. The impact on build time should be negligible - through the > > tests, all the code will get compiled anyway. And the build file setup will be > > much simpler that way. > > Am I missing something? > > Anyway, I'll leave the final decision to you. > > I also value simplicity and tend to agree with Marc. > However, I am also fine with the proposed separate BUILD setup. It is more that we have one target per folder in ios/ so I am used to it :) Moved to ntp_snippets/BUILD https://codereview.chromium.org/2755113002/diff/80001/components/BUILD.gn File components/BUILD.gn (right): https://codereview.chromium.org/2755113002/diff/80001/components/BUILD.gn#new... components/BUILD.gn:108: "//components/ntp_snippets/reading_list:unit_tests", On 2017/03/24 08:45:19, jkrcal wrote: > On 2017/03/23 19:01:49, Marc Treib wrote: > > Hm, I was about to suggest moving this dependency be moved into the "main" > > //components/ntp_snippets:unit_tests target, so that we don't have to change > the > > top-level components/ build file. But I guess that would introduce a cyclic > > dependency? > > +1 I do not see the cyclicity. Is it really cyclic? There is no cyclic, it is just what we usually do in ios (one target per folder then adding it to the main test target). Moved to ntp_snippets target. https://codereview.chromium.org/2755113002/diff/80001/components/ntp_snippets... File components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2755113002/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:21: model_ = base::MakeUnique<ReadingListModelImpl>( On 2017/03/23 19:01:49, Marc Treib wrote: > optional: If this is created in the ctor anyway, it doesn't have to be a > pointer, it could just be an instance, created in the initializer list. The clock will be used in future tests. https://codereview.chromium.org/2755113002/diff/80001/components/ntp_snippets... components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc:33: Category Category() { On 2017/03/23 19:01:49, Marc Treib wrote: > nit: I find it confusing to overload the name between the type and the "getter". > Maybe ReadingListCategory(), or just GetCategory()? Done.
hey, I can only review UseCounter additions. Please ask a member of the metrics team to review this best -jochen
gambard@chromium.org changed reviewers: + jwd@chromium.org - jochen@chromium.org
+jwd@: PTAL histograms.xml
https://codereview.chromium.org/2755113002/diff/100001/components/ntp_snippet... File components/ntp_snippets/BUILD.gn (right): https://codereview.chromium.org/2755113002/diff/100001/components/ntp_snippet... components/ntp_snippets/BUILD.gn:112: "//components/reading_list/core", I would prefer not add a dependency to reading list on platform that don't use it. You can use the gn flag enable_reading_list to condition this dep (and the file compilation). https://codereview.chromium.org/2755113002/diff/100001/components/ntp_snippet... File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc (right): https://codereview.chromium.org/2755113002/diff/100001/components/ntp_snippet... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:19: ReadingListSuggestionsProvider::ReadingListSuggestionsProvider( Where is this created? Is this only on ios? Can this be moved to components/ntp_snippets/ios/... ?
https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... File components/ntp_snippets/ios/BUILD.gn (right): https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/BUILD.gn:1: # Copyright 2017 The Chromium Authors. All rights reserved. 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.
Maybe the best solution is to land this as one target then create a bug to split each provider in its own target?
histograms LGTM
https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... File components/ntp_snippets/ios/BUILD.gn (right): https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/BUILD.gn:1: # Copyright 2017 The Chromium Authors. All rights reserved. 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.
On 2017/03/24 15:50:26, gambard wrote: > Maybe the best solution is to land this as one target then create a bug to split > each provider in its own target? Yup, I think this makes sense. If Olivier objects, then we'd have to do the splitting up first and then rebase this CL.
https://codereview.chromium.org/2755113002/diff/120001/components/ntp_snippet... File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc (right): https://codereview.chromium.org/2755113002/diff/120001/components/ntp_snippet... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:72: LOG(DFATAL) << "ReadingListSuggestionsProvider has no |Fetch| functionality!"; DFATAL smells a bit like trying to handle a DCHECK failure, which is not allowed, per https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md.... If this method is genuinely not supposed to be called, just put NOTREACHED() here and no production code.
https://codereview.chromium.org/2755113002/diff/120001/components/ntp_snippet... File components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc (right): https://codereview.chromium.org/2755113002/diff/120001/components/ntp_snippet... components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc:72: LOG(DFATAL) << "ReadingListSuggestionsProvider has no |Fetch| functionality!"; On 2017/03/27 15:15:02, Bernhard (OOO until Mar 27) wrote: > DFATAL smells a bit like trying to handle a DCHECK failure, which is not > allowed, per > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md.... > If this method is genuinely not supposed to be called, just put NOTREACHED() > here and no production code. This is a pattern we have in all the providers - the intention was that we'd notice if this ever happened, but we still don't crash prod binaries. The "contract" that this is never called is quite far away, it essentially depends on the UI doing the right thing. All this clearly needs a redesign, but it's not a problem with this CL.
lgtm https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... File components/ntp_snippets/ios/BUILD.gn (right): https://codereview.chromium.org/2755113002/diff/1/components/ntp_snippets/ios... components/ntp_snippets/ios/BUILD.gn:1: # Copyright 2017 The Chromium Authors. All rights reserved. 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.
Thanks!
The CQ bit was checked by gambard@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, jkrcal@chromium.org Link to the patchset: https://codereview.chromium.org/2755113002/#ps120001 (title: "Fix strings")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by gambard@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gambard@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490687522888080, "parent_rev": "c454fad8104929e2624e221aeb2f7a27e9844671", "commit_rev": "c9bff741d84b56d8f5963a398958fe85cc110fae"}
Message was sent while issue was closed.
Description was changed from ========== Create ReadingListSuggestionsProvider This CL creates the ReadingListSuggestionsProvider. BUG=702241 ========== to ========== Create ReadingListSuggestionsProvider This CL creates the ReadingListSuggestionsProvider. BUG=702241 Review-Url: https://codereview.chromium.org/2755113002 Cr-Commit-Position: refs/heads/master@{#460039} Committed: https://chromium.googlesource.com/chromium/src/+/c9bff741d84b56d8f5963a398958... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/c9bff741d84b56d8f5963a398958... |