Bug 30369 – Fix media layout test file ordering so both branded and unbranded builds of Chromium will pass layout tests.

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[Andrew Scherkus]

Since Google Chrome also supports mp4, etc... it was failing layout tests because our baselines assuming ogg content is used.

Quick fix, regular webkit on mac still passes layout tests.

[Andrew Scherkus]

Created attachment 41197 [details]
Round 1

[Darin Adler]

Comment on attachment 41197 [details]
Round 1

Seems fine. I think that ideally the file should contain a comment explaining that the Ogg format comes first for the convenience of people working on Chromium. Otherwise, it seems too subtle to me.

[Andrew Scherkus]

Good idea.. I'll get something uploaded

[Andrew Scherkus]

Created attachment 41228 [details]
Round 2

Added a comment.

[Eric Seidel (no email)]

Comment on attachment 41228 [details]
Round 2

LGTM.

[WebKit Commit Bot]

Comment on attachment 41228 [details]
Round 2

Clearing flags on attachment: 41228

Committed r49661: <http://trac.webkit.org/changeset/49661>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Xan Lopez]

This commit breaks a lot of tests in the GTK+ port. I'm not too familiar with this, but I think that since we support both ogg and mp4 we are exiting this loop :

    for (var i = 0; i < codecs.length; ++i) {
        if (element.canPlayType(codecs[i][0]))
            return name + "." + codecs[i][1];
    }

in findMediaFile on the first try (which is now 'video/ogg'), which makes us fail the test since 'video/mp4' is expected.

Any solution for this? We are struggling to make our bots green, but if we find 20 more tests failing each day when we wake up I don't see any light at the end of the tunnel ;)

[Gustavo Noronha (kov)]

This is the second time this happens - the first one was when the ogg/mp4 selection first got added. We fixed all the breakage, then. Now, I am OK with rebaselining the tests again, since I reckon it is best for us to test ogv than mp4, but it would be good to reach some kind of agreement that our media tests (which took quite some time to get working in a stable way) will be considered for future changes.

[Andrew Scherkus]

Apologies -- I was afraid this change might cause breakage I wasn't aware about.

Xan: unfortunately the fix would be rebaselining tests to use ogg, if possible

Gustavo: this should be the last of the ogg/mp4 related changes, but moving forward I'll try to run the GTK+ media layout tests.

If it's too much of a hassle I'm fine reverting.

[Eric Carlson]

This also makes many tests fail for anyone using a QuickTime backed media engine with the Xiph component installed (like me).

[Andrew Scherkus]

Shall we revert, then?

[Eric Carlson]

Yes, I think reverting is a good idea for now. 

Maybe the right way to fix the problem is to add a layoutTestController function that checks a user preference so findMediaFile can change the default media type?

[Gustavo Noronha (kov)]

Created attachment 41302 [details]
for future reference, GTK+ tests new baseline with current situation

[Gustavo Noronha (kov)]

(In reply to comment #13)
> Yes, I think reverting is a good idea for now. 
> 
> Maybe the right way to fix the problem is to add a layoutTestController
> function that checks a user preference so findMediaFile can change the default
> media type?

This makes a lot of sense to me, since that makes it depend less on the environment.

[Gustavo Noronha (kov)]

(In reply to comment #10)
> Gustavo: this should be the last of the ogg/mp4 related changes, but moving
> forward I'll try to run the GTK+ media layout tests.

No problem if you can't run them, it's just important that we keep an eye on them failing in the GTK+ bot. We're striving for a while now on keeping the bot green, but we would like people to keep an eye on increased number of failures, and see if they are related to their changes, and at least report bugs or skip tests where they think it makes sense.

+1 for reverting for now, and getting a configuration-based solution implemented instead.

[Andrew Scherkus]

(In reply to comment #16)
> (In reply to comment #10)
> > Gustavo: this should be the last of the ogg/mp4 related changes, but moving
> > forward I'll try to run the GTK+ media layout tests.
> 
> No problem if you can't run them, it's just important that we keep an eye on
> them failing in the GTK+ bot. We're striving for a while now on keeping the bot
> green, but we would like people to keep an eye on increased number of failures,
> and see if they are related to their changes, and at least report bugs or skip
> tests where they think it makes sense.
> 
> +1 for reverting for now, and getting a configuration-based solution
> implemented instead.

Yeah let's revert this and I'll come up with something using layoutTestController.

Gustavo, I'll make sure to loop you in on anything I come up with.

[Gustavo Noronha (kov)]

I reverted this in r49704. Reopening the bug.

(In reply to comment #17)
> Yeah let's revert this and I'll come up with something using
> layoutTestController.
> 
> Gustavo, I'll make sure to loop you in on anything I come up with.

Thank you!

[Mark Rowe (bdash)]

Comment on attachment 41197 [details]
Round 1

Clearing review flag on an obsolete patch.