Last Comment Bug 503879 - deCOMtaminate nsIToolkit
: deCOMtaminate nsIToolkit
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Neil Deakin
:
:
Mentors:
Depends on: 503877 508397 697629
Blocks: 71895 deCOM
  Show dependency treegraph
 
Reported: 2009-07-13 09:59 PDT by Rob Arnold [:robarnold]
Modified: 2011-11-21 19:10 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1.0 (87.41 KB, patch)
2009-07-13 09:59 PDT, Rob Arnold [:robarnold]
no flags Details | Diff | Splinter Review
v1.1 (98.27 KB, patch)
2009-07-13 10:08 PDT, Rob Arnold [:robarnold]
no flags Details | Diff | Splinter Review
Updated patch (120.88 KB, patch)
2011-10-19 11:25 PDT, Neil Deakin
roc: review+
Details | Diff | Splinter Review
Part 2, extra removed Windows code (16.13 KB, patch)
2011-10-19 11:26 PDT, Neil Deakin
jmathies: review+
Details | Diff | Splinter Review
cleanup patch (8.06 KB, patch)
2011-10-31 09:56 PDT, Jim Mathies [:jimm]
enndeakin: review+
Details | Diff | Splinter Review

Description Rob Arnold [:robarnold] 2009-07-13 09:59:40 PDT
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.
Comment 1 Rob Arnold [:robarnold] 2009-07-13 10:08:35 PDT
Created attachment 388233 [details] [diff] [review]
v1.1

Forgot to refresh after getting it to compile.
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-07-13 20:52:02 PDT
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 2009-07-14 03:55:59 PDT
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 2011-10-19 11:25:04 PDT
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.
Comment 5 Neil Deakin 2011-10-19 11:26:56 PDT
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 6 Peter Weilbacher 2011-10-20 06:38:55 PDT
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] 2011-10-25 18:08:35 PDT
https://hg.mozilla.org/mozilla-central/rev/3131f4933dd4
Comment 8 Jim Mathies [:jimm] 2011-10-31 09:55:25 PDT
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] 2011-10-31 09:56:17 PDT
Created attachment 570741 [details] [diff] [review]
cleanup patch
Comment 10 Neil Deakin 2011-11-03 13:43:35 PDT
Checked in patch 'Part 2':

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca673b65d7eb
Comment 11 Ed Morley [:emorley] 2011-11-04 03:11:15 PDT
https://hg.mozilla.org/mozilla-central/rev/ca673b65d7eb
Comment 12 Ed Morley [:emorley] 2011-11-21 19:10:49 PST
https://hg.mozilla.org/mozilla-central/rev/fa1aaea9b7e6

Note You need to log in before you can comment on or make changes to this bug.