Add feature use counting for window.external.AddSearchProvider (Blink side).

Add feature use counting for window.external.AddSearchProvider (Blink side).

See also https://codereview.chromium.org/1410823003/

BUG=542487, 521128
Committed: https://crrev.com/75f8614691dcbe73f10d690c11af9d33c7e92530
Cr-Commit-Position: refs/heads/master@{#355849}

[Evan Stade, 2015-10-17 00:57:11]

estade@chromium.org changed reviewers:
+ dcheng@chromium.org, jochen@chromium.org, thestig@chromium.org

[Evan Stade, 2015-10-17 00:58:07]

+jochen can review everything, but he said he would be ooo, so
+dcheng for review
+thestig for chrome/renderer/ stamp

[Evan Stade, 2015-10-17 00:58:46]

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

[commit-bot: I haz the power, 2015-10-17 00:58:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1407163002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1407163002/20001

[dcheng, 2015-10-17 01:11:46]

Any particular reason not to just combine the two patches?

[dcheng, 2015-10-17 01:15:21]

https://codereview.chromium.org/1407163002/diff/20001/chrome/renderer/externa...
File chrome/renderer/external_extension.cc (right):

https://codereview.chromium.org/1407163002/diff/20001/chrome/renderer/externa...
chrome/renderer/external_extension.cc:148: webframe->countFeatureUse(
Why not just add a UMA in Chrome? It doesn't seem necessary to plumb this into
Blink?

[commit-bot: I haz the power, 2015-10-17 02:19:08]

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

[commit-bot: I haz the power, 2015-10-17 02:19:09]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2015-10-18 03:26:10]

> Any particular reason not to just combine the two patches?

They are atomic and have no overlap in reviewers.

https://codereview.chromium.org/1407163002/diff/20001/chrome/renderer/externa...
File chrome/renderer/external_extension.cc (right):

https://codereview.chromium.org/1407163002/diff/20001/chrome/renderer/externa...
chrome/renderer/external_extension.cc:148: webframe->countFeatureUse(
On 2015/10/17 01:15:21, dcheng wrote:
> Why not just add a UMA in Chrome? It doesn't seem necessary to plumb this into
> Blink?

Jochen and other API owners wanted to follow the standard process for measuring
blink feature usage to determine whether we should deprecate. See
https://codereview.chromium.org/1403803003/

[dcheng, 2015-10-19 21:49:36]

Instead of exposing UseCounter (and providing a tempting surface for people to
try to add more functionality to), let's just expose very narrow helpers off
WebDocument or WebLocalFrame:
 - willCallAddSearchProvider()
 - willCallIsSearchProviderInstalled()

[Evan Stade, 2015-10-19 23:12:00]

On 2015/10/19 21:49:36, dcheng wrote:
> Instead of exposing UseCounter (and providing a tempting surface for people to
> try to add more functionality to), let's just expose very narrow helpers off
> WebDocument or WebLocalFrame:
>  - willCallAddSearchProvider()
>  - willCallIsSearchProviderInstalled()

Sure, done, except s/will/did. I vacillated on this point while wishing Jochen
had been more explicit about his expectations for the new API. In retrospect I
don't know why I brought WebFrame into it.

[dcheng, 2015-10-19 23:18:09]

lgtm

[Evan Stade, 2015-10-19 23:22:35]

forgot to ask, is there an easy way to verify this is working, similar to
chrome://histograms?

[Evan Stade, 2015-10-19 23:22:40]

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

[commit-bot: I haz the power, 2015-10-19 23:22:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1407163002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1407163002/40001

[dcheng, 2015-10-19 23:23:49]

On 2015/10/19 at 23:22:35, estade wrote:
> forgot to ask, is there an easy way to verify this is working, similar to
chrome://histograms?

WebCore.FeatureObserver in chrome://histograms should be the relevant one.

[Lei Zhang, 2015-10-19 23:24:08]

chrome/ lgtm

[commit-bot: I haz the power, 2015-10-19 23:26:11]

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

[commit-bot: I haz the power, 2015-10-19 23:26:12]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Evan Stade, 2015-10-20 00:03:19]

On 2015/10/19 23:23:49, dcheng wrote:
> On 2015/10/19 at 23:22:35, estade wrote:
> > forgot to ask, is there an easy way to verify this is working, similar to
> chrome://histograms?
> 
> WebCore.FeatureObserver in chrome://histograms should be the relevant one.

It's not showing up there and it's not obvious to me why (even after updating
histograms.xml). It doesn't seem like the ~UseCounter histogram log is working.

[Evan Stade, 2015-10-20 00:06:08]

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

[Evan Stade, 2015-10-20 00:06:29]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2015-10-20 00:07:04]

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

[commit-bot: I haz the power, 2015-10-20 00:07:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1407163002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1407163002/80001

[Evan Stade, 2015-10-20 00:50:30]

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

[Evan Stade, 2015-10-20 00:50:31]

+Ilya, do you have any insight into why this wouldn't show up in
chrome://histograms? Something about fast renderer shutdown perhaps?

[commit-bot: I haz the power, 2015-10-20 01:20:11]

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

[commit-bot: I haz the power, 2015-10-20 01:20:12]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2015-10-20 02:26:52]

histograms.xml lgtm

On 2015/10/20 00:50:31, Evan Stade wrote:
> +Ilya, do you have any insight into why this wouldn't show up in
> chrome://histograms? Something about fast renderer shutdown perhaps?

Yep.  See https://code.google.com/p/chromium/issues/detail?id=513059

[jochen (gone - plz use gerrit), 2015-10-20 08:08:06]

I'd prefer a WebUseCounter that exposes enums for the features you want to
observe:

- it will make it easier to add more counters in the future
- the methods don't really make sense on WebLocalFrame
- it's hard to tell from looking at the methods what they'll actually do

[Evan Stade, 2015-10-20 15:15:58]

On 2015/10/20 08:08:06, jochen wrote:
> I'd prefer a WebUseCounter that exposes enums for the features you want to
> observe:
> 
> - it will make it easier to add more counters in the future
> - the methods don't really make sense on WebLocalFrame
> - it's hard to tell from looking at the methods what they'll actually do

I'm on dcheng's side here. I think this is over-engineering unless we know
someone else is going to want to use it, especially given that it's likely we'll
deprecate and remove this feature and therefore no longer need the new API. The
use counter is a property of the frame (in that UseCounter::count() takes a
frame argument and each feature is tallied at most once per frame nav) so why
doesn't it make sense to be part of WebLocalFrame?

[jochen (gone - plz use gerrit), 2015-10-20 15:33:01]

On 2015/10/20 at 15:15:58, estade wrote:
> On 2015/10/20 08:08:06, jochen wrote:
> > I'd prefer a WebUseCounter that exposes enums for the features you want to
> > observe:
> > 
> > - it will make it easier to add more counters in the future
> > - the methods don't really make sense on WebLocalFrame
> > - it's hard to tell from looking at the methods what they'll actually do
> 
> I'm on dcheng's side here. I think this is over-engineering unless we know
someone else is going to want to use it, especially given that it's likely we'll
deprecate and remove this feature and therefore no longer need the new API. The
use counter is a property of the frame (in that UseCounter::count() takes a
frame argument and each feature is tallied at most once per frame nav) so why
doesn't it make sense to be part of WebLocalFrame?

having something like countUsage() or so on frame makes more sense than having
custom callbacks to count specific features

[Evan Stade, 2015-10-20 16:00:28]

On 2015/10/20 15:33:01, jochen wrote:
> On 2015/10/20 at 15:15:58, estade wrote:
> > On 2015/10/20 08:08:06, jochen wrote:
> > > I'd prefer a WebUseCounter that exposes enums for the features you want to
> > > observe:
> > > 
> > > - it will make it easier to add more counters in the future
> > > - the methods don't really make sense on WebLocalFrame
> > > - it's hard to tell from looking at the methods what they'll actually do
> > 
> > I'm on dcheng's side here. I think this is over-engineering unless we know
> someone else is going to want to use it, especially given that it's likely
we'll
> deprecate and remove this feature and therefore no longer need the new API.
The
> use counter is a property of the frame (in that UseCounter::count() takes a
> frame argument and each feature is tallied at most once per frame nav) so why
> doesn't it make sense to be part of WebLocalFrame?
> 
> having something like countUsage() or so on frame makes more sense than having
> custom callbacks to count specific features

I disagree it makes more sense if we only ever want it for these two features,
but that is what I had originally (see ps2, ignore changes to WebFrame) before
dcheng recommended this approach. I side with Daniel, but if the two of you can
agree on an approach I'll go with whatever you decide on.

[dcheng, 2015-10-20 16:27:51]

On 2015/10/20 at 16:00:28, estade wrote:
> On 2015/10/20 15:33:01, jochen wrote:
> > On 2015/10/20 at 15:15:58, estade wrote:
> > > On 2015/10/20 08:08:06, jochen wrote:
> > > > I'd prefer a WebUseCounter that exposes enums for the features you want
to
> > > > observe:
> > > > 
> > > > - it will make it easier to add more counters in the future
> > > > - the methods don't really make sense on WebLocalFrame
> > > > - it's hard to tell from looking at the methods what they'll actually do
> > > 
> > > I'm on dcheng's side here. I think this is over-engineering unless we know
> > someone else is going to want to use it, especially given that it's likely
we'll
> > deprecate and remove this feature and therefore no longer need the new API.
The
> > use counter is a property of the frame (in that UseCounter::count() takes a
> > frame argument and each feature is tallied at most once per frame nav) so
why
> > doesn't it make sense to be part of WebLocalFrame?
> > 
> > having something like countUsage() or so on frame makes more sense than
having
> > custom callbacks to count specific features
> 
> I disagree it makes more sense if we only ever want it for these two features,
but that is what I had originally (see ps2, ignore changes to WebFrame) before
dcheng recommended this approach. I side with Daniel, but if the two of you can
agree on an approach I'll go with whatever you decide on.

To give credit where credit is due, it's actually esprehn's idea.

I'd prefer to limit the scope/extensibility of this API to just what we need.

1. Use counters are used to count Blink features. Search provider is a Blink
feature and probably shouldn't have been implemented as a v8 extension, but
clearly that ship has sailed. Thus, having to expose use counters in the public
API seems like an exceptional circumstance and not something we should
encourage.

2. From a layering perspective, it's strange for the embedder to ask Blink to
count feature usage, only for Blink to export this information back out to the
embedder.

[jochen (gone - plz use gerrit), 2015-10-20 18:04:50]

I chatted with Daniel, and we came to the agreement to go with this version, but
if we don't end up removing the api once the use counter hits stable, he signs
up for moving the implantation to Blink/idl where it should go as part of the
html implementation

lgtm

[Evan Stade, 2015-10-20 18:32:47]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-10-20 18:32:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1407163002/#ps80001
(title: "remove dbg line")

[commit-bot: I haz the power, 2015-10-20 18:34:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1407163002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1407163002/80001

[commit-bot: I haz the power, 2015-10-20 18:53:49]

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

[commit-bot: I haz the power, 2015-10-20 18:53:50]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Evan Stade, 2015-10-23 01:19:44]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-10-23 01:19:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, isherman@chromium.org, jochen@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1407163002/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2015-10-23 01:20:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1407163002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1407163002/100001

[commit-bot: I haz the power, 2015-10-23 01:23:13]

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

[commit-bot: I haz the power, 2015-10-23 01:23:14]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Evan Stade, 2015-10-23 17:40:51]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-10-23 17:40:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, isherman@chromium.org, jochen@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1407163002/#ps120001
(title: "way too much contention on this enum")

[commit-bot: I haz the power, 2015-10-23 17:41:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1407163002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1407163002/120001

[commit-bot: I haz the power, 2015-10-23 18:55:51]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-23 18:57:54]

Patchset 7 (id:??) landed as
https://crrev.com/75f8614691dcbe73f10d690c11af9d33c7e92530
Cr-Commit-Position: refs/heads/master@{#355849}