[webkit-reviews] review granted: [Bug 74443] Media url with fragment may not load : [Attachment 119084] Proposed patch.

[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 74443: Media url with fragment may not load
https://bugs.webkit.org/show_bug.cgi?id=74443

Attachment 119084: Proposed patch.
https://bugs.webkit.org/attachment.cgi?id=119084&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=119084&action=review


> Source/WebCore/platform/graphics/MediaPlayer.cpp:341
> +	   if (protocolIs(urlString, "data"))

Would be better to call url.protocolIsData() since we have the URL.

> Source/WebCore/platform/graphics/MediaPlayer.cpp:347
> -	       size_t pos = url.reverseFind('.');
> +	       String lastPathComponent = url.lastPathComponent();
> +	       size_t pos = lastPathComponent.reverseFind('.');
>	       if (pos != notFound) {
> -		   String extension = url.substring(pos + 1);
> +		   String extension = lastPathComponent.substring(pos + 1);

Is this the only place in our code where we get the extension from a URL? If
not, maybe they can share more code.

> Source/WebCore/platform/graphics/MediaPlayer.h:37
> +#include "KURL.h"

This include is not needed. All the header needs is a forward declaration of
the KURL class.

>> Source/WebCore/platform/graphics/MediaPlayer.h:211
>> -	bool load(const String& url, const ContentType&);
>> +	bool load(const KURL& url, const ContentType&);
> 
> The parameter name "url" adds no information, so it should be removed. 
[readability/parameter_name] [5]

I agree with the style bot.