Closed Bug 283531 Opened 19 years ago Closed 16 years ago

nsIEventStateManager::DispatchNewEvent() has confusing argument names

Categories

(Core :: DOM: Events, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jst, Assigned: jst)

Details

Attachments

(2 files)

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.
Attached patch Fix.Splinter Review
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?
Yes, thanks Ryan.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: