43484 2010-08-04 07:34:37 -0700 [Qt] Setting wmode to "opaque" is not necessary for Symbian 2010-10-15 07:25:14 -0700 1 1 1 Unclassified WebKit Plug-ins 528+ (Nightly build) S60 Emulator Other RESOLVED FIXED Qt P2 Major --- 1 hui_huang hui_huang ademar commit-queue cshu darin eric girish hausmann hui_huang laszlo.gombos oldest_to_newest 260016 0 hui_huang 2010-08-04 07:34:37 -0700 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. 260618 1 63594 hui_huang 2010-08-05 08:06:35 -0700 Created attachment 63594 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. 261432 2 eric 2010-08-06 13:41:30 -0700 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? 261553 3 hui_huang 2010-08-06 16:21:36 -0700 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 261555 4 eric 2010-08-06 16:26:48 -0700 How does the symbian flash player work with other engines (like Gecko)? Do they also ahve this "origwmode" parameter? 261570 5 hui_huang 2010-08-06 16:42:37 -0700 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. 261577 6 63594 eric 2010-08-06 16:48:01 -0700 Comment on attachment 63594 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. 261638 7 hui_huang 2010-08-06 20:15:35 -0700 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. 261711 8 hui_huang 2010-08-07 05:16:45 -0700 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. 261952 9 63854 hui_huang 2010-08-08 18:52:11 -0700 Created attachment 63854 Proposed patch based on Eric's comments Submit new patch for review. Tested successfully with Symbian flash player on S60 emulator. 262063 10 hausmann 2010-08-09 03:38:05 -0700 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. 262147 11 hui_huang 2010-08-09 08:34:47 -0700 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. 291022 12 hausmann 2010-10-07 08:14:40 -0700 (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? 291043 13 hui_huang 2010-10-07 09:09:22 -0700 Hi Simon, it was tested with Nokia Flash player on Nokia Browser 8.x Alpha, which is a QGraphicsWebView based browser. Thanks. 291185 14 girish 2010-10-07 12:36:01 -0700 (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? 291592 15 63854 hausmann 2010-10-08 06:01:30 -0700 Comment on attachment 63854 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. 291618 16 63854 commit-queue 2010-10-08 06:58:25 -0700 Comment on attachment 63854 Proposed patch based on Eric's comments Clearing flags on attachment: 63854 Committed r69396: <http://trac.webkit.org/changeset/69396> 291619 17 commit-queue 2010-10-08 06:58:32 -0700 All reviewed patches have been landed. Closing bug. 291639 18 hui_huang 2010-10-08 07:23:39 -0700 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. 294746 19 ademar 2010-10-15 07:24:57 -0700 Revision r69396 cherry-picked into qtwebkit-2.1 with commit e36bbba <http://gitorious.org/webkit/qtwebkit/commit/e36bbba> 63594 2010-08-05 08:06:35 -0700 2010-08-08 18:52:11 -0700 Fix Bug 43484 bug43484patch.txt text/plain 1783 hui_huang SW5kZXg6IFdlYktpdC9xdC9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViS2l0L3F0L0NoYW5n ZUxvZwkocmV2aXNpb24gNjQ3NDkpCisrKyBXZWJLaXQvcXQvQ2hhbmdlTG9nCSh3b3JraW5nIGNv cHkpCkBAIC0xLDMgKzEsMTMgQEAKKzIwMTAtMDgtMDUgIEh1aSBIdWFuZyAgPGh1aS4yLmh1YW5n QG5va2lhLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICBQcmVzZXJ2ZSBvcmlnaW5hbCB3bW9kZSBwYXJhbWV0ZXIgdmFsdWUgZm9yIEZsYXNoIFBs dWdpbgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDM0 ODQKKworICAgICAgICAqIFdlYkNvcmVTdXBwb3J0L0ZyYW1lTG9hZGVyQ2xpZW50UXQuY3BwOgor ICAgICAgICAoV2ViQ29yZTo6RnJhbWVMb2FkZXJDbGllbnRRdDo6Y3JlYXRlUGx1Z2luKToKKwog MjAxMC0wOC0wNSAgU2ltb24gSGF1c21hbm4gIDxzaW1vbi5oYXVzbWFubkBub2tpYS5jb20+CiAK ICAgICAgICAgUmV2aWV3ZWQgYnkgVG9yIEFybmUgVmVzdGLDuC4KSW5kZXg6IFdlYktpdC9xdC9X ZWJDb3JlU3VwcG9ydC9GcmFtZUxvYWRlckNsaWVudFF0LmNwcAo9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJL aXQvcXQvV2ViQ29yZVN1cHBvcnQvRnJhbWVMb2FkZXJDbGllbnRRdC5jcHAJKHJldmlzaW9uIDY0 NzQyKQorKysgV2ViS2l0L3F0L1dlYkNvcmVTdXBwb3J0L0ZyYW1lTG9hZGVyQ2xpZW50UXQuY3Bw CSh3b3JraW5nIGNvcHkpCkBAIC0xNDY1LDEwICsxNDY1LDE5IEBAIFBhc3NSZWZQdHI8V2lkZ2V0 PiBGcmFtZUxvYWRlckNsaWVudFF0OjoKICAgICAgICAgICAgICAgICAvLyBpbmplY3Qgd21vZGU9 b3BhcXVlIHdoZW4gdGhlcmUgaXMgbm8gY2xpZW50IG9yIHRoZSBjbGllbnQgaXMgbm90IGEgUVdl YlZpZXcKICAgICAgICAgICAgICAgICBzaXplX3Qgd21vZGVJbmRleCA9IHBhcmFtcy5maW5kKCJ3 bW9kZSIpOwogICAgICAgICAgICAgICAgIGlmICh3bW9kZUluZGV4ID09IC0xKSB7CisgICAgICAg ICAgICAgICAgICAgIC8vIEluamVjdCB3bW9kZT1vcGFxdWUKICAgICAgICAgICAgICAgICAgICAg cGFyYW1zLmFwcGVuZCgid21vZGUiKTsKICAgICAgICAgICAgICAgICAgICAgdmFsdWVzLmFwcGVu ZCgib3BhcXVlIik7Ci0gICAgICAgICAgICAgICAgfSBlbHNlCisgICAgICAgICAgICAgICAgICAg IC8vIFByZXNlcnZlIG9yaWdpbmFsIHdtb2RlIHdoaWNoIGlzIHdpbmRvdyBieSBkZWZhdWx0Cisg ICAgICAgICAgICAgICAgICAgIHBhcmFtcy5hcHBlbmQoIm9yaWd3bW9kZSIpOworICAgICAgICAg ICAgICAgICAgICB2YWx1ZXMuYXBwZW5kKCJ3aW5kb3ciKTsKKyAgICAgICAgICAgICAgICB9IGVs c2UgeworICAgICAgICAgICAgICAgICAgICAvLyBQcmVzZXJ2ZSBvcmlnaW5hbCB3bW9kZQorICAg ICAgICAgICAgICAgICAgICBwYXJhbXMuYXBwZW5kKCJvcmlnd21vZGUiKTsKKyAgICAgICAgICAg ICAgICAgICAgdmFsdWVzLmFwcGVuZCh2YWx1ZXNbd21vZGVJbmRleF0pOworICAgICAgICAgICAg ICAgICAgICAvLyBJbmplY3Qgd21vZGU9b3BhcXVlCiAgICAgICAgICAgICAgICAgICAgIHZhbHVl c1t3bW9kZUluZGV4XSA9ICJvcGFxdWUiOworICAgICAgICAgICAgICAgIH0KICAgICAgICAgICAg IH0KICAgICAgICAgfQogCg== 63854 2010-08-08 18:52:11 -0700 2010-10-08 06:58:25 -0700 Proposed patch based on Eric's comments bug43484patch2.txt text/plain 1641 hui_huang SW5kZXg6IFdlYktpdC9xdC9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViS2l0L3F0L0NoYW5n ZUxvZwkocmV2aXNpb24gNjQ5NTYpCisrKyBXZWJLaXQvcXQvQ2hhbmdlTG9nCSh3b3JraW5nIGNv cHkpCkBAIC0xLDMgKzEsMTMgQEAKKzIwMTAtMDgtMDggIEh1aSBIdWFuZyAgPGh1aS4yLmh1YW5n QG5va2lhLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICBpZmRlZiBvdXQgY29kZSB0aGF0IHJld3JpdGVzIEZsYXNoIFBsdWctaW4gd21vZGUgZm9y IFN5bWJpYW4gYnVpbGQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTQzNDg0CisKKyAgICAgICAgKiBXZWJDb3JlU3VwcG9ydC9GcmFtZUxvYWRlckNsaWVu dFF0LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkZyYW1lTG9hZGVyQ2xpZW50UXQ6OmNyZWF0ZVBs dWdpbik6CisKIDIwMTAtMDgtMDYgIEplc3NpZSBCZXJsaW4gIDxqYmVybGluQGFwcGxlLmNvbT4K IAogICAgICAgICBSb2xsIG91dCBodHRwOi8vdHJhYy53ZWJraXQub3JnL2NoYW5nZXNldC82NDgw MSwgd2hpY2ggYnJva2UgdGhlIFNhZmFyaSBXaW5kb3dzIEJ1aWxkLgpJbmRleDogV2ViS2l0L3F0 L1dlYkNvcmVTdXBwb3J0L0ZyYW1lTG9hZGVyQ2xpZW50UXQuY3BwCj09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdl YktpdC9xdC9XZWJDb3JlU3VwcG9ydC9GcmFtZUxvYWRlckNsaWVudFF0LmNwcAkocmV2aXNpb24g NjQ5NDgpCisrKyBXZWJLaXQvcXQvV2ViQ29yZVN1cHBvcnQvRnJhbWVMb2FkZXJDbGllbnRRdC5j cHAJKHdvcmtpbmcgY29weSkKQEAgLTE0NTksNiArMTQ1OSw3IEBAIFBhc3NSZWZQdHI8V2lkZ2V0 PiBGcmFtZUxvYWRlckNsaWVudFF0OjoKICAgICB9IGVsc2UgeyAvLyBOUEFQSSBQbHVnaW5zCiAg ICAgICAgIFZlY3RvcjxTdHJpbmc+IHBhcmFtcyA9IHBhcmFtTmFtZXM7CiAgICAgICAgIFZlY3Rv cjxTdHJpbmc+IHZhbHVlcyA9IHBhcmFtVmFsdWVzOworI2lmICFPUyhTWU1CSUFOKQogICAgICAg ICBpZiAobWltZVR5cGUgPT0gImFwcGxpY2F0aW9uL3gtc2hvY2t3YXZlLWZsYXNoIikgewogICAg ICAgICAgICAgUVdlYlBhZ2VDbGllbnQqIGNsaWVudCA9IG1fd2ViRnJhbWUtPnBhZ2UoKS0+ZC0+ Y2xpZW50OwogICAgICAgICAgICAgaWYgKCFjbGllbnQgfHwgIXFvYmplY3RfY2FzdDxRV2lkZ2V0 Kj4oY2xpZW50LT5wbHVnaW5QYXJlbnQoKSkpIHsKQEAgLTE0NzEsNiArMTQ3Miw3IEBAIFBhc3NS ZWZQdHI8V2lkZ2V0PiBGcmFtZUxvYWRlckNsaWVudFF0OjoKICAgICAgICAgICAgICAgICAgICAg dmFsdWVzW3dtb2RlSW5kZXhdID0gIm9wYXF1ZSI7CiAgICAgICAgICAgICB9CiAgICAgICAgIH0K KyNlbmRpZgogCiAgICAgICAgIFJlZlB0cjxQbHVnaW5WaWV3PiBwbHVnaW5WaWV3ID0gUGx1Z2lu Vmlldzo6Y3JlYXRlKG1fZnJhbWUsIHBsdWdpblNpemUsIGVsZW1lbnQsIHVybCwKICAgICAgICAg ICAgIHBhcmFtcywgdmFsdWVzLCBtaW1lVHlwZSwgbG9hZE1hbnVhbGx5KTsK