802575 - AppMenu button doesn't get added to Chat tab automatically

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

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) (:⚙️)]

Attachment: Patch v1

First go at this.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 672304 [details] [diff] [review]
Patch v1

Look OK?

Comment 3

[Florian Quèze [:florian]]

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) (:⚙️)]

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]]

What about the people who worked on bug 785692? Paenglab maybe?

Comment 6

[Mike Conley (:mconley) (:⚙️)]

(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)]

For me it looks good and this is what we missed on the migration.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

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 [:mkmelin]]

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) (:⚙️)]

Attachment: 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)]

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) (:⚙️)]

Attachment: Checked in patch

Thanks - I made the change that Magnus suggested. This is the version of the patch that got checked in.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

comm-central: https://hg.mozilla.org/comm-central/rev/ecc2d449db40
comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/6e059f816a3e
comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/d450fefad0d0

Comment 14

[Mike Conley (:mconley) (:⚙️)]

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) (:⚙️)]

Attachment: 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) (:⚙️)]

Attachment: Possible test fix (for real)

The real patch this time.

Comment 17

[Mark Banner (:standard8)]

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...

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Attachment 673855 [details] [diff]
comm-central: https://hg.mozilla.org/comm-central/rev/5ba465f0c7f0
comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/5172c9cf6417
comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/40b0e18f5f7e

Attachment 674704 [details] [diff]
comm-central: https://hg.mozilla.org/comm-central/rev/ce1bbebaef20
comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/994992a767aa
comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/97dd0c4aadee