Usage counter for add/removeEventListener third argument.

Usage counter for add/removeEventListener third argument.

Check to see if the third argument on the add/removeEventListener is
a boolean; if it isn't log a UMA metric.

BUG=543685
Committed: https://crrev.com/bd5f09eed76868c8aa00a9ab4fa66a9c72ccc6dc
Cr-Commit-Position: refs/heads/master@{#354908}

[dtapuska, 2015-10-16 20:34:25]

dtapuska@chromium.org changed reviewers:
+ esprehn@chromium.org, isherman@chromium.org

[Ilya Sherman, 2015-10-16 21:47:29]

histograms.xml lgtm

[esprehn, 2015-10-16 21:56:55]

I think you actually want to check how often the third parameter is an object,
not how often it's not a boolean. I suspect lots of people call it with numbers
and random non-boolean things that are convertable to a boolean (ex. undefined).

ex.

addEventListener(options.type, options.fn, options.capture)

where options.capture is undefined, or

addEventListener("foo", fn, array.length)

[philipj_slow, 2015-10-19 13:58:50]

On 2015/10/16 21:56:55, esprehn wrote:
> I think you actually want to check how often the third parameter is an object,
> not how often it's not a boolean. I suspect lots of people call it with
numbers
> and random non-boolean things that are convertable to a boolean (ex.
undefined).
> 
> ex.
> 
> addEventListener(options.type, options.fn, options.capture)
> 
> where options.capture is undefined, or
> 
> addEventListener("foo", fn, array.length)

Yep, I second that, it's only things that can be mistaken for dictionaries that
matter, risk-wise.

[dtapuska, 2015-10-19 14:52:53]

On 2015/10/16 21:56:55, esprehn wrote:
> I think you actually want to check how often the third parameter is an object,
> not how often it's not a boolean. I suspect lots of people call it with
numbers
> and random non-boolean things that are convertable to a boolean (ex.
undefined).
> 
> ex.
> 
> addEventListener(options.type, options.fn, options.capture)
> 
> where options.capture is undefined, or
> 
> addEventListener("foo", fn, array.length)

Done

[dtapuska, 2015-10-19 17:44:58]

On 2015/10/19 14:52:58, dtapuska wrote:

Updated the code to include null and undefined; as the bindings code that is
generated when this is a union it includes null and undefined.

So basically this is summing the number of calls we will change behavior on.

If we want to use custom bindings; we can force different behavior for null and
undefined; but I don't know if that is what we'd really want.

[dtapuska, 2015-10-19 17:49:14]

Patchset #4 (id:60001) has been deleted

[dtapuska, 2015-10-19 17:49:47]

On 2015/10/19 17:44:58, dtapuska wrote:
> On 2015/10/19 14:52:58, dtapuska wrote:
> 
> Updated the code to include null and undefined; as the bindings code that is
> generated when this is a union it includes null and undefined.
> 
> So basically this is summing the number of calls we will change behavior on.
> 
> If we want to use custom bindings; we can force different behavior for null
and
> undefined; but I don't know if that is what we'd really want.

Sorry disregard this comment.

[esprehn, 2015-10-19 18:01:08]

Lgtm, we don't usually add lines to that XML file... we just check the UMA for
FeatureObserver or the chromestatus page. Is there a reason to change it here?

[dtapuska, 2015-10-19 18:03:37]

On 2015/10/19 18:01:08, esprehn wrote:
> Lgtm, we don't usually add lines to that XML file... we just check the UMA for
> FeatureObserver or the chromestatus page. Is there a reason to change it here?

We didn't add lines to that XML file in the past because of the blink split.
There is a script to update the histograms.xml file
(update_use_counter_feature_enum.py).

It's just that previously someone use to periodically run it on the chromium
repo when the blink repo changed. Since they are merged together now; whenever
you add a UseCounter you probably should update the histograms.xml..

[Alexei Svitkine (slow), 2015-10-19 18:03:54]

On 2015/10/19 18:01:08, esprehn wrote:
> Lgtm, we don't usually add lines to that XML file... we just check the UMA for
> FeatureObserver or the chromestatus page. Is there a reason to change it here?

FYI: Actually, the XML file is usually updated - which is needed for UMA
dashboards (and I think chromestatus) to show the data. It's just before the
Blink/Chromium repository merge, it was done in a separate CL.

[dtapuska, 2015-10-19 18:07:06]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2015-10-19 18:07:06]

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/1414533002/#ps40001
(title: "Fix histograms.xml")

[commit-bot: I haz the power, 2015-10-19 18:09:16]

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

[commit-bot: I haz the power, 2015-10-19 20:28:58]

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

[commit-bot: I haz the power, 2015-10-19 20:28:59]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dtapuska, 2015-10-19 20:42:36]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2015-10-19 20:42:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
esprehn@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1414533002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2015-10-19 20:43:19]

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

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

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-10-19 23:28:50]

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