Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2388783004:
Ensure PopularSite icon availability in ntp_tiles. (Closed)
|
Created:
4 years, 2 months ago by sfiera Modified:
4 years, 1 month ago CC:
chromium-reviews, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, droger+watchlist_chromium.org, ntp-dev+reviews_chromium.org, agrieve+watch_chromium.org, noyau (Ping after 24h) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnsure PopularSite icon availability in ntp_tiles.
Previously, MostVisitedSites dumped all PopularSites information into
its own observer, then let Java ensure availability of icons. Popular
Sites on iOS needs these icons cached too, so the component should
handle icon availability itself.
MostVisitedSites::Observer::OnPopularURLsAvailable() is removed and
MostVisitedSites::Observer::OnIconMadeAvailable() is added in its place.
Note that FaviconHelper.ensureIconIsAvailable() hasn't been removed in
this CL, as it's still used to load favicons for snippets. Of course,
one day, iOS will need to do that too, so an eventual plan to share
IconCacher and broaden its interface makes sense (maybe coupled with a
move to the favicon component?).
BUG=631990
Committed: https://crrev.com/ae1ae693f77cfef75097263227b958389cae471b
Cr-Commit-Position: refs/heads/master@{#427100}
Patch Set 1 #Patch Set 2 : Out-of-line destructor. #Patch Set 3 : Remove no-longer-needed code. #Patch Set 4 : Merge branch 'refs/heads/master' into icons #Patch Set 5 : Update iOS factory. #Patch Set 6 : Update iOS factory. #Patch Set 7 : Fix comments, variable names. #
Total comments: 56
Patch Set 8 : Address review comments. #Patch Set 9 : Fetch popular site icons only when needed. #
Total comments: 5
Patch Set 10 : Add DCHECK. #Patch Set 11 : Merge branch 'master' into icons #Messages
Total messages: 59 (41 generated)
The CQ bit was checked by sfiera@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by sfiera@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 checked by sfiera@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...
A preview. I need to look over this more before a proper review, but that won't be until next week.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
The CQ bit was checked by sfiera@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by sfiera@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...
Description was changed from ========== NOT YET READY FOR REVIEW Ensure PopularSite icon availability in ntp_tiles. Previously, MostVisitedSites dumped all PopularSites information into its own observer, then let Java ensure availability of icons. Popular Sites on iOS needs these icons cached too, so the component should handle icon availability itself. MostVisitedSites::Observer::OnPopularURLsAvailable() is removed and MostVisitedSites::Observer::OnIconMadeAvailable() is added in its place. Note that FaviconHelper.ensureIconIsAvailable() hasn't been removed in this CL, as it's still used to load favicons for snippets. Of course, one day, iOS will need to do that too, so an eventual plan to share IconCacher and broaden its interface makes sense (maybe coupled with a move to the favicon component?). BUG=631990 ========== to ========== Ensure PopularSite icon availability in ntp_tiles. Previously, MostVisitedSites dumped all PopularSites information into its own observer, then let Java ensure availability of icons. Popular Sites on iOS needs these icons cached too, so the component should handle icon availability itself. MostVisitedSites::Observer::OnPopularURLsAvailable() is removed and MostVisitedSites::Observer::OnIconMadeAvailable() is added in its place. Note that FaviconHelper.ensureIconIsAvailable() hasn't been removed in this CL, as it's still used to load favicons for snippets. Of course, one day, iOS will need to do that too, so an eventual plan to share IconCacher and broaden its interface makes sense (maybe coupled with a move to the favicon component?). BUG=631990 ==========
sfiera@chromium.org changed reviewers: + noyau@chromium.org, treib@chromium.org
Ready for a proper review now. I elected to fetch the icons for all popular sites, instead of trying to limit it to the 1-7 selected for inclusion on the NTP, because I don't think it matters much. For old users (who often have 8 tiles already) we're not fetching popular sites at all. For new users, we will fetch 10 icons instead of the necessary 8. However, I just checked UMA for NewTabPage.NumberOfTiles, and to my surprise, more than half of stable users are in-betweeners (1-7 tiles), so maybe I shouldn't be so cavalier. Éric, this is almost enough for iOS, but I don't know how to use the iOS simulator to reproduce the main remaining problem. Is there a way to start the simulator without internet access, then turn it on later? I suppose I should get myself a real device while I'm at it.
sfiera@chromium.org changed reviewers: - noyau@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Bunch of comments, all small. However, I'd indeed be happier if we could retain the "fetch only on demand" behavior. But I see how that'd be annoying to pipe through... hrm. https://codereview.chromium.org/2388783004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java (right): https://codereview.chromium.org/2388783004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java:38: * @param faviconUrls Array of URLs for the corresponding favicons (if known). Please update the comment https://codereview.chromium.org/2388783004/diff/120001/chrome/browser/android... File chrome/browser/android/ntp/most_visited_sites_bridge.cc (right): https://codereview.chromium.org/2388783004/diff/120001/chrome/browser/android... chrome/browser/android/ntp/most_visited_sites_bridge.cc:171: ServiceAccessType::IMPLICIT_ACCESS), This will return nullptr in incognito, and IconCacher doesn't seem to handle that. We prooobably can't get here in incognito? But I'd prefer that to be more explicit, i.e. a DCHECK or something. https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher.cc (right): https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.cc:7: #include "base/callback.h" No need to repeat all the includes from the header https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.cc:15: #include "ui/gfx/image/image_skia.h" // DO_NOT_SUBMIT :) https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.cc:27: data_use_measurement::DataUseUserData::NTP_TILES); :) There's a TODO to add just this in popular_sites.cc, you can remove that now! https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.cc:37: weak_ptr_factory_.GetWeakPtr(), std::move(site), done), WeakPtr isn't necessary I think: the CancelableTaskTracker should make sure we're not called back if we get destroyed https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.cc:61: "", IconURL(site), nit: s/""/std::string()/ https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.cc:63: weak_ptr_factory_.GetWeakPtr(), site, done)); WeakPtr isn't necessary I think (we own the ImageFetcher) https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher.h (right): https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:26: class IconCacher { Hm, I'm not entirely happy with the name - caching isn't really what this class does. FaviconFetcher maybe? https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:29: std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher); #include <memory> https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:34: void StartFetch(PopularSites::Site site, base::Callback<void(bool)> done); Pass the callback by const ref (to avoid some unnecessary copies). optional: You could also add an alias for it. https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:41: base::WeakPtrFactory<IconCacher> weak_ptr_factory_; Methods come before member variables https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:45: static const GURL& IconURL(const PopularSites::Site& site); Are these needed by tests? If not, you could move them into an anonymous namespace in the .cc https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:49: base::Callback<void(bool)> done, Also here: const ref https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:53: base::Callback<void(bool)> done, And here https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher_unittest.cc (right): https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_unittest.cc:48: favicon_service_(nullptr, &history_service_) { /*favicon_client=*/nullptr https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_unittest.cc:49: CHECK(history_dir_.CreateUniqueTempDir()); Can this be ASSERT_TRUE rather than CHECK, so the test will fail gracefully rather than crashing? https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_unittest.cc:51: history::HistoryDatabaseParams(history_dir_.GetPath(), 0, 0))); misaligned? https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_unittest.cc:121: IconCacher cacher(&favicon_service_, base::WrapUnique(image_fetcher_)); The WrapUnique is quite unsafe - it needs to happen exactly once per test, otherwise we'll either leak memory or delete more than once. Can you make the ImageFetcher a local variable in the test? Then I'd be more comfortable with retaining the raw pointer after passing out ownership. https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/m... File components/ntp_tiles/most_visited_sites.cc (right): https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/m... components/ntp_tiles/most_visited_sites.cc:533: weak_ptr_factory_.GetWeakPtr(), site.url)); Is the WeakPtr necessary? Since we own the IconCacher, it'll get destroyed when we get destroyed, so presumably it won't try to call us back anymore? https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/m... File components/ntp_tiles/most_visited_sites.h (right): https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/m... components/ntp_tiles/most_visited_sites.h:150: void OnIconMadeAvailable(const GURL& site_url, bool newly_available); nit: Maybe move this below OnPopularSitesAvailable?
Ah, also: You asked Eric something, but he isn't actually on the reviewer list :)
The CQ bit was checked by sfiera@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 checked by sfiera@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...
sfiera@chromium.org changed reviewers: + bauerb@chromium.org, stevenjb@chromium.org
Thanks! +stevenjb for favicon deps +bauerb for Java https://codereview.chromium.org/2388783004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java (right): https://codereview.chromium.org/2388783004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java:38: * @param faviconUrls Array of URLs for the corresponding favicons (if known). On 2016/10/12 09:11:27, Marc Treib wrote: > Please update the comment Done. https://codereview.chromium.org/2388783004/diff/120001/chrome/browser/android... File chrome/browser/android/ntp/most_visited_sites_bridge.cc (right): https://codereview.chromium.org/2388783004/diff/120001/chrome/browser/android... chrome/browser/android/ntp/most_visited_sites_bridge.cc:171: ServiceAccessType::IMPLICIT_ACCESS), On 2016/10/12 09:11:27, Marc Treib wrote: > This will return nullptr in incognito, and IconCacher doesn't seem to handle > that. We prooobably can't get here in incognito? But I'd prefer that to be more > explicit, i.e. a DCHECK or something. Is that really the case? You can still see already-cached favicons in incognito mode, so is EXPLICIT_ACCESS the right setting? (Perhaps we still shouldn't be issuing any requests on the user's behalf in incognito mode) https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher.cc (right): https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/10/12 09:11:28, Marc Treib wrote: > 2016 Done. https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.cc:7: #include "base/callback.h" On 2016/10/12 09:11:28, Marc Treib wrote: > No need to repeat all the includes from the header Took this as an opportunity to trim the includes in the header. https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.cc:15: #include "ui/gfx/image/image_skia.h" // DO_NOT_SUBMIT On 2016/10/12 09:11:28, Marc Treib wrote: > :) :X https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.cc:27: data_use_measurement::DataUseUserData::NTP_TILES); On 2016/10/12 09:11:28, Marc Treib wrote: > :) > There's a TODO to add just this in popular_sites.cc, you can remove that now! Done. https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.cc:37: weak_ptr_factory_.GetWeakPtr(), std::move(site), done), On 2016/10/12 09:11:28, Marc Treib wrote: > WeakPtr isn't necessary I think: the CancelableTaskTracker should make sure > we're not called back if we get destroyed Done. https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.cc:61: "", IconURL(site), On 2016/10/12 09:11:28, Marc Treib wrote: > nit: s/""/std::string()/ Done. https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.cc:63: weak_ptr_factory_.GetWeakPtr(), site, done)); On 2016/10/12 09:11:28, Marc Treib wrote: > WeakPtr isn't necessary I think (we own the ImageFetcher) Done. https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher.h (right): https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/10/12 09:11:28, Marc Treib wrote: > 2016 Done. https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:26: class IconCacher { On 2016/10/12 09:11:28, Marc Treib wrote: > Hm, I'm not entirely happy with the name - caching isn't really what this class > does. FaviconFetcher maybe? I don't like "Favicon" in the name, because that's a specific type of icon, and we prefer TOUCH_ICON over FAVICON if available. I'm not sure how I feel about "Fetcher". In the common case, we won't actually fetch anything, we'll just verify that the icon is cached and move on. But I guess IconFetcher would be OK with me. https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:29: std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher); On 2016/10/12 09:11:28, Marc Treib wrote: > #include <memory> Done. https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:34: void StartFetch(PopularSites::Site site, base::Callback<void(bool)> done); On 2016/10/12 09:11:28, Marc Treib wrote: > Pass the callback by const ref (to avoid some unnecessary copies). > optional: You could also add an alias for it. Done. https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:41: base::WeakPtrFactory<IconCacher> weak_ptr_factory_; On 2016/10/12 09:11:28, Marc Treib wrote: > Methods come before member variables Done. https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:45: static const GURL& IconURL(const PopularSites::Site& site); On 2016/10/12 09:11:28, Marc Treib wrote: > Are these needed by tests? If not, you could move them into an anonymous > namespace in the .cc Done. https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:49: base::Callback<void(bool)> done, On 2016/10/12 09:11:28, Marc Treib wrote: > Also here: const ref Done. https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:53: base::Callback<void(bool)> done, On 2016/10/12 09:11:28, Marc Treib wrote: > And here Done. https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher_unittest.cc (right): https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_unittest.cc:48: favicon_service_(nullptr, &history_service_) { On 2016/10/12 09:11:28, Marc Treib wrote: > /*favicon_client=*/nullptr Done. https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_unittest.cc:49: CHECK(history_dir_.CreateUniqueTempDir()); On 2016/10/12 09:11:28, Marc Treib wrote: > Can this be ASSERT_TRUE rather than CHECK, so the test will fail gracefully > rather than crashing? Not easily; ASSERT_TRUE() doesn't work well outside TEST_F() functions. Here, it would cause the constructor to return early, but I think the test would proceed, even though it would ultimately report failure. https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_unittest.cc:51: history::HistoryDatabaseParams(history_dir_.GetPath(), 0, 0))); On 2016/10/12 09:11:28, Marc Treib wrote: > misaligned? Indentation, you mean? I've run git cl format, so I think it approves. https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_unittest.cc:121: IconCacher cacher(&favicon_service_, base::WrapUnique(image_fetcher_)); On 2016/10/12 09:11:28, Marc Treib wrote: > The WrapUnique is quite unsafe - it needs to happen exactly once per test, > otherwise we'll either leak memory or delete more than once. > Can you make the ImageFetcher a local variable in the test? Then I'd be more > comfortable with retaining the raw pointer after passing out ownership. Switched to a std::unique_ptr<> in the fixture and std::move(). The latter could be done with a more explicit "TransferOwnership()" function if you'd prefer. (originally the IconCacher was also owned by the fixture) https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/m... File components/ntp_tiles/most_visited_sites.cc (right): https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/m... components/ntp_tiles/most_visited_sites.cc:533: weak_ptr_factory_.GetWeakPtr(), site.url)); On 2016/10/12 09:11:28, Marc Treib wrote: > Is the WeakPtr necessary? Since we own the IconCacher, it'll get destroyed when > we get destroyed, so presumably it won't try to call us back anymore? No, I suppose it isn't. Switched for base::Unretained. https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/m... File components/ntp_tiles/most_visited_sites.h (right): https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/m... components/ntp_tiles/most_visited_sites.h:150: void OnIconMadeAvailable(const GURL& site_url, bool newly_available); On 2016/10/12 09:11:29, Marc Treib wrote: > nit: Maybe move this below OnPopularSitesAvailable? Done.
Fetching on demand turned out to be pretty easy. Verified it by dismissing popular sites; the favicons load in place correctly.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! LGTM https://codereview.chromium.org/2388783004/diff/120001/chrome/browser/android... File chrome/browser/android/ntp/most_visited_sites_bridge.cc (right): https://codereview.chromium.org/2388783004/diff/120001/chrome/browser/android... chrome/browser/android/ntp/most_visited_sites_bridge.cc:171: ServiceAccessType::IMPLICIT_ACCESS), On 2016/10/13 09:06:45, sfiera wrote: > On 2016/10/12 09:11:27, Marc Treib wrote: > > This will return nullptr in incognito, and IconCacher doesn't seem to handle > > that. We prooobably can't get here in incognito? But I'd prefer that to be > more > > explicit, i.e. a DCHECK or something. > > Is that really the case? You can still see already-cached favicons in incognito > mode, so is EXPLICIT_ACCESS the right setting? (Perhaps we still shouldn't be > issuing any requests on the user's behalf in incognito mode) See https://cs.chromium.org/chromium/src/chrome/browser/favicon/favicon_service_f... - it'll return null for IMPLICIT_ACCESS in incognito. We *definitely* shouldn't store any favicons in incognito. But anyway, we shouldn't ever get here in incognito. So I'd just add a DCHECK. https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher.h (right): https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:26: class IconCacher { On 2016/10/13 09:06:46, sfiera wrote: > On 2016/10/12 09:11:28, Marc Treib wrote: > > Hm, I'm not entirely happy with the name - caching isn't really what this > class > > does. FaviconFetcher maybe? > > I don't like "Favicon" in the name, because that's a specific type of icon, and > we prefer TOUCH_ICON over FAVICON if available. It'd be consistent with FaviconService, which also handles both types. I guess "favicon" is also used as a term that encompasses all kinds of "site icon"? But I don't feel that strongly. > I'm not sure how I feel about "Fetcher". In the common case, we won't actually > fetch anything, we'll just verify that the icon is cached and move on. But I > guess IconFetcher would be OK with me. So it either fetches an icon or does nothing... IconMaybeFetcher? :D https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher_unittest.cc (right): https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_unittest.cc:49: CHECK(history_dir_.CreateUniqueTempDir()); On 2016/10/13 09:06:46, sfiera wrote: > On 2016/10/12 09:11:28, Marc Treib wrote: > > Can this be ASSERT_TRUE rather than CHECK, so the test will fail gracefully > > rather than crashing? > > Not easily; ASSERT_TRUE() doesn't work well outside TEST_F() functions. Here, it > would cause the constructor to return early, but I think the test would proceed, > even though it would ultimately report failure. Alright, fair enough. https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_unittest.cc:51: history::HistoryDatabaseParams(history_dir_.GetPath(), 0, 0))); On 2016/10/13 09:06:46, sfiera wrote: > On 2016/10/12 09:11:28, Marc Treib wrote: > > misaligned? > > Indentation, you mean? I've run git cl format, so I think it approves. Yes, I meant indentation, which looks a bit weird to me. But I'll submit to our overlord git cl format :)
https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher_unittest.cc (right): https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_unittest.cc:49: CHECK(history_dir_.CreateUniqueTempDir()); On 2016/10/13 09:06:46, sfiera wrote: > On 2016/10/12 09:11:28, Marc Treib wrote: > > Can this be ASSERT_TRUE rather than CHECK, so the test will fail gracefully > > rather than crashing? > > Not easily; ASSERT_TRUE() doesn't work well outside TEST_F() functions. Here, it > would cause the constructor to return early, but I think the test would proceed, > even though it would ultimately report failure. I'm not 100% sure -- ASSERT not only returns from the current function, but it records a "fatal" failure", and the test framework might just check for those before running the test. It would be worth trying it out? https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_unittest.cc:51: history::HistoryDatabaseParams(history_dir_.GetPath(), 0, 0))); On 2016/10/13 12:31:16, Marc Treib wrote: > On 2016/10/13 09:06:46, sfiera wrote: > > On 2016/10/12 09:11:28, Marc Treib wrote: > > > misaligned? > > > > Indentation, you mean? I've run git cl format, so I think it approves. > > Yes, I meant indentation, which looks a bit weird to me. But I'll submit to our > overlord git cl format :) Just FTR, that is not actually how it works: clang-format does not override the style guide (or a reviewer's reasonable interpretation thereof), even if it might appear easier to just always rely on its output. With that being said, in this case the indentation looks fine to me. https://codereview.chromium.org/2388783004/diff/160001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher.h (right): https://codereview.chromium.org/2388783004/diff/160001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:64: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2388783004/diff/160001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher_unittest.cc (right): https://codereview.chromium.org/2388783004/diff/160001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_unittest.cc:119: EXPECT_CALL(done, Call(false)).WillOnce(Quit(&loop)); Hm, this seems more complicated than, say, using captureless lambdas like above. Or just making a custom callback class: template<T> class ResultCallback<T> { public: base::Callback<void(T)> GetCallback() { return base::Bind(&ResultCallback<T>::SetResult, base::Unretained(this)); } T WaitForResult() { run_loop_.Run(); return result_; } private: void SetResult(T result) { result_ = result; run_loop_.Quit(); } base::RunLoop run_loop_; T result_; };
stevenjb@chromium.org changed reviewers: - stevenjb@chromium.org
The CQ bit was checked by sfiera@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...
sfiera@chromium.org changed reviewers: + stevenjb@chromium.org
Oops, missed +stevenjb in my previous email. https://codereview.chromium.org/2388783004/diff/120001/chrome/browser/android... File chrome/browser/android/ntp/most_visited_sites_bridge.cc (right): https://codereview.chromium.org/2388783004/diff/120001/chrome/browser/android... chrome/browser/android/ntp/most_visited_sites_bridge.cc:171: ServiceAccessType::IMPLICIT_ACCESS), On 2016/10/13 12:31:15, Marc Treib wrote: > On 2016/10/13 09:06:45, sfiera wrote: > > On 2016/10/12 09:11:27, Marc Treib wrote: > > > This will return nullptr in incognito, and IconCacher doesn't seem to handle > > > that. We prooobably can't get here in incognito? But I'd prefer that to be > > more > > > explicit, i.e. a DCHECK or something. > > > > Is that really the case? You can still see already-cached favicons in > incognito > > mode, so is EXPLICIT_ACCESS the right setting? (Perhaps we still shouldn't be > > issuing any requests on the user's behalf in incognito mode) > > See > https://cs.chromium.org/chromium/src/chrome/browser/favicon/favicon_service_f... > - it'll return null for IMPLICIT_ACCESS in incognito. …after NOTREACHED(). So it's not valid to get a FaviconService for IMPLICIT_ACCESS in incognito. > We *definitely* shouldn't store any favicons in incognito. But anyway, we > shouldn't ever get here in incognito. So I'd just add a DCHECK. I've added one, because yes, we don't get here in incognito, but I still feel a little iffy about that. As far as I know, there isn't a requirement that we not use MostVisitedSites in incognito mode, it just happens that we don't at present. https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher_unittest.cc (right): https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_unittest.cc:49: CHECK(history_dir_.CreateUniqueTempDir()); On 2016/10/13 13:20:09, Bernhard Bauer wrote: > On 2016/10/13 09:06:46, sfiera wrote: > > On 2016/10/12 09:11:28, Marc Treib wrote: > > > Can this be ASSERT_TRUE rather than CHECK, so the test will fail gracefully > > > rather than crashing? > > > > Not easily; ASSERT_TRUE() doesn't work well outside TEST_F() functions. Here, > it > > would cause the constructor to return early, but I think the test would > proceed, > > even though it would ultimately report failure. > > I'm not 100% sure -- ASSERT not only returns from the current function, but it > records a "fatal" failure", and the test framework might just check for those > before running the test. It would be worth trying it out? Tried it. Apparently ASSERT_* does return something—a failure message or something like that. https://codereview.chromium.org/2388783004/diff/120001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_unittest.cc:51: history::HistoryDatabaseParams(history_dir_.GetPath(), 0, 0))); On 2016/10/13 13:20:09, Bernhard Bauer wrote: > On 2016/10/13 12:31:16, Marc Treib wrote: > > On 2016/10/13 09:06:46, sfiera wrote: > > > On 2016/10/12 09:11:28, Marc Treib wrote: > > > > misaligned? > > > > > > Indentation, you mean? I've run git cl format, so I think it approves. > > > > Yes, I meant indentation, which looks a bit weird to me. But I'll submit to > our > > overlord git cl format :) > > Just FTR, that is not actually how it works: clang-format does not override the > style guide (or a reviewer's reasonable interpretation thereof), even if it > might appear easier to just always rely on its output. With that being said, in > this case the indentation looks fine to me. No, that is how it works: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md "By policy, Clang's formatting of code should always be accepted in code reviews." Of course, there are things that clang-format doesn't care about (like line 120), so using it doesn't mean you can ignore questions of formatting entirely. Its indentation should always be acceptable, though. https://codereview.chromium.org/2388783004/diff/160001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher.h (right): https://codereview.chromium.org/2388783004/diff/160001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:64: }; On 2016/10/13 13:20:09, Bernhard Bauer wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2388783004/diff/160001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher_unittest.cc (right): https://codereview.chromium.org/2388783004/diff/160001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_unittest.cc:119: EXPECT_CALL(done, Call(false)).WillOnce(Quit(&loop)); On 2016/10/13 13:20:09, Bernhard Bauer wrote: > Hm, this seems more complicated than, say, using captureless lambdas like above. > Or just making a custom callback class: > > template<T> > class ResultCallback<T> { > public: > base::Callback<void(T)> GetCallback() { > return base::Bind(&ResultCallback<T>::SetResult, base::Unretained(this)); > } > > T WaitForResult() { > run_loop_.Run(); > return result_; > } > > private: > void SetResult(T result) { > result_ = result; > run_loop_.Quit(); > } > > base::RunLoop run_loop_; > T result_; > }; To me, the captureless lambda seems like the most convoluted option. ResultCallback<T> is OK, but to me the mock approach fits better with the use of the MockImageFetcher.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2388783004/diff/120001/chrome/browser/android... File chrome/browser/android/ntp/most_visited_sites_bridge.cc (right): https://codereview.chromium.org/2388783004/diff/120001/chrome/browser/android... chrome/browser/android/ntp/most_visited_sites_bridge.cc:171: ServiceAccessType::IMPLICIT_ACCESS), On 2016/10/14 06:27:32, sfiera wrote: > On 2016/10/13 12:31:15, Marc Treib wrote: > > On 2016/10/13 09:06:45, sfiera wrote: > > > On 2016/10/12 09:11:27, Marc Treib wrote: > > > > This will return nullptr in incognito, and IconCacher doesn't seem to > handle > > > > that. We prooobably can't get here in incognito? But I'd prefer that to be > > > more > > > > explicit, i.e. a DCHECK or something. > > > > > > Is that really the case? You can still see already-cached favicons in > > incognito > > > mode, so is EXPLICIT_ACCESS the right setting? (Perhaps we still shouldn't > be > > > issuing any requests on the user's behalf in incognito mode) > > > > See > > > https://cs.chromium.org/chromium/src/chrome/browser/favicon/favicon_service_f... > > - it'll return null for IMPLICIT_ACCESS in incognito. > > …after NOTREACHED(). So it's not valid to get a FaviconService for > IMPLICIT_ACCESS in incognito. d'oh, obviously I'm blind. > > We *definitely* shouldn't store any favicons in incognito. But anyway, we > > shouldn't ever get here in incognito. So I'd just add a DCHECK. > > I've added one, because yes, we don't get here in incognito, but I still feel a > little iffy about that. As far as I know, there isn't a requirement that we not > use MostVisitedSites in incognito mode, it just happens that we don't at > present. Well, it seems that if we ever wanted to use MVS in incognito, that would require more work (at least, we'd have to check that we don't do anything we shouldn't). So IMO a DCHECK here is the right thing for now.
On 2016/10/14 08:46:21, Marc Treib wrote: > https://codereview.chromium.org/2388783004/diff/120001/chrome/browser/android... > File chrome/browser/android/ntp/most_visited_sites_bridge.cc (right): > > https://codereview.chromium.org/2388783004/diff/120001/chrome/browser/android... > chrome/browser/android/ntp/most_visited_sites_bridge.cc:171: > ServiceAccessType::IMPLICIT_ACCESS), > On 2016/10/14 06:27:32, sfiera wrote: > > On 2016/10/13 12:31:15, Marc Treib wrote: > > > On 2016/10/13 09:06:45, sfiera wrote: > > > > On 2016/10/12 09:11:27, Marc Treib wrote: > > > > > This will return nullptr in incognito, and IconCacher doesn't seem to > > handle > > > > > that. We prooobably can't get here in incognito? But I'd prefer that to > be > > > > more > > > > > explicit, i.e. a DCHECK or something. > > > > > > > > Is that really the case? You can still see already-cached favicons in > > > incognito > > > > mode, so is EXPLICIT_ACCESS the right setting? (Perhaps we still shouldn't > > be > > > > issuing any requests on the user's behalf in incognito mode) > > > > > > See > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/favicon/favicon_service_f... > > > - it'll return null for IMPLICIT_ACCESS in incognito. > > > > …after NOTREACHED(). So it's not valid to get a FaviconService for > > IMPLICIT_ACCESS in incognito. > > d'oh, obviously I'm blind. > > > > We *definitely* shouldn't store any favicons in incognito. But anyway, we > > > shouldn't ever get here in incognito. So I'd just add a DCHECK. > > > > I've added one, because yes, we don't get here in incognito, but I still feel > a > > little iffy about that. As far as I know, there isn't a requirement that we > not > > use MostVisitedSites in incognito mode, it just happens that we don't at > > present. > > Well, it seems that if we ever wanted to use MVS in incognito, that would > require more work (at least, we'd have to check that we don't do anything we > shouldn't). So IMO a DCHECK here is the right thing for now. I am not familiar with any of this code, what did you want me to review specifically?
https://codereview.chromium.org/2388783004/diff/160001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher_unittest.cc (right): https://codereview.chromium.org/2388783004/diff/160001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_unittest.cc:119: EXPECT_CALL(done, Call(false)).WillOnce(Quit(&loop)); On 2016/10/14 06:27:32, sfiera wrote: > On 2016/10/13 13:20:09, Bernhard Bauer wrote: > > Hm, this seems more complicated than, say, using captureless lambdas like > above. > > Or just making a custom callback class: > > > > template<T> > > class ResultCallback<T> { > > public: > > base::Callback<void(T)> GetCallback() { > > return base::Bind(&ResultCallback<T>::SetResult, > base::Unretained(this)); > > } > > > > T WaitForResult() { > > run_loop_.Run(); > > return result_; > > } > > > > private: > > void SetResult(T result) { > > result_ = result; > > run_loop_.Quit(); > > } > > > > base::RunLoop run_loop_; > > T result_; > > }; > > To me, the captureless lambda seems like the most convoluted option. > ResultCallback<T> is OK, but to me the mock approach fits better with the use of > the MockImageFetcher. Yeah, TBH I'm not a big fan of the MockImageFetcher either :) Too much magic going on IMO, in particular with ACTIONs... But if you prefer this, LGTM.
> I am not familiar with any of this code, what did you want me to review > specifically? Steven, your approval is required for the new dependency on components/favicon and components/favicon_base (from components/ntp_tiles/DEPS). Thanks!
RS OWNER lgtm for adding the favicon dependency.
The CQ bit was checked by sfiera@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 sfiera@chromium.org
The CQ bit was checked by sfiera@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, bauerb@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2388783004/#ps200001 (title: "Merge branch 'master' into icons")
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 ========== Ensure PopularSite icon availability in ntp_tiles. Previously, MostVisitedSites dumped all PopularSites information into its own observer, then let Java ensure availability of icons. Popular Sites on iOS needs these icons cached too, so the component should handle icon availability itself. MostVisitedSites::Observer::OnPopularURLsAvailable() is removed and MostVisitedSites::Observer::OnIconMadeAvailable() is added in its place. Note that FaviconHelper.ensureIconIsAvailable() hasn't been removed in this CL, as it's still used to load favicons for snippets. Of course, one day, iOS will need to do that too, so an eventual plan to share IconCacher and broaden its interface makes sense (maybe coupled with a move to the favicon component?). BUG=631990 ========== to ========== Ensure PopularSite icon availability in ntp_tiles. Previously, MostVisitedSites dumped all PopularSites information into its own observer, then let Java ensure availability of icons. Popular Sites on iOS needs these icons cached too, so the component should handle icon availability itself. MostVisitedSites::Observer::OnPopularURLsAvailable() is removed and MostVisitedSites::Observer::OnIconMadeAvailable() is added in its place. Note that FaviconHelper.ensureIconIsAvailable() hasn't been removed in this CL, as it's still used to load favicons for snippets. Of course, one day, iOS will need to do that too, so an eventual plan to share IconCacher and broaden its interface makes sense (maybe coupled with a move to the favicon component?). BUG=631990 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Ensure PopularSite icon availability in ntp_tiles. Previously, MostVisitedSites dumped all PopularSites information into its own observer, then let Java ensure availability of icons. Popular Sites on iOS needs these icons cached too, so the component should handle icon availability itself. MostVisitedSites::Observer::OnPopularURLsAvailable() is removed and MostVisitedSites::Observer::OnIconMadeAvailable() is added in its place. Note that FaviconHelper.ensureIconIsAvailable() hasn't been removed in this CL, as it's still used to load favicons for snippets. Of course, one day, iOS will need to do that too, so an eventual plan to share IconCacher and broaden its interface makes sense (maybe coupled with a move to the favicon component?). BUG=631990 ========== to ========== Ensure PopularSite icon availability in ntp_tiles. Previously, MostVisitedSites dumped all PopularSites information into its own observer, then let Java ensure availability of icons. Popular Sites on iOS needs these icons cached too, so the component should handle icon availability itself. MostVisitedSites::Observer::OnPopularURLsAvailable() is removed and MostVisitedSites::Observer::OnIconMadeAvailable() is added in its place. Note that FaviconHelper.ensureIconIsAvailable() hasn't been removed in this CL, as it's still used to load favicons for snippets. Of course, one day, iOS will need to do that too, so an eventual plan to share IconCacher and broaden its interface makes sense (maybe coupled with a move to the favicon component?). BUG=631990 Committed: https://crrev.com/ae1ae693f77cfef75097263227b958389cae471b Cr-Commit-Position: refs/heads/master@{#427100} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/ae1ae693f77cfef75097263227b958389cae471b Cr-Commit-Position: refs/heads/master@{#427100} |
