Bug 72358 – [Qt] Mouse move event not working with Flash 11 on Mac

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Titta Heikkala]

Overview:
Mouse move events aren't forwarded to the Flash Player 11 from Webkit on Mac OS. 

Steps to reproduce:
For example, this page: http://www.kirupa.com/developer/mx/animation/slider_tutorial_part2.swf works fine with Flash Player 10.3.183.11, but not with the latest Flash Player 11 (11.1.102.55) when viewed with fancybrowser. Attached also a file that has the page as src to view it with fancybrowser.

Actual results:
With the Flash Player 11 the slider won't work.

Expected results:
The slider can be moved.

Environment:
Mac OS X, Qt 4.7.4 (configured with -universal)

[Titta Heikkala]

Created attachment 115106 [details]
html file for testing

[Alessandro La Piana]

I was able to reproduce it on Mac OS X 10.7.2

[Anton Patiev]

Reproduced on the following environment:
* Mac OS X 10.6
* Qt 4.7.4
* Flash Player 11.0.1.152

[Anton Patiev]

Also reproduced on QtWebKit-2.2.0 using QtTestBrowser (/Tools/Scripts/run-launcher --qt).

[Alexis Menard (darktears)]

*** Bug 72014 has been marked as a duplicate of this bug. ***

[Alessandro La Piana]

After a more detailed analysis it seems the problem is a regression in Flash Player 11 that does not handle the the null event in the Carbon even model that, apparently, is the only way used to track the mouse movement. Since the Carbon event model is deprecated it's unlikely Adobe will fix this. 
It will probably be necessary to rewrite PluginViewMac.mm switching to Cocoa event model.

Created attachment 164361 [details]
Preliminary fix for the problem based on Qt 4.8

Additionally this patch solves a number of issues regarding indirect rendering, signal emission to indicate the frame needs repainting and 64bit support as well as Cocoa support on Mac.

[Simon Hausmann]

Comment on attachment 164361 [details]
Preliminary fix for the problem based on Qt 4.8

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

Nice. Here is some initial feedback :). I suggest to rebase this against webkit trunk for official submission with ChangeLog and corrected paths (no src/3rdparty/webkit). You can also use Tools/Scripts/check-webkit-style to do some rudimentary style checking.

> src/3rdparty/webkit/Source/WebCore/plugins/PluginView.cpp:1282
> +static const char* ChromeUserAgent = "Mozilla/5.0 ("

This should be static const char* const ChromeUserAgent or static const char ChromeUserAgent[] - either way the variable should be const, too, not only the characters it points to.

> src/3rdparty/webkit/Source/WebCore/plugins/PluginView.h:233
> +        bool popUpContextMenu(NPMenu *menu);

Coding style nitpick: The * should be placed near the type instead of the name.

> src/3rdparty/webkit/Source/WebCore/plugins/mac/PluginViewMac.mm:154
> +static void InitializeNPCocoaEvent(NPCocoaEvent* event)

Initialize* -> initialize*

> src/3rdparty/webkit/Source/WebCore/plugins/mac/PluginViewMac.mm:156
> +  memset(event, 0, sizeof(NPCocoaEvent));

Incorrect level of indentation.

> src/3rdparty/webkit/Source/WebCore/plugins/mac/PluginViewMac.mm:402
> +        if (NPEventModelCocoa != m_eventModel)

I think m_eventModel != NPEventModelCocoa would be more consistent in style than NPEventModelCocoa != m_eventModel :)

> src/3rdparty/webkit/Source/WebCore/plugins/mac/PluginViewMac.mm:482
> +        NP_CGContext *miniContext = new NP_CGContext;
> +        miniContext->context = this->m_contextRef;
> +        m_npWindow.window = (void*)miniContext;

Why can't you use m_npCgContext here?

> src/3rdparty/webkit/Source/WebCore/plugins/mac/PluginViewMac.mm:558
> +    QPainter *p = context->platformContext();

Coding style nitpick: * placement

> src/3rdparty/webkit/Source/WebCore/plugins/mac/PluginViewMac.mm:568
> +        if (!cgContext) {
> +            return;
> +        } else {

Single line bodies don't use braces.

> src/3rdparty/webkit/Source/WebCore/plugins/mac/PluginViewMac.mm:574
> +    } else {
> +        m_usePixmap = false;
> +    }

Ditto.

> src/3rdparty/webkit/Source/WebCore/plugins/mac/PluginViewMac.mm:773
> +        NSEvent *currentEvent = [NSApp currentEvent];

Coding style nitpick: * placement

Created attachment 164714 [details]
Patch against trunk

Followed the steps to build and test this against trunk, I think that I covered everything.

[WebKit Review Bot]

Attachment 164714 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/plugins/npapi.h:418:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/plugins/npapi.h:420:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/ChangeLog:4:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 5 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 164720 [details]
Fixed patch with code style changes against trunk

The npapi.h file has a couple of code style oddities because it is done to match the rest of the file.  If this should change then I can do that.

[WebKit Review Bot]

Attachment 164720 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/plugins/npapi.h:418:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/plugins/npapi.h:420:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jiang Jiang]

Regarding the title "[Qt] Implement support for Cooca based NAPAI plugins on Mac", should it be NPAPI?

Good point, that is a typo on my side, I can rectify that quickly enough.

[Jiang Jiang]

(In reply to comment #14)
> Good point, that is a typo on my side, I can rectify that quickly enough.

And Cooca -> Cocoa as well :)

Created attachment 165348 [details]
Updated patch with the typos in the change log fixed

[WebKit Review Bot]

Attachment 165348 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/plugins/npapi.h:418:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/plugins/npapi.h:420:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Bobber]

Hi Andy,

I applyied your patch qtwebkit_23 and changed code
'JSC::JSLock::DropAllLocks dropAllLocks(JSC::SilenceAssertionsOnly); ' in Source/WebCore/plugins/mac/PluginViewMac.mm to 'JSC::JSLock::DropAllLocks dropAllLocks(JSDOMWindowBase::commonJSGlobalData());' because I got error that JSC::SilenceAssertionsOnly is not found on Mac. page: http://www.kirupa.com/developer/mx/animation/slider_tutorial_part2.swf works fine now. Could you please confirmed the change is ok?

Thanks,
Bobber

Bobber: I am afraid I don't know as I don't know that part of webkit well enough, maybe someone else can comment on that.

[Simon Hausmann]

Comment on attachment 165348 [details]
Updated patch with the typos in the change log fixed

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

> Source/WebCore/plugins/mac/PluginViewMac.mm:1078
> +    JSC::JSLock::DropAllLocks dropAllLocks(JSC::SilenceAssertionsOnly);

Will fix this when landing, the API was changed just recently.

[Simon Hausmann]

Committed r136124: <http://trac.webkit.org/changeset/136124>

[Bobber]

Hi guys, could you please check bug 113553?