Last Comment Bug 802575 - AppMenu button doesn't get added to Chat tab automatically
: AppMenu button doesn't get added to Chat tab automatically
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Instant Messaging (show other bugs)
: 17 Branch
: x86 All
: -- normal (vote)
: Thunderbird 19.0
Assigned To: Mike Conley (:mconley) - (needinfo me!)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-17 06:46 PDT by Mike Conley (:mconley) - (needinfo me!)
Modified: 2012-10-25 07:07 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
fixed


Attachments
Patch v1 (8.10 KB, patch)
2012-10-17 07:38 PDT, Mike Conley (:mconley) - (needinfo me!)
mkmelin+mozilla: review+
florian: feedback+
Details | Diff | Review
Patch v2 (r+'d by mkmelin, feedback+ from florian) (8.09 KB, patch)
2012-10-17 12:22 PDT, Mike Conley (:mconley) - (needinfo me!)
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Review
Checked in patch (8.09 KB, patch)
2012-10-22 06:44 PDT, Mike Conley (:mconley) - (needinfo me!)
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Review
Possible test fix (8.09 KB, patch)
2012-10-24 08:58 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Review
Possible test fix (for real) (3.97 KB, patch)
2012-10-24 10:00 PDT, Mike Conley (:mconley) - (needinfo me!)
standard8: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Review

Description Mike Conley (:mconley) - (needinfo me!) 2012-10-17 06:46:12 PDT
I just noticed that even though bug 785692 landed, we didn't migrate the AppMenu button onto the chat tab.

We should do that.

I'll try to come up with a patch today.
Comment 1 Mike Conley (:mconley) - (needinfo me!) 2012-10-17 07:38:28 PDT
Created attachment 672304 [details] [diff] [review]
Patch v1

First go at this.
Comment 2 Mike Conley (:mconley) - (needinfo me!) 2012-10-17 07:40:10 PDT
Comment on attachment 672304 [details] [diff] [review]
Patch v1

Look OK?
Comment 3 Florian Quèze [:florian] [:flo] 2012-10-17 07:49:07 PDT
Comment on attachment 672304 [details] [diff] [review]
Patch v1

Looks OK to me, but I think you will want a reviewer who knows this code a bit better to double check.
Comment 4 Mike Conley (:mconley) - (needinfo me!) 2012-10-17 07:50:11 PDT
Comment on attachment 672304 [details] [diff] [review]
Patch v1

The only one I can think of would be Blake...or maybe Magnus, or Sid. I'll try Blake first.
Comment 5 Florian Quèze [:florian] [:flo] 2012-10-17 08:02:26 PDT
What about the people who worked on bug 785692? Paenglab maybe?
Comment 6 Mike Conley (:mconley) - (needinfo me!) 2012-10-17 08:03:20 PDT
(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> What about the people who worked on bug 785692? Paenglab maybe?

I'll Cc him to see what he thinks.
Comment 7 Richard Marti (:Paenglab) 2012-10-17 09:04:08 PDT
For me it looks good and this is what we missed on the migration.
Comment 8 Mike Conley (:mconley) - (needinfo me!) 2012-10-17 10:04:26 PDT
Comment on attachment 672304 [details] [diff] [review]
Patch v1

Blake is tied up. Magnus, do you have a few minutes to review me?
Comment 9 Magnus Melin 2012-10-17 12:16:51 PDT
Comment on attachment 672304 [details] [diff] [review]
Patch v1

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

Looks good to me. r=mkmelin

::: mail/base/modules/mailMigrator.js
@@ +244,5 @@
> +         */
> +        let addButtonToEnd = function(aToolbarID, aButtonID) {
> +          let barResource = this._rdf.GetResource(MESSENGER_DOCURL +
> +                                                  aToolbarID);
> +          if (barResource !== null) {

shouldn't this just be if (barResource) ?
Comment 10 Mike Conley (:mconley) - (needinfo me!) 2012-10-17 12:22:13 PDT
Created attachment 672451 [details] [diff] [review]
Patch v2 (r+'d by mkmelin, feedback+ from florian)

This got forgotten when we put in the AppMenu.

With this patch, users on Beta won't get migrated forward...we *could* add a new migration in that case. Let me know what's preferred.
Comment 11 Mark Banner (:standard8) 2012-10-22 01:21:56 PDT
Comment on attachment 672451 [details] [diff] [review]
Patch v2 (r+'d by mkmelin, feedback+ from florian)

Don't forget to land this on trunk as well.
Comment 12 Mike Conley (:mconley) - (needinfo me!) 2012-10-22 06:44:30 PDT
Created attachment 673855 [details] [diff] [review]
Checked in patch

Thanks - I made the change that Magnus suggested. This is the version of the patch that got checked in.
Comment 14 Mike Conley (:mconley) - (needinfo me!) 2012-10-22 09:08:00 PDT
Backed out due to oranges on Linux:

comm-central: https://hg.mozilla.org/comm-central/rev/2c84c87d353c
comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/e9502f3d7590
comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/4178cdbe1daf
Comment 15 Mike Conley (:mconley) - (needinfo me!) 2012-10-24 08:58:44 PDT
Created attachment 674680 [details] [diff] [review]
Possible test fix

It's not clear to me what was happening on our Mozmill machines when I first landed the patch.

My suspicion is that my menubar tests were running so quickly that Thunderbird started to shut down before it had finished starting up. So I've added some waits to ensure that TB has finished starting.

Whether or not this actually helps is unclear, but it certainly doesn't hurt.
Comment 16 Mike Conley (:mconley) - (needinfo me!) 2012-10-24 10:00:15 PDT
Created attachment 674704 [details] [diff] [review]
Possible test fix (for real)

The real patch this time.
Comment 17 Mark Banner (:standard8) 2012-10-24 10:04:43 PDT
Comment on attachment 674704 [details] [diff] [review]
Possible test fix (for real)

Yep, lets try it, although if it works, we could also argue that we should be waiting for mail-startup-done anyway...

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