Chromium Code Reviews
Help | Chromium Project | Sign in
(2)

Issue 2695713004: Add baked-in favicons for default popular sites on NTP

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 days, 7 hours ago by fhorschig
Modified:
2 days, 9 hours ago
Reviewers:
mastiz, jkrcal, sfiera, thanhphong04071993
CC:
chromium-reviews, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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

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 : [TEMP] revision for iOS w/ new tests #

Patch Set 8 : refine test and download script #

Unified diffs Side-by-side diffs Delta from patch set Stats (+337 lines, -63 lines) Patch
M components/ntp_tiles/BUILD.gn View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M components/ntp_tiles/DEPS View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M components/ntp_tiles/icon_cacher.h View 1 2 3 4 5 6 1 chunk +5 lines, -2 lines 0 comments Download
M components/ntp_tiles/icon_cacher_impl.h View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M components/ntp_tiles/icon_cacher_impl.cc View 1 2 3 4 5 6 7 2 chunks +34 lines, -13 lines 0 comments Download
M components/ntp_tiles/icon_cacher_impl_unittest.cc View 1 2 3 4 5 6 6 chunks +101 lines, -5 lines 0 comments Download
M components/ntp_tiles/popular_sites.h View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M components/ntp_tiles/popular_sites_impl.cc View 1 2 3 4 5 6 3 chunks +24 lines, -1 line 0 comments Download
M components/ntp_tiles/popular_sites_impl_unittest.cc View 1 2 3 4 1 chunk +12 lines, -0 lines 0 comments Download
M components/ntp_tiles/resources/default_popular_sites.json View 1 2 3 4 5 6 1 chunk +1 line, -42 lines 0 comments Download
A components/ntp_tiles/resources/icon0.png View 1 2 3 4 5 6 Binary file 0 comments Download
A components/ntp_tiles/resources/icon1.png View 1 2 3 4 5 6 Binary file 0 comments Download
A components/ntp_tiles/update_default_sites_resources.py View 1 2 3 4 5 6 7 1 chunk +146 lines, -0 lines 0 comments Download
M components/resources/ntp_tiles_resources.grdp View 1 2 3 4 1 chunk +8 lines, -0 lines 0 comments Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Messages

Total messages: 21 (6 generated)
fhorschig
Would you please have a look? BTW: Do we have some kind of MockFaviconService that ...
6 days, 7 hours ago (2017-02-13 18:18:29 UTC) #2
sfiera
On 2017/02/13 18:18:29, fhorschig wrote: > BTW: Do we have some kind of MockFaviconService that ...
6 days, 7 hours ago (2017-02-13 18:36:36 UTC) #3
jkrcal
https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/popular_sites_impl.cc File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/popular_sites_impl.cc#newcode139 components/ntp_tiles/popular_sites_impl.cc:139: PopularSites::SitesVector GetDefaultFromPrefs( drive-by comment: When is this function called? ...
5 days, 17 hours ago (2017-02-14 08:23:06 UTC) #5
Michael van Ouwerkerk
Just some drive-by questions: why don't these icons have file extensions (.png)? Have they been ...
5 days, 15 hours ago (2017-02-14 09:49:03 UTC) #6
sfiera
https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/popular_sites_impl.cc File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/popular_sites_impl.cc#newcode139 components/ntp_tiles/popular_sites_impl.cc:139: PopularSites::SitesVector GetDefaultFromPrefs( On 2017/02/14 08:23:06, jkrcal wrote: > drive-by ...
5 days, 15 hours ago (2017-02-14 09:59:16 UTC) #7
tschumann
https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/popular_sites_impl.cc File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/popular_sites_impl.cc#newcode147 components/ntp_tiles/popular_sites_impl.cc:147: #if defined(OS_ANDROID) || defined(OS_IOS) i wonder why is this ...
5 days, 13 hours ago (2017-02-14 11:56:51 UTC) #9
mastiz
https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/popular_sites_impl.cc File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/popular_sites_impl.cc#newcode151 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 ...
5 days, 10 hours ago (2017-02-14 14:44:59 UTC) #11
fhorschig
Thanks for the many comments! The current patchset addresses mainly c++ issues. Adjusting the python ...
5 days, 8 hours ago (2017-02-14 17:29:03 UTC) #12
sfiera
Is this ready for another look? I'd been waiting for another message about the python ...
4 days, 9 hours ago (2017-02-15 16:38:47 UTC) #13
fhorschig
Thank you for reminding me! https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/update_default_sites_resources.py File components/ntp_tiles/update_default_sites_resources.py (right): https://codereview.chromium.org/2695713004/diff/1/components/ntp_tiles/update_default_sites_resources.py#newcode18 components/ntp_tiles/update_default_sites_resources.py:18: print("... done. (" + ...
4 days, 7 hours ago (2017-02-15 18:14:35 UTC) #14
Thanhphong04071993
3 days, 14 hours ago (2017-02-16 11:00:41 UTC) #16
sfiera
https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/icon_cacher_impl.h File components/ntp_tiles/icon_cacher_impl.h (right): https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/icon_cacher_impl.h#newcode66 components/ntp_tiles/icon_cacher_impl.h:66: std::map<const std::string, DefaultIcon> default_icons_; Instead of having a map ...
3 days, 14 hours ago (2017-02-16 11:14:23 UTC) #18
fhorschig
https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/icon_cacher_impl.h File components/ntp_tiles/icon_cacher_impl.h (right): https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/icon_cacher_impl.h#newcode66 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: ...
3 days, 8 hours ago (2017-02-16 17:06:43 UTC) #19
sfiera
This mostly looks OK, but I'm not ready to approve until we get some word ...
3 days, 6 hours ago (2017-02-16 18:54:17 UTC) #20
fhorschig
2 days, 9 hours ago (2017-02-17 16:24:04 UTC) #21
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f8e48bd