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

Issue 2796343003: Remove preferences among favicon types when choosing large icons

Created:
3 years, 8 months ago by mastiz
Modified:
3 years, 8 months ago
Reviewers:
CC:
chromium-reviews, tfarina, Matt Giuca
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove preferences among favicon types when choosing large icons Make all UIs to use the largest available icon regardless of its type. Prior to this patch, many UI chose favicons over touch icons, as long as some minimum size was achieved (UI-dependent, often 16x16 on Android due to default_favicon_min_size). Sample affected site: m.youtube.com on Android's bookmark or history UI: - Before: UI chose 16x16 favicon: http://s.ytimg.com/yts/favicon-vflz7uhzw.ico - After: UI chose 144x144 touch icon: http://s.ytimg.com/yts/mobile/img/apple-touch-icon-144x144-precomposed-vflwq-hLZ.png This change will (in a later CL) allow reducing the number of downloaded favicons on mobile, because currently two large icons are downloaded, the largest one per type (one for non-touch icons and the other one for touch icons), since the FaviconService's API allow clients to define arbitrary type preferences. BUG=698671

Patch Set 1 #

Patch Set 2 : Fix iOS. #

Patch Set 3 : Leftovers. #

Patch Set 4 : Rebased. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+42 lines, -202 lines) Patch
M chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc View 1 2 1 chunk +1 line, -8 lines 0 comments Download
M chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc View 1 2 1 chunk +1 line, -6 lines 0 comments Download
M components/favicon/core/favicon_service.h View 1 chunk +1 line, -3 lines 0 comments Download
M components/favicon/core/favicon_service_impl.h View 1 chunk +0 lines, -2 lines 0 comments Download
M components/favicon/core/favicon_service_impl.cc View 2 chunks +1 line, -4 lines 0 comments Download
M components/favicon/core/large_icon_service.h View 1 2 3 1 chunk +0 lines, -5 lines 0 comments Download
M components/favicon/core/large_icon_service.cc View 1 2 3 2 chunks +3 lines, -7 lines 0 comments Download
M components/favicon/core/large_icon_service_unittest.cc View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M components/favicon/core/test/mock_favicon_service.h View 1 chunk +1 line, -3 lines 0 comments Download
M components/history/core/browser/history_backend.h View 1 2 3 1 chunk +0 lines, -2 lines 0 comments Download
M components/history/core/browser/history_backend.cc View 1 2 3 2 chunks +9 lines, -41 lines 0 comments Download
M components/history/core/browser/history_backend_unittest.cc View 1 chunk +9 lines, -88 lines 0 comments Download
M components/history/core/browser/history_service.h View 1 2 3 1 chunk +1 line, -13 lines 0 comments Download
M components/history/core/browser/history_service.cc View 1 2 3 2 chunks +1 line, -3 lines 0 comments Download
M ios/chrome/app/spotlight/spotlight_manager_unittest.mm View 1 1 chunk +2 lines, -3 lines 0 comments Download
M ios/chrome/browser/ui/history/favicon_view_provider_unittest.mm View 1 2 chunks +6 lines, -6 lines 0 comments Download
M ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller_unittest.mm View 1 1 chunk +2 lines, -3 lines 0 comments Download
M ios/chrome/browser/ui/reading_list/reading_list_coordinator_unittest.mm View 1 1 chunk +2 lines, -3 lines 0 comments Download

Messages

Total messages: 12 (12 generated)
mastiz
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
3 years, 8 months ago (2017-04-05 13:13:59 UTC) #1
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2796343003/1
3 years, 8 months ago (2017-04-05 13:14:17 UTC) #2
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
3 years, 8 months ago (2017-04-05 13:22:41 UTC) #3
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/184349) ios-simulator-xcode-clang on ...
3 years, 8 months ago (2017-04-05 13:22:42 UTC) #4
mastiz
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
3 years, 8 months ago (2017-04-05 13:29:20 UTC) #5
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2796343003/20001
3 years, 8 months ago (2017-04-05 13:29:39 UTC) #6
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
3 years, 8 months ago (2017-04-05 13:42:52 UTC) #7
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_chromium_compile_only_ng/builds/312757) linux_chromium_chromeos_ozone_rel_ng on ...
3 years, 8 months ago (2017-04-05 13:42:53 UTC) #8
mastiz
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
3 years, 8 months ago (2017-04-05 14:03:44 UTC) #9
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2796343003/40001
3 years, 8 months ago (2017-04-05 14:04:07 UTC) #10
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
3 years, 8 months ago (2017-04-05 14:19:23 UTC) #11
commit-bot: I haz the power
3 years, 8 months ago (2017-04-05 14:19:24 UTC) #12
Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

Powered by Google App Engine
This is Rietveld 408576698