| Summary: | [BlackBerry] Add support for FLAC audio and OGG/Vorbis audio | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Max Feil <mfeil> | ||||||||||||
| Component: | Media | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
| Status: | CLOSED FIXED | ||||||||||||||
| Severity: | Enhancement | CC: | abarth, dglazkov, tonikitoo, webkit.review.bot | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Other | ||||||||||||||
| OS: | Other | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Max Feil
2012-02-24 12:06:19 PST
Created attachment 128808 [details]
Proposed patch for review
Comment on attachment 128808 [details] Proposed patch for review Attachment 128808 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11628196 New failing tests: media/media-can-play-flac-audio.html Ok, obviously I need to add the new layout test to the Skipped file for a number of upstream ports. But how do I figure out which ones? All of them? (except Blackberry) Comment on attachment 128808 [details]
Proposed patch for review
What you could do as well, is add the test in LayoutTests/platform/blackberry/media/media-can-play-flac-audio.html . So it will be a platform specific test. I think we need to upstream our tests machinery first though right?
Created attachment 129090 [details]
Updated patch with new flac layout test skipped where necessary
Comment on attachment 129090 [details]
Updated patch with new flac layout test skipped where necessary
So Max, any port wont skip it other than BlackBerry port?
I added the new layout test to all the "Skipped" files where it made sense. If the Skipped file did not look like it was being maintained, for example if it was very short or blank, I didn't add the test. Here are the Skipped files I am not touching: LayoutTests/platform/gtk-wk2/Skipped LayoutTests/platform/mac-wk2/Skipped LayoutTests/platform/qt-4.8/Skipped LayoutTests/platform/qt-5.0/Skipped LayoutTests/platform/qt-5.0-wk1/Skipped LayoutTests/platform/qt-5.0-wk2/Skipped LayoutTests/platform/qt-arm/Skipped LayoutTests/platform/qt-linux/Skipped LayoutTests/platform/win-wk2/Skipped LayoutTests/platform/win-xp/Skipped LayoutTests/platform/wk2/Skipped I was assuming the WebKit Review Bot would catch any failing tests, like it did in Comment #2. > LayoutTests/platform/qt-4.8/Skipped > LayoutTests/platform/qt-5.0/Skipped > LayoutTests/platform/qt-5.0-wk1/Skipped > LayoutTests/platform/qt-5.0-wk2/Skipped > LayoutTests/platform/qt-arm/Skipped > LayoutTests/platform/qt-linux/Skipped If you want to skip it to Qt generally speaking, you do not need to all it to each different Skip file, but to LayoutTests/platform/qt/Skipped > LayoutTests/platform/win-wk2/Skipped > LayoutTests/platform/wk2/Skipped > LayoutTests/platform/gtk-wk2/Skipped > LayoutTests/platform/mac-wk2/Skipped I believe this one skips the test for all wk2 ports. Any my previous question come again: is any ports supposed to run this test other than BlackBerry? Hi Antonio. My intention is to make the test available to any upstream ports that wish to use it, but not introduce errors in the upstream builds. To me it just makes sense to put the new test in the same place as the other media-can-play tests such as ogg. Also, maybe you misunderstood the list in my previous comment. Those were the files that I didn't touch. The ones I did modify are: LayoutTests/platform/chromium/test_expectations.txt LayoutTests/platform/efl/Skipped LayoutTests/platform/gtk/Skipped LayoutTests/platform/mac-leopard/Skipped LayoutTests/platform/mac-lion/Skipped LayoutTests/platform/mac-snowleopard/Skipped LayoutTests/platform/mac/Skipped LayoutTests/platform/qt-mac/Skipped LayoutTests/platform/qt-win/Skipped LayoutTests/platform/qt/Skipped LayoutTests/platform/win/Skipped LayoutTests/platform/wincairo/Skipped Do you see any that are redundant? The hierarchy of the files is not obvious to me. Are you saying that tests skipped in LayoutTests/platform/mac/Skipped do not need to be separately specified in, for example, LayoutTests/platform/mac-snowleopard/Skipped? If that is the case, there are other redundant tests listed as well, specifically media/media-can-play-ogg.html is listed in both those files. > Are you saying that tests skipped in LayoutTests/platform/mac/Skipped do not need to be separately specified in, for example, LayoutTests/platform/mac-snowleopard/Skipped? If that is the case, there are other redundant tests listed as well, specifically media/media-can-play-ogg.html is listed in both those files.
That is what I am saying, yes :-)
Created attachment 129337 [details]
Updated patch with redundant "Skipped" file changes removed
Attachment 129337 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
LayoutTests/platform/chromium/test_expectations.txt:489: Path does not exist. media/media-can-play-flac-audio.html [test/expectations] [5]
Total errors found: 1 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #12) > Attachment 129337 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 > LayoutTests/platform/chromium/test_expectations.txt:489: Path does not exist. media/media-can-play-flac-audio.html [test/expectations] [5] > Total errors found: 1 in 12 files This seems really dumb. Can't add a test that fails because it's not being skipped. Can't add it to be skipped because the test doesn't exist yet. How do we avoid chicken-egg cases like this? All this automated checking seems like it makes getting any work done at all difficult... Or am I missing something here? > This seems really dumb. Can't add a test that fails because it's not being skipped. Can't add it to be skipped because the test doesn't exist yet. How do we avoid chicken-egg cases like this? All this automated checking seems like it makes getting any work done at all difficult...
Traditionally folks add both the test and the instruction to skip the test at the same time.
Created attachment 129383 [details]
Trying the last patch again - was missing the new test
Thanks Adam. In my GIT fiddling to remove some files I forgot to add back in the new files.
Created attachment 129384 [details]
Trying the last patch again - was missing the new test
Comment on attachment 129384 [details]
Trying the last patch again - was missing the new test
And my question comes again: what upstream port runs this test you are adding?
(In reply to comment #17) > (From update of attachment 129384 [details]) > And my question comes again: what upstream port runs this test you are adding? My intention is to make the test available to upstream ports and let them turn it on as desired. So Blackberry is the only port currently intended to run the test, even though it is potentially useful for other ports. please keep one eye at the bot, Max. Thanks Comment on attachment 129384 [details] Trying the last patch again - was missing the new test Clearing flags on attachment: 129384 Committed r109238: <http://trac.webkit.org/changeset/109238> All reviewed patches have been landed. Closing bug. Closing bug for patch that landed a long time ago. |