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

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

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.

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 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.

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 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.

(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.

(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.

(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.

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 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.

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

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

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