1415805 - [webvtt] enable region preference

Status: RESOLVED FIXED

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

Description

[Benjamin Chen [:bechen]]

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

Comment 1

[Benjamin Chen [:bechen]]

Attachment: Bug 1415805 - throw exception at region.lines setter if value is negative.

Review commit: https://reviewboard.mozilla.org/r/200208/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200208/

Comment 2

[Benjamin Chen [:bechen]]

Attachment: Bug 1415805 - region.scroll setter should not throw.

Review commit: https://reviewboard.mozilla.org/r/200210/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200210/

Comment 3

[Benjamin Chen [:bechen]]

Attachment: Bug 1415805 - enable webvtt/paring/file-parsing/tests/region-old.html.

Review commit: https://reviewboard.mozilla.org/r/200212/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200212/

Comment 4

[Benjamin Chen [:bechen]]

Attachment: Bug 1415805 - enable region preference and wpt tests webvtt/api/VTTRegion.

Review commit: https://reviewboard.mozilla.org/r/200214/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200214/

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 7

[Benjamin Chen [:bechen]]

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 8

[Benjamin Chen [:bechen]]

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 9

[Benjamin Chen [:bechen]]

Comment on attachment 8928897 [details]
Bug 1415805 - enable webvtt/paring/file-parsing/tests/region-old.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200212/diff/1-2/

Comment 10

[Benjamin Chen [:bechen]]

Comment on attachment 8928898 [details]
Bug 1415805 - enable region preference and wpt tests webvtt/api/VTTRegion.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200214/diff/1-2/

Comment 11

[Benjamin Chen [:bechen]]

Comment on attachment 8928896 [details]
Bug 1415805 - region.scroll setter should not throw.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200210/diff/1-2/

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 13

[Benjamin Chen [:bechen]]

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?

Comment 14

[Ralph Giles (:rillian)]

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.

Comment 15

[Ralph Giles (:rillian)]

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!

Comment 16

[Benjamin Chen [:bechen]]

Comment on attachment 8928895 [details]
Bug 1415805 - throw exception at region.lines setter if value is negative.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200208/diff/1-2/

Comment 17

[Benjamin Chen [:bechen]]

Comment on attachment 8928898 [details]
Bug 1415805 - enable region preference and wpt tests webvtt/api/VTTRegion.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200214/diff/2-3/

Comment 18

[Benjamin Chen [:bechen]]

Comment on attachment 8928896 [details]
Bug 1415805 - region.scroll setter should not throw.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200210/diff/2-3/

Comment 19

[Benjamin Chen [:bechen]]

Comment on attachment 8928897 [details]
Bug 1415805 - enable webvtt/paring/file-parsing/tests/region-old.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200212/diff/2-3/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Cristian Brindusan [:cbrindusan]]

Backed out for mochitest failures on dom/tests/mochitest/general/test_interfaces.html:

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=edede99fa1c939b3061c99eb0494aaf139607403&filter-searchStr=483867fffa0d510e61db7e62f704c85e5d91bea0

https://hg.mozilla.org/integration/autoland/rev/48c532b07ee7fff86d37c15191b62aaf9b9c0db5

https://treeherder.mozilla.org/logviewer.html#?job_id=147078339&repo=autoland&lineNumber=18031

Comment 22

[Benjamin Chen [:bechen]]

Attachment: Bug 1415805 - expose VTTRegion interface.

Review commit: https://reviewboard.mozilla.org/r/202684/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202684/

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8931561 [details]
Bug 1415805 - expose VTTRegion interface.

https://reviewboard.mozilla.org/r/202684/#review208120

Comment 24

[Pulsebot]

Pushed by [e-mail]:
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

Comment 25

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/d8b370343cc3
https://hg.mozilla.org/mozilla-central/rev/2fd44617b8b8
https://hg.mozilla.org/mozilla-central/rev/0affcb3cdc54
https://hg.mozilla.org/mozilla-central/rev/2b12df089020
https://hg.mozilla.org/mozilla-central/rev/8adca8ee9b3e

Comment 26

[Eric Shepherd [:sheppy]]

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.