Closed
Bug 1250635
Opened 6 years ago
Closed 6 years ago
remove ENABLE_TESTS from dom/media cpp files
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla47
People
(Reporter: jmaher, Unassigned)
References
Details
Attachments
(1 file)
58 bytes,
text/x-review-board-request
|
esawin
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details |
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.
Reporter | ||
Comment 1•6 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/36129/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36129/
Reporter | ||
Updated•6 years ago
|
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)
Reporter | ||
Comment 4•6 years ago
|
||
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 5•6 years ago
|
||
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+
Comment 7•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7bcbdd73b99f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Reporter | ||
Comment 8•6 years ago
|
||
[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)
status-firefox46:
--- → affected
tracking-firefox46:
--- → ?
Reporter | ||
Comment 9•6 years ago
|
||
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
Reporter | ||
Comment 12•6 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/a576eea081ad
Comment 13•6 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #12) > https://hg.mozilla.org/releases/mozilla-aurora/rev/a576eea081ad setting flags
You need to log in
before you can comment on or make changes to this bug.
Description
•