971462 - Simplify constructor of WidgetCommandEvent

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

This is used only for onAppCommand...

InitWithAppCommand() or SetAppCommand() is better?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8374716 [details] [diff] [review]
Patch

This feels odd.
Perhaps InitAppCommand should take two nsIAtoms, the one for userType and the one
for command.

Or just let the ctor to take command but hide nsGkAtoms::onAppCommand in the
implementation of ctor, and don't have InitAppCommand at all.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #2)
> Perhaps InitAppCommand should take two nsIAtoms, the one for userType and
> the one for command.

I think that if we'll take this approach, the name should be InitCommand(). However, it looks like redundant all callers to pass nsGkAtoms::onAppCommand for the userType. Therefore, I added InitAppCommand() only for onAppCommand.

Don't you mind the redundancy?

> Or just let the ctor to take command but hide nsGkAtoms::onAppCommand in the
> implementation of ctor, and don't have InitAppCommand at all.

I think that this is not good for the case of document.createEvent("commandevent").

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

How is .createEvent case relevant here?

Could we actually get rid of gecko only CommandEvent, and just make FF UI to use the better
keyboard event support we have these days.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 971462 - Hide event type from constructor of WidgetCommandEvent

The constructor of WidgetCommandEvent takes 2 nsAtom pointers.  One is for
specifying event type, the other is for specifying the command.  The
difference of these arguments are pretty unclear for other developers and
the former argument is always nsGkAtoms::onAppCommand unless nullptr in
C++ code.  So, we can hide the former argument.

Then, we should create another constructor for creating empty command event
from constructor of dom::CommandEvent.

Comment 6

[Phabricator Automation]

Comment on attachment 8995941 [details]
Bug 971462 - Hide event type from constructor of WidgetCommandEvent

Olli Pettay [:smaug] has approved the revision.

https://phabricator.services.mozilla.com/D2506

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86f8589018f6
Hide event type from constructor of WidgetCommandEvent r=smaug

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a72ef5aa2c9fd24e20202dedf2004e98a6cb6ac

Comment 9

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/86f8589018f6