Keep ServiceWorkerMetrics::Site in ServiceWorkerVersion

Keep ServiceWorkerMetrics::Site in ServiceWorkerVersion

To reduce the string comparison in SiteFromURL().

This CL depends on https://codereview.chromium.org/2251633002/.

https://codereview.chromium.org/2249063004/ depends on this CL.

BUG=561209
Committed: https://crrev.com/5c7b49dbd8ada94c81ee49f9b3508e49a43649d8
Cr-Commit-Position: refs/heads/master@{#413393}

[horo, 2016-08-17 06:15:26]

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

[commit-bot: I haz the power, 2016-08-17 06:15:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-08-17 06:29:17]

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

[commit-bot: I haz the power, 2016-08-17 06:29:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-08-17 06:36:59]

Description was changed from

==========
uma


use optional for has_fetch_handler

BUG=

patch from issue 2251633002 at patchset 1 (http://crrev.com/2251633002#ps1)

fix testss

add comment
==========

to

==========
Keep ServiceWorkerMetrics::Site in ServiceWorkerVersion

This CL depends on https://codereview.chromium.org/2251633002/.

BUG=561209
==========

[horo, 2016-08-17 06:37:52]

Description was changed from

==========
Keep ServiceWorkerMetrics::Site in ServiceWorkerVersion

This CL depends on https://codereview.chromium.org/2251633002/.

BUG=561209
==========

to

==========
Keep ServiceWorkerMetrics::Site in ServiceWorkerVersion

To reduce the string comparison in SiteFromURL().

This CL depends on https://codereview.chromium.org/2251633002/.

BUG=561209
==========

[horo, 2016-08-17 06:38:01]

Patchset #1 (id:1) has been deleted

[horo, 2016-08-17 06:38:48]

Patchset #2 (id:40001) has been deleted

[horo, 2016-08-17 06:39:02]

Patchset #1 (id:20001) has been deleted

[horo, 2016-08-17 06:39:17]

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

[commit-bot: I haz the power, 2016-08-17 06:39:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-08-17 06:40:54]

Patchset #2 (id:80001) has been deleted

[horo, 2016-08-17 06:41:58]

Patchset #1 (id:60001) has been deleted

[horo, 2016-08-17 06:42:11]

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

[commit-bot: I haz the power, 2016-08-17 06:42:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-08-17 06:45:14]

horo@chromium.org changed reviewers:
+ shimazu@chromium.org

[horo, 2016-08-17 06:45:15]

shimazu@
Could you please review this?

[horo, 2016-08-17 06:48:00]

Description was changed from

==========
Keep ServiceWorkerMetrics::Site in ServiceWorkerVersion

To reduce the string comparison in SiteFromURL().

This CL depends on https://codereview.chromium.org/2251633002/.

BUG=561209
==========

to

==========
Keep ServiceWorkerMetrics::Site in ServiceWorkerVersion

To reduce the string comparison in SiteFromURL().

This CL depends on https://codereview.chromium.org/2251633002/.

https://codereview.chromium.org/2249063004/ depends on this CL.

BUG=561209
==========

[shimazu, 2016-08-18 04:47:36]

https://codereview.chromium.org/2250623003/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2250623003/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:159:
ServiceWorkerMetrics::Site site_for_uma() const { return site_for_uma_; }
I think storing Site as |site_for_uma_| is adding a bit complexity of keeping
the state up to date. If there is no other concern (about performance?), I
prefer calling SiteFromURL(script_url_, has_fetch_handler_).
WDYT?

[horo, 2016-08-18 05:44:54]

https://codereview.chromium.org/2250623003/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2250623003/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:159:
ServiceWorkerMetrics::Site site_for_uma() const { return site_for_uma_; }
On 2016/08/18 04:47:36, shimazu wrote:
> I think storing Site as |site_for_uma_| is adding a bit complexity of keeping
> the state up to date. If there is no other concern (about performance?), I
> prefer calling SiteFromURL(script_url_, has_fetch_handler_).
> WDYT?

We will use this Site for UMA of all fetch events.
https://codereview.chromium.org/2218943002/
I don't think calling SiteFromURL() every time is a good idea.

For example, HistogramCustomizer in the renderer process keep
common_host_histogram_suffix_ which is set in the page navigation
and will be used when recording UMAs.

[shimazu, 2016-08-18 06:53:24]

https://codereview.chromium.org/2250623003/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2250623003/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:416: if (site_for_uma_
!= ServiceWorkerMetrics::Site::OTHER)
How about using SiteFromURL for keeping consistency?

https://codereview.chromium.org/2250623003/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2250623003/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:159:
ServiceWorkerMetrics::Site site_for_uma() const { return site_for_uma_; }
On 2016/08/18 05:44:54, horo wrote:
> On 2016/08/18 04:47:36, shimazu wrote:
> > I think storing Site as |site_for_uma_| is adding a bit complexity of
keeping
> > the state up to date. If there is no other concern (about performance?), I
> > prefer calling SiteFromURL(script_url_, has_fetch_handler_).
> > WDYT?
> 
> We will use this Site for UMA of all fetch events.
> https://codereview.chromium.org/2218943002/
> I don't think calling SiteFromURL() every time is a good idea.
> 
> For example, HistogramCustomizer in the renderer process keep
> common_host_histogram_suffix_ which is set in the page navigation
> and will be used when recording UMAs.

I see. Let's follow HistogramCustomizer:)

# The reason of the comment was that I considered SiteFromURL wouldn't take much
time (I guess it might be at most ~1us or so?).

[horo, 2016-08-21 07:57:37]

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

[commit-bot: I haz the power, 2016-08-21 07:57:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-08-21 08:39:58]

https://codereview.chromium.org/2250623003/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2250623003/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:416: if (site_for_uma_
!= ServiceWorkerMetrics::Site::OTHER)
On 2016/08/18 06:53:24, shimazu wrote:
> How about using SiteFromURL for keeping consistency?

I removed FetchHandlerExistence from the arguments of SiteFromURL() to avoid the
circular dependency of header files.

[commit-bot: I haz the power, 2016-08-21 09:26:50]

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

[commit-bot: I haz the power, 2016-08-21 09:26:51]

Dry run: This issue passed the CQ dry run.

[shimazu, 2016-08-22 02:48:03]

lgtm

https://codereview.chromium.org/2250623003/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2250623003/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:416: if (site_for_uma_
!= ServiceWorkerMetrics::Site::OTHER)
On 2016/08/21 08:39:58, horo wrote:
> On 2016/08/18 06:53:24, shimazu wrote:
> > How about using SiteFromURL for keeping consistency?
> 
> I removed FetchHandlerExistence from the arguments of SiteFromURL() to avoid
the
> circular dependency of header files.  

Sure.

[horo, 2016-08-22 02:59:08]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2016-08-22 02:59:23]

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, 2016-08-22 03:03:21]

Description was changed from

==========
Keep ServiceWorkerMetrics::Site in ServiceWorkerVersion

To reduce the string comparison in SiteFromURL().

This CL depends on https://codereview.chromium.org/2251633002/.

https://codereview.chromium.org/2249063004/ depends on this CL.

BUG=561209
==========

to

==========
Keep ServiceWorkerMetrics::Site in ServiceWorkerVersion

To reduce the string comparison in SiteFromURL().

This CL depends on https://codereview.chromium.org/2251633002/.

https://codereview.chromium.org/2249063004/ depends on this CL.

BUG=561209
==========

[commit-bot: I haz the power, 2016-08-22 03:03:22]

Committed patchset #3 (id:140001)

[commit-bot: I haz the power, 2016-08-22 03:04:42]

Description was changed from

==========
Keep ServiceWorkerMetrics::Site in ServiceWorkerVersion

To reduce the string comparison in SiteFromURL().

This CL depends on https://codereview.chromium.org/2251633002/.

https://codereview.chromium.org/2249063004/ depends on this CL.

BUG=561209
==========

to

==========
Keep ServiceWorkerMetrics::Site in ServiceWorkerVersion

To reduce the string comparison in SiteFromURL().

This CL depends on https://codereview.chromium.org/2251633002/.

https://codereview.chromium.org/2249063004/ depends on this CL.

BUG=561209

Committed: https://crrev.com/5c7b49dbd8ada94c81ee49f9b3508e49a43649d8
Cr-Commit-Position: refs/heads/master@{#413393}
==========

[commit-bot: I haz the power, 2016-08-22 03:04:43]

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