Last Comment Bug 539066 - port bug 151244 to seamonkey, so return receipts keep working
: port bug 151244 to seamonkey, so return receipts keep working
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: MailNews: Message Display (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1a1
Assigned To: Philip Chee
:
Mentors:
Depends on: 151244
Blocks: 568447
  Show dependency treegraph
 
Reported: 2010-01-11 13:03 PST by Michiel van Leeuwen (email: mvl+moz@)
Modified: 2010-05-27 02:24 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
a1+


Attachments
Patch v1.0 Straight port. (13.71 KB, patch)
2010-04-29 02:18 PDT, Philip Chee
stefanh: feedback+
Details | Diff | Splinter Review
Patch v1.1 Address review comment. (13.91 KB, patch)
2010-05-05 20:51 PDT, Philip Chee
neil: superreview+
Details | Diff | Splinter Review
Patch v1.2 Fix Nits. sr=Neil (13.95 KB, patch)
2010-05-09 09:46 PDT, Philip Chee
iann_bugzilla: review+
philip.chee: superreview+
Details | Diff | Splinter Review
Patch v 1.2a Fix typo. r=IanN sr=Neil a2.1a1+ (13.95 KB, patch)
2010-05-09 21:03 PDT, Philip Chee
philip.chee: review+
philip.chee: superreview+
Details | Diff | Splinter Review
Patch v1.2b Fix OS X missing image. (977 bytes, patch)
2010-05-10 11:23 PDT, Philip Chee
stefanh: review+
iann_bugzilla: approval‑seamonkey2.1a1+
iann_bugzilla: approval‑seamonkey2.1a2+
Details | Diff | Splinter Review

Description Michiel van Leeuwen (email: mvl+moz@) 2010-01-11 13:03:53 PST
The patch in bug 151244 will change the MDN api. The UI now has to ask the user for permission, instead of the backend. This allows for better (non-blocking) UI. The SeaMonkey UI needs to be adapted to this change. Otherwise, no return receipts will be send if the user has set the prefs to be asked first.
Comment 1 Mark Banner (:standard8) 2010-04-01 13:29:32 PDT
Note that the patch in bug 151244 has been reviewed and is almost ready for landing. Given that TB 3.1b2 is imminent and afaik SM 2.1a1 hasn't been scheduled yet, I've given the go ahead for that to land. Hence setting this as blocking 2.1a1 - I'm pretty sure this will just affect prompting for return receipts (i.e. SM won't) in nightly builds and nothing else.

Note when fixing this bug, the following strings either need to be removed or used:

MsgMdnWishToSend
MsgMdnIgnoreRequest
MsgMdnSendReceipt
Comment 2 Robert Kaiser 2010-04-27 05:57:39 PDT
Could we get some traction here? Our 2.1a1 code freeze is in a week and this is a blocker!
Comment 3 Philip Chee 2010-04-28 09:07:16 PDT
I'll make a stab at it (assuming that I can get trunk to build)
Comment 4 Philip Chee 2010-04-29 02:18:53 PDT
Created attachment 442350 [details] [diff] [review]
Patch v1.0 Straight port.

Straight port from Thunderbird.

Stefanh: Can you check the mac changes? Especially the following styles. I'm not sure if they are really needed.

+#mdnBar {
+  border: 1px solid #DDDDE1;
+  background-color: #FBFBC8;
+  padding: 0.3em 0.2em;
+  -moz-border-radius: 0.2em;
+}

> diff --git a/suite/themes/modern/global/icons/Warning.png b/suite/themes/modern/global/icons/Warning.png
> new file mode 100644 

Warning.png is from Past Modern. We have blanket permission from Kuden to use her images from Past Modern under the mozilla tri-licence.
Comment 5 Jens Hatlak (:InvisibleSmiley) 2010-04-29 04:12:03 PDT
(In reply to comment #4)
> > diff --git a/suite/themes/modern/global/icons/Warning.png b/suite/themes/modern/global/icons/Warning.png
> > new file mode 100644 

I guess that'll be a good idea anyway:
http://mxr.mozilla.org/comm-central/source/suite/themes/modern/mozapps/update/updates.css#120

Which was done by... you. :-P
Comment 6 Philip Chee 2010-04-29 08:28:28 PDT
> Which was done by... you. :-P

Err, oops.

BTW, I managed to build trunk successfully by doing a hg update mozilla-central to an older revision.

the xpcshell test from bug 151244 ran successfully.

TEST-PASS | c:\t1\hg\objdir-sm\mozilla\_tests\xpcshell\test_mdn\unit\test_askuser.js | test passed
INFO | Result summary:
INFO | Passed: 1
INFO | Failed: 0
Comment 7 Philip Chee 2010-04-30 05:08:54 PDT
Comment on attachment 442350 [details] [diff] [review]
Patch v1.0 Straight port.

For some reason I got r? and sr? reversed.
Comment 8 Philip Chee 2010-04-30 05:10:11 PDT
> the xpcshell test from bug 151244 ran successfully.
But appears to test only the backend.
Comment 9 Stefan [:stefanh] 2010-04-30 05:54:17 PDT
Comment on attachment 442350 [details] [diff] [review]
Patch v1.0 Straight port.

diff --git a/suite/themes/classic/mac/messenger/primaryToolbar.css b/suite/themes/classic/mac/messenger/primaryToolbar.css
--- a/suite/themes/classic/mac/messenger/primaryToolbar.css
+++ b/suite/themes/classic/mac/messenger/primaryToolbar.css
@@ -472,11 +472,22 @@ toolbar[iconsize="small"] > #button-stop
   list-style-image: url("chrome://messenger/skin/icons/folder-junk.png");
 }
 
 #remoteContentImage {
   list-style-image: url("chrome://messenger/skin/icons/remote-blocked.png"); 
   padding: 3px;
 }
 
+#mdnBarImage {
+  list-style-image: url("chrome://global/skin/icons/warning-32.png");
+}
Please add 3px padding here (as it is for #remoteContentImage)

And you're right, we don't need #mdnBar rules, so just leave them out. Then: please also remove the "-moz-appearance: toolbox;" rule at line 457 (.msgNotificationBar rules), since I can't see that it has any effect.

r=me (or is it f+?) with those changes
Comment 10 neil@parkwaycc.co.uk 2010-05-03 16:26:27 PDT
Comment on attachment 442350 [details] [diff] [review]
Patch v1.0 Straight port.

>+                    8  // 1 << (kMsgNotificationMSN - 1)
Typo: MDN

>+    this.msgHeader = aMsgHeader;
Unused?

>+  if (!msgFlags)
>+    return;
What does this achieve?

>+  list-style-image: url("chrome://global/skin/icons/Warning.png");
Surely the old dialog would have used a Question or possibly Information image?
Comment 11 Philip Chee 2010-05-05 20:51:59 PDT
Created attachment 443798 [details] [diff] [review]
Patch v1.1 Address review comment.
Comment 12 Philip Chee 2010-05-05 20:53:10 PDT
Ooops. Forgot to attach notes.

> Stefan      2010-04-30 05:54:17 PDT
> 
> +#mdnBarImage {
> +  list-style-image: url("chrome://global/skin/icons/warning-32.png");
> +}
> Please add 3px padding here (as it is for #remoteContentImage)
Fixed.

> And you're right, we don't need #mdnBar rules, so just leave them out. Then:
> please also remove the "-moz-appearance: toolbox;" rule at line 457
> (.msgNotificationBar rules), since I can't see that it has any effect.
Fixed.

>  neil@parkwaycc.co.uk      2010-05-03 16:26:27 PDT
> 
> (From update of attachment 442350 [details] [diff] [review])
> >+                    8  // 1 << (kMsgNotificationMSN - 1)
> Typo: MDN
Fixed.

> >+    this.msgHeader = aMsgHeader;
> Unused?
Removed. It was introduced in the first version of the Thunderbird patch without any comment on the reason.

> >+  if (!msgFlags)
> >+    return;
> What does this achieve?
No idea. Removed.

> >+  list-style-image: url("chrome://global/skin/icons/Warning.png");
> Surely the old dialog would have used a Question or possibly Information image?

The old dialog used Question, switching to Question.png.
Comment 13 Robert Kaiser 2010-05-06 04:10:20 PDT
(In reply to comment #12)
> > >+    this.msgHeader = aMsgHeader;
> > Unused?
> Removed. It was introduced in the first version of the Thunderbird patch
> without any comment on the reason.

Just as a side note, should a Thunderbird bug be raised on that and possibly any other things review improved here over their code?
Comment 14 David :Bienvenu 2010-05-06 07:51:59 PDT
sure, that would be nice, thx for thinking of that.
Comment 15 Philip Chee 2010-05-08 07:53:08 PDT
Comment on attachment 443798 [details] [diff] [review]
Patch v1.1 Address review comment.

Switching review to IanN.
Comment 16 neil@parkwaycc.co.uk 2010-05-08 16:11:22 PDT
Comment on attachment 443798 [details] [diff] [review]
Patch v1.1 Address review comment.

>+#mdnBarImage {
>+  list-style-image: url("chrome://global/skin/icons/Question.png");
>+  padding: 3px;
The other themes need 3px padding too, although in the case of Modern I'm unsure as to whether you couldn't just use alert-question.gif instead (I didn't think to try it before starting a rebuild, sorry.) If you stick with Question.png you will of course need to add it to the jar.mn at some point!
Comment 17 neil@parkwaycc.co.uk 2010-05-08 16:48:47 PDT
Comment on attachment 443798 [details] [diff] [review]
Patch v1.1 Address review comment.

Of course, the .gif has no transparency... 3px margin and jar.mn it is, then!
Comment 18 neil@parkwaycc.co.uk 2010-05-09 08:46:11 PDT
(In reply to comment #17)
> (From update of attachment 443798 [details] [diff] [review])
> Of course, the .gif has no transparency
[Maybe add a comment to that effect?]
Comment 19 Philip Chee 2010-05-09 09:46:25 PDT
Created attachment 444308 [details] [diff] [review]
Patch v1.2 Fix Nits. sr=Neil

carrying forward sr=Neil

> (From update of attachment 443798 [details] [diff] [review])

>>+#mdnBarImage {
>>+  list-style-image: url("chrome://global/skin/icons/Question.png");
>>+  padding: 3px;
> The other themes need 3px padding too, although in the case of Modern I'm

Fixed.

> unsure as to whether you couldn't just use alert-question.gif instead (I didn't
> think to try it before starting a rebuild, sorry.) If you stick with
> Question.png you will of course need to add it to the jar.mn at some point!
> 
> Of course, the .gif has no transparency... 3px margin and jar.mn it is, then!
> 
Right Said Fred.
Comment 20 Philip Chee 2010-05-09 09:48:06 PDT
>> Of course, the .gif has no transparency
> [Maybe add a comment to that effect?]
Oops saw this comment too late. Perhaps a separate bug to switch Modern to Question.png instead?
Comment 21 Ian Neal 2010-05-09 12:29:10 PDT
Comment on attachment 444308 [details] [diff] [review]
Patch v1.2 Fix Nits. sr=Neil

>+++ b/suite/themes/classic/mac/messenger/primaryToolbar.css
>+#mdnBarImage {
>+  list-style-image: url("chrome://global/skin/icons/Question.png");
>+  padding: 3px;
>+}

>+++ b/suite/themes/classic/messenger/primaryToolbar.css
>+#mdnBarImage {
>+  list-style-image: url("chrome://global/skin/icons/Warning.png");
>+  padding: 3px;
>+}

>+++ b/suite/themes/modern/messenger/primaryToolbar.css
>+#mdnBarImage {
>+  list-style-image: url("chrome://global/skin/icons/Question.png");
>+  padding: 3px;
>+}

I'm guessing the use of Warning still on classic is a typo?
r=me with that explained/changed.
Comment 22 Philip Chee 2010-05-09 18:04:55 PDT
> I'm guessing the use of Warning still on classic is a typo?
> r=me with that explained/changed.

Oops.
Comment 23 Philip Chee 2010-05-09 21:03:22 PDT
Created attachment 444339 [details] [diff] [review]
Patch v 1.2a Fix typo. r=IanN sr=Neil a2.1a1+

Carrying forward r=IanN and sr=Neil

>> I'm guessing the use of Warning still on classic is a typo?
>> r=me with that explained/changed.
> Oops.
Typo fixed Warning.png => Question.png.
Comment 24 Robert Kaiser 2010-05-10 03:38:38 PDT
Pushed as http://hg.mozilla.org/comm-central/rev/e9d005c309d8 - please mark fixed with appropriate target milestone if that was all for this bug.
Comment 25 Stefan [:stefanh] 2010-05-10 09:02:44 PDT
--- a/suite/themes/classic/messenger/primaryToolbar.css
+++ b/suite/themes/classic/messenger/primaryToolbar.css
@@ -572,6 +572,11 @@ toolbar[iconsize="small"] > #button-stop
   padding: 3px;
 }
 
+#mdnBarImage {
+  list-style-image: url("chrome://global/skin/icons/Question.png");
+  padding: 3px;
+}
+
 #allowRemoteContentForAuthorDesc {
   -moz-padding-start: 10px;
 }

So, did anyone check that Question.png exists in pinstripe (http://mxr.mozilla.org/mozilla-central/find?string=question.png)?
Comment 26 Philip Chee 2010-05-10 11:09:45 PDT
Oops. Question.png => question-32.png. Will attach a followup patch if you don't get to it first.
Comment 27 Philip Chee 2010-05-10 11:23:07 PDT
Created attachment 444437 [details] [diff] [review]
Patch v1.2b Fix OS X missing image.
Comment 28 Ian Neal 2010-05-10 13:52:19 PDT
Comment on attachment 444437 [details] [diff] [review]
Patch v1.2b Fix OS X missing image.

a=me for whichever it can be done for.
Comment 29 Stefan [:stefanh] 2010-05-10 14:02:24 PDT
http://hg.mozilla.org/comm-central/rev/519b9b0655ac
Comment 30 Robert Kaiser 2010-05-10 17:49:05 PDT
Not that a2 needs approval when the tree reopens ;-)

Still, I updated the a1 release config to include this one.

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