503879 - deCOMtaminate nsIToolkit

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Rob Arnold [:robarnold]]

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

Comment 1

[Rob Arnold [:robarnold]]

Attachment: v1.1

Forgot to refresh after getting it to compile.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 3

[Peter Weilbacher]

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.

Comment 4

[Neil Deakin]

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

Comment 5

[Neil Deakin]

Attachment: 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 6

[Peter Weilbacher]

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 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3131f4933dd4

Comment 8

[Jim Mathies [:jimm]]

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.

Comment 9

[Jim Mathies [:jimm]]

Attachment: cleanup patch

Comment 10

[Neil Deakin]

Checked in patch 'Part 2':

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca673b65d7eb

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ca673b65d7eb

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fa1aaea9b7e6