Last Comment Bug 709737 - Test Pilot notifications don't work
: Test Pilot notifications don't work
Product: Thunderbird
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 11.0
Assigned To: Mark Banner (:standard8, afk until Dec)
Depends on:
Blocks: 709270
  Show dependency treegraph
Reported: 2011-12-12 02:04 PST by Mark Banner (:standard8, afk until Dec)
Modified: 2011-12-20 14:35 PST (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

The fix (23.58 KB, patch)
2011-12-14 16:06 PST, Mark Banner (:standard8, afk until Dec)
bwinton: review+
bwinton: ui‑review+
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description Mark Banner (:standard8, afk until Dec) 2011-12-12 02:04:38 PST
Having taken part in the test study on the development channel, the study is now complete, but I haven't been prompted to submit my data.

The error console says:

Error: popup is null
Source File: resource://testpilot/modules/notifications.js
Line: 103

Looks like as we removed the feedback button, we've not got a notification option. Is there somewhere else we could hang this from?
Comment 1 Jim Porter (:squib) 2011-12-12 23:57:26 PST
Mconley, do you mind taking a look at this? It should be fairly straightforward.
Comment 2 Mark Banner (:standard8, afk until Dec) 2011-12-13 01:06:24 PST
I can take it.
Comment 3 Mark Banner (:standard8, afk until Dec) 2011-12-14 16:06:19 PST
Created attachment 581822 [details] [diff] [review]
The fix

This should fix up test pilot for us.

It fixes:

- Notifications (they now get displayed, anchored to the toolbar)
- Build test pilot on all channels (i.e. include release)
- Looks the same on all channels
- Links in the experiment page now open in the browser, and use the FF style one.
- Removes obsolete interface.js.orig
- Removes the preferences box that was meant for mobile and not the browser.
Comment 4 Blake Winton (:bwinton) (:☕️) 2011-12-14 16:27:43 PST
Comment on attachment 581822 [details] [diff] [review]
The fix

Review of attachment 581822 [details] [diff] [review]:

So, I'm not so happy with the flash you can see in and the notification pops up in a bit of a weird place as seen in , but I'm going to say ui-r=me for now, and we can fix these later.

And I'm pretty happy with the code, too, so r=me, modulo the comments below.

::: mail/app/profile/extensions/
@@ -6,4 +6,4 @@
> >       xmlns:em="">
> >    <Description about="urn:mozilla:install-manifest">
> >      <em:id></em:id>
> > -    <em:version>1.3.1</em:version>
> > +    <em:version>1.3.2</em:version>

The version you sent me on IRC said "1.3.1" and "By: Mozilla Messaging".  As long as those are "1.3.2" and "Mozilla Corporation" (or "Mozilla"), I'm happy with that.

::: mail/app/profile/extensions/
@@ -186,4 +189,4 @@
> >      // Show the popup:
> >      popup.hidden = false;
> >      popup.setAttribute("open", "true");
> > -    popup.openPopup( anchor, "after_end");
> > +    // XXX Thunderbird needs after_start for now.

Why is this?  If it's a reasonable reason, can we remove the "XXX"?  If not, can we file a followup bug?
Comment 6 Mark Banner (:standard8, afk until Dec) 2011-12-15 04:48:59 PST
I had to do a follow-up to create the panel in the menu bar element rather than the toolbar, as this was messing up the migration assistant. The position is still the same though. This also fixed the unit tests on trunk:
Comment 7 Philip Chee 2011-12-15 19:58:52 PST
> -<toolbar id="mail-bar3">
> +<toolbar id="mail-toolbox">
shouldn't this be <*toolbox* id="mail-toolbox">
Comment 8 Mike Conley (:mconley) 2011-12-15 20:00:28 PST
Hm - yes, it should be.
Comment 9 Mark Banner (:standard8, afk until Dec) 2011-12-16 14:41:16 PST
Checked in an additional fix for the element tag, with r=mconley over irc:
Comment 10 Mike Conley (:mconley) 2011-12-20 14:30:12 PST
Checked in additional additional fix to comm-central as:
Comment 11 Mark Banner (:standard8, afk until Dec) 2011-12-20 14:35:12 PST
and to aurora as:

this also made it onto comm-release (comm-beta isn't required at this time, as that had already merged to release):

Note You need to log in before you can comment on or make changes to this bug.