[Mac, iOS] REGRESSION: Improve YouTube compatibility in WK2

• Component: Media
• Status: RESOLVED FIXED

[Brent Fulgham]

Since WK2 doesn't support plugins, we need to handle YouTube URLs differently.

[Brent Fulgham]

Created attachment 230833 [details]
Patch

[Brent Fulgham]

This patch is mainly just moving code so that WK1 and WK2 can share the implementation.

[Brent Fulgham]

<rdar://problem/11464344>

[Sam Weinig]

(In reply to comment #0)
> Since WK2 doesn't support plugins, we need to handle YouTube URLs differently.

Just to be clear...WebKit2 does support plug-ins, just not WebKit Plug-ins.

[Sam Weinig]

Comment on attachment 230833 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230833&action=review

> Source/WebCore/platform/mac/WebCoreNSURLExtras.h:53
> +NSURL *webkitYouTubeURL(NSURL *);

This is definitely the wrong layer for this.

[Brent Fulgham]

Created attachment 230939 [details]
(Exposed symbols we don't need anymore)

[Brent Fulgham]

Created attachment 230941 [details]
(Exported an iOS Symbol we don't need anymore)

[Brent Fulgham]

Created attachment 230944 [details]
Patch

[Darin Adler]

Comment on attachment 230944 [details]
Patch

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.

[Sam Weinig]

I'd really like this cleaned up not to use Objective-C before landing.

[Brent Fulgham]

(In reply to comment #9)
> (From update of attachment 230944 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230944&action=review
> 
> > Source/WebCore/Modules/plugins/YouTubePluginReplacement.h:30
> > +#include "ScriptState.h"
> > +#include <bindings/ScriptObject.h>
> > +#include <wtf/RefCounted.h>
> > +#include <wtf/text/WTFString.h>
> 
> Why [are these] include[s] needed?

They aren't needed. I'll remove them.

> > Source/WebCore/Modules/plugins/YouTubePluginReplacement.h:51
> > +    YouTubePluginReplacement();
> 
> What is this default constructor needed for?

It's not needed. I'll remove.

> > +    virtual bool willCreateRenderer() override { return m_videoElement; }
> > +    virtual RenderPtr<RenderElement> createElementRenderer(HTMLPlugInElement&, PassRef<RenderStyle>) override;
> 
> Can these overrides be private instead of public?

Yes, I'll move them.
 
> > Source/WebCore/Modules/plugins/YouTubePluginReplacement.h:58
> > +    YouTubeEmbedShadowElement* parentElement() { return m_videoElement.get(); }
> 
> Can this ever be null?

Yes -- it is none before the layout phase where the video element is generated.

> It’s very confusing to call this function parentElement, when m_parentElement is something different.

I'm using this naming based on the QuickTimePluginReplacement, upon which this code is based. However, the YouTube plugin doesn't need it, so I'll just remove the code.

> > 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?

Good point! Fixed.

> > 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?

Ditto.

> > 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.

I'll rename this to "toNSMutableArray" to be a little clearer.

> None of this code follows our rule for where to put a "*" in an Objective-C type like NSMutableArray *.

I'll fix that.

> > 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.

Done.

> > Source/WebCore/Modules/plugins/YouTubePluginReplacement.mm:78
> > +    : PluginReplacement()
> 
> Why do we need to explicitly initialize the base class like this?

I don't think we do. I've removed it.
 
> > 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.

Done.
 
> > 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.

Done.

> > Source/WebCore/Modules/plugins/YouTubePluginReplacement.mm:101
> > +static NSString* objectForKey(CFDictionaryRef dict, NSString* key)
> 
> I suggest naming this dictionary rather than "dict".

Done.

> 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?

Yes. We build this dictionary ourselves from two Vectors of Strings. This could probably be replaced with a HashMap, but I was trying to retain as much of the existing Cocoa-based code for this iteration. We can follow-up with a second refactoring to pure C++ types once this initial functionality is in place.

> > 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.

Done (and the other case);
 
> > Source/WebCore/Modules/plugins/YouTubePluginReplacement.mm:137
> > +    ASSERT(videoID && [videoID length] > 0 && ![videoID isEqualToString:@"/"]);
> 
> This should be three assertions.

Done.
 
> > 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.

I'd prefer to do the conversion to C++ as a second step. This is almost all existing code moved from WebKit, so I'd like to get this refactoring done first, then convert to C++.

> > 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.

Done.

> > 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.

See above. This is existing NSURL-based code, and I didn't want to add more complexity to the change.

> > 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

Done.

> > Source/WebCore/platform/mac/WebCoreNSURLExtras.h:54
> > +NSDictionary *queryKeysAndValues(NSString*);
> > +NSString *unescapedQueryValue(NSString*);
> 
> Missing spaces here in NSString *.

Done.

> 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.

Agreed. How about if I do that next?

[Brent Fulgham]

(In reply to comment #10)
> I'd really like this cleaned up not to use Objective-C before landing.

*Sigh*.

Okay Sam, JavaScript it is!  :-)

[David Kilzer (:ddkilzer)]

(In reply to comment #12)
> (In reply to comment #10)
> > I'd really like this cleaned up not to use Objective-C before landing.
> 
> *Sigh*.
> 
> Okay Sam, JavaScript it is!  :-)

There could be performance issues on pages with large numbers of YouTube videos in <enbed> tags!   :)

[Simon Fraser (smfr)]

Comment on attachment 230944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230944&action=review

> Source/WebCore/Modules/plugins/YouTubePluginReplacement.mm:260
> +    // Transform the youtubeURL (youtube:VideoID) to iframe embed url which has the format: http://www.youtube.com/embed/VideoID
> +    NSString* srcPath = srcURL.path;
> +    NSString* videoID = [[youTubeURL absoluteString] substringFromIndex:[[youTubeURL scheme] length] + 1];
> +    NSRange rangeOfVideoIDInPath = [srcPath rangeOfString:videoID];
> +    if (rangeOfVideoIDInPath.location == NSNotFound)
> +        return srcString;
> +    
> +    ASSERT(rangeOfVideoIDInPath.length);
> +    
> +    // From the original URL, we need to get the part before /path/VideoId.
> +    NSRange rangeOfPathBeforeVideoID = [srcString rangeOfString:[srcPath substringToIndex:rangeOfVideoIDInPath.location]];
> +    ASSERT(rangeOfPathBeforeVideoID.length);
> +    
> +    NSString* srcURLPrefix = [srcString substringToIndex:rangeOfPathBeforeVideoID.location];
> +    NSString* query = srcURL.query;
> +
> +    // By default, the iframe will display information like the video title and uploader on top of the video. Don't display
> +    // them if the embeding html doesn't specify it.
> +    if (query && query.length && [query rangeOfString:@"showinfo"].location == NSNotFound)
> +        query = [query stringByAppendingFormat:@"&showinfo=0"];
> +    else
> +        query = @"showinfo=0";
> +    
> +    // Append the query string if it is valid. Some sites apparently forget to add "?" for the query string, in that case,
> +    // we will discard the parameters in the url.
> +    // See: <rdar://problem/11535155> [Bincompat] Regression: SC2Casts app: Videos don't play in SC2Casts
> +    NSString* embedSrc = query ? [srcURLPrefix stringByAppendingFormat:@"/embed/%@?%@", videoID, query] : [srcURLPrefix stringByAppendingFormat:@"/embed/%@", videoID];
> +    

Is this all Obj-C for a reason, or just because it was moved? I don't see why any of this has to be platform-specific.

[Brent Fulgham]

Comment on attachment 230944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230944&action=review

>> Source/WebCore/Modules/plugins/YouTubePluginReplacement.mm:260
>> +    
> 
> Is this all Obj-C for a reason, or just because it was moved? I don't see why any of this has to be platform-specific.

It's because it was moved. I was hoping to avoid rewriting the entire thing just to correct the lack of WK2 support.

[Eric Carlson]

Comment on attachment 230944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230944&action=review

> Source/WebCore/Modules/plugins/YouTubePluginReplacement.h:66
> +    RefPtr<YouTubeEmbedShadowElement> m_videoElement;

Nit: This ins't a <video> element, so I think "m_videoElement" is the not the right name to use. Maybe "m_embedShadowElement" instead?

[Brent Fulgham]

(In reply to comment #16)
> (From update of attachment 230944 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230944&action=review
> 
> > Source/WebCore/Modules/plugins/YouTubePluginReplacement.h:66
> > +    RefPtr<YouTubeEmbedShadowElement> m_videoElement;
> 
> Nit: This ins't a <video> element, so I think "m_videoElement" is the not the right name to use. Maybe "m_embedShadowElement" instead?

Will do.

[Brent Fulgham]

Created attachment 231009 [details]
Patch

[Eric Carlson]

Comment on attachment 231009 [details]
Patch

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?

[Brent Fulgham]

(In reply to comment #19)
> (From update of attachment 231009 [details])
> 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.

Done!

> > Source/WebCore/Modules/plugins/YouTubePluginReplacement.cpp:137
> > +        unsigned indexAfterEqual = equalLocation + 1;
> 
> why unsigned for indexAfterEqual and size_t for equalSearchLocation?

This is some crud left over from when I ported the Objective C code. I'll correct this.
 
> > Source/WebCore/Modules/plugins/YouTubePluginReplacement.cpp:164
> > +        // At the end.
> 
> Nit: this comment doesn't add much.

Yuck! It's gone.

> > 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.

The signature for "startsWith" is:

bool startsWith(const String&, bool caseSensitive);

I meant to use "false' for the caseSensitive flag.

Good catch!
 
> > 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.

Fixed.

> > 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?

Sure.

> > 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.

All except 'registerPluginReplacement'. Done!

> > 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?

Nope. The code has been deleted. I'll fix.

[Brent Fulgham]

Committed r168442: <http://trac.webkit.org/changeset/168442>