Closed Bug 802575 Opened 8 years ago Closed 8 years ago

AppMenu button doesn't get added to Chat tab automatically

Categories

(Thunderbird :: Instant Messaging, defect)

17 Branch
x86
All
defect
Not set
normal

Tracking

(thunderbird17+ fixed, thunderbird18 fixed)

RESOLVED FIXED
Thunderbird 19.0
Tracking Status
thunderbird17 + fixed
thunderbird18 --- fixed

People

(Reporter: mconley, Assigned: mconley)

Details

Attachments

(2 files, 3 obsolete files)

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.
Attached 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+
Attached patch Checked in patchSplinter Review
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: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 19.0
Attached 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+
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.