Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(194)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 months, 1 week ago by fhorschig
Modified:
6 months, 3 weeks ago
Reviewers:
sky, mastiz, thanhphong04071993, jkrcal, Marc Treib, blundell
CC:
chromium-reviews, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org, sfiera
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+318 lines, -112 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 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 1 chunk +2 lines, -1 line 0 comments Download
M components/ntp_tiles/icon_cacher.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +7 lines, -3 lines 0 comments Download
M components/ntp_tiles/icon_cacher_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +8 lines, -3 lines 0 comments Download
M components/ntp_tiles/icon_cacher_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 27 28 2 chunks +30 lines, -10 lines 0 comments Download
M components/ntp_tiles/icon_cacher_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 27 28 11 chunks +143 lines, -28 lines 0 comments Download
M components/ntp_tiles/most_visited_sites.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -1 line 0 comments Download
M components/ntp_tiles/most_visited_sites.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +6 lines, -7 lines 0 comments Download
M components/ntp_tiles/most_visited_sites_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +4 lines, -3 lines 0 comments Download
M components/ntp_tiles/popular_sites.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -0 lines 0 comments Download
M components/ntp_tiles/popular_sites_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 16 18 21 2 chunks +31 lines, -5 lines 0 comments Download
M components/ntp_tiles/popular_sites_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 4 chunks +27 lines, -10 lines 0 comments Download
M components/ntp_tiles/resources/default_popular_sites.json View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +40 lines, -40 lines 0 comments Download
M components/resources/ntp_tiles_resources.grdp View 1 2 3 4 5 6 7 8 1 chunk +17 lines, -1 line 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 85 (48 generated)
fhorschig
Would you please have a look? BTW: Do we have some kind of MockFaviconService that ...
7 months, 1 week 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 ...
7 months, 1 week 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? ...
7 months, 1 week 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 ...
7 months, 1 week 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 ...
7 months, 1 week 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 ...
7 months, 1 week 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 ...
7 months, 1 week 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 ...
7 months, 1 week 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 ...
7 months, 1 week 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. (" + ...
7 months, 1 week ago (2017-02-15 18:14:35 UTC) #14
Thanhphong04071993
7 months, 1 week 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 ...
7 months, 1 week 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: ...
7 months, 1 week 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 ...
7 months, 1 week ago (2017-02-16 18:54:17 UTC) #20
fhorschig
I wouldn't have expected approval whith the copyright discussion still in place ;) https://codereview.chromium.org/2695713004/diff/80001/components/ntp_tiles/update_default_sites_resources.py File ...
7 months ago (2017-02-17 16:24:04 UTC) #21
fhorschig
Hi Chris, would you please check the updated state without the icons and the script? ...
6 months, 3 weeks ago (2017-02-27 11:10:28 UTC) #24
sfiera
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/icon_cacher.h File components/ntp_tiles/icon_cacher.h (right): https://codereview.chromium.org/2695713004/diff/240001/components/ntp_tiles/icon_cacher.h#newcode27 ...
6 months, 3 weeks ago (2017-02-27 11:42:20 UTC) #25
Marc Treib
Some drive-by comments. The description should probably be updated, now that the images and the ...
6 months, 3 weeks ago (2017-02-27 12:41:49 UTC) #27
fhorschig
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 ...
6 months, 3 weeks ago (2017-02-28 13:13:12 UTC) #28
fhorschig
Update: All internal dependencies have landed.
6 months, 3 weeks ago (2017-02-28 15:40:20 UTC) #29
Marc Treib
On 2017/02/28 15:40:20, fhorschig wrote: > Update: All internal dependencies have landed. ...but not DEPSed ...
6 months, 3 weeks ago (2017-02-28 15:44:20 UTC) #30
fhorschig
On 2017/02/28 15:44:20, Marc Treib wrote: > On 2017/02/28 15:40:20, fhorschig wrote: > > Update: ...
6 months, 3 weeks ago (2017-02-28 15:59:04 UTC) #34
mastiz
https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/icon_cacher.h File components/ntp_tiles/icon_cacher.h (right): https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/icon_cacher.h#newcode24 components/ntp_tiles/icon_cacher.h:24: // newly fetched or loaded from a default source. ...
6 months, 3 weeks ago (2017-03-01 08:48:12 UTC) #37
fhorschig
Adressed the comments but broke MostVisitedTests in the process. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/icon_cacher.h File components/ntp_tiles/icon_cacher.h (right): https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/icon_cacher.h#newcode24 components/ntp_tiles/icon_cacher.h:24: ...
6 months, 3 weeks ago (2017-03-01 20:57:36 UTC) #38
fhorschig
Found the bug.
6 months, 3 weeks ago (2017-03-02 08:49:09 UTC) #39
mastiz
Thanks! LGTM with minor comments. https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/icon_cacher.h File components/ntp_tiles/icon_cacher.h (right): https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/icon_cacher.h#newcode24 components/ntp_tiles/icon_cacher.h:24: // newly fetched or ...
6 months, 3 weeks ago (2017-03-02 11:05:39 UTC) #41
fhorschig
Thank you for the help! https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/icon_cacher.h File components/ntp_tiles/icon_cacher.h (right): https://codereview.chromium.org/2695713004/diff/280001/components/ntp_tiles/icon_cacher.h#newcode24 components/ntp_tiles/icon_cacher.h:24: // newly fetched or ...
6 months, 3 weeks ago (2017-03-02 11:56:24 UTC) #45
fhorschig
moved sfiera (OOO) to CC (Resting concerns about copyright have been handled in the meantime.)
6 months, 3 weeks ago (2017-03-02 14:19:26 UTC) #53
sky
LGTM
6 months, 3 weeks ago (2017-03-02 19:23:36 UTC) #61
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2695713004/680001
6 months, 3 weeks ago (2017-03-02 21:33:17 UTC) #69
commit-bot: I haz the power
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_presubmit/builds/377236)
6 months, 3 weeks ago (2017-03-02 21:42:46 UTC) #71
blundell
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/DEPS#newcode12 components/ntp_tiles/DEPS:12: "+components/resources:components_resources", If you're using this, it needs to be ...
6 months, 3 weeks ago (2017-03-02 21:58:13 UTC) #73
fhorschig
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/DEPS#newcode12 components/ntp_tiles/DEPS:12: "+components/resources:components_resources", On 2017/03/02 21:58:13, blundell wrote: > If you're ...
6 months, 3 weeks ago (2017-03-02 22:10:54 UTC) #74
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2695713004/720001
6 months, 3 weeks ago (2017-03-02 22:11:52 UTC) #77
commit-bot: I haz the power
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/163906) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, ...
6 months, 3 weeks ago (2017-03-02 22:15:16 UTC) #79
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2695713004/740001
6 months, 3 weeks ago (2017-03-02 22:19:09 UTC) #82
commit-bot: I haz the power
6 months, 3 weeks ago (2017-03-02 23:17:04 UTC) #85
Message was sent while issue was closed.
Committed patchset #29 (id:740001) as
https://chromium.googlesource.com/chromium/src/+/fed34be47fbaa165e6d0817f55ae...
Sign in to reply to this message.

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