Closed Bug 1250635 Opened 4 years ago Closed 4 years ago

remove ENABLE_TESTS from dom/media cpp files

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 - fixed
firefox47 --- fixed

People

(Reporter: jmaher, Unassigned)

References

Details

Attachments

(1 file)

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.
Attachment #8722613 - Flags: review?(dglastonbury)
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.
Attachment #8722613 - Flags: review?(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/#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.
Attachment #8722613 - Flags: review?(dglastonbury)
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/
Attachment #8722613 - Attachment description: MozReview Request: Bug 1250635 - remove ENABLE_TESTS from dom/media cpp files. r?dglastonbury → MozReview Request: Bug 1250635 - remove ENABLE_TESTS from dom/media cpp files. r?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.
Attachment #8722613 - Flags: review?(esawin) → review+
https://hg.mozilla.org/mozilla-central/rev/7bcbdd73b99f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
[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 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
Attachment #8722613 - Flags: approval-mozilla-aurora?
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.
Attachment #8722613 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
We don't need to track this, as we can see the issues open from bug 1248590
You need to log in before you can comment on or make changes to this bug.