[WebVTT] Rename Middle to Center

[WebVTT] Rename Middle to Center

BUG=663797
Review-Url: https://codereview.chromium.org/2683633006
Cr-Commit-Position: refs/heads/master@{#450901}
Committed: https://chromium.googlesource.com/chromium/src/+/c129fcf7b932d95939244304d7e6db825c65117b

[maksims (do not use this acc), 2017-02-08 13:06:58]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 13:07:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maksims (do not use this acc), 2017-02-08 13:07:08]

Description was changed from

==========
[WebTT] Rename Middle to Center

BUG=663797
==========

to

==========
[WebVTT] Rename Middle to Center

BUG=663797
==========

[maksims (do not use this acc), 2017-02-08 13:08:04]

maksim.sisov@intel.com changed reviewers:
+ foolip@chromium.org

[maksims (do not use this acc), 2017-02-08 13:08:45]

maksim.sisov@intel.com changed reviewers:
+ ericwilligers@chromium.org

[maksims (do not use this acc), 2017-02-08 13:08:46]

ericwilligers@, please review histograms

foolip@, please review vtt

[foolip, 2017-02-08 13:39:40]

foolip@chromium.org changed reviewers:
+ rbyers@chromium.org

[foolip, 2017-02-08 13:39:41]

Change itself lgtm, but I think there will be failing tests to fix.

I think we really should import
https://github.com/w3c/web-platform-tests/tree/master/webvtt so that we'll
notice if changes like this affects test in web-platform-tests. Before landing
this, can you make a CL similar to https://codereview.chromium.org/2682913003/
to get the tests imported and wait for the automatic importer to run?

As for the use counters, rbyers@, have you ever changed the name long after
landing, do you know what things blow up?

https://codereview.chromium.org/2683633006/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2683633006/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:89508: +  <int value="416"
label="VTTCueRenderAlignNotCenter"/>
Did you rename this manually, or did running
tools/metrics/histograms/update_use_counter_feature_enum.py update it? I had
thought that the script wouldn't change existing entries.

I don't know if changing this file will trickle through to
https://www.chromestatus.com/metrics/feature/timeline/popularity/416

[maksims (do not use this acc), 2017-02-08 14:44:27]

https://codereview.chromium.org/2683633006/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2683633006/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:89508: +  <int value="416"
label="VTTCueRenderAlignNotCenter"/>
On 2017/02/08 13:39:41, foolip wrote:
> Did you rename this manually, or did running
> tools/metrics/histograms/update_use_counter_feature_enum.py update it? I had
> thought that the script wouldn't change existing entries.
> 
> I don't know if changing this file will trickle through to
> https://www.chromestatus.com/metrics/feature/timeline/popularity/416

I've changed it using the above mentioned script

[commit-bot: I haz the power, 2017-02-08 15:06:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 15:06:04]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Rick Byers, 2017-02-08 22:00:52]

https://codereview.chromium.org/2683633006/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2683633006/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:89508: +  <int value="416"
label="VTTCueRenderAlignNotCenter"/>
On 2017/02/08 14:44:27, maksims wrote:
> On 2017/02/08 13:39:41, foolip wrote:
> > Did you rename this manually, or did running
> > tools/metrics/histograms/update_use_counter_feature_enum.py update it? I had
> > thought that the script wouldn't change existing entries.
> > 
> > I don't know if changing this file will trickle through to
> > https://www.chromestatus.com/metrics/feature/timeline/popularity/416
> 
> I've changed it using the above mentioned script

Renaming enum members is supposed to be OK (IIRC we removed some comments from
UseCounter.h saying not to do this), but chromestatus.com still has an open bug
about handling it properly:
https://github.com/GoogleChrome/chromium-dashboard/issues/266

We decided not to put in the presubmit rule to block renames, so go ahead. 
We'll worry about the chromestatus issue separately.

[foolip, 2017-02-08 23:06:32]

Thanks Rick, so the use counter stuff is fine, just needs tests.

[Eric Willigers, 2017-02-08 23:24:37]

histograms.xml lgtm

[maksims (do not use this acc), 2017-02-09 07:30:34]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 07:30:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 09:12:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 09:12:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[foolip, 2017-02-15 05:50:03]

A test for this is now being enabled in
https://codereview.chromium.org/2694623006

[maksims (do not use this acc), 2017-02-15 06:15:07]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 06:15:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maksims (do not use this acc), 2017-02-15 06:15:58]

The CQ bit was unchecked by maksim.sisov@intel.com

[maksims (do not use this acc), 2017-02-15 06:19:41]

Patchset #3 (id:40001) has been deleted

[maksims (do not use this acc), 2017-02-15 06:20:49]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 06:21:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 07:59:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 07:59:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[maksims (do not use this acc), 2017-02-15 09:14:18]

On 2017/02/15 05:50:03, foolip wrote:
> A test for this is now being enabled in
> https://codereview.chromium.org/2694623006

Would you please check the following test -
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/...
?
The expected file looks wrong -
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/...
.

I get the following result - 
CONSOLE WARNING: line 24: The provided value 'start�' is not a valid enum value
of type AlignSetting.
CONSOLE ERROR: Text track from origin 'null' has been blocked from loading: Not
at same origin as the document, and parent of track element does not have a
'crossorigin' attribute. Origin 'http://web-platform.test:8001' is therefore not
allowed access.
This is a testharness.js-based test.
PASS VTTCue.align, script-created cue 
FAIL VTTCue.align, parsed cue assert_unreached: got error event Reached
unreachable code
Harness: the test ran to completion.

The problem is in CONSOLE WARNING. There is a NULL written with unicode, but I
get console warnings.Another thing is "PASS VTTCue.align, script-created cue",
but align-expected.txt
has "FAIL VTTCue.align, script-created cue assert_equals: expected "center" but
got "middle"" instead, which looks wrong.

[fs, 2017-02-15 09:38:49]

fs@opera.com changed reviewers:
+ fs@opera.com

[fs, 2017-02-15 09:38:50]

On 2017/02/15 at 09:14:18, maksim.sisov wrote:
> On 2017/02/15 05:50:03, foolip wrote:
> > A test for this is now being enabled in
> > https://codereview.chromium.org/2694623006
> 
> Would you please check the following test -
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/...
?
> The expected file looks wrong -
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/...
.
> 
> I get the following result - 
> CONSOLE WARNING: line 24: The provided value 'start�' is not a valid enum
value of type AlignSetting.
> CONSOLE ERROR: Text track from origin 'null' has been blocked from loading:
Not at same origin as the document, and parent of track element does not have a
'crossorigin' attribute. Origin 'http://web-platform.test:8001' is therefore not
allowed access.
> This is a testharness.js-based test.
> PASS VTTCue.align, script-created cue 
> FAIL VTTCue.align, parsed cue assert_unreached: got error event Reached
unreachable code
> Harness: the test ran to completion.
> 
> The problem is in CONSOLE WARNING. There is a NULL written with unicode, but I
get console warnings.

Yes, this is emitted by the bindings layer. Previously the assertion in question
wasn't reached, so no console warning was emitted.

> Another thing is "PASS VTTCue.align, script-created cue", but
align-expected.txt
> has "FAIL VTTCue.align, script-created cue assert_equals: expected "center"
but got "middle"" instead, which looks wrong.

Yes, you need to update the -expected.txt file (for this and the above.)

https://codereview.chromium.org/2683633006/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/opera/track/webvtt/parsing/001.html
(right):

https://codereview.chromium.org/2683633006/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/opera/track/webvtt/parsing/001.html:20:
"align":"center"
You need to update the source data (support/align.vtt supposedly) here too.

https://codereview.chromium.org/2683633006/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc018-align-text-line-position.html
(right):

https://codereview.chromium.org/2683633006/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc018-align-text-line-position.html:46:
{ align : "genter", position : "auto", line : "auto", snapToLines : true },
center

[maksims (do not use this acc), 2017-02-15 10:38:05]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 10:38:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maksims (do not use this acc), 2017-02-15 12:03:46]

Patchset #4 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-02-15 12:14:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[maksims (do not use this acc), 2017-02-15 12:22:01]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 12:22:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 13:59:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 13:59:58]

Dry run: This issue passed the CQ dry run.

[maksims (do not use this acc), 2017-02-16 06:41:09]

Any additional comments?

https://codereview.chromium.org/2683633006/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/opera/track/webvtt/parsing/001.html
(right):

https://codereview.chromium.org/2683633006/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/opera/track/webvtt/parsing/001.html:20:
"align":"center"
On 2017/02/15 09:38:50, fs wrote:
> You need to update the source data (support/align.vtt supposedly) here too.

Yep

https://codereview.chromium.org/2683633006/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc018-align-text-line-position.html
(right):

https://codereview.chromium.org/2683633006/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-webvtt-tc018-align-text-line-position.html:46:
{ align : "genter", position : "auto", line : "auto", snapToLines : true },
On 2017/02/15 09:38:50, fs wrote:
> center

Done.

[foolip, 2017-02-16 07:47:30]

Nothing further from me, just happy to see
third_party/WebKit/LayoutTests/external/wpt/webvtt/webvtt-api-for-browsers/vttcue-interface/align-expected.txt
in there :)

[maksims (do not use this acc), 2017-02-16 08:33:35]

The CQ bit was checked by maksim.sisov@intel.com

[maksims (do not use this acc), 2017-02-16 08:33:35]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org,
ericwilligers@chromium.org
Link to the patchset: https://codereview.chromium.org/2683633006/#ps100001
(title: "fix tests")

[commit-bot: I haz the power, 2017-02-16 08:34:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 08:49:21]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487234015042200,
"parent_rev": "c32cd2069ae8062b52e5b7b1faf5936bd71a583a", "commit_rev":
"c129fcf7b932d95939244304d7e6db825c65117b"}

[commit-bot: I haz the power, 2017-02-16 08:50:12]

Description was changed from

==========
[WebVTT] Rename Middle to Center

BUG=663797
==========

to

==========
[WebVTT] Rename Middle to Center

BUG=663797

Review-Url: https://codereview.chromium.org/2683633006
Cr-Commit-Position: refs/heads/master@{#450901}
Committed:
https://chromium.googlesource.com/chromium/src/+/c129fcf7b932d95939244304d7e6...
==========

[commit-bot: I haz the power, 2017-02-16 08:50:14]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/c129fcf7b932d95939244304d7e6...