Closed Bug 857022 Opened 11 years ago Closed 11 years ago

Figure out what to do with MOZ_MEDIA

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: Ms2ger, Assigned: rillian)

References

Details

Attachments

(4 files, 3 obsolete files)

I bet compiling without MOZ_MEDIA is all sorts of broken, and I don't think we are interested in maintaining this configuration, so I tend to think we should kill the useless ifdefs.

Thoughts?
If we aren't testing with it, I agree it's probably broken. I don't know that anyone actually relies on it.
In the past Thunderbird has complained about not wanting the extra code.

I agree we should remove it.
I also agree we should remove it.
Removing MOZ_MEDIA sounds good. Projects can still disable individual codecs and even all codecs using --disable-ogg etc if they wish to prevent media playing.

And if we remove MOZ_MEDIA and require projects that want to disable media to  disable all codecs, this has the advantage that HTMLMediaElement.canPlayType() will be available to report the formats that are unsupported (i.e. all of them), which is what that API is supposed to be used for.
(In reply to Ralph Giles (:rillian) from comment #2)
> In the past Thunderbird has complained about not wanting the extra code.

Not as far as I know. These days we generally want to be as close to core as possible.

I've no objections to MOZ_MEDIA being removed from the Thunderbird perspective, it appears we build it by default anyway at the moment.
Excellent, thanks for responding.
Assignee: nobody → giles
Blocks: 762201, 760968
Attached patch remove MOZ_MEDIA from content (obsolete) — Splinter Review
Attachment #733543 - Flags: review?(cpearce)
Attached patch remove MOZ_MEDIA from layout (obsolete) — Splinter Review
Attachment #733545 - Flags: review?(cpearce)
Attachment #733546 - Flags: review?(bugs)
Attachment #733548 - Flags: review?(ted)
Comment on attachment 733543 [details] [diff] [review]
remove MOZ_MEDIA from content

Review of attachment 733543 [details] [diff] [review]:
-----------------------------------------------------------------

Neat

::: content/html/content/src/Makefile.in
@@ +62,5 @@
>  		HTMLSharedElement.h \
>  		HTMLSharedListElement.h \
>  		HTMLSharedObjectElement.h \
> +		HTMLSourceElement.h \
> +		HTMLMenuElement.h \

... uh?
Attachment #733543 - Attachment description: remove MOZ_BUILD from content → remove MOZ_MEDIA from content
Attachment #733545 - Attachment description: remove MOZ_BUILD from content → remove MOZ_MEDIA from content
Attachment #733546 - Attachment description: remove MOZ_BUILD from parser → remove MOZ_MEDIA from parser
Attached patch remove MOZ_MEDIA from content (obsolete) — Splinter Review
Fixed spurious HTMLMenuElement.h move.
Attachment #733553 - Flags: review?(cpearce)
Attachment #733545 - Attachment is obsolete: true
Attachment #733545 - Flags: review?(cpearce)
Attachment #733545 - Attachment is obsolete: false
Attachment #733545 - Attachment description: remove MOZ_MEDIA from content → remove MOZ_MEDIA from layout
Attachment #733545 - Flags: review?(cpearce)
Attachment #733548 - Attachment description: remove MOZ_MEDIA from content → remove MOZ_MEDIA from remaining code
Attachment #733543 - Attachment is obsolete: true
Attachment #733543 - Flags: review?(cpearce)
Comment on attachment 733545 [details] [diff] [review]
remove MOZ_MEDIA from layout

Review of attachment 733545 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/build/Makefile.in
@@ -152,4 @@
>    $(NULL)
>  endif #}
>  
> -ifdef MOZ_MEDIA

These libs can go in main the SHARED_LIBRARY_LIBS on line 40, right?

You could fix the indenting on line 84 while you're here too (for domaudiochannel_s).
Attachment #733545 - Flags: review?(cpearce) → review+
Comment on attachment 733553 [details] [diff] [review]
remove MOZ_MEDIA from content

Review of attachment 733553 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

::: content/base/src/nsTreeSanitizer.cpp
@@ +1038,1 @@
>                         )) {

Could you move the ")) {" up onto the end of the "nsGkAtoms::source == aLocal" line to match the prevailing style of that file please? Thanks.

::: content/xslt/src/xslt/txMozillaXMLOutput.cpp
@@ +315,1 @@
>                    )) {

Plese move the ")) {" up to the end of previous line.
Attachment #733553 - Flags: review?(cpearce) → review+
Thanks for the review. Updating patch to include cpearce's comments. Carrying forward r+.
Attachment #733553 - Attachment is obsolete: true
Attachment #733568 - Flags: review+
Updated patch incorporating suggestions. Carrying forward r+.
Attachment #733545 - Attachment is obsolete: true
Attachment #733570 - Flags: review+
Attachment #733546 - Flags: review?(bugs) → review+
Attachment #733548 - Flags: review?(ted) → review+
I am the one complaining about this not being able to be disabled in Thunderbird :) But I have no problem if you remove MOZ_MEDIA so that all the DOM elements etc. are defined, as long as --disable-ogg, --disable-opus and similar (i.e. media codecs) still work. See https://bugzilla.mozilla.org/show_bug.cgi?id=760968#c1 . And that is what check in comment in patch https://bugzilla.mozilla.org/attachment.cgi?id=733548 confirms, so I am happy :)
Thanks for following up, sorry I mangled your requirements in my memory.

FWIW, I disagree that TB shouldn't support HTML5 features like <audio> and <video>.
Does this make alsa a required dependency of firefox/thunderbird/etc.?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: