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

Issue 38523002: Using largest favicon for shortcut when possible. (Closed)

Created:
7 years, 2 months ago by michaelbai
Modified:
7 years, 1 month ago
Reviewers:
Yaron, gone, yfriedman
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Using largest favicon for shortcut when possible. The icon will be fetched as 1. The largest favicon which is not smaller than platform's launch icon. 2. The largest icon among all icons. BUG=298446 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231194

Patch Set 1 #

Patch Set 2 : #

Total comments: 1

Patch Set 3 : Using platform icon size #

Patch Set 4 : #

Total comments: 2

Patch Set 5 : #

Patch Set 6 : pass in launcher_large_icon_size #

Total comments: 4

Patch Set 7 : address comment #

Patch Set 8 : Sync #

Unified diffs Side-by-side diffs Delta from patch set Stats (+35 lines, -21 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java View 1 2 3 4 5 6 3 chunks +8 lines, -3 lines 0 comments Download
M chrome/android/javatests/src/org/chromium/chrome/browser/ShortcutHelperTest.java View 1 2 3 4 5 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/android/shortcut_helper.h View 1 2 3 4 5 3 chunks +5 lines, -2 lines 0 comments Download
M chrome/browser/android/shortcut_helper.cc View 1 2 3 4 5 4 chunks +20 lines, -15 lines 0 comments Download

Messages

Total messages: 18 (0 generated)
michaelbai
PTAL https://codereview.chromium.org/38523002/diff/30001/chrome/browser/android/shortcut_helper.cc File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/38523002/diff/30001/chrome/browser/android/shortcut_helper.cc#newcode89 chrome/browser/android/shortcut_helper.cc:89: // from 57x57 to 144x144. I have a ...
7 years, 2 months ago (2013-10-23 23:20:47 UTC) #1
gone
Is there an example website where we could see the effects of applying the patch?
7 years, 2 months ago (2013-10-23 23:46:01 UTC) #2
michaelbai
On 2013/10/23 23:46:01, dfalcantara wrote: > Is there an example website where we could see ...
7 years, 2 months ago (2013-10-23 23:57:08 UTC) #3
gone
lgtm
7 years, 2 months ago (2013-10-24 18:59:05 UTC) #4
michaelbai
PTAL, change to use platform's icon size as threshold.
7 years, 2 months ago (2013-10-24 21:40:31 UTC) #5
gone
https://codereview.chromium.org/38523002/diff/90001/chrome/browser/android/shortcut_helper.cc File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/38523002/diff/90001/chrome/browser/android/shortcut_helper.cc#newcode87 chrome/browser/android/shortcut_helper.cc:87: minimum_size - 1, Why are you subtracting off 1 ...
7 years, 2 months ago (2013-10-24 22:07:31 UTC) #6
michaelbai
PTAL https://codereview.chromium.org/38523002/diff/90001/chrome/browser/android/shortcut_helper.cc File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/38523002/diff/90001/chrome/browser/android/shortcut_helper.cc#newcode87 chrome/browser/android/shortcut_helper.cc:87: minimum_size - 1, The GetLargestRawFaviconForURL finds the icon ...
7 years, 2 months ago (2013-10-25 03:36:43 UTC) #7
michaelbai
PTAL, passed in launcher_large_icon_size, instead of adding a JNI.
7 years, 1 month ago (2013-10-25 16:46:01 UTC) #8
gone
Really strange that the minimum size is not actually a minimum, but I guess that's ...
7 years, 1 month ago (2013-10-25 17:13:33 UTC) #9
Yaron
https://codereview.chromium.org/38523002/diff/190001/chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/38523002/diff/190001/chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java#newcode42 chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:42: * @param tab Tab to create a shortcut for. ...
7 years, 1 month ago (2013-10-25 18:50:16 UTC) #10
michaelbai
PTAL https://codereview.chromium.org/38523002/diff/190001/chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/38523002/diff/190001/chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java#newcode42 chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:42: * @param tab Tab to create a shortcut ...
7 years, 1 month ago (2013-10-25 19:00:59 UTC) #11
Yaron
lgtm
7 years, 1 month ago (2013-10-25 19:04:29 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/38523002/270001
7 years, 1 month ago (2013-10-25 19:09:24 UTC) #13
commit-bot: I haz the power
Retried try job too often on win_rel for step(s) browser_tests, chrome_frame_net_tests, chrome_frame_tests, chrome_frame_unittests, content_browsertests, mini_installer_test, ...
7 years, 1 month ago (2013-10-25 22:11:32 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/38523002/550001
7 years, 1 month ago (2013-10-26 01:33:43 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/38523002/550001
7 years, 1 month ago (2013-10-26 04:14:38 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/michaelbai@chromium.org/38523002/550001
7 years, 1 month ago (2013-10-26 09:47:29 UTC) #17
commit-bot: I haz the power
7 years, 1 month ago (2013-10-26 10:51:24 UTC) #18
Message was sent while issue was closed.
Change committed as 231194

Powered by Google App Engine
This is Rietveld 408576698