AppMenu button doesn't get added to Chat tab automatically

RESOLVED FIXED in Thunderbird 19.0

Status

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

17 Branch
Thunderbird 19.0
x86
All

Thunderbird Tracking Flags

(thunderbird17+ fixed, thunderbird18 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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.
Posted patch Patch v1 (obsolete) — Splinter Review
First go at this.
Comment on attachment 672304 [details] [diff] [review]
Patch v1

Look OK?
Attachment #672304 - Flags: review?(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.
Attachment #672304 - Flags: review?(florian)
Attachment #672304 - Flags: review?
Attachment #672304 - Flags: feedback+
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.
Attachment #672304 - Flags: review? → review?(bwinton)
What about the people who worked on bug 785692? Paenglab maybe?
(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.
For me it looks good and this is what we missed on the migration.
Comment on attachment 672304 [details] [diff] [review]
Patch v1

Blake is tied up. Magnus, do you have a few minutes to review me?
Attachment #672304 - Flags: review?(bwinton) → review?(mkmelin+mozilla)
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) ?
Attachment #672304 - Flags: review?(mkmelin+mozilla) → review+
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.
Attachment #672304 - Attachment is obsolete: true
Attachment #672451 - Flags: approval-comm-beta?
Attachment #672451 - Flags: approval-comm-aurora?
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.
Attachment #672451 - Flags: approval-comm-beta?
Attachment #672451 - Flags: approval-comm-beta+
Attachment #672451 - Flags: approval-comm-aurora?
Attachment #672451 - Flags: approval-comm-aurora+
Thanks - I made the change that Magnus suggested. This is the version of the patch that got checked in.
Attachment #672451 - Attachment is obsolete: true
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
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 19.0
Posted patch Possible test fix (obsolete) — Splinter Review
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.
Attachment #674680 - Flags: review?(mbanner)
The real patch this time.
Attachment #674680 - Attachment is obsolete: true
Attachment #674680 - Flags: review?(mbanner)
Attachment #674704 - Flags: review?(mbanner)
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...
Attachment #674704 - Flags: review?(mbanner) → review+
Assignee

Updated

7 years ago
Attachment #674704 - Flags: approval-comm-beta?
Attachment #674704 - Flags: approval-comm-aurora?
Attachment #674704 - Flags: approval-comm-beta?
Attachment #674704 - Flags: approval-comm-beta+
Attachment #674704 - Flags: approval-comm-aurora?
Attachment #674704 - Flags: approval-comm-aurora+
Attachment #673855 - Flags: approval-comm-beta+
Attachment #673855 - Flags: approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.