[webkit-reviews] review granted: [Bug 50209] MediaPlayer should try all installed media engines : [Attachment 76064] Patch that might compile

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 50209: MediaPlayer should try all installed media engines
https://bugs.webkit.org/show_bug.cgi?id=50209

Attachment 76064: Patch that might compile
https://bugs.webkit.org/attachment.cgi?id=76064&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.