Closed Bug 1470930 Opened Last year Closed Last year

Use enums instead of booleans to pass arguments to event dispatch.

Categories

(Core :: DOM: Events, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

I'll be more confident writing the "composed" changes in bug 1470545 after this :)
Comment on attachment 8987552 [details]
Bug 1470930: Make RunnableKind an enum class.

https://reviewboard.mozilla.org/r/252790/#review259280
Attachment #8987552 - Flags: review?(nfroyd) → review+
Comment on attachment 8987553 [details]
Bug 1470930: Use enums for passing arguments for event dispatch.

https://reviewboard.mozilla.org/r/252792/#review259364

Required changes are mechanical, but because of aOnlyChromeDispatch change my guess is that this doesn't pass on try.

::: dom/base/nsContentUtils.h:1380
(Diff revision 1)
> -                                       nsISupports* aTarget,
> +    nsISupports* aTarget,
> -                                       mozilla::EventMessage aEventMessage,
> -                                       bool aCanBubble,
> -                                       bool aCancelable,
> -                                       bool *aDefaultAction = nullptr,
> -                                       bool aOnlyChromeDispatch = false)
> +    EventMessage aEventMessage,
> +    CanBubble aCanBubble,
> +    Cancelable aCancelable,
> +    bool* aDefaultAction = nullptr,
> +    ChromeOnlyDispatch aOnlyChromeDispatch = ChromeOnlyDispatch::Yes)

You switch onlychromedispatch from false to Yes. That is wrong and surprising.

::: dom/html/HTMLMediaElement.cpp:6961
(Diff revision 1)
>  bool
>  HTMLMediaElement::IsAllowedToPlay()
>  {
>    if (!AutoplayPolicy::IsMediaElementAllowedToPlay(WrapNotNull(this))) {
>  #if defined(MOZ_WIDGET_ANDROID)
> +    // FIXME: This should be chrome-only.

Want to file a bug?

::: widget/EventForwards.h:39
(Diff revision 1)
>  namespace mozilla {
>  
> +enum class CanBubble
> +{
> +  Yes,
> +  No,

You're going to kick me, but per Mozilla coding style, these should be eYes, eNo.
And I see no reason to not follow our coding style.
Modern event handling code in particular tries to be rather strict with coding style.

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Variable_prefixes

::: widget/EventForwards.h:39
(Diff revision 1)
>  namespace mozilla {
>  
> +enum class CanBubble
> +{
> +  Yes,
> +  No,

Drop , after No.
Same also elsewhere.
Attachment #8987553 - Flags: review?(bugs) → review-
See Also: → 1471013
Comment on attachment 8987553 [details]
Bug 1470930: Use enums for passing arguments for event dispatch.

https://reviewboard.mozilla.org/r/252792/#review259364

> You switch onlychromedispatch from false to Yes. That is wrong and surprising.

Whoops, nice catch.

> Want to file a bug?

Sure. Filed bug 1471013.

> You're going to kick me, but per Mozilla coding style, these should be eYes, eNo.
> And I see no reason to not follow our coding style.
> Modern event handling code in particular tries to be rather strict with coding style.
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Variable_prefixes

Eek, I don't like that at all, but sure.

```
find . -type f -name '*.h' -exec sed -i 's/\(Cancelable\|ChromeOnlyDispatch\|Trusted\|CanBubble\)::\(Yes\|No\)/\1::e\2/g' {} \;;
```

And same for `*.cpp`.

> Drop , after No.
> Same also elsewhere.

Done.
Comment on attachment 8987553 [details]
Bug 1470930: Use enums for passing arguments for event dispatch.

https://reviewboard.mozilla.org/r/252792/#review259710
Attachment #8987553 - Flags: review?(bugs) → review+
hg error in cmd: hg pull gecko -r 69eb422db6a29c70d2235a441858f30bb26e7b76: pulling from https://reviewboard-hg.mozilla.org/gecko
searching for changes
abort: HTTP Error 504: Gateway Timeout
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/71186bdcca21
Make RunnableKind an enum class. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/508445453d96
Use enums for passing arguments for event dispatch. r=smaug
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a825cd43372d
Fix a typo in OSX-only code. r=me CLOSED TREE
Depends on: 1492136
You need to log in before you can comment on or make changes to this bug.