Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2668943002:
provide static popular sites for first run (Closed)
Descriptionprovide static popular sites for first run
For users with slow internet, a selection of popular sites that come
with the binary can improve the first run experience.
BUG=672939
Review-Url: https://codereview.chromium.org/2668943002
Cr-Commit-Position: refs/heads/master@{#450637}
Committed: https://chromium.googlesource.com/chromium/src/+/7d16fb2132a21ea413ecaca45e0e4b4570594c12
Patch Set 1 #Patch Set 2 : Resolving huge merge conflicts. #Patch Set 3 : Reverted changed callback testing. #Patch Set 4 : Move default site definition into resource file #
Total comments: 20
Patch Set 5 : Remove unnecessary rebuild. Fix tests. #
Total comments: 4
Patch Set 6 : Revert test changes. Construction of PopularSites is always safe. #
Total comments: 8
Patch Set 7 : Add dependencies. Protect access with field_trial #
Total comments: 2
Patch Set 8 : most_visited_unittest registers prefs properly. #
Total comments: 11
Patch Set 9 : Clean build/DEPS files #
Messages
Total messages: 52 (27 generated)
fhorschig@chromium.org changed reviewers: + sfiera@chromium.org
Hi Chris, would you please take a look?
https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/re... File components/ntp_tiles/resources/default_popular_sites.json (right): https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/re... components/ntp_tiles/resources/default_popular_sites.json:38: "title": "Electronics, Cars, Fashion, Collectibles, Coupons and More | eBay", drive-by: could we use short titles for all of these?
https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/re... File components/ntp_tiles/resources/default_popular_sites.json (right): https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/re... components/ntp_tiles/resources/default_popular_sites.json:38: "title": "Electronics, Cars, Fashion, Collectibles, Coupons and More | eBay", On 2017/02/09 11:09:01, Michael van Ouwerkerk wrote: > drive-by: could we use short titles for all of these? Good question. I am quite indifferent. To provide a little context: this file is really just a snapshot of https://www.gstatic.com/chrome/ntp/suggested_sites_DEFAULT_5.json That means, as soon as the real thing was loaded, the long titles would be replacing the short titles again. If we go for the nicest first run, we can take the short_urls. If we want consistent names for the same suggestions, we should wait. (There is a P1 bug to use short_name from manifests: https://crbug.com/688341 as soon as we do that, I would second using them here, too. There should be a smart way to associate the url with the short_name, so maybe it is a good idea to the same mechanism for these short title as for the planned one.)
On 2017/02/09 12:21:36, fhorschig wrote: > https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/re... > File components/ntp_tiles/resources/default_popular_sites.json (right): > > https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/re... > components/ntp_tiles/resources/default_popular_sites.json:38: "title": > "Electronics, Cars, Fashion, Collectibles, Coupons and More | eBay", > On 2017/02/09 11:09:01, Michael van Ouwerkerk wrote: > > drive-by: could we use short titles for all of these? > > Good question. I am quite indifferent. > > To provide a little context: this file is really just a snapshot of > https://www.gstatic.com/chrome/ntp/suggested_sites_DEFAULT_5.json > That means, as soon as the real thing was loaded, the long titles would > be replacing the short titles again. > > If we go for the nicest first run, we can take the short_urls. > If we want consistent names for the same suggestions, we should wait. > > (There is a P1 bug to use short_name from manifests: https://crbug.com/688341 > as soon as we do that, I would second using them here, too. > There should be a smart way to associate the url with the short_name, so > maybe it is a good idea to the same mechanism for these short title as for > the planned one.) We should probably leave the discussion on short titles out of this CL then :-) Thanks!
On 2017/02/09 12:23:39, Michael van Ouwerkerk wrote: > On 2017/02/09 12:21:36, fhorschig wrote: > > > https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/re... > > File components/ntp_tiles/resources/default_popular_sites.json (right): > > > > > https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/re... > > components/ntp_tiles/resources/default_popular_sites.json:38: "title": > > "Electronics, Cars, Fashion, Collectibles, Coupons and More | eBay", > > On 2017/02/09 11:09:01, Michael van Ouwerkerk wrote: > > > drive-by: could we use short titles for all of these? > > > > Good question. I am quite indifferent. > > > > To provide a little context: this file is really just a snapshot of > > https://www.gstatic.com/chrome/ntp/suggested_sites_DEFAULT_5.json > > That means, as soon as the real thing was loaded, the long titles would > > be replacing the short titles again. > > > > If we go for the nicest first run, we can take the short_urls. > > If we want consistent names for the same suggestions, we should wait. > > > > (There is a P1 bug to use short_name from manifests: https://crbug.com/688341 > > as soon as we do that, I would second using them here, too. > > There should be a smart way to associate the url with the short_name, so > > maybe it is a good idea to the same mechanism for these short title as for > > the planned one.) > > We should probably leave the discussion on short titles out of this CL then :-) > Thanks! Agreed. We should only make sure to have short-titles included in M58 -- do you mind filing a bug for this so that we don't forget?
https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/mo... File components/ntp_tiles/most_visited_sites.cc (right): https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/mo... components/ntp_tiles/most_visited_sites.cc:425: if (current_tiles_.empty()) { Can this actually happen? For a new user, we should already have been able to come up with eight tiles. https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/po... File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/po... components/ntp_tiles/popular_sites_impl.cc:136: if (!value) { ListValue::From() accepts nullptr, returning nullptr in that case, so this could just be a DCHECK(). I would like to see us DCHECK() that we got a list value too. You could combine that all into: std::unique_ptr<base::ListValue> list_value = ListValue::From(value); DCHECK(list_value); return list_value; https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/po... components/ntp_tiles/popular_sites_impl.cc:140: return base::ListValue::From(std::move(value)).release(); Return a std::unique_ptr<> and have the caller release(). https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/po... components/ntp_tiles/popular_sites_impl.cc:195: // Assuming the prefs were cleared, there would be no popular sites. I don't understand this comment. - What do you mean by "Assuming…"? Do you mean "If…"? - If the prefs are cleared, then we'd get the default value, right? (which is not empty) I think the case of json being null is not one we have to handle, is it? https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/po... components/ntp_tiles/popular_sites_impl.cc:198: sites_ = ParseSiteList(*json); Any reason we wouldn't want to do this in the constructor, or even just in the site list's accessor? https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/po... File components/ntp_tiles/popular_sites_impl_unittest.cc (right): https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/po... components/ntp_tiles/popular_sites_impl_unittest.cc:159: I think I'd like to see a test of what sites() returns before StartFetch() is called. Currently an empty list, but it could be the cached sites. https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/re... File components/ntp_tiles/resources/default_popular_sites.json (right): https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/re... components/ntp_tiles/resources/default_popular_sites.json:38: "title": "Electronics, Cars, Fashion, Collectibles, Coupons and More | eBay", On 2017/02/09 12:21:36, fhorschig wrote: > On 2017/02/09 11:09:01, Michael van Ouwerkerk wrote: > > drive-by: could we use short titles for all of these? > > Good question. I am quite indifferent. > > To provide a little context: this file is really just a snapshot of > https://www.gstatic.com/chrome/ntp/suggested_sites_DEFAULT_5.json > That means, as soon as the real thing was loaded, the long titles would > be replacing the short titles again. > > If we go for the nicest first run, we can take the short_urls. > If we want consistent names for the same suggestions, we should wait. > > (There is a P1 bug to use short_name from manifests: https://crbug.com/688341 > as soon as we do that, I would second using them here, too. > There should be a smart way to associate the url with the short_name, so > maybe it is a good idea to the same mechanism for these short title as for > the planned one.) I think we should keep this file in sync with the real DEFAULT_5.json. https://codereview.chromium.org/2668943002/diff/60001/components/resources/nt... File components/resources/ntp_tiles_resources.grdp (right): https://codereview.chromium.org/2668943002/diff/60001/components/resources/nt... components/resources/ntp_tiles_resources.grdp:8: <include name="IDR_DEFAULT_POPULAR_SITES_JSON" file="../ntp_tiles/resources/default_popular_sites.json" type="BINDATA" /> Inside the android/ios block.
On 2017/02/09 12:27:15, tschumann wrote: > On 2017/02/09 12:23:39, Michael van Ouwerkerk wrote: > > On 2017/02/09 12:21:36, fhorschig wrote: > > > > > > https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/re... > > > File components/ntp_tiles/resources/default_popular_sites.json (right): > > > > > > > > > https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/re... > > > components/ntp_tiles/resources/default_popular_sites.json:38: "title": > > > "Electronics, Cars, Fashion, Collectibles, Coupons and More | eBay", > > > On 2017/02/09 11:09:01, Michael van Ouwerkerk wrote: > > > > drive-by: could we use short titles for all of these? > > > > > > Good question. I am quite indifferent. > > > > > > To provide a little context: this file is really just a snapshot of > > > https://www.gstatic.com/chrome/ntp/suggested_sites_DEFAULT_5.json > > > That means, as soon as the real thing was loaded, the long titles would > > > be replacing the short titles again. > > > > > > If we go for the nicest first run, we can take the short_urls. > > > If we want consistent names for the same suggestions, we should wait. > > > > > > (There is a P1 bug to use short_name from manifests: > https://crbug.com/688341 > > > as soon as we do that, I would second using them here, too. > > > There should be a smart way to associate the url with the short_name, so > > > maybe it is a good idea to the same mechanism for these short title as for > > > the planned one.) > > > > We should probably leave the discussion on short titles out of this CL then > :-) > > Thanks! > > Agreed. We should only make sure to have short-titles included in M58 -- do you > mind filing a bug for this so that we don't forget? Sure: https://crbug.com/690449
The MostLikely unittests had to be changed. They assumed that the registration of popular sites prefs depend on whether the feature is enabled. This assumption is wrong, especially if you try to instantiate the PopularSitesImpl in either case. Also, by removing the conditional rebuild, it is questionable whether we still need the callback. Besides the internal page and unittests, nobody seems to use it. https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/mo... File components/ntp_tiles/most_visited_sites.cc (right): https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/mo... components/ntp_tiles/most_visited_sites.cc:425: if (current_tiles_.empty()) { On 2017/02/09 12:39:06, sfiera wrote: > Can this actually happen? For a new user, we should already have been able to > come up with eight tiles. Removed completely. https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/po... File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/po... components/ntp_tiles/popular_sites_impl.cc:136: if (!value) { On 2017/02/09 12:39:06, sfiera wrote: > ListValue::From() accepts nullptr, returning nullptr in that case, so this could > just be a DCHECK(). I would like to see us DCHECK() that we got a list value > too. You could combine that all into: > > std::unique_ptr<base::ListValue> list_value = ListValue::From(value); > DCHECK(list_value); > return list_value; This is what I was looking for, thank you! https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/po... components/ntp_tiles/popular_sites_impl.cc:140: return base::ListValue::From(std::move(value)).release(); On 2017/02/09 12:39:06, sfiera wrote: > Return a std::unique_ptr<> and have the caller release(). Done. https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/po... components/ntp_tiles/popular_sites_impl.cc:195: // Assuming the prefs were cleared, there would be no popular sites. On 2017/02/09 12:39:06, sfiera wrote: > I don't understand this comment. > - What do you mean by "Assuming…"? Do you mean "If…"? > - If the prefs are cleared, then we'd get the default value, right? (which is > not empty) > > I think the case of json being null is not one we have to handle, is it? Thank you for the clarification, I was assuming the defaults could be cleared as well. In this case, json is never null. https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/po... components/ntp_tiles/popular_sites_impl.cc:198: sites_ = ParseSiteList(*json); On 2017/02/09 12:39:06, sfiera wrote: > Any reason we wouldn't want to do this in the constructor, or even just in the > site list's accessor? Doing this in the constructor sounds good. Parsing in the accessor would have no advantages. Refetching in the accessor would be a pretty huge side effect. https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/po... File components/ntp_tiles/popular_sites_impl_unittest.cc (right): https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/po... components/ntp_tiles/popular_sites_impl_unittest.cc:159: On 2017/02/09 12:39:06, sfiera wrote: > I think I'd like to see a test of what sites() returns before StartFetch() is > called. Currently an empty list, but it could be the cached sites. Good idea, especially when reading prefs in the ctr. https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/re... File components/ntp_tiles/resources/default_popular_sites.json (right): https://codereview.chromium.org/2668943002/diff/60001/components/ntp_tiles/re... components/ntp_tiles/resources/default_popular_sites.json:38: "title": "Electronics, Cars, Fashion, Collectibles, Coupons and More | eBay", On 2017/02/09 12:39:06, sfiera wrote: > On 2017/02/09 12:21:36, fhorschig wrote: > > On 2017/02/09 11:09:01, Michael van Ouwerkerk wrote: > > > drive-by: could we use short titles for all of these? > > > > Good question. I am quite indifferent. > > > > To provide a little context: this file is really just a snapshot of > > https://www.gstatic.com/chrome/ntp/suggested_sites_DEFAULT_5.json > > That means, as soon as the real thing was loaded, the long titles would > > be replacing the short titles again. > > > > If we go for the nicest first run, we can take the short_urls. > > If we want consistent names for the same suggestions, we should wait. > > > > (There is a P1 bug to use short_name from manifests: https://crbug.com/688341 > > as soon as we do that, I would second using them here, too. > > There should be a smart way to associate the url with the short_name, so > > maybe it is a good idea to the same mechanism for these short title as for > > the planned one.) > > I think we should keep this file in sync with the real DEFAULT_5.json. +1 (if possible) https://codereview.chromium.org/2668943002/diff/60001/components/resources/nt... File components/resources/ntp_tiles_resources.grdp (right): https://codereview.chromium.org/2668943002/diff/60001/components/resources/nt... components/resources/ntp_tiles_resources.grdp:8: <include name="IDR_DEFAULT_POPULAR_SITES_JSON" file="../ntp_tiles/resources/default_popular_sites.json" type="BINDATA" /> On 2017/02/09 12:39:06, sfiera wrote: > Inside the android/ios block. It used to be there. I would prefer it outside. That way, component_unittests can easily verify the contents of this file.
On 2017/02/09 15:30:18, fhorschig wrote: > The MostLikely unittests had to be changed. > They assumed that the registration of popular sites > prefs depend on whether the feature is enabled. > This assumption is wrong, especially if you try to > instantiate the PopularSitesImpl in either case. I think the unittests were correct and MostVisitedSites needs to be changed to not query PopularSites when they're disabled. You can check on a device. > Also, by removing the conditional rebuild, it is > questionable whether we still need the callback. > Besides the internal page and unittests, nobody > seems to use it. Let's keep it for now. I think it's the best option for the internals page, but it could be optional. https://codereview.chromium.org/2668943002/diff/60001/components/resources/nt... File components/resources/ntp_tiles_resources.grdp (right): https://codereview.chromium.org/2668943002/diff/60001/components/resources/nt... components/resources/ntp_tiles_resources.grdp:8: <include name="IDR_DEFAULT_POPULAR_SITES_JSON" file="../ntp_tiles/resources/default_popular_sites.json" type="BINDATA" /> On 2017/02/09 15:30:18, fhorschig wrote: > On 2017/02/09 12:39:06, sfiera wrote: > > Inside the android/ios block. > > It used to be there. I would prefer it outside. > That way, component_unittests can easily verify the contents of this file. I would rather restrict the unit test to android/ios, but you're welcome to get a second opinion. https://codereview.chromium.org/2668943002/diff/80001/components/ntp_tiles/mo... File components/ntp_tiles/most_visited_sites_unittest.cc (right): https://codereview.chromium.org/2668943002/diff/80001/components/ntp_tiles/mo... components/ntp_tiles/most_visited_sites_unittest.cc:234: if (!enabled_in_variations_) { I don't think this change is correct. I think that even when disabled by variations, popular sites will be non-null. You should change CreatePopularSitesTiles() to check variations and leave the test unchanged. https://codereview.chromium.org/2668943002/diff/80001/components/ntp_tiles/po... File components/ntp_tiles/popular_sites_impl_unittest.cc (right): https://codereview.chromium.org/2668943002/diff/80001/components/ntp_tiles/po... components/ntp_tiles/popular_sites_impl_unittest.cc:255: ASSERT_TRUE(save_success.has_value()); I would just start with "bool save_success = false". Then, EXPECT_TRUE is enough, and you don't need an ASSERT that would potentially block the erst of the test body.
https://codereview.chromium.org/2668943002/diff/60001/components/resources/nt... File components/resources/ntp_tiles_resources.grdp (right): https://codereview.chromium.org/2668943002/diff/60001/components/resources/nt... components/resources/ntp_tiles_resources.grdp:8: <include name="IDR_DEFAULT_POPULAR_SITES_JSON" file="../ntp_tiles/resources/default_popular_sites.json" type="BINDATA" /> On 2017/02/10 10:40:03, sfiera wrote: > On 2017/02/09 15:30:18, fhorschig wrote: > > On 2017/02/09 12:39:06, sfiera wrote: > > > Inside the android/ios block. > > > > It used to be there. I would prefer it outside. > > That way, component_unittests can easily verify the contents of this file. > > I would rather restrict the unit test to android/ios, but you're welcome to get > a second opinion. Done. The unittest checks the number of tiles for mobile only. https://codereview.chromium.org/2668943002/diff/80001/components/ntp_tiles/mo... File components/ntp_tiles/most_visited_sites_unittest.cc (right): https://codereview.chromium.org/2668943002/diff/80001/components/ntp_tiles/mo... components/ntp_tiles/most_visited_sites_unittest.cc:234: if (!enabled_in_variations_) { On 2017/02/10 10:40:03, sfiera wrote: > I don't think this change is correct. I think that even when disabled by > variations, popular sites will be non-null. You should change > CreatePopularSitesTiles() to check variations and leave the test unchanged. Undone. The whole file is untouched. The problem was not calling popular sites but constructing it. https://codereview.chromium.org/2668943002/diff/80001/components/ntp_tiles/po... File components/ntp_tiles/popular_sites_impl_unittest.cc (right): https://codereview.chromium.org/2668943002/diff/80001/components/ntp_tiles/po... components/ntp_tiles/popular_sites_impl_unittest.cc:255: ASSERT_TRUE(save_success.has_value()); On 2017/02/10 10:40:03, sfiera wrote: > I would just start with "bool save_success = false". Then, EXPECT_TRUE is > enough, and you don't need an ASSERT that would potentially block the erst of > the test body. Done.
The CQ bit was checked by fhorschig@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2668943002/diff/100001/components/ntp_tiles/p... File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2668943002/diff/100001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:133: if (command_line->HasSwitch(ntp_tiles::switches::kDisableNTPPopularSites)) { This check is not sufficient; use field_trial.h. Also, it's better to make this check in MostVisitedSites::CreatePopularSitesTiles(), because if the user doesn't need popular sites (they already have enough personalized tiles), we don't even want to check if they're needed. Checking Finch could opt them into an experimental or control group, and they're not really either. https://codereview.chromium.org/2668943002/diff/100001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:141: #if defined(OS_IOS) || defined(OS_ANDROID) Alphabetically, Android is before iOS. Just sayin'. https://codereview.chromium.org/2668943002/diff/100001/components/ntp_tiles/p... File components/ntp_tiles/popular_sites_impl_unittest.cc (right): https://codereview.chromium.org/2668943002/diff/100001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl_unittest.cc:59: #endif I'd put "return 0" in an #else because it would otherwise look like dead code on ios/android.
The CQ bit was checked by fhorschig@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...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #7 (id:120001) has been deleted
Patchset #7 (id:140001) has been deleted
Patchset #7 (id:160001) has been deleted
https://codereview.chromium.org/2668943002/diff/100001/components/ntp_tiles/p... File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2668943002/diff/100001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:133: if (command_line->HasSwitch(ntp_tiles::switches::kDisableNTPPopularSites)) { On 2017/02/13 10:58:01, sfiera wrote: > This check is not sufficient; use field_trial.h. Done. > > Also, it's better to make this check in > MostVisitedSites::CreatePopularSitesTiles(), because if the user doesn't need > popular sites (they already have enough personalized tiles), we don't even want > to check if they're needed. Checking Finch could opt them into an experimental > or control group, and they're not really either. Okay, I checked it there, too. But the check here has to remain. For some reason, the most_visited_unittests wants to instantiate this class without registering any prefs (which can not happen). https://codereview.chromium.org/2668943002/diff/100001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:141: #if defined(OS_IOS) || defined(OS_ANDROID) On 2017/02/13 10:58:01, sfiera wrote: > Alphabetically, Android is before iOS. Just sayin'. Done. https://codereview.chromium.org/2668943002/diff/100001/components/ntp_tiles/p... File components/ntp_tiles/popular_sites_impl_unittest.cc (right): https://codereview.chromium.org/2668943002/diff/100001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl_unittest.cc:59: #endif On 2017/02/13 10:58:01, sfiera wrote: > I'd put "return 0" in an #else because it would otherwise look like dead code on > ios/android. Done.
https://codereview.chromium.org/2668943002/diff/100001/components/ntp_tiles/p... File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2668943002/diff/100001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:133: if (command_line->HasSwitch(ntp_tiles::switches::kDisableNTPPopularSites)) { On 2017/02/13 14:57:00, fhorschig wrote: > On 2017/02/13 10:58:01, sfiera wrote: > > This check is not sufficient; use field_trial.h. > Done. > > > > > Also, it's better to make this check in > > MostVisitedSites::CreatePopularSitesTiles(), because if the user doesn't need > > popular sites (they already have enough personalized tiles), we don't even > want > > to check if they're needed. Checking Finch could opt them into an experimental > > or control group, and they're not really either. > > Okay, I checked it there, too. But the check here has to remain. > For some reason, the most_visited_unittests wants to instantiate > this class without registering any prefs (which can not happen). If the test is going to instantiate the class, it should register the prefs too. https://codereview.chromium.org/2668943002/diff/180001/components/ntp_tiles/m... File components/ntp_tiles/most_visited_sites.cc (right): https://codereview.chromium.org/2668943002/diff/180001/components/ntp_tiles/m... components/ntp_tiles/most_visited_sites.cc:330: !ShouldShowPopularSites()) I think this should be in the lower if-statement, after we've determined if we would need to backfill with popular sites. Otherwise, calling ShouldShowPopularSites() could put a user into a control or experiment group when they won't actually see popular sites.
https://codereview.chromium.org/2668943002/diff/100001/components/ntp_tiles/p... File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2668943002/diff/100001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:133: if (command_line->HasSwitch(ntp_tiles::switches::kDisableNTPPopularSites)) { On 2017/02/13 15:05:54, sfiera wrote: > On 2017/02/13 14:57:00, fhorschig wrote: > > On 2017/02/13 10:58:01, sfiera wrote: > > > This check is not sufficient; use field_trial.h. > > Done. > > > > > > > > Also, it's better to make this check in > > > MostVisitedSites::CreatePopularSitesTiles(), because if the user doesn't > need > > > popular sites (they already have enough personalized tiles), we don't even > > want > > > to check if they're needed. Checking Finch could opt them into an > experimental > > > or control group, and they're not really either. > > > > Okay, I checked it there, too. But the check here has to remain. > > For some reason, the most_visited_unittests wants to instantiate > > this class without registering any prefs (which can not happen). > > If the test is going to instantiate the class, it should register the prefs too. Done. https://codereview.chromium.org/2668943002/diff/180001/components/ntp_tiles/m... File components/ntp_tiles/most_visited_sites.cc (right): https://codereview.chromium.org/2668943002/diff/180001/components/ntp_tiles/m... components/ntp_tiles/most_visited_sites.cc:330: !ShouldShowPopularSites()) On 2017/02/13 15:05:54, sfiera wrote: > I think this should be in the lower if-statement, after we've determined if we > would need to backfill with popular sites. Otherwise, calling > ShouldShowPopularSites() could put a user into a control or experiment group > when they won't actually see popular sites. Okay.
LGTM
The CQ bit was checked by fhorschig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by fhorschig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
fhorschig@chromium.org changed reviewers: + avi@chromium.org, sdefresne@chromium.org
Hi avi@ and sdefresne@, would you please have a look at my CL? It uses resources in popular_sites_impl.cc that depend on grit and the resource bundle.
lgtm https://codereview.chromium.org/2668943002/diff/200001/components/ntp_tiles/B... File components/ntp_tiles/BUILD.gn (right): https://codereview.chromium.org/2668943002/diff/200001/components/ntp_tiles/B... components/ntp_tiles/BUILD.gn:56: "//components/resources:components_resources", "//components/resources", https://codereview.chromium.org/2668943002/diff/200001/components/ntp_tiles/B... components/ntp_tiles/BUILD.gn:61: "//ui/base:base", "//ui/base", https://codereview.chromium.org/2668943002/diff/200001/components/ntp_tiles/DEPS File components/ntp_tiles/DEPS (right): https://codereview.chromium.org/2668943002/diff/200001/components/ntp_tiles/D... components/ntp_tiles/DEPS:12: "+components/resources:components_resources", There is no directory named components/resources:components_resources. I guess you copied this line from "gn check" output and it does not belong in this file. Please remove. https://codereview.chromium.org/2668943002/diff/200001/components/ntp_tiles/p... File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2668943002/diff/200001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:135: // Creates the List of popular sites based on a snapshot available for mobile. s/List/list/ https://codereview.chromium.org/2668943002/diff/200001/components/ntp_tiles/r... File components/ntp_tiles/resources/default_popular_sites.json (right): https://codereview.chromium.org/2668943002/diff/200001/components/ntp_tiles/r... components/ntp_tiles/resources/default_popular_sites.json:5: "large_icon_url": "https://static.xx.fbcdn.net/rsrc.php/v3/ya/r/O2aKM2iSbOw.png" How would large_icon_url be kept up-to-date? I'm afraid they will become invalid really quickly.
https://codereview.chromium.org/2668943002/diff/200001/components/ntp_tiles/r... File components/ntp_tiles/resources/default_popular_sites.json (right): https://codereview.chromium.org/2668943002/diff/200001/components/ntp_tiles/r... components/ntp_tiles/resources/default_popular_sites.json:5: "large_icon_url": "https://static.xx.fbcdn.net/rsrc.php/v3/ya/r/O2aKM2iSbOw.png" On 2017/02/14 12:37:35, sdefresne wrote: > How would large_icon_url be kept up-to-date? I'm afraid they will become invalid > really quickly. Note: I know this is unactionable feedback, so I'm fine with the CL landing without any fix for that, just curious how you plan to keep those URLs up-to-date.
Resources LGTM
https://codereview.chromium.org/2668943002/diff/200001/components/ntp_tiles/B... File components/ntp_tiles/BUILD.gn (right): https://codereview.chromium.org/2668943002/diff/200001/components/ntp_tiles/B... components/ntp_tiles/BUILD.gn:56: "//components/resources:components_resources", On 2017/02/14 12:37:35, sdefresne wrote: > "//components/resources", Done. https://codereview.chromium.org/2668943002/diff/200001/components/ntp_tiles/B... components/ntp_tiles/BUILD.gn:61: "//ui/base:base", On 2017/02/14 12:37:35, sdefresne wrote: > "//ui/base", Done. https://codereview.chromium.org/2668943002/diff/200001/components/ntp_tiles/DEPS File components/ntp_tiles/DEPS (right): https://codereview.chromium.org/2668943002/diff/200001/components/ntp_tiles/D... components/ntp_tiles/DEPS:12: "+components/resources:components_resources", On 2017/02/14 12:37:35, sdefresne wrote: > There is no directory named components/resources:components_resources. I guess > you copied this line from "gn check" output and it does not belong in this file. > Please remove. Gone. https://codereview.chromium.org/2668943002/diff/200001/components/ntp_tiles/p... File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2668943002/diff/200001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:135: // Creates the List of popular sites based on a snapshot available for mobile. On 2017/02/14 12:37:35, sdefresne wrote: > s/List/list/ Done. https://codereview.chromium.org/2668943002/diff/200001/components/ntp_tiles/r... File components/ntp_tiles/resources/default_popular_sites.json (right): https://codereview.chromium.org/2668943002/diff/200001/components/ntp_tiles/r... components/ntp_tiles/resources/default_popular_sites.json:5: "large_icon_url": "https://static.xx.fbcdn.net/rsrc.php/v3/ya/r/O2aKM2iSbOw.png" On 2017/02/14 12:38:35, sdefresne wrote: > On 2017/02/14 12:37:35, sdefresne wrote: > > How would large_icon_url be kept up-to-date? I'm afraid they will become > invalid > > really quickly. > > Note: I know this is unactionable feedback, so I'm fine with the CL landing > without any fix for that, just curious how you plan to keep those URLs > up-to-date. The question is valid. The decision was to update it manually in the first iteration and provide a script in the follow-up CL [1] which then downloads the latest, minified version and associated icons. We plan on scheduling a rotation for updating this frequently. [1] https://codereview.chromium.org/2695713004/
The CQ bit was checked by fhorschig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by fhorschig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, sfiera@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2668943002/#ps220001 (title: "Clean build/DEPS files")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1487146956429040,
"parent_rev": "0dd58d4d47f012cdbcce409e6f3716118d364245", "commit_rev":
"7d16fb2132a21ea413ecaca45e0e4b4570594c12"}
Message was sent while issue was closed.
Description was changed from ========== provide static popular sites for first run For users with slow internet, a selection of popular sites that come with the binary can improve the first run experience. BUG=672939 ========== to ========== provide static popular sites for first run For users with slow internet, a selection of popular sites that come with the binary can improve the first run experience. BUG=672939 Review-Url: https://codereview.chromium.org/2668943002 Cr-Commit-Position: refs/heads/master@{#450637} Committed: https://chromium.googlesource.com/chromium/src/+/7d16fb2132a21ea413ecaca45e0e... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:220001) as https://chromium.googlesource.com/chromium/src/+/7d16fb2132a21ea413ecaca45e0e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
