Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(212)

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

Created:
3 years, 10 months ago by fhorschig
Modified:
3 years, 9 months 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

Messages

Total messages: 85 (48 generated)
fhorschig
Would you please have a look? BTW: Do we have some kind of MockFaviconService that ...
3 years, 10 months 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 ...
3 years, 10 months 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? ...
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months 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 ...
3 years, 10 months 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. (" + ...
3 years, 10 months ago (2017-02-15 18:14:35 UTC) #14
Thanhphong04071993
3 years, 10 months 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 years, 10 months 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 years, 10 months 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 years, 10 months 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 ...
3 years, 10 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? ...
3 years, 9 months 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 ...
3 years, 9 months 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 ...
3 years, 9 months 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 ...
3 years, 9 months ago (2017-02-28 13:13:12 UTC) #28
fhorschig
Update: All internal dependencies have landed.
3 years, 9 months 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 ...
3 years, 9 months 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: ...
3 years, 9 months 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. ...
3 years, 9 months 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: ...
3 years, 9 months ago (2017-03-01 20:57:36 UTC) #38
fhorschig
Found the bug.
3 years, 9 months 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 ...
3 years, 9 months 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 ...
3 years, 9 months 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.)
3 years, 9 months ago (2017-03-02 14:19:26 UTC) #53
sky
LGTM
3 years, 9 months 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
3 years, 9 months 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)
3 years, 9 months 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 ...
3 years, 9 months 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 ...
3 years, 9 months 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
3 years, 9 months 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, ...
3 years, 9 months 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
3 years, 9 months ago (2017-03-02 22:19:09 UTC) #82
commit-bot: I haz the power
3 years, 9 months 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...

Powered by Google App Engine
This is Rietveld 408576698