Closed Bug 345251 Opened 18 years ago Closed 18 years ago

Remove Calls to GetObjectProperty on a DOM Window

Categories

(Thunderbird :: General, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3

People

(Reporter: mscott, Assigned: mscott)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Attached patch fix (work in progress) (obsolete) — Splinter Review
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?
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)
getting rid of nsIMsgMessagePaneController works for me.
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)
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? :)
Attached patch updated fixSplinter Review
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
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 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+
(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!
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. 
fixed on the trunk. Let's find out what I broke :)
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3
Blocks: 345791
(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. ;-)
(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. 
Depends on: 350874
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: