[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