1167492 - [US Regulation] To Create Settings for Configuring Default Attributes of WebVTT

Status: RESOLVED INCOMPLETE

(Firefox OS Graveyard :: Gaia::Settings, defect)

Description

[Marco Chen [:mchen]]

In Accessibility of US regulation, a device should have a settings for user to configure the font family / size / color ...etc of subtitle (WebVTT in Firefox OS).

This might not be a gaia work only, gecko should also have a way to adapt these settings value from user.

Comment 1

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Do we still need this for red-tai?

Comment 2

[Eitan Isaacson [:eeejay]]

I think we should do it.

To do it correctly, webvtt track cues should be stylable, and we should havea way to override the default "browser" style for all apps. This depends on bug 865395.

To add this feature quickly(*), I think we could simply have the default font/color/size be prefs. The implementation steps would be:

1. We would use prefs instead of hard-coded values here:
https://dxr.mozilla.org/mozilla-central/source/dom/media/webvtt/vtt.jsm#736

2. We would add have the prefs proxies by content-facing settings here:
https://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/settings.js

3. We would add a panel under the accessibility section in the settings app to tweak those settings.

(*) Traditionally, we allow overriding site styles with prefs for accessibility needs. I don't think this would be any different.

Comment 3

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

(In reply to Eitan Isaacson [:eeejay] from comment #2)
> I think we should do it.
Assuming ETA = 9/30, and land to 2.2R?

Comment 4

[Eitan Isaacson [:eeejay]]

(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #3)
> (In reply to Eitan Isaacson [:eeejay] from comment #2)
> > I think we should do it.
> Assuming ETA = 9/30, and land to 2.2R?

Sounds good.

Comment 5

[kang hee don]

According to FCC Part 79.103 requirement, WebVTT should support below attributes
 - Character color
 - Character opacity
 - Character size
 - Fonts
 - Caption background color and opacity
 - Character edge attributes
 - Caption window color
 - Language
 
 Could you consider to implement additional attributes?

Comment 6

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

I wonder if accessibility team can do the additional ones given the short timeline.

Comment 7

[Eitan Isaacson [:eeejay]]

What is the specific timeline? Could we start work on this after September?

Comment 8

[kang hee don]

I could implement below features refer to #2.
 - Character color
 - Character opacity
 - Character size
 - Caption background color and opacity
 - Character edge attributes

and add pref observer to catch up user's setting whenever user changes settings.

However I could not implement "Fonts and Caption window color".
Could you advise about that?

Thanks.

Comment 9

[Jinyoon]

FYI, The below is test site. If user changes any value in setting menu for caption, it affect to player both of borwser inline and local player.
http://www.cpcweb.com/webcasts/webcast_samples.htm

Comment 10

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

(In reply to Eitan Isaacson [:eeejay] from comment #7)
> What is the specific timeline? Could we start work on this after September?

Hi Eitan, 
I think partner is willing to help per comment 8.
Yes, we'll have updated timeline which is being discussed.
Could you reply to comment 8 and then we can proceed?

Comment 11

[Eitan Isaacson [:eeejay]]

(In reply to kang hee don from comment #8)
> 
> However I could not implement "Fonts and Caption window color".
> Could you advise about that?
> 

I think your guess is as good as mine.

It seems like there is a distinction in how the captions are presented as an overlay of the video or a separate window. So one requirement is the overlay background, and another is for the window.

We only support an overlay now and not a window, I expect that to be good enough for a mobile device. So I don't think the rest is relevant to us.

https://www.law.cornell.edu/cfr/text/47/79.103

Comment 12

[Eitan Isaacson [:eeejay]]

Re-assigning, as per comment #8.

Comment 13

[kang hee don]

I will make patches and attach

Comment 14

[Wesly Huang (EPM)]

Hi kang hee don:

Appreciate for the feedback, would you also help confirm we are agreed on current feature scope of this one, due to the limitation mentioned in comment#11?

Thanks!

Comment 15

[kang hee don]

Hi Wesly:

I'm sorry, I didn't have no authority to agree on current feature scope.
As far as I know, The person in charge is discussing about the scope.

Thanks

Comment 16

[Wesly Huang (EPM)]

Thanks Kang Hee Don for the feedback.

Then do you think the person in charge will be back and comment on bugzilla directly? Would you help ask him/her to do so? Or you will help on it? Thanks.

Comment 17

[Wesly Huang (EPM)]

Would you help me confirm the relation between this one and bug 1208290?

Per the meeting on Sep. 24, I think there a 2 steps of this issue:
(1) LG will double check w/ VZW for the rest items, 
(2) if possible upload patches for below 6 items (seems you used bug 1208290 to cover this)

- Character color
- Character opacity
- Character size
- Caption background color and opacity
- Character edge attributes
- Preview and setting retention

May I assume above (1) is done and we are good w/ current scope discussed (no any further action on both sides for this issue), and bug 1208290 is for (2)? 

If yes, then how about changing bug 1208290 to *block* 1167492 (this issue)?

Comment 18

[promise09th]

Dear Wesly Huang

Mr. Kang said bug 1167492 do not cover implementing Settings menu of Gaia Settings app. He said this bug covers only Browser - video
So, I create bug 1208290 for Settings menu of Gaia Settings app

Comment 19

[Wesly Huang (EPM)]

(In reply to promise09th from comment #18)
> Dear Wesly Huang
> 
> Mr. Kang said bug 1167492 do not cover implementing Settings menu of Gaia
> Settings app. He said this bug covers only Browser - video
> So, I create bug 1208290 for Settings menu of Gaia Settings app

Thanks, then I will set 1208290 to *block* this 1167492. (for (2) in comment#17)

And please kindly let us know once you get confirmation from VZW, about rest items. (for (1) in comment#17)

Thanks.

Comment 20

[kang hee don]

Attachment: webvtt.patch

This patch is for webvtt setting

Comment 21

[kang hee don]

Sorry for my late patch.
Could you review patch for setting configures default attributes of WebVTT?

I found a problem.
[reproduce]
 1. pause video in browser
 2. press home button
 3. change some attributes of WebVTT
 4. long press home button
 5. select browser
 6. resume video
 7. move to some previous time using progress bar in video
 
[problem]
 The attributes of WebVTT was not applied.
 I think the attributes of WebVTT is applied after that time of pause.

Thanks

Comment 22

[Eitan Isaacson [:eeejay]]

(In reply to kang hee don from comment #21)
> Sorry for my late patch.
> Could you review patch for setting configures default attributes of WebVTT?
> 
> I found a problem.
> [reproduce]
>  1. pause video in browser
>  2. press home button
>  3. change some attributes of WebVTT
>  4. long press home button
>  5. select browser
>  6. resume video
>  7. move to some previous time using progress bar in video
>  
> [problem]
>  The attributes of WebVTT was not applied.
>  I think the attributes of WebVTT is applied after that time of pause.
> 
> Thanks

It is worth filing a followup bug for that. Obviously, the captions are not being recreated, but played back instead.

Comment 23

[Eitan Isaacson [:eeejay]]

Comment on attachment 8668850 [details] [diff] [review]
webvtt.patch

Review of attachment 8668850 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for this :)

I know enough about these two files to know that the module peers will probably not accept this patch as-is.

When you have a new patch ready, please flag Ralph Giles ([e-mail]) for the webvtt bits, and Fabrice Desré ([e-mail]) for the settings.js changes.

::: b2g/chrome/content/settings.js
@@ +308,5 @@
> +
> +SettingsListener.observe("webvtt.edge.color", "000000", function(value){
> +  Services.prefs.setCharPref("webvtt.edge.color", value);
> +  Services.prefs.savePrefFile(null);
> +});

I think it is enough to add these setting names to settingsToObserve. You don't need to set up a separate observer for each. savePrefFile() is not needed.

::: dom/media/webvtt/vtt.jsm
@@ +10,3 @@
>  /**
>   * Code below is vtt.js the JS WebVTT implementation.
>   * Current source code can be found at http://github.com/mozilla/vtt.js

It looks like the canonical source of this file is not in the Mozilla tree. I imagine that this file needs to remain untouched so that it will be easier to pull in a newer upstream version (update-webvtt.js does just that).

I apologize for my comment earlier saying this is the file you want to change. I think the real change should happen in WebVTTParserWrapper.js. WebVTTParserWrapper.convertCueToDOMTree should apply some styling before returning.

Comment 24

[kang hee don]

(In reply to Eitan Isaacson [:eeejay] from comment #23)
> Comment on attachment 8668850 [details] [diff] [review]
> webvtt.patch
> 
> Review of attachment 8668850 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for this :)
> 
> I know enough about these two files to know that the module peers will
> probably not accept this patch as-is.
> 
> When you have a new patch ready, please flag Ralph Giles ([e-mail])
> for the webvtt bits, and Fabrice Desré ([e-mail]) for the
> settings.js changes.
> 
> ::: b2g/chrome/content/settings.js
> @@ +308,5 @@
> > +
> > +SettingsListener.observe("webvtt.edge.color", "000000", function(value){
> > +  Services.prefs.setCharPref("webvtt.edge.color", value);
> > +  Services.prefs.savePrefFile(null);
> > +});
> 
> I think it is enough to add these setting names to settingsToObserve. You
> don't need to set up a separate observer for each. savePrefFile() is not
> needed.

Could you explain more detaily?
You mean the observe functions don't need to have default value?
If my thought waw right, I added default value for default attributes.
Because users can change each attributes at any time and the attributes of WebVTT have different type of value,I added a separate observer for applying user's changes. 

> ::: dom/media/webvtt/vtt.jsm
> @@ +10,3 @@
> >  /**
> >   * Code below is vtt.js the JS WebVTT implementation.
> >   * Current source code can be found at http://github.com/mozilla/vtt.js
> 
> It looks like the canonical source of this file is not in the Mozilla tree.
> I imagine that this file needs to remain untouched so that it will be easier
> to pull in a newer upstream version (update-webvtt.js does just that).
> 
> I apologize for my comment earlier saying this is the file you want to
> change. I think the real change should happen in WebVTTParserWrapper.js.
> WebVTTParserWrapper.convertCueToDOMTree should apply some styling before
> returning.

First of all, This patches are for 2.2r+.
As I understood, because the vtt.jsm was created using vtt.js and update-webvtt.js, some changes for attributes of WebVTT should not happen in vtt.jsm.

As your comment #22, 
I checked convertCueToDOMTree, but the convertCueToDOMTree was not called.
What do you think about applying patches to vtt.js?

About comment# 21
If I made some modify function shouldCompute in vtt.jsm, This bug could be resolved.
Can I make some modify at shouldCompute function?

One more, I changed font family from "sans-serif" to "serif". 
I checked the font family of text was applied.
Could you let me know what font family were supported?

Comment 25

[kang hee don]

(In reply to kang hee don from comment #24)
> (In reply to Eitan Isaacson [:eeejay] from comment #23)
> > Comment on attachment 8668850 [details] [diff] [review]
> > webvtt.patch
> > 
> > Review of attachment 8668850 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Thanks for this :)
> > 
> > I know enough about these two files to know that the module peers will
> > probably not accept this patch as-is.
> > 
> > When you have a new patch ready, please flag Ralph Giles ([e-mail])
> > for the webvtt bits, and Fabrice Desré ([e-mail]) for the
> > settings.js changes.
> > 
> > ::: b2g/chrome/content/settings.js
> > @@ +308,5 @@
> > > +
> > > +SettingsListener.observe("webvtt.edge.color", "000000", function(value){
> > > +  Services.prefs.setCharPref("webvtt.edge.color", value);
> > > +  Services.prefs.savePrefFile(null);
> > > +});
> > 
> > I think it is enough to add these setting names to settingsToObserve. You
> > don't need to set up a separate observer for each. savePrefFile() is not
> > needed.
> 
> Could you explain more detaily?
> You mean the observe functions don't need to have default value?
> If my thought waw right, I added default value for default attributes.
> Because users can change each attributes at any time and the attributes of
> WebVTT have different type of value,I added a separate observer for applying
> user's changes. 
I'v got your comment.
I'll put codes for webvtt to settingsToObserve function in settings.js as your comment#23
 
> > ::: dom/media/webvtt/vtt.jsm
> > @@ +10,3 @@
> > >  /**
> > >   * Code below is vtt.js the JS WebVTT implementation.
> > >   * Current source code can be found at http://github.com/mozilla/vtt.js
> > 
> > It looks like the canonical source of this file is not in the Mozilla tree.
> > I imagine that this file needs to remain untouched so that it will be easier
> > to pull in a newer upstream version (update-webvtt.js does just that).
> > 
> > I apologize for my comment earlier saying this is the file you want to
> > change. I think the real change should happen in WebVTTParserWrapper.js.
> > WebVTTParserWrapper.convertCueToDOMTree should apply some styling before
> > returning.
> 
First of all, This patches are for 2.2r+.
As I understood, because the vtt.jsm was created using vtt.js and
update-webvtt.js, some changes for attributes of WebVTT should not happen in
vtt.jsm.
 
As your comment #22, 
I checked convertCueToDOMTree, but the convertCueToDOMTree was not called.
What do you think about applying patches to vtt.js?
 
About comment# 21
If I made some modify function shouldCompute in vtt.jsm, This bug could be
resolved.
Can I make some modify at shouldCompute function?

One more, I changed font family from "sans-serif" to "serif". 
I checked the font family of text was applied.
Could you let me know what font family were supported?

Comment 26

[Eitan Isaacson [:eeejay]]

(In reply to kang hee don from comment #25)
> First of all, This patches are for 2.2r+.
> As I understood, because the vtt.jsm was created using vtt.js and
> update-webvtt.js, some changes for attributes of WebVTT should not happen in
> vtt.jsm.
>  
> As your comment #22, 
> I checked convertCueToDOMTree, but the convertCueToDOMTree was not called.
> What do you think about applying patches to vtt.js?
>  

I don't think that is an option. The idea is to have this supported first in trunk, and then uplifted to the 2.2 branch. I would much prefer adding features that benefit future releases, otherwise we will be condemned to fixing this in each release.

> About comment# 21
> If I made some modify function shouldCompute in vtt.jsm, This bug could be
> resolved.
> Can I make some modify at shouldCompute function?
> 

I think the best solution for styling captions is to have a CSS rule that is changed when prefs change. Then it would affect all cues instantly.

It is not very straightforward. I think you need to consult with the module owner of those bits. I am flagging him here.

> One more, I changed font family from "sans-serif" to "serif". 
> I checked the font family of text was applied.
> Could you let me know what font family were supported?

I'm not sure. serif/san-serif should work.

Comment 27

[Wesly Huang (EPM)]

Also ping Kang-Hee-Don so he is aware of the reply in comment#26.

Comment 28

[pkj1020]

Attachment: settings.js.patch

Could you review the file I uploaded for the settings.js changes?

Comment 29

[pkj1020]

Attachment: vtt.js.patch

Could you review the file I uploaded for the vtt.js changes?

Comment 30

[Ralph Giles (:rillian)]

Comment on attachment 8677280 [details] [diff] [review]
vtt.js.patch

Review of attachment 8677280 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for taking this on. I'd like to see some cleanup before we try to land this to avoid future problems though.

This needs to be split into a patch we can apply upstream (https://github.com/mozilla/vtt.js) and the gecko-integration part which should be applied by the update-webvtt.js script. I think it would be good to work as much of the preference support as possible into upstream, since other users will need this, bug references to the Services module need to be in the gecko-integration part so it still works in normal contexts.

Please address comments and ask me for review again when you have an updated patch.

Rick, do you have any comments?

::: lib/vtt.js
@@ +17,5 @@
>  /* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>  /* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */
>  
> +const Cu = Components.utils;
> +Cu.import("resource://gre/modules/Services.jsm");

This is specific to gecko integration as a jsm, and needs to be in a separate patch applied by update-webvtt.js.

@@ +23,2 @@
>  (function(global) {
> +    var webVttSet = {};

webVTTSet

@@ +29,5 @@
> +    var backgroundOpacity = Services.prefs.getIntPref("webvtt.bg.opacity") / 100;
> +    var edgeType = Services.prefs.getIntPref("webvtt.edge.type");
> +    var edgeColor = Services.prefs.getCharPref("webvtt.edge.color");
> +
> +    webVttSet.fontSize = Services.prefs.getIntPref("webvtt.font.size") / 100;

webVTTSet.fontScale would be a better name, I think? This is a percentage multiplying the CSS font-size. not a font size.

@@ +38,5 @@
> +    function makeColorSet(color, opacity = 1) {
> +        return "rgba(" + parseInt(color.substring(0, 2), 16) + ","
> +		       + parseInt(color.substring(2, 4), 16) + ","
> +		       + parseInt(color.substring(4, 6), 16) + ","
> +		       + opacity + ")";

jshint doesn't like your formatting here:

     40 |           + parseInt(color.substring(2, 4), 16) + ","
                    ^ Bad line breaking before '+'.
     41 |           + parseInt(color.substring(4, 6), 16) + ","
                    ^ Bad line breaking before '+'.
     42 |           + opacity + ")";
                    ^ Bad line breaking before '+'.

I don't know what it wants, but you might be able to simplify a bit with + [r,g,b].join(",").

As I understand it, we need this because the 'opacity' css property applies to both text and background, so merging these into rgba objects is the only way to set them independently. Is that correct?

@@ +42,5 @@
> +		       + opacity + ")";
> +    }
> +
> +    let webvttPrefs = ['webvtt.font.color', 'webvtt.font.opacity', 'webvtt.font.size', 'webvtt.bg.color',
> +                 'webvtt.bg.opacity', 'webvtt.edge.color', 'webvtt.edge.type'];

The initialization lines above duplicate the pref lookups in the update function below. Can you combine them by iterating over webvttPrefs and calling observe() on each?

@@ +775,5 @@
>      var isIE8 = (/MSIE\s8\.0/).test(navigator.userAgent);
>      var color = "rgba(255, 255, 255, 1)";
>      var backgroundColor = "rgba(0, 0, 0, 0.8)";
>  
> +    var isFirefox = (/firefox/i).test(navigator.userAgent);

Move this up so it's next to the corresponding isIE8 initialization above.

@@ +789,5 @@
> +      color = webVttSet.fontSet;
> +      backgroundColor = webVttSet.backgroundSet;
> +      textShadow = webVttSet.edgeSet;
> +      fontSizeRate = webVttSet.fontSize;
> +    }

Browser sniffing is ugly. What about 'if (webVTTSet)' instead?

Comment 31

[[:fabrice] Fabrice Desré]

Comment on attachment 8677278 [details] [diff] [review]
settings.js.patch

Review of attachment 8677278 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine, I'll just like to take a final look once the other part is ready to make sure we don't mess up the pref names.

::: b2g/chrome/content/settings.js
@@ +677,5 @@
>    'wap.UAProf.tagname': 'x-wap-profile',
> +  'wap.UAProf.url': '',
> +  'webvtt.font.color': {
> +    prefName: 'webvtt.font.color',
> +    defaultValue: "FFFFFF"

nit: use single quotes everywhere.

@@ +684,5 @@
> +    prefName: 'webvtt.font.opacity',
> +    defaultValue: 100
> +  },
> +  'webvtt.font.size': {
> +    prefName: 'webvtt.font.size',

Change this prefName to fontScale based on Ralph's comment.

@@ +701,5 @@
> +    defaultValue: 0
> +  },
> +  'webvtt.edge.color': {
> +    prefName: 'webvtt.edge.color',
> +    defaultValue: "FFFFFF"

nit: use single quotes everywhere

Comment 32

[John Chin]

Closed captioning settings shall include: font size and caption style (i.e. color of text and background) on streamed or pre-loaded native on-device video player for help and how-to-videos.

These settings shall be adjustable in the video player and may additionally be adjustable in system settings.  
Readability is the greatest concern so the preferred default settings should have the highest possible contrast, which is white text on a black background.

To ensure that readers have enough time to read, roll-up is a preferred format rather than pop-up presentation method for captioning.  Roll-up requires at least two lines or more of text to display the captioning.  Roll-up would present more text and information over a longer period of time than a single line pop-up.  This provides slower readers more time to read the captioning.  This is especially helpful for readers when English is their second language. For many deaf users, English is a second language after American Sign Language (ASL).

Comment 33

[John Chin]

Real-time or near real-time streaming is a MUST.  On-device pre-loaded is nice to have.

Comment 34

[pkj1020]

Attachment: 1028_settings.js.patch

I have modified 'settings.js' as your comment.
Could you review the file I uploaded for the settings.js changes?
- 1028_settings.js.patch

Comment 35

[pkj1020]

Attachment: 1028_vtt.js.patch

I have modified 'vtt.js' as your comment.
Could you review the file I uploaded for the vtt.js changes?
- 1028_vtt.js.patch

Comment 36

[pkj1020]

Attachment: 1028_update-webvtt.js.patch

As your comment, I also have modified 'update-webvtt.js'.
Please review the updated file.
- 1028_update-webvtt.js.patch

Comment 37

[Ralph Giles (:rillian)]

Comment on attachment 8679825 [details] [diff] [review]
1028_update-webvtt.js.patch

Review of attachment 8679825 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine, but you'll probably need more to make the vtt.js changes work ourside gecko.

Comment 38

[[:fabrice] Fabrice Desré]

Comment on attachment 8679820 [details] [diff] [review]
1028_settings.js.patch

Review of attachment 8679820 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits fixed

::: b2g/chrome/content/settings.js
@@ +683,5 @@
> +  'webvtt.font.opacity': {
> +    prefName: 'webvtt.font.opacity',
> +    defaultValue: 100
> +  },
> +  'webvtt.font.scale': {           

nit: trailing whitespace

@@ +696,5 @@
> +    prefName: 'webvtt.bg.opacity',
> +    defaultValue: 100
> +  },
> +  'webvtt.edge.type': {
> +    prefName: 'webvtt.edge.type', 

nit: trailing whitespace

Comment 39

[Ralph Giles (:rillian)]

Comment on attachment 8679824 [details] [diff] [review]
1028_vtt.js.patch

Review of attachment 8679824 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on the patch. It needs a some more changes before it will work. Please address comments and ask for review again.

::: lib/vtt.js
@@ +17,5 @@
>  /* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>  /* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */
>  
>  (function(global) {
> +  var webVttSet = {};

Again, please use webVTTSet or WebVTTSet to conform to local style.

@@ +19,5 @@
>  
>  (function(global) {
> +  var webVttSet = {};
> +  var edgeTypeList = ["", "0px 0px ", "4px 4px 4px ", "-2px -2px ", "2px 2px "];
> +  var fontColor = Services.prefs.getCharPref("webvtt.font.color");

All of these Services calls need to be conditional on the Service module being present, for this to work outside gecko.

@@ +31,5 @@
> +  webVttSet.fontSet = makeColorSet(fontColor, fontOpacity);
> +  webVttSet.backgroundSet = makeColorSet(backgroundColor, backgroundOpacity);
> +  webVttSet.edgeSet = edgeTypeList[edgeType] + makeColorSet(edgeColor);
> +
> +  function makeColorSet(color, opacity = 1) {

Default values for function arguments is an ES6 feature. It works in Firefox but not a lot of other browsers. To land this upstream please use and ES5 version of the code, or patch the build for https://github.com/mozilla/vtt.js with something like babeljs to compile down to ES5.

For example:

-  function makeColorSet(color, opacity = 1) {
-    return "rgba(" + [parseInt(color.substring(0, 2), 16), parseInt(color.substring(2, 4), 16), parseInt(color.substring(4, 6), 16), opacity].join(",") + ")";
+  function makeColorSet(color, opacity) {
+    if (opacity === undefined) {
+      opacity = 1;
+    }
+    return "rgba(" + [parseInt(color.substring(0, 2), 16),
+                      parseInt(color.substring(2, 4), 16),
+                      parseInt(color.substring(4, 6), 16),
+                      opacity].join(",") + ")";

@@ +38,5 @@
> +
> +  Services.prefs.addObserver("webvtt.font.color", function() {
> +      fontColor = Services.prefs.getCharPref("webvtt.font.color");
> +      webVttSet.fontSet = makeColorSet(fontColor, fontOpacity);
> +  }, false);

You're still duplicating the initialization code here. Having two places where the same variable is set by reading the same preference is harder to read, and makes it more likely someone will make a mistake later by changing one place and not the other. This principle is called "Do no repeat yourself".

How about going back to the earlier code with a shared observe(), and WebVTTPrefs.forEach() to install the observers, but also add a direct call to the observe function in that loop to do the initialization?

@@ +781,5 @@
> +    if(webVttSet){
> +      color = webVttSet.fontSet;
> +      backgroundColor = webVttSet.backgroundSet;
> +      textShadow = webVttSet.edgeSet;
> +      fontScaleRate = webVttSet.fontScale;

Calling this just fontScale like the WebVTTSet member is more clear, I think.

Comment 40

[pkj1020]

Attachment: 1030_settings.js.patch

I have removed white space.
Please review the attached patch.

Comment 41

[pkj1020]

Attachment: 1030_update-webvtt.js.patch

Services calls were moved to the update-vtt.js.
Please review the attached file.

Comment 42

[pkj1020]

Attachment: 1030_vtt.js.patch

Thank you for your review. 
Please see attached for your review and comments.	

1) changed 'webVttSet' to 'WebVTTSet'.

2) Services calls were moved to the update-vtt.js.
But This duplicated initialization code is essential.
If the observe function was not called from gaia app (ex: settings), 
the undefined value is assigned to the duplicated variables(fontColor...)
So the captions is not shown on the video if the settings were not ever changed.

3) added the opacity check code instead of default parameter as your comment.

4) The observe function was moved into WebVTTPrefs.forEach().

5) Removed the 'fontScale' variable to use the 'WebVTTSet.fontScale' variable directly.

Comment 43

[pkj1020]

Attachment: 1030_vtt.js.patch

Comment 44

[Ralph Giles (:rillian)]

Marking older patches as obsolete so it's clear what's proposed for landing. Feel free to do this yourself when you upload new versions. There's a checkbox for it on the 'Attach File' page.

Comment 45

[Ralph Giles (:rillian)]

Comment on attachment 8681134 [details] [diff] [review]
1030_vtt.js.patch

Review of attachment 8681134 [details] [diff] [review]:
-----------------------------------------------------------------

This still doesn't work outside gecko. Please address the comments and let me see it again.

::: lib/vtt.js
@@ +20,5 @@
>  (function(global) {
> +  var WebVTTSet = {};
> +  var edgeTypeList = ["", "0px 0px ", "4px 4px 4px ", "-2px -2px ", "2px 2px "];
> +
> +  WebVTTSet.fontScale = fontScale;

Where does the fontScale initialization value come from? The reason to split the patch in two is so this version still works without the preferences interface.

Why do you need to define these variables both inside and outside of WebVTTSet if they're all initialized from the pref values?

@@ +27,5 @@
> +  WebVTTSet.edgeSet = edgeTypeList[edgeType] + makeColorSet(edgeColor);
> +
> +  function makeColorSet(color, opacity) {
> +    if(opacity === undefined) {
> +      opacity = 1

Semicolon (;) at the end, please.

@@ +36,5 @@
> +                      opacity].join(",") + ")";
> +  }
> +
> +  let WebVTTPrefs = ['webvtt.font.color', 'webvtt.font.opacity', 'webvtt.font.size', 'webvtt.bg.color',
> +               'webvtt.bg.opacity', 'webvtt.edge.color', 'webvtt.edge.type'];

upstream's uglify doesn't like 'let' either. Please just use 'var' here.

@@ +44,5 @@
> +    function observe(subject, topic, data) {
> +       switch (data) {
> +          case "webvtt.font.color":
> +          case "webvtt.font.opacity":
> +              fontColor = Services.prefs.getCharPref("webvtt.font.color");

This still calls Services, which isn't available outside gecko. Try something like:

function observe_pref(subject, topic, data) {
  switch (data) {
    case "webvtt.font.color":
    case "webvtt.font.opacity":
	fontColor = Services.prefs.getCharPref("webvtt.font.color");
        ...
  }
}
// Initialize WebVTTSet and set observers for changes if
// the preferences API is available.
if (typeof Services !== "undefined") {
  WebVTTPrefs.forEach(function(pref) {
    observe_pref(undefined, undefined, pref);
    Services.prefs.addObserver(pref, observe_pref, false);
  }
}

@@ +779,5 @@
> +    if(WebVTTSet){
> +      color = WebVTTSet.fontSet;
> +      backgroundColor = WebVTTSet.backgroundSet;
> +      textShadow = WebVTTSet.edgeSet;
> +    }

WebVTTSet is always initialized in this patch, so the check doesn't do anything.

@@ +1236,5 @@
>      var boxPositions = [],
>          containerBox = BoxPosition.getSimpleBoxPosition(paddedOverlay),
>          fontSize = Math.round(containerBox.height * FONT_SIZE_PERCENT * 100) / 100;
>      var styleOptions = {
> +      font: (fontSize * WebVTTSet.fontScale) + "px " + FONT_STYLE

This won't work if WebVTTSet.fontScale isn't defined.

Comment 46

[pkj1020]

Attachment: 1102_update-webvtt.js.patch

Could you please review and let me know.

Comment 47

[pkj1020]

Attachment: 1102_vtt.js.patch

Could you please review and let me know.

Comment 48

[Ralph Giles (:rillian)]

Comment on attachment 8681805 [details] [diff] [review]
1102_vtt.js.patch

Review of attachment 8681805 [details] [diff] [review]:
-----------------------------------------------------------------

Very close now. Just a little more clean up needed. r=me with comments addressed.

::: lib/vtt.js
@@ +18,5 @@
>  /* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */
>  
>  (function(global) {
> +  var WebVTTSet = {};
> +  var edgeTypeList = ["", "0px 0px ", "4px 4px 4px ", "-2px -2px ", "2px 2px "];

Move edgeTypeList into the observe_pref function body. That's now the only place it's used, so it's better to define it there rather than in the outer scope.

@@ +25,5 @@
> +  var fontOpacity;
> +  var backgroundColor;
> +  var backgroundOpacity;
> +  var edgeType;
> +  var edgeColor;

You don't need these here either. Remove these and just add 'var' declarations to to their initializations in the observe_pref function body.

@@ +40,5 @@
> +
> +  var WebVTTPrefs = ['webvtt.font.color', 'webvtt.font.opacity', 'webvtt.font.scale', 'webvtt.bg.color',
> +               'webvtt.bg.opacity', 'webvtt.edge.color', 'webvtt.edge.type'];
> +
> +  function observe_pref(subject, topic, data) {

Sorry, my fault. Upstream wants this to be 'observePref' (camel case) instead. Your original 'observe' is also fine.

@@ +777,5 @@
>      var backgroundColor = "rgba(0, 0, 0, 0.8)";
> +    var textShadow = "";
> +    var fontScale = 1;
> +
> +    if(isFirefox){

This is not so good still. If navigator.userAgent matches 'Firefox', but we don't have the Services API, these attributes will be set to undefined. What matters is whether the WebVTTSet values are available, not what browser the code is running. See the fontScale errros in https://travis-ci.org/mozilla/vtt.js/builds/88908363 for an example.

Try moving 'var WebVTTSet = {};' line inside the Services conditional above. Then you can do something like 'if (context.WebVTTSet)' or 'if (typeof WebVTTSet !== "undefined")' here.

Comment 49

[pkj1020]

Attachment: 1103_vtt.js.patch

Please review the latest patch.
And I have a quesetion. Is this patch going to be applied to 2.2r branch?
Plaese let me know.

Thanks :)

Comment 50

[Ralph Giles (:rillian)]

Comment on attachment 8682326 [details] [diff] [review]
1103_vtt.js.patch

Review of attachment 8682326 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks. r=me with nit addressed.

::: lib/vtt.js
@@ +766,5 @@
>      var isIE8 = (/MSIE\s8\.0/).test(navigator.userAgent);
>      var color = "rgba(255, 255, 255, 1)";
>      var backgroundColor = "rgba(0, 0, 0, 0.8)";
> +    var textShadow = "";
> +    var fontScale = 1;

One last issue. This defines fontScale in CueStyleBox, but it's only used in defining styleOptions below in the outer scope. Please move this one to the first hunk of changes and add a second 'if (typeof WebVTTSet !== "undefined")' clause to override the default there.

https://travis-ci.org/rillian/vtt.js/builds/89108859

Comment 51

[Ralph Giles (:rillian)]

(In reply to pkj1020 from comment #49)

> And I have a quesetion. Is this patch going to be applied to 2.2r branch?

I don't know. Typically you would land the change on master, then set approval‑mozilla‑b2g37_v2_2r=? on the 'details' page for each patch you want to land on the branch, then wait a week or two for the release managers to consider the request.

Fabrice, do you have anything to add about backporting to 2.2r?

> Plaese let me know.
> 
> Thanks :)

Comment 52

[[:fabrice] Fabrice Desré]

Nothing to add, that's the usual process. But I'm sure we are faster than "one to 2 weeks" to grant approvals ;)

Comment 53

[pkj1020]

Attachment: 1105_vtt.js.patch

As your comments, I changed the patch.
Please review the attached file.

Thanks.

Comment 54

[Ralph Giles (:rillian)]

Comment on attachment 8683421 [details] [diff] [review]
1105_vtt.js.patch

Review of attachment 8683421 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks. That work programmatically, so please just address the style issue. r=me with change as requested.

::: lib/vtt.js
@@ +69,5 @@
> +  var fontScale = 1;
> +
> +  if(typeof WebVTTSet !== "undefined") {
> +    fontScale = WebVTTSet.fontScale;
> +  }

Sigh. Upstream's jshint checker warns here about using WebVTTSet out of scope. 

https://travis-ci.org/rillian/vtt.js/builds/89556823

I'm pretty sure that's a bug in the lint check, but move the 'var fontScale' declaration to before the 'if (typeof Services ...)' block. Then combine the fontScale override with that one.

That's shorter as well, so it's still an improvement.

Comment 55

[pkj1020]

Attachment: 1106_vtt.js.patch

Thank you for your review.
But if the same as now, some problems will be occured.

[reproduce]
  1. pause video in browser
  2. press home button
  3. change the fontScale
  4. long press home button
  5. select browser
  6. resume video

But the fontScale will be not changed.
(The other values will be changed normally.)
So I have removed the WebVTT.fontScale and used fontScale instead of WebVTT.fontScale.

Comment 56

[Ralph Giles (:rillian)]

Comment on attachment 8684036 [details] [diff] [review]
1106_vtt.js.patch

Review of attachment 8684036 [details] [diff] [review]:
-----------------------------------------------------------------

This one passed the upstream tests. Hooray!

https://travis-ci.org/rillian/vtt.js/builds/89688969

::: lib/vtt.js
@@ +57,5 @@
> +            break;
> +    }
> +  }
> +
> +  var fontScale = 1;

I assume this ends up being the same var observe() updates? Might be more clear to move the declaration above that of the observe function.

Comment 57

[Ralph Giles (:rillian)]

I've rebased the patches on mozilla-central + bug 1222537 and started test builds on our try service. If that shows no issues, and you're happy with the state of the patches, you can put 'checkin-needed' in the keyword field and someone will check the code into the master branch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38215e6720b9

Comment 58

[Ralph Giles (:rillian)]

Comment on attachment 8684036 [details] [diff] [review]
1106_vtt.js.patch

Review of attachment 8684036 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/vtt.js
@@ +57,5 @@
> +            break;
> +    }
> +  }
> +
> +  var fontScale = 1;

:shobson reminds me variable creation (but not initialization) is hoisted to the beginning of the closing scope, so this is valid, if a bit confusing. Best practice would still be to move it up above the first reference.

Comment 59

[pkj1020]

Attachment: 1109_vtt.js.patch

I have moved the declaration above the observe function.

Thank you :)

Comment 60

[Ralph Giles (:rillian)]

Comment on attachment 8684720 [details] [diff] [review]
1109_vtt.js.patch

Review of attachment 8684720 [details] [diff] [review]:
-----------------------------------------------------------------

Sure, thanks!

Comment 61

[Ralph Giles (:rillian)]

I've merged the vtt.js patch upstream in https://github.com/mozilla/vtt.js/commit/4e3da7b067c6d6cad2c78eeb57e3f4aaadc0293e

pkj1020, you haven't asked for checkin, but I assume this is ready. Doing so myself.

Comment 62

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a800bbb09f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/d02a1e8ea53d
https://hg.mozilla.org/integration/mozilla-inbound/rev/19ded68eecfb
https://hg.mozilla.org/integration/mozilla-inbound/rev/e35dba35a6ad

Comment 63

[Ralph Giles (:rillian)]

Rebased on current inbound, fixed indents, and ran the update-webvtt script to apply the patch importing the Services object for pref querying.

Comment 64

[Ralph Giles (:rillian)]

Looks like we have timeout problems on a number of tests. Can you investigate please? It's possible I missed something in rebasing, but it's also likely something hanging.

e.g. 

 TEST-UNEXPECTED-TIMEOUT | /webvtt/webvtt-file-format-parsing/webvtt-cue-text-parsing-rules/tests/entities.html | WebVTT cue data parser test entities - 87986551b0e6180cb279f2aa4cdddf77daa90c11 - Test timed out 

 TEST-UNEXPECTED-TIMEOUT | /webvtt/webvtt-file-format-parsing/webvtt-cue-text-parsing-rules/tests/tags.html | WebVTT cue data parser test tags - d49e42f7582c6f00b2569f2d14629611c0c6b0e6 - Test timed out

 TEST-UNEXPECTED-TIMEOUT | /webvtt/webvtt-file-format-parsing/webvtt-cue-text-parsing-rules/tests/timestamps.html | WebVTT cue data parser test timestamps - 66ba641ff047a226fa60fe867fd2479d40f3ff0f - Test timed out

Comment 65

[Ralph Giles (:rillian)]

See https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&selectedJob=17937001 and https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&selectedJob=17937325 from https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e35dba35a6ad for more examples.

Comment 66

[Phil Ringnalda (:philor)]

Or with any luck, https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e35dba35a6ad&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-failure_classification_id=2

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/6ea07e990e01

Comment 67

[Ralph Giles (:rillian)]

From IRC:

12:01 < catalinb> rillian: this might be worth investigating: 
                  https://pastebin.mozilla.org/8853265
12:02 < catalinb> rillian: and then JavaScript error: , line 0: uncaught 
                  exception: 
                  ..serviceworkers/fetch/context/index.html?testTrack 
12:02 < catalinb> rillian: I think something in the vtt js is causing an 
                  exception in the test page

Comment 68

[Ralph Giles (:rillian)]

No one at Mozilla is working on Firefox OS at this point. I think it's unlikely this will be addressed. Thanks for the report.