Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2228553003:
a provider of Physical Web pages. (Closed)
|
Created:
4 years, 4 months ago by vitaliii Modified:
4 years, 4 months ago CC:
chromium-reviews, ntp-dev+reviews_chromium.org, droger+watchlist_chromium.org, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, arv+watch_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiona provider of Physical Web pages.
BUG=635893
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/762478d1961b46576d67263bf766d7c391f8514d
Cr-Commit-Position: refs/heads/master@{#412484}
Patch Set 1 #
Total comments: 28
Patch Set 2 : added unittest #
Total comments: 48
Patch Set 3 : fix #
Total comments: 8
Patch Set 4 : nits. #
Total comments: 1
Patch Set 5 : rewrote comment. #Patch Set 6 : git cl format #
Total comments: 12
Patch Set 7 : Bernhard's comments. #Patch Set 8 : Bernhard's comments. #Patch Set 9 : rebase + autoformat #Patch Set 10 : resolved compile error. #Dependent Patchsets: Messages
Total messages: 57 (22 generated)
Description was changed from ========== a provider of Physical Web pages. BUG= ========== to ========== a provider of Physical Web pages. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
vitaliii@chromium.org changed reviewers: + tschumann@chromium.org
treib@chromium.org changed reviewers: + treib@chromium.org
Some drive-by comments :) You should also file a bug and add the number to the description here. https://codereview.chromium.org/2228553003/diff/1/chrome/browser/android/chro... File chrome/browser/android/chrome_feature_list.h (right): https://codereview.chromium.org/2228553003/diff/1/chrome/browser/android/chro... chrome/browser/android/chrome_feature_list.h:22: extern const base::Feature kNTPPhysicalWebPageSuggestionsFeature; This should move to components/ntp_snippets/features.h/cc I'm in the process of moving all our features there, see https://codereview.chromium.org/2221123003/ https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets.gypi File components/ntp_snippets.gypi (right): https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets.gyp... components/ntp_snippets.gypi:61: 'ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h', Random info: As of last week, we don't have to update .gyp files anymore. So feel free to just ignore them next time :) https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category.h (right): https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category.h:21: PHYSICAL_WEB_PAGES, This needs a rebase, there's a BOOKMARKS entry now https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:85: ++i) { range-based for ftw! for (const UrlInfo& url_info : urls) https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h (right): https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:56: // UrlManager::Listener implementation No, it isn't :) https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:57: void onDisplayableUrlsChanged(const std::vector<UrlInfo>& urls); OnDisplayableUrlsChanged (capitalize)
Ah, also: You have a few lines > 80 chars. Try "git cl format" to fix them automatically.
Description was changed from ========== a provider of Physical Web pages. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== a provider of Physical Web pages. BUG=635893 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
a test for the new provider and the category factory change would be good :-) Marc, any idea how to test the code we added to the other factories? Also, Marc, are you fine with submitting hard-coded test data. I'm sort of in-between... Oh, and then I saw Marc's command to fix the lines > 80 cols; might be easier. But you should definitely set up clang-format -- makes your life a lot easier :-) https://codereview.chromium.org/2228553003/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2228553003/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:51: using ntp_snippets::PhysicalWebPageSuggestionsProvider; please sort in alphabetically (after ntp_snippets::OfflinePageSuggestionsProvider). https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:50: std::vector<Category> PhysicalWebPageSuggestionsProvider::GetProvidedCategories() { line too long. clang-format (go/clang-format) suggests: std::vector<Category> PhysicalWebPageSuggestionsProvider::GetProvidedCategories() { ... } https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:75: void PhysicalWebPageSuggestionsProvider::ClearDismissedSuggestionsForDebugging() { line > 80 columns. clangformat (go/clangformat) suggests void PhysicalWebPageSuggestionsProvider:: ClearDismissedSuggestionsForDebugging() { ... } https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:99: std::move(suggestions)); indentation. https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:107: drop the blank line? In general, i'd try to minimize vertical space. In some cases it makes sense to group statements, but this method is so short, there's no need for further structure. https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h (right): https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:25: class UrlInfo { nit: if all members are public and the class doesn't contain any functionality, you should make it a struct. https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:59: // Updates the |category_status_| and notifies the |observer_|, if necessary. private methods are implementation details and only exposed in the header for technical reasons. No need to document those in the header -- better put such comments inside the .cc file at the method's implementation (however, if the code is simple, the comment won't likely be needed). https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:63: you can drop the blank line between the two variables.
On 2016/08/09 15:24:59, tschumann wrote: > a test for the new provider and the category factory change would be good :-) > > Marc, any idea how to test the code we added to the other factories? Not sure I understand the question.. what "other factories"? > Also, Marc, are you fine with submitting hard-coded test data. I'm sort of > in-between... If we have an actual, short-term plan for getting rid of it, then IMO it's fine - i.e. a measure to keep CLs small. If it stays there for more than a week or two, then we're probably doing something wrong. In this case, I don't know how concrete and short-term the plans are. Maybe it's better to just return empty results for now? > Oh, and then I saw Marc's command to fix the lines > 80 cols; might be easier. > But you should definitely set up clang-format -- makes your life a lot easier > :-) "git cl format" is actually based on clang-format :)
On 2016/08/09 15:40:48, Marc Treib wrote: > On 2016/08/09 15:24:59, tschumann wrote: > > a test for the new provider and the category factory change would be good :-) > > > > Marc, any idea how to test the code we added to the other factories? > > Not sure I understand the question.. what "other factories"? ok, it's singular: content_suggestions_service_factory.cc Do we have any idea how to test things here? Would be nice to have at least some easy coverage... (and ifdefs make me feel uncomfortable ;-)). > > > Also, Marc, are you fine with submitting hard-coded test data. I'm sort of > > in-between... > > If we have an actual, short-term plan for getting rid of it, then IMO it's fine > - i.e. a measure to keep CLs small. If it stays there for more than a week or > two, then we're probably doing something wrong. > In this case, I don't know how concrete and short-term the plans are. Maybe it's > better to just return empty results for now? > > > Oh, and then I saw Marc's command to fix the lines > 80 cols; might be easier. > > But you should definitely set up clang-format -- makes your life a lot easier > > :-) > > "git cl format" is actually based on clang-format :) ;-) I know, but you should also integrate it into your editor (e.g. when I hit CTRL-K the current line gets indented). Not having to think about indentation makes coding quite a bit faster and lets you focus on the nice parts ;-)
On 2016/08/09 15:52:24, tschumann wrote: > On 2016/08/09 15:40:48, Marc Treib wrote: > > On 2016/08/09 15:24:59, tschumann wrote: > > > a test for the new provider and the category factory change would be good > :-) > > > > > > Marc, any idea how to test the code we added to the other factories? > > > > Not sure I understand the question.. what "other factories"? > ok, it's singular: content_suggestions_service_factory.cc > Do we have any idea how to test things here? > Would be nice to have at least some easy coverage... (and ifdefs make me feel > uncomfortable ;-)). Well, what exactly would the test test? That the Provider isn't instantiated if the Feature is disabled? IMO that's not really worth it... so little logic that's tested, vs so much stubbing out of dependencies, since collecting dependencies is pretty much all the factory does. I suppose we could create some browser_tests, if you really insist :) > > > > > Also, Marc, are you fine with submitting hard-coded test data. I'm sort of > > > in-between... > > > > If we have an actual, short-term plan for getting rid of it, then IMO it's > fine > > - i.e. a measure to keep CLs small. If it stays there for more than a week or > > two, then we're probably doing something wrong. > > In this case, I don't know how concrete and short-term the plans are. Maybe > it's > > better to just return empty results for now? > > > > > Oh, and then I saw Marc's command to fix the lines > 80 cols; might be > easier. > > > But you should definitely set up clang-format -- makes your life a lot > easier > > > :-) > > > > "git cl format" is actually based on clang-format :) > > ;-) I know, but you should also integrate it into your editor (e.g. when I hit > CTRL-K the current line gets indented). Not having to think about indentation > makes coding quite a bit faster and lets you focus on the nice parts ;-)
added a unittest. https://codereview.chromium.org/2228553003/diff/1/chrome/browser/android/chro... File chrome/browser/android/chrome_feature_list.h (right): https://codereview.chromium.org/2228553003/diff/1/chrome/browser/android/chro... chrome/browser/android/chrome_feature_list.h:22: extern const base::Feature kNTPPhysicalWebPageSuggestionsFeature; On 2016/08/09 12:45:17, Marc Treib wrote: > This should move to components/ntp_snippets/features.h/cc > > I'm in the process of moving all our features there, see > https://codereview.chromium.org/2221123003/ Done. https://codereview.chromium.org/2228553003/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2228553003/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:51: using ntp_snippets::PhysicalWebPageSuggestionsProvider; On 2016/08/09 15:24:59, tschumann wrote: > please sort in alphabetically (after > ntp_snippets::OfflinePageSuggestionsProvider). Done. https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets.gypi File components/ntp_snippets.gypi (right): https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets.gyp... components/ntp_snippets.gypi:61: 'ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h', On 2016/08/09 12:45:17, Marc Treib wrote: > Random info: As of last week, we don't have to update .gyp files anymore. So > feel free to just ignore them next time :) Acknowledged. https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category.h (right): https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category.h:21: PHYSICAL_WEB_PAGES, On 2016/08/09 12:45:17, Marc Treib wrote: > This needs a rebase, there's a BOOKMARKS entry now Done. https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:50: std::vector<Category> PhysicalWebPageSuggestionsProvider::GetProvidedCategories() { On 2016/08/09 15:24:59, tschumann wrote: > line too long. clang-format (go/clang-format) suggests: > > std::vector<Category> > PhysicalWebPageSuggestionsProvider::GetProvidedCategories() { > ... > } Done. https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:75: void PhysicalWebPageSuggestionsProvider::ClearDismissedSuggestionsForDebugging() { On 2016/08/09 15:24:59, tschumann wrote: > line > 80 columns. > clangformat (go/clangformat) suggests > > > void PhysicalWebPageSuggestionsProvider:: > ClearDismissedSuggestionsForDebugging() { > ... > } Done. https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:85: ++i) { On 2016/08/09 12:45:17, Marc Treib wrote: > range-based for ftw! > for (const UrlInfo& url_info : urls) Done. https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:99: std::move(suggestions)); On 2016/08/09 15:24:59, tschumann wrote: > indentation. Done. https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:107: On 2016/08/09 15:24:59, tschumann wrote: > drop the blank line? In general, i'd try to minimize vertical space. In some > cases it makes sense to group statements, but this method is so short, there's > no need for further structure. Done. https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h (right): https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:25: class UrlInfo { On 2016/08/09 15:24:59, tschumann wrote: > nit: if all members are public and the class doesn't contain any functionality, > you should make it a struct. Done. https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:56: // UrlManager::Listener implementation On 2016/08/09 12:45:17, Marc Treib wrote: > No, it isn't :) Do you mean that PhysicalWebPageSuggestionsProvider is not inherited from UrlManager::Listener? If yes, there is nothing to inherit from, since C++ interface is not provided yet. Or did you mean that it was private? https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:57: void onDisplayableUrlsChanged(const std::vector<UrlInfo>& urls); On 2016/08/09 12:45:17, Marc Treib wrote: > OnDisplayableUrlsChanged (capitalize) Done. https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:59: // Updates the |category_status_| and notifies the |observer_|, if necessary. On 2016/08/09 15:24:59, tschumann wrote: > private methods are implementation details and only exposed in the header for > technical reasons. No need to document those in the header -- better put such > comments inside the .cc file at the method's implementation (however, if the > code is simple, the comment won't likely be needed). Done. https://codereview.chromium.org/2228553003/diff/1/components/ntp_snippets/phy... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:63: On 2016/08/09 15:24:59, tschumann wrote: > you can drop the blank line between the two variables. Done.
https://codereview.chromium.org/2228553003/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2228553003/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:153: #if defined(OS_ANDROID) This block is in the wrong place: It's inside the kArticleSuggestionsFeature check. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... File components/ntp_snippets/category_factory.cc (right): https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/category_factory.cc:17: AddKnownCategory(KnownCategories::PHYSICAL_WEB_PAGES); Have we decided on an order here? https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:42: for (const auto& url_info: urls) { I'd use the actual type (UrlInfo) here instead of auto https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:43: if (static_cast<int>(suggestions.size()) >= kMaxSuggestionsCount) { You could make kMaxSuggestionsCount a size_t and avoid the cast? Also, shouldn't this go at the end of the loop, after you added a suggestion? https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:92: // Ignored. This is really "not implemented" or "not supported" rather than "ignored". https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h (right): https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:25: struct UrlInfo { Does this need to be public? I think you could make it a private member of the class, and actually move the definition into the .cc file (i.e. only have "struct UrlInfo;" in the header) https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:48: void OnDisplayableUrlsChanged(const std::vector<UrlInfo>& urls); This can probably be private? Also it's not an implementation, since this doesn't implement any UrlManager::Listener interface. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:42: OnNewSuggestionsUrls(urls); You could make the mock method take a "const std::vector<ContentSuggestion>&", then you don't have to copy stuff. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:57: void TearDown() override {} No need to implement SetUp and TearDown if they don't do anything. In fact, if the test class doesn't do anything, you can just remove it (and use TEST() instead of TEST_F()). If more tests are coming soon, it might make sense to move e.g. the observer into the class though. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:67: PhysicalWebPageSuggestionsProvider provider(ob, &category_factory); Just do &observer here and get rid of the ob variable? https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:76: } // namespace Only the global functions should be in the anonymous namespace
https://codereview.chromium.org/2228553003/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2228553003/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:153: #if defined(OS_ANDROID) On 2016/08/10 16:22:03, Marc Treib wrote: > This block is in the wrong place: It's inside the kArticleSuggestionsFeature > check. not really related to this CL, but as we are here... @Marc: I wonder if we should refactor this method to make it harder getting this wrong. Something like: if (base::FeatureList::IsEnabled(ntp_snippets::kBookmarkSuggestionsFeature)) { RegisterBookmarksProvider(service); } if (base::FeatureList::IsEnabled(ntp_snippets::ntp_snippets::kArticleSuggestionsFeature) { RegisterArticleProvider(service); } etc. With more providers to come, this might get a bit crowded here... https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... File components/ntp_snippets/category_factory.cc (right): https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/category_factory.cc:17: AddKnownCategory(KnownCategories::PHYSICAL_WEB_PAGES); On 2016/08/10 16:22:03, Marc Treib wrote: > Have we decided on an order here? hehe... we should clarify the comment above and make clear that this will be the default sorting order for local categories. That said, I guess adding it to the end of the local categories should be fine. We might want to double-check with Patrick, though. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:43: if (static_cast<int>(suggestions.size()) >= kMaxSuggestionsCount) { On 2016/08/10 16:22:03, Marc Treib wrote: > You could make kMaxSuggestionsCount a size_t and avoid the cast? > Also, shouldn't this go at the end of the loop, after you added a suggestion? I'd probably leave the check at the beginning as you keep the 2 loop conditions close together. For the cast, I'd probably go as far to make kMaxSuggestionsCount a size_t. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:34: class MockProviderObserver : public ContentSuggestionsProvider::Observer { Marc, do we already have such an observer for other tests? Might make sense to move this into a mock_content_suggestions_observer.h library or the like. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:42: OnNewSuggestionsUrls(urls); On 2016/08/10 16:22:03, Marc Treib wrote: > You could make the mock method take a "const std::vector<ContentSuggestion>&", > then you don't have to copy stuff. This seems odd -- you're adding a mock function (OnNewSuggestionsUrls()) that isn't on the base class? Unless I'm mistaken something that's sort-of abusing the mock system and might actually break in the future (actually I thought MOCK_METHOD would also add an override to the signature, but apparently not). Anyways, what you really want is a better matcher that can verify the suggestions you got have a certain URL. You can do this with something like this: MATCHER_P(HasUrl, url, "") { *result_listener << "expected URL: " << url << "has URL: " << arg.url().spec(); return arg.url().spec() == url; } EXPECT_CALL(observer, OnNewSuggestions(::testing::UnorderedElementsAre(HasUrl("http://test1.com/"), HasUrl("http://test2.com/")); (module formatting ;-)). (for more details see the gMock docs, https://wiki.corp.google.com/twiki/bin/view/Main/GMockGuide#MatcherList) https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:63: TEST_F(PhysicalWebPageSuggestionsProviderTest, ShouldCreateSuggestions) { If you don't use anything of the test fixture (==the class class PhysicalWebPageSuggestionsProviderTest), then you don't have to use the TEST_F() macro but can use TEST() instead. Simply remove the fixture and write: TEST(PhysicalWebPageSuggestionsProviderTest, ShouldCreateSuggestions) { https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:76: } // namespace On 2016/08/10 16:22:03, Marc Treib wrote: > Only the global functions should be in the anonymous namespace actually, i think there's a tendency to have all tests in the unnamed namespace (https://yaqs.googleplex.com/eng/q/5783337608151040). Unless you need to make use of friend tests. I'm usually not feeling strong enough to argue, but if we had to settle on something, I'd be in favor of putting it all into an unnamed namespace.
https://codereview.chromium.org/2228553003/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2228553003/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:153: #if defined(OS_ANDROID) On 2016/08/10 17:37:57, tschumann wrote: > On 2016/08/10 16:22:03, Marc Treib wrote: > > This block is in the wrong place: It's inside the kArticleSuggestionsFeature > > check. > > not really related to this CL, but as we are here... > @Marc: I wonder if we should refactor this method to make it harder getting this > wrong. Something like: > if (base::FeatureList::IsEnabled(ntp_snippets::kBookmarkSuggestionsFeature)) { > RegisterBookmarksProvider(service); > } > if > (base::FeatureList::IsEnabled(ntp_snippets::ntp_snippets::kArticleSuggestionsFeature) > { > RegisterArticleProvider(service); > } > > etc. With more providers to come, this might get a bit crowded here... Yep, this sounds like a good idea. Doesn't need to happen in this CL though. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:42: OnNewSuggestionsUrls(urls); On 2016/08/10 17:37:57, tschumann wrote: > On 2016/08/10 16:22:03, Marc Treib wrote: > > You could make the mock method take a "const std::vector<ContentSuggestion>&", > > then you don't have to copy stuff. > > This seems odd -- you're adding a mock function (OnNewSuggestionsUrls()) that > isn't on the base class? > > Unless I'm mistaken something that's sort-of abusing the mock system and might > actually break in the future (actually I thought MOCK_METHOD would also add an > override to the signature, but apparently not). Anyways, what you really want is > a better matcher that can verify the suggestions you got have a certain URL. You > can do this with something like this: > > MATCHER_P(HasUrl, url, "") { > *result_listener << "expected URL: " << url > << "has URL: " << arg.url().spec(); > return arg.url().spec() == url; > } > > EXPECT_CALL(observer, > OnNewSuggestions(::testing::UnorderedElementsAre(HasUrl("http://test1.com/"), > HasUrl("http://test2.com/")); > > (module formatting ;-)). > > (for more details see the gMock docs, > https://wiki.corp.google.com/twiki/bin/view/Main/GMockGuide#MatcherList) This is a workaround for gMock's lack of support for movable types. Ideally, we'd just like to mock the OnNewSuggestions method directly and then use proper matchers in the test, but that doesn't compile. The common workaround is to override the method manually, and pass the argument as a const ref to a different (non-overriding) mock method. Hardly optimal, but this is what we're working with. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:76: } // namespace On 2016/08/10 17:37:57, tschumann wrote: > On 2016/08/10 16:22:03, Marc Treib wrote: > > Only the global functions should be in the anonymous namespace > > actually, i think there's a tendency to have all tests in the unnamed namespace > (https://yaqs.googleplex.com/eng/q/5783337608151040). Unless you need to make > use of friend tests. > > I'm usually not feeling strong enough to argue, but if we had to settle on > something, I'd be in favor of putting it all into an unnamed namespace. This is not a common tendency in Chrome I think, but alright. I think it won't work though if we need to make the test a friend of the tested class? So if we go for "everything in the anon namespace", then there'd always be exceptions.
https://codereview.chromium.org/2228553003/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2228553003/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:153: #if defined(OS_ANDROID) On 2016/08/10 16:22:03, Marc Treib wrote: > This block is in the wrong place: It's inside the kArticleSuggestionsFeature > check. Done. https://codereview.chromium.org/2228553003/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:153: #if defined(OS_ANDROID) On 2016/08/10 17:37:57, tschumann wrote: > On 2016/08/10 16:22:03, Marc Treib wrote: > > This block is in the wrong place: It's inside the kArticleSuggestionsFeature > > check. > > not really related to this CL, but as we are here... > @Marc: I wonder if we should refactor this method to make it harder getting this > wrong. Something like: > if (base::FeatureList::IsEnabled(ntp_snippets::kBookmarkSuggestionsFeature)) { > RegisterBookmarksProvider(service); > } > if > (base::FeatureList::IsEnabled(ntp_snippets::ntp_snippets::kArticleSuggestionsFeature) > { > RegisterArticleProvider(service); > } > > etc. With more providers to come, this might get a bit crowded here... Acknowledged. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... File components/ntp_snippets/category_factory.cc (right): https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/category_factory.cc:17: AddKnownCategory(KnownCategories::PHYSICAL_WEB_PAGES); On 2016/08/10 17:37:57, tschumann wrote: > On 2016/08/10 16:22:03, Marc Treib wrote: > > Have we decided on an order here? > > hehe... we should clarify the comment above and make clear that this will be the > default sorting order for local categories. That said, I guess adding it to the > end of the local categories should be fine. We might want to double-check with > Patrick, though. If I understand right and this order defines the order of suggestions, then go/zine-prd requires additional functionality, since "Nearby suggestions should get a boost while the user is close to the suggestion as much as Offline suggestions should get a boost when the user is offline.". https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:42: for (const auto& url_info: urls) { On 2016/08/10 16:22:03, Marc Treib wrote: > I'd use the actual type (UrlInfo) here instead of auto Done. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:43: if (static_cast<int>(suggestions.size()) >= kMaxSuggestionsCount) { On 2016/08/10 17:37:57, tschumann wrote: > On 2016/08/10 16:22:03, Marc Treib wrote: > > You could make kMaxSuggestionsCount a size_t and avoid the cast? > > Also, shouldn't this go at the end of the loop, after you added a suggestion? > > I'd probably leave the check at the beginning as you keep the 2 loop conditions > close together. > For the cast, I'd probably go as far to make kMaxSuggestionsCount a size_t. The check at the beginning handles kMaxSuggestionsCount = 0 case correctly. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:92: // Ignored. On 2016/08/10 16:22:03, Marc Treib wrote: > This is really "not implemented" or "not supported" rather than "ignored". Done. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h (right): https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:25: struct UrlInfo { On 2016/08/10 16:22:03, Marc Treib wrote: > Does this need to be public? I think you could make it a private member of the > class, and actually move the definition into the .cc file (i.e. only have > "struct UrlInfo;" in the header) Since there is no Physical Web C++ interface yet, this is just a guess how the data could be organized, so that I could write something in the Provider. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:48: void OnDisplayableUrlsChanged(const std::vector<UrlInfo>& urls); On 2016/08/10 16:22:03, Marc Treib wrote: > This can probably be private? Also it's not an implementation, since this > doesn't implement any UrlManager::Listener interface. This function will be used by Physical Web team to provide us the data, therefore, it is public. However, there is no C++ interface yet, so no one is using it now. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:34: class MockProviderObserver : public ContentSuggestionsProvider::Observer { On 2016/08/10 17:37:57, tschumann wrote: > Marc, do we already have such an observer for other tests? Might make sense to > move this into a mock_content_suggestions_observer.h library or the like. Done. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:42: OnNewSuggestionsUrls(urls); On 2016/08/10 17:37:57, tschumann wrote: > On 2016/08/10 16:22:03, Marc Treib wrote: > > You could make the mock method take a "const std::vector<ContentSuggestion>&", > > then you don't have to copy stuff. > > This seems odd -- you're adding a mock function (OnNewSuggestionsUrls()) that > isn't on the base class? > > Unless I'm mistaken something that's sort-of abusing the mock system and might > actually break in the future (actually I thought MOCK_METHOD would also add an > override to the signature, but apparently not). Anyways, what you really want is > a better matcher that can verify the suggestions you got have a certain URL. You > can do this with something like this: > > MATCHER_P(HasUrl, url, "") { > *result_listener << "expected URL: " << url > << "has URL: " << arg.url().spec(); > return arg.url().spec() == url; > } > > EXPECT_CALL(observer, > OnNewSuggestions(::testing::UnorderedElementsAre(HasUrl("http://test1.com/"), > HasUrl("http://test2.com/")); > > (module formatting ;-)). > > (for more details see the gMock docs, > https://wiki.corp.google.com/twiki/bin/view/Main/GMockGuide#MatcherList) Since ContentSuggestion cannot be copied and assigned, MOCK_METHOD3(OnNewSuggestions, (omited)) does not compile. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:57: void TearDown() override {} On 2016/08/10 16:22:03, Marc Treib wrote: > No need to implement SetUp and TearDown if they don't do anything. > > In fact, if the test class doesn't do anything, you can just remove it (and use > TEST() instead of TEST_F()). If more tests are coming soon, it might make sense > to move e.g. the observer into the class though. Done. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:63: TEST_F(PhysicalWebPageSuggestionsProviderTest, ShouldCreateSuggestions) { On 2016/08/10 17:37:57, tschumann wrote: > If you don't use anything of the test fixture (==the class class > PhysicalWebPageSuggestionsProviderTest), then you don't have to use the TEST_F() > macro but can use TEST() instead. > > Simply remove the fixture and write: > TEST(PhysicalWebPageSuggestionsProviderTest, ShouldCreateSuggestions) { Done. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:67: PhysicalWebPageSuggestionsProvider provider(ob, &category_factory); On 2016/08/10 16:22:03, Marc Treib wrote: > Just do &observer here and get rid of the ob variable? Done. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:76: } // namespace On 2016/08/10 17:37:57, tschumann wrote: > On 2016/08/10 16:22:03, Marc Treib wrote: > > Only the global functions should be in the anonymous namespace > > actually, i think there's a tendency to have all tests in the unnamed namespace > (https://yaqs.googleplex.com/eng/q/5783337608151040). Unless you need to make > use of friend tests. > > I'm usually not feeling strong enough to argue, but if we had to settle on > something, I'd be in favor of putting it all into an unnamed namespace. Other files in the folder (e.g. https://cs.chromium.org/chromium/src/components/ntp_snippets/ntp_snippets_dat...) do not use an unnamed namespace, so I removed this one too.
https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:42: OnNewSuggestionsUrls(urls); On 2016/08/11 12:15:25, vitaliii wrote: > On 2016/08/10 17:37:57, tschumann wrote: > > On 2016/08/10 16:22:03, Marc Treib wrote: > > > You could make the mock method take a "const > std::vector<ContentSuggestion>&", > > > then you don't have to copy stuff. > > > > This seems odd -- you're adding a mock function (OnNewSuggestionsUrls()) that > > isn't on the base class? > > > > Unless I'm mistaken something that's sort-of abusing the mock system and might > > actually break in the future (actually I thought MOCK_METHOD would also add an > > override to the signature, but apparently not). Anyways, what you really want > is > > a better matcher that can verify the suggestions you got have a certain URL. > You > > can do this with something like this: > > > > MATCHER_P(HasUrl, url, "") { > > *result_listener << "expected URL: " << url > > << "has URL: " << arg.url().spec(); > > return arg.url().spec() == url; > > } > > > > EXPECT_CALL(observer, > > OnNewSuggestions(::testing::UnorderedElementsAre(HasUrl("http://test1.com/"), > > HasUrl("http://test2.com/")); > > > > (module formatting ;-)). > > > > (for more details see the gMock docs, > > https://wiki.corp.google.com/twiki/bin/view/Main/GMockGuide#MatcherList) > > Since ContentSuggestion cannot be copied and assigned, > MOCK_METHOD3(OnNewSuggestions, (omited)) does not compile. yuck... I start wondering if it's really worth having ContentSuggestion not being copy-able -- that's actually the reason why we're not passing this value by const reference. As it is, I'm opposed as tests get super confusing:inside the TEST() you set up expectations about calling methods that don't exist... However, what should work and might hide this ugliness inside the Mock would be to add an overload that behaves the same way, e.g.: void OnNewSuggestions(ContentSuggestionsProvider* provider, Category category, std::vector<ContentSuggestion> suggestions) override { std::list<ContentSuggestion> suggestions_list; for (auto& suggestion: suggestion) { suggestions_list.emplace_back(std::move(suggestion)); } OnNewSuggestions(provider, category, suggestions_list); } MOCK_METHOD3(OnNewSuggestions, void(ContentSuggestionsProvider* provider, Category category, const std::list<ContentSuggestion>& suggestions)); We should also add some comments explaining this hack and why it's needed. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:76: } // namespace On 2016/08/11 12:15:24, Marc Treib wrote: > On 2016/08/10 17:37:57, tschumann wrote: > > On 2016/08/10 16:22:03, Marc Treib wrote: > > > Only the global functions should be in the anonymous namespace > > > > actually, i think there's a tendency to have all tests in the unnamed > namespace > > (https://yaqs.googleplex.com/eng/q/5783337608151040). Unless you need to make > > use of friend tests. > > > > I'm usually not feeling strong enough to argue, but if we had to settle on > > something, I'd be in favor of putting it all into an unnamed namespace. > > This is not a common tendency in Chrome I think, but alright. > I think it won't work though if we need to make the test a friend of the tested > class? So if we go for "everything in the anon namespace", then there'd always > be exceptions. Consistency trumps; so let's remove it. [however, i'd be fine with exceptions for friend tests as they should be exceptional ;-)].
LGTM with some more nits. Nice! https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... File components/ntp_snippets/category_factory.cc (right): https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/category_factory.cc:17: AddKnownCategory(KnownCategories::PHYSICAL_WEB_PAGES); On 2016/08/11 12:15:24, vitaliii wrote: > On 2016/08/10 17:37:57, tschumann wrote: > > On 2016/08/10 16:22:03, Marc Treib wrote: > > > Have we decided on an order here? > > > > hehe... we should clarify the comment above and make clear that this will be > the > > default sorting order for local categories. That said, I guess adding it to > the > > end of the local categories should be fine. We might want to double-check with > > Patrick, though. > > If I understand right and this order defines the order of suggestions, then > go/zine-prd requires additional functionality, since "Nearby suggestions should > get a boost while the user is close to the suggestion as much as Offline > suggestions should get a boost when the user is offline.". That sounds reasonable, but for M54 we're going with a fixed ordering. Any boosting isn't implemented yet. Anyway, since this won't go live in M54, it doesn't matter right now :) https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:43: if (static_cast<int>(suggestions.size()) >= kMaxSuggestionsCount) { On 2016/08/11 12:15:24, vitaliii wrote: > On 2016/08/10 17:37:57, tschumann wrote: > > On 2016/08/10 16:22:03, Marc Treib wrote: > > > You could make kMaxSuggestionsCount a size_t and avoid the cast? > > > Also, shouldn't this go at the end of the loop, after you added a > suggestion? > > > > I'd probably leave the check at the beginning as you keep the 2 loop > conditions > > close together. > > For the cast, I'd probably go as far to make kMaxSuggestionsCount a size_t. > > The check at the beginning handles kMaxSuggestionsCount = 0 case correctly. ...which should never actually happen. But okay, fair enough. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h (right): https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:25: struct UrlInfo { On 2016/08/11 12:15:25, vitaliii wrote: > On 2016/08/10 16:22:03, Marc Treib wrote: > > Does this need to be public? I think you could make it a private member of the > > class, and actually move the definition into the .cc file (i.e. only have > > "struct UrlInfo;" in the header) > > Since there is no Physical Web C++ interface yet, this is just a guess how the > data could be organized, so that I could write something in the Provider. Alright, fair enough. Still, even if this is just a dummy: Can we remove all fields that aren't used by our code yet? https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:48: void OnDisplayableUrlsChanged(const std::vector<UrlInfo>& urls); On 2016/08/11 12:15:24, vitaliii wrote: > On 2016/08/10 16:22:03, Marc Treib wrote: > > This can probably be private? Also it's not an implementation, since this > > doesn't implement any UrlManager::Listener interface. > > This function will be used by Physical Web team to provide us the data, > therefore, it is public. However, there is no C++ interface yet, so no one is > using it now. If there was an observer that we'd implement, then the method could actually be private. If there won't be any such interface, then it needs to be public. Since I suppose we don't know yet, this is alright for now. https://codereview.chromium.org/2228553003/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2228553003/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:180: nit: remove extra empty line https://codereview.chromium.org/2228553003/diff/40001/components/ntp_snippets... File components/ntp_snippets/mock_content_suggestions_provider_observer.h (right): https://codereview.chromium.org/2228553003/diff/40001/components/ntp_snippets... components/ntp_snippets/mock_content_suggestions_provider_observer.h:21: void OnNewSuggestions(ContentSuggestionsProvider* provider, This pattern (manually overridden method + mock below) might be worth a comment. https://codereview.chromium.org/2228553003/diff/40001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h (right): https://codereview.chromium.org/2228553003/diff/40001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:36: ~UrlInfo(); nit: methods before fields https://codereview.chromium.org/2228553003/diff/40001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2228553003/diff/40001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:56: ::testing::UnorderedElementsAre( optional: You can add "using testing::UnorderedElementsAre;" (etc) above to make this kind of thing a bit easier to read.
https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:42: OnNewSuggestionsUrls(urls); On 2016/08/11 12:52:24, tschumann wrote: > On 2016/08/11 12:15:25, vitaliii wrote: > > On 2016/08/10 17:37:57, tschumann wrote: > > > On 2016/08/10 16:22:03, Marc Treib wrote: > > > > You could make the mock method take a "const > > std::vector<ContentSuggestion>&", > > > > then you don't have to copy stuff. > > > > > > This seems odd -- you're adding a mock function (OnNewSuggestionsUrls()) > that > > > isn't on the base class? > > > > > > Unless I'm mistaken something that's sort-of abusing the mock system and > might > > > actually break in the future (actually I thought MOCK_METHOD would also add > an > > > override to the signature, but apparently not). Anyways, what you really > want > > is > > > a better matcher that can verify the suggestions you got have a certain URL. > > You > > > can do this with something like this: > > > > > > MATCHER_P(HasUrl, url, "") { > > > *result_listener << "expected URL: " << url > > > << "has URL: " << arg.url().spec(); > > > return arg.url().spec() == url; > > > } > > > > > > EXPECT_CALL(observer, > > > > OnNewSuggestions(::testing::UnorderedElementsAre(HasUrl("http://test1.com/"), > > > HasUrl("http://test2.com/")); > > > > > > (module formatting ;-)). > > > > > > (for more details see the gMock docs, > > > https://wiki.corp.google.com/twiki/bin/view/Main/GMockGuide#MatcherList) > > > > Since ContentSuggestion cannot be copied and assigned, > > MOCK_METHOD3(OnNewSuggestions, (omited)) does not compile. > > yuck... I start wondering if it's really worth having ContentSuggestion not > being copy-able -- that's actually the reason why we're not passing this value > by const reference. No, that's not the only reason. This way, the ContentSuggestionsService doesn't have to copy all the suggestions it gets; it can just std::move them out of the vector it gets passed. Of course, you could argue that that's not worth the hassle, since the copying shouldn't be *that* expensive. > As it is, I'm opposed as tests get super confusing:inside the TEST() you set up > expectations about calling methods that don't exist... > > However, what should work and might hide this ugliness inside the Mock would be > to add an overload > that behaves the same way, e.g.: > void OnNewSuggestions(ContentSuggestionsProvider* provider, Category category, > std::vector<ContentSuggestion> suggestions) override { > std::list<ContentSuggestion> suggestions_list; > for (auto& suggestion: suggestion) { > suggestions_list.emplace_back(std::move(suggestion)); > } > OnNewSuggestions(provider, category, suggestions_list); > } > > MOCK_METHOD3(OnNewSuggestions, void(ContentSuggestionsProvider* provider, > Category category, > const std::list<ContentSuggestion>& > suggestions)); Wait, std::list so that it gets disambiguated from std::vector? Not sure I prefer that... > We should also add some comments explaining this hack and why it's needed. +1 (said the same thing :) ) https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:76: } // namespace On 2016/08/11 12:52:24, tschumann wrote: > On 2016/08/11 12:15:24, Marc Treib wrote: > > On 2016/08/10 17:37:57, tschumann wrote: > > > On 2016/08/10 16:22:03, Marc Treib wrote: > > > > Only the global functions should be in the anonymous namespace > > > > > > actually, i think there's a tendency to have all tests in the unnamed > > namespace > > > (https://yaqs.googleplex.com/eng/q/5783337608151040). Unless you need to > make > > > use of friend tests. > > > > > > I'm usually not feeling strong enough to argue, but if we had to settle on > > > something, I'd be in favor of putting it all into an unnamed namespace. > > > > This is not a common tendency in Chrome I think, but alright. > > I think it won't work though if we need to make the test a friend of the > tested > > class? So if we go for "everything in the anon namespace", then there'd always > > be exceptions. > > Consistency trumps; so let's remove it. [however, i'd be fine with exceptions > for friend tests as they should be exceptional ;-)]. They're somewhat common in unit tests. Personally, I prefer friend declarations to "ForTesting" methods or to "// public for testing".
https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:42: OnNewSuggestionsUrls(urls); On 2016/08/11 13:09:14, Marc Treib wrote: > On 2016/08/11 12:52:24, tschumann wrote: > > On 2016/08/11 12:15:25, vitaliii wrote: > > > On 2016/08/10 17:37:57, tschumann wrote: > > > > On 2016/08/10 16:22:03, Marc Treib wrote: > > > > > You could make the mock method take a "const > > > std::vector<ContentSuggestion>&", > > > > > then you don't have to copy stuff. > > > > > > > > This seems odd -- you're adding a mock function (OnNewSuggestionsUrls()) > > that > > > > isn't on the base class? > > > > > > > > Unless I'm mistaken something that's sort-of abusing the mock system and > > might > > > > actually break in the future (actually I thought MOCK_METHOD would also > add > > an > > > > override to the signature, but apparently not). Anyways, what you really > > want > > > is > > > > a better matcher that can verify the suggestions you got have a certain > URL. > > > You > > > > can do this with something like this: > > > > > > > > MATCHER_P(HasUrl, url, "") { > > > > *result_listener << "expected URL: " << url > > > > << "has URL: " << arg.url().spec(); > > > > return arg.url().spec() == url; > > > > } > > > > > > > > EXPECT_CALL(observer, > > > > > > OnNewSuggestions(::testing::UnorderedElementsAre(HasUrl("http://test1.com/"), > > > > HasUrl("http://test2.com/")); > > > > > > > > (module formatting ;-)). > > > > > > > > (for more details see the gMock docs, > > > > https://wiki.corp.google.com/twiki/bin/view/Main/GMockGuide#MatcherList) > > > > > > Since ContentSuggestion cannot be copied and assigned, > > > MOCK_METHOD3(OnNewSuggestions, (omited)) does not compile. > > > > yuck... I start wondering if it's really worth having ContentSuggestion not > > being copy-able -- that's actually the reason why we're not passing this value > > by const reference. > > No, that's not the only reason. This way, the ContentSuggestionsService doesn't > have to copy all the suggestions it gets; it can just std::move them out of the > vector it gets passed. > Of course, you could argue that that's not worth the hassle, since the copying > shouldn't be *that* expensive. > > > As it is, I'm opposed as tests get super confusing:inside the TEST() you set > up > > expectations about calling methods that don't exist... > > > > However, what should work and might hide this ugliness inside the Mock would > be > > to add an overload > > that behaves the same way, e.g.: > > void OnNewSuggestions(ContentSuggestionsProvider* provider, Category category, > > std::vector<ContentSuggestion> suggestions) override { > > std::list<ContentSuggestion> suggestions_list; > > for (auto& suggestion: suggestion) { > > suggestions_list.emplace_back(std::move(suggestion)); > > } > > OnNewSuggestions(provider, category, suggestions_list); > > } > > > > MOCK_METHOD3(OnNewSuggestions, void(ContentSuggestionsProvider* provider, > > Category category, > > const std::list<ContentSuggestion>& > > suggestions)); > > Wait, std::list so that it gets disambiguated from std::vector? Not sure I > prefer that... > > > We should also add some comments explaining this hack and why it's needed. > > +1 (said the same thing :) ) yes, we need to somehow allow the compiler to distinguish the methods, simply having an overload with const& doesn't work out. The nice property of the std::list is that from a test perspective, you don't see a difference (except maybe in the error message). If gmock get's fixed one day, we just need to clean up the mock itself and are done. My primary concern is that I can understand the test by just looking at the TEST() and the observer interface. wdyt?
https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:42: OnNewSuggestionsUrls(urls); On 2016/08/11 13:14:05, tschumann wrote: > On 2016/08/11 13:09:14, Marc Treib wrote: > > On 2016/08/11 12:52:24, tschumann wrote: > > > On 2016/08/11 12:15:25, vitaliii wrote: > > > > On 2016/08/10 17:37:57, tschumann wrote: > > > > > On 2016/08/10 16:22:03, Marc Treib wrote: > > > > > > You could make the mock method take a "const > > > > std::vector<ContentSuggestion>&", > > > > > > then you don't have to copy stuff. > > > > > > > > > > This seems odd -- you're adding a mock function (OnNewSuggestionsUrls()) > > > that > > > > > isn't on the base class? > > > > > > > > > > Unless I'm mistaken something that's sort-of abusing the mock system and > > > might > > > > > actually break in the future (actually I thought MOCK_METHOD would also > > add > > > an > > > > > override to the signature, but apparently not). Anyways, what you really > > > want > > > > is > > > > > a better matcher that can verify the suggestions you got have a certain > > URL. > > > > You > > > > > can do this with something like this: > > > > > > > > > > MATCHER_P(HasUrl, url, "") { > > > > > *result_listener << "expected URL: " << url > > > > > << "has URL: " << arg.url().spec(); > > > > > return arg.url().spec() == url; > > > > > } > > > > > > > > > > EXPECT_CALL(observer, > > > > > > > > > OnNewSuggestions(::testing::UnorderedElementsAre(HasUrl("http://test1.com/"), > > > > > HasUrl("http://test2.com/")); > > > > > > > > > > (module formatting ;-)). > > > > > > > > > > (for more details see the gMock docs, > > > > > https://wiki.corp.google.com/twiki/bin/view/Main/GMockGuide#MatcherList) > > > > > > > > Since ContentSuggestion cannot be copied and assigned, > > > > MOCK_METHOD3(OnNewSuggestions, (omited)) does not compile. > > > > > > yuck... I start wondering if it's really worth having ContentSuggestion not > > > being copy-able -- that's actually the reason why we're not passing this > value > > > by const reference. > > > > No, that's not the only reason. This way, the ContentSuggestionsService > doesn't > > have to copy all the suggestions it gets; it can just std::move them out of > the > > vector it gets passed. > > Of course, you could argue that that's not worth the hassle, since the copying > > shouldn't be *that* expensive. > > > > > As it is, I'm opposed as tests get super confusing:inside the TEST() you set > > up > > > expectations about calling methods that don't exist... > > > > > > However, what should work and might hide this ugliness inside the Mock would > > be > > > to add an overload > > > that behaves the same way, e.g.: > > > void OnNewSuggestions(ContentSuggestionsProvider* provider, Category > category, > > > std::vector<ContentSuggestion> suggestions) override { > > > std::list<ContentSuggestion> suggestions_list; > > > for (auto& suggestion: suggestion) { > > > suggestions_list.emplace_back(std::move(suggestion)); > > > } > > > OnNewSuggestions(provider, category, suggestions_list); > > > } > > > > > > MOCK_METHOD3(OnNewSuggestions, void(ContentSuggestionsProvider* provider, > > > Category category, > > > const std::list<ContentSuggestion>& > > > suggestions)); > > > > Wait, std::list so that it gets disambiguated from std::vector? Not sure I > > prefer that... > > > > > We should also add some comments explaining this hack and why it's needed. > > > > +1 (said the same thing :) ) > > yes, we need to somehow allow the compiler to distinguish the methods, simply > having an overload with const& doesn't work out. The nice property of the > std::list is that from a test perspective, you don't see a difference (except > maybe in the error message). If gmock get's fixed one day, we just need to clean > up the mock itself and are done. > My primary concern is that I can understand the test by just looking at the > TEST() and the observer interface. > wdyt? Alright, makes sense. No objections :)
https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... File components/ntp_snippets/category_factory.cc (right): https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/category_factory.cc:17: AddKnownCategory(KnownCategories::PHYSICAL_WEB_PAGES); On 2016/08/11 12:53:09, Marc Treib wrote: > On 2016/08/11 12:15:24, vitaliii wrote: > > On 2016/08/10 17:37:57, tschumann wrote: > > > On 2016/08/10 16:22:03, Marc Treib wrote: > > > > Have we decided on an order here? > > > > > > hehe... we should clarify the comment above and make clear that this will be > > the > > > default sorting order for local categories. That said, I guess adding it to > > the > > > end of the local categories should be fine. We might want to double-check > with > > > Patrick, though. > > > > If I understand right and this order defines the order of suggestions, then > > go/zine-prd requires additional functionality, since "Nearby suggestions > should > > get a boost while the user is close to the suggestion as much as Offline > > suggestions should get a boost when the user is offline.". > > That sounds reasonable, but for M54 we're going with a fixed ordering. Any > boosting isn't implemented yet. > Anyway, since this won't go live in M54, it doesn't matter right now :) Acknowledged. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:43: if (static_cast<int>(suggestions.size()) >= kMaxSuggestionsCount) { On 2016/08/11 12:53:09, Marc Treib wrote: > On 2016/08/11 12:15:24, vitaliii wrote: > > On 2016/08/10 17:37:57, tschumann wrote: > > > On 2016/08/10 16:22:03, Marc Treib wrote: > > > > You could make kMaxSuggestionsCount a size_t and avoid the cast? > > > > Also, shouldn't this go at the end of the loop, after you added a > > suggestion? > > > > > > I'd probably leave the check at the beginning as you keep the 2 loop > > conditions > > > close together. > > > For the cast, I'd probably go as far to make kMaxSuggestionsCount a size_t. > > > > The check at the beginning handles kMaxSuggestionsCount = 0 case correctly. > > ...which should never actually happen. But okay, fair enough. Acknowledged. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h (right): https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:25: struct UrlInfo { On 2016/08/11 12:53:09, Marc Treib wrote: > On 2016/08/11 12:15:25, vitaliii wrote: > > On 2016/08/10 16:22:03, Marc Treib wrote: > > > Does this need to be public? I think you could make it a private member of > the > > > class, and actually move the definition into the .cc file (i.e. only have > > > "struct UrlInfo;" in the header) > > > > Since there is no Physical Web C++ interface yet, this is just a guess how the > > data could be organized, so that I could write something in the Provider. > > Alright, fair enough. > Still, even if this is just a dummy: Can we remove all fields that aren't used > by our code yet? Done. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:48: void OnDisplayableUrlsChanged(const std::vector<UrlInfo>& urls); On 2016/08/11 12:53:09, Marc Treib wrote: > On 2016/08/11 12:15:24, vitaliii wrote: > > On 2016/08/10 16:22:03, Marc Treib wrote: > > > This can probably be private? Also it's not an implementation, since this > > > doesn't implement any UrlManager::Listener interface. > > > > This function will be used by Physical Web team to provide us the data, > > therefore, it is public. However, there is no C++ interface yet, so no one is > > using it now. > > If there was an observer that we'd implement, then the method could actually be > private. If there won't be any such interface, then it needs to be public. Since > I suppose we don't know yet, this is alright for now. Acknowledged. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:42: OnNewSuggestionsUrls(urls); On 2016/08/11 13:17:53, Marc Treib wrote: > On 2016/08/11 13:14:05, tschumann wrote: > > On 2016/08/11 13:09:14, Marc Treib wrote: > > > On 2016/08/11 12:52:24, tschumann wrote: > > > > On 2016/08/11 12:15:25, vitaliii wrote: > > > > > On 2016/08/10 17:37:57, tschumann wrote: > > > > > > On 2016/08/10 16:22:03, Marc Treib wrote: > > > > > > > You could make the mock method take a "const > > > > > std::vector<ContentSuggestion>&", > > > > > > > then you don't have to copy stuff. > > > > > > > > > > > > This seems odd -- you're adding a mock function > (OnNewSuggestionsUrls()) > > > > that > > > > > > isn't on the base class? > > > > > > > > > > > > Unless I'm mistaken something that's sort-of abusing the mock system > and > > > > might > > > > > > actually break in the future (actually I thought MOCK_METHOD would > also > > > add > > > > an > > > > > > override to the signature, but apparently not). Anyways, what you > really > > > > want > > > > > is > > > > > > a better matcher that can verify the suggestions you got have a > certain > > > URL. > > > > > You > > > > > > can do this with something like this: > > > > > > > > > > > > MATCHER_P(HasUrl, url, "") { > > > > > > *result_listener << "expected URL: " << url > > > > > > << "has URL: " << arg.url().spec(); > > > > > > return arg.url().spec() == url; > > > > > > } > > > > > > > > > > > > EXPECT_CALL(observer, > > > > > > > > > > > > OnNewSuggestions(::testing::UnorderedElementsAre(HasUrl("http://test1.com/"), > > > > > > HasUrl("http://test2.com/")); > > > > > > > > > > > > (module formatting ;-)). > > > > > > > > > > > > (for more details see the gMock docs, > > > > > > > https://wiki.corp.google.com/twiki/bin/view/Main/GMockGuide#MatcherList) > > > > > > > > > > Since ContentSuggestion cannot be copied and assigned, > > > > > MOCK_METHOD3(OnNewSuggestions, (omited)) does not compile. > > > > > > > > yuck... I start wondering if it's really worth having ContentSuggestion > not > > > > being copy-able -- that's actually the reason why we're not passing this > > value > > > > by const reference. > > > > > > No, that's not the only reason. This way, the ContentSuggestionsService > > doesn't > > > have to copy all the suggestions it gets; it can just std::move them out of > > the > > > vector it gets passed. > > > Of course, you could argue that that's not worth the hassle, since the > copying > > > shouldn't be *that* expensive. > > > > > > > As it is, I'm opposed as tests get super confusing:inside the TEST() you > set > > > up > > > > expectations about calling methods that don't exist... > > > > > > > > However, what should work and might hide this ugliness inside the Mock > would > > > be > > > > to add an overload > > > > that behaves the same way, e.g.: > > > > void OnNewSuggestions(ContentSuggestionsProvider* provider, Category > > category, > > > > std::vector<ContentSuggestion> suggestions) override > { > > > > std::list<ContentSuggestion> suggestions_list; > > > > for (auto& suggestion: suggestion) { > > > > suggestions_list.emplace_back(std::move(suggestion)); > > > > } > > > > OnNewSuggestions(provider, category, suggestions_list); > > > > } > > > > > > > > MOCK_METHOD3(OnNewSuggestions, void(ContentSuggestionsProvider* provider, > > > > Category category, > > > > const std::list<ContentSuggestion>& > > > > suggestions)); > > > > > > Wait, std::list so that it gets disambiguated from std::vector? Not sure I > > > prefer that... > > > > > > > We should also add some comments explaining this hack and why it's needed. > > > > > > +1 (said the same thing :) ) > > > > yes, we need to somehow allow the compiler to distinguish the methods, simply > > having an overload with const& doesn't work out. The nice property of the > > std::list is that from a test perspective, you don't see a difference (except > > maybe in the error message). If gmock get's fixed one day, we just need to > clean > > up the mock itself and are done. > > My primary concern is that I can understand the test by just looking at the > > TEST() and the observer interface. > > wdyt? > > Alright, makes sense. No objections :) Acknowledged. https://codereview.chromium.org/2228553003/diff/20001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:76: } // namespace On 2016/08/11 13:09:14, Marc Treib wrote: > On 2016/08/11 12:52:24, tschumann wrote: > > On 2016/08/11 12:15:24, Marc Treib wrote: > > > On 2016/08/10 17:37:57, tschumann wrote: > > > > On 2016/08/10 16:22:03, Marc Treib wrote: > > > > > Only the global functions should be in the anonymous namespace > > > > > > > > actually, i think there's a tendency to have all tests in the unnamed > > > namespace > > > > (https://yaqs.googleplex.com/eng/q/5783337608151040). Unless you need to > > make > > > > use of friend tests. > > > > > > > > I'm usually not feeling strong enough to argue, but if we had to settle on > > > > something, I'd be in favor of putting it all into an unnamed namespace. > > > > > > This is not a common tendency in Chrome I think, but alright. > > > I think it won't work though if we need to make the test a friend of the > > tested > > > class? So if we go for "everything in the anon namespace", then there'd > always > > > be exceptions. > > > > Consistency trumps; so let's remove it. [however, i'd be fine with exceptions > > for friend tests as they should be exceptional ;-)]. > > They're somewhat common in unit tests. Personally, I prefer friend declarations > to "ForTesting" methods or to "// public for testing". Acknowledged. https://codereview.chromium.org/2228553003/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2228553003/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:180: On 2016/08/11 12:53:09, Marc Treib wrote: > nit: remove extra empty line Done. https://codereview.chromium.org/2228553003/diff/40001/components/ntp_snippets... File components/ntp_snippets/mock_content_suggestions_provider_observer.h (right): https://codereview.chromium.org/2228553003/diff/40001/components/ntp_snippets... components/ntp_snippets/mock_content_suggestions_provider_observer.h:21: void OnNewSuggestions(ContentSuggestionsProvider* provider, On 2016/08/11 12:53:09, Marc Treib wrote: > This pattern (manually overridden method + mock below) might be worth a comment. Done. https://codereview.chromium.org/2228553003/diff/40001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h (right): https://codereview.chromium.org/2228553003/diff/40001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h:36: ~UrlInfo(); On 2016/08/11 12:53:09, Marc Treib wrote: > nit: methods before fields Done. https://codereview.chromium.org/2228553003/diff/40001/components/ntp_snippets... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2228553003/diff/40001/components/ntp_snippets... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider_unittest.cc:56: ::testing::UnorderedElementsAre( On 2016/08/11 12:53:09, Marc Treib wrote: > optional: You can add "using testing::UnorderedElementsAre;" (etc) above to make > this kind of thing a bit easier to read. Done.
lgtm https://codereview.chromium.org/2228553003/diff/60001/components/ntp_snippets... File components/ntp_snippets/mock_content_suggestions_provider_observer.h (right): https://codereview.chromium.org/2228553003/diff/60001/components/ntp_snippets... components/ntp_snippets/mock_content_suggestions_provider_observer.h:25: // ContentSuggestion. please add: [which takes const list of suggestions.] We do this trick so that the MOCK_METHOD behaves the same way in tests as the actual method and we can keep this gMock issue limited to the mock class.
The CQ bit was checked by vitaliii@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 vitaliii@chromium.org
The CQ bit was checked by vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, tschumann@chromium.org Link to the patchset: https://codereview.chromium.org/2228553003/#ps80001 (title: "rewrote comment.")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
vitaliii@chromium.org changed reviewers: + bauerb@chromium.org - treib@chromium.org, tschumann@chromium.org
bauerb@chromium.org: Please review changes.
https://codereview.chromium.org/2228553003/diff/100001/components/ntp_snippet... File components/ntp_snippets/mock_content_suggestions_provider_observer.h (right): https://codereview.chromium.org/2228553003/diff/100001/components/ntp_snippet... components/ntp_snippets/mock_content_suggestions_provider_observer.h:35: const std::list<ContentSuggestion>& suggestions)); If you use a vector here, could you just directly call this method? https://codereview.chromium.org/2228553003/diff/100001/components/ntp_snippet... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2228553003/diff/100001/components/ntp_snippet... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:41: if (suggestions.size() >= kMaxSuggestionsCount) { Nit: Braces are optional for single-line bodies. In principle you could keep them here, but then you should be consistent at least in the file, and also use them below -- or the other way around (which is I think a bit more common overall). https://codereview.chromium.org/2228553003/diff/100001/components/ntp_snippet... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:53: suggestions.emplace_back(std::move(suggestion)); Nit: Use push_back()? It feels a bit strange to use emplace_back(), as it constructs a new item but with the move constructor, whereas with push_back() the semantic is directly that you move the existing object onto the vector.
vitaliii@chromium.org changed reviewers: + treib@chromium.org, tschumann@chromium.org
https://codereview.chromium.org/2228553003/diff/100001/components/ntp_snippet... File components/ntp_snippets/mock_content_suggestions_provider_observer.h (right): https://codereview.chromium.org/2228553003/diff/100001/components/ntp_snippet... components/ntp_snippets/mock_content_suggestions_provider_observer.h:35: const std::list<ContentSuggestion>& suggestions)); On 2016/08/12 12:55:19, Bernhard Bauer wrote: > If you use a vector here, could you just directly call this method? Let's consider all possible cases: - No |void OnNewSuggestions|, MOCK with const std::vector<ContentSuggestion>& suggestions error: 'ntp_snippets::MockContentSuggestionsProviderObserver::OnNewSuggestions' hides overloaded virtual function - No |void OnNewSuggestions|, MOCK with const std::vector<ContentSuggestion> suggestions error: call to deleted constructor of 'ntp_snippets::ContentSuggestion' - No |void OnNewSuggestions|, MOCK with std::vector<ContentSuggestion>& suggestions error: 'ntp_snippets::MockContentSuggestionsProviderObserver::OnNewSuggestions' hides overloaded virtual function - No |void OnNewSuggestions|, MOCK with std::vector<ContentSuggestion> suggestions error: call to deleted constructor of 'ntp_snippets::ContentSuggestion' https://codereview.chromium.org/2228553003/diff/100001/components/ntp_snippet... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2228553003/diff/100001/components/ntp_snippet... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:41: if (suggestions.size() >= kMaxSuggestionsCount) { On 2016/08/12 12:55:19, Bernhard Bauer wrote: > Nit: Braces are optional for single-line bodies. In principle you could keep > them here, but then you should be consistent at least in the file, and also use > them below -- or the other way around (which is I think a bit more common > overall). Done. https://codereview.chromium.org/2228553003/diff/100001/components/ntp_snippet... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:53: suggestions.emplace_back(std::move(suggestion)); On 2016/08/12 12:55:19, Bernhard Bauer wrote: > Nit: Use push_back()? It feels a bit strange to use emplace_back(), as it > constructs a new item but with the move constructor, whereas with push_back() > the semantic is directly that you move the existing object onto the vector. Done.
https://codereview.chromium.org/2228553003/diff/100001/components/ntp_snippet... File components/ntp_snippets/mock_content_suggestions_provider_observer.cc (right): https://codereview.chromium.org/2228553003/diff/100001/components/ntp_snippet... components/ntp_snippets/mock_content_suggestions_provider_observer.cc:20: for (auto& suggestion : suggestions) { Can you use the actual type here? It's not that much longer. https://codereview.chromium.org/2228553003/diff/100001/components/ntp_snippet... components/ntp_snippets/mock_content_suggestions_provider_observer.cc:21: suggestions_list.emplace_back(std::move(suggestion)); Same comment about push_back() applies here. https://codereview.chromium.org/2228553003/diff/100001/components/ntp_snippet... File components/ntp_snippets/mock_content_suggestions_provider_observer.h (right): https://codereview.chromium.org/2228553003/diff/100001/components/ntp_snippet... components/ntp_snippets/mock_content_suggestions_provider_observer.h:35: const std::list<ContentSuggestion>& suggestions)); On 2016/08/12 13:32:46, vitaliii wrote: > On 2016/08/12 12:55:19, Bernhard Bauer wrote: > > If you use a vector here, could you just directly call this method? > > Let's consider all possible cases: > > - No |void OnNewSuggestions|, MOCK with const std::vector<ContentSuggestion>& > suggestions > error: > 'ntp_snippets::MockContentSuggestionsProviderObserver::OnNewSuggestions' hides > overloaded virtual function > > - No |void OnNewSuggestions|, MOCK with const std::vector<ContentSuggestion> > suggestions > error: call to deleted constructor of 'ntp_snippets::ContentSuggestion' > > - No |void OnNewSuggestions|, MOCK with std::vector<ContentSuggestion>& > suggestions > error: > 'ntp_snippets::MockContentSuggestionsProviderObserver::OnNewSuggestions' hides > overloaded virtual function > > - No |void OnNewSuggestions|, MOCK with std::vector<ContentSuggestion> > suggestions > error: call to deleted constructor of 'ntp_snippets::ContentSuggestion' Sorry, what I meant was calling the mock method from the non-mock method without having to write a loop. That would only work with a non-overloaded method though, which I guess Tim dislikes...
https://codereview.chromium.org/2228553003/diff/100001/components/ntp_snippet... File components/ntp_snippets/mock_content_suggestions_provider_observer.cc (right): https://codereview.chromium.org/2228553003/diff/100001/components/ntp_snippet... components/ntp_snippets/mock_content_suggestions_provider_observer.cc:20: for (auto& suggestion : suggestions) { On 2016/08/12 13:49:06, Bernhard Bauer wrote: > Can you use the actual type here? It's not that much longer. Done. https://codereview.chromium.org/2228553003/diff/100001/components/ntp_snippet... components/ntp_snippets/mock_content_suggestions_provider_observer.cc:21: suggestions_list.emplace_back(std::move(suggestion)); On 2016/08/12 13:49:06, Bernhard Bauer wrote: > Same comment about push_back() applies here. Done. https://codereview.chromium.org/2228553003/diff/100001/components/ntp_snippet... File components/ntp_snippets/mock_content_suggestions_provider_observer.h (right): https://codereview.chromium.org/2228553003/diff/100001/components/ntp_snippet... components/ntp_snippets/mock_content_suggestions_provider_observer.h:35: const std::list<ContentSuggestion>& suggestions)); On 2016/08/12 13:49:06, Bernhard Bauer wrote: > On 2016/08/12 13:32:46, vitaliii wrote: > > On 2016/08/12 12:55:19, Bernhard Bauer wrote: > > > If you use a vector here, could you just directly call this method? > > > > Let's consider all possible cases: > > > > - No |void OnNewSuggestions|, MOCK with const std::vector<ContentSuggestion>& > > suggestions > > error: > > 'ntp_snippets::MockContentSuggestionsProviderObserver::OnNewSuggestions' hides > > overloaded virtual function > > > > - No |void OnNewSuggestions|, MOCK with const std::vector<ContentSuggestion> > > suggestions > > error: call to deleted constructor of 'ntp_snippets::ContentSuggestion' > > > > - No |void OnNewSuggestions|, MOCK with std::vector<ContentSuggestion>& > > suggestions > > error: > > 'ntp_snippets::MockContentSuggestionsProviderObserver::OnNewSuggestions' hides > > overloaded virtual function > > > > - No |void OnNewSuggestions|, MOCK with std::vector<ContentSuggestion> > > suggestions > > error: call to deleted constructor of 'ntp_snippets::ContentSuggestion' > > Sorry, what I meant was calling the mock method from the non-mock method without > having to write a loop. That would only work with a non-overloaded method > though, which I guess Tim dislikes... Indeed, I tried this before. Tim's argument was that non-overloaded method confuses the reader and, moreover, with overloaded method if the issue gets resolved in gMock, one needs to modify the mock class, but not all the code using it.
lgtm
The CQ bit was checked by vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, tschumann@chromium.org Link to the patchset: https://codereview.chromium.org/2228553003/#ps140001 (title: "Bernhard's comments.")
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, treib@chromium.org, tschumann@chromium.org Link to the patchset: https://codereview.chromium.org/2228553003/#ps160001 (title: "rebase + autoformat")
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-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) 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 vitaliii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, treib@chromium.org, tschumann@chromium.org Link to the patchset: https://codereview.chromium.org/2228553003/#ps180001 (title: "resolved compile error.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== a provider of Physical Web pages. BUG=635893 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== a provider of Physical Web pages. BUG=635893 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== a provider of Physical Web pages. BUG=635893 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== a provider of Physical Web pages. BUG=635893 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/762478d1961b46576d67263bf766d7c391f8514d Cr-Commit-Position: refs/heads/master@{#412484} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/762478d1961b46576d67263bf766d7c391f8514d Cr-Commit-Position: refs/heads/master@{#412484} |
