Last Comment Bug 588895 - Implement getBrowser() in MailNews windows
: Implement getBrowser() in MailNews windows
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: MailNews: Message Display (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1b1
Assigned To: Robert Kaiser
:
Mentors:
Depends on:
Blocks: 386363
  Show dependency treegraph
 
Reported: 2010-08-19 11:59 PDT by Robert Kaiser
Modified: 2010-08-21 13:23 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1: implement it! (1.63 KB, patch)
2010-08-19 12:24 PDT, Robert Kaiser
iann_bugzilla: review+
Details | Diff | Splinter Review

Description Robert Kaiser 2010-08-19 11:59:21 PDT
Thunderbird has a getBrowser() implementation in its mail window, originally introduced for their use of the find bar, but also used with toolkit's zoom manager and potentially others, see http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindow.js#506

We should implement it for the same reasons as well as for Thunderbird add-on compat.

I'll need it for the zoom manager in bug 386363, for one thing.
Comment 1 Robert Kaiser 2010-08-19 12:02:45 PDT
FYI, bug 328795 originally introduced it in Thunderbird.

I already worked around it not being there in bug bug 562339 ending up with http://mxr.mozilla.org/comm-central/source/suite/mailnews/mailContextMenus.js#119 - we should probably changes that one to use getBrowser() as well.
Comment 2 Robert Kaiser 2010-08-19 12:24:20 PDT
Created attachment 467496 [details] [diff] [review]
v1: implement it!

This patch is relatively straight-forward, esp. as that caller it adds right now had exactly the same code in use before. :)
Comment 3 Karsten Düsterloh 2010-08-19 12:41:34 PDT
Comment on attachment 467496 [details] [diff] [review]
v1: implement it!

>+// The zoom manager, view source and possibly some other functions still rely
>+// on the getBrowser function.
>+function getBrowser()
>+{
>+  return GetTabMail() ? GetTabMail().getBrowserForSelectedTab() :
>+                        document.getElementById("messagepane");
>+}

Um, getMessageBrowser() exists...
Comment 4 Robert Kaiser 2010-08-19 12:48:37 PDT
(In reply to comment #3)
> Um, getMessageBrowser() exists...

for one thing, it can't replace getBrowser() as toolkit code doesn't know about anything else but getBrowser. For the other, it doesn't take .getBrowserForSelectedTab() into account, from what I see, and Thunderbird switched away from what getMessageBrowser does in bug 495818 because that returned the wrong browser in some cases.
Comment 5 Ian Neal 2010-08-19 14:13:16 PDT
Comment on attachment 467496 [details] [diff] [review]
v1: implement it!

I'd prefer getBrowser function to be in mailWindowOverlay.js, so r=me with that addressed.
Comment 6 Karsten Düsterloh 2010-08-19 22:51:27 PDT
> > Um, getMessageBrowser() exists...
> 
> for one thing, it can't replace getBrowser() as toolkit code doesn't know about
> anything else but getBrowser. For the other, it doesn't take
> .getBrowserForSelectedTab() into account, from what I see, and Thunderbird
> switched away from what getMessageBrowser does in bug 495818 because that
> returned the wrong browser in some cases.

I meant that you should call getMessageBrowser() (or even gMessageBrowser) instead of touching document.getElementById("messagepane") directly.
Comment 7 Robert Kaiser 2010-08-21 13:23:22 PDT
Pushed as http://hg.mozilla.org/comm-central/rev/071ee24478ff with Ian's and Karsten's nits addressed.

Note You need to log in before you can comment on or make changes to this bug.