Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1260113002:
Adds an experiment that enabled SuggestionsService suggestions (MostLikely) on LocalNTP (Closed)
|
Created:
5 years, 4 months ago by fserb Modified:
5 years, 4 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, skanuj+watch_chromium.org, melevin+watch_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, donnd+watch_chromium.org, jfweitz+watch_chromium.org, David Black, arv+watch_chromium.org, samarth+watch_chromium.org, chromium-apps-reviews_chromium.org, kmadhusu+watch_chromium.org, Jered Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds an experiment that enables SuggestionsService suggestions (MostLikely) on LocalNTP
BUG=514752
Committed: https://crrev.com/a485e91eb08876ca79f1acc968a7e050a5c45596
Cr-Commit-Position: refs/heads/master@{#341958}
Patch Set 1 #Patch Set 2 : #
Total comments: 10
Patch Set 3 : #
Total comments: 8
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Total comments: 14
Patch Set 6 : #
Messages
Total messages: 25 (7 generated)
fserb@chromium.org changed reviewers: + mathp@chromium.org
Starting with you, then sending to the rest of the ppl.
https://codereview.chromium.org/1260113002/diff/20001/chrome/browser/resource... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/1260113002/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_single.js:281: data.thumbnailUrls = [data.thumbnailUrl]; should we make sure data.thumbnailUrls is not undefined when exiting addTile? I know in your proposed change it wouldn't, but later on that may change. renderTile wouldn't handle it well. https://codereview.chromium.org/1260113002/diff/20001/chrome/browser/search/i... File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/1260113002/diff/20001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:139: if (!base::FieldTrialList::FindFullName("LocalNTPSuggestionsService") bring the field trial check to anonymous namespace, and I would rather check if the group name starts with "Enabled", like here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sea... https://codereview.chromium.org/1260113002/diff/20001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:141: suggestions::SuggestionsService* suggestions_service = Instead of calling the factory every time should we instead store a weak pointer to the service? https://codereview.chromium.org/1260113002/diff/20001/chrome/renderer/searchb... File chrome/renderer/searchbox/searchbox_extension.cc (right): https://codereview.chromium.org/1260113002/diff/20001/chrome/renderer/searchb... chrome/renderer/searchbox/searchbox_extension.cc:156: if (!mv_item.thumbnail.spec().empty()) { put a comment above this block https://codereview.chromium.org/1260113002/diff/20001/chrome/renderer/searchb... chrome/renderer/searchbox/searchbox_extension.cc:168: if (!mv_item.favicon.spec().empty()) { ...and this block, saying that Chrome can pre-populate some Thumbnail/Favicon URLs to be sent to the renderer in addition to local URLs
On 2015/07/28 13:32:43, Mathieu Perreault wrote: > https://codereview.chromium.org/1260113002/diff/20001/chrome/browser/resource... > File chrome/browser/resources/local_ntp/most_visited_single.js (right): > > https://codereview.chromium.org/1260113002/diff/20001/chrome/browser/resource... > chrome/browser/resources/local_ntp/most_visited_single.js:281: > data.thumbnailUrls = [data.thumbnailUrl]; > should we make sure data.thumbnailUrls is not undefined when exiting addTile? I > know in your proposed change it wouldn't, but later on that may change. > renderTile wouldn't handle it well. > > https://codereview.chromium.org/1260113002/diff/20001/chrome/browser/search/i... > File chrome/browser/search/instant_service.cc (right): > > https://codereview.chromium.org/1260113002/diff/20001/chrome/browser/search/i... > chrome/browser/search/instant_service.cc:139: if > (!base::FieldTrialList::FindFullName("LocalNTPSuggestionsService") > bring the field trial check to anonymous namespace, and I would rather check if > the group name starts with "Enabled", like here: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sea... > > https://codereview.chromium.org/1260113002/diff/20001/chrome/browser/search/i... > chrome/browser/search/instant_service.cc:141: suggestions::SuggestionsService* > suggestions_service = > Instead of calling the factory every time should we instead store a weak pointer > to the service? > > https://codereview.chromium.org/1260113002/diff/20001/chrome/renderer/searchb... > File chrome/renderer/searchbox/searchbox_extension.cc (right): > > https://codereview.chromium.org/1260113002/diff/20001/chrome/renderer/searchb... > chrome/renderer/searchbox/searchbox_extension.cc:156: if > (!mv_item.thumbnail.spec().empty()) { > put a comment above this block > > https://codereview.chromium.org/1260113002/diff/20001/chrome/renderer/searchb... > chrome/renderer/searchbox/searchbox_extension.cc:168: if > (!mv_item.favicon.spec().empty()) { > ...and this block, saying that Chrome can pre-populate some Thumbnail/Favicon > URLs to be sent to the renderer in addition to local URLs Please add tests to InstantService.
https://codereview.chromium.org/1260113002/diff/20001/chrome/browser/resource... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/1260113002/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_single.js:281: data.thumbnailUrls = [data.thumbnailUrl]; On 2015/07/28 13:32:42, Mathieu Perreault wrote: > should we make sure data.thumbnailUrls is not undefined when exiting addTile? I > know in your proposed change it wouldn't, but later on that may change. > renderTile wouldn't handle it well. Done. https://codereview.chromium.org/1260113002/diff/20001/chrome/browser/search/i... File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/1260113002/diff/20001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:139: if (!base::FieldTrialList::FindFullName("LocalNTPSuggestionsService") On 2015/07/28 13:32:42, Mathieu Perreault wrote: > bring the field trial check to anonymous namespace, and I would rather check if > the group name starts with "Enabled", like here: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sea... Done. https://codereview.chromium.org/1260113002/diff/20001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:141: suggestions::SuggestionsService* suggestions_service = On 2015/07/28 13:32:42, Mathieu Perreault wrote: > Instead of calling the factory every time should we instead store a weak pointer > to the service? Done. https://codereview.chromium.org/1260113002/diff/20001/chrome/renderer/searchb... File chrome/renderer/searchbox/searchbox_extension.cc (right): https://codereview.chromium.org/1260113002/diff/20001/chrome/renderer/searchb... chrome/renderer/searchbox/searchbox_extension.cc:156: if (!mv_item.thumbnail.spec().empty()) { On 2015/07/28 13:32:42, Mathieu Perreault wrote: > put a comment above this block Done. https://codereview.chromium.org/1260113002/diff/20001/chrome/renderer/searchb... chrome/renderer/searchbox/searchbox_extension.cc:168: if (!mv_item.favicon.spec().empty()) { On 2015/07/28 13:32:43, Mathieu Perreault wrote: > ...and this block, saying that Chrome can pre-populate some Thumbnail/Favicon > URLs to be sent to the renderer in addition to local URLs Done.
Approach lgtm, although I have no owners right at all :)
fserb@chromium.org changed reviewers: + kmadhusu@chromium.org, wfh@chromium.org
fserb@chromium.org changed required reviewers: + kmadhusu@chromium.org, wfh@chromium.org
kmadhusu: Everything but the JS :) wfh: chrome/common/render_messages.h
Seems fine for a browser controlled thumbnail and favicon to go from browser -> renderer via IPC. chrome/common/render_messages.h lgtm
Thanks. https://codereview.chromium.org/1260113002/diff/40001/chrome/browser/search/i... File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/1260113002/diff/40001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:148: suggestions::SuggestionsService* if (suggestions_service) { Can you please explain this code? Why do you need "suggestions::SuggestionsService* " before an if statement? https://codereview.chromium.org/1260113002/diff/40001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:194: url, base::Bind(&InstantService::OnSuggestionsAvailable, Instead of passing a callback as an argument, have you considered implementing SuggestionServiceObserver something similar to TopSitesObserver?
https://codereview.chromium.org/1260113002/diff/40001/chrome/browser/search/i... File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/1260113002/diff/40001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:148: suggestions::SuggestionsService* if (suggestions_service) { On 2015/07/29 17:42:31, kmadhusu wrote: > Can you please explain this code? Why do you need > "suggestions::SuggestionsService* " before an if statement? I messed up the last comment fix. :) thanks. done. https://codereview.chromium.org/1260113002/diff/40001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:194: url, base::Bind(&InstantService::OnSuggestionsAvailable, On 2015/07/29 17:42:31, kmadhusu wrote: > Instead of passing a callback as an argument, have you considered implementing > SuggestionServiceObserver something similar to TopSitesObserver? > SuggestionsService has a callback model, it doesn't expose an observer.
https://codereview.chromium.org/1260113002/diff/40001/chrome/browser/search/i... File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/1260113002/diff/40001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:194: url, base::Bind(&InstantService::OnSuggestionsAvailable, On 2015/07/29 19:03:00, fserb wrote: > On 2015/07/29 17:42:31, kmadhusu wrote: > > Instead of passing a callback as an argument, have you considered implementing > > SuggestionServiceObserver something similar to TopSitesObserver? > > > > SuggestionsService has a callback model, it doesn't expose an observer. (My 2 cents) Hmm. I am not entirely convinced by the callback model. I don't know the rationale behind the original model. InstantService should not worry about suggestions_service->BlacklistURL() response success/failure. InstantService should just notify SuggestionService about the new black list url and SuggestionService should notify the corresponding observers about this. Git blame says that mathp@ modified/wrote most of the suggestions_service. Please discuss with mathp@ and consider adding an observer model in the future. https://codereview.chromium.org/1260113002/diff/40001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:203: if (!top_sites) Do you want to do an early return when top_sites is null even when suggestions_service is not null? https://codereview.chromium.org/1260113002/diff/60001/chrome/browser/search/i... File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/1260113002/diff/60001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:140: if (base::StartsWith( Define a helper function in the anonymous namespace for this if condition. namespace { const char kLocalNTPSuggestionService = "LocalNTPSuggestionsService" const char kLocalNTPSuggestionServiceEnabled = "Enabled" bool IsLocalNTPSuggestionServiceEnabled() { return base::StartsWith(base::FieldTrialList::FindFullName(kLocalNTPSuggestionService), kLocalNTPSuggestionServiceEnabled, base::CompareCase::INSENSITIVE_ASCII)); } https://codereview.chromium.org/1260113002/diff/60001/chrome/common/instant_t... File chrome/common/instant_types.cc (right): https://codereview.chromium.org/1260113002/diff/60001/chrome/common/instant_t... chrome/common/instant_types.cc:93: InstantMostVisitedItem::~InstantMostVisitedItem() {} Any specific reason to define an empty constructor and destructor?
https://codereview.chromium.org/1260113002/diff/40001/chrome/browser/search/i... File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/1260113002/diff/40001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:203: if (!top_sites) On 2015/07/30 21:57:02, kmadhusu wrote: > Do you want to do an early return when top_sites is null even when > suggestions_service is not null? ops. Fixed all of them. Thanks for catching this. https://codereview.chromium.org/1260113002/diff/40001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:203: if (!top_sites) On 2015/07/30 21:57:02, kmadhusu wrote: > Do you want to do an early return when top_sites is null even when > suggestions_service is not null? Nice catch. Fixed them all. https://codereview.chromium.org/1260113002/diff/60001/chrome/browser/search/i... File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/1260113002/diff/60001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:140: if (base::StartsWith( On 2015/07/30 21:57:02, kmadhusu wrote: > Define a helper function in the anonymous namespace for this if condition. > > namespace { > const char kLocalNTPSuggestionService = "LocalNTPSuggestionsService" > const char kLocalNTPSuggestionServiceEnabled = "Enabled" > > bool IsLocalNTPSuggestionServiceEnabled() { > return > base::StartsWith(base::FieldTrialList::FindFullName(kLocalNTPSuggestionService), > kLocalNTPSuggestionServiceEnabled, base::CompareCase::INSENSITIVE_ASCII)); > } > Done. https://codereview.chromium.org/1260113002/diff/60001/chrome/common/instant_t... File chrome/common/instant_types.cc (right): https://codereview.chromium.org/1260113002/diff/60001/chrome/common/instant_t... chrome/common/instant_types.cc:93: InstantMostVisitedItem::~InstantMostVisitedItem() {} On 2015/07/30 21:57:02, kmadhusu wrote: > Any specific reason to define an empty constructor and destructor? The build complains that the struct has more than 3 items and must have a constructor/destructor.
LGTM with nits. Thanks. https://codereview.chromium.org/1260113002/diff/80001/chrome/browser/search/i... File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/1260113002/diff/80001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:35: #include "components/suggestions/proto/suggestions.pb.h" This is already included in instant_service.h. You can remove this from here. https://codereview.chromium.org/1260113002/diff/80001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:36: #include "components/suggestions/suggestions_service.h" This is already included in instant_service.h. You can remove this from here. https://codereview.chromium.org/1260113002/diff/80001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:58: const char kLocalNTPSuggestionService[] = "LocalNTPSuggestionsService"; style nit: Add a blank line before this. https://codereview.chromium.org/1260113002/diff/80001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:64: kLocalNTPSuggestionServiceEnabled, base::CompareCase::INSENSITIVE_ASCII); Include base/strings/string_util.h for CompareCase. https://codereview.chromium.org/1260113002/diff/80001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:65: } style nit: Add a blank line after this. https://codereview.chromium.org/1260113002/diff/80001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:320: item.thumbnail = GURL(suggestion.thumbnail()); nit: Its better to check if suggestion.has_thumbnail() before assigning the value to item.thumbnail. (same for favicon_url). https://codereview.chromium.org/1260113002/diff/80001/chrome/renderer/searchb... File chrome/renderer/searchbox/searchbox_extension.cc (right): https://codereview.chromium.org/1260113002/diff/80001/chrome/renderer/searchb... chrome/renderer/searchbox/searchbox_extension.cc:161: thumbs->Set(0, UTF8ToV8String(isolate, base::StringPrintf( nit: Create a helper function to generate this thumbnail url. It will be much easier to read this code.
Dona all. Thanks. :) https://codereview.chromium.org/1260113002/diff/80001/chrome/browser/search/i... File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/1260113002/diff/80001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:35: #include "components/suggestions/proto/suggestions.pb.h" On 2015/07/31 21:05:45, kmadhusu wrote: > This is already included in instant_service.h. You can remove this from here. Done. https://codereview.chromium.org/1260113002/diff/80001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:36: #include "components/suggestions/suggestions_service.h" On 2015/07/31 21:05:44, kmadhusu wrote: > This is already included in instant_service.h. You can remove this from here. Done. https://codereview.chromium.org/1260113002/diff/80001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:58: const char kLocalNTPSuggestionService[] = "LocalNTPSuggestionsService"; On 2015/07/31 21:05:44, kmadhusu wrote: > style nit: Add a blank line before this. Done. https://codereview.chromium.org/1260113002/diff/80001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:64: kLocalNTPSuggestionServiceEnabled, base::CompareCase::INSENSITIVE_ASCII); On 2015/07/31 21:05:44, kmadhusu wrote: > Include base/strings/string_util.h for CompareCase. Done. https://codereview.chromium.org/1260113002/diff/80001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:65: } On 2015/07/31 21:05:44, kmadhusu wrote: > style nit: Add a blank line after this. Done. https://codereview.chromium.org/1260113002/diff/80001/chrome/browser/search/i... chrome/browser/search/instant_service.cc:320: item.thumbnail = GURL(suggestion.thumbnail()); On 2015/07/31 21:05:45, kmadhusu wrote: > nit: Its better to check if suggestion.has_thumbnail() before assigning the > value to item.thumbnail. > > (same for favicon_url). Done. https://codereview.chromium.org/1260113002/diff/80001/chrome/renderer/searchb... File chrome/renderer/searchbox/searchbox_extension.cc (right): https://codereview.chromium.org/1260113002/diff/80001/chrome/renderer/searchb... chrome/renderer/searchbox/searchbox_extension.cc:161: thumbs->Set(0, UTF8ToV8String(isolate, base::StringPrintf( On 2015/07/31 21:05:45, kmadhusu wrote: > nit: Create a helper function to generate this thumbnail url. It will be much > easier to read this code. Done.
The CQ bit was checked by fserb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, wfh@chromium.org, kmadhusu@chromium.org Link to the patchset: https://codereview.chromium.org/1260113002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1260113002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1260113002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by fserb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1260113002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1260113002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a485e91eb08876ca79f1acc968a7e050a5c45596 Cr-Commit-Position: refs/heads/master@{#341958} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
