Remove Calls to GetObjectProperty on a DOM Window

RESOLVED FIXED in Thunderbird 3

Status

Thunderbird
General
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Scott MacGregor, Assigned: Scott MacGregor)

Tracking

Thunderbird 3
x86
Windows XP
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

11 years ago
Johnny was talking to me about removing the private DOM Window API: GetObjectProperty. 

There are only three consumers and they all live in mail:

http://lxr.mozilla.org/mozilla/source/mailnews/base/src/nsMessengerWinIntegration.cpp#155
http://lxr.mozilla.org/mozilla/source/mailnews/base/src/nsMsgWindow.cpp#374
http://lxr.mozilla.org/mozilla/source/mailnews/base/src/nsMessengerUnixIntegration.cpp#98 

I think we should be able to do this pretty easily.
(Assignee)

Comment 1

11 years ago
Created attachment 229925 [details] [diff] [review]
fix (work in progress)

I haven't fully tested this patch out yet (nor have I tried to test it on Linux). Currently activateWindow in nsMessengerWinIntegration isn't called by the new mail alert notification yet so I couldn't verify that specific change.

This patch adds an attribute to nsIMsgWindow for getting the window commands object. I also made the domwindowinternal an attribute, replacing SetDOMWindow. Callers of GetObjectProperty in mailnews now go through the mail session to get the topmost nsIMsgWindow object. From there, they can get the window command object or get the dom window. 

Johnny, I assume you are looking to make the DOM API change on the trunk only? So I don't have to worry about changing the nsIMsgWindow interface?
(Assignee)

Comment 2

11 years ago
Comment on attachment 229925 [details] [diff] [review]
fix (work in progress)

David, see comment 1 for more details.

I'm also tempted to get rid of nsIMsgMessagePaneController and move its one method into nsIMsgWindowCommands so we only have one interface for the backend to use when it wants to manipulate the front end for loading a folder, clearing the message pane, etc. 

If you agree with that, I'll attach a new patch instead of making you review this one.
Attachment #229925 - Flags: superreview?(bienvenu)
Attachment #229925 - Flags: review?(jst)

Comment 3

11 years ago
getting rid of nsIMsgMessagePaneController works for me.
(Assignee)

Comment 4

11 years ago
Comment on attachment 229925 [details] [diff] [review]
fix (work in progress)

clearing review requests. I'm going to do some more cleanup here.
Attachment #229925 - Flags: superreview?(bienvenu)
Attachment #229925 - Flags: review?(jst)
Created attachment 230015 [details] [diff] [review]
Remove the last place that uses GetObjectProperty() too.

There's one more place in the source that uses GetObjectProperty(), and that code gets the property "MsgStatusFeedback". This one's easier to remove since the code that sets up MsgStatusFeedback also sets the same object to be the XULBrowserWindow. This is untested, but should work. Scott, wanna roll this into your patch and give this some testing too? :)
(Assignee)

Comment 6

11 years ago
Created attachment 230053 [details] [diff] [review]
updated fix

In addition to the previous comments, this patch:

1) Combines nsIMsgMessagePaneController and nsIMsgWindowCommands into a single object.

2) Removes SelectFolder and SelectMessage from nsIMsgWindow, force callers to get the nsIMsgWindowCommands object from the msg window.

3) I ended up taking a stick to nsIMsgStatusFeedback.idl in order to fix Johnny's last caller of GetObjectProperty without going through the xul browser window. We now explicitly pass in our JS implementation of status feedback into the C++ implementation using setWrappedStatusFeedback. This allowed me to get rid of several methods in this interface.
Attachment #229925 - Attachment is obsolete: true
(Assignee)

Comment 7

11 years ago
Comment on attachment 230053 [details] [diff] [review]
updated fix

What do you think of this patch David? I'm not sure about the name: setWrappedStatusFeedback...

See the comments earlier in the bug for a summary of all the changes.
Attachment #230053 - Flags: superreview?(bienvenu)

Comment 8

11 years ago
Comment on attachment 230053 [details] [diff] [review]
updated fix

that touched a lot! Do you think we might be breaking any extensions with the nsMsgWindowCommands/nsMessagePaneController changes?
Attachment #230053 - Flags: superreview?(bienvenu) → superreview+
(Assignee)

Comment 9

11 years ago
(In reply to comment #8)
> (From update of attachment 230053 [details] [diff] [review] [edit])
> that touched a lot! Do you think we might be breaking any extensions with the
> nsMsgWindowCommands/nsMessagePaneController changes?
> 

I believe this is for the trunk only, jump in if I'm wrong Johnny! So it should be ok to change these interfaces.
(In reply to comment #9)
> I believe this is for the trunk only, jump in if I'm wrong Johnny! So it should
> be ok to change these interfaces.

Yes, trunk only for sure. Thanks for cleaning this up Scott!
(Assignee)

Comment 11

11 years ago
cc'ing karsten to keep him in the loop, although I believe I fixed all of the call spots in seamonkey that changed from the API changes to nsIMsgWindow and nsIMsgStatusFeedback. 
(Assignee)

Comment 12

11 years ago
fixed on the trunk. Let's find out what I broke :)
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3

Updated

11 years ago
Blocks: 345791

Comment 13

11 years ago
(In reply to comment #12)
> fixed on the trunk. Let's find out what I broke :)
> 
Probably sending Messages for Windows-Trunk-Builds, see Bug 346055. ;-)
(Assignee)

Comment 14

11 years ago
(In reply to comment #13)
> (In reply to comment #12)
> > fixed on the trunk. Let's find out what I broke :)
> > 
> Probably sending Messages for Windows-Trunk-Builds, see Bug 346055. ;-)
> 

I doubt this caused Bug 346055. APIs for sending mail changed on the trunk only for another bug and you use enigmail which is no longer compatible with those APIs. 
(Assignee)

Updated

11 years ago
Depends on: 350874
You need to log in before you can comment on or make changes to this bug.