Log runtime of StartLoadingFavicon and OnFaviconDataAvailable for JumpList

Log runtime of StartLoadingFavicon and OnFaviconDataAvailable for 
JumpList

Currently there're lots of redundant of favicon loading calls in 
JumpList update. The total time cost of one favicon loading process
consists of the runtime of one StartLoadingFavicon call, one 
asynchronous GetFaviconImageForPageURL call and one 
OnFaviconDataAvailable call.

UMA currently knows the total amount of GetFaviconImageForPageURL 
function calls and their runtime distribution. However, UMA doesn't 
know its exact contribution from the JumpList update. It would be nice 
to log some information from the JumpList class so that we know how 
much we can possibly improve. 

BUG=40407, 179576, 717236
Review-Url: https://codereview.chromium.org/2860573005
Cr-Commit-Position: refs/heads/master@{#469365}
Committed: https://chromium.googlesource.com/chromium/src/+/a16438df21e2fb1b99b8b3f6b80180407c78d3cc

[chengx, 2017-05-02 23:39:45]

Description was changed from

==========
Log favicon loading related method duration

BUG=
==========

to

==========
Log runtime of StartLoadingFavicon and OnFaviconDataAvailable for JumpList

Currently there're lots of redundant of favicon loading calls in 
JumpList update. The total time cost of one faviicon loading call consists of
one StartLoadingFavicon call, one asynchronous GetFaviconImageForPageURL call
and one OnFaviconDataAvailable call.

UMA has the information of the total call amount of GetFaviconImageForPageURL
and the runtime distribution. However, we 
don't know how many of them are from the JumpList update. It would be nice to
log some information from the JumpList class so that we know how much we can
possibly improve. 

BUG=
==========

[chengx, 2017-05-02 23:40:03]

Description was changed from

==========
Log runtime of StartLoadingFavicon and OnFaviconDataAvailable for JumpList

Currently there're lots of redundant of favicon loading calls in 
JumpList update. The total time cost of one faviicon loading call consists of
one StartLoadingFavicon call, one asynchronous GetFaviconImageForPageURL call
and one OnFaviconDataAvailable call.

UMA has the information of the total call amount of GetFaviconImageForPageURL
and the runtime distribution. However, we 
don't know how many of them are from the JumpList update. It would be nice to
log some information from the JumpList class so that we know how much we can
possibly improve. 

BUG=
==========

to

==========
Log runtime of StartLoadingFavicon and OnFaviconDataAvailable for JumpList

Currently there're lots of redundant of favicon loading calls in 
JumpList update. The total time cost of one faviicon loading call 
consists of one StartLoadingFavicon call, one asynchronous 
GetFaviconImageForPageURL call and one OnFaviconDataAvailable call.

UMA has the information of the total call amount of 
GetFaviconImageForPageURL and the runtime distribution. However, we 
don't know how many of them are from the JumpList update. It would be 
nice to log some information from the JumpList class so that we know 
how much we can possibly improve. 

BUG=
==========

[chengx, 2017-05-02 23:42:20]

Description was changed from

==========
Log runtime of StartLoadingFavicon and OnFaviconDataAvailable for JumpList

Currently there're lots of redundant of favicon loading calls in 
JumpList update. The total time cost of one faviicon loading call 
consists of one StartLoadingFavicon call, one asynchronous 
GetFaviconImageForPageURL call and one OnFaviconDataAvailable call.

UMA has the information of the total call amount of 
GetFaviconImageForPageURL and the runtime distribution. However, we 
don't know how many of them are from the JumpList update. It would be 
nice to log some information from the JumpList class so that we know 
how much we can possibly improve. 

BUG=
==========

to

==========
Log runtime of StartLoadingFavicon and OnFaviconDataAvailable for JumpList

Currently there're lots of redundant of favicon loading calls in 
JumpList update. The total time cost of one favicon loading process
consists of the runtime of one StartLoadingFavicon call, one 
asynchronous GetFaviconImageForPageURL call and one 
OnFaviconDataAvailable call.

UMA has the information about the total amount of 
GetFaviconImageForPageURL 
function call and their runtime distribution. However, we don't know 
its exact contribution from the JumpList update. It would be nice to 
log some information from the JumpList class so that we know how much 
we can possibly improve. 

BUG=
==========

[chengx, 2017-05-02 23:42:28]

Description was changed from

==========
Log runtime of StartLoadingFavicon and OnFaviconDataAvailable for JumpList

Currently there're lots of redundant of favicon loading calls in 
JumpList update. The total time cost of one favicon loading process
consists of the runtime of one StartLoadingFavicon call, one 
asynchronous GetFaviconImageForPageURL call and one 
OnFaviconDataAvailable call.

UMA has the information about the total amount of 
GetFaviconImageForPageURL 
function call and their runtime distribution. However, we don't know 
its exact contribution from the JumpList update. It would be nice to 
log some information from the JumpList class so that we know how much 
we can possibly improve. 

BUG=
==========

to

==========
Log runtime of StartLoadingFavicon and OnFaviconDataAvailable for JumpList

Currently there're lots of redundant of favicon loading calls in 
JumpList update. The total time cost of one favicon loading process
consists of the runtime of one StartLoadingFavicon call, one 
asynchronous GetFaviconImageForPageURL call and one 
OnFaviconDataAvailable call.

UMA has the information about the total amount of GetFaviconImageForPageURL 
function call and their runtime distribution. However, we don't know 
its exact contribution from the JumpList update. It would be nice to 
log some information from the JumpList class so that we know how much 
we can possibly improve. 

BUG=
==========

[chengx, 2017-05-02 23:43:01]

Description was changed from

==========
Log runtime of StartLoadingFavicon and OnFaviconDataAvailable for JumpList

Currently there're lots of redundant of favicon loading calls in 
JumpList update. The total time cost of one favicon loading process
consists of the runtime of one StartLoadingFavicon call, one 
asynchronous GetFaviconImageForPageURL call and one 
OnFaviconDataAvailable call.

UMA has the information about the total amount of GetFaviconImageForPageURL 
function call and their runtime distribution. However, we don't know 
its exact contribution from the JumpList update. It would be nice to 
log some information from the JumpList class so that we know how much 
we can possibly improve. 

BUG=
==========

to

==========
Log runtime of StartLoadingFavicon and OnFaviconDataAvailable for 
JumpList

Currently there're lots of redundant of favicon loading calls in 
JumpList update. The total time cost of one favicon loading process
consists of the runtime of one StartLoadingFavicon call, one 
asynchronous GetFaviconImageForPageURL call and one 
OnFaviconDataAvailable call.

UMA currently knows the total amount of GetFaviconImageForPageURL 
function call and their runtime distribution. However, we don't know 
its exact contribution from the JumpList update. It would be nice to 
log some information from the JumpList class so that we know how much 
we can possibly improve. 

BUG=
==========

[chengx, 2017-05-02 23:45:11]

Description was changed from

==========
Log runtime of StartLoadingFavicon and OnFaviconDataAvailable for 
JumpList

Currently there're lots of redundant of favicon loading calls in 
JumpList update. The total time cost of one favicon loading process
consists of the runtime of one StartLoadingFavicon call, one 
asynchronous GetFaviconImageForPageURL call and one 
OnFaviconDataAvailable call.

UMA currently knows the total amount of GetFaviconImageForPageURL 
function call and their runtime distribution. However, we don't know 
its exact contribution from the JumpList update. It would be nice to 
log some information from the JumpList class so that we know how much 
we can possibly improve. 

BUG=
==========

to

==========
Log runtime of StartLoadingFavicon and OnFaviconDataAvailable for 
JumpList

Currently there're lots of redundant of favicon loading calls in 
JumpList update. The total time cost of one favicon loading process
consists of the runtime of one StartLoadingFavicon call, one 
asynchronous GetFaviconImageForPageURL call and one 
OnFaviconDataAvailable call.

UMA currently knows the total amount of GetFaviconImageForPageURL 
function call and their runtime distribution. However, UMA doesn't 
know its exact contribution from the JumpList update. It would be nice 
to log some information from the JumpList class so that we know how 
much we can possibly improve. 

BUG=40407, 179576, 717236
==========

[chengx, 2017-05-02 23:45:11]

chengx@chromium.org changed reviewers:
+ grt@chromium.org

[chengx, 2017-05-02 23:46:09]

Description was changed from

==========
Log runtime of StartLoadingFavicon and OnFaviconDataAvailable for 
JumpList

Currently there're lots of redundant of favicon loading calls in 
JumpList update. The total time cost of one favicon loading process
consists of the runtime of one StartLoadingFavicon call, one 
asynchronous GetFaviconImageForPageURL call and one 
OnFaviconDataAvailable call.

UMA currently knows the total amount of GetFaviconImageForPageURL 
function call and their runtime distribution. However, UMA doesn't 
know its exact contribution from the JumpList update. It would be nice 
to log some information from the JumpList class so that we know how 
much we can possibly improve. 

BUG=40407, 179576, 717236
==========

to

==========
Log runtime of StartLoadingFavicon and OnFaviconDataAvailable for 
JumpList

Currently there're lots of redundant of favicon loading calls in 
JumpList update. The total time cost of one favicon loading process
consists of the runtime of one StartLoadingFavicon call, one 
asynchronous GetFaviconImageForPageURL call and one 
OnFaviconDataAvailable call.

UMA currently knows the total amount of GetFaviconImageForPageURL 
function calls and their runtime distribution. However, UMA doesn't 
know its exact contribution from the JumpList update. It would be nice 
to log some information from the JumpList class so that we know how 
much we can possibly improve. 

BUG=40407, 179576, 717236
==========

[chengx, 2017-05-02 23:47:01]

chengx@chromium.org changed reviewers:
+ isherman@chromium.org

[chengx, 2017-05-02 23:47:02]

PTAL, thanks!

[Ilya Sherman, 2017-05-03 00:10:10]

lgtm

[chengx, 2017-05-03 00:30:41]

The CQ bit was checked by chengx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 00:31:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 01:45:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 01:45:25]

Dry run: This issue passed the CQ dry run.

[grt (UTC plus 2), 2017-05-03 09:55:42]

it looks like these are measuring the time of the cheap operations (asking for
and then using the favicon) rather than the expensive operation (the
asynchronous favicon fetch from the history service). is that your intent?

[chengx, 2017-05-03 17:38:08]

On 2017/05/03 09:55:42, grt (UTC plus 2) wrote:
> it looks like these are measuring the time of the cheap operations (asking for
> and then using the favicon) rather than the expensive operation (the
> asynchronous favicon fetch from the history service). is that your intent?

Not really. The time cost of the asynchronous favicon fetch is already recorded.
HistoryService::GetFaviconsForURL does this favicon fetch job actually. You can
see its runtime distribution via the link below:

https://uma.googleplex.com/p/chrome/profiler/?end_date=20170502&ui_chrome_ver...

However, there're a few places calling HistoryService::GetFaviconsForURL, so
it's unclear how many samples are coming from the JumpList favicon fetch. 

You're right, these two methods I'm measuring are much cheaper operations than
the favicon fetch. Measuring their runtime is one purpose, but not all. It's not
even the major purpose. What I care about the most is the amount of these
function calls. Since These two methods should have the same amount of calls as
the asynchronous favicon fetch, measuring these methods can answer the question
that how many samples of HistoryService::GetFaviconsForURL are coming from
JumpList. 

I'm measuring both of the two methods instead of one just to make the results
more convincing to me. Moreover, since measuring the time can give me the
information about both the runtime and the number of samples, I think it's not a
bad choice.

[chengx, 2017-05-03 17:44:57]

The CQ bit was checked by chengx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 17:45:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 18:56:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 18:56:16]

Dry run: This issue passed the CQ dry run.

[grt (UTC plus 2), 2017-05-04 11:19:18]

Okay. LGTM.

[chengx, 2017-05-04 17:01:39]

The CQ bit was checked by chengx@chromium.org

[chengx, 2017-05-04 17:01:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2860573005/#ps20001
(title: "Add TODO to remove the UMA metric.")

[commit-bot: I haz the power, 2017-05-04 17:02:27]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 17:08:26]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493917299791620,
"parent_rev": "357dc8ff5f8b6e82e881b656a9bf3e99bd229540", "commit_rev":
"a16438df21e2fb1b99b8b3f6b80180407c78d3cc"}

[commit-bot: I haz the power, 2017-05-04 17:09:05]

Description was changed from

==========
Log runtime of StartLoadingFavicon and OnFaviconDataAvailable for 
JumpList

Currently there're lots of redundant of favicon loading calls in 
JumpList update. The total time cost of one favicon loading process
consists of the runtime of one StartLoadingFavicon call, one 
asynchronous GetFaviconImageForPageURL call and one 
OnFaviconDataAvailable call.

UMA currently knows the total amount of GetFaviconImageForPageURL 
function calls and their runtime distribution. However, UMA doesn't 
know its exact contribution from the JumpList update. It would be nice 
to log some information from the JumpList class so that we know how 
much we can possibly improve. 

BUG=40407, 179576, 717236
==========

to

==========
Log runtime of StartLoadingFavicon and OnFaviconDataAvailable for 
JumpList

Currently there're lots of redundant of favicon loading calls in 
JumpList update. The total time cost of one favicon loading process
consists of the runtime of one StartLoadingFavicon call, one 
asynchronous GetFaviconImageForPageURL call and one 
OnFaviconDataAvailable call.

UMA currently knows the total amount of GetFaviconImageForPageURL 
function calls and their runtime distribution. However, UMA doesn't 
know its exact contribution from the JumpList update. It would be nice 
to log some information from the JumpList class so that we know how 
much we can possibly improve. 

BUG=40407, 179576, 717236

Review-Url: https://codereview.chromium.org/2860573005
Cr-Commit-Position: refs/heads/master@{#469365}
Committed:
https://chromium.googlesource.com/chromium/src/+/a16438df21e2fb1b99b8b3f6b801...
==========

[commit-bot: I haz the power, 2017-05-04 17:09:06]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/a16438df21e2fb1b99b8b3f6b801...