539066 - port bug 151244 to seamonkey, so return receipts keep working

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Michiel van Leeuwen (email: mvl+moz@)]

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)]

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]

Could we get some traction here? Our 2.1a1 code freeze is in a week and this is a blocker!

Comment 3

[Philip Chee]

I'll make a stab at it (assuming that I can get trunk to build)

Comment 4

[Philip Chee]

Attachment: 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)]

(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]

> 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]

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]

> the xpcshell test from bug 151244 ran successfully.
But appears to test only the backend.

Comment 9

[Stefan [:stefanh]]

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

[[e-mail]]

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]

Attachment: Patch v1.1 Address review comment.

Comment 12

[Philip Chee]

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.

>  [e-mail]      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]

(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]

sure, that would be nice, thx for thinking of that.

Comment 15

[Philip Chee]

Comment on attachment 443798 [details] [diff] [review]
Patch v1.1 Address review comment.

Switching review to IanN.

Comment 16

[[e-mail]]

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

[[e-mail]]

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

[[e-mail]]

(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]

Attachment: 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]

>> 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]

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]

> I'm guessing the use of Warning still on classic is a typo?
> r=me with that explained/changed.

Oops.

Comment 23

[Philip Chee]

Attachment: 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]

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]]

--- 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]

Oops. Question.png => question-32.png. Will attach a followup patch if you don't get to it first.

Comment 27

[Philip Chee]

Attachment: Patch v1.2b Fix OS X missing image.

Comment 28

[Ian Neal]

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]]

http://hg.mozilla.org/comm-central/rev/519b9b0655ac

Comment 30

[Robert Kaiser]

Not that a2 needs approval when the tree reopens ;-)

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