Enable WebVTT regions for runtime testing, updated tests and minor fixes

Enable WebVTT regions for runtime testing, updated tests and minor fixes

This patch removes all the ifdefs surrounding WebVTT regions and enables
the feature for testing in RuntimeEnabledFeatures, along with a couple
of bug fixes.

Tests were rebased wrongly (since the feature wasn't build by default),
so I updated the -expected and added two others that refer to the display
of regions and update TestExpectations to not skip these anymore.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159784

[vcarbune.chromium, 2013-10-04 10:03:31]

Adding WebVTT Regions to status=test

[acolwell GONE FROM CHROMIUM, 2013-10-04 19:48:20]

looks pretty good. In general, I'd like to minimize the number of runtime flag
checks. In general, I'd like us to have a "protect our borders" approach where
we make guarantees based on the flag at the "borders" where we receive data from
the user. If we do this right, then we don't need the extra checks sprinkled
around the code.

https://codereview.chromium.org/25798003/diff/3001/Source/core/html/shadow/Me...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/25798003/diff/3001/Source/core/html/shadow/Me...
Source/core/html/shadow/MediaControlElements.cpp:745: if
(!RuntimeEnabledFeatures::webVTTRegionsEnabled() || !region) {
nit: Isn't the !RuntimeEnabledFeatures::webVTTRegionsEnabled() check redundant
here? If it isn't set !region will always be true.

https://codereview.chromium.org/25798003/diff/3001/Source/core/html/track/Tex...
File Source/core/html/track/TextTrack.cpp (right):

https://codereview.chromium.org/25798003/diff/3001/Source/core/html/track/Tex...
Source/core/html/track/TextTrack.cpp:125: if
(RuntimeEnabledFeatures::webVTTRegionsEnabled() && m_regions) {
nit: Why is this covered by the feature guard. Will regions ever have a length >
0 if the flag isn't enabled? If so then why is running this loop bad in that
case?

https://codereview.chromium.org/25798003/diff/3001/Source/core/html/track/Tex...
File Source/core/html/track/TextTrackCue.cpp (right):

https://codereview.chromium.org/25798003/diff/3001/Source/core/html/track/Tex...
Source/core/html/track/TextTrackCue.cpp:110: if
(RuntimeEnabledFeatures::webVTTRegionsEnabled() && !m_cue->regionId().isEmpty())
{
nit: Can the regionId be non-empty if this flag is not set? If not, then drop
the flag check.

https://codereview.chromium.org/25798003/diff/3001/Source/core/html/track/Tex...
Source/core/html/track/TextTrackCue.cpp:834: if
(RuntimeEnabledFeatures::webVTTRegionsEnabled() && m_notifyRegion) {
nit: Do you really need the flag check here? What is it protecting?

https://codereview.chromium.org/25798003/diff/3001/Source/core/html/track/Tex...
Source/core/html/track/TextTrackCue.cpp:1097: if
(RuntimeEnabledFeatures::webVTTRegionsEnabled()) {
nit: The check in settingName() should make this check unnecessary and prevent
this code getting executed right? If so please remove this flag check.

https://codereview.chromium.org/25798003/diff/3001/Source/core/html/track/Tex...
Source/core/html/track/TextTrackCue.cpp:1109: if
(RuntimeEnabledFeatures::webVTTRegionsEnabled()) {
nit: ditto. If m_regionId can only become non-empty when this flag is enabled,
then I think the flag check should be removed.

https://codereview.chromium.org/25798003/diff/3001/Source/core/html/track/Web...
File Source/core/html/track/WebVTTParser.cpp (right):

https://codereview.chromium.org/25798003/diff/3001/Source/core/html/track/Web...
Source/core/html/track/WebVTTParser.cpp:165: if
(RuntimeEnabledFeatures::webVTTRegionsEnabled() && m_client &&
m_regionList.size())
nit: Can m_regionList.size() be > 0 if the flag is not set? If not then please
remove flag check.

https://codereview.chromium.org/25798003/diff/3001/Source/core/html/track/Web...
Source/core/html/track/WebVTTParser.cpp:171: if
(RuntimeEnabledFeatures::webVTTRegionsEnabled())
nit: Do you really need a flag check here? It seems like you should transition
to Metadata instead of Header if the flag isn't set instead of doing multiple
checks here. In fact I don't see any code that causes m_state transition to
Metadata.

https://codereview.chromium.org/25798003/diff/3001/Source/core/page/RuntimeEn...
File Source/core/page/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/25798003/diff/3001/Source/core/page/RuntimeEn...
Source/core/page/RuntimeEnabledFeatures.in:107: WebVTTRegions status=test
nit: I think you want =experimental here don't you?

https://codereview.chromium.org/25798003/diff/3001/Source/core/rendering/Rend...
File Source/core/rendering/RenderTextTrackCue.cpp (right):

https://codereview.chromium.org/25798003/diff/3001/Source/core/rendering/Rend...
Source/core/rendering/RenderTextTrackCue.cpp:50: if
(RuntimeEnabledFeatures::webVTTRegionsEnabled() && !m_cue->regionId().isEmpty())
nit: If regionId's can't be non-empty w/o the flag set, please remove the flag
check.

[philipj_slow, 2013-10-07 09:09:00]

I'm happy to see this behind a flag. I was rather skeptical of this feature when
Silvia spec'd it, but was absent (paternity leave) from the discussion when the
implementation actually happened. Are others also implementing this? Is it
considered complete or does it need a full spec + implementation review?

[nessy, 2013-10-07 12:43:37]

On 2013/10/07 09:09:00, philipj wrote:
> I'm happy to see this behind a flag. I was rather skeptical of this feature
when
> Silvia spec'd it, but was absent (paternity leave) from the discussion when
the
> implementation actually happened. Are others also implementing this? Is it
> considered complete or does it need a full spec + implementation review?

Spec for this feature is finished, but it can of course always do with a review!

Anyone who wants full CEA708 support will want to implement this, but I haven't
heard much activity from other browsers. I guess right now we're focusing on
fixing the basic functionality, so the implementation focus will be there, too.

[vcarbune.chromium, 2013-10-08 18:07:38]

https://codereview.chromium.org/25798003/diff/3001/Source/core/html/shadow/Me...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/25798003/diff/3001/Source/core/html/shadow/Me...
Source/core/html/shadow/MediaControlElements.cpp:745: if
(!RuntimeEnabledFeatures::webVTTRegionsEnabled() || !region) {
On 2013/10/04 19:48:20, acolwell wrote:
> nit: Isn't the !RuntimeEnabledFeatures::webVTTRegionsEnabled() check redundant
> here? If it isn't set !region will always be true.

Correct, I removed it from here and the line above, but added it in an the upper
level of TextTrack::regions().

https://codereview.chromium.org/25798003/diff/3001/Source/core/html/track/Tex...
File Source/core/html/track/TextTrack.cpp (right):

https://codereview.chromium.org/25798003/diff/3001/Source/core/html/track/Tex...
Source/core/html/track/TextTrack.cpp:125: if
(RuntimeEnabledFeatures::webVTTRegionsEnabled() && m_regions) {
On 2013/10/04 19:48:20, acolwell wrote:
> nit: Why is this covered by the feature guard. Will regions ever have a length
>
> 0 if the flag isn't enabled? If so then why is running this loop bad in that
> case?

Removed the runtime flag check. It should be zero if it isn't enabled.

https://codereview.chromium.org/25798003/diff/3001/Source/core/html/track/Tex...
File Source/core/html/track/TextTrackCue.cpp (right):

https://codereview.chromium.org/25798003/diff/3001/Source/core/html/track/Tex...
Source/core/html/track/TextTrackCue.cpp:110: if
(RuntimeEnabledFeatures::webVTTRegionsEnabled() && !m_cue->regionId().isEmpty())
{
On 2013/10/04 19:48:20, acolwell wrote:
> nit: Can the regionId be non-empty if this flag is not set? If not, then drop
> the flag check.

Done.

https://codereview.chromium.org/25798003/diff/3001/Source/core/html/track/Tex...
Source/core/html/track/TextTrackCue.cpp:834: if
(RuntimeEnabledFeatures::webVTTRegionsEnabled() && m_notifyRegion) {
On 2013/10/04 19:48:20, acolwell wrote:
> nit: Do you really need the flag check here? What is it protecting?

Removed, added the proper check that track->regions() isn't NULL.

https://codereview.chromium.org/25798003/diff/3001/Source/core/html/track/Tex...
Source/core/html/track/TextTrackCue.cpp:1097: if
(RuntimeEnabledFeatures::webVTTRegionsEnabled()) {
On 2013/10/04 19:48:20, acolwell wrote:
> nit: The check in settingName() should make this check unnecessary and prevent
> this code getting executed right? If so please remove this flag check.

Done.

https://codereview.chromium.org/25798003/diff/3001/Source/core/html/track/Tex...
Source/core/html/track/TextTrackCue.cpp:1109: if
(RuntimeEnabledFeatures::webVTTRegionsEnabled()) {
On 2013/10/04 19:48:20, acolwell wrote:
> nit: ditto. If m_regionId can only become non-empty when this flag is enabled,
> then I think the flag check should be removed.

Done.

https://codereview.chromium.org/25798003/diff/3001/Source/core/html/track/Web...
File Source/core/html/track/WebVTTParser.cpp (right):

https://codereview.chromium.org/25798003/diff/3001/Source/core/html/track/Web...
Source/core/html/track/WebVTTParser.cpp:165: if
(RuntimeEnabledFeatures::webVTTRegionsEnabled() && m_client &&
m_regionList.size())
On 2013/10/04 19:48:20, acolwell wrote:
> nit: Can m_regionList.size() be > 0 if the flag is not set? If not then please
> remove flag check.

Done.

https://codereview.chromium.org/25798003/diff/3001/Source/core/html/track/Web...
Source/core/html/track/WebVTTParser.cpp:171: if
(RuntimeEnabledFeatures::webVTTRegionsEnabled())
On 2013/10/04 19:48:20, acolwell wrote:
> nit: Do you really need a flag check here? It seems like you should transition
> to Metadata instead of Header if the flag isn't set instead of doing multiple
> checks here. In fact I don't see any code that causes m_state transition to
> Metadata.

I moved the flag check and inside the function and renamed it to
collectMetadataHeader to match the spec ("Metadata header loop"). I removed the
Metadata state, because it's not needed.

On the other hand, I think these steps need a spec re-sync (filed
http://crbug.com/305317)

https://codereview.chromium.org/25798003/diff/3001/Source/core/page/RuntimeEn...
File Source/core/page/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/25798003/diff/3001/Source/core/page/RuntimeEn...
Source/core/page/RuntimeEnabledFeatures.in:107: WebVTTRegions status=test
On 2013/10/04 19:48:20, acolwell wrote:
> nit: I think you want =experimental here don't you?

Yes.

https://codereview.chromium.org/25798003/diff/3001/Source/core/rendering/Rend...
File Source/core/rendering/RenderTextTrackCue.cpp (right):

https://codereview.chromium.org/25798003/diff/3001/Source/core/rendering/Rend...
Source/core/rendering/RenderTextTrackCue.cpp:50: if
(RuntimeEnabledFeatures::webVTTRegionsEnabled() && !m_cue->regionId().isEmpty())
On 2013/10/04 19:48:20, acolwell wrote:
> nit: If regionId's can't be non-empty w/o the flag set, please remove the flag
> check.

Done.

[vcarbune.chromium, 2013-10-08 18:11:09]

On 2013/10/07 09:09:00, philipj wrote:
> I'm happy to see this behind a flag. I was rather skeptical of this feature
when
> Silvia spec'd it, but was absent (paternity leave) from the discussion when
the
> implementation actually happened. Are others also implementing this? Is it
> considered complete or does it need a full spec + implementation review?

It is not yet complete in terms of implementation. CSS support is not there and
there are some other things needing to be fixed. Besides, current code was
written when regions weren't integrated in the main WebVTT specification (so
maybe some things changed).

So there's still work to be done, but the first step to make this Blink-friendly
is this patch to get rid of #ifdefs.

[acolwell GONE FROM CHROMIUM, 2013-10-08 22:09:05]

lgtm % nits

https://codereview.chromium.org/25798003/diff/29001/Source/core/html/track/Lo...
File Source/core/html/track/LoadableTextTrack.cpp (right):

https://codereview.chromium.org/25798003/diff/29001/Source/core/html/track/Lo...
Source/core/html/track/LoadableTextTrack.cpp:29: #include
"RuntimeEnabledFeatures.h"
nit: Not needed.

https://codereview.chromium.org/25798003/diff/29001/Source/core/rendering/Ren...
File Source/core/rendering/RenderTextTrackCue.cpp (right):

https://codereview.chromium.org/25798003/diff/29001/Source/core/rendering/Ren...
Source/core/rendering/RenderTextTrackCue.cpp:29: #include
"RuntimeEnabledFeatures.h"
nit: not needed anymore

[vcarbune.chromium, 2013-10-12 22:33:54]

Thank you, Aaron, for the review!

Pending OWNER review now, Adam, maybe you can take a look?

https://codereview.chromium.org/25798003/diff/29001/Source/core/html/track/Lo...
File Source/core/html/track/LoadableTextTrack.cpp (right):

https://codereview.chromium.org/25798003/diff/29001/Source/core/html/track/Lo...
Source/core/html/track/LoadableTextTrack.cpp:29: #include
"RuntimeEnabledFeatures.h"
On 2013/10/08 22:09:06, acolwell wrote:
> nit: Not needed.

Done.

https://codereview.chromium.org/25798003/diff/29001/Source/core/rendering/Ren...
File Source/core/rendering/RenderTextTrackCue.cpp (right):

https://codereview.chromium.org/25798003/diff/29001/Source/core/rendering/Ren...
Source/core/rendering/RenderTextTrackCue.cpp:29: #include
"RuntimeEnabledFeatures.h"
On 2013/10/08 22:09:06, acolwell wrote:
> nit: not needed anymore

Done.

[adamk, 2013-10-14 17:08:01]

I'm not in the RuntimeEnabledFeatures OWNERS, adding eseidel

[eseidel1, 2013-10-14 17:11:52]

Lgtm on the REF changes.

[commit-bot: I haz the power, 2013-10-14 17:14:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vcarbune@chromium.org/25798003/45001

[commit-bot: I haz the power, 2013-10-14 17:31:43]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[vcarbune.chromium, 2013-10-14 21:45:39]

On 2013/10/14 17:11:52, eseidel1 wrote:
> Lgtm on the REF changes.

I seem to still be getting missing OWNER approval, is it because of LGTM-ing
with your @google instead of @chromium?

[eseidel, 2013-10-15 22:20:31]

lgtm on REF changes.  I suspect it may also have been a CQ outage from last
week.

[commit-bot: I haz the power, 2013-10-16 11:58:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vcarbune@chromium.org/25798003/57001

[commit-bot: I haz the power, 2013-10-16 16:19:06]

Change committed as 159784