Status

()

defect
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: robarnold, Assigned: enndeakin)

Tracking

(Blocks 2 bugs)

Trunk
mozilla10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Posted patch v1.0 (obsolete) — Splinter Review
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)
Posted patch v1.1 (obsolete) — Splinter Review
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)
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?
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.
No longer blocks: 503892
Attachment #388233 - Flags: superreview?(roc)
Attachment #388233 - Flags: review?(vladimir)
Depends on: 508397
Blocks: deCOM
Posted patch Updated patchSplinter Review
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)
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
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).
https://hg.mozilla.org/mozilla-central/rev/3131f4933dd4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Depends on: 697629
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+
Posted patch cleanup patchSplinter Review
Attachment #570741 - Flags: review?(enndeakin)
Attachment #570741 - Flags: review?(enndeakin) → review+
You need to log in before you can comment on or make changes to this bug.