Closed Bug 1022209 Opened 10 years ago Closed 10 years ago

msgButton is null error when get new messages button not found on toolbar

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 35.0

People

(Reporter: merike, Assigned: aceman)

References

Details

Attachments

(1 file, 2 obsolete files)

JavaScript error: chrome://messenger/content/mailWindowOverlay.js, line 733: msgButton is null

That function should check if button was found on toolbar.
Right. Do you want to fix it?
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Attachment #8440121 - Flags: review?(mkmelin+mozilla)
Attachment #8440121 - Flags: feedback?(gasell+mozilla)
Comment on attachment 8440121 [details] [diff] [review]
patch

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

Looks good to me.
Attachment #8440121 - Flags: feedback?(gasell+mozilla) → feedback+
> +  // The button has a different ID if it isn't on the toolbar but in the Customize
> +  // palette. In that case we do not need to update its tooltip.

In Thunderbird the toolbarpalette is removed from the DOM but a reference is kept. that's why document.getElementById() can't find it, not that it has a different id. Unless you're talking about Australis+CustomizableUI which Thunderbird doesn't have.

> +  var usernames = new Set([v.server.username for (v of folders)]);
Nit: (var v of folders) or (let v of folders)

> +  var tooltip = document.getElementById("bundle_messenger")
> +  var listSeparator = document.getElementById("bundle_messenger")

If you're going to change these lines you could at least remove the duplication.

var bundle = gMessageNotificationBar.stringBundle;
var tooltip = bundle.getString(...)
var listSeparator = bundle.getString(...)
(In reply to Philip Chee from comment #4)
> > +  // The button has a different ID if it isn't on the toolbar but in the Customize
> > +  // palette. In that case we do not need to update its tooltip.
> 
> In Thunderbird the toolbarpalette is removed from the DOM but a reference is
> kept. that's why document.getElementById() can't find it, not that it has a
> different id. Unless you're talking about Australis+CustomizableUI which
> Thunderbird doesn't have.
I probably mixed it with the state when the Customize palette is open. Then the elements are there with an id prefixed with "wrapper-".

> > +  var usernames = new Set([v.server.username for (v of folders)]);
> Nit: (var v of folders) or (let v of folders)
Adding the 'let' actually breaks the function. Keeping it without one does not produce the "assigning to uninitialized variable' warnings.

> > +  var tooltip = document.getElementById("bundle_messenger")
> > +  var listSeparator = document.getElementById("bundle_messenger")
> 
> If you're going to change these lines you could at least remove the
> duplication.
Yeah, thanks.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #8440121 - Attachment is obsolete: true
Attachment #8440121 - Flags: review?(mkmelin+mozilla)
Attachment #8443753 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8443753 [details] [diff] [review]
patch v2

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

LGTM! r=mkmelin
Attachment #8443753 - Flags: review?(mkmelin+mozilla) → review+
Thanks.
Keywords: checkin-needed
This doesn't apply cleanly, can it be updated please?
Yeah, bitrotted by 967583. I'll update it.
Depends on: 967583
(In reply to :aceman from comment #10)
> Yeah, bitrotted by 967583. I'll update it.

Reminder for updating. If you don't have time to unbitrott your patch, I could also do this for you. Luckily this is easy in this case. :-)
Attached patch Patch V2.1Splinter Review
Updated for trunk.
Attachment #8443753 - Attachment is obsolete: true
Attachment #8479997 - Flags: review+
Thanks guys. Of course I could update it too, but my build is failing for 2 months now and I didn't want to attach the new patch without re-testing. So I'd be glad if anybody could update it and test and attach.
https://hg.mozilla.org/comm-central/rev/b473e9f8b1d9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 35.0
Depends on: 1062833
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: