[webkit-reviews] review granted: [Bug 132568] [Mac, iOS] REGRESSION: Improve YouTube compatibility in WK2 : [Attachment 231009] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed May 7 12:51:31 PDT 2014


Eric Carlson <eric.carlson at apple.com> has granted Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 132568: [Mac, iOS] REGRESSION: Improve YouTube compatibility in WK2
https://bugs.webkit.org/show_bug.cgi?id=132568

Attachment 231009: Patch
https://bugs.webkit.org/attachment.cgi?id=231009&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=231009&action=review


> Source/WebCore/Modules/plugins/YouTubePluginReplacement.cpp:65
> +    for (size_t i = 0; i < paramNames.size(); ++i)

This could use a modern for loop instead.

> Source/WebCore/Modules/plugins/YouTubePluginReplacement.cpp:73
> +    if (!m_videoElement)

Something like m_embedShadowElement would be a better name since it isn't a
video element.

> Source/WebCore/Modules/plugins/YouTubePluginReplacement.cpp:137
> +	   unsigned indexAfterEqual = equalLocation + 1;

why unsigned for indexAfterEqual and size_t for equalSearchLocation?

> Source/WebCore/Modules/plugins/YouTubePluginReplacement.cpp:143
> +	   unsigned keyLocation = equalSearchLocation;
> +	   unsigned keyLength = equalLocation - equalSearchLocation;

Ditto.

> Source/WebCore/Modules/plugins/YouTubePluginReplacement.cpp:164
> +	   // At the end.

Nit: this comment doesn't add much.

> Source/WebCore/Modules/plugins/YouTubePluginReplacement.cpp:179
> +    return input.startsWith(prefix, true);

The bool parameter to startsWith is for case-sensitivity, so this *is* doing a
case sensitive search. This or the function name is wrong.

> Source/WebCore/Modules/plugins/YouTubePluginReplacement.cpp:235
> +	       const auto& queryDict = queryKeysAndValues(query);

Nit: "Dict" ->	"Dictionary"

> Source/WebCore/Modules/plugins/YouTubePluginReplacement.cpp:242
> +		   const auto& fragmentDict =
queryKeysAndValues(url.fragmentIdentifier());

Ditto.

> Source/WebCore/Modules/plugins/YouTubePluginReplacement.cpp:272
> +		   const auto& queryDict = queryKeysAndValues(query);
> +		   String videoID;
> +		   const auto& videoValue = queryDict.find("v");
> +		   if (videoValue != queryDict.end())
> +		       videoID = videoValue->value;
> +		   
> +		   if (!videoID.isEmpty()) {
> +		       String timeID;
> +		       const auto& timeValue = queryDict.find("t");
> +		       if (timeValue != queryDict.end())
> +			   timeID = timeValue->value;
> +		       
> +		       return createYouTubeURL(videoID, timeID);
> +		   }
> +	       }

Can this logic be put into a function and shared with the "/watch" url code
above?

> Source/WebCore/Modules/plugins/YouTubePluginReplacement.h:56
> +    static void registerPluginReplacement(PluginReplacementRegistrar);
> +    static bool supportsMimeType(const String&);
> +    static bool supportsFileExtension(const String&);
> +    static bool supportsURL(const URL&);
> +    
> +    static PassRefPtr<PluginReplacement> create(HTMLPlugInElement&, const
Vector<String>& paramNames, const Vector<String>& paramValues);
> +
> +    virtual bool installReplacement(ShadowRoot*) override;
> +    
> +    String youTubeURL(const String& rawURL);
> +    
> +    typedef HashMap<String, String> KeyValueMap;

I think these can all be marked private.

> Source/WebKit/mac/ChangeLog:11
> +	   * Misc/WebNSURLExtras.mm: Move useful code to WebCore so that it can

> +	   be shared with WK2.

Is this comment still true?


More information about the webkit-reviews mailing list