Bug 46535 – WebKit does not build when PLUGIN_PROXY_FOR_VIDEO is enabled

• Component: Media
• Status: RESOLVED CONFIGURATION CHANGED

[Joe Mason]

Created attachment 68777 [details]
build fixes

The patches for these three bugs introduced a lot of build errors in code that's wrapped in "if ENABLE(PLUGIN_PROXY_FOR_VIDEO)":

        https://bugs.webkit.org/show_bug.cgi?id=36681
        https://bugs.webkit.org/show_bug.cgi?id=42770
        https://bugs.webkit.org/show_bug.cgi?id=45058

The attached patch fixes them.

I'm not 100% sure that changing "url = src()" to "url = currentSrc()" is correct, but I don't see anything else obvious to do here.

[Eric Seidel (no email)]

Comment on attachment 68777 [details]
build fixes

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

> WebCore/loader/FrameLoader.cpp:3531
> +#if ENABLE(PLUGIN_PROXY_FOR_VIDEO)
> +
> +void FrameLoader::hideMediaPlayerProxyPlugin(Widget* widget)
> +{
> +    m_client->hideMediaPlayerProxyPlugin(widget);
> +}
> +
> +void FrameLoader::showMediaPlayerProxyPlugin(Widget* widget)
> +{
> +    m_client->showMediaPlayerProxyPlugin(widget);
> +}
> +
> +#endif // ENABLE(PLUGIN_PROXY_FOR_VIDEO)

These methods do not belong on FrameLoader.  Just talk directly to the client.

[Adam Barth]

Comment on attachment 68777 [details]
build fixes

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

> WebCore/html/HTMLMediaElement.cpp:2235
>      SubframeLoader* loader = document()->frame()->loader()->subframeLoader();

What if frame is null?  Doesn't that crash?

[Joe Mason]

Created attachment 68803 [details]
updated patch

Fixed Eric's suggestion, and fixed the "url = currentSrc()" line which didn't actually compile.  (Which kind of defeats the purpose)

[Joe Mason]

Comment on attachment 68803 [details]
updated patch

Oops, didn't notice Adam's comment on the potential crash.  I might as well fix that too while I'm here.

[Joe Mason]

Created attachment 68804 [details]
build fixes plus crash fix

[David Kilzer (:ddkilzer)]

Eric Carlson should really review this.

[Eric Carlson]

Comment on attachment 68804 [details]
build fixes plus crash fix

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

> WebCore/html/HTMLMediaElement.cpp:3
> + * Copyright (C) Research In Motion Limited 2010. All rights reserved.

I don't know if we have guidelines about this, but my own policy is to only add a new copyright line when I make significant additions or changes to a file.


> WebCore/html/HTMLMediaElement.cpp:2182
> -    url = src();
> +    url = document()->completeURL(m_currentSrc);

This is not correct, m_currentSrc may not have been set at this point (it is set a few lines later in this function). Use "getNonEmptyURLAttribute(srcAttr)" instead.

[Ahmad Saleem]

This flag is gone since 2014:

https://github.com/WebKit/WebKit/commit/898c9ce2877a71613aea3c172aaac20c3a5da95c & https://github.com/WebKit/WebKit/commit/3b7e09d5b985ce159418e6bac001ea9f62aa17fc

I don't think we have anything to fix here.

Marking this as 'RESOLVED CONFIGURATION CHANGED'.