Last Comment Bug 814414 - Menu entries for chat still shown in AppMenu when mail.chat.enabled set to false
: Menu entries for chat still shown in AppMenu when mail.chat.enabled set to false
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Toolbars and Tabs (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 20.0
Assigned To: Richard Marti (:Paenglab)
:
Mentors:
Depends on:
Blocks: TB-AppMenu
  Show dependency treegraph
 
Reported: 2012-11-22 06:50 PST by Serg Iv
Modified: 2012-12-13 14:44 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wontfix
fixed
fixed
17+
fixed


Attachments
patch (1.27 KB, patch)
2012-11-23 10:09 PST, Richard Marti (:Paenglab)
no flags Details | Diff | Splinter Review
patch v2 (2.20 KB, patch)
2012-11-23 13:42 PST, Richard Marti (:Paenglab)
florian: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑esr17+
Details | Diff | Splinter Review

Description Serg Iv 2012-11-22 06:50:22 PST
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:15.0) Gecko/20100101 Firefox/15.0.1
Build ID: 20120905151427

Steps to reproduce:

mail.chat.enabled to false


Actual results:

Appmenu entries "Go -> Chat" and "Tools -> Chat status" still here


Expected results:

No menu entries for chats.
Comment 1 Thomas D. (currently busy elsewhere; needinfo?me) 2012-11-22 08:29:16 PST
confirming per BWinton's Bug 650170 Comment 99.
Comment 2 Richard Marti (:Paenglab) 2012-11-23 10:09:55 PST
Created attachment 684734 [details] [diff] [review]
patch

This patch removes the menuitems.

I tried also to combine two menuitems like you suggested in bug 650170 comment 100, but this didn't work:

  setStatusMenupopupCommand: function(aEvent) {
    let target = aEvent.originalTarget;
    if (target.getAttribute("id") == "imStatusShowAccounts" || "appmenu_imStatusShowAccounts") {
      openIMAccountMgr();
      return;
    }

Instead of the status change only the IMAccountMgr opens.

Florian, what do you think, is this patch safe for 17ESR?
Comment 3 Florian Quèze [:florian] [:flo] 2012-11-23 10:34:35 PST
(In reply to Richard Marti [:Paenglab] from comment #2)

Shouldn't appmenu_afterChatSeparator also be hidden?


>   setStatusMenupopupCommand: function(aEvent) {
>     let target = aEvent.originalTarget;
>     if (target.getAttribute("id") == "imStatusShowAccounts" ||
> "appmenu_imStatusShowAccounts") {
>       openIMAccountMgr();
>       return;
>     }

I suggested:
     if (target.getAttribute("id") == "imStatusShowAccounts") ||
         target.getAttribute("id") == "appmenu_imStatusShowAccounts")) {
Comment 4 Richard Marti (:Paenglab) 2012-11-23 13:42:37 PST
Created attachment 684792 [details] [diff] [review]
patch v2

(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> (In reply to Richard Marti [:Paenglab] from comment #2)
> 
> Shouldn't appmenu_afterChatSeparator also be hidden?

fixed

> I suggested:
>      if (target.getAttribute("id") == "imStatusShowAccounts") ||
>          target.getAttribute("id") == "appmenu_imStatusShowAccounts")) {

This works after removing of two braces.
Comment 5 Richard Marti (:Paenglab) 2012-12-10 10:42:06 PST
Florian, please could you review this? Maybe this would be something for ESR17 (especially for enterprises).
Comment 6 Florian Quèze [:florian] [:flo] 2012-12-10 15:53:33 PST
Comment on attachment 684792 [details] [diff] [review]
patch v2

I have some doubts on appmenu_afterChatSeparator, but it shouldn't matter much, and I assume you have tested the patch and it seemed fine to you when looking at that menu with chat disabled.

Sorry for the delay here :-/.

This indeed seems safe for esr17.

[Approval Request Comment]
Regression caused by (bug #): bug 650170
User impact if declined: Some chat menu items will be visible when chat is pref'ed off, which may happen in enterprise contexts.
Testing completed (on c-c, etc.): none by me. I hope Richard has tested the patch.
Risk to taking this patch (and alternatives if risky): Extremely low (even if there was a typo in some ids, there's a null check in the code that would ensure the typo doesn't break anything...).
Comment 7 Mark Banner (:standard8) 2012-12-11 01:26:51 PST
Comment on attachment 684792 [details] [diff] [review]
patch v2

[Triage Comment]
Please land an aurora as well. We shouldn't need to land on beta as we're not releasing that.

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