Count document.createEvent() usage

Count document.createEvent() usage

Count all document.createEvent() calls outside what's listed in:

https://dom.spec.whatwg.org/#dom-document-createevent

BUG=569690
Committed: https://crrev.com/b864804d2c75a5024c4b6d57ee23b17b55396730
Cr-Commit-Position: refs/heads/master@{#375149}

[davve, 2016-02-08 16:19:43]

davve@opera.com changed reviewers:
+ philipj@opera.com

[davve, 2016-02-08 16:19:44]

There is a number of ways we can go here. This is one of them; simply
add a counter for each event (excluding SVGZoomEvent which is slated
from deprecation and part of an upcoming CL) that don't have a
matching init*Event(). Use an extended attribute in the idl file for
requesting measurement.

An alternative would be to count them all in one and the same bucket,
if we think none of these will see any hits. I dunno.

Also, I guess it would be possible to keep the list centralized to
make_event_factory.py as we do for case insensitive matching if this
seems to intrusive. Looking forward, it seems like we'll have to mark
the ones white-listed by createEvent in some way, and that info likely
belongs in the .idl-file?

[philipj_slow, 2016-02-09 10:17:31]

On 2016/02/08 16:19:44, David Vest wrote:
> There is a number of ways we can go here. This is one of them; simply
> add a counter for each event (excluding SVGZoomEvent which is slated
> from deprecation and part of an upcoming CL) that don't have a
> matching init*Event(). Use an extended attribute in the idl file for
> requesting measurement.
> 
> An alternative would be to count them all in one and the same bucket,
> if we think none of these will see any hits. I dunno.
> 
> Also, I guess it would be possible to keep the list centralized to
> make_event_factory.py as we do for case insensitive matching if this
> seems to intrusive. Looking forward, it seems like we'll have to mark
> the ones white-listed by createEvent in some way, and that info likely
> belongs in the .idl-file?

Treating createEvent() as a legacy compat thing, I think that a mapping of the
sort in https://dom.spec.whatwg.org/#dom-document-createevent inside of our
createEvent() implementation might be the end goal here. It would also be good
to stop newly added events from getting support in createEvent().

That makes the make_event_factory.py idea more appealing. One could also stop
the spread of createEvent() support in the IDL files by requiring
[LegacyCreateEvent] or something, but could that capture everything interesting,
like the plural forms of some events? Keeping the whole mess in one place with
the aim of turning that into 10-20 lines of non-generated C++ down the line
sounds worth a try.

As for bucket sizes, this certainly does look wasteful, but the SVG1DOM case
makes me lean towards this.

[davve, 2016-02-09 10:37:32]

On 2016/02/09 10:17:31, philipj_UTC7 wrote:
> On 2016/02/08 16:19:44, David Vest wrote:
> > There is a number of ways we can go here. This is one of them; simply
> > add a counter for each event (excluding SVGZoomEvent which is slated
> > from deprecation and part of an upcoming CL) that don't have a
> > matching init*Event(). Use an extended attribute in the idl file for
> > requesting measurement.
> > 
> > An alternative would be to count them all in one and the same bucket,
> > if we think none of these will see any hits. I dunno.
> > 
> > Also, I guess it would be possible to keep the list centralized to
> > make_event_factory.py as we do for case insensitive matching if this
> > seems to intrusive. Looking forward, it seems like we'll have to mark
> > the ones white-listed by createEvent in some way, and that info likely
> > belongs in the .idl-file?
> 
> Treating createEvent() as a legacy compat thing, I think that a mapping of the
> sort in https://dom.spec.whatwg.org/#dom-document-createevent inside of our
> createEvent() implementation might be the end goal here. It would also be good
> to stop newly added events from getting support in createEvent().
> 
> That makes the make_event_factory.py idea more appealing. One could also stop
> the spread of createEvent() support in the IDL files by requiring
> [LegacyCreateEvent] or something, but could that capture everything
interesting,
> like the plural forms of some events? Keeping the whole mess in one place with
> the aim of turning that into 10-20 lines of non-generated C++ down the line
> sounds worth a try.

Having behavioral data in the code generation script feels like a hack
(under build/scripts/ of all places), so a end-state of 10-20 lines of
non-generated C++ sounds good. So if we in the long run write
createEvent() without code-generation it makes more sense to not
have anything relating to this in the .idl-files.

That said, doing counting in the code generation is probably fine in
preparation for the next step. I'll look into it.

> As for bucket sizes, this certainly does look wasteful, but the SVG1DOM case
> makes me lean towards this.

SVG1DOM was/is at least useful for _something_. Can these events be
used for anything?

[philipj_slow, 2016-02-09 12:41:28]

On 2016/02/09 10:37:32, David Vest wrote:
> On 2016/02/09 10:17:31, philipj_UTC7 wrote:
> > As for bucket sizes, this certainly does look wasteful, but the SVG1DOM case
> > makes me lean towards this.
> 
> SVG1DOM was/is at least useful for _something_. Can these events be
> used for anything?

My main fear would be feature-detection code that just checks for support of an
event type, either for events where the interfaces had [NoInterfaceObject] for
some reason, or code that mutated from code testing for other events. I'd be
surprised, but delighted, if all of the events being measured can be dropped at
once.

[davve, 2016-02-10 13:36:18]

On 2016/02/09 12:41:28, philipj_OOO_til_Feb15 wrote:
> On 2016/02/09 10:37:32, David Vest wrote:
> > On 2016/02/09 10:17:31, philipj_UTC7 wrote:
> > > As for bucket sizes, this certainly does look wasteful, but the SVG1DOM
case
> > > makes me lean towards this.
> > 
> > SVG1DOM was/is at least useful for _something_. Can these events be
> > used for anything?
> 
> My main fear would be feature-detection code that just checks for support of
an
> event type, either for events where the interfaces had [NoInterfaceObject] for
> some reason, or code that mutated from code testing for other events. I'd be
> surprised, but delighted, if all of the events being measured can be dropped
at
> once.

Ok, new attempt uploaded.

[philipj_slow, 2016-02-11 14:40:29]

LGTM with nit

https://codereview.chromium.org/1673243002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/make_event_factory.py (right):

https://codereview.chromium.org/1673243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_event_factory.py:62: # All events
_not_ on the following whitelist are being measured in
I like this approach! But can you reduce the list to just what the spec says? I
think there's some hope that device motion and orientation events could be
excluded from this list, and it would be a practical benefit if the list
eventually only contains things in core/.

(The things you remove should correspond to init*Event() with TODOs, but I may
not have documented all of them properly yet.

[davve, 2016-02-11 16:28:53]

Description was changed from

==========
Count document.createEvent() usage

Count all document.createEvent() calls that returns events without
corresponding init*Event function.

BUG=569690
==========

to

==========
Count document.createEvent() usage

Count all document.createEvent() calls outside what's listed in:

https://dom.spec.whatwg.org/#dom-document-createevent

BUG=569690
==========

[davve, 2016-02-11 16:30:23]

The list now happens to correlate quite closely to case_insensitive_matching :)
(I take that as a good sign.)

I'll merge them when I address SVGZoomEvent() in a follow-up.

[davve, 2016-02-12 08:01:13]

The CQ bit was checked by davve@opera.com

[davve, 2016-02-12 08:01:14]

The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com
Link to the patchset: https://codereview.chromium.org/1673243002/#ps120001
(title: "Leave histograms.xml for later to avoid having to re-enumerate
everything again.")

[commit-bot: I haz the power, 2016-02-12 08:02:41]

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

[commit-bot: I haz the power, 2016-02-12 09:18:22]

Description was changed from

==========
Count document.createEvent() usage

Count all document.createEvent() calls outside what's listed in:

https://dom.spec.whatwg.org/#dom-document-createevent

BUG=569690
==========

to

==========
Count document.createEvent() usage

Count all document.createEvent() calls outside what's listed in:

https://dom.spec.whatwg.org/#dom-document-createevent

BUG=569690
==========

[commit-bot: I haz the power, 2016-02-12 09:18:24]

Committed patchset #7 (id:120001)

[philipj_slow, 2016-02-13 14:29:58]

https://codereview.chromium.org/1673243002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/build/scripts/make_event_factory.py (right):

https://codereview.chromium.org/1673243002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/make_event_factory.py:54: # with the
exepction for |keyevents| not present in Blink.
https://github.com/whatwg/dom/issues/148

[commit-bot: I haz the power, 2016-02-16 22:42:22]

Description was changed from

==========
Count document.createEvent() usage

Count all document.createEvent() calls outside what's listed in:

https://dom.spec.whatwg.org/#dom-document-createevent

BUG=569690
==========

to

==========
Count document.createEvent() usage

Count all document.createEvent() calls outside what's listed in:

https://dom.spec.whatwg.org/#dom-document-createevent

BUG=569690

Committed: https://crrev.com/b864804d2c75a5024c4b6d57ee23b17b55396730
Cr-Commit-Position: refs/heads/master@{#375149}
==========

[commit-bot: I haz the power, 2016-02-16 22:42:24]

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