Bug 87399 – Enable Chromium media player to instantiate a plugin

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[David Dorwin]

Enable Chromium media player to instantiate a plugin

[David Dorwin]

Created attachment 143842 [details]
Patch

[Kent Tamura]

Comment on attachment 143842 [details]
Patch

It looks a reasonable approach.
I don't think HelperPluginWidget should be a WebPagePopupImpl because their behavior and purposes are different. If we'd like to share code, we can use PageWidgetDelegate.

[David Dorwin]

Created attachment 144956 [details]
Changed to create its own RenderWidget and addressed open issues.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

What's this for?  Is there a bug somewhere discussing this?

[David Dorwin]

The Chromium changes are tracked in http://crbug.com/130425. The first CL is at http://codereview.chromium.org/10442102/.

Notes about the implementation in the current (second) patch:
 1) This only affects the Chromium port.
 2) The implementation uses createPopupMenu() and adds a new WebPopupType. This is not really a popup, but it is a non-main-page HTML widget. Maybe it makes sense to refactor the RenderWidget initialization functions later.
 3) This could probably be _somewhat_ generalized for generic off-screen widgets, but that might be over-engineering at this point. We'd still need RenderWidget logic specific to the HelperPlugin class. We could generalize the entire off-screen page (the HTML), but I'd like to wait for the second phase to see how that affects this class.
 4) There might be some code that could be shared between WebPagePopupImpl and WebHelperPluginImpl. I think it would be best to finish this implementation then re-evaluate what is common and whether it is worth sharing.

[James Robinson]

That bug is very vague - I'm not sure what additional "plugin" functionality the media player is supposed to be able to access.

[WebKit Review Bot]

Comment on attachment 144956 [details]
Changed to create its own RenderWidget and addressed open issues.

Attachment 144956 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12850991

[David Dorwin]

Created attachment 145127 [details]
Fixes DumpRenderTree build

[David Dorwin]

Created attachment 145178 [details]
Replaced hard-coded plugin type with parameter

[Kent Tamura]

Comment on attachment 145178 [details]
Replaced hard-coded plugin type with parameter

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

Don't we need any ENABLE(foo) flag to disable the new code?  Will it used on Android?

> Source/WebKit/chromium/public/WebPopupType.h:41
> +    WebPopupHelperPlugin, // An off-screen helper plugin.

This should be WebPopupTypeHelperPlugin
http://trac.webkit.org/wiki/ChromiumWebKitAPI#Enums

> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:163
> +    writer->setMIMEType("text/html");
> +    writer->setEncoding("UTF-8", false);
> +    writer->begin();
> +    writeDocument(*writer, pluginType);
> +    writer->end();

nit: writer->foo calls should be moved into writeDocument() to put related code to one place.

> Source/WebKit/chromium/src/WebViewImpl.cpp:476
>  {
>      ASSERT(!m_page);
> +    ASSERT(!hasOpenedPopup());
>  }

This looks unnecessary for this patch.

[Adam Barth]

> Don't we need any ENABLE(foo) flag to disable the new code?

The existence of the helper plugin shouldn't be visible to the platform, so I don't think we need ENABLE macros.

> Will it used on Android?

My understanding is that desktop chrome is going to be the main consumer in the near term.

[WebKit Review Bot]

Comment on attachment 145178 [details]
Replaced hard-coded plugin type with parameter

Attachment 145178 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12869396

New failing tests:
http/tests/media/media-source/video-media-source-event-attributes.html

[WebKit Review Bot]

Created attachment 145212 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[David Dorwin]

Comment on attachment 145178 [details]
Replaced hard-coded plugin type with parameter

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

> > Don't we need any ENABLE(foo) flag to disable the new code?
> 
> The existence of the helper plugin shouldn't be visible to the platform, so I don't think we need ENABLE macros.

This was my reasoning. All the changes are in chromium directories.

> > Will it used on Android?
> 
> My understanding is that desktop chrome is going to be the main consumer in the near term.

TBD. Android can always choose not to use it. The code size would seem negligible and it is one less build option to maintain.

>> Source/WebKit/chromium/public/WebPopupType.h:41
>> +    WebPopupHelperPlugin, // An off-screen helper plugin.
> 
> This should be WebPopupTypeHelperPlugin
> http://trac.webkit.org/wiki/ChromiumWebKitAPI#Enums

Oops. Done.

>> Source/WebKit/chromium/src/WebHelperPluginImpl.cpp:163
>> +    writer->end();
> 
> nit: writer->foo calls should be moved into writeDocument() to put related code to one place.

I left it this way to be consistent with WebPagePopupImpl::initPage(). The main difference is that WebPagePopupImpl uses a client, so this makes slightly more sense. If we ever generalize this, we'll want to do the same. For now, I've moved them.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:476
>>  }
> 
> This looks unnecessary for this patch.

Removed.
(Is this correct and worth including in a separate patch? In an earlier version, I had a similar check for the helper plugin widget what I found useful.)

[David Dorwin]

Created attachment 145347 [details]
Addressed review feedback

[Kent Tamura]

Comment on attachment 145347 [details]
Addressed review feedback

Looks ok.

[WebKit Review Bot]

Comment on attachment 145347 [details]
Addressed review feedback

Clearing flags on attachment: 145347

Committed r119411: <http://trac.webkit.org/changeset/119411>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

This patch caused a compilation failure on Windows, and still causing it:
http://build.webkit.org/builders/Chromium%20Win%20Release/builds/44809/steps/compile-webkit/logs/stdio

At this point, I'd like to roll this patch out. I don't have access to a Windows machine, I can't keep adding #include one by one since that'll take hours.

[WebKit Review Bot]

Re-opened since this is blocked by 88262

[Ryosuke Niwa]

Build fixed after http://trac.webkit.org/changeset/119424, http://trac.webkit.org/changeset/119433, and http://trac.webkit.org/changeset/119434