Note: There are a few cases of duplicates in user autocompletion which are being worked on.

deCOMtaminate nsIToolkit

RESOLVED FIXED in mozilla10

Status

()

Core
Widget
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: robarnold, Assigned: Neil Deakin (not available until Aug 9))

Tracking

(Blocks: 2 bugs)

Trunk
mozilla10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
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.
Attachment #388230 - Flags: superreview?(roc)
Attachment #388230 - Flags: review?(vladimir)
(Reporter)

Comment 1

8 years ago
Created attachment 388233 [details] [diff] [review]
v1.1

Forgot to refresh after getting it to compile.
Attachment #388230 - Attachment is obsolete: true
Attachment #388233 - Flags: superreview?(roc)
Attachment #388233 - Flags: review?(vladimir)
Attachment #388230 - Flags: superreview?(roc)
Attachment #388230 - Flags: review?(vladimir)
(Reporter)

Updated

8 years ago
Blocks: 503892
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

8 years ago
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.
(Reporter)

Updated

8 years ago
No longer blocks: 503892
Attachment #388233 - Flags: superreview?(roc)
Attachment #388233 - Flags: review?(vladimir)
(Reporter)

Updated

8 years ago
Depends on: 508397

Updated

8 years ago
Blocks: 105431
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.
Assignee: tellrob → enndeakin
Attachment #388233 - Attachment is obsolete: true
Attachment #568124 - Flags: review?(roc)
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.
Attachment #568127 - Flags: review?(jmathies)
Blocks: 71895
Attachment #568124 - Flags: review?(roc) → review+

Comment 6

6 years ago
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

6 years ago
https://hg.mozilla.org/mozilla-central/rev/3131f4933dd4
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10

Updated

6 years ago
Depends on: 697629

Comment 8

6 years ago
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.
Attachment #568127 - Flags: review?(jmathies) → review+

Comment 9

6 years ago
Created attachment 570741 [details] [diff] [review]
cleanup patch
Attachment #570741 - Flags: review?(enndeakin)
Attachment #570741 - Flags: review?(enndeakin) → review+
Checked in patch 'Part 2':

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca673b65d7eb
https://hg.mozilla.org/mozilla-central/rev/ca673b65d7eb
https://hg.mozilla.org/mozilla-central/rev/fa1aaea9b7e6
You need to log in before you can comment on or make changes to this bug.