Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2279123002:
[Sync] Initial implementation of foreign sessions suggestions provider. (Closed)
Description[Sync] Initial implementation of foreign sessions suggestions provider.
BUG=646951
Committed: https://crrev.com/9e961dbb273300d2d04c2e207788a2843b784444
Cr-Commit-Position: refs/heads/master@{#419520}
Patch Set 1 : Remove foreign sessions suggestions when user disabled session data syncing. #
Total comments: 51
Patch Set 2 : Updates and tests! #Patch Set 3 : Adding sessions deps to BUILD.gn #
Total comments: 67
Patch Set 4 : More updates for comments! #Patch Set 5 : Explicitly compare raw pointer with nullptr. #
Total comments: 27
Patch Set 6 : Updating for comments, again! #Messages
Total messages: 66 (43 generated)
The CQ bit was checked by skym@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.
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by skym@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...
skym@chromium.org changed reviewers: + bauerb@google.com, pavely@chromium.org, peconn@chromium.org, skuhne@chromium.org, treib@chromium.org
PTAL */ntp_snipppets/* - treib@chromium.org chrome/browser/prefs/* - bauerb@chromium.org chrome/android/java/src/org/chromium/chrome/browser/ntp/* - peconn@chromium.org DEP on components/sessions - skuhne@ DEP on components/sync_sessions - pavely@
Description was changed from ========== [Sync] Initial implementation of foreign sessions suggestions provider. BUG=641166 ========== to ========== [Sync] Initial implementation of foreign sessions suggestions provider. BUG=641166 ==========
skym@chromium.org changed reviewers: + battre@chromium.org - bauerb@google.com
Adding battre@ for chrome/browser/prefs/* , removing bauerb@ since they're OOO.
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionItem.java LGTM
https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/DEPS (right): https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/DEPS:12: "+components/sync_sessions", You only use components/sync_sessions in foreign_sessions_suggestions_provider. You can move this dependency into ntp_snippets/sessions/DEPS
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
Nice! Mostly LG, just some minor comments. https://codereview.chromium.org/2279123002/diff/60001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2279123002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:204: DependsOn(ProfileSyncServiceFactory::GetInstance()); nit: please keep alphabetized https://codereview.chromium.org/2279123002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:276: sync_driver::SyncService* sync_service = Please put this into a RegisterSessionsProvider helper function, like the others https://codereview.chromium.org/2279123002/diff/60001/chrome/browser/prefs/br... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/2279123002/diff/60001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:563: registry); Is any of this really Android-specific? If not, please put it outside the #ifdef. (We only have UI for Android right now, but we do plan to support other platforms soon-ish, specifically iOS.) https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/category.h (right): https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/category.h:35: FOREIGN_TABS, You should also add the new category in content_suggestions_metrics.cc (we need at least a comment here saying that...) https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/category_factory.cc (right): https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/category_factory.cc:19: AddKnownCategory(KnownCategories::FOREIGN_TABS); Have we decided on an order here? I think we might want to put it above bookmarks. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/features.cc (right): https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/features.cc:43: if (!value_as_string.empty()) nit: Braces please if the body doesn't fit on one line https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc (right): https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:366: pref_service_); Hm, might as well get rid of the members now and just call the new helper directly. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/pref_names.h (right): https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/pref_names.h:45: PrefService* pref_service); nit: const nit²: Should |pref_service| be the first param? https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc (right): https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:42: const char* kMaxForeignTabsPerDeviceParamName = "max_tabs_per_device"; max_foreign_tabs_per_device, for consistency? https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:112: dismissed_ids_.insert(suggestion_id); Right now, this can only ever grow. We need some way to clean up no-longer-relevant IDs, e.g. a full cleanup pass on startup. Also maybe an expiry time? (All that can wait for another CL, but at least a TODO would be nice.) https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:160: // It is difficult to tell if sync simply has not initialized yet or there Here, category_status_ can only be INITIALIZING or NOT_PROVIDED, right? DCHECK that maybe, to make clear that this won't reset any error state? https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:172: // suggestions to remove previous suggestions that are now filtered out. ...so the "if" this is in shouldn't be there? https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:201: if (tab->navigations.size() > 0) { optional nit: "if (tab->navigations.size() == 0) continue;", to remove one of the many levels of indentation? https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:237: // Pick up to max (total and per device) tabs, and ensure no duplcates s/duplcates/duplicates/ https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:248: suggestions.emplace_back(BuildSuggestion(tab, navigation)); nit: push_back will do the same thing here https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:257: // is not currently a convineint way to show this in the UI. If device s/convineint/convenient/ https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:259: // recency and then tab/navigation recency. Some ideas (not thought through): - Have a section per device - Add it to the publisher_name field ("Chromebook - facebook.com") https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.h (right): https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.h:69: std::set<std::string> dismissed_ids_; optional: Just read these from the pref on demand? Then there won't be any problem with keeping the two in sync.
https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/pref_names.h (right): https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/pref_names.h:45: PrefService* pref_service); On 2016/08/29 09:18:50, Marc Treib wrote: > nit: const > nit²: Should |pref_service| be the first param? yeah, let's make |pref_service| a const reference and the first parameter. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc (right): https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:74: sync_service_(sync_service), nit: I wonder if ForeignSessionsSuggestionsProvider plays too many roles (looking at the many interface methods it implements). What about having a SyncServiceApapter class so that we can use composition instead of inheritance? Something like class SyncServiceApapter : public sync_driver::SyncServiceObserver { public: SyncServiceAdapter(sync_driver::SyncService* sync_service, base::Callback<void(sync_driver::OpenTabsUIDelegate* on_data); } which also hides away the registration with the service? WDYT?
https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right): https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:48: base::Time GetThresholdTime() { Just FYI. From the perspective of somebody who is not familiar with the code, these functions could use some comments. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/pref_names.h (right): https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/pref_names.h:45: PrefService* pref_service); Do these functions belong into pref_names.h? https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc (right): https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:112: dismissed_ids_.insert(suggestion_id); On 2016/08/29 09:18:51, Marc Treib wrote: > Right now, this can only ever grow. We need some way to clean up > no-longer-relevant IDs, e.g. a full cleanup pass on startup. Also maybe an > expiry time? +1 - I am a bit concerned that the preferences file becomes a huge dumping ground if suggestions are ephemeral things and you get the opportunity to dismiss several ones per day. > (All that can wait for another CL, but at least a TODO would be nice.) Really? Then you need to do a data scheme migration afterwards. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:201: if (tab->navigations.size() > 0) { On 2016/08/29 09:18:51, Marc Treib wrote: > optional nit: "if (tab->navigations.size() == 0) continue;", to remove one of > the many levels of indentation? I think we prefer empty() over size() == 0
https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc (right): https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:112: dismissed_ids_.insert(suggestion_id); On 2016/08/29 09:48:26, battre (OOO until Aug 22) wrote: > On 2016/08/29 09:18:51, Marc Treib wrote: > > Right now, this can only ever grow. We need some way to clean up > > no-longer-relevant IDs, e.g. a full cleanup pass on startup. Also maybe an > > expiry time? > > +1 - I am a bit concerned that the preferences file becomes a huge dumping > ground if suggestions are ephemeral things and you get the opportunity to > dismiss several ones per day. > > > (All that can wait for another CL, but at least a TODO would be nice.) > > Really? Then you need to do a data scheme migration afterwards. Right now, this is behind a disabled-by-default flag, so it's okay if the implementation is incomplete. Clearly this will need to be addressed before any launch. The "data migration" shouldn't be any different from the periodic cleanup which we'll need anyway.
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
The CQ bit was checked by skym@chromium.org to run a CQ dry run
Updates and added tests, PTAL. Thank you everyone for the extremely prompt reviews ~3 weeks ago. My sincerest apologizes for taking so long to update and respond to comments. https://codereview.chromium.org/2279123002/diff/60001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2279123002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:204: DependsOn(ProfileSyncServiceFactory::GetInstance()); On 2016/08/29 09:18:50, Marc Treib wrote: > nit: please keep alphabetized Done. https://codereview.chromium.org/2279123002/diff/60001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:276: sync_driver::SyncService* sync_service = On 2016/08/29 09:18:50, Marc Treib wrote: > Please put this into a RegisterSessionsProvider helper function, like the others Done. https://codereview.chromium.org/2279123002/diff/60001/chrome/browser/prefs/br... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/2279123002/diff/60001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:563: registry); On 2016/08/29 09:18:50, Marc Treib wrote: > Is any of this really Android-specific? If not, please put it outside the > #ifdef. (We only have UI for Android right now, but we do plan to support other > platforms soon-ish, specifically iOS.) Done. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/DEPS (right): https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/DEPS:12: "+components/sync_sessions", On 2016/08/26 19:41:25, pavely wrote: > You only use components/sync_sessions in foreign_sessions_suggestions_provider. > You can move this dependency into ntp_snippets/sessions/DEPS Done. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right): https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:48: base::Time GetThresholdTime() { On 2016/08/29 09:48:26, battre wrote: > Just FYI. From the perspective of somebody who is not familiar with the code, > these functions could use some comments. Added comments! https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/category.h (right): https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/category.h:35: FOREIGN_TABS, On 2016/08/29 09:18:50, Marc Treib wrote: > You should also add the new category in content_suggestions_metrics.cc (we need > at least a comment here saying that...) Done and Done. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/category_factory.cc (right): https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/category_factory.cc:19: AddKnownCategory(KnownCategories::FOREIGN_TABS); On 2016/08/29 09:18:50, Marc Treib wrote: > Have we decided on an order here? I think we might want to put it above > bookmarks. I don't think there's been any thought/agreement on ordering here. Moving to above bookmarks for now. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/features.cc (right): https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/features.cc:43: if (!value_as_string.empty()) On 2016/08/29 09:18:50, Marc Treib wrote: > nit: Braces please if the body doesn't fit on one line Done. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc (right): https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:366: pref_service_); On 2016/08/29 09:18:50, Marc Treib wrote: > Hm, might as well get rid of the members now and just call the new helper > directly. I can do this if you still really want me to, and this was my original plan. But calling GetDismissedPref(category) and adding pref_service_ is a decent amount of boilerplate and each of these two helpers is called 5x throughout the file. I think these functions are actually helping keep the code more readable (fewer multiline lines), which is why I ended up deciding to leave them in. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/pref_names.h (right): https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/pref_names.h:45: PrefService* pref_service); On 2016/08/29 09:18:50, Marc Treib wrote: > nit: const > nit²: Should |pref_service| be the first param? Done. Done. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/pref_names.h:45: PrefService* pref_service); On 2016/08/29 09:39:15, tschumann wrote: > On 2016/08/29 09:18:50, Marc Treib wrote: > > nit: const > > nit²: Should |pref_service| be the first param? > > yeah, let's make |pref_service| a const reference and the first parameter. Done. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/pref_names.h:45: PrefService* pref_service); On 2016/08/29 09:48:26, battre wrote: > Do these functions belong into pref_names.h? Moved to their own file. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc (right): https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:42: const char* kMaxForeignTabsPerDeviceParamName = "max_tabs_per_device"; On 2016/08/29 09:18:51, Marc Treib wrote: > max_foreign_tabs_per_device, for consistency? Done. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:74: sync_service_(sync_service), On 2016/08/29 09:39:15, tschumann wrote: > nit: I wonder if ForeignSessionsSuggestionsProvider plays too many roles > (looking at the many interface methods it implements). > > What about having a SyncServiceApapter class so that we can use composition > instead of inheritance? > > Something like > class SyncServiceApapter : public sync_driver::SyncServiceObserver { > public: > SyncServiceAdapter(sync_driver::SyncService* sync_service, > base::Callback<void(sync_driver::OpenTabsUIDelegate* > on_data); > } > > which also hides away the registration with the service? > WDYT? Done. Tried to make it DI as well, got a little awkward, let me know if you think there's a way I could improve it. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:112: dismissed_ids_.insert(suggestion_id); On 2016/08/29 09:56:38, Marc Treib wrote: > On 2016/08/29 09:48:26, battre (OOO until Aug 22) wrote: > > On 2016/08/29 09:18:51, Marc Treib wrote: > > > Right now, this can only ever grow. We need some way to clean up > > > no-longer-relevant IDs, e.g. a full cleanup pass on startup. Also maybe an > > > expiry time? > > > > +1 - I am a bit concerned that the preferences file becomes a huge dumping > > ground if suggestions are ephemeral things and you get the opportunity to > > dismiss several ones per day. > > > > > (All that can wait for another CL, but at least a TODO would be nice.) > > > > Really? Then you need to do a data scheme migration afterwards. > > Right now, this is behind a disabled-by-default flag, so it's okay if the > implementation is incomplete. Clearly this will need to be addressed before any > launch. > > The "data migration" shouldn't be any different from the periodic cleanup which > we'll need anyway. Done. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:160: // It is difficult to tell if sync simply has not initialized yet or there On 2016/08/29 09:18:51, Marc Treib wrote: > Here, category_status_ can only be INITIALIZING or NOT_PROVIDED, right? DCHECK > that maybe, to make clear that this won't reset any error state? Done. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:172: // suggestions to remove previous suggestions that are now filtered out. On 2016/08/29 09:18:51, Marc Treib wrote: > ...so the "if" this is in shouldn't be there? Whooops, you are correct! Great catch. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:201: if (tab->navigations.size() > 0) { On 2016/08/29 09:18:51, Marc Treib wrote: > optional nit: "if (tab->navigations.size() == 0) continue;", to remove one of > the many levels of indentation? Done. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:201: if (tab->navigations.size() > 0) { On 2016/08/29 09:48:26, battre wrote: > On 2016/08/29 09:18:51, Marc Treib wrote: > > optional nit: "if (tab->navigations.size() == 0) continue;", to remove one of > > the many levels of indentation? > > I think we prefer empty() over size() == 0 Done. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:237: // Pick up to max (total and per device) tabs, and ensure no duplcates On 2016/08/29 09:18:51, Marc Treib wrote: > s/duplcates/duplicates/ Done. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:248: suggestions.emplace_back(BuildSuggestion(tab, navigation)); On 2016/08/29 09:18:51, Marc Treib wrote: > nit: push_back will do the same thing here Done. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:257: // is not currently a convineint way to show this in the UI. If device On 2016/08/29 09:18:51, Marc Treib wrote: > s/convineint/convenient/ Done. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:259: // recency and then tab/navigation recency. On 2016/08/29 09:18:51, Marc Treib wrote: > Some ideas (not thought through): > - Have a section per device > - Add it to the publisher_name field ("Chromebook - facebook.com") The line with the publisher already looks like: [icon] www.google.com - 15 minutes ago Still short enough most of the time, but once in a while a long domain name will cause things to get cut off. I'm switching the ordering so that the icon stays next to the domain, I think it is more usable with device, although polish-wise, occasionally ugly. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.h (right): https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.h:69: std::set<std::string> dismissed_ids_; On 2016/08/29 09:18:51, Marc Treib wrote: > optional: Just read these from the pref on demand? Then there won't be any > problem with keeping the two in sync. Interesting. So if we assume prefs are all stored in memory. The trade off is then, extra computations to deserialize upon read (base::Value into std::set<std::string>), vs more state in this class and doubled memory usage of Chrome. I guess on mobile memory is probably more constrained, Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Sync] Initial implementation of foreign sessions suggestions provider. BUG=641166 ========== to ========== [Sync] Initial implementation of foreign sessions suggestions provider. BUG=646951 ==========
skym@chromium.org changed reviewers: + rkaplow@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by skym@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: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Thanks, looking good! A few more comments below, but all minor. https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.h (right): https://codereview.chromium.org/2279123002/diff/60001/components/ntp_snippets... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.h:69: std::set<std::string> dismissed_ids_; On 2016/09/15 23:18:18, skym wrote: > On 2016/08/29 09:18:51, Marc Treib wrote: > > optional: Just read these from the pref on demand? Then there won't be any > > problem with keeping the two in sync. > > Interesting. So if we assume prefs are all stored in memory. The trade off is > then, extra computations to deserialize upon read (base::Value into > std::set<std::string>), vs more state in this class and doubled memory usage of > Chrome. I guess on mobile memory is probably more constrained, Done. Yes, all prefs are in memory, otherwise reading them couldn't be synchronous. TBH, I think the difference in memory usage is probably too small to matter; I just find it a bit clearer not to duplicate the state. Anyway, thanks! https://codereview.chromium.org/2279123002/diff/140001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2279123002/diff/140001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:185: Profile* profile) { profile doesn't seem to be required https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:58: // visited timestamp is present. nit: This sounds like it would return a bool. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:73: // threshold are used. I don't think this quite matches the behavior (which is super confusing and has way too many modes, due to last-minute changes to the product design...). This value only matters at all if the creation date fallback is active. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/category.h (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/category.h:23: // to histogram suffix mapping to content_suggestions_metrics.cc. Thanks! This warning should soon not be needed anymore though, see https://codereview.chromium.org/2337103003/ https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/features.cc (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/features.cc:41: int value_as_int; I think some compilers will complain if you don't initialize this. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/features.h (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/features.h:33: const int default_value); nit: "const" doesn't do anything here https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/pref_names.cc (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/pref_names.cc:33: "ntp_suggestoin.foreign_sessions.dismissed_ids"; typo, "ntp_suggestoin" should be "ntp_suggestions" https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/pref_names.h (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/pref_names.h:8: class PrefService; not needed? https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:170: sync_driver::OpenTabsUIDelegate* open_tabs_ui_delegate) { Is it necessary to pass in the delegate? Can't we just get it via delegate_provider_->GetOpenTabsUIDelegate()? https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:291: navigation.virtual_url().host() + " - " + session.session_name)); This should probably be a string resource with two placeholders, so that e.g. the two can get properly swapped in right-to-left languages. Or maybe some languages use a different separator char. (If we're not sure what the final version will look like, it's also fine to just add a TODO for now.) https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.h (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.h:35: OnChangeWithDelegate; nit: I prefer using OnChangeWithDelegate = base::Callback<void(sync_driver::OpenTabsUIDelegate*)>; https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc:52: SessionWindow* GetWindow(SyncedSession* session, int window_id) { GetOrCreateWindow? https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc:142: // During the provider's construction the follow mock calls occur. nit: s/follow/following/ https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc:144: OnNewSuggestions(_, category(), testing::IsEmpty())); nit: "testing::" not required, there's a "using" above. (There's a few more instances of this below.) https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc:154: SyncedSession* GetSession(int session_id) { GotOrCreateSession? https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc:156: std::string id_as_string = std::to_string(session_id); Per https://chromium-cpp.appspot.com/, std::to_string isn't allowed yet :( https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc:181: void Dismiss(const std::string& url) { s/url/id/ ? https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc:195: std::unique_ptr<TestingPrefServiceSimple> pref_service_; I think this could just be an instance rather than a pointer? https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc:252: EXPECT_CALL( nit: I think all of these tests deserve a one-line comment explaining the expected behavior, e.g. something like "We should get a maximum of 10 suggestions in total." https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/sessions/tab_delegate_sync_adapter.cc (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/tab_delegate_sync_adapter.cc:30: change_callback_ = change_callback; Maybe DCHECK that change_callback_ was null before? https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/sessions/tab_delegate_sync_adapter.h (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/tab_delegate_sync_adapter.h:34: // sync_driver::SyncServiceObserver implementation. These can (and IMO should) be private. https://codereview.chromium.org/2279123002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2279123002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:100283: + <suffix name="ForeignTabs" label="Open tabs on other devices"/> nit: I'd prefer to keep "Experimental" last.
tschumann@chromium.org changed reviewers: + tschumann@chromium.org
Thanks for adding the adapter class! I left a few minor issues overall looks good! https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/features.h (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/features.h:33: const int default_value); On 2016/09/16 12:26:48, Marc Treib wrote: > nit: "const" doesn't do anything here right. You can keep it in the definition to prevent accidental modification though. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:171: if (open_tabs_ui_delegate) { nit: i'd invert this condition and exit early instead. Makes it easier to focus on the main path and saves extra indentations. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:229: MakeUniqueID(provided_category_, navigation.virtual_url().spec()); this line confused me at first a bit, as we don't have de-duplication logic in here [found it afterwards]. I was tempted to suggest to do the filtering later when we also de-duplicate. But it's actually nice to keep the two steps separated and the code cleaner structured. That said, what about moving this into a function like: std::vector<SessionTuple> suggestion_candidates = GetSuggestionCandidates(foreign_sessions, dismissed_ids, max_forein_tab_age); I'd also suggest to turn SessionTuple into a struct, like SessionData and define operator< within that struct. Looking at line 255-257, named members seem useful anyways. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:242: // Sort by recency. Note that SerializedNavigationEntry::timestamp() is nit: rephrase to better reflect why we sort? like // Sort by recency so that we keep the most recent entries. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:252: std::set<std::string> duplicate_urls; nano nit: this variable is technically not collecting duplicated urls. Rename to included_urls? (the iterator can still be called duplicate_iter as it nicely explains the semantics in that case). https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.h (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.h:32: class OpenTabsUIDelegateProvider { not feeling strongly, but IMO this doesn't have to be a nested class. We can just move it out and save typing long names ;-) https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc:78: : public sync_driver::OpenTabsUIDelegate, do we actually need to implement this interface? https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/sessions/tab_delegate_sync_adapter.cc (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/tab_delegate_sync_adapter.cc:30: change_callback_ = change_callback; On 2016/09/16 12:26:49, Marc Treib wrote: > Maybe DCHECK that change_callback_ was null before? yes, please. Also add a comment to the OpenTabsUIDelegateProvider interface documenting that it may only be called once.
lgtm histogram lg
The CQ bit was checked by skym@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...
https://codereview.chromium.org/2279123002/diff/140001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2279123002/diff/140001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:185: Profile* profile) { On 2016/09/16 12:26:48, Marc Treib wrote: > profile doesn't seem to be required Done. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:58: // visited timestamp is present. On 2016/09/16 12:26:48, Marc Treib wrote: > nit: This sounds like it would return a bool. This value is confusing. Updated. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:73: // threshold are used. On 2016/09/16 12:26:48, Marc Treib wrote: > I don't think this quite matches the behavior (which is super confusing and has > way too many modes, due to last-minute changes to the product design...). > This value only matters at all if the creation date fallback is active. Okay, I've updated this, let me know if you thinks its okay now. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/category.h (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/category.h:23: // to histogram suffix mapping to content_suggestions_metrics.cc. On 2016/09/16 12:26:48, Marc Treib wrote: > Thanks! This warning should soon not be needed anymore though, see > https://codereview.chromium.org/2337103003/ Sweet, removed! https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/features.cc (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/features.cc:41: int value_as_int; On 2016/09/16 12:26:48, Marc Treib wrote: > I think some compilers will complain if you don't initialize this. Done. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/features.h (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/features.h:33: const int default_value); On 2016/09/16 12:26:48, Marc Treib wrote: > nit: "const" doesn't do anything here Removed from declaration only. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/pref_names.cc (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/pref_names.cc:33: "ntp_suggestoin.foreign_sessions.dismissed_ids"; On 2016/09/16 12:26:48, Marc Treib wrote: > typo, "ntp_suggestoin" should be "ntp_suggestions" Done. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/pref_names.h (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/pref_names.h:8: class PrefService; On 2016/09/16 12:26:48, Marc Treib wrote: > not needed? Done. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:170: sync_driver::OpenTabsUIDelegate* open_tabs_ui_delegate) { On 2016/09/16 12:26:48, Marc Treib wrote: > Is it necessary to pass in the delegate? Can't we just get it via > delegate_provider_->GetOpenTabsUIDelegate()? Done. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:171: if (open_tabs_ui_delegate) { On 2016/09/16 13:33:20, tschumann wrote: > nit: i'd invert this condition and exit early instead. Makes it easier to focus > on the main path and saves extra indentations. Done. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:229: MakeUniqueID(provided_category_, navigation.virtual_url().spec()); On 2016/09/16 13:33:20, tschumann wrote: > this line confused me at first a bit, as we don't have de-duplication logic in > here [found it afterwards]. > I was tempted to suggest to do the filtering later when we also de-duplicate. > But it's actually nice to keep the two steps separated and the code cleaner > structured. > > That said, what about moving this into a function like: > std::vector<SessionTuple> suggestion_candidates = > GetSuggestionCandidates(foreign_sessions, dismissed_ids, max_forein_tab_age); > > I'd also suggest to turn SessionTuple into a struct, like SessionData and define > operator< within that struct. Looking at line 255-257, named members seem useful > anyways. > Done. I was hesitant to initially break up this function because I felt the big tuple kind of really tightly coupled them. But with the struct I feel much better about this. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:242: // Sort by recency. Note that SerializedNavigationEntry::timestamp() is On 2016/09/16 13:33:20, tschumann wrote: > nit: rephrase to better reflect why we sort? > like // Sort by recency so that we keep the most recent entries. Done. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:252: std::set<std::string> duplicate_urls; On 2016/09/16 13:33:20, tschumann wrote: > nano nit: this variable is technically not collecting duplicated urls. Rename to > included_urls? (the iterator can still be called duplicate_iter as it nicely > explains the semantics in that case). Done. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:291: navigation.virtual_url().host() + " - " + session.session_name)); On 2016/09/16 12:26:48, Marc Treib wrote: > This should probably be a string resource with two placeholders, so that e.g. > the two can get properly swapped in right-to-left languages. Or maybe some > languages use a different separator char. > (If we're not sure what the final version will look like, it's also fine to just > add a TODO for now.) I'll throw in a big comment here. The favicon is on the far left, so I think it would only make sense to swap these if we also moved around where the favicon shows up, we want the domain adjacent to the icon. Though admittedly, I am not familiar with localizing for right-to-left languages. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.h (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.h:32: class OpenTabsUIDelegateProvider { On 2016/09/16 13:33:20, tschumann wrote: > not feeling strongly, but IMO this doesn't have to be a nested class. > We can just move it out and save typing long names ;-) Done. I find I have a hard time understand if things like this should be nested on not. On one hand, it seems pretty generic and isn't tied to directly to ForeignSessionsSuggestionsProvider but it is a pretty niche interface. And though it isn't used anywhere else, and if you wanted to start using it, you wouldn't want to #include ...foreign_sessions_suggestions_provider.h to get it. You'd move it out to somewhere shared. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.h:35: OnChangeWithDelegate; On 2016/09/16 12:26:48, Marc Treib wrote: > nit: I prefer > using OnChangeWithDelegate = > base::Callback<void(sync_driver::OpenTabsUIDelegate*)>; Done. And then removed in favor of base::Closure https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc:52: SessionWindow* GetWindow(SyncedSession* session, int window_id) { On 2016/09/16 12:26:49, Marc Treib wrote: > GetOrCreateWindow? Done. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc:78: : public sync_driver::OpenTabsUIDelegate, On 2016/09/16 13:33:20, tschumann wrote: > do we actually need to implement this interface? Done. Ended up removing OpenTabsUIDelegate from the provider interface, turned out a lot prettier I think. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc:142: // During the provider's construction the follow mock calls occur. On 2016/09/16 12:26:48, Marc Treib wrote: > nit: s/follow/following/ Done. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc:144: OnNewSuggestions(_, category(), testing::IsEmpty())); On 2016/09/16 12:26:49, Marc Treib wrote: > nit: "testing::" not required, there's a "using" above. (There's a few more > instances of this below.) Done. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc:154: SyncedSession* GetSession(int session_id) { On 2016/09/16 12:26:48, Marc Treib wrote: > GotOrCreateSession? Done. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc:156: std::string id_as_string = std::to_string(session_id); On 2016/09/16 12:26:48, Marc Treib wrote: > Per https://chromium-cpp.appspot.com/, std::to_string isn't allowed yet :( :( Done. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc:181: void Dismiss(const std::string& url) { On 2016/09/16 12:26:48, Marc Treib wrote: > s/url/id/ ? So someone has to decide that to create a within_category_id for foreign tabs you just use the url. Right now that decision is made right here. If we renamed this field to id, then that decision is essentially being made at every location that calls this function instead. I think the former is better. I'll add a comment here making it more clear that this conversion is happening. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc:195: std::unique_ptr<TestingPrefServiceSimple> pref_service_; On 2016/09/16 12:26:48, Marc Treib wrote: > I think this could just be an instance rather than a pointer? Done. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc:252: EXPECT_CALL( On 2016/09/16 12:26:49, Marc Treib wrote: > nit: I think all of these tests deserve a one-line comment explaining the > expected behavior, e.g. something like "We should get a maximum of 10 > suggestions in total." Done. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/sessions/tab_delegate_sync_adapter.cc (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/tab_delegate_sync_adapter.cc:30: change_callback_ = change_callback; On 2016/09/16 12:26:49, Marc Treib wrote: > Maybe DCHECK that change_callback_ was null before? Done. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/tab_delegate_sync_adapter.cc:30: change_callback_ = change_callback; On 2016/09/16 13:33:20, tschumann wrote: > On 2016/09/16 12:26:49, Marc Treib wrote: > > Maybe DCHECK that change_callback_ was null before? > > yes, please. Also add a comment to the OpenTabsUIDelegateProvider interface > documenting that it may only be called once. Done. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/sessions/tab_delegate_sync_adapter.h (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/tab_delegate_sync_adapter.h:34: // sync_driver::SyncServiceObserver implementation. On 2016/09/16 12:26:49, Marc Treib wrote: > These can (and IMO should) be private. Can you explain why these should be private? Like what's the advantage? If someone knows this class is a TabDelegateSyncAdapter, then they also know that we're a sync_driver::SyncServiceObserver, which means they know we have these public (and virtual) methods. And since (afaik) we're only allowed to use public inheritance within the code base, I don't see how making these private does anything. https://codereview.chromium.org/2279123002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2279123002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:100283: + <suffix name="ForeignTabs" label="Open tabs on other devices"/> On 2016/09/16 12:26:49, Marc Treib wrote: > nit: I'd prefer to keep "Experimental" last. Done. Moved between Articles and Experimental.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by skym@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.
lgtm https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:229: MakeUniqueID(provided_category_, navigation.virtual_url().spec()); On 2016/09/16 18:18:48, skym wrote: > On 2016/09/16 13:33:20, tschumann wrote: > > this line confused me at first a bit, as we don't have de-duplication logic in > > here [found it afterwards]. > > I was tempted to suggest to do the filtering later when we also de-duplicate. > > But it's actually nice to keep the two steps separated and the code cleaner > > structured. > > > > That said, what about moving this into a function like: > > std::vector<SessionTuple> suggestion_candidates = > > GetSuggestionCandidates(foreign_sessions, dismissed_ids, max_forein_tab_age); > > > > I'd also suggest to turn SessionTuple into a struct, like SessionData and > define > > operator< within that struct. Looking at line 255-257, named members seem > useful > > anyways. > > > > Done. I was hesitant to initially break up this function because I felt the big > tuple kind of really tightly coupled them. But with the struct I feel much > better about this. Thanks! https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.h (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.h:32: class OpenTabsUIDelegateProvider { On 2016/09/16 18:18:49, skym wrote: > On 2016/09/16 13:33:20, tschumann wrote: > > not feeling strongly, but IMO this doesn't have to be a nested class. > > We can just move it out and save typing long names ;-) > > Done. I find I have a hard time understand if things like this should be nested > on not. On one hand, it seems pretty generic and isn't tied to directly to > ForeignSessionsSuggestionsProvider but it is a pretty niche interface. And > though it isn't used anywhere else, and if you wanted to start using it, you > wouldn't want to #include ...foreign_sessions_suggestions_provider.h to get it. > You'd move it out to somewhere shared. Exactly. I try to avoid nested classes unless they are an essential part of the interface, like ContentSuggestionsProvider::Observer. If you don't need the outer classes name/context to grasp the semantics of the inner class, then I usually don't nest them. And if not, I'd think about the names again ;-) Another slight benefit is that inner classes cannot be forward declared -- but my main motivation is to reduce the dependencies between the types and save awkward long type names ;-) https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc:78: : public sync_driver::OpenTabsUIDelegate, On 2016/09/16 18:18:49, skym wrote: > On 2016/09/16 13:33:20, tschumann wrote: > > do we actually need to implement this interface? > > Done. Ended up removing OpenTabsUIDelegate from the provider interface, turned > out a lot prettier I think. :-) I think so too! https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/sessions/tab_delegate_sync_adapter.h (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/tab_delegate_sync_adapter.h:34: // sync_driver::SyncServiceObserver implementation. On 2016/09/16 18:18:50, skym wrote: > On 2016/09/16 12:26:49, Marc Treib wrote: > > These can (and IMO should) be private. > > Can you explain why these should be private? Like what's the advantage? If > someone knows this class is a TabDelegateSyncAdapter, then they also know that > we're a sync_driver::SyncServiceObserver, which means they know we have these > public (and virtual) methods. And since (afaik) we're only allowed to use public > inheritance within the code base, I don't see how making these private does > anything. I guess you could argue that we use the adapter to deal with (and hide away) the details of the sync service. So any client of the adapter really only wants the ForeignSessionsProvider semantics. Marking the SyncServiceObserver functions private makes the interface clearer and easier to use. I'm not feeling strongly, but would be in favor of having them private, too. https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc (right): https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:106: /* has_more_button */ true, nit: if you follow the pattern /*has_more_button=*/true, then clang_tidy will issue warnings in case you the comment wrong (https://g3doc.corp.google.com/devtools/cymbal/clang_tidy/g3doc/checks/misc-ar...). https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:221: /*const SyncedSession& session = *std::get<0>(tuple); please remove ;-) https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:257: *pref_service_, prefs::kDismissedForeignSessionsSuggestions); I'm not feeling strongly, just want to mention the option. if you pass in the dismissed_ids into this function and provided_category_ in BuildSuggestion(), you can move these methods off the class into an unnamed namespace and don't have to define SessionData in the header. (you can still keep the functions and struct close to BuildSuggestions() -- no need to have only one unnamed namespace). It's not a big difference, but removes some distracting implementation details from the header.
chrome/browser/prefs/* LGTM https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right): https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:73: // parameters this may or not be respected. Currntly creation date fallback must nit: Currently https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... File components/ntp_snippets/pref_util.h (right): https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/pref_util.h:17: std::set<std::string> ReadDismissedIDsFromPrefs(const PrefService& pref_service, nit: I think that in the majority of cases, we pass this as a "const PrefService* pref_service" today. I don't know whether any approach is better than the other. (just a thing of consistency)
Thanks! LGTM with the remaining (minor) comments from Tim and me addressed. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:291: navigation.virtual_url().host() + " - " + session.session_name)); On 2016/09/16 18:18:48, skym wrote: > On 2016/09/16 12:26:48, Marc Treib wrote: > > This should probably be a string resource with two placeholders, so that e.g. > > the two can get properly swapped in right-to-left languages. Or maybe some > > languages use a different separator char. > > (If we're not sure what the final version will look like, it's also fine to > just > > add a TODO for now.) > > I'll throw in a big comment here. The favicon is on the far left, so I think it > would only make sense to swap these if we also moved around where the favicon > shows up, we want the domain adjacent to the icon. Though admittedly, I am not > familiar with localizing for right-to-left languages. Acknowledged. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/sessions/tab_delegate_sync_adapter.h (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/tab_delegate_sync_adapter.h:34: // sync_driver::SyncServiceObserver implementation. On 2016/09/17 15:52:56, tschumann wrote: > On 2016/09/16 18:18:50, skym wrote: > > On 2016/09/16 12:26:49, Marc Treib wrote: > > > These can (and IMO should) be private. > > > > Can you explain why these should be private? Like what's the advantage? If > > someone knows this class is a TabDelegateSyncAdapter, then they also know that > > we're a sync_driver::SyncServiceObserver, which means they know we have these > > public (and virtual) methods. And since (afaik) we're only allowed to use > public > > inheritance within the code base, I don't see how making these private does > > anything. > > I guess you could argue that we use the adapter to deal with (and hide away) the > details of the sync service. So any client of the adapter really only wants the > ForeignSessionsProvider semantics. Marking the SyncServiceObserver functions > private makes the interface clearer and easier to use. > I'm not feeling strongly, but would be in favor of having them private, too. Pretty much what Tim says: To me, it's an implementation detail that this class implements SyncServiceObserver; these methods aren't really part of the public interface. By marking them private, you make it clear to the casual reader that they're not meant to call these methods. https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc (right): https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:106: /* has_more_button */ true, On 2016/09/17 15:52:57, tschumann wrote: > nit: if you follow the pattern /*has_more_button=*/true, then clang_tidy will > issue warnings in case you the comment wrong > (https://g3doc.corp.google.com/devtools/cymbal/clang_tidy/g3doc/checks/misc-ar...). Do we have that in Chrome? If so, that should resolve the param-name-comment-style discussion once and for all :) https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:213: // appear as nit: remove the extra line break https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:257: *pref_service_, prefs::kDismissedForeignSessionsSuggestions); On 2016/09/17 15:52:56, tschumann wrote: > I'm not feeling strongly, just want to mention the option. > if you pass in the dismissed_ids into this function and provided_category_ in > BuildSuggestion(), you can move these methods off the class into an unnamed > namespace and don't have to define SessionData in the header. > (you can still keep the functions and struct close to BuildSuggestions() -- no > need to have only one unnamed namespace). > It's not a big difference, but removes some distracting implementation details > from the header. This seems like a good idea. If you keep the member functions (which is fine too), I'd prefer to remove the |foreign_sessions| param from GetSuggestionCandidates and BuildSuggestions, and instead just get the foreign sessions here where you need them. https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:293: // TODO(skym): It's unclear if this single approach is sufficient for s/single/simple/ https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:296: // and the |publish_date|, which is to the right. The domain always appear nit: should this be "The domain always appear*s*", or "The domain *should* always appear"? https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.h (right): https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.h:55: struct SessionData { optional: I think it's sufficient to forward-declare this here (like "struct SessionData;") and move the definition to the .cc https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc:216: // Each device, which is to equivalent equivilant a unique |session_tag|, has s/which is to equivalent equivilant/which is equivalent to/ https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... File components/ntp_snippets/sessions/tab_delegate_sync_adapter.h (right): https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/sessions/tab_delegate_sync_adapter.h:23: // simplified notifications and accesors for foreign tabs data. s/accesors/accessors/
lgtm https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... File components/ntp_snippets/pref_util.h (right): https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/pref_util.h:17: std::set<std::string> ReadDismissedIDsFromPrefs(const PrefService& pref_service, On 2016/09/19 08:54:34, battre wrote: > nit: I think that in the majority of cases, we pass this as a "const > PrefService* pref_service" today. I don't know whether any approach is better > than the other. (just a thing of consistency) If nullptr is not allowed and a method does not keep a reference to an input parameter, const reference is preferred over const pointer. (https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...) life-time requirements are very clear then at the call site. https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc (right): https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:106: /* has_more_button */ true, On 2016/09/19 09:24:08, Marc Treib wrote: > On 2016/09/17 15:52:57, tschumann wrote: > > nit: if you follow the pattern /*has_more_button=*/true, then clang_tidy will > > issue warnings in case you the comment wrong > > > (https://g3doc.corp.google.com/devtools/cymbal/clang_tidy/g3doc/checks/misc-ar...). > > Do we have that in Chrome? If so, that should resolve the > param-name-comment-style discussion once and for all :) Apparently, you can run it manually -- not sure how hard it is to get it run automatically though: https://chromium.googlesource.com/chromium/src/+show/master/docs/clang_tidy.md But for me, that would already be reason enough to settle on that style.
lgtm for DEPS component/sessions
The CQ bit was checked by skym@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.
https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... File components/ntp_snippets/sessions/tab_delegate_sync_adapter.h (right): https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/tab_delegate_sync_adapter.h:34: // sync_driver::SyncServiceObserver implementation. On 2016/09/17 15:52:56, tschumann wrote: > On 2016/09/16 18:18:50, skym wrote: > > On 2016/09/16 12:26:49, Marc Treib wrote: > > > These can (and IMO should) be private. > > > > Can you explain why these should be private? Like what's the advantage? If > > someone knows this class is a TabDelegateSyncAdapter, then they also know that > > we're a sync_driver::SyncServiceObserver, which means they know we have these > > public (and virtual) methods. And since (afaik) we're only allowed to use > public > > inheritance within the code base, I don't see how making these private does > > anything. > > I guess you could argue that we use the adapter to deal with (and hide away) the > details of the sync service. So any client of the adapter really only wants the > ForeignSessionsProvider semantics. Marking the SyncServiceObserver functions > private makes the interface clearer and easier to use. > I'm not feeling strongly, but would be in favor of having them private, too. Done. https://codereview.chromium.org/2279123002/diff/140001/components/ntp_snippet... components/ntp_snippets/sessions/tab_delegate_sync_adapter.h:34: // sync_driver::SyncServiceObserver implementation. On 2016/09/19 09:24:08, Marc Treib wrote: > On 2016/09/17 15:52:56, tschumann wrote: > > On 2016/09/16 18:18:50, skym wrote: > > > On 2016/09/16 12:26:49, Marc Treib wrote: > > > > These can (and IMO should) be private. > > > > > > Can you explain why these should be private? Like what's the advantage? If > > > someone knows this class is a TabDelegateSyncAdapter, then they also know > that > > > we're a sync_driver::SyncServiceObserver, which means they know we have > these > > > public (and virtual) methods. And since (afaik) we're only allowed to use > > public > > > inheritance within the code base, I don't see how making these private does > > > anything. > > > > I guess you could argue that we use the adapter to deal with (and hide away) > the > > details of the sync service. So any client of the adapter really only wants > the > > ForeignSessionsProvider semantics. Marking the SyncServiceObserver functions > > private makes the interface clearer and easier to use. > > I'm not feeling strongly, but would be in favor of having them private, too. > > Pretty much what Tim says: To me, it's an implementation detail that this class > implements SyncServiceObserver; these methods aren't really part of the public > interface. By marking them private, you make it clear to the casual reader that > they're not meant to call these methods. Done. https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right): https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:73: // parameters this may or not be respected. Currntly creation date fallback must On 2016/09/19 08:54:34, battre wrote: > nit: Currently Done. https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... File components/ntp_snippets/pref_util.h (right): https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/pref_util.h:17: std::set<std::string> ReadDismissedIDsFromPrefs(const PrefService& pref_service, On 2016/09/19 12:19:23, tschumann wrote: > On 2016/09/19 08:54:34, battre wrote: > > nit: I think that in the majority of cases, we pass this as a "const > > PrefService* pref_service" today. I don't know whether any approach is better > > than the other. (just a thing of consistency) > > If nullptr is not allowed and a method does not keep a reference to an input > parameter, const reference is preferred over const pointer. > (https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...) > life-time requirements are very clear then at the call site. Acknowledged. https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc (right): https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:106: /* has_more_button */ true, On 2016/09/17 15:52:57, tschumann wrote: > nit: if you follow the pattern /*has_more_button=*/true, then clang_tidy will > issue warnings in case you the comment wrong > (https://g3doc.corp.google.com/devtools/cymbal/clang_tidy/g3doc/checks/misc-ar...). Cool, had no idea this was a thing, done. Also updated bookmark_suggestion_provider.cc and offline_page_suggestions_provider.cc to use this formatting as well. https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:213: // appear as On 2016/09/19 09:24:08, Marc Treib wrote: > nit: remove the extra line break Done. https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:221: /*const SyncedSession& session = *std::get<0>(tuple); On 2016/09/17 15:52:57, tschumann wrote: > please remove ;-) Whooops, done! https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:257: *pref_service_, prefs::kDismissedForeignSessionsSuggestions); On 2016/09/17 15:52:56, tschumann wrote: > I'm not feeling strongly, just want to mention the option. > if you pass in the dismissed_ids into this function and provided_category_ in > BuildSuggestion(), you can move these methods off the class into an unnamed > namespace and don't have to define SessionData in the header. > (you can still keep the functions and struct close to BuildSuggestions() -- no > need to have only one unnamed namespace). > It's not a big difference, but removes some distracting implementation details > from the header. Acknowledged. https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:257: *pref_service_, prefs::kDismissedForeignSessionsSuggestions); On 2016/09/19 09:24:08, Marc Treib wrote: > On 2016/09/17 15:52:56, tschumann wrote: > > I'm not feeling strongly, just want to mention the option. > > if you pass in the dismissed_ids into this function and provided_category_ in > > BuildSuggestion(), you can move these methods off the class into an unnamed > > namespace and don't have to define SessionData in the header. > > (you can still keep the functions and struct close to BuildSuggestions() -- no > > need to have only one unnamed namespace). > > It's not a big difference, but removes some distracting implementation details > > from the header. > > This seems like a good idea. If you keep the member functions (which is fine > too), I'd prefer to remove the |foreign_sessions| param from > GetSuggestionCandidates and BuildSuggestions, and instead just get the foreign > sessions here where you need them. I like keeping this a member function. Going forward, I'm thinking this method will need even more member state (pref service to save pruned dismissed ids). Hopefully just forward declaring the struct will keep the header clean enough. Removed |foreign_sessions| parameter from this function. https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:293: // TODO(skym): It's unclear if this single approach is sufficient for On 2016/09/19 09:24:09, Marc Treib wrote: > s/single/simple/ Done. https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.cc:296: // and the |publish_date|, which is to the right. The domain always appear On 2016/09/19 09:24:09, Marc Treib wrote: > nit: should this be "The domain always appear*s*", or "The domain *should* > always appear"? Going with should. https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.h (right): https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.h:55: struct SessionData { On 2016/09/19 09:24:09, Marc Treib wrote: > optional: I think it's sufficient to forward-declare this here (like "struct > SessionData;") and move the definition to the .cc Oooh, great idea, Done. https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... File components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/sessions/foreign_sessions_suggestions_provider_unittest.cc:216: // Each device, which is to equivalent equivilant a unique |session_tag|, has On 2016/09/19 09:24:09, Marc Treib wrote: > s/which is to equivalent equivilant/which is equivalent to/ Done. https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... File components/ntp_snippets/sessions/tab_delegate_sync_adapter.h (right): https://codereview.chromium.org/2279123002/diff/180001/components/ntp_snippet... components/ntp_snippets/sessions/tab_delegate_sync_adapter.h:23: // simplified notifications and accesors for foreign tabs data. On 2016/09/19 09:24:09, Marc Treib wrote: > s/accesors/accessors/ Done.
The CQ bit was checked by skym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peconn@chromium.org, pavely@chromium.org, rkaplow@chromium.org, skuhne@chromium.org, tschumann@chromium.org, treib@chromium.org, battre@chromium.org Link to the patchset: https://codereview.chromium.org/2279123002/#ps200001 (title: "Updating for comments, again!")
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 ========== [Sync] Initial implementation of foreign sessions suggestions provider. BUG=646951 ========== to ========== [Sync] Initial implementation of foreign sessions suggestions provider. BUG=646951 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== [Sync] Initial implementation of foreign sessions suggestions provider. BUG=646951 ========== to ========== [Sync] Initial implementation of foreign sessions suggestions provider. BUG=646951 Committed: https://crrev.com/9e961dbb273300d2d04c2e207788a2843b784444 Cr-Commit-Position: refs/heads/master@{#419520} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/9e961dbb273300d2d04c2e207788a2843b784444 Cr-Commit-Position: refs/heads/master@{#419520} |
