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)

Reporter

Description

10 years ago
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)
Reporter

Comment 1

10 years ago
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)
Reporter

Updated

10 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

10 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

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

Updated

10 years ago
Depends on: 508397
Blocks: deCOM
Assignee

Comment 4

8 years ago
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)
Assignee

Comment 5

8 years ago
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)
Assignee

Updated

8 years ago
Blocks: 71895

Comment 6

8 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).
https://hg.mozilla.org/mozilla-central/rev/3131f4933dd4
Status: ASSIGNED → RESOLVED
Last Resolved: 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)
Assignee

Updated

8 years ago
Attachment #570741 - Flags: review?(enndeakin) → review+
You need to log in before you can comment on or make changes to this bug.