540052 - [OOPP] Test failure in test_plugin_focus.html

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Benjamin Smedberg]

s: win32-slave5364654 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_plugin_focus.html | Test timed out.

Comment 1

[Jim Mathies [:jimm]]

Latest runs seem to indicate this has gone away. Was this a sporadic failure? FWIW, I can't reproduce locally, the test succeeds.

Comment 2

[Benjamin Smedberg]

The last e10s push was 6f003829a096
Jim Mathies – Bug 539955 - [OOPP] unit test failure in test_plugin_mouse_coords.html. r=bsmedberg. default tip

and it produced this failure, see the log: http://tinderbox.mozilla.org/showlog.cgi?log=Electrolysis/1263601541.1263604010.26516.gz

If you're looking at mozilla-central, you won't see this, because it only appears with the IPC pref on.

Comment 3

[Jim Mathies [:jimm]]

Attachment: patch

Comment 4

[Jim Mathies [:jimm]]

Attachment: simple test case

Comment 5

[Jim Mathies [:jimm]]

Attachment: child-parent focus sync patch v.1

All in all, this ended up boiling down into something pretty non-invasive. Child and parent communicate focus changes via ipc. To keep the dom synced, we send a custom windows event down to widget from the parent instance, which posts a plugin activate gui event to the event & focus managers.

Comment 6

[Jim Mathies [:jimm]]

Attachment: child-parent focus sync patch v.2

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 423362 [details] [diff] [review]
child-parent focus sync patch v.2

Updates based on our discussion last Friday.  I'm using send message as well. While testing this time I didn't experience any lock ups during the send, but I wonder if there's still a possibility. I'm working on bug 541362 next so I'll be doing more testing w/this patch applied. We'll see what comes up.

Comment 8

[Jim Mathies [:jimm]]

Also, I decided not to use nsIWidget - changes to a cross-platform interface for a win specific, oopp specific method didn't make a lot of sense, and send message required less code / changes.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 423362 [details] [diff] [review]
child-parent focus sync patch v.2

>+PluginInstanceChild::PluginFocus()
>+{
>+  CallPluginGotFocus();
>+}

Does that need to be a separate method? Why not just call directly from the wndproc?

>+PluginInstanceChild::AnswerSetPluginFocus()

In general I prefer one method with inner #ifdef, not two separate methods. Makes mxr and reading more difficult imo.

>+    // Win specific

That's not true, really... Maybe clarify?

>+static const PRUnichar kMozOOPPPluginFocusEventId[] = L"MozOOPPPluginFocus";

Rather than do this how about we do the same thing as in WindowsMessageLoop? Something like:

  UINT gEventLoopMessage =
    RegisterWindowMessage(L"SyncChannel Windows Message Loop Message");

Then it would only be called once instead of with each new instance.

>+        HookPluginWindow(reinterpret_cast<HWND>(aWindow->window));
>+        

Unnecessary spaces instead of empty line.

>+    FM calls on widget to set the focus on the window. We pick up the resulting wm_setfocus

Super nit, ca you say "focus manager" instead of FM?

>+static const TCHAR kPluginInstanceParentProperty[] = TEXT("PluginInstanceParentProperty");

I think we've decided to ditch TCHAR for PRUnichar/wchar_t and explicitly use the L"" and W-methods.

>+    switch (message) {
>+        case WM_SETFOCUS:
>+        self->CallSetPluginFocus();
>+        break;

Nit, add a newline here to break this up.

>+    LRESULT res = ::CallWindowProc(self->mPluginWndProc, hWnd, message, wParam,
>+                                   lParam);
>+
>+    return res;

Kill unused |res| variable?

>+PluginInstanceParent::HookPluginWindow(HWND aWnd)

I think we should rename this to "SubclassPluginWindow" since we do have hooks lurking and there's no reason to confuse the terms ;)

>+    if (aWnd != mPluginHWND)
>+        UnhookPluginWindow(aWnd);

Is this a typo? Did you mean to unhook mPluginHWND and then null it?

Also, do we want to support passing multiple different windows in? Maybe we should assert that we only subclass once, and on the same window?

>+        mPluginWndProc = 
>+            (WNDPROC)::SetWindowLongPtrA(mPluginHWND, GWLP_WNDPROC,
>+                                         reinterpret_cast<LONG>(PluginWindowHookProc));
>+
>+        ::SetProp(mPluginHWND, kPluginInstanceParentProperty, this);

I'd assert the mPluginWndProc result and that SetProp succeeds.

>+PluginInstanceParent::UnhookPluginWindow(HWND aWnd)

"UnsubclassPluginWindow"? And again, not sure we should support passing in a different window here. At the very least we should have some assertions rather than silently doing nothing.

>+PluginInstanceParent::AnswerPluginGotFocus()

NS_NOTREACHED too so we noisily assert.

>+static const PRUnichar kMozOOPPPluginFocusEventId[] = L"MozOOPPPluginFocus";

Here again an inline call to RegisterMessage should be fine

>+#ifdef MOZ_IPC
>+    if (msg == sOOPPPluginFocusEvent) {

I wonder... Could we avoid duplicating code here by doing this:

  if (msg == sOOPPPluginFocusEvent) {
    ::SendMessage(mWnd, WM_MOUSEACTIVATE, 0, 0);
  }

Seems like that would then call the subclass in nsPluginNativeWindowWin and generate the event.

Comment 10

[Jim Mathies [:jimm]]

(In reply to comment #9)
> (From update of attachment 423362 [details] [diff] [review])
> 
> >+    if (aWnd != mPluginHWND)
> >+        UnhookPluginWindow(aWnd);
> 
> Is this a typo? Did you mean to unhook mPluginHWND and then null it?

yep, my mistake. I'm surprised I didn't get a crash at some point, my assumptiuon was...

> Also, do we want to support passing multiple different windows in? Maybe we
> should assert that we only subclass once, and on the same window?

Isn't it possible for the window to be destroyed and a new one to take it's place? Say if you switched from windowed to windowless and back again? I'm not sure honestly, but I figured I'd cover all the bases.

Comment 11

[Jim Mathies [:jimm]]

(In reply to comment #9)
> (From update of attachment 423362 [details] [diff] [review])
> >+#ifdef MOZ_IPC
> >+    if (msg == sOOPPPluginFocusEvent) {
> 
> I wonder... Could we avoid duplicating code here by doing this:
> 
>   if (msg == sOOPPPluginFocusEvent) {
>     ::SendMessage(mWnd, WM_MOUSEACTIVATE, 0, 0);
>   }
> 
> Seems like that would then call the subclass in nsPluginNativeWindowWin and
> generate the event.

Not sure I like this, the code in nsPluginNativeWindowWin is disconnected, and feels to me as if it's quickly becoming obsolete with oopp. Are three lines of code really worth calling all the way up into nsPluginNativeWindowWin just to call back down into widget? Sending the event from widget seemed a more reliable approach.

Comment 12

[Jim Mathies [:jimm]]

(In reply to comment #11)
> (In reply to comment #9)
> > (From update of attachment 423362 [details] [diff] [review] [details])
> > >+#ifdef MOZ_IPC
> > >+    if (msg == sOOPPPluginFocusEvent) {
> > 
> > I wonder... Could we avoid duplicating code here by doing this:
> > 
> >   if (msg == sOOPPPluginFocusEvent) {
> >     ::SendMessage(mWnd, WM_MOUSEACTIVATE, 0, 0);
> >   }
> > 
> > Seems like that would then call the subclass in nsPluginNativeWindowWin and
> > generate the event.
> 

Just tested and this works as well, so I'm not going to be particularly picky.

Comment 13

[Jim Mathies [:jimm]]

Attachment: child-parent focus sync patch v.3

This should address everything you brought up. FYI, I went with the nsNativeWindow code and just updated the comments so devs know the code is critical to OOPP.

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 423524 [details] [diff] [review]
child-parent focus sync patch v.3

>+PluginInstanceChild::AnswerSetPluginFocus()

I'm a little worried that this may trip some dead-code removal tool, how about putting the unimplemented part in an #else block?

>+  plugin focus changes between processes
> ...
>+static const PRUnichar kPluginInstanceParentProperty[] = L"PluginInstanceParentProperty";

Nit, can you rewrap to keep this under 80 chars? You've got some assertions that go too long, also.

>+PluginInstanceParent::SubclassPluginWindow(HWND aWnd)

Assert that SetWindowLongPtr and SetProp succeed.

>+PluginInstanceParent::AnswerPluginGotFocus()

Same concern about dead-code identification

>+    PRUint32           mOOPPPluginFocusEvent;

This is no longer needed.

>+PRUint32        nsWindow::sOOPPPluginFocusEvent   =
>+                  RegisterWindowMessageW(L"OOPP Plugin Focus Widget Event");

It's unfortunate that we have to do this again. Is there no way to share this with plugin code?

r=me with those things addressed, regardless of whether we share the message id.

Comment 15

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/6a942d99ce1e

Comment 16

[Benjamin Smedberg]

and http://hg.mozilla.org/mozilla-central/rev/528c0d576acb

Comment 17

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/04af7ba7bc8e

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 19

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430