[webvtt] enable region preference

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: bechen, Assigned: bechen)

Tracking

(Depends on 1 bug, Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

()

Attachments

(5 attachments)

Assignee

Description

2 years ago
Now the preference of webvtt region is disble.
In this bug, I want to enable or remove it, and fix some wpt testcases.
Assignee

Updated

2 years ago
No longer blocks: 1338030
Depends on: 1338030
Target Milestone: --- → mozilla59
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Blocks: 1417820

Comment 5

2 years ago
mozreview-review
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 6

2 years ago
mozreview-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-
Assignee

Comment 7

2 years ago
mozreview-review-reply
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.
Assignee

Comment 8

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

2 years ago
mozreview-review
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+
Assignee

Comment 13

2 years ago
mozreview-review-reply
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?
Assignee

Updated

2 years ago
Blocks: 1419288

Comment 14

2 years ago
mozreview-review
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 15

2 years ago
mozreview-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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 20

2 years ago
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
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Blocks: 1420357

Comment 23

2 years ago
mozreview-review
Comment on attachment 8931561 [details]
Bug 1415805 - expose VTTRegion interface.

https://reviewboard.mozilla.org/r/202684/#review208120
Attachment #8931561 - Flags: review?(bugs) → review+
Assignee

Updated

2 years ago
Flags: needinfo?(bechen)
Keywords: checkin-needed

Comment 24

2 years ago
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.
Depends on: 1453774
You need to log in before you can comment on or make changes to this bug.