Using largest favicon for shortcut when possible.

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

[michaelbai, 2013-10-23 23:20:47]

PTAL

https://codereview.chromium.org/38523002/diff/30001/chrome/browser/android/sh...
File chrome/browser/android/shortcut_helper.cc (right):

https://codereview.chromium.org/38523002/diff/30001/chrome/browser/android/sh...
chrome/browser/android/shortcut_helper.cc:89: // from 57x57 to 144x144.
I have a CL to fix this issue, the icon will not be resized before stored to
database. Once that CL landed, I will update this comments. Since favicon in
current database is already resized, I have to put this logic here.

[gone, 2013-10-23 23:46:01]

Is there an example website where we could see the effects of applying the
patch?

[michaelbai, 2013-10-23 23:57:08]

On 2013/10/23 23:46:01, dfalcantara wrote:
> Is there an example website where we could see the effects of applying the
> patch?

I need to land the CL https://codereview.chromium.org/28943004/ to fetch large
favicon first.
The API FaviconService::GetLargestRawFaviconForURL() should work fine as there
are unit tests for it.

[gone, 2013-10-24 18:59:05]

lgtm

[michaelbai, 2013-10-24 21:40:31]

PTAL, change to use platform's icon size as threshold.

[gone, 2013-10-24 22:07:31]

https://codereview.chromium.org/38523002/diff/90001/chrome/browser/android/sh...
File chrome/browser/android/shortcut_helper.cc (right):

https://codereview.chromium.org/38523002/diff/90001/chrome/browser/android/sh...
chrome/browser/android/shortcut_helper.cc:87: minimum_size - 1,
Why are you subtracting off 1 pixel btw?

[michaelbai, 2013-10-25 03:36:43]

PTAL

https://codereview.chromium.org/38523002/diff/90001/chrome/browser/android/sh...
File chrome/browser/android/shortcut_helper.cc (right):

https://codereview.chromium.org/38523002/diff/90001/chrome/browser/android/sh...
chrome/browser/android/shortcut_helper.cc:87: minimum_size - 1,
The GetLargestRawFaviconForURL finds the icon larger than minimum_size, not
equal to or larger than minimum_size.


On 2013/10/24 22:07:32, dfalcantara wrote:
> Why are you subtracting off 1 pixel btw?

[michaelbai, 2013-10-25 16:46:01]

PTAL, passed in launcher_large_icon_size, instead of adding a JNI.

[gone, 2013-10-25 17:13:33]

Really strange that the minimum size is not actually a minimum, but I guess
that's just how the API works.  lgtm

[Yaron, 2013-10-25 18:50:16]

https://codereview.chromium.org/38523002/diff/190001/chrome/android/java/src/...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/38523002/diff/190001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:42: *
@param tab Tab to create a shortcut for.
Nit: update

https://codereview.chromium.org/38523002/diff/190001/chrome/browser/android/s...
File chrome/browser/android/shortcut_helper.cc (right):

https://codereview.chromium.org/38523002/diff/190001/chrome/browser/android/s...
chrome/browser/android/shortcut_helper.cc:86:
favicon_service->GetLargestRawFaviconForURL(profile, url_, icon_types,
Can you add a test that triggers this?

[michaelbai, 2013-10-25 19:00:59]

PTAL

https://codereview.chromium.org/38523002/diff/190001/chrome/android/java/src/...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/38523002/diff/190001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:42: *
@param tab Tab to create a shortcut for.
On 2013/10/25 18:50:17, Yaron wrote:
> Nit: update

Done.

https://codereview.chromium.org/38523002/diff/190001/chrome/browser/android/s...
File chrome/browser/android/shortcut_helper.cc (right):

https://codereview.chromium.org/38523002/diff/190001/chrome/browser/android/s...
chrome/browser/android/shortcut_helper.cc:86:
favicon_service->GetLargestRawFaviconForURL(profile, url_, icon_types,
There already unit test for HistoryBackend::GetLargestFaviconForURL(), From
FaviconService to HistoryBackend is just some callback. I think we don't need to
test that case.

See

https://codereview.chromium.org/26563004/


On 2013/10/25 18:50:17, Yaron wrote:
> Can you add a test that triggers this?

[Yaron, 2013-10-25 19:04:29]

lgtm

[commit-bot: I haz the power, 2013-10-25 19:09:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/38523002/270001

[commit-bot: I haz the power, 2013-10-25 22:11:32]

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, nacl_integration,
sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-10-26 01:33:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/38523002/550001

[commit-bot: I haz the power, 2013-10-26 04:14:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/38523002/550001

[commit-bot: I haz the power, 2013-10-26 09:47:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/38523002/550001

[commit-bot: I haz the power, 2013-10-26 10:51:24]

Change committed as 231194