Closed Bug 887978 Opened 7 years ago Closed 6 years ago

Turn WebVTT on by default on trunk.

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla28
Tracking Status
firefox28 + disabled
relnote-firefox --- 28+

People

(Reporter: davidb, Assigned: rillian)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, relnote)

Attachments

(1 file, 4 obsolete files)

Filing this bug so we can track required dependencies for WebVTT on by default.
I think this should be yours now.
Assignee: giles → rick.eyre
Thanks Ralph :-). What needs to be completed for this to happen?

I'm guessing https://bugzilla.mozilla.org/show_bug.cgi?id=webvtt should be resolved and enough fuzz testing that we believe it's safe?
Duplicate of this bug: 880711
Blocks: 880713
Blocks: webvtt
Attached patch Enable for non-release builds (obsolete) — Splinter Review
It looks like we've resolved all the issues blocking pref on. I'd like to get wider testing and decide if the implementation we have now is sufficiently useful to enable for releases. Anything else we need before doing this?

This patch enables webvtt by default on Nightly and Aurora only as a first step.
Attachment #8335770 - Flags: review?(cpearce)
Attachment #8335770 - Flags: feedback?(ehsan)
Attachment #8335770 - Flags: feedback?(ehsan) → feedback+
Attachment #8335770 - Flags: review?(cpearce) → review+
Hey Ralph, sorry had backed this out because of the suspicion that this caused bustages on mochitest 2/3 like 

https://tbpl.mozilla.org/php/getParsedLog.php?id=30886569&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=30887641&tree=Mozilla-Inbound
Definitely looks like our failure. Sorry, for the hassle.
Attached patch Enable for non-release builds (obsolete) — Splinter Review
Add new interfaces to the global list in test_interfaces.html. Carrying forward r=cpearce for media, passing to bz for DOM peer review.

What happens with this test in a release build?
Assignee: rick.eyre → giles
Attachment #8335770 - Attachment is obsolete: true
Attachment #8336352 - Flags: review?(bzbarsky)
Attached patch Enable for non-release builds v3 (obsolete) — Splinter Review
Handle the release case in the interface test. Thanks to smaug for help on that.

Carrying forward r=cpearce for media, passing to bz for DOM peer review.
Attachment #8336352 - Attachment is obsolete: true
Attachment #8336352 - Flags: review?(bzbarsky)
Attachment #8336372 - Flags: review?(bzbarsky)
bz: I moved the comment change to bug 941890.
Comment on attachment 8336372 [details] [diff] [review]
Enable for non-release builds v3

r=me
Attachment #8336372 - Flags: review?(bzbarsky) → review+
(In reply to Ralph Giles (:rillian) from comment #12)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1da0b2521da2

Hey Ralph,

had to backout this again :( seems it caused testfailures like https://tbpl.mozilla.org/php/getParsedLog.php?id=30947119&tree=Mozilla-Inbound on all platforms - android failure is this https://tbpl.mozilla.org/php/getParsedLog.php?id=30946777&tree=Mozilla-Inbound
Looks like the "problem" was (consistently) an unexpected-pass:

14651 ERROR TEST-UNEXPECTED-PASS | /tests/dom/imptests/html/microdata/microdata-dom-api/test_001.html | itemValue must reflect the src attribute on track elements

Maybe that's now expected?
(In reply to Jonathan Kew (:jfkthame) from comment #14)
> 14651 ERROR TEST-UNEXPECTED-PASS |
> /tests/dom/imptests/html/microdata/microdata-dom-api/test_001.html |
> itemValue must reflect the src attribute on track elements
> 
> Maybe that's now expected?

Yeah, looks like that. 

Ralph seems to be on PTO now, so can somebody please fix the test so this can land in Fx 28?

(In reply to Florian Bender from comment #15)
> (In reply to Jonathan Kew (:jfkthame) from comment #14)
> > 14651 ERROR TEST-UNEXPECTED-PASS |
> > /tests/dom/imptests/html/microdata/microdata-dom-api/test_001.html |
> > itemValue must reflect the src attribute on track elements
> > 
> > Maybe that's now expected?
> 
> Yeah, looks like that. 
> 
> Ralph seems to be on PTO now, so can somebody please fix the test so this
> can land in Fx 28?

I'll see if I can fix it.
I didn't find existing conditional code for the imptests, beyond a 'debug' option we don't seem to be using. So we need to add a 'nonrelease' conditional, or a blanket pref for the imptests like the mochitests have.

Or enable it unconditionally, I suppose.
Flags: needinfo?(Ms2ger)
Attached patch Enable unconditionally (obsolete) — Splinter Review
Since it's work to conditionalize the imptests, just enable it unconditionally. We have 8 weeks to uplift is reversion if we change our minds before release.

Pushed to try as https://tbpl.mozilla.org/?tree=Try&rev=d835ea8fb9fb
Attachment #8340150 - Flags: review?(cpearce)
Attachment #8340150 - Flags: review?(bzbarsky)
Attachment #8340150 - Flags: review?(cpearce) → review+
Remove trailing comma in failures.json. Carrying forward r=cpearce.

Green on try https://tbpl.mozilla.org/?tree=Try&rev=264b069325d5.
Attachment #8336372 - Attachment is obsolete: true
Attachment #8340150 - Attachment is obsolete: true
Attachment #8340150 - Flags: review?(bzbarsky)
Attachment #8340575 - Flags: review?(bzbarsky)
Flags: needinfo?(Ms2ger)
Comment on attachment 8340575 [details] [diff] [review]
Enable unconditionally

r=me
Attachment #8340575 - Flags: review?(bzbarsky) → review+
Oh, and I assume that the plan is in fact to enable this for the 28 release?  If not, we should have something tracking re-disabling it on Aurora/Beta?
https://hg.mozilla.org/integration/mozilla-inbound/rev/76e799f77069

Thanks for the review. Yes, the plan is to enable it for the 28 release.
https://hg.mozilla.org/mozilla-central/rev/76e799f77069
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Keywords: relnote
Thanks, missed the relnote-firefox? flag. I'd just noticed I'd forgotten to nominate WebVTT support for the release notes. I think it's important enough to mention, and there's already been some developer confusion about what versions support it.
We don't put the setting to version+ until it's in the notes DB and live on a channel's notes.  We can leave this nominated and pick it up when we edit the current 28 Beta notes for 28 Release.
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (X11; Linux x86_64; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:28.0) Gecko/20100101 Firefox/28.0

Verified as fixed with Firefox 28 beta 8 (Build ID: 20140303165517): media.webvtt.enabled pref is set to true by default.
Status: RESOLVED → VERIFIED
Added in the release note database for 28.
Thanks!
I've backed this out of beta 28; see bug 981280.
Depends on: 1048120
You need to log in before you can comment on or make changes to this bug.