Closed Bug 1415805 Opened 7 years ago Closed 7 years ago

[webvtt] enable region preference

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

Product:
Core
Component:
Audio/Video: Playback
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: bechen, Assigned: bechen)

References

(
URL
)

Details

(Keywords: dev-doc-complete)

Attachments

(5 files)

Bug 1415805 - throw exception at region.lines setter if value is negative.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1415805 - region.scroll setter should not throw.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1415805 - enable webvtt/paring/file-parsing/tests/region-old.html.
59 bytes, text/x-review-board-request
rillian
: review+
Details
Bug 1415805 - enable region preference and wpt tests webvtt/api/VTTRegion.
59 bytes, text/x-review-board-request
rillian
: review+
Details
Bug 1415805 - expose VTTRegion interface.
59 bytes, text/x-review-board-request
smaug
: review+
Details 
Now the preference of webvtt region is disble.
In this bug, I want to enable or remove it, and fix some wpt testcases.
No longer blocks: 1338030
Depends on: 1338030
Target Milestone: --- → mozilla59
Blocks: 1417820
Comment on attachment 8928895 [details]
Bug 1415805 - throw exception at region.lines setter if value is negative.

https://reviewboard.mozilla.org/r/200208/#review205396

There is no wpt test for this?
Attachment #8928895 - Flags: review?(bugs) → review+
Comment on attachment 8928896 [details]
Bug 1415805 - region.scroll setter should not throw.

https://reviewboard.mozilla.org/r/200210/#review205404

It should throw, but not from the method, but automatically from the bindings layer https://heycam.github.io/webidl/#es-enumeration.
We aren't following the spec here since we use DOMString and not ScrollSetting.
Is there any reason to not follow the spec?
Attachment #8928896 - Flags: review?(bugs) → review-
Comment on attachment 8928896 [details]
Bug 1415805 - region.scroll setter should not throw.

https://reviewboard.mozilla.org/r/200210/#review205404

I did not notice that the we should use ScrollSetting instead DOMString.
Comment on attachment 8928895 [details]
Bug 1415805 - throw exception at region.lines setter if value is negative.

https://reviewboard.mozilla.org/r/200208/#review205396

We have wpt test, and enable them in other patch/commit.
Comment on attachment 8928896 [details]
Bug 1415805 - region.scroll setter should not throw.

https://reviewboard.mozilla.org/r/200210/#review205976

::: dom/media/TextTrackRegion.h:127
(Diff revision 2)
> -    aScroll = mScroll;
> +    return mScroll;
>    }
>  
> -  void SetScroll(const nsAString& aScroll, ErrorResult& aRv)
> +  void SetScroll(const ScrollSetting& aScroll)
>    {
> -    if (!aScroll.EqualsLiteral("") && !aScroll.EqualsLiteral("up")) {
> +    if (aScroll == ScrollSetting::_empty || aScroll == ScrollSetting::Up) {

Why we need the if check. Just always assign the value.
The generated endguard value shouldn't be used
Attachment #8928896 - Flags: review?(bugs) → review+
Comment on attachment 8928896 [details]
Bug 1415805 - region.scroll setter should not throw.

https://reviewboard.mozilla.org/r/200210/#review205976

> Why we need the if check. Just always assign the value.
> The generated endguard value shouldn't be used

https://www.w3.org/TR/webvtt1/#dom-vttregion-scroll
The value of scroll can only be "up" or "", and there is also wpt test to test them.

https://www.w3.org/TR/webvtt1/#dom-scrollsetting
The _empty is the empty string, not the guard value.

The generated code in VTTRegionBinding.cpp :
extern const EnumEntry strings[3] = {
  {"", 0},
  {"up", 2},
  { nullptr, 0 }
};

Or did I miss something in the webidl file?
Blocks: 1419288
Comment on attachment 8928898 [details]
Bug 1415805 - enable region preference and wpt tests webvtt/api/VTTRegion.

https://reviewboard.mozilla.org/r/200214/#review206984

::: modules/libpref/init/all.js:584
(Diff revision 2)
>  #endif
>  
>  pref("media.getusermedia.audiocapture.enabled", false);
>  
>  // TextTrack WebVTT Region extension support.
> -pref("media.webvtt.regions.enabled", false);
> +pref("media.webvtt.regions.enabled", true);

Please send an 'intent to ship' email to dev-platform so people know the feature will be available.
Attachment #8928898 - Flags: review?(giles) → review+
Comment on attachment 8928897 [details]
Bug 1415805 - enable webvtt/paring/file-parsing/tests/region-old.html.

https://reviewboard.mozilla.org/r/200212/#review206988

Sorry for the slow review. Hooray for passing more tests!
Attachment #8928897 - Flags: review?(giles) → review+
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fa895478ed71
throw exception at region.lines setter if value is negative. r=smaug
https://hg.mozilla.org/integration/autoland/rev/ddfad494a323
enable region preference and wpt tests webvtt/api/VTTRegion. r=rillian
https://hg.mozilla.org/integration/autoland/rev/67a5bc2dcdc7
region.scroll setter should not throw. r=smaug
https://hg.mozilla.org/integration/autoland/rev/edede99fa1c9
enable webvtt/paring/file-parsing/tests/region-old.html. r=rillian
Keywords: checkin-needed
Blocks: 1420357
Comment on attachment 8931561 [details]
Bug 1415805 - expose VTTRegion interface.

https://reviewboard.mozilla.org/r/202684/#review208120
Attachment #8931561 - Flags: review?(bugs) → review+
Flags: needinfo?(bechen)
Keywords: checkin-needed
Pushed by aciure@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d8b370343cc3
throw exception at region.lines setter if value is negative. r=smaug
https://hg.mozilla.org/integration/autoland/rev/2fd44617b8b8
enable region preference and wpt tests webvtt/api/VTTRegion. r=rillian
https://hg.mozilla.org/integration/autoland/rev/0affcb3cdc54
region.scroll setter should not throw. r=smaug
https://hg.mozilla.org/integration/autoland/rev/2b12df089020
enable webvtt/paring/file-parsing/tests/region-old.html. r=rillian
https://hg.mozilla.org/integration/autoland/rev/8adca8ee9b3e
expose VTTRegion interface. r=smaug
Keywords: checkin-needed
Updated the following page to include info below the compatibility table about this change:

https://developer.mozilla.org/en-US/docs/Web/API/WebVTT_API#Browser_compatibility

Added a note to Firefox 59 for developers.
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 1453774
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: