WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED WONTFIX
30369
Fix media layout test file ordering so both branded and unbranded builds of Chromium will pass layout tests.
https://bugs.webkit.org/show_bug.cgi?id=30369
Summary
Fix media layout test file ordering so both branded and unbranded builds of C...
Andrew Scherkus
Reported
2009-10-14 18:42:18 PDT
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.
Attachments
Round 1
(1.12 KB, patch)
2009-10-14 18:45 PDT
,
Andrew Scherkus
no flags
Details
Formatted Diff
Diff
Round 2
(1.24 KB, patch)
2009-10-15 10:27 PDT
,
Andrew Scherkus
no flags
Details
Formatted Diff
Diff
for future reference, GTK+ tests new baseline with current situation
(17.67 KB, patch)
2009-10-16 11:47 PDT
,
Gustavo Noronha (kov)
gustavo
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Andrew Scherkus
Comment 1
2009-10-14 18:45:25 PDT
Created
attachment 41197
[details]
Round 1
Darin Adler
Comment 2
2009-10-15 09:14:51 PDT
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
Comment 3
2009-10-15 10:13:16 PDT
Good idea.. I'll get something uploaded
Andrew Scherkus
Comment 4
2009-10-15 10:27:03 PDT
Created
attachment 41228
[details]
Round 2 Added a comment.
Eric Seidel (no email)
Comment 5
2009-10-15 13:42:07 PDT
Comment on
attachment 41228
[details]
Round 2 LGTM.
WebKit Commit Bot
Comment 6
2009-10-15 14:53:26 PDT
Comment on
attachment 41228
[details]
Round 2 Clearing flags on attachment: 41228 Committed
r49661
: <
http://trac.webkit.org/changeset/49661
>
WebKit Commit Bot
Comment 7
2009-10-15 14:53:29 PDT
All reviewed patches have been landed. Closing bug.
Xan Lopez
Comment 8
2009-10-16 05:06:21 PDT
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)
Comment 9
2009-10-16 06:23:44 PDT
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
Comment 10
2009-10-16 09:49:26 PDT
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
Comment 11
2009-10-16 11:07:20 PDT
This also makes many tests fail for anyone using a QuickTime backed media engine with the Xiph component installed (like me).
Andrew Scherkus
Comment 12
2009-10-16 11:10:40 PDT
Shall we revert, then?
Eric Carlson
Comment 13
2009-10-16 11:37:46 PDT
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)
Comment 14
2009-10-16 11:47:47 PDT
Created
attachment 41302
[details]
for future reference, GTK+ tests new baseline with current situation
Gustavo Noronha (kov)
Comment 15
2009-10-16 11:48:47 PDT
(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)
Comment 16
2009-10-16 11:51:55 PDT
(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
Comment 17
2009-10-16 11:53:42 PDT
(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)
Comment 18
2009-10-16 15:30:16 PDT
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 19
2009-10-19 00:20:04 PDT
Comment on
attachment 41197
[details]
Round 1 Clearing review flag on an obsolete patch.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug