Status

()

defect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: robarnold, Assigned: robarnold)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Assignee

Description

10 years ago
Posted patch win32 patchSplinter Review
It appears to be unused and unnecessary for OS/2 and Windows.
Attachment #388226 - Flags: review?(vladimir)
Assignee

Updated

10 years ago
Blocks: 503879
Assignee

Comment 1

10 years ago
Posted patch os/2 patchSplinter Review
Also want to do this to os/2 (which is similar to the windows widget backend) so that bug 503879 can remove more code.
Assignee: nobody → tellrob
Status: NEW → ASSIGNED
Attachment #388231 - Flags: review?(vladimir)
How sure are you that this can go away?  Would we ever receive a destroy message on the wrong thread?

(Have you looked at where this code came from?)
Assignee

Comment 3

10 years ago
This code has been there since day 1. See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/windows/nsWindow.cpp&rev=1.1&root=/cvsroot

I don't think we ever get calls to widget on other threads, given how thread adverse xpcom (nsIToolkit/nsIWidget) and the rest of the codebase are. Also, this interface doesn't exist on the newer cocoa and gtk2 backends so I suspect it has not been used or needed for quite some time.
Assignee

Updated

10 years ago
Attachment #388231 - Flags: review?(vladimir) → review?(mozilla)

Comment 4

10 years ago
Comment on attachment 388231 [details] [diff] [review]
os/2 patch

> MRESULT EXPENTRY nsToolkitWindowProc(HWND hWnd, ULONG msg, MPARAM mp1,
>                                      MPARAM mp2)
> {
>-    switch (msg) {
>-        case WM_CALLMETHOD:
>-        {
>-            MethodInfo *info = (MethodInfo *)mp2;
>-            return (MRESULT)info->Invoke();
>-        }
>-    }
>+    // XXX: is this needed anymore?
>     return ::WinDefWindowProc(hWnd, msg, mp1, mp2);

I don't think it is still needed. I completely removed this function and used NULL instead in the class registration call. That didn't seem to change anything.

>--- a/widget/src/os2/nsWindow.h
>+++ b/widget/src/os2/nsWindow.h
[...]
> class nsWindow : public nsBaseWidget,
>-                 public nsSwitchToUIThread

Please remove the comma at the end of the line.

Comment 5

10 years ago
Rich, do you have a deeper understanding of the nsToolkit class on OS/2 to comment on the necessity of the nsToolkitWindowProc() function?

I have only run an OS/2 build with this patch for 20 minutes or so, and I'm a bit surprised that it didn't seem to have any effect. But if it works, this is great cleanup! :-)

Comment 6

10 years ago
(In reply to comment #5)
> Rich, do you have a deeper understanding of the nsToolkit class on
> OS/2 to comment on the necessity of the nsToolkitWindowProc() function?

Not really.  I did some basic testing (viewed various pages & dialogs, dragged tabs off to open a new window, watched Flash & Ogg videos, etc) and never once saw the toolkit window proc invoked.  I also confirmed that Firefox never creates more than one message queue (on thread 1, as expected).

Some comments on the patch:
- the OS/2 nsToolkitWindowProc() is no longer used & should be eliminated
- it might not be a bad idea to include an assertion or whatever in Create() and Destroy() to ensure that they're being invoked on thread 1 (I assume that TID 1 will remain the one & only GUI thread).
- to forestall future problems, it should be clearly documented (especially in the Win32 version) that the GUI functions formerly handled by toolkit must be invoked from thread 1 only
Assignee

Comment 7

10 years ago
(In reply to comment #6)
> Some comments on the patch:
> - the OS/2 nsToolkitWindowProc() is no longer used & should be eliminated

Will do, gladly.

> - it might not be a bad idea to include an assertion or whatever in Create()
> and Destroy() to ensure that they're being invoked on thread 1 (I assume that
> TID 1 will remain the one & only GUI thread).

I'm not sure if this is necessary. All of widget/ (well ok, maybe not BeOS) is written to assume a single thread and it would probably not be beneficial to write this assert.

> - to forestall future problems, it should be clearly documented (especially in
> the Win32 version) that the GUI functions formerly handled by toolkit must be
> invoked from thread 1 only

I think it's just understood by convention/a lack of locks (this is true for most of the mozilla codebase). All our widget backends work this way. This code is buggy anyways - there's a race condition in the initialization of the TLS index for nsToolkit.
Assignee

Updated

10 years ago
Blocks: 508397
Assignee

Comment 8

10 years ago
Windows patch pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/da889626b008

OS/2 patch and bug expanded to bug 508397
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Attachment #388231 - Flags: review?(mozilla)
You need to log in before you can comment on or make changes to this bug.