Bug 43484 - [Qt] Setting wmode to "opaque" is not necessary for Symbian
Summary: [Qt] Setting wmode to "opaque" is not necessary for Symbian
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: S60 Emulator Other
: P2 Major
Assignee: Hui Huang
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2010-08-04 07:34 PDT by Hui Huang
Modified: 2010-10-15 07:25 PDT (History)
9 users (show)

See Also:


Attachments
Fix Bug 43484 (1.74 KB, patch)
2010-08-05 08:06 PDT, Hui Huang
eric: review-
Details | Formatted Diff | Diff
Proposed patch based on Eric's comments (1.60 KB, patch)
2010-08-08 18:52 PDT, Hui Huang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hui Huang 2010-08-04 07:34:37 PDT
In Webkit/qt/WebCoreSupport/FrameLoaderClientQt.cpp FrameLoaderClientQt::createPlugin:
        if (mimeType == "application/x-shockwave-flash") {
1463	            QWebPageClient* client = m_webFrame->page()->d->client;
1464	            if (!client || !qobject_cast<QWidget*>(client->pluginParent())) {
1465	                // inject wmode=opaque when there is no client or the client is not a QWebView
1466	                size_t wmodeIndex = params.find("wmode");
1467	                if (wmodeIndex == -1) {
1468	                    params.append("wmode");
1469	                    values.append("opaque");
1470	                } else
1471	                    values[wmodeIndex] = "opaque";
1472	            }

wmode parameter is always changed to opaque. the original value should be preserved for Flash Plugins that supports wmode=window as default.
Comment 1 Hui Huang 2010-08-05 08:06:35 PDT
Created attachment 63594 [details]
Fix Bug 43484

wmode is changed to windowless for Flash Plugins. Preserve the original wmode for Flash Plugins that prefer to support window and windowless transparent modes.
Comment 2 Eric Seidel 2010-08-06 13:41:30 PDT
I'm confused.  Are you editing the flash code?

Why can't flash just reach out and read from the DOM itself it if needs to find the un-altered wmode parameter anyway?
Comment 3 Hui Huang 2010-08-06 16:21:36 PDT
Eric,
Thanks for the comments. Are you suggesting that Symbian Flash Player use NPN_GetValue(NPNVPluginElementNPObject) to get the DOM element that loaded the plugin and then use NPN_GetProperty to get the unaltered wmode? Thanks
Comment 4 Eric Seidel 2010-08-06 16:26:48 PDT
How does the symbian flash player work with other engines (like Gecko)?  Do they also ahve this "origwmode" parameter?
Comment 5 Hui Huang 2010-08-06 16:42:37 PDT
I see your point, but the other browser Symbian Flash Player works with doesn't modify wmode parameter to be "opaque" all the time. What about "transparent" and "window" which is supposed to be the default mode? Would be nice if Webkit could pass wmode to the plugin and let it decide which mode it runs in.
Comment 6 Eric Seidel 2010-08-06 16:48:01 PDT
Comment on attachment 63594 [details]
Fix Bug 43484

It sounds like symbian builds of WebKit want to #ifdef out the section of code which rewrites the param value then, instead of adding a new param.
Comment 7 Hui Huang 2010-08-06 20:15:35 PDT
I think you are right. I will ifdef out the code that rewrites the parameter value for Symbian, retest with Symbian Flash player, and submit a new patch for review if it works. Thanks a lot for the advice.
Comment 8 Hui Huang 2010-08-07 05:16:45 PDT
Symbian Flash Player has been working OK in windowed mode with S60 OSS Browser, which is a Webkit Browser. Windowless mode is better than windowed mode, but unlike Adode Flash Player on Windows and X11, Symbian Flash Player can ignore wmode parameter value and always run in windowless mode if it needs to. Setting wmode to "opaque" is not necessary for Symbian Flash Player. Introducing platform specific code is not the best practice but in this case it may be better than adding a new parameter or getting wmode from DOM.
Comment 9 Hui Huang 2010-08-08 18:52:11 PDT
Created attachment 63854 [details]
Proposed patch based on Eric's comments

Submit new patch for review. Tested successfully with Symbian flash player on S60 emulator.
Comment 10 Simon Hausmann 2010-08-09 03:38:05 PDT
While we could do this, it makes QGraphicsWebView on Symbian incompatible with the other platforms, where we are making an effort to support application side composition.

This change _does_ break any existing browser UI based on QGraphicsView.

Are you _SURE_ that you want that?

I am not, which is why I'm not in favour of this patch altogether. I think efforts are better spent on making Flash on Symbian support windowless rendering.
Comment 11 Hui Huang 2010-08-09 08:34:47 PDT
Thanks for the comments. I agree with you that windowless mode is better. However, at this point, it is impossible to convince Symbian Flash Player to drop windowed mode completely, which is their default mode with better performance and is working OK with S60 OSS Browser. In addition, unlike Adobe Flash Player, Symbian Flash Player can ignore wmode parameter value and run in windowless mode as default, so the problem of windowed mode breaking Browser UI can be solved from Plug-in side on Symbian if needed. I don't think Symbian specific implementation is the best approach, but I can't find a better solution in this case and we need to meet the deadline for product release. Your comments and proposal are appreciated.
Comment 12 Simon Hausmann 2010-10-07 08:14:40 PDT
(In reply to comment #11)
> Thanks for the comments. I agree with you that windowless mode is better. However, at this point, it is impossible to convince Symbian Flash Player to drop windowed mode completely, which is their default mode with better performance and is working OK with S60 OSS Browser. In addition, unlike Adobe Flash Player, Symbian Flash Player can ignore wmode parameter value and run in windowless mode as default, so the problem of windowed mode breaking Browser UI can be solved from Plug-in side on Symbian if needed. I don't think Symbian specific implementation is the best approach, but I can't find a better solution in this case and we need to meet the deadline for product release. Your comments and proposal are appreciated.

Before reviewing the patch, I'd like to ask: Have you tried flash with your patch applied inside the QGraphicsWebView based browser or QtTestBrowser?
Comment 13 Hui Huang 2010-10-07 09:09:22 PDT
Hi Simon, it was tested with Nokia Flash player on Nokia Browser 8.x Alpha, which is a QGraphicsWebView based browser. Thanks.
Comment 14 Girish Ramakrishnan 2010-10-07 12:36:01 PDT
(In reply to comment #13)
> Hi Simon, it was tested with Nokia Flash player on Nokia Browser 8.x Alpha, which is a QGraphicsWebView based browser. Thanks.

What won't work is QGV features like rotation, scaling etc. Maybe that's not important on Symbian?
Comment 15 Simon Hausmann 2010-10-08 06:01:30 PDT
Comment on attachment 63854 [details]
Proposed patch based on Eric's comments

Well well, I'm pretty sure we'll end up reverting this in a little while, but for the moment this patch doesn't cause any problems on platforms other than Symbian.
Comment 16 WebKit Commit Bot 2010-10-08 06:58:25 PDT
Comment on attachment 63854 [details]
Proposed patch based on Eric's comments

Clearing flags on attachment: 63854

Committed r69396: <http://trac.webkit.org/changeset/69396>
Comment 17 WebKit Commit Bot 2010-10-08 06:58:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Hui Huang 2010-10-08 07:23:39 PDT
Hi Simon, thanks a lot for reviewing my patch. This is what Nokia Flash player wanted, though I personally agree with you that Windowless mode works better with QGraphicsView. As you said, we can always revert this change if Flash player changes their minds.
Comment 19 Ademar Reis 2010-10-15 07:24:57 PDT
Revision r69396 cherry-picked into qtwebkit-2.1 with commit e36bbba <http://gitorious.org/webkit/qtwebkit/commit/e36bbba>