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] (PTO until February 27)
: instant-messaging
:
Mentors:
Depends on:
Blocks: 763522
  Show dependency treegraph
 
Reported: 2012-07-18 08:02 PDT by Florian Quèze [:florian] [:flo] (PTO until February 27)
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] (PTO until February 27)
mconley: review+
bwinton: approval‑comm‑aurora+
bwinton: approval‑comm‑beta+
Details | Diff | Splinter Review
Same patch unbitrotted (15.27 KB, patch)
2012-07-23 13:15 PDT, Florian Quèze [:florian] [:flo] (PTO until February 27)
no flags Details | Diff | Splinter Review

Description User image Florian Quèze [:florian] [:flo] (PTO until February 27) 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 User image Mike Conley (:mconley) 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 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 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 User image Mike Conley (:mconley) 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 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 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 User image Mike Conley (:mconley) 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 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 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 User image Blake Winton (:bwinton) (:☕️) 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 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 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.