[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