Closed Bug 1667774 Opened 9 months ago Closed 8 months ago

HTTP Refresh no longer work in content tabs

Categories

(Thunderbird :: Add-Ons: Extensions API, defect)

defect

Tracking

(thunderbird_esr78+ fixed, thunderbird82 fixed)

RESOLVED FIXED
83 Branch
Tracking Status
thunderbird_esr78 + fixed
thunderbird82 --- fixed

People

(Reporter: neil, Assigned: neil)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Bug 511240 added a progress listener to Thunderbird's content tabs. Unfortunately, it doesn't quite honour the API correctly, which means that it ends up blocking all HTTP Refresh redirects.
Bug 1455471 then duplicated the error with its own progress listener. (This error could possibly have been avoided by comparing with Firefox's tabs progress listener.)

Attached patch Proposed patch (obsolete) — Splinter Review
Assignee: nobody → neil
Attachment #9178207 - Flags: review?(geoff)
Comment on attachment 9178207 [details] [diff] [review]
Proposed patch

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

Ah, the old "all of these methods are void, except one" trick! Thanks for finding and fixing this. r+ with a nit fixed.

::: mail/base/content/tabmail.js
@@ +1947,4 @@
>        for (let listener of this.mTabsProgressListeners.values()) {
>          if (aMethod in listener) {
>            try {
> +            if (!listener[aMethod](...aArgs))

You'll need these here: {}
Attachment #9178207 - Flags: review?(geoff) → review+
Comment on attachment 9178207 [details] [diff] [review]
Proposed patch

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

::: mail/base/content/tabmail.js
@@ +1947,4 @@
>        for (let listener of this.mTabsProgressListeners.values()) {
>          if (aMethod in listener) {
>            try {
> +            if (!listener[aMethod](...aArgs))

I'd also just return false here and not bother having an rv set. (Then return true down there also of course)
Status: NEW → ASSIGNED

(In reply to Geoff Lankow from comment #2)

You'll need these here: {}

Sorry, my mach lint complained about protovis and I hadn't realised that meant it had actually skipped tabmail.js.

(In reply to Magnus Melin from comment #3)

I'd also just return false here and not bother having an rv set. (Then return true down there also of course)

I was copying Firefox's code here; I think they want to call all of the listeners, and then return false if any of them did.

Attachment #9178207 - Attachment is obsolete: true
Attachment #9178999 - Flags: review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/d47938982f6b
Fix HTTP refresh for content tabs r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → 83 Branch

Thanks for checking this in. Is this change eligible for Thunderbird 78 uplift via comm-beta?

I don't see why not. Do set the approval request flags.

It will need to be reworked for 78.

Comment on attachment 9178999 [details] [diff] [review]
Addressed review comment

(In reply to Geoff Lankow from comment #8)

It will need to be reworked for 78.

I take it that means that this version should already apply to c-b? I can easily write a backport for 78.

[Approval Request Comment]
Regression caused by (bug #): bug 511240, bug 1455471
User impact if declined: Some sites don't refresh correctly when opened in content tabs
Risk to taking this patch (and alternatives if risky): Low, only affects content tabs

Attachment #9178999 - Flags: approval-comm-beta?

Comment on attachment 9178999 [details] [diff] [review]
Addressed review comment

[Triage Comment]
Approved for beta

Attachment #9178999 - Flags: approval-comm-beta? → approval-comm-beta+

Neil, can you attach a backport and request 78 approval?

Attached patch ESR 78 backportSplinter Review

[Approval Request Comment]
Regression caused by (bug #): bug 511240, bug 1455471
User impact if declined: Some sites don't refresh correctly when opened in content tabs
Risk to taking this patch (and alternatives if risky): Low, only affects content tabs

Attachment #9190314 - Flags: approval-comm-esr78?

Comment on attachment 9190314 [details] [diff] [review]
ESR 78 backport

[Triage Comment]
Approved for esr78

Attachment #9190314 - Flags: approval-comm-esr78? → approval-comm-esr78+
Keywords: regression
You need to log in before you can comment on or make changes to this bug.