Last Comment Bug 775105 - Presence information in email headers isn't shown correctly when there are multiple email windows
: Presence information in email headers isn't shown correctly when there are mu...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Instant Messaging (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 17.0
Assigned To: Florian Quèze [:florian] [:flo]
: instant-messaging
Mentors:
Depends on:
Blocks: 763522
  Show dependency treegraph
 
Reported: 2012-07-18 08:02 PDT by Florian Quèze [:florian] [:flo]
Modified: 2012-07-23 13:38 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed


Attachments
Patch (15.28 KB, patch)
2012-07-18 08:02 PDT, Florian Quèze [:florian] [:flo]
mconley: review+
bwinton: approval‑comm‑aurora+
bwinton: approval‑comm‑beta+
Details | Diff | Review
Same patch unbitrotted (15.27 KB, patch)
2012-07-23 13:15 PDT, Florian Quèze [:florian] [:flo]
no flags Details | Diff | Review

Description Florian Quèze [:florian] [:flo] 2012-07-18 08:02:25 PDT
Created attachment 643376 [details] [diff] [review]
Patch

The presence information in email headers added in bug 763522 doesn't work at all for standalone message windows, and shows as online only the contact that signed on after the 3pane window was opened, so it's unreliable for windows that haven't been opened at startup.

The patch also fixes another issue related to having multiple 3pane windows: the OAuth dialog to authenticate with twitter was shown once per 3pane window.
Comment 1 Mike Conley (:mconley) - (Away until June 29th) 2012-07-18 11:35:36 PDT
So, I'm in the process of reviewing this patch, but I can't seem to get the presence info in mail headers with or without this patch, on Linux.

Is this feature working on Linux?
Comment 2 Florian Quèze [:florian] [:flo] 2012-07-18 11:45:56 PDT
I don't know any reason why this wouldn't work on Linux.
You need to have a Gtalk account connected with at least one online contact, and look at an email coming from that contact (ie either the email address used to send the email is the gtalk address, or there's an address book card associated with the email address, and that address book card contains the gtalk address of the person).
Comment 3 Mike Conley (:mconley) - (Away until June 29th) 2012-07-19 12:12:46 PDT
Comment on attachment 643376 [details] [diff] [review]
Patch

Review of attachment 643376 [details] [diff] [review]:
-----------------------------------------------------------------

Mind you, I have limited chat component experience, so I may have missed something...but this looks pretty reasonable.

Just two points, see below.

::: mail/base/content/msgHdrViewOverlay.js
@@ +1517,5 @@
>    let prplConv = emailAddressNode.chatContact.createConversation();
>    let uiConv = Services.conversations.getUIConversation(prplConv);
> +
> +  let win = window;
> +  if (!("focusConversation" in chatHandler)) {

Can we ever get into a state where this is called before UpdateEmailNodeDetails, such that chatHandler does not exist?

::: mail/components/im/content/chat-messenger-overlay.js
@@ +88,4 @@
>        return;
>  
> +    if (aArgs.convType == "focus") {
> +      chatHandler.focusConversation(aArgs.conv);

Should we be checking for aArgs.conv on lines 86-87 too?
Comment 4 Florian Quèze [:florian] [:flo] 2012-07-20 05:19:43 PDT
(In reply to Mike Conley (:mconley) from comment #3)
> Comment on attachment 643376 [details] [diff] [review]

> ::: mail/base/content/msgHdrViewOverlay.js
> @@ +1517,5 @@
> >    let prplConv = emailAddressNode.chatContact.createConversation();
> >    let uiConv = Services.conversations.getUIConversation(prplConv);
> > +
> > +  let win = window;
> > +  if (!("focusConversation" in chatHandler)) {
> 
> Can we ever get into a state where this is called before
> UpdateEmailNodeDetails, such that chatHandler does not exist?

This code (onClickEmailPresence) is an event handler on a node that is shown only if UpdateEmailNodeDetails has set the chatStatus attribute, so no.
So I don't mind the code causing a JS error if chatHandler isn't defined; that would tell us that something went horribly wrong.

> ::: mail/components/im/content/chat-messenger-overlay.js
> @@ +88,4 @@
> >        return;
> >  
> > +    if (aArgs.convType == "focus") {
> > +      chatHandler.focusConversation(aArgs.conv);
> 
> Should we be checking for aArgs.conv on lines 86-87 too?

The only reason to check for it would be to throw an exception when it's missing or null. Isn't the current code going to cause a JS error anyway?
Comment 5 Mike Conley (:mconley) - (Away until June 29th) 2012-07-20 06:36:55 PDT
Comment on attachment 643376 [details] [diff] [review]
Patch

Review of attachment 643376 [details] [diff] [review]:
-----------------------------------------------------------------

Good points, both. r=me.
Comment 6 Florian Quèze [:florian] [:flo] 2012-07-20 07:02:37 PDT
Comment on attachment 643376 [details] [diff] [review]
Patch

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):
Comment 7 Blake Winton (:bwinton) (:☕️) (PTO 'til London. Find me there for quick answers!) 2012-07-23 11:48:58 PDT
Comment on attachment 643376 [details] [diff] [review]
Patch

Yeah, I really think we want to have this on aurora/beta.
Comment 8 Florian Quèze [:florian] [:flo] 2012-07-23 13:15:39 PDT
Created attachment 645039 [details] [diff] [review]
Same patch unbitrotted

Fix the bitrot from bug 767155.

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