| Summary: | MediaPlayer should try all installed media engines | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||||
| Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | buildbot, dglazkov, eric, webkit-ews, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Eric Carlson
2010-11-29 22:35:58 PST
Created attachment 75233 [details]
Proposed patch
Created attachment 75913 [details]
Updated patch
Updated to behave correctly when retrying when there is no MIME type.
Attachment 75913 [details] did not build on mac: Build output: http://queues.webkit.org/results/6754115 Attachment 75913 [details] did not build on qt: Build output: http://queues.webkit.org/results/6719133 Attachment 75913 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6862077 Attachment 75913 [details] did not build on win: Build output: http://queues.webkit.org/results/6720123 Attachment 75913 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6759127 Created attachment 76064 [details]
Patch that might compile
Comment on attachment 76064 [details] Patch that might compile View in context: https://bugs.webkit.org/attachment.cgi?id=76064&action=review > WebCore/platform/graphics/MediaPlayer.cpp:233 > unsigned count = engines.size(); > for (unsigned ndx = 0; ndx < count; ndx++) { (see comment below) > WebCore/platform/graphics/MediaPlayer.cpp:257 > + Vector<MediaPlayerFactory*>& engines = installedMediaEngines(); > + if (engines.isEmpty()) > + return 0; > + > + MediaPlayerFactory* engine = 0; > + unsigned count = engines.size(); > + for (unsigned ndx = 0; ndx < count; ndx++) { I recently learned that Vector::size() is size_t, and on 64bit systems may be 64bits, but unsigned is always 32bits. Although it probably isn't an issue here (count and ndx), its something to keep in mind for future code. Comment on attachment 76064 [details] Patch that might compile View in context: https://bugs.webkit.org/attachment.cgi?id=76064&action=review > WebCore/platform/graphics/MediaPlayer.cpp:253 > + if (engines.isEmpty()) > + return 0; No need for a special case. The normal code path handles an empty vector just fine. >> WebCore/platform/graphics/MediaPlayer.cpp:257 >> + for (unsigned ndx = 0; ndx < count; ndx++) { > > I recently learned that Vector::size() is size_t, and on 64bit systems may be 64bits, > but unsigned is always 32bits. Although it probably isn't an issue here (count and > ndx), its something to keep in mind for future code. Yes, better style to use size_t here. Also, we normally prefer “i” over “ndx”. And I would call it size rather than count, just to make it clearer that it just reflects the size function result. > WebCore/platform/graphics/MediaPlayer.cpp:264 > + if (current) { > + if (current == engines[ndx]) > + current = 0; > + continue; > + } > + engine = engines[ndx]; > + break; The logic here is a little convoluted. You could write this more clearly by just saying “if (!current) return first" and for the non-zero case using the find function that vectors have without writing out a loop. |