There's no reason to have it be an xpcom interface. There is now an abstract base class mozilla::widget::Toolkit. The widget backends (windows, os2, gtk2, qt and cocoa) still have their nsToolkit class which inherits from it. There's no reason nsToolkit should be instantiated once per thread - we don't handle multiple threads with widget anyways and there was a race condition with initialization. Now there is a global instance and no pretense of thread safety. I also simplified some aspects of the windows and os2 implementation due to needing to support only one instance. I also discovered that all the widget implementations returned NS_OK for nsIToolkit::Init so I moved their code into the constructor. There's more simplification to be done, but it is out of scope for this bug.

Forgot to refresh after getting it to compile.

If we're having a global instance, why keep passing it as a parameter to nsIWidget::Create? and why refcount it? In fact, since the public Toolkit interface has no useful methods, why even expose it at all? Can't we actually completely remove it and make it a per-platform implementation detail?

I just see this bug by chance. If you are doing such code changes to OS/2 stuff, please remember to CC one of the people still working on that, so that we know what is going on and why our build may break.

This updated patch removes nsIToolkit, doesn't pass it around in widget creation and removes some android os2 and qt nsToolkit implementations entirely.

Rob's original patch also removed and changed some Windows specific code. I don't know exactly what it does so I separated these changes out.

Comment on attachment 568124 [details] [diff] [review] Updated patch >diff --git a/widget/src/os2/nsWindow.cpp b/widget/src/os2/nsWindow.cpp >--- a/widget/src/os2/nsWindow.cpp >+++ b/widget/src/os2/nsWindow.cpp >@@ -1,9 +1,9 @@ >-/* vim: set sw=2 sts=2 et cin: */ >+tool/* vim: set sw=2 sts=2 et cin: */ I guess this "tool" is not supposed to be there. Maybe wuno wants to comment on other problems for OS/2 (if there are any).

Comment on attachment 568127 [details] [diff] [review] Part 2, extra removed Windows code Review of attachment 568127 [details] [diff] [review]: ----------------------------------------------------------------- Some follow up in a patch I'll post next. ::: widget/src/windows/nsWindow.cpp @@ +4672,5 @@ > + // The Win32 toolkit normally only sends these events to top-level windows. > + // But we cycle through all of the childwindows and send it to them as well > + // so all presentations get notified properly. > + // See nsWindow::GlobalMsgWindowProc. > + DispatchStandardEvent(NS_SYSCOLORCHANGED); Please break this out into an OnSysColorChanged handler. We're trying to minimize the amount of code we have in the main event case statement.

Checked in patch 'Part 2': https://hg.mozilla.org/integration/mozilla-inbound/rev/ca673b65d7eb