|
|
DescriptionAdd access to baked-in favicons for default popular sites on NTP
This change enables branded builds to use icons for the first
opened New Tab Page by adding them to the initial icon cache.
Non-branded builds receive a new set of popular site tiles.
(This profits developers mainly.)
BUG=672939
Review-Url: https://codereview.chromium.org/2695713004
Cr-Commit-Position: refs/heads/master@{#454424}
Committed: https://chromium.googlesource.com/chromium/src/+/fed34be47fbaa165e6d0817f55ae907a97ea00e8
Patch Set 1 #
Total comments: 26
Patch Set 2 : Move loading icons into IconCacher as fallback for non-existent icons #Patch Set 3 : Testing the access of the default resource #Patch Set 4 : Have python script adhere to guidelines #
Total comments: 40
Patch Set 5 : Add default resource attribute to Site #
Total comments: 28
Patch Set 6 : Add python doc strings #Patch Set 7 : Controle image size in tests and refine download script #Patch Set 8 : Fix iOS images and resize only if necessary. #Patch Set 9 : Remove images from public repo & adjust script accordingly #Patch Set 10 : Ignoring the internal resource folder #Patch Set 11 : Split updating script into internal repo #
Total comments: 12
Patch Set 12 : Adjust gitignore, debugging default sites and some parsing. #Patch Set 13 : Drop bool from callback #
Total comments: 32
Patch Set 14 : Rebase (someone made the gitignore change which should have been more timely...) #Patch Set 15 : Use two closures instead of one callback. #Patch Set 16 : Split out assigning resources #Patch Set 17 : Readded the accidentally deleted line. (All tests pass) #Patch Set 18 : Split test into new CL 2725923003 #
Total comments: 2
Patch Set 19 : Use Mock instead of verbose Fake #Patch Set 20 : Use NiceMock and abstract from platform-specific behavior in tests #Patch Set 21 : Rebase #Patch Set 22 : iOS Debugging. #Patch Set 23 : Mock and reset resources properly. #Patch Set 24 : Changing ui/base dependency for test only #Patch Set 25 : Change ui/base dependency #Patch Set 26 : Rebase #
Total comments: 2
Patch Set 27 : Add coorect resources DEPS #Patch Set 28 : ... and deleting unused DEPS #Patch Set 29 : Rebase. #Messages
Total messages: 85 (48 generated)
fhorschig@chromium.org changed reviewers: + sfiera@chromium.org, tschumann@chromium.org
Would you please have a look? BTW: Do we have some kind of MockFaviconService that allows checking for calls to |SetFavicon|?
On 2017/02/13 18:18:29, fhorschig wrote: > BTW: Do we have some kind of MockFaviconService that allows checking for calls > to |SetFavicon|? I don't think so. I ended up writing IconCacherTest::IconIsCachedFor() against a real favicon service. https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/update... File components/ntp_tiles/update_default_sites_resources.py (right): https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/update... components/ntp_tiles/update_default_sites_resources.py:1: #!/usr/bin/env python Is this file written according to http://google.github.io/styleguide/pyguide.html? It doesn't seem to, w.r.t. imports, naming, and main(). https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/update... components/ntp_tiles/update_default_sites_resources.py:18: print("... done. (" + str(len(data)) + " sites found)") print("... done. (%d sites found)" % len(data)) https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/update... components/ntp_tiles/update_default_sites_resources.py:24: """Deleting old icons explicitly avoids wrong icons for pages without such.""" Regular comments are #. Only docstrings have """ https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/update... components/ntp_tiles/update_default_sites_resources.py:27: if re.search(kSiteIconName + "*", f): I think you are looking for https://docs.python.org/2/library/glob.html (Also, doesn't this regexp match ico, icon, iconnnnn, etc.?) https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/update... components/ntp_tiles/update_default_sites_resources.py:33: print("(Source: " + data[i]['large_icon_url']) +")" Not all sites have large_icon_url. It's probably safe to assume that all sites on the default list have it, but I'd leave a comment here for the benefit of the hapless user that might one day see a KeyError here. https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/update... components/ntp_tiles/update_default_sites_resources.py:34: urllib.urlretrieve(data[i]['large_icon_url'], Why are you using requests above and urllib here?
jkrcal@chromium.org changed reviewers: + jkrcal@chromium.org
https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/popula... File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:139: PopularSites::SitesVector GetDefaultFromPrefs( drive-by comment: When is this function called? You should make sure you never overwrite anything in the favicon service (because possibly messing up with sync). In other words, you should not SetFavicons(site_url, ...) if there are already same favicons for the given site_url.
Just some drive-by questions: why don't these icons have file extensions (.png)? Have they been optimized? Why are they not all the same size? Please add some notes in the CL description about how many bytes this adds to the binary.
https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/popula... File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:139: PopularSites::SitesVector GetDefaultFromPrefs( On 2017/02/14 08:23:06, jkrcal wrote: > drive-by comment: When is this function called? > > You should make sure you never overwrite anything in the favicon service > (because possibly messing up with sync). In other words, you should not > SetFavicons(site_url, ...) if there are already same favicons for the given > site_url. Even in the absence of sync, I think this implementation would prevent us from seeing new favicon if a popular site ever updated it, right? Every time an NTP is opened, it will override the update. Did you consider implementing this feature as an implementation of IconCacher (that falls back to IconCacherImpl if it doesn't have an icon)? That seems like a good place to encapsulate this logic. https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:234: sites_(GetDefaultFromPrefs(*prefs_, favicon_service)), Initializing the favicon service in PopularSitesImpl's constructor is a surprising side-effect. https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/resour... File components/ntp_tiles/resources/default_popular_sites.json (right): https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/resour... components/ntp_tiles/resources/default_popular_sites.json:1: [{"url": "https://m.facebook.com/", "large_icon_url": "https://static.xx.fbcdn.net/rsrc.php/v3/ya/r/O2aKM2iSbOw.png", "title": "Facebook - Log In or Sign Up"}, {"url": "https://m.youtube.com/", "large_icon_url": "https://s.ytimg.com/yts/mobile/img/apple-touch-icon-144x144-precomposed-vflwq-hLZ.png", "title": "YouTube"}, {"url": "https://www.amazon.com/", "large_icon_url": "https://images-na.ssl-images-amazon.com/images/G/01/anywhere/a_smile_196x196._CB368246573_.png", "title": "Amazon.com: Online Shopping for Electronics, Apparel, Computers, Books, DVDs & more"}, {"url": "https://en.m.wikipedia.org/", "large_icon_url": "https://en.m.wikipedia.org/static/apple-touch/wikipedia.png", "title": "Wikipedia, the free encyclopedia"}, {"url": "http://www.espn.com/", "large_icon_url": "http://a.espncdn.com/wireless/mw5/r1/images/bookmark-icons/espn_icon-152x152.min.png", "title": "ESPN: The Worldwide Leader in Sports"}, {"url": "https://www.yahoo.com/", "large_icon_url": "https://s.yimg.com/dh/ap/default/130909/y_200_a.png", "title": "Yahoo"}, {"url": "https://www.instagram.com/", "large_icon_url": "https://instagramstatic-a.akamaihd.net/h1/images/ico/favicon-192.png/b407fa101800.png", "title": "Instagram"}, {"url": "http://m.ebay.com/", "large_icon_url": "http://ir.ebaystatic.com/pictures/aw/pics/mobile/images/apple-touch-icon.png", "title": "Electronics, Cars, Fashion, Collectibles, Coupons and More | eBay"}] Did you intentionally minify the file for this CL? That's a reasonable change, but can we do that first and separately?
tschumann@chromium.org changed reviewers: + mastiz@chromium.org - tschumann@chromium.org
https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/popula... File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:147: #if defined(OS_ANDROID) || defined(OS_IOS) i wonder why is this part only relevant to mobile platforms but not the first part? Do we ever want this method to be called on other platforms? https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:151: favicons->SetFavicons(sites[0].url, sites[0].large_icon_url, +mastiz who looked into this. Feels weird to set this every time we load the popular sites from prefs. Also this is quite a tricky setup. Would it be possible to have the fallback on a higher level? Like at some point up the stack, we make the decision to load popular sites and figure out we don't have any. There, we could simply load all data (sites + icons) from the built-in config and bypass the favicon service. I'm deferring this to Mikel as I have only little understanding of that stack and it's dependencies.
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/popula... File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:151: favicons->SetFavicons(sites[0].url, sites[0].large_icon_url, On 2017/02/14 11:56:51, tschumann wrote: > +mastiz who looked into this. > Feels weird to set this every time we load the popular sites from prefs. Also > this is quite a tricky setup. Would it be possible to have the fallback on a > higher level? > Like at some point up the stack, we make the decision to load popular sites and > figure out we don't have any. There, we could simply load all data (sites + > icons) from the built-in config and bypass the favicon service. > I'm deferring this to Mikel as I have only little understanding of that stack > and it's dependencies. This comment seems obsolete now. Please loop me in when this has settled down a bit, there's quite some reviewers already :-)
Thanks for the many comments! The current patchset addresses mainly c++ issues. Adjusting the python script will follow tomorrow (including verification/resizing of images). https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/popula... File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:139: PopularSites::SitesVector GetDefaultFromPrefs( On 2017/02/14 08:23:06, jkrcal wrote: > drive-by comment: When is this function called? > > You should make sure you never overwrite anything in the favicon service > (because possibly messing up with sync). In other words, you should not > SetFavicons(site_url, ...) if there are already same favicons for the given > site_url. Good remark. It's now called by the IconCacher which tries to retrieve an existing icon first. > Even in the absence of sync, I think this implementation would prevent us from > seeing new favicon if a popular site ever updated it, right? Every time an NTP > is opened, it will override the update. Right, this was not at all the right place for this. > Did you consider implementing this feature as an implementation of IconCacher > (that falls back to IconCacherImpl if it doesn't have an icon)? That seems like > a good place to encapsulate this logic. The IconFetcher not only is a better place but also allows to load the resources only if really needed and to mock the favicon_service part for tests. https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:147: #if defined(OS_ANDROID) || defined(OS_IOS) On 2017/02/14 11:56:51, tschumann wrote: > i wonder why is this part only relevant to mobile platforms but not the first > part? Do we ever want this method to be called on other platforms? It used to load the defaults and if on mobile, register the resources. Now, these parts are separated again. https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:151: favicons->SetFavicons(sites[0].url, sites[0].large_icon_url, On 2017/02/14 11:56:51, tschumann wrote: > +mastiz who looked into this. > Feels weird to set this every time we load the popular sites from prefs. Also > this is quite a tricky setup. Would it be possible to have the fallback on a > higher level? > Like at some point up the stack, we make the decision to load popular sites and > figure out we don't have any. There, we could simply load all data (sites + > icons) from the built-in config and bypass the favicon service. > I'm deferring this to Mikel as I have only little understanding of that stack > and it's dependencies. Like Jan and Chris have pointed out, this was neither the right place nor the right frequency. The loading happens in the IconCacher (where all tile images are loaded) and notifying the cacher about the default tiles happens when the cacher is build in the chrome_most_visited_sites_factory. https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:151: favicons->SetFavicons(sites[0].url, sites[0].large_icon_url, On 2017/02/14 14:44:59, mastiz wrote: > On 2017/02/14 11:56:51, tschumann wrote: > > +mastiz who looked into this. > > Feels weird to set this every time we load the popular sites from prefs. Also > > this is quite a tricky setup. Would it be possible to have the fallback on a > > higher level? > > Like at some point up the stack, we make the decision to load popular sites > and > > figure out we don't have any. There, we could simply load all data (sites + > > icons) from the built-in config and bypass the favicon service. > > I'm deferring this to Mikel as I have only little understanding of that stack > > and it's dependencies. > > This comment seems obsolete now. Please loop me in when this has settled down a > bit, there's quite some reviewers already :-) Absolutely. It seems to be quite an interesting CL with lots of shortcomings ... https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:234: sites_(GetDefaultFromPrefs(*prefs_, favicon_service)), On 2017/02/14 09:59:16, sfiera wrote: > Initializing the favicon service in PopularSitesImpl's constructor is a > surprising side-effect. Agreed and removed. https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/resour... File components/ntp_tiles/resources/default_popular_sites.json (right): https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/resour... components/ntp_tiles/resources/default_popular_sites.json:1: [{"url": "https://m.facebook.com/", "large_icon_url": "https://static.xx.fbcdn.net/rsrc.php/v3/ya/r/O2aKM2iSbOw.png", "title": "Facebook - Log In or Sign Up"}, {"url": "https://m.youtube.com/", "large_icon_url": "https://s.ytimg.com/yts/mobile/img/apple-touch-icon-144x144-precomposed-vflwq-hLZ.png", "title": "YouTube"}, {"url": "https://www.amazon.com/", "large_icon_url": "https://images-na.ssl-images-amazon.com/images/G/01/anywhere/a_smile_196x196._CB368246573_.png", "title": "Amazon.com: Online Shopping for Electronics, Apparel, Computers, Books, DVDs & more"}, {"url": "https://en.m.wikipedia.org/", "large_icon_url": "https://en.m.wikipedia.org/static/apple-touch/wikipedia.png", "title": "Wikipedia, the free encyclopedia"}, {"url": "http://www.espn.com/", "large_icon_url": "http://a.espncdn.com/wireless/mw5/r1/images/bookmark-icons/espn_icon-152x152.min.png", "title": "ESPN: The Worldwide Leader in Sports"}, {"url": "https://www.yahoo.com/", "large_icon_url": "https://s.yimg.com/dh/ap/default/130909/y_200_a.png", "title": "Yahoo"}, {"url": "https://www.instagram.com/", "large_icon_url": "https://instagramstatic-a.akamaihd.net/h1/images/ico/favicon-192.png/b407fa101800.png", "title": "Instagram"}, {"url": "http://m.ebay.com/", "large_icon_url": "http://ir.ebaystatic.com/pictures/aw/pics/mobile/images/apple-touch-icon.png", "title": "Electronics, Cars, Fashion, Collectibles, Coupons and More | eBay"}] On 2017/02/14 09:59:16, sfiera wrote: > Did you intentionally minify the file for this CL? That's a reasonable change, > but can we do that first and separately? I intentionally didn't beautified the JSON. This is just the response from the server. The last was just beautified because there was no script yet to update it and reviewing would otherwise be nearly impossible. https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/update... File components/ntp_tiles/update_default_sites_resources.py (right): https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/update... components/ntp_tiles/update_default_sites_resources.py:1: #!/usr/bin/env python On 2017/02/13 18:36:35, sfiera wrote: > Is this file written according to > http://google.github.io/styleguide/pyguide.html > > It doesn't seem to, w.r.t. imports, naming, and main(). Thanks for the link! It was not (minimal linting/presubmit requirements). All violations I found are gone but I consider looking for someone who wrote Chromium python code before... https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/update... components/ntp_tiles/update_default_sites_resources.py:34: urllib.urlretrieve(data[i]['large_icon_url'], Requests is an easy way to get the response as JSON whereas just downloading a file is easier with urllib. I don't see a good reason to use just one library (although possible for either) but if you name one, I will change it.
Is this ready for another look? I'd been waiting for another message about the python script.
Thank you for reminding me! https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/update... File components/ntp_tiles/update_default_sites_resources.py (right): https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/update... components/ntp_tiles/update_default_sites_resources.py:18: print("... done. (" + str(len(data)) + " sites found)") On 2017/02/13 18:36:35, sfiera wrote: > print("... done. (%d sites found)" % len(data)) Done. https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/update... components/ntp_tiles/update_default_sites_resources.py:24: """Deleting old icons explicitly avoids wrong icons for pages without such.""" On 2017/02/13 18:36:35, sfiera wrote: > Regular comments are #. Only docstrings have """ Done. https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/update... components/ntp_tiles/update_default_sites_resources.py:27: if re.search(kSiteIconName + "*", f): On 2017/02/13 18:36:35, sfiera wrote: > I think you are looking for https://docs.python.org/2/library/glob.html > > (Also, doesn't this regexp match ico, icon, iconnnnn, etc.?) nice! (Yes, it was intentional but not entirely correct. See new regexp) https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/update... components/ntp_tiles/update_default_sites_resources.py:33: print("(Source: " + data[i]['large_icon_url']) +")" On 2017/02/13 18:36:35, sfiera wrote: > Not all sites have large_icon_url. It's probably safe to assume that all sites > on the default list have it, but I'd leave a comment here for the benefit of the > hapless user that might one day see a KeyError here. Done. https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/update... components/ntp_tiles/update_default_sites_resources.py:34: urllib.urlretrieve(data[i]['large_icon_url'], On 2017/02/14 17:29:02, fhorschig wrote: > Requests is an easy way to get the response as JSON whereas > just downloading a file is easier with urllib. > I don't see a good reason to use just one library (although > possible for either) but if you name one, I will change it. I used urllib everywhere. requests ran into some problem with https pages.
thanhphong04071993@gmail.com changed reviewers: + Thanhphong04071993@gmail.com
sfiera@chromium.org changed reviewers: + thanhphong04071993@gmail.com - Thanhphong04071993@gmail.com
https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/ic... File components/ntp_tiles/icon_cacher_impl.h (right): https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/ic... components/ntp_tiles/icon_cacher_impl.h:66: std::map<const std::string, DefaultIcon> default_icons_; Instead of having a map here, could PopularSites::Site have a "default resource" field? https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/re... File components/ntp_tiles/resources/default_popular_sites.json (right): https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/re... components/ntp_tiles/resources/default_popular_sites.json:1: [{"url": "https://m.facebook.com/", "large_icon_url": "https://static.xx.fbcdn.net/rsrc.php/v3/ya/r/O2aKM2iSbOw.png", "title": "Facebook - Log In or Sign Up"}, {"url": "https://m.youtube.com/", "large_icon_url": "https://s.ytimg.com/yts/mobile/img/apple-touch-icon-144x144-precomposed-vflwq-hLZ.png", "title": "YouTube"}, {"url": "https://www.amazon.com/", "large_icon_url": "https://images-na.ssl-images-amazon.com/images/G/01/anywhere/a_smile_196x196._CB368246573_.png", "title": "Amazon.com: Online Shopping for Electronics, Apparel, Computers, Books, DVDs & more"}, {"url": "https://en.m.wikipedia.org/", "large_icon_url": "https://en.m.wikipedia.org/static/apple-touch/wikipedia.png", "title": "Wikipedia, the free encyclopedia"}, {"url": "http://www.espn.com/", "large_icon_url": "http://a.espncdn.com/wireless/mw5/r1/images/bookmark-icons/espn_icon-152x152.min.png", "title": "ESPN: The Worldwide Leader in Sports"}, {"url": "https://www.yahoo.com/", "large_icon_url": "https://s.yimg.com/dh/ap/default/130909/y_200_a.png", "title": "Yahoo"}, {"url": "https://www.instagram.com/", "large_icon_url": "https://instagramstatic-a.akamaihd.net/h1/images/ico/favicon-192.png/b407fa101800.png", "title": "Instagram"}, {"url": "http://m.ebay.com/", "large_icon_url": "http://ir.ebaystatic.com/pictures/aw/pics/mobile/images/apple-touch-icon.png", "title": "Electronics, Cars, Fashion, Collectibles, Coupons and More | eBay"}] Hmm, this isn't actually minified. Check the "separators" argument to json.dump(). https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... File components/ntp_tiles/update_default_sites_resources.py (right): https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:5: py-style: please add __future__ imports: from __future__ import absolute_import from __future__ import division from __future__ import print_function (you may add additional ones, such as division) https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:12: from io import BytesIO py-style: from x import y is not allowed unless y is a package. https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:17: DEFAULT_POPULAR_SITES = 'https://www.gstatic.com/chrome/ntp/'\ py-style: use parens, not backslashes. https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:22: SITE_ICON_NAME = 'icon' You might prefer `SITE_ICON_FORMAT = "icon%d.png"` so you can use `SITE_ICON_FORMAT % i` below. https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:26: 'default_popular_sites.json') py-style: alignment https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:28: def download_popular_sites(): Docstrings, please. For example, it's useful here to know that this function both writes the file and returns the dict. https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:29: print("Downloading popular sites... (" + DEFAULT_POPULAR_SITES + ")") py-style: You're switching between ' and " quotes. Either is OK, but please pick one. https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:29: print("Downloading popular sites... (" + DEFAULT_POPULAR_SITES + ")") py-style: Proper indentation is 2 spaces. https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:45: image_response_data = urllib.urlopen(url=site[LARGE_ICON_KEY]).read() Please use "with …urlopen() as …" to ensure that the file object is closed after use. https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:48: def resize_if_too_large(image): Have we determined yet if resizing is OK? I would hold off for now. Besides, resizing a PNG without subsequently optimizing might result in a growth in file size. https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:49: if image.size > MAXIMAL_SIZE: This does not mean what you think it means. Tuple comparison is element-wise, i.e.: (0, 0) < (0, 1) < (0, 2) < (0, 196) < (1, 0) < (196, 0) < (196, 196) (however, "x < y < z" actually means what it looks like, unlike C++!) https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:60: for i in range(len(popular_sites)): for i, site in enumerate(popular_sites): https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:63: print("Could not download image for malformed entry: " + str(site)) "malformed" is not quote the right term here. Some countries have sites with a favicon_url instead of a large_icon_url. Use repr() instead of str() here (or "…: %r" % site) https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:65: print("Downloading icon for \"" + site[SITE_TITLE_KEY]) +"\"..." "Downloading icon for %r..." % site[SITE_TITLE_KEY] https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:68: image_name = SITE_ICON_NAME + str(i) + ".png" In general, using str() looks not very python-y. If you want to put a number in a string, it's better to use format strings. https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:70: print("... done.(Stored as " + image_name + ")"); Nit: Space between . and ( https://codereview.chromium.org/2695713004/diff/80001/components/resources/nt... File components/resources/ntp_tiles_resources.grdp (right): https://codereview.chromium.org/2695713004/diff/80001/components/resources/nt... components/resources/ntp_tiles_resources.grdp:8: <include name="IDR_DEFAULT_POPULAR_SITES_ICON0" file="../ntp_tiles/resources/icon0" type="BINDATA" /> They have extensions now, right?
https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/ic... File components/ntp_tiles/icon_cacher_impl.h (right): https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/ic... components/ntp_tiles/icon_cacher_impl.h:66: std::map<const std::string, DefaultIcon> default_icons_; On 2017/02/16 11:14:22, sfiera wrote: > Instead of having a map here, could PopularSites::Site have a "default resource" > field? Yes. It has now and the default resource ID is -1. https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/re... File components/ntp_tiles/resources/default_popular_sites.json (right): https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/re... components/ntp_tiles/resources/default_popular_sites.json:1: [{"url": "https://m.facebook.com/", "large_icon_url": "https://static.xx.fbcdn.net/rsrc.php/v3/ya/r/O2aKM2iSbOw.png", "title": "Facebook - Log In or Sign Up"}, {"url": "https://m.youtube.com/", "large_icon_url": "https://s.ytimg.com/yts/mobile/img/apple-touch-icon-144x144-precomposed-vflwq-hLZ.png", "title": "YouTube"}, {"url": "https://www.amazon.com/", "large_icon_url": "https://images-na.ssl-images-amazon.com/images/G/01/anywhere/a_smile_196x196._CB368246573_.png", "title": "Amazon.com: Online Shopping for Electronics, Apparel, Computers, Books, DVDs & more"}, {"url": "https://en.m.wikipedia.org/", "large_icon_url": "https://en.m.wikipedia.org/static/apple-touch/wikipedia.png", "title": "Wikipedia, the free encyclopedia"}, {"url": "http://www.espn.com/", "large_icon_url": "http://a.espncdn.com/wireless/mw5/r1/images/bookmark-icons/espn_icon-152x152.min.png", "title": "ESPN: The Worldwide Leader in Sports"}, {"url": "https://www.yahoo.com/", "large_icon_url": "https://s.yimg.com/dh/ap/default/130909/y_200_a.png", "title": "Yahoo"}, {"url": "https://www.instagram.com/", "large_icon_url": "https://instagramstatic-a.akamaihd.net/h1/images/ico/favicon-192.png/b407fa101800.png", "title": "Instagram"}, {"url": "http://m.ebay.com/", "large_icon_url": "http://ir.ebaystatic.com/pictures/aw/pics/mobile/images/apple-touch-icon.png", "title": "Electronics, Cars, Fashion, Collectibles, Coupons and More | eBay"}] On 2017/02/16 11:14:22, sfiera wrote: > Hmm, this isn't actually minified. Check the "separators" argument to > json.dump(). Thanks, it's integrated. https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... File components/ntp_tiles/update_default_sites_resources.py (right): https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:5: On 2017/02/16 11:14:23, sfiera wrote: > py-style: please add __future__ imports: > > from __future__ import absolute_import > from __future__ import division > from __future__ import print_function > > (you may add additional ones, such as division) Done. https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:12: from io import BytesIO On 2017/02/16 11:14:23, sfiera wrote: > py-style: from x import y is not allowed unless y is a package. Done. https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:17: DEFAULT_POPULAR_SITES = 'https://www.gstatic.com/chrome/ntp/'\ On 2017/02/16 11:14:23, sfiera wrote: > py-style: use parens, not backslashes. Done. https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:22: SITE_ICON_NAME = 'icon' On 2017/02/16 11:14:23, sfiera wrote: > You might prefer `SITE_ICON_FORMAT = "icon%d.png"` so you can use > `SITE_ICON_FORMAT % i` below. I like this but now I need a second one for deleting icons. https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:26: 'default_popular_sites.json') On 2017/02/16 11:14:23, sfiera wrote: > py-style: alignment Done. https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:28: def download_popular_sites(): On 2017/02/16 11:14:23, sfiera wrote: > Docstrings, please. For example, it's useful here to know that this function > both writes the file and returns the dict. Split the function. (For multiple reasons) https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:29: print("Downloading popular sites... (" + DEFAULT_POPULAR_SITES + ")") On 2017/02/16 11:14:22, sfiera wrote: > py-style: Proper indentation is 2 spaces. Done although your guidelines say 4 [1]. Is there a Chromium-specific style? [1] http://google.github.io/styleguide/pyguide.html?showone=Indentation#Indentation https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:29: print("Downloading popular sites... (" + DEFAULT_POPULAR_SITES + ")") On 2017/02/16 11:14:23, sfiera wrote: > py-style: You're switching between ' and " quotes. Either is OK, but please pick > one. Done. https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:45: image_response_data = urllib.urlopen(url=site[LARGE_ICON_KEY]).read() On 2017/02/16 11:14:23, sfiera wrote: > Please use "with …urlopen() as …" to ensure that the file object is closed after > use. Done with hack: https://docs.python.org/2/library/contextlib.html#contextlib.closing https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:48: def resize_if_too_large(image): On 2017/02/16 11:14:23, sfiera wrote: > Have we determined yet if resizing is OK? I would hold off for now. Besides, > resizing a PNG without subsequently optimizing might result in a growth in file > size. We have not yet decided. I added a flag to prevent resizing. https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:49: if image.size > MAXIMAL_SIZE: On 2017/02/16 11:14:22, sfiera wrote: > This does not mean what you think it means. Tuple comparison is element-wise, > i.e.: > > (0, 0) < (0, 1) < (0, 2) < (0, 196) < (1, 0) < (196, 0) < (196, 196) > > (however, "x < y < z" actually means what it looks like, unlike C++!) I am actually fine with since sizes must be quadratic but made my intention more clear by comparing only widths. https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:60: for i in range(len(popular_sites)): On 2017/02/16 11:14:23, sfiera wrote: > for i, site in enumerate(popular_sites): Uuuh, nice. https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:63: print("Could not download image for malformed entry: " + str(site)) On 2017/02/16 11:14:23, sfiera wrote: > "malformed" is not quote the right term here. Some countries have sites with a > favicon_url instead of a large_icon_url. > > Use repr() instead of str() here (or "…: %r" % site) Done. https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:65: print("Downloading icon for \"" + site[SITE_TITLE_KEY]) +"\"..." On 2017/02/16 11:14:23, sfiera wrote: > "Downloading icon for %r..." % site[SITE_TITLE_KEY] Done. https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:68: image_name = SITE_ICON_NAME + str(i) + ".png" On 2017/02/16 11:14:22, sfiera wrote: > In general, using str() looks not very python-y. If you want to put a number in > a string, it's better to use format strings. Gone. https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:70: print("... done.(Stored as " + image_name + ")"); On 2017/02/16 11:14:22, sfiera wrote: > Nit: Space between . and ( Done. https://codereview.chromium.org/2695713004/diff/80001/components/resources/nt... File components/resources/ntp_tiles_resources.grdp (right): https://codereview.chromium.org/2695713004/diff/80001/components/resources/nt... components/resources/ntp_tiles_resources.grdp:8: <include name="IDR_DEFAULT_POPULAR_SITES_ICON0" file="../ntp_tiles/resources/icon0" type="BINDATA" /> On 2017/02/16 11:14:23, sfiera wrote: > They have extensions now, right? Right. Added.
This mostly looks OK, but I'm not ready to approve until we get some word about how to appropriately mark the icon copyrights (if indeed such a way exists). https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... File components/ntp_tiles/update_default_sites_resources.py (right): https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:29: print("Downloading popular sites... (" + DEFAULT_POPULAR_SITES + ")") On 2017/02/16 17:06:42, fhorschig wrote: > On 2017/02/16 11:14:22, sfiera wrote: > > py-style: Proper indentation is 2 spaces. > > Done although your guidelines say 4 [1]. > Is there a Chromium-specific style? > > [1] > http://google.github.io/styleguide/pyguide.html?showone=Indentation#Indentation Oh, apparently 2 space is Google internal but 4 is Google external. We should use 4 here, then. https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher_impl.h (right): https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_impl.h:42: const base::Callback<void(bool)>& done) override; If I see a callback "done" I assume it's going to be called once. Can you change the name and add a comment explaining how many times it will be called and when? https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher_impl_unittest.cc (right): https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_impl_unittest.cc:48: class MockResourceDelegate : public ui::ResourceBundle::Delegate { This class is (mostly) not a mock—please don't mix patterns. I think this class would be a good candidate for a regular fake, with a std::map<int, gfx::Image> used to implement GetImageNamed(). https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_impl_unittest.cc:239: if (ui::ResourceBundle::HasSharedInstance()) { I'd put this as setup for IconCacherTest. Even if it's not necessary for the other tests, I like to start reading each test with the assumption of a clean slate. https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_impl_unittest.cc:269: EXPECT_TRUE(IconIsCachedFor(site_.url, favicon_base::TOUCH_ICON)); Can you add a check for the size here, so that we can distinguish which version we see here and below? https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/m... File components/ntp_tiles/most_visited_sites_unittest.cc (right): https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/m... components/ntp_tiles/most_visited_sites_unittest.cc:200: void(const PopularSites::Site& site, int image_resource_id)); Obsolete? https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/p... File components/ntp_tiles/popular_sites.h (right): https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/p... components/ntp_tiles/popular_sites.h:40: int default_resource_id; // 0 if there is none. Used for popular defaults. <0 https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/p... File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:145: site->SetInteger("default_resource_id", resource_id); Feels a little weird to be storing a resource ID in preferences. But I guess it's not really in the preferences, just an ephemeral default value… https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:157: SetDefaultResourceForSite(sites.get(), 0, IDR_DEFAULT_POPULAR_SITES_ICON0); Couldn't you write this as a foreach loop? Like: int index = 0; for (int resource : {IDR_DEFAULT_POPULAR_SITES_ICON0, …}) { https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:182: default_resource_id(-1) {} // Valid resources would be >=0 I don't think any comment is needed in addition to the header. https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/u... File components/ntp_tiles/update_default_sites_resources.py (right): https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/u... components/ntp_tiles/update_default_sites_resources.py:21: # with it. If an icon is too large, it will get resized in the process. This here would be a good use of a file-level """ comment. https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/u... components/ntp_tiles/update_default_sites_resources.py:63: return Image.open(io.BytesIO(image_response_data)) I'm still concerned that you might be un-optimizing the compression here. https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/u... components/ntp_tiles/update_default_sites_resources.py:92: parser.add_argument("-u", "--url", metavar="url", type=str, If you have a long flag name, the metavar should default to it. https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/u... components/ntp_tiles/update_default_sites_resources.py:95: parser.add_argument("--skip_icons", action="store_true", In Chromium, I think we typically use --skip-icons, not --skip_icons style flag naming. (still accessed as args.skip_icons later) I would also say "--no-icons" "--no-resizing". I think the --no pattern is more common, at least at Google. https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/u... components/ntp_tiles/update_default_sites_resources.py:100: type=str, See also argparse.FileType.
I wouldn't have expected approval whith the copyright discussion still in place ;) https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... File components/ntp_tiles/update_default_sites_resources.py (right): https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/up... components/ntp_tiles/update_default_sites_resources.py:29: print("Downloading popular sites... (" + DEFAULT_POPULAR_SITES + ")") On 2017/02/16 18:54:16, sfiera wrote: > On 2017/02/16 17:06:42, fhorschig wrote: > > On 2017/02/16 11:14:22, sfiera wrote: > > > py-style: Proper indentation is 2 spaces. > > > > Done although your guidelines say 4 [1]. > > Is there a Chromium-specific style? > > > > [1] > > > http://google.github.io/styleguide/pyguide.html?showone=Indentation#Indentation > > Oh, apparently 2 space is Google internal but 4 is Google external. We should > use 4 here, then. Done. https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher_impl.h (right): https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_impl.h:42: const base::Callback<void(bool)>& done) override; On 2017/02/16 18:54:16, sfiera wrote: > If I see a callback "done" I assume it's going to be called once. Can you change > the name and add a comment explaining how many times it will be called and when? I added a comment and changed the name. Strictly, I expect only OnceCallback to be called once. https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher_impl_unittest.cc (right): https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_impl_unittest.cc:48: class MockResourceDelegate : public ui::ResourceBundle::Delegate { On 2017/02/16 18:54:16, sfiera wrote: > This class is (mostly) not a mock—please don't mix patterns. I think this class > would be a good candidate for a regular fake, with a std::map<int, gfx::Image> > used to implement GetImageNamed(). Done. https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_impl_unittest.cc:239: if (ui::ResourceBundle::HasSharedInstance()) { On 2017/02/16 18:54:16, sfiera wrote: > I'd put this as setup for IconCacherTest. Even if it's not necessary for the > other tests, I like to start reading each test with the assumption of a clean > slate. Done. https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_impl_unittest.cc:269: EXPECT_TRUE(IconIsCachedFor(site_.url, favicon_base::TOUCH_ICON)); On 2017/02/16 18:54:16, sfiera wrote: > Can you add a check for the size here, so that we can distinguish which version > we see here and below? Done. https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/m... File components/ntp_tiles/most_visited_sites_unittest.cc (right): https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/m... components/ntp_tiles/most_visited_sites_unittest.cc:200: void(const PopularSites::Site& site, int image_resource_id)); On 2017/02/16 18:54:16, sfiera wrote: > Obsolete? Gone. https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/p... File components/ntp_tiles/popular_sites.h (right): https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/p... components/ntp_tiles/popular_sites.h:40: int default_resource_id; // 0 if there is none. Used for popular defaults. On 2017/02/16 18:54:16, sfiera wrote: > <0 Correct. https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/p... File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:145: site->SetInteger("default_resource_id", resource_id); On 2017/02/16 18:54:16, sfiera wrote: > Feels a little weird to be storing a resource ID in preferences. But I guess > it's not really in the preferences, just an ephemeral default value… Yes. With the first real fetch, this value is gone. I had the same thought but adding this property apart from the definition of the default popular sites makes for some huge exceptions and likely pitfalls. https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:157: SetDefaultResourceForSite(sites.get(), 0, IDR_DEFAULT_POPULAR_SITES_ICON0); On 2017/02/16 18:54:16, sfiera wrote: > Couldn't you write this as a foreach loop? Like: > > int index = 0; > for (int resource : {IDR_DEFAULT_POPULAR_SITES_ICON0, > …}) { YES! This is exactly what I have been looking for. Thanks! https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:182: default_resource_id(-1) {} // Valid resources would be >=0 On 2017/02/16 18:54:16, sfiera wrote: > I don't think any comment is needed in addition to the header. Done. https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/u... File components/ntp_tiles/update_default_sites_resources.py (right): https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/u... components/ntp_tiles/update_default_sites_resources.py:21: # with it. If an icon is too large, it will get resized in the process. On 2017/02/16 18:54:16, sfiera wrote: > This here would be a good use of a file-level """ comment. Okay. https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/u... components/ntp_tiles/update_default_sites_resources.py:63: return Image.open(io.BytesIO(image_response_data)) On 2017/02/16 18:54:16, sfiera wrote: > I'm still concerned that you might be un-optimizing the compression here. As discussed, I noew introduced the highest compression level of PIL. (Used to default to 6, now it's 9) I will have a look at PNGOUT as the current downscaling seems to have disadvantages for logos like Wikipedia's. https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/u... components/ntp_tiles/update_default_sites_resources.py:92: parser.add_argument("-u", "--url", metavar="url", type=str, On 2017/02/16 18:54:16, sfiera wrote: > If you have a long flag name, the metavar should default to it. Yes but it's CAPS. But in this case it's maybe more appropriate. https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/u... components/ntp_tiles/update_default_sites_resources.py:95: parser.add_argument("--skip_icons", action="store_true", On 2017/02/16 18:54:16, sfiera wrote: > In Chromium, I think we typically use --skip-icons, not --skip_icons style flag > naming. (still accessed as args.skip_icons later) > > I would also say "--no-icons" "--no-resizing". I think the --no pattern is more > common, at least at Google. Done. https://codereview.chromium.org/2695713004/diff/100001/components/ntp_tiles/u... components/ntp_tiles/update_default_sites_resources.py:100: type=str, On 2017/02/16 18:54:16, sfiera wrote: > See also argparse.FileType. There is little advantage using that as I would need to distinguish source (file or urllib response) later.
Patchset #7 (id:140001) has been deleted
Description was changed from ========== Add baked-in favicons for default popular sites on NTP This CL adds the 8 icons for the most popular sites tiles on the NTP. By providing the images as resource, Chromium will show high resolution images during the first run even if there is little to no data connection. Updating popular sites and their icons happens by manually executing the python script in components/ntp_tiles. BUG=672939 ========== to ========== Add baked-in favicons for default popular sites on NTP This CL adds the 8 icons for the most popular sites tiles on the NTP. By providing the images as resource, Chromium will show high resolution images during the first run even if there is little to no data connection. The image sizes amount to 56 KiB. Updating popular sites and their icons happens by manually executing the python script in components/ntp_tiles. BUG=672939 ==========
Hi Chris, would you please check the updated state without the icons and the script? (Most notable, maybe controversial change: the grdp file uses different resources for _google_chrome builds)
https://codereview.chromium.org/2695713004/diff/240001/.gitignore File .gitignore (right): https://codereview.chromium.org/2695713004/diff/240001/.gitignore#newcode152 .gitignore:152: /components/ntp_tiles/resources resources or resources/internal? https://codereview.chromium.org/2695713004/diff/240001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher.h (right): https://codereview.chromium.org/2695713004/diff/240001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:27: // first place. This docstring confuses me, probably because it's documenting confusing behavior. I think the one caller doesn't care about Run(false), so maybe we can remove those calls and drop the parameter? Then it will be simple: invokes the callback when a new icon is available (could be twice). I don't remember why the false notifications existed. Maybe just because it's easier to test; I'm not sure how best to test if it doesn't call back. https://codereview.chromium.org/2695713004/diff/240001/components/ntp_tiles/r... File components/ntp_tiles/resources/default_popular_sites.json (right): https://codereview.chromium.org/2695713004/diff/240001/components/ntp_tiles/r... components/ntp_tiles/resources/default_popular_sites.json:1: [{"url":"https://m.facebook.com/","large_icon_url":"https://static.xx.fbcdn.net/rsrc.php/v3/ya/r/O2aKM2iSbOw.png","title":"Facebook - Log In or Sign Up"},{"url":"https://m.youtube.com/","large_icon_url":"https://s.ytimg.com/yt...: Online Shopping for Electronics, Apparel, Computers, Books, DVDs & more"},{"url":"https://en.m.wikipedia.org/","large_icon_url":"https://en.m.wikipedia.org/static/apple-touch/wikipedia.png","title":"Wikipedia, the free encyclopedia"},{"url":"http://www.espn.com/","large_icon_url":"http://a.espncdn.com/wireless/mw5/r1/images/bookmark-icons/espn_icon-152x152.min.png","title":"ESPN: The Worldwide Leader in Sports"},{"url":"https://www.yahoo.com/","large_icon_url":"https://s.yimg.com/dh/ap/default/130909/y_200_a.png","title":"Yahoo"},{"url":"https://www.instagram.com/","large_icon_url":"https://instagramstatic-a.akamaihd.net/h1/images/ico/favicon-192.png/b407fa101800.png","title":"Instagram"},{"url":"http://m.ebay.com/","large_icon_url":"http://ir.ebaystatic.com/pictures/aw/pics/mobile/images/apple-touch-icon.png","title":"Electronics, Cars, Fashion, Collectibles, Coupons and More | eBay"}] Could we make this different from the internal version of the file? If we update that one but keep this one the same, it will be confusing. Some possible URLs: https://www.chromium.org/ https://codereview.chromium.org/ https://chromium.googlesource.com/ https://developers.google.com/open-source/ https://freenode.net/ https://github.com/chromium/ Actually, I think it would be a fun mini-project to make a bunch of fake sites that have features we want to test on the NTP, but I won't do that any time soon :)
treib@chromium.org changed reviewers: + treib@chromium.org
Some drive-by comments. The description should probably be updated, now that the images and the script aren't here anymore. Overall, I'm not too happy with having two versions of the file. Is there any reason why we need a separate copy in the internal repo? https://codereview.chromium.org/2695713004/diff/240001/components/ntp_tiles/p... File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2695713004/diff/240001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:154: base::ListValue::From(base::JSONReader().ReadToValue( Just use the static Read() instead of creating a JSONReader instance? https://codereview.chromium.org/2695713004/diff/240001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:158: DCHECK(sites); Put this outside the #if ? https://codereview.chromium.org/2695713004/diff/240001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:170: return base::MakeUnique<base::ListValue>(); Should this be in an #else? Otherwise I'd expect "unreachable code" warnings.
https://codereview.chromium.org/2695713004/diff/240001/.gitignore File .gitignore (right): https://codereview.chromium.org/2695713004/diff/240001/.gitignore#newcode152 .gitignore:152: /components/ntp_tiles/resources On 2017/02/27 11:42:20, sfiera wrote: > resources or resources/internal? Thanks! https://codereview.chromium.org/2695713004/diff/240001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher.h (right): https://codereview.chromium.org/2695713004/diff/240001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:27: // first place. On 2017/02/27 11:42:20, sfiera wrote: > This docstring confuses me, probably because it's documenting confusing > behavior. I think the one caller doesn't care about Run(false), so maybe we can > remove those calls and drop the parameter? Then it will be simple: invokes the > callback when a new icon is available (could be twice). I don't remember why the > false notifications existed. Maybe just because it's easier to test; I'm not > sure how best to test if it doesn't call back. Dropped the bool. If you don't like this change, I added it in a separate Patch Set and could also easily split it into a new CL. MockFunctions (as currently used) support .Times(0) cardinality and a TestTaskRunner can ensure that the task was finished. As I learned, we prefer RunLoops over TestTaskRunner, so let me know if you prefer to keep it like it used to be. https://codereview.chromium.org/2695713004/diff/240001/components/ntp_tiles/p... File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2695713004/diff/240001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:154: base::ListValue::From(base::JSONReader().ReadToValue( On 2017/02/27 12:41:49, Marc Treib wrote: > Just use the static Read() instead of creating a JSONReader instance? Done. https://codereview.chromium.org/2695713004/diff/240001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:158: DCHECK(sites); On 2017/02/27 12:41:49, Marc Treib wrote: > Put this outside the #if ? Done. https://codereview.chromium.org/2695713004/diff/240001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:170: return base::MakeUnique<base::ListValue>(); On 2017/02/27 12:41:49, Marc Treib wrote: > Should this be in an #else? Otherwise I'd expect "unreachable code" warnings. It is now, but there haven't been warnings. https://codereview.chromium.org/2695713004/diff/240001/components/ntp_tiles/r... File components/ntp_tiles/resources/default_popular_sites.json (right): https://codereview.chromium.org/2695713004/diff/240001/components/ntp_tiles/r... components/ntp_tiles/resources/default_popular_sites.json:1: [{"url":"https://m.facebook.com/","large_icon_url":"https://static.xx.fbcdn.net/rsrc.php/v3/ya/r/O2aKM2iSbOw.png","title":"Facebook - Log In or Sign Up"},{"url":"https://m.youtube.com/","large_icon_url":"https://s.ytimg.com/yt...: Online Shopping for Electronics, Apparel, Computers, Books, DVDs & more"},{"url":"https://en.m.wikipedia.org/","large_icon_url":"https://en.m.wikipedia.org/static/apple-touch/wikipedia.png","title":"Wikipedia, the free encyclopedia"},{"url":"http://www.espn.com/","large_icon_url":"http://a.espncdn.com/wireless/mw5/r1/images/bookmark-icons/espn_icon-152x152.min.png","title":"ESPN: The Worldwide Leader in Sports"},{"url":"https://www.yahoo.com/","large_icon_url":"https://s.yimg.com/dh/ap/default/130909/y_200_a.png","title":"Yahoo"},{"url":"https://www.instagram.com/","large_icon_url":"https://instagramstatic-a.akamaihd.net/h1/images/ico/favicon-192.png/b407fa101800.png","title":"Instagram"},{"url":"http://m.ebay.com/","large_icon_url":"http://ir.ebaystatic.com/pictures/aw/pics/mobile/images/apple-touch-icon.png","title":"Electronics, Cars, Fashion, Collectibles, Coupons and More | eBay"}] On 2017/02/27 11:42:20, sfiera wrote: > Could we make this different from the internal version of the file? If we update > that one but keep this one the same, it will be confusing. > > Some possible URLs: > https://www.chromium.org/ > https://codereview.chromium.org/ > https://chromium.googlesource.com/ > https://developers.google.com/open-source/ > https://freenode.net/ > https://github.com/chromium/ > > Actually, I think it would be a fun mini-project to make a bunch of fake sites > that have features we want to test on the NTP, but I won't do that any time soon > :) Okay. I think pretty-printed sites are more appropriate then, too. I quickly added some sites (with different properties).
Update: All internal dependencies have landed.
On 2017/02/28 15:40:20, fhorschig wrote: > Update: All internal dependencies have landed. ...but not DEPSed in yet, right? And I still think the description needs to be updated :)
Description was changed from ========== Add baked-in favicons for default popular sites on NTP This CL adds the 8 icons for the most popular sites tiles on the NTP. By providing the images as resource, Chromium will show high resolution images during the first run even if there is little to no data connection. The image sizes amount to 56 KiB. Updating popular sites and their icons happens by manually executing the python script in components/ntp_tiles. BUG=672939 ========== to ========== Add access to baked-in favicons for default popular sites on NTP This change enables branded builds to use icons for the first opened New Tab Page by adding them to the initial icon cache. Non-branded builds receive a new set of popular site tiles. (This profits developers mainly.) BUG=672939 ==========
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...
On 2017/02/28 15:44:20, Marc Treib wrote: > On 2017/02/28 15:40:20, fhorschig wrote: > > Update: All internal dependencies have landed. > > ...but not DEPSed in yet, right? It is. > > And I still think the description needs to be updated :) Absolutely. Updated.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher.h (right): https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:24: // newly fetched or loaded from a default source. How about the failure case? I see you removed the boolean but that seems, at least, orthogonal to your patch. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:27: const base::Callback<void()>& icon_available) = 0; Having the callback called twice is not something I like much. As a slightly less ugly alternative, you could introduce a second callback. More interestingly: a) Please talk to UX whether we should actually update icons in this situation. b) For longer term, ntp_tiles might as well observe all FaviconService icon change notifications. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher_impl_unittest.cc (right): https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_impl_unittest.cc:56: return pack_path; // Continue Loading pack file. Nit: lowercase "loading". Similar occurrences follow. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_impl_unittest.cc:91: bool HasProvidedResource(int resource_id) { Have you considered using a mock instead? This would make most sense of only one or two fake functions above are exercised (or even if more are used, if the default return values would suffice, which is what you're doing anyway -- then NiceMock). It could look like this: EXPECT_CALL(mock_resource_delegate, GetImageNamed(12345)) .WillOnce(Return(gfx::test::CreateImage(64, 64))); https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/p... File components/ntp_tiles/popular_sites.h (right): https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/p... components/ntp_tiles/popular_sites.h:40: int default_resource_id; // <0 if there is none. Used for popular defaults. Did you choose -1 because 0 is a valid resource id? If not, 0 would be more appropriate, since it's the default value of ints. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/p... components/ntp_tiles/popular_sites.h:40: int default_resource_id; // <0 if there is none. Used for popular defaults. Rename such that the term 'icon' is part of the name? IIUC, this is only about the icon. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/p... File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:139: void SetDefaultResourceForSite(base::ListValue* sites, Nit: output params should be last according to the style guide. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:152: #if defined(OS_ANDROID) || defined(OS_IOS) Nit: how about inverting this logic and returning early? https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:160: for (int resource : Nit: rename to resource_id for consistency with the signature introduced earlier. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/p... File components/ntp_tiles/popular_sites_impl_unittest.cc (right): https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl_unittest.cc:244: for (const auto& site : popular_sites->sites()) { Perhaps ASSERT the list is non-empty, just in case? https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl_unittest.cc:245: EXPECT_TRUE(site.default_resource_id > 0); Nit: EXPECT_THAT(...resource_id, Gt(0));
Adressed the comments but broke MostVisitedTests in the process. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher.h (right): https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:24: // newly fetched or loaded from a default source. On 2017/03/01 08:48:12, mastiz wrote: > How about the failure case? I see you removed the boolean but that seems, at > least, orthogonal to your patch. There is exactly one caller (as the previous reviewer pointed out) which doesn't even need the failure case. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:27: const base::Callback<void()>& icon_available) = 0; On 2017/03/01 08:48:12, mastiz wrote: > Having the callback called twice is not something I like much. > > As a slightly less ugly alternative, you could introduce a second callback. We now have two OnceCallbacks > More interestingly: > a) Please talk to UX whether we should actually update icons in this situation. Asked and will receive an answer tomorrow. I suggest that this behavior should be changed in the near future. This CL contains already more changes than I prefer. > b) For longer term, ntp_tiles might as well observe all FaviconService icon > change notifications. Absolutely. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher_impl_unittest.cc (right): https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_impl_unittest.cc:56: return pack_path; // Continue Loading pack file. On 2017/03/01 08:48:12, mastiz wrote: > Nit: lowercase "loading". Similar occurrences follow. Done. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_impl_unittest.cc:91: bool HasProvidedResource(int resource_id) { On 2017/03/01 08:48:12, mastiz wrote: > Have you considered using a mock instead? > > This would make most sense of only one or two fake functions above are exercised > (or even if more are used, if the default return values would suffice, which is > what you're doing anyway -- then NiceMock). > > It could look like this: > EXPECT_CALL(mock_resource_delegate, GetImageNamed(12345)) > .WillOnce(Return(gfx::test::CreateImage(64, 64))); I didn't. But now I did. I tried a complete Mock and a partial Mock. Both started to hang (for reasons I cannot explain because it only occurred on release build tests on Android). Likely connected to the registering in the constructor. I reverted after more than an hour of debugging. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/p... File components/ntp_tiles/popular_sites.h (right): https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/p... components/ntp_tiles/popular_sites.h:40: int default_resource_id; // <0 if there is none. Used for popular defaults. On 2017/03/01 08:48:12, mastiz wrote: > Did you choose -1 because 0 is a valid resource id? If not, 0 would be more > appropriate, since it's the default value of ints. I was asked to change from -1 to 0. Partly because it shouldn't be a valid default. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/p... components/ntp_tiles/popular_sites.h:40: int default_resource_id; // <0 if there is none. Used for popular defaults. On 2017/03/01 08:48:12, mastiz wrote: > Rename such that the term 'icon' is part of the name? IIUC, this is only about > the icon. Done. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/p... File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:139: void SetDefaultResourceForSite(base::ListValue* sites, On 2017/03/01 08:48:12, mastiz wrote: > Nit: output params should be last according to the style guide. Done. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:152: #if defined(OS_ANDROID) || defined(OS_IOS) On 2017/03/01 08:48:12, mastiz wrote: > Nit: how about inverting this logic and returning early? Done. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl.cc:160: for (int resource : On 2017/03/01 08:48:12, mastiz wrote: > Nit: rename to resource_id for consistency with the signature introduced > earlier. Done. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/p... File components/ntp_tiles/popular_sites_impl_unittest.cc (right): https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl_unittest.cc:244: for (const auto& site : popular_sites->sites()) { On 2017/03/01 08:48:12, mastiz wrote: > Perhaps ASSERT the list is non-empty, just in case? Done. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/p... components/ntp_tiles/popular_sites_impl_unittest.cc:245: EXPECT_TRUE(site.default_resource_id > 0); On 2017/03/01 08:48:12, mastiz wrote: > Nit: EXPECT_THAT(...resource_id, Gt(0)); Done.
Found the bug.
Patchset #19 (id:400001) has been deleted
Thanks! LGTM with minor comments. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher.h (right): https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:24: // newly fetched or loaded from a default source. On 2017/03/01 20:57:35, fhorschig wrote: > On 2017/03/01 08:48:12, mastiz wrote: > > How about the failure case? I see you removed the boolean but that seems, at > > least, orthogonal to your patch. > > There is exactly one caller (as the previous reviewer pointed out) > which doesn't even need the failure case. I don't feel strongly but it feels wrong that an API doesn't notify completion in some circumstances. I would also expect this useful or testing (that is, how would you test otherwise that the callback won't be called?). https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:27: const base::Callback<void()>& icon_available) = 0; On 2017/03/01 20:57:35, fhorschig wrote: > On 2017/03/01 08:48:12, mastiz wrote: > > Having the callback called twice is not something I like much. > > > > As a slightly less ugly alternative, you could introduce a second callback. > We now have two OnceCallbacks > > > More interestingly: > > a) Please talk to UX whether we should actually update icons in this > situation. > Asked and will receive an answer tomorrow. I suggest that this behavior > should be changed in the near future. This CL contains already more > changes than I prefer. > > > b) For longer term, ntp_tiles might as well observe all FaviconService icon > > change notifications. > > Absolutely. > Ack, let's wait for UX feedback, since this could simplify the code. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher_impl_unittest.cc (right): https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_impl_unittest.cc:91: bool HasProvidedResource(int resource_id) { On 2017/03/01 20:57:35, fhorschig wrote: > On 2017/03/01 08:48:12, mastiz wrote: > > Have you considered using a mock instead? > > > > This would make most sense of only one or two fake functions above are > exercised > > (or even if more are used, if the default return values would suffice, which > is > > what you're doing anyway -- then NiceMock). > > > > It could look like this: > > EXPECT_CALL(mock_resource_delegate, GetImageNamed(12345)) > > .WillOnce(Return(gfx::test::CreateImage(64, 64))); > > I didn't. But now I did. I tried a complete Mock and a partial Mock. > Both started to hang (for reasons I cannot explain because it only > occurred on release build tests on Android). Likely connected to the > registering in the constructor. > I reverted after more than an hour of debugging. I can help you with that debugging, it could save ~50 LoC of boilterplate. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_impl_unittest.cc:162: base::Callback<Fn> BindMockFunction(MockFunction<Fn>* function) { Optional: there's MockCallback which is probably what these tests should use. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/p... File components/ntp_tiles/popular_sites.h (right): https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/p... components/ntp_tiles/popular_sites.h:40: int default_resource_id; // <0 if there is none. Used for popular defaults. On 2017/03/01 20:57:35, fhorschig wrote: > On 2017/03/01 08:48:12, mastiz wrote: > > Did you choose -1 because 0 is a valid resource id? If not, 0 would be more > > appropriate, since it's the default value of ints. > > I was asked to change from -1 to 0. Partly because it shouldn't be a > valid default. Did you mean from 0 to -1? Is 0 a valid resource ID? https://codereview.chromium.org/2695713004/diff/420001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher_impl_unittest.cc (right): https://codereview.chromium.org/2695713004/diff/420001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_impl_unittest.cc:266: base::RunLoop fetch_loop; With the adoption of a mock delegate, I believe you wouldn't need two RunLoops.
Patchset #20 (id:440001) has been deleted
Patchset #17 (id:360001) has been deleted
The CQ bit was checked by fhorschig@chromium.org to run a CQ dry run
Thank you for the help! https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher.h (right): https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:24: // newly fetched or loaded from a default source. On 2017/03/02 11:05:39, mastiz wrote: > On 2017/03/01 20:57:35, fhorschig wrote: > > On 2017/03/01 08:48:12, mastiz wrote: > > > How about the failure case? I see you removed the boolean but that seems, at > > > least, orthogonal to your patch. > > > > There is exactly one caller (as the previous reviewer pointed out) > > which doesn't even need the failure case. > > I don't feel strongly but it feels wrong that an API doesn't notify completion Relatable. I will consider reintroducing it. > in some circumstances. I would also expect this useful or testing (that is, how > would you test otherwise that the callback won't be called?). So far, we wait for pending tasks to be finished. The thing about the icon cacher is, that it works in the background and failure are usually not interesting to anyone at all. As soon as we rely on that (even if just for metrics), we should improve this class. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher.h:27: const base::Callback<void()>& icon_available) = 0; On 2017/03/02 11:05:39, mastiz wrote: > On 2017/03/01 20:57:35, fhorschig wrote: > > On 2017/03/01 08:48:12, mastiz wrote: > > > Having the callback called twice is not something I like much. > > > > > > As a slightly less ugly alternative, you could introduce a second callback. > > We now have two OnceCallbacks > > > > > More interestingly: > > > a) Please talk to UX whether we should actually update icons in this > > situation. > > Asked and will receive an answer tomorrow. I suggest that this behavior > > should be changed in the near future. This CL contains already more > > changes than I prefer. > > > > > b) For longer term, ntp_tiles might as well observe all FaviconService icon > > > change notifications. > > > > Absolutely. > > > > Ack, let's wait for UX feedback, since this could simplify the code. Acknowledged. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher_impl_unittest.cc (right): https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_impl_unittest.cc:91: bool HasProvidedResource(int resource_id) { On 2017/03/02 11:05:39, mastiz wrote: > On 2017/03/01 20:57:35, fhorschig wrote: > > On 2017/03/01 08:48:12, mastiz wrote: > > > Have you considered using a mock instead? > > > > > > This would make most sense of only one or two fake functions above are > > exercised > > > (or even if more are used, if the default return values would suffice, which > > is > > > what you're doing anyway -- then NiceMock). > > > > > > It could look like this: > > > EXPECT_CALL(mock_resource_delegate, GetImageNamed(12345)) > > > .WillOnce(Return(gfx::test::CreateImage(64, 64))); > > > > I didn't. But now I did. I tried a complete Mock and a partial Mock. > > Both started to hang (for reasons I cannot explain because it only > > occurred on release build tests on Android). Likely connected to the > > registering in the constructor. > > I reverted after more than an hour of debugging. > > I can help you with that debugging, it could save ~50 LoC of boilterplate. Thanks, it did. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_impl_unittest.cc:162: base::Callback<Fn> BindMockFunction(MockFunction<Fn>* function) { On 2017/03/02 11:05:39, mastiz wrote: > Optional: there's MockCallback which is probably what these tests should use. Done. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/p... File components/ntp_tiles/popular_sites.h (right): https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/p... components/ntp_tiles/popular_sites.h:40: int default_resource_id; // <0 if there is none. Used for popular defaults. On 2017/03/02 11:05:39, mastiz wrote: > On 2017/03/01 20:57:35, fhorschig wrote: > > On 2017/03/01 08:48:12, mastiz wrote: > > > Did you choose -1 because 0 is a valid resource id? If not, 0 would be more > > > appropriate, since it's the default value of ints. > > > > I was asked to change from -1 to 0. Partly because it shouldn't be a > > valid default. > > Did you mean from 0 to -1? Is 0 a valid resource ID? Yes, this is what I meant. 0 is (technically) valid although all resources are assigned ranges [a,b] with 0 < a < b. https://codereview.chromium.org/2695713004/diff/420001/components/ntp_tiles/i... File components/ntp_tiles/icon_cacher_impl_unittest.cc (right): https://codereview.chromium.org/2695713004/diff/420001/components/ntp_tiles/i... components/ntp_tiles/icon_cacher_impl_unittest.cc:266: base::RunLoop fetch_loop; On 2017/03/02 11:05:39, mastiz wrote: > With the adoption of a mock delegate, I believe you wouldn't need two RunLoops. As we discussed, more elegant solutions would require rewriting large parts of this test.
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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #20 (id:480001) has been deleted
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...
fhorschig@chromium.org changed reviewers: - sfiera@chromium.org
moved sfiera (OOO) to CC (Resting concerns about copyright have been handled in the meantime.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #21 (id:520001) has been deleted
Patchset #21 (id:540001) has been deleted
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...
sky@chromium.org changed reviewers: + sky@chromium.org
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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #26 (id:660001) has been deleted
The CQ bit was checked by fhorschig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mastiz@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2695713004/#ps680001 (title: "Rebase")
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...)
blundell@chromium.org changed reviewers: + blundell@chromium.org
https://codereview.chromium.org/2695713004/diff/680001/components/ntp_tiles/DEPS File components/ntp_tiles/DEPS (right): https://codereview.chromium.org/2695713004/diff/680001/components/ntp_tiles/D... components/ntp_tiles/DEPS:12: "+components/resources:components_resources", If you're using this, it needs to be added as a dependency in GN, and it should just be "+components/resources", here. I don't see it used anywhere in this CL though, unless I'm missing something?
https://codereview.chromium.org/2695713004/diff/680001/components/ntp_tiles/DEPS File components/ntp_tiles/DEPS (right): https://codereview.chromium.org/2695713004/diff/680001/components/ntp_tiles/D... components/ntp_tiles/DEPS:12: "+components/resources:components_resources", On 2017/03/02 21:58:13, blundell wrote: > If you're using this, it needs to be added as a dependency in GN, and it should > just be "+components/resources", here. > > I don't see it used anywhere in this CL though, unless I'm missing something? Done.
The CQ bit was checked by fhorschig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, mastiz@chromium.org Link to the patchset: https://codereview.chromium.org/2695713004/#ps720001 (title: "... and deleting unused DEPS")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by fhorschig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, mastiz@chromium.org Link to the patchset: https://codereview.chromium.org/2695713004/#ps740001 (title: "Rebase.")
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": 740001, "attempt_start_ts": 1488493112003950, "parent_rev": "204c508db463f78401249576c2b5001079da9650", "commit_rev": "fed34be47fbaa165e6d0817f55ae907a97ea00e8"}
Message was sent while issue was closed.
Description was changed from ========== Add access to baked-in favicons for default popular sites on NTP This change enables branded builds to use icons for the first opened New Tab Page by adding them to the initial icon cache. Non-branded builds receive a new set of popular site tiles. (This profits developers mainly.) BUG=672939 ========== to ========== Add access to baked-in favicons for default popular sites on NTP This change enables branded builds to use icons for the first opened New Tab Page by adding them to the initial icon cache. Non-branded builds receive a new set of popular site tiles. (This profits developers mainly.) BUG=672939 Review-Url: https://codereview.chromium.org/2695713004 Cr-Commit-Position: refs/heads/master@{#454424} Committed: https://chromium.googlesource.com/chromium/src/+/fed34be47fbaa165e6d0817f55ae... ==========
Message was sent while issue was closed.
Committed patchset #29 (id:740001) as https://chromium.googlesource.com/chromium/src/+/fed34be47fbaa165e6d0817f55ae... |