Bug 97186 – QuickTime movie is not scaled when opened by movie file URL

• Component: Plug-ins
• Status: RESOLVED WONTFIX

When user enters URL to the QuickTime movie in the address bar, opened movie will not scale when browser window resizes. This is especially big problem when movie has bigger resolution then computer screen, since in that case user forced to use scrollbars to view entire picture.

Created attachment 164914 [details]
Proposed patch

[Eric Carlson]

Comment on attachment 164914 [details]
Proposed patch

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

> Source/WebCore/ChangeLog:8
> +        This patch developed by Yandex LLC.

This is unusual for a ChangeLog entry. If you want attribution you can add a copyright line to the file you modified.

> Source/WebCore/html/PluginDocument.cpp:105
>          m_embedElement->setAttribute(typeAttr, loader->writer()->mimeType());
> +    const PluginData* pluginData = document()->page()->pluginData();
> +    String mimeType = loader->writer()->mimeType();
> +    String pluginName = pluginData ? pluginData->pluginNameForMimeType(mimeType) : String();
> +    if (pluginName.contains("QuickTime", false)) {
> +        // By default QuickTime plugin will not scale when browser window resizes.
> +        const QualifiedName scaleAttr(nullAtom, "scale", xhtmlNamespaceURI);
> +        m_embedElement->setAttribute(scaleAttr, "aspect");

This indentation is wrong.

You should define "mimeType" earlier in the function and use it in "m_embedElement->setAttribute(...)"

(In reply to comment #2)
> 
> This indentation is wrong.
> 
Sorry, I don't understand whats wrong here with indentation. I used 4-space indent, as WebKit coding style recommends. The same indent used in "if (loader) ...", which goes right before lines I have added.
Thank you for other recommendations, I'll fix them in nearest future.

[Alexey Proskuryakov]

Comment on attachment 164914 [details]
Proposed patch

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

> Source/WebCore/html/PluginDocument.cpp:104
> +        const QualifiedName scaleAttr(nullAtom, "scale", xhtmlNamespaceURI);

Shouldn't this be coming from HTMLNames.h?

(In reply to comment #4)
At present, there are no "scale" attribute in HTMLNames.h. I placed definition of this attribute here since it specific to QuickTime plugin. If it is better to have it in HTMLNames.h, with other HTML attributes, I'll move it.

[Eric Carlson]

(In reply to comment #3)
> (In reply to comment #2)
> > 
> > This indentation is wrong.
> > 
> Sorry, I don't understand whats wrong here with indentation. I used 4-space indent, as WebKit coding style recommends. The same indent used in "if (loader) ...", which goes right before lines I have added.

Sorry, it must be an artifact of the pretty-print diff formatting.

Created attachment 165543 [details]
New patch 1

Patch with local definition of "scale" attribute

Created attachment 165544 [details]
New patch 2

Patch with global definition of "scale" attribute

[Eric Carlson]

Comment on attachment 165544 [details]
New patch 2

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

> Source/WebCore/html/PluginDocument.cpp:105
> +    String pluginName = pluginData ? pluginData->pluginNameForMimeType(mimeType) : String();

mimeType will be uninitialized if loader is NULL>

(In reply to comment #9)
I supposed that in that case default constructor of Stirng() will init it as empty string. Thus pluginNameForMimeType will not be able to find plugin name for it and just return empty string, so no additional problems will arise. Am I wrong?
I can re-design code in other way, if you think that present code have problems, but could you tell, which variant of our patches is better - with global definition of "scale" attribute or local one?

Created attachment 166219 [details]
New patch 1

Patch with local definition of scale attribute

Created attachment 166220 [details]
New patch 2

Patch with global definition of scale attribute

[Alexey Proskuryakov]

Could you please provide a URL to test with?

When I try to load QuickTime movies found on the Web, they just get downloaded.

[Eric Carlson]

(In reply to comment #10)
> (In reply to comment #9)
> I supposed that in that case default constructor of Stirng() will init it as empty string. Thus pluginNameForMimeType will not be able to find plugin name for it and just return empty string, so no additional problems will arise. Am I wrong?
> I can re-design code in other way, if you think that present code have problems, but could you tell, which variant of our patches is better - with global definition of "scale" attribute or local one?

"uninitialized" was the wrong word to use because you are correct that it won't cause any harm, but it is inefficient to call a function that you know will fail. IOW, something like this would be more efficient:

    String pluginName = pluginData && !mimeType.isEmpty() ? pluginData->pluginNameForMimeType(mimeType) : String();

I think the patch with the scale as a global attribute makes more sense, as we also define other non-spec'd attributes globally (eg. "classid").

I am concerned that this patch will not be a positive change for most users. While I agree that it will be an improvement when video file that is larger than the browser window I don't think that is the most common case. Because WebKit always tries to create a <video> element first and only creates an <embed> if that fails to load, and because in my experience the kinds of files that end up going to the QuickTime plug-in are typically smaller than the browser window, and these will *not* usually look good scaled up to the size of the window.

What types of files to you encounter that end up in a PluginDocument?

(In reply to comment #14)
> (In reply to comment #10)
> > (In reply to comment #9)
Thank you so much for the explanation. New Patch 2 from 28 of September shoul confirm to you requests.

You're right that in most cases video will go to the <video> tag. But some browsers, which use WebKit, do not support all required video formats due to patent problems. In my experience it was .mp4 file with computer-screen-wide screencast, and it was very difficult to watch it on computers with smaller screen resolution. If you think main WebKit repository does not need this feature, since most popular WebKit-based browsers support all required media formats, I'll give up and you can close this issue.

[Alexey Proskuryakov]

Mass closing plug-in bugs, as plug-in support has been removed from WebKit.

Please comment and/or reopen if this still affects WebKit in some way.