Closed Bug 1575708 Opened 5 years ago Closed 5 years ago

Message display API

Categories

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

Product:
Thunderbird
Component:
Add-Ons: Extensions API
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(thunderbird_esr6870+ fixed, thunderbird70 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 70.0
Tracking Flags:
Tracking Status
thunderbird_esr68 70+ fixed
thunderbird70 --- fixed

People

(Reporter: darktrojan, Assigned: darktrojan)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

1575708-message-display-api-1.diff
14.22 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
1575708-message-display-api-esr.diff
13.84 KB, patch
jorgk-bmo
: approval-comm-esr68+
Details | Diff | Splinter Review

We need an API that can tell us when a message is being displayed and where, before we can do things with the message. This is further complicated by the windows API knowing nothing about message display windows, but I'll deal with that in another bug.

Depends on: 1575710

Adds an event that fires when a message is displayed, and a function to find out what message has been displayed.

Attachment #9089284 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9089284 [details] [diff] [review]
1575708-message-display-api-1.diff

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

::: mail/components/extensions/parent/ext-messageDisplay.js
@@ +42,5 @@
> +
> +          if (displayedMessage) {
> +            return convertMessage(displayedMessage, extension);
> +          }
> +          return null;

convertMessage handles null, so you can just do return convertMessage(........)
Attachment #9089284 - Flags: review?(mkmelin+mozilla) → review+

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/cba2237adb94
Message display API; r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 70.0
Comment on attachment 9089284 [details] [diff] [review]
1575708-message-display-api-1.diff

Wanted in 68.2.
Attachment #9089284 - Flags: approval-comm-esr68?
Attachment #9089284 - Flags: approval-comm-esr68? → approval-comm-esr68+
Comment on attachment 9089284 [details] [diff] [review]
1575708-message-display-api-1.diff

Please rebase this. Patch mostly applies, but I don't know whether you want to land bug 1575710 first. Please coordinate with Paul whether he wants to reformat with "Prettier" first.
Flags: needinfo?(geoff)
Attachment #9089284 - Flags: approval-comm-esr68+
Comment on attachment 9089284 [details] [diff] [review]
1575708-message-display-api-1.diff

Sorry, this is a prettified patch which applies after prettifying c-esr68.
Flags: needinfo?(geoff)
Attachment #9089284 - Flags: approval-comm-esr68+

I uplifted this with bug 1575710 and bug 1576492 and a bunch more stuff and now all the Mochitests are broken: No suite end message was emitted by this harness.
https://treeherder.mozilla.org/#/jobs?repo=comm-esr68&revision=0f180c4b528ce17f06daa7191f84a1e6f23bb4d9
https://hg.mozilla.org/releases/comm-esr68/pushloghtml?changeset=0f180c4b528ce17f06daa7191f84a1e6f23bb4d9

I guess one of these three bugs is to blame. Note that in TB 68 tabmail is still in XBL, see bug 1544793 comment #16, that one resisted uplift.

Flags: needinfo?(geoff)

Please backout this bug and bug 1575710 from ESR. I asked for them to be landed on 68.2 because I don't believe in adding new features in point-point releases (ie. 68.x.y).

It looks like the problem with mochitest is caused by the # e10s comment in the browser.ini file. It can be removed.

Flags: needinfo?(geoff) → needinfo?(jorgk)

… and it seems the test itself is also broken.

(In reply to Geoff Lankow (:darktrojan) from comment #10)

Please backout this bug and bug 1575710 from ESR. I asked for them to be landed on 68.2 because I don't believe in adding new features in point-point releases (ie. 68.x.y).

Well, I wanted to land this one here in and bug 1575710 and bug 1544793 in 68.1.1 to give them some workout before it hits the masses when we upgrade all 60.x users to 68.2. Is that such a bad idea? I doesn't really matter at which number a new thing gets introduced, it's more important that we don't affect too many people if there are issues.

So I'd prefer to fix the test and also go ahead with bug 1544793.

Flags: needinfo?(jorgk) → needinfo?(geoff)

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5c67b36325cfec3743176f07e157e03fe326adb5
removed browser.tabs.remote.autostart=false # e10s as suggested. I don't know what comment #11 is meant to mean.

Looks like it means this:
TEST-UNEXPECTED-FAIL | comm/mail/components/extensions/test/browser/browser_ext_messageDisplay.js | Test timed out -
TEST-UNEXPECTED-FAIL | comm/mail/components/extensions/test/browser/browser_ext_messageDisplay.js | no tasks awaiting on messages - Got ["show message 2"], expected []
TEST-UNEXPECTED-FAIL | comm/mail/components/extensions/test/browser/browser_ext_messageDisplay.js | Extension left running at test shutdown

(In reply to Jorg K (GMT+2) from comment #12)

Well, I wanted to land this one here in and bug 1575710 and bug 1544793 in 68.1.1 to give them some workout before it hits the masses when we upgrade all 60.x users to 68.2. Is that such a bad idea? I doesn't really matter at which number a new thing gets introduced, it's more important that we don't affect too many people if there are issues.

So I'd prefer to fix the test and also go ahead with bug 1544793.

We have a beta channel to give patches a workout. As these are new API features I doubt they'd get any more use on release than they would on the beta channel. Plus it makes it more complicated for extension authors to keep track of what version contained what so they can set extension compatibility correctly, and also that means ATN has to add 68.1.1 as a valid option for minimum version.

It seems the test failure (yes, like in comment 14) is not permanent, I just happened to strike it the first time. So what I did that I thought fixed it in fact probably didn't, but was coincidental. I don't think that the failure should be any more frequent than on comm-central and I'm not aware of it being a problem there.

Flags: needinfo?(geoff)

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d393a6801b0ec878d24381ba475c92c91f9bf357

Another try run with only the # e10s part removed. If the test still fails, I'll back it out, otherwise we can ship it and advertise it for 68.2 so add-on authors don't get confused.

Well, in that try M(bct) failed again just like in comment #14. I haven't seen such failures on trunk (or beta) since this API landed. I've retriggered the test now, but I have the impression that this is not intermittent given that 2 of the 2 samples I have are failures.

Comment on attachment 9089284 [details] [diff] [review]
1575708-message-display-api-1.diff

OK, make this "3 of 3 failed". I'll back this out.
Attachment #9089284 - Flags: approval-comm-esr68+
Flags: needinfo?(geoff)
Flags: needinfo?(geoff)
Attachment #9099797 - Flags: approval-comm-esr68?
Attachment #9099797 - Flags: approval-comm-esr68? → approval-comm-esr68+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: