[webkit-reviews] review granted: [Bug 132568] [Mac, iOS] REGRESSION: Improve YouTube compatibility in WK2 : [Attachment 230944] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue May 6 18:11:34 PDT 2014
Darin Adler <darin 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 230944: Patch
https://bugs.webkit.org/attachment.cgi?id=230944&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=230944&action=review
> Source/WebCore/Modules/plugins/YouTubePluginReplacement.h:30
> +#include "ScriptState.h"
Why is this include needed?
> Source/WebCore/Modules/plugins/YouTubePluginReplacement.h:31
> +#include <bindings/ScriptObject.h>
Why is this include needed?
> Source/WebCore/Modules/plugins/YouTubePluginReplacement.h:32
> +#include <wtf/RefCounted.h>
This include is not needed.
> Source/WebCore/Modules/plugins/YouTubePluginReplacement.h:33
> +#include <wtf/text/WTFString.h>
Why is this include needed?
> Source/WebCore/Modules/plugins/YouTubePluginReplacement.h:51
> + YouTubePluginReplacement();
What is this default constructor needed for?
> Source/WebCore/Modules/plugins/YouTubePluginReplacement.h:56
> + virtual bool installReplacement(ShadowRoot*) override;
> +
> + virtual bool willCreateRenderer() override { return m_videoElement; }
> + virtual RenderPtr<RenderElement>
createElementRenderer(HTMLPlugInElement&, PassRef<RenderStyle>) override;
Can these overrides be private instead of public?
> Source/WebCore/Modules/plugins/YouTubePluginReplacement.h:58
> + YouTubeEmbedShadowElement* parentElement() { return
m_videoElement.get(); }
Can this ever be null?
It’s very confusing to call this function parentElement, when m_parentElement
is something different.
> Source/WebCore/Modules/plugins/YouTubePluginReplacement.mm:60
> + return mimeType == "application/x-shockwave-flash"
> + || mimeType == "application/futuresplash";
MIME types normally aren’t case sensitive. Why is this doing a case sensitive
check?
> Source/WebCore/Modules/plugins/YouTubePluginReplacement.mm:65
> + return extension == "spl" || extension == "swf";
Extensions normally aren’t case sensitive. Why is this doing a case sensitive
check?
> Source/WebCore/Modules/plugins/YouTubePluginReplacement.mm:68
> +static NSMutableArray* kit(const Vector<String>& vector)
This is a questionable function name in WebKit, but really lame in WebCore. I
guess we can leave it, though.
None of this code follows our rule for where to put a "*" in an Objective-C
type like NSMutableArray *.
> Source/WebCore/Modules/plugins/YouTubePluginReplacement.mm:70
> + unsigned len = vector.size();
Can we just call this "size" instead of an abbreviated name len.
> Source/WebCore/Modules/plugins/YouTubePluginReplacement.mm:73
> + for (unsigned x = 0; x < len; x++)
> + [array addObject:vector[x]];
Should use a modern for loop instead.
> Source/WebCore/Modules/plugins/YouTubePluginReplacement.mm:78
> + : PluginReplacement()
Why do we need to explicitly initialize the base class like this?
> Source/WebCore/Modules/plugins/YouTubePluginReplacement.mm:87
> + , m_videoElement(nullptr)
No need for this. RefPtr is initialized to null even if you don’T initialize it
explicitly.
> Source/WebCore/Modules/plugins/YouTubePluginReplacement.mm:98
> + if (m_videoElement)
> + return m_videoElement->createElementRenderer(std::move(style));
> +
> + return nullptr;
I prefer early return for the error case rather than for the success case.
> Source/WebCore/Modules/plugins/YouTubePluginReplacement.mm:101
> +static NSString* objectForKey(CFDictionaryRef dict, NSString* key)
I suggest naming this dictionary rather than "dict".
But also, do we really need this function just to do a type cast? It’s also
lame that this returns NSString *. Do we have a guarantee that it's a string?
> Source/WebCore/Modules/plugins/YouTubePluginReplacement.mm:110
> + root->appendChild(m_videoElement.get(), ASSERT_NO_EXCEPTION);
No need for ASSERT_NO_EXCEPTION here. It’s the default.
> Source/WebCore/Modules/plugins/YouTubePluginReplacement.mm:127
> + m_videoElement->appendChild(iframeElement, ASSERT_NO_EXCEPTION);
No need for ASSERT_NO_EXCEPTION here. It’s the default.
> Source/WebCore/Modules/plugins/YouTubePluginReplacement.mm:137
> + ASSERT(videoID && [videoID length] > 0 && ![videoID
isEqualToString:@"/"]);
This should be three assertions.
> Source/WebCore/Modules/plugins/YouTubePluginReplacement.mm:149
> + NSString* scheme = [url.scheme lowercaseString];
> + if (![scheme isEqualToString:@"http"] && ![scheme
isEqualToString:@"https"])
> + return nullptr;
WebCore::URL does operations like this better. It’s strange that this is all
done with the Objective-C classes.
> Source/WebCore/Modules/plugins/YouTubePluginReplacement.mm:167
> + if (videoID && ![videoID isEqualToString:@"/"])
> + return createYouTubeURL(videoID, nil);
> + return nullptr;
Again, I prefer to put error cases first with early return.
> Source/WebCore/Modules/plugins/YouTubePluginReplacement.mm:228
> + NSURL* srcURL = [NSURL URLWithString:srcString];
It’s really strange to be using NSURL here instead of WebCore::URL.
> Source/WebCore/Modules/plugins/YouTubePluginReplacement.mm:261
> + NSString* embedSrc = query ? [srcURLPrefix
stringByAppendingFormat:@"/embed/%@?%@", videoID, query] : [srcURLPrefix
stringByAppendingFormat:@"/embed/%@", videoID];
> +
> + return embedSrc;
Seems like this should just be a return statement
> Source/WebCore/platform/mac/WebCoreNSURLExtras.h:54
> +NSDictionary *queryKeysAndValues(NSString*);
> +NSString *unescapedQueryValue(NSString*);
Missing spaces here in NSString *.
At some point we should just use WebCore::URL for all of this. It doesn’t make
sense to convert everything to NSString and NSURL.
More information about the webkit-reviews
mailing list