1286760 - The member initialization list of MediaDecoderStateMachine is a bit messy

Status: RESOLVED FIXED

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

Description

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

We will use some macros to make the code more compact and more readable.

Comment 1

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

Attachment: Bug 1286760 - use macro to simplify the code.

Review commit: https://reviewboard.mozilla.org/r/64176/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64176/

Comment 2

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

Comment on attachment 8770844 [details]
Bug 1286760 - use macro to simplify the code.

https://reviewboard.mozilla.org/r/64176/#review61222

I hate macros, but it does look better there, so fiiine: r+ with issue addressed.

Also, if similar macros could be useful in other files, you may want to create generic macros in StateWatching.h, and then have the macros in this file refine the generic ones with the local details ('mTaskQueue' and "MediaDecoderStateMachine::") -- But this could be a separate bug for later.

::: dom/media/MediaDecoderStateMachine.cpp:220
(Diff revision 1)
> +#define DECL_WATCHABLE(name, val) \
> +  name(val, "MediaDecoderStateMachine::" #name)
> +#define DECL_MIRROR(name, val) \
> +  name(mTaskQueue, val, "MediaDecoderStateMachine::" #name "(Mirror)")
> +#define DECL_CANONICAL(name, val) \
> +  name(mTaskQueue, val, "MediaDecoderStateMachine::" #name "(Canonical)")

These macros are used to *initialize* (not declare) members, so shouldn't they rather start with "INIT_"?

::: dom/media/MediaDecoderStateMachine.cpp:222
(Diff revision 1)
> +#define DECL_MIRROR(name, val) \
> +  name(mTaskQueue, val, "MediaDecoderStateMachine::" #name "(Mirror)")
> +#define DECL_CANONICAL(name, val) \
> +  name(mTaskQueue, val, "MediaDecoderStateMachine::" #name "(Canonical)")

Add a space at the start of " (Mirror)" and " (Canonical)" to match the original strings, unless you *do* want to remove that space.

Comment 3

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

https://reviewboard.mozilla.org/r/64176/#review61222

Can you illustrate how this could be done?

> These macros are used to *initialize* (not declare) members, so shouldn't they rather start with "INIT_"?

Will do.

> Add a space at the start of " (Mirror)" and " (Canonical)" to match the original strings, unless you *do* want to remove that space.

Will add the space.

Comment 4

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

Comment on attachment 8770844 [details]
Bug 1286760 - use macro to simplify the code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64176/diff/1-2/

Comment 5

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

https://reviewboard.mozilla.org/r/64176/#review61222

Sorry if I was unclear. I was thinking of something like this:

StateWatching.h:
#define INIT_MIRROR(class, taskqueue, name, val) \
  name(taskqueue, val, #class "::" #name "(Mirror)")

MediaDecoderStateMachine.cpp:
#define INIT_MSDM_MIRROR(name, val) \
  INIT_MIRROR(MediaDecoderStateMachine, mTaskQueue, name, val)

This way other classes could follow the same pattern by copying what's in MDSM.cpp, and all such files would be consistent.

Disclaimer: I'm scared of nested macros, so make sure that it actually works if you want to go ahead with it!

Comment 6

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

IMHO, INIT_MIRROR is just too thin to reduce complexity as much as nested macro adds. Consistency is easy to achieve when the macro is simple. I would prefer to define a similar INIT_MIRROR when it come to MediaDecoder member initialization.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cb1812acfcd1
use macro to simplify the code. r=gerald

Comment 8

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

(In reply to JW Wang [:jwwang] from comment #6)
> IMHO, INIT_MIRROR is just too thin to reduce complexity as much as nested
> macro adds. Consistency is easy to achieve when the macro is simple. I would
> prefer to define a similar INIT_MIRROR when it come to MediaDecoder member
> initialization.

Makes sense. Thank you for considering it.

Comment 9

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

Thanks for the review!

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cb1812acfcd1