Simplify constructor of WidgetCommandEvent

RESOLVED FIXED in Firefox 63

Status

()

defect
RESOLVED FIXED
6 years ago
5 months ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

Trunk
mozilla63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(2 attachments)

Posted patch PatchSplinter Review
This is used only for onAppCommand...

InitWithAppCommand() or SetAppCommand() is better?
Attachment #8374716 - Flags: review?(bugs)
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.
Attachment #8374716 - Flags: review?(bugs) → review-
(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").
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
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 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
Attachment #8995941 - Flags: review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/86f8589018f6
Hide event type from constructor of WidgetCommandEvent r=smaug
https://hg.mozilla.org/mozilla-central/rev/86f8589018f6
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.