Remove unneeded TODOs from media-fragment tests

Remove unneeded TODOs from media-fragment tests

media-fragment tests are not in a state to be upstreamed
to wpt. So removing the unneeded TODO's for now.

BUG=588956
Committed: https://crrev.com/5ec2d0a03a7de5fdd47874baabcc2d2887af94bc
Cr-Commit-Position: refs/heads/master@{#403200}

[Srirama, 2016-06-28 10:39:37]

Patchset #1 (id:1) has been deleted

[Srirama, 2016-06-28 10:41:03]

Description was changed from

==========
mediaframgent


fix

BUG=
==========

to

==========
Convert media-fragment tests to testharness.js

Cleaning up media-fragment tests in media/
to use testharness.js instead of video-test.js.
This will enable to upstream these tests to web-platform-tests.

BUG=588956
==========

[Srirama, 2016-06-28 11:00:45]

PTAL.
If this is fine, i will go ahead and modify all the test cases and add it in my
next PS.

https://codereview.chromium.org/2103003002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/media-fragments/media-fragments.js
(right):

https://codereview.chromium.org/2103003002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/media-fragments/media-fragments.js:109:
currentTest = fileName.substring(0, fileName.lastIndexOf("."));
Should i pass the testcase name instead?

[Srirama, 2016-06-28 11:00:57]

srirama.m@samsung.com changed reviewers:
+ foolip@chromium.org, fs@opera.com

[Srirama, 2016-06-28 11:00:57]

PTAL.
If this is fine, i will go ahead and modify all the test cases and add it in my
next PS.

[fs, 2016-06-28 11:28:35]

On 2016/06/28 at 11:00:57, srirama.m wrote:
> PTAL.
> If this is fine, i will go ahead and modify all the test cases and add it in
my next PS.

I'm not a big fan of the original structure (and hence not the suggested one
either). I'd much rather this was generating a number of tests from the table
and ran those, rather than have 94 identical "drivers" and magic filenames.
Since these all involve video, I guess there might be a risk that we run into
random resource limits. So maybe another way to do it would be to split into a
few categories based on what is actually tested - invalid syntax ("valid:
false") seems like a good place to start (maybe media-fragments-invalid.html, or
just invalid.html or so). Then you can remove tests from testData as you go
(maybe adding test helpers if deemed useful.)

I'd like to hear what foolip thinks too.

[Srirama, 2016-06-30 13:39:36]

On 2016/06/28 11:28:35, fs wrote:
> On 2016/06/28 at 11:00:57, srirama.m wrote:
> > PTAL.
> > If this is fine, i will go ahead and modify all the test cases and add it in
> my next PS.
> 
> I'm not a big fan of the original structure (and hence not the suggested one
> either). I'd much rather this was generating a number of tests from the table
> and ran those, rather than have 94 identical "drivers" and magic filenames.
> Since these all involve video, I guess there might be a risk that we run into
> random resource limits. So maybe another way to do it would be to split into a
> few categories based on what is actually tested - invalid syntax ("valid:
> false") seems like a good place to start (maybe media-fragments-invalid.html,
or
> just invalid.html or so). Then you can remove tests from testData as you go
> (maybe adding test helpers if deemed useful.)
> 
> I'd like to hear what foolip thinks too.

@foolip, would you please give your thought on this?

[foolip, 2016-06-30 14:16:09]

On 2016/06/30 13:39:36, Srirama wrote:
> On 2016/06/28 11:28:35, fs wrote:
> > On 2016/06/28 at 11:00:57, srirama.m wrote:
> > > PTAL.
> > > If this is fine, i will go ahead and modify all the test cases and add it
in
> > my next PS.
> > 
> > I'm not a big fan of the original structure (and hence not the suggested one
> > either). I'd much rather this was generating a number of tests from the
table
> > and ran those, rather than have 94 identical "drivers" and magic filenames.
> > Since these all involve video, I guess there might be a risk that we run
into
> > random resource limits. So maybe another way to do it would be to split into
a
> > few categories based on what is actually tested - invalid syntax ("valid:
> > false") seems like a good place to start (maybe
media-fragments-invalid.html,
> or
> > just invalid.html or so). Then you can remove tests from testData as you go
> > (maybe adding test helpers if deemed useful.)
> > 
> > I'd like to hear what foolip thinks too.
> 
> @foolip, would you please give your thought on this?

These are from https://www.w3.org/2008/WebVideo/Fragments/TC/ua-test-cases so
there's some value in keeping the magic filenames.

I think overall it might be a little bit less ugly if the test data ({ start:
null, end: null, valid: false, description: "#t=,", fragment: "t=,", comment:
"Syntax error, not allowed according to the ABNF. The media fragment is
ignored."}) were passed into the common code from each individual test file
instead.

But... I don't think it's worth spending the time on these tests. Only a small
part of Media Fragments were implemented, and handling of fragment end time
never ended up in the HTML spec:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24910

So, I'm thinking that the whole parsing bit could be redefined in terms of
https://url.spec.whatwg.org/#interface-urlsearchparams and the end time handling
dropped. But that would require writing new tests, so I'd just leave these ones
alone, perhaps forever.

[Srirama, 2016-06-30 14:33:05]

On 2016/06/30 14:16:09, Philip Jägenstedt wrote:
> On 2016/06/30 13:39:36, Srirama wrote:
> > On 2016/06/28 11:28:35, fs wrote:
> > > On 2016/06/28 at 11:00:57, srirama.m wrote:
> > > > PTAL.
> > > > If this is fine, i will go ahead and modify all the test cases and add
it
> in
> > > my next PS.
> > > 
> > > I'm not a big fan of the original structure (and hence not the suggested
one
> > > either). I'd much rather this was generating a number of tests from the
> table
> > > and ran those, rather than have 94 identical "drivers" and magic
filenames.
> > > Since these all involve video, I guess there might be a risk that we run
> into
> > > random resource limits. So maybe another way to do it would be to split
into
> a
> > > few categories based on what is actually tested - invalid syntax ("valid:
> > > false") seems like a good place to start (maybe
> media-fragments-invalid.html,
> > or
> > > just invalid.html or so). Then you can remove tests from testData as you
go
> > > (maybe adding test helpers if deemed useful.)
> > > 
> > > I'd like to hear what foolip thinks too.
> > 
> > @foolip, would you please give your thought on this?
> 
> These are from https://www.w3.org/2008/WebVideo/Fragments/TC/ua-test-cases so
> there's some value in keeping the magic filenames.
> 
> I think overall it might be a little bit less ugly if the test data ({ start:
> null, end: null, valid: false, description: "#t=,", fragment: "t=,", comment:
> "Syntax error, not allowed according to the ABNF. The media fragment is
> ignored."}) were passed into the common code from each individual test file
> instead.
> 
> But... I don't think it's worth spending the time on these tests. Only a small
> part of Media Fragments were implemented, and handling of fragment end time
> never ended up in the HTML spec:
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=24910
> 
> So, I'm thinking that the whole parsing bit could be redefined in terms of
> https://url.spec.whatwg.org/#interface-urlsearchparams and the end time
handling
> dropped. But that would require writing new tests, so I'd just leave these
ones
> alone, perhaps forever.

So shall i go ahead and change all the test cases the same way i did
TC0001.html, without disturbing the test data?

[foolip, 2016-06-30 14:41:53]

On 2016/06/30 14:33:05, Srirama wrote:
> On 2016/06/30 14:16:09, Philip Jägenstedt wrote:
> > On 2016/06/30 13:39:36, Srirama wrote:
> > > On 2016/06/28 11:28:35, fs wrote:
> > > > On 2016/06/28 at 11:00:57, srirama.m wrote:
> > > > > PTAL.
> > > > > If this is fine, i will go ahead and modify all the test cases and add
> it
> > in
> > > > my next PS.
> > > > 
> > > > I'm not a big fan of the original structure (and hence not the suggested
> one
> > > > either). I'd much rather this was generating a number of tests from the
> > table
> > > > and ran those, rather than have 94 identical "drivers" and magic
> filenames.
> > > > Since these all involve video, I guess there might be a risk that we run
> > into
> > > > random resource limits. So maybe another way to do it would be to split
> into
> > a
> > > > few categories based on what is actually tested - invalid syntax
("valid:
> > > > false") seems like a good place to start (maybe
> > media-fragments-invalid.html,
> > > or
> > > > just invalid.html or so). Then you can remove tests from testData as you
> go
> > > > (maybe adding test helpers if deemed useful.)
> > > > 
> > > > I'd like to hear what foolip thinks too.
> > > 
> > > @foolip, would you please give your thought on this?
> > 
> > These are from https://www.w3.org/2008/WebVideo/Fragments/TC/ua-test-cases
so
> > there's some value in keeping the magic filenames.
> > 
> > I think overall it might be a little bit less ugly if the test data ({
start:
> > null, end: null, valid: false, description: "#t=,", fragment: "t=,",
comment:
> > "Syntax error, not allowed according to the ABNF. The media fragment is
> > ignored."}) were passed into the common code from each individual test file
> > instead.
> > 
> > But... I don't think it's worth spending the time on these tests. Only a
small
> > part of Media Fragments were implemented, and handling of fragment end time
> > never ended up in the HTML spec:
> > https://www.w3.org/Bugs/Public/show_bug.cgi?id=24910
> > 
> > So, I'm thinking that the whole parsing bit could be redefined in terms of
> > https://url.spec.whatwg.org/#interface-urlsearchparams and the end time
> handling
> > dropped. But that would require writing new tests, so I'd just leave these
> ones
> > alone, perhaps forever.
> 
> So shall i go ahead and change all the test cases the same way i did
> TC0001.html, without disturbing the test data?

Well, I guess I don't see much value in changing these tests at all, since they
can't be upstreamed in this state. I'd just leave them alone, but you could
remove the TODOs.

[Srirama, 2016-06-30 14:57:51]

Description was changed from

==========
Convert media-fragment tests to testharness.js

Cleaning up media-fragment tests in media/
to use testharness.js instead of video-test.js.
This will enable to upstream these tests to web-platform-tests.

BUG=588956
==========

to

==========
Remove unneeded TODOs from media-fragment tests

media-fragment tests are not in a state to be upstreamed
to wpt. So removing the unneeded TODO's for now.

BUG=588956
==========

[Srirama, 2016-06-30 15:01:53]

Removed TODO's as discussed. 68 TODO's vanished at once and my load reduced :)

[foolip, 2016-06-30 15:45:16]

The CQ bit was checked by foolip@chromium.org

[foolip, 2016-06-30 15:45:17]

lgtm

[commit-bot: I haz the power, 2016-06-30 15:45:41]

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, 2016-06-30 16:54:48]

Description was changed from

==========
Remove unneeded TODOs from media-fragment tests

media-fragment tests are not in a state to be upstreamed
to wpt. So removing the unneeded TODO's for now.

BUG=588956
==========

to

==========
Remove unneeded TODOs from media-fragment tests

media-fragment tests are not in a state to be upstreamed
to wpt. So removing the unneeded TODO's for now.

BUG=588956
==========

[commit-bot: I haz the power, 2016-06-30 16:54:49]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-06-30 16:54:53]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 16:56:16]

Description was changed from

==========
Remove unneeded TODOs from media-fragment tests

media-fragment tests are not in a state to be upstreamed
to wpt. So removing the unneeded TODO's for now.

BUG=588956
==========

to

==========
Remove unneeded TODOs from media-fragment tests

media-fragment tests are not in a state to be upstreamed
to wpt. So removing the unneeded TODO's for now.

BUG=588956

Committed: https://crrev.com/5ec2d0a03a7de5fdd47874baabcc2d2887af94bc
Cr-Commit-Position: refs/heads/master@{#403200}
==========

[commit-bot: I haz the power, 2016-06-30 16:56:18]

Patchset 2 (id:??) landed as
https://crrev.com/5ec2d0a03a7de5fdd47874baabcc2d2887af94bc
Cr-Commit-Position: refs/heads/master@{#403200}