If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

nsIEventStateManager::DispatchNewEvent() has confusing argument names

RESOLVED FIXED

Status

()

Core
DOM: Events
RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: jst, Assigned: jst)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

13 years ago
The current nsIEventStateManager::DispatchNewEvent() signature looks like this:

  NS_IMETHOD DispatchNewEvent(nsISupports* aTarget, nsIDOMEvent* aEvent,
                              PRBool * aPreventDefault);

the confusing part of that is the last argument, aPreventDefault. What we do
with that argument, and IMO what we do is correct per the specs etc, is to tell
whether or not the default action is enabled, but the name suggests that it
means the exact opposite of that. This has lead to confusing code all throughout
our codebase, code that looks like this:

  PRBool noDefault;
  esm->DispatchNewEvent(target, event, &noDefault);

and that leads anyone who reads that code to believe that if noDefault is true,
then the default action was prevented. But again, the exact opposite is what it
really means.

I'm proposing to change the signature to:

  NS_IMETHOD DispatchNewEvent(nsISupports* aTarget, nsIDOMEvent* aEvent,
                              PRBool * aDefaultActionEnabled);

and to change code like the above to use a variable named "defaultActionEnabled"
to make it clear what we're really dealing with here. Patch coming up.
(Assignee)

Comment 1

13 years ago
Created attachment 175501 [details] [diff] [review]
Fix.
(Assignee)

Comment 2

13 years ago
Created attachment 175503 [details] [diff] [review]
Fix (diff -w for reviewers).
Attachment #175503 - Flags: superreview?(bryner)
Attachment #175503 - Flags: review?(bzbarsky)
Comment on attachment 175503 [details] [diff] [review]
Fix (diff -w for reviewers).

r=bzbarsky
Attachment #175503 - Flags: review?(bzbarsky) → review+
Attachment #175503 - Flags: superreview?(bryner) → superreview+
3 years later, I'm having a hard time telling for sure whether or not this has been checked in. However, searching MXR, it appears it might have been. jst, can this be marked FIXED?
(Assignee)

Comment 5

10 years ago
Yes, thanks Ryan.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.