1219984 - Add support for multiple arguments to MediaEventSource

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[JW Wang [:jwwang] [:jw_wang]]

Instead of saying:

struct SomeStruct {
  int foo;
  int bar;
} someEvent;

someProducer.Notify(someEvent);

you can say:

int foo = 1;
int bar = 2;
someProducer.Notify(foo, bar);

without defining a new struct.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

You probably have a good use case for that, but just in case:
For simple structs, you can create them on the spot using aggregate initialization, e.g.:
> someProducer.Notify({foo, bar});

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Attachment: MozReview Request: Bug 1219984. Part 1 - remove EventPassMode::Both. In order to support multiple arguments, all arguments must be either moved or copied. r=kinetik.

Bug 1219984. Part 1 - remove EventPassMode::Both. In order to support multiple arguments, all arguments must be either moved or copied. r=kinetik.

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Attachment: MozReview Request: Bug 1219984. Part 2 - add support for multiple arguments. r=kinetik.

Bug 1219984. Part 2 - add support for multiple arguments. r=kinetik.

Comment 4

[Matthew Gregan [:kinetik]]

Comment on attachment 8681090 [details]
MozReview Request: Bug 1219984. Part 1 - remove EventPassMode::Both. In order to support multiple arguments, all arguments must be either moved or copied. r=kinetik.

https://reviewboard.mozilla.org/r/23765/#review21327

Comment 5

[Matthew Gregan [:kinetik]]

Comment on attachment 8681091 [details]
MozReview Request: Bug 1219984. Part 2 - add support for multiple arguments. r=kinetik.

https://reviewboard.mozilla.org/r/23767/#review21329

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Thanks for the review!

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Gerald Squelart [:gerald] from comment #1)
> You probably have a good use case for that, but just in case:
> For simple structs, you can create them on the spot using aggregate
> initialization, e.g.:
> > someProducer.Notify({foo, bar});

It looks like initializer_list doesn't play well with perfect forwarding.

struct SomeStruct {
    int foo;
    int bar;
};

void DoSomething(const SomeStruct& a) {
    std::cout << "foo=" << a.foo << "\n";
    std::cout << "bar=" << a.bar << "\n";
}

template <typename... Ts>
void ForwardSomething(Ts&&... args) {
    DoSomething(std::forward<Ts>(args)...);
}

int main()
{
    SomeStruct s {1, 2};
    
    DoSomething(s);
    DoSomething({3, 4});
    ForwardSomething(s);
    //ForwardSomething({3, 4}); // error!
}

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a027a19e086
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca24bcc14866

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Btw, I think we should support |someProducer.Notify({foo, bar})|. I will file a new bug to fix it.

Comment 10

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to JW Wang [:jwwang] from comment #7)
> (In reply to Gerald Squelart [:gerald] from comment #1)
> > You probably have a good use case for that, but just in case:
> > For simple structs, you can create them on the spot using aggregate
> > initialization, e.g.:
> > > someProducer.Notify({foo, bar});
> 
> It looks like initializer_list doesn't play well with perfect forwarding.
> 
> struct SomeStruct {..};
> void DoSomething(const SomeStruct& a) {..}
> template <typename... Ts>
> void ForwardSomething(Ts&&... args) {
>   DoSomething(std::forward<Ts>(args)...);
> }
> int main()
> {   ..
>     //ForwardSomething({3, 4}); // error!
> }

Good point.
That's due to the template automatic type deduction seeing '{..}' as an initializer_list, in absence of any direct clue.
'ForwardSomething<SomeStruct>({3, 4});' or 'ForwardSomething(SomeStruct{3, 4})' work fine.


(In reply to JW Wang [:jwwang] from comment #9)
> Btw, I think we should support |someProducer.Notify({foo, bar})|. I will
> file a new bug to fix it.

I'm not sure supporting initializer_list for a function accepting a struct is necessarily a good idea, as struct's may have different types of members while initializer_list only supports one type for all values; it usually makes most sense for things like initializing a container of values with the same type.
But once again, you probably know more about use cases in this code, where it would make sense.

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Gerald Squelart [:gerald] from comment #10)
> I'm not sure supporting initializer_list for a function accepting a struct
> is necessarily a good idea, as struct's may have different types of members
> while initializer_list only supports one type for all values;

It will not be very useful to requiring all members to be the same type. However, since |ForwardSomething(SomeStruct{3, 4})| works fine, we don't need to fix bug 1220514 until we figure out a good way to do that.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a027a19e086
https://hg.mozilla.org/mozilla-central/rev/ca24bcc14866

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/2a027a19e086
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/ca24bcc14866