1250635 - remove ENABLE_TESTS from dom/media cpp files

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Joel Maher ( :jmaher ) (UTC -0800)]

In bug 1248590 we are looking to remove ENABLE_TESTS from ifdef's and in comment 18 we confirm we can do that:
https://bugzilla.mozilla.org/show_bug.cgi?id=1248590#c18

this bug will remove MP3Demuxer.cpp and ADTSDemuxer.cpp references.

Comment 1

[Joel Maher ( :jmaher ) (UTC -0800)]

Attachment: MozReview Request: Bug 1250635 - remove ENABLE_TESTS from dom/media cpp files. r?esawin

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

Comment 2

[u480271]

Comment on attachment 8722613 [details]
MozReview Request: Bug 1250635 - remove ENABLE_TESTS from dom/media cpp files. r?esawin

Eugen,

please review changes to MP3Demuxer. Just removing #ifdefs, exposing methods only used in TestMP3Demuxer.cpp, doesn't feel right.

Comment 3

[u480271]

Comment on attachment 8722613 [details]
MozReview Request: Bug 1250635 - remove ENABLE_TESTS from dom/media cpp files. r?esawin

https://reviewboard.mozilla.org/r/36129/#review32815

::: dom/media/ADTSDemuxer.h
(Diff revision 1)
> -#ifdef ENABLE_TESTS

For ADTSDemuxer, none of the code in #ifdef ENABLE_TESTS is used, so just delete it all.

::: dom/media/ADTSDemuxer.cpp
(Diff revision 1)
> -#ifdef ENABLE_TESTS

For ADTSDemuxer, none of the code in #ifdef ENABLE_TESTS is used, so just delete it all.

::: dom/media/MP3Demuxer.h
(Diff revision 1)
> -#ifdef ENABLE_TESTS

This code is used in gtest TestMP3Demuxer.cpp. :esawin is the author of this code and should review.

Comment 4

[Joel Maher ( :jmaher ) (UTC -0800)]

Comment on attachment 8722613 [details]
MozReview Request: Bug 1250635 - remove ENABLE_TESTS from dom/media cpp files. r?esawin

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

Comment 5

[Eugen Sawin [:esawin]]

Comment on attachment 8722613 [details]
MozReview Request: Bug 1250635 - remove ENABLE_TESTS from dom/media cpp files. r?esawin

https://reviewboard.mozilla.org/r/36129/#review32863

It's a safe change, we can look into rewriting tests eventually to remove some of the debug cruft.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7bcbdd73b99f

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7bcbdd73b99f

Comment 8

[Joel Maher ( :jmaher ) (UTC -0800)]

[Tracking Requested - why for this release]: I would like to uplift this to aurora so firefox 46 can take advantage of build promotion (bug 1248590)

Comment 9

[Joel Maher ( :jmaher ) (UTC -0800)]

Comment on attachment 8722613 [details]
MozReview Request: Bug 1250635 - remove ENABLE_TESTS from dom/media cpp files. r?esawin

Approval Request Comment
[Feature/regressing bug #]: 1248590
[User impact if declined]: N/A
[Describe test coverage new/current, TreeHerder]: removing code inside ifdef's and for MP3Demuxer removing ifdef's and leaving mPrevFrame and lastFrame() api available.
[Risks and why]: mostly no risk.  this is removing test code and exposing an api that is working in all our automation.
[String/UUID change made/needed]: N?A

Comment 10

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8722613 [details]
MozReview Request: Bug 1250635 - remove ENABLE_TESTS from dom/media cpp files. r?esawin

Taking out some redundant test situations, should help with release promotion, ok to uplift to aurora.

Comment 11

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

We don't need to track this, as we can see the issues open from bug 1248590

Comment 12

[Joel Maher ( :jmaher ) (UTC -0800)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a576eea081ad

Comment 13

[Carsten Book [:Tomcat]]

(In reply to Joel Maher (:jmaher) from comment #12)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/a576eea081ad

setting flags