Created attachment 388230 [details] [diff] [review] v1.0 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.
Created attachment 388233 [details] [diff] [review] v1.1 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.
Created attachment 568124 [details] [diff] [review] Updated patch This updated patch removes nsIToolkit, doesn't pass it around in widget creation and removes some android os2 and qt nsToolkit implementations entirely.
Created attachment 568127 [details] [diff] [review] Part 2, extra removed Windows code 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.
Created attachment 570741 [details] [diff] [review] cleanup patch
Checked in patch 'Part 2': https://hg.mozilla.org/integration/mozilla-inbound/rev/ca673b65d7eb