Closed Bug 1235830 Opened 4 years ago Closed 4 years ago

Renaming GetInternalNSEvent to WidgetEvent, in nsIDOMEvent

Categories

(Core :: DOM: Events, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: aidin, Assigned: aidin)

Details

Attachments

(1 file)

As Olli Pettay suggested in Bug 1230216 Comment 9
Assignee: nobody → aidin
"WidgetEvent" is also the name of a type, so I can't use it as the name of a method.
Should I change the name to "GetWidgetEvent"?
Flags: needinfo?(bugs)
Oh, right. GetWidgetEvent() hints that it may return null, but it should never return null, so something else. Perhaps AsWidgetEvent() ? Not quite happy with that but nothing better comes to my mind now.
Flags: needinfo?(bugs)
I'm also using Get*() when * is a type/class name even if it never returns nullptr. Should we discuss this issue in dev-platform?
Perhaps, though I guess it has been mostly a dom/* convention that
Foo* GetFoo() may return null and Foo* AsFoo() or Foo* SomeOtherGetterListMethod() doesn't return null.
I asked on dev-platform, and Bobby Holley suggest the name WidgetEventRef. I think that's fine.

Here's the try server build without error:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4124741e66f4
Why would we name it WidgetEventRef if it doesn't actually return a reference?
Yeah, WidgetEventRef isn't right here.
WidgetEventPtr() would be ok, or make the method to return WidgetEvent& and then use WidgetEventRef.
If WidgetEventPtr in .idl is causing trouble here, feel free to change that name.
Attachment #8718421 - Flags: review?(bugs)
Attachment #8718421 - Flags: review?(bugs)
Comment on attachment 8718421 [details]
MozReview Request: Bug 1235830 - Renaming GetInternalNSEvent to WidgetEvent, in nsIDOMEvent r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34567/diff/1-2/
Now it's look better (:

Here's the try server build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1983d133e3dd
Attachment #8718421 - Flags: review?(bugs) → review+
Comment on attachment 8718421 [details]
MozReview Request: Bug 1235830 - Renaming GetInternalNSEvent to WidgetEvent, in nsIDOMEvent r?smaug

https://reviewboard.mozilla.org/r/34567/#review31435

Ah, WidgetEvent is in mozilla namespace so .idl part should work.
Thanks.
Thank you for the review (:
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/03fc467e1002
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.