[DevTools] Fix favicon images on chrome://inspect

[DevTools] Fix favicon images on chrome://inspect

Favicon is resolved as data URL to avoid discrepancies if favicon depends on environment (e.g. user-agent).

BUG=461581
Committed: https://crrev.com/c49162e3fa9557ce8a42fd1c5562e0d4ae825dc6
Cr-Commit-Position: refs/heads/master@{#329831}

[vkuzkokov, 2015-05-06 16:03:53]

vkuzkokov@chromium.org changed reviewers:
+ dgozman@chromium.org, michaelbai@chromium.org

[dgozman, 2015-05-08 11:37:03]

devtools side looks good. Over to michaelbai@ for review.

https://codereview.chromium.org/1124023004/diff/1/chrome/browser/android/dev_...
File chrome/browser/android/dev_tools_discovery_provider_android.cc (right):

https://codereview.chromium.org/1124023004/diff/1/chrome/browser/android/dev_...
chrome/browser/android/dev_tools_discovery_provider_android.cc:144: GURL
GetFaviconURL() {
There is a method with the same name in this class. Please rename.

https://codereview.chromium.org/1124023004/diff/1/chrome/browser/android/tab_...
File chrome/browser/android/tab_android.h (right):

https://codereview.chromium.org/1124023004/diff/1/chrome/browser/android/tab_...
chrome/browser/android/tab_android.h:120: GURL GetFaviconURL();
Perhaps, we should mark this method as being slow here (in a comment or function
name).

[michaelbai, 2015-05-08 15:25:42]

https://codereview.chromium.org/1124023004/diff/1/chrome/browser/android/tab_...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/1124023004/diff/1/chrome/browser/android/tab_...
chrome/browser/android/tab_android.cc:716: return GURL("data:image/png;base64,"
+ base_64_data);
Should the png and URL encoding be in dev_tools_discovery_provider_android.cc?
It is unrelated to TabAndroid, It seem to me the GetFaviconURL() can be only
used by dev_tools now.

[vkuzkokov, 2015-05-12 12:16:46]

dgozman, PNG and base64 encoding is moved to DevToolsDiscoveryProviderAndroid.
PTAL again.

https://codereview.chromium.org/1124023004/diff/1/chrome/browser/android/dev_...
File chrome/browser/android/dev_tools_discovery_provider_android.cc (right):

https://codereview.chromium.org/1124023004/diff/1/chrome/browser/android/dev_...
chrome/browser/android/dev_tools_discovery_provider_android.cc:144: GURL
GetFaviconURL() {
On 2015/05/08 11:37:03, dgozman_ooo wrote:
> There is a method with the same name in this class. Please rename.

Was there? It's getFaviconBitmap now.

https://codereview.chromium.org/1124023004/diff/1/chrome/browser/android/tab_...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/1124023004/diff/1/chrome/browser/android/tab_...
chrome/browser/android/tab_android.cc:716: return GURL("data:image/png;base64,"
+ base_64_data);
On 2015/05/08 15:25:42, michaelbai wrote:
> Should the png and URL encoding be in dev_tools_discovery_provider_android.cc?
> It is unrelated to TabAndroid, It seem to me the GetFaviconURL() can be only
> used by dev_tools now.

Moved.

https://codereview.chromium.org/1124023004/diff/1/chrome/browser/android/tab_...
File chrome/browser/android/tab_android.h (right):

https://codereview.chromium.org/1124023004/diff/1/chrome/browser/android/tab_...
chrome/browser/android/tab_android.h:120: GURL GetFaviconURL();
On 2015/05/08 11:37:03, dgozman_ooo wrote:
> Perhaps, we should mark this method as being slow here (in a comment or
function
> name).

Slow part moved out of TabAndroid.

[dgozman, 2015-05-13 01:12:22]

https://codereview.chromium.org/1124023004/diff/20001/chrome/browser/android/...
File chrome/browser/android/dev_tools_discovery_provider_android.cc (right):

https://codereview.chromium.org/1124023004/diff/20001/chrome/browser/android/...
chrome/browser/android/dev_tools_discovery_provider_android.cc:64: GURL
GetFaviconURL() const override {
Here is the similar method. Please rename private one.

https://codereview.chromium.org/1124023004/diff/20001/chrome/browser/android/...
chrome/browser/android/dev_tools_discovery_provider_android.cc:151: if
(TabAndroid* tab = model->GetTabAt(index)) {
Let's have early return instead.

https://codereview.chromium.org/1124023004/diff/20001/chrome/browser/android/...
chrome/browser/android/dev_tools_discovery_provider_android.cc:158: bool encoded
= gfx::PNGCodec::Encode(
I don't like sync encoding on UI. Let's make GetFaviconURL() async.

https://codereview.chromium.org/1124023004/diff/20001/chrome/browser/android/...
chrome/browser/android/dev_tools_discovery_provider_android.cc:163: false,
std::vector<gfx::PNGCodec::Comment>(), &data);
nit: one parameter per line?

[vkuzkokov, 2015-05-13 16:40:56]

https://codereview.chromium.org/1124023004/diff/20001/chrome/browser/android/...
File chrome/browser/android/dev_tools_discovery_provider_android.cc (right):

https://codereview.chromium.org/1124023004/diff/20001/chrome/browser/android/...
chrome/browser/android/dev_tools_discovery_provider_android.cc:64: GURL
GetFaviconURL() const override {
On 2015/05/13 01:12:21, dgozman_ooo wrote:
> Here is the similar method. Please rename private one.

Done.

https://codereview.chromium.org/1124023004/diff/20001/chrome/browser/android/...
chrome/browser/android/dev_tools_discovery_provider_android.cc:151: if
(TabAndroid* tab = model->GetTabAt(index)) {
On 2015/05/13 01:12:22, dgozman_ooo wrote:
> Let's have early return instead.

Done.

https://codereview.chromium.org/1124023004/diff/20001/chrome/browser/android/...
chrome/browser/android/dev_tools_discovery_provider_android.cc:158: bool encoded
= gfx::PNGCodec::Encode(
On 2015/05/13 01:12:21, dgozman_ooo wrote:
> I don't like sync encoding on UI. Let's make GetFaviconURL() async.

With FastEncodeBGRASkBitmap it's 1ms per tab compared to 0.1ms per tab for the
rest of processing. Is that worth making DevToolsDiscoveryManager API
(GetDescriptors, CreateNew, Provider::GetDescriptors) async again?

https://codereview.chromium.org/1124023004/diff/20001/chrome/browser/android/...
chrome/browser/android/dev_tools_discovery_provider_android.cc:163: false,
std::vector<gfx::PNGCodec::Comment>(), &data);
On 2015/05/13 01:12:21, dgozman_ooo wrote:
> nit: one parameter per line?

Done.

[michaelbai, 2015-05-13 18:11:51]

michaelbai@chromium.org changed reviewers:
+ dtrainor@chromium.org

[michaelbai, 2015-05-13 18:11:52]

android change LGTM, but I am not owner, added dtrainor@

[David Trainor- moved to gerrit, 2015-05-13 18:31:53]

lgtm

[dgozman, 2015-05-13 21:58:05]

lgtm

https://codereview.chromium.org/1124023004/diff/20001/chrome/browser/android/...
File chrome/browser/android/dev_tools_discovery_provider_android.cc (right):

https://codereview.chromium.org/1124023004/diff/20001/chrome/browser/android/...
chrome/browser/android/dev_tools_discovery_provider_android.cc:158: bool encoded
= gfx::PNGCodec::Encode(
On 2015/05/13 16:40:56, vkuzkokov wrote:
> On 2015/05/13 01:12:21, dgozman_ooo wrote:
> > I don't like sync encoding on UI. Let's make GetFaviconURL() async.
> 
> With FastEncodeBGRASkBitmap it's 1ms per tab compared to 0.1ms per tab for the
> rest of processing. Is that worth making DevToolsDiscoveryManager API
> (GetDescriptors, CreateNew, Provider::GetDescriptors) async again?

You are right, it's just a small favicon.

[vkuzkokov, 2015-05-14 11:31:26]

The CQ bit was checked by vkuzkokov@chromium.org

[commit-bot: I haz the power, 2015-05-14 11:32:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1124023004/40001

[commit-bot: I haz the power, 2015-05-14 11:57:52]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-14 11:58:56]

Patchset 3 (id:??) landed as
https://crrev.com/c49162e3fa9557ce8a42fd1c5562e0d4ae825dc6
Cr-Commit-Position: refs/heads/master@{#329831}

[dgozman, 2015-06-03 12:05:43]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1161903005/ by dgozman@chromium.org.

The reason for reverting is: Performance regression. See
http://crbug.com/489208.
BUG=489208.