Closed Bug 1235830 Opened 4 years ago Closed 4 years ago
Internal NSEvent to Widget Event, in ns IDOMEvent
58 bytes, text/x-review-board-request
As Olli Pettay suggested in Bug 1230216 Comment 9
"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"?
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.
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.
Review commit: https://reviewboard.mozilla.org/r/34567/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/34567/
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.
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
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 (:
You need to log in before you can comment on or make changes to this bug.