The default bug view has changed. See this FAQ.

port bug 151244 to seamonkey, so return receipts keep working

RESOLVED FIXED in seamonkey2.1a1

Status

SeaMonkey
MailNews: Message Display
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Michiel van Leeuwen (email: mvl+moz@), Assigned: Philip Chee)

Tracking

Trunk
seamonkey2.1a1
Dependency tree / graph

Firefox Tracking Flags

(blocking-seamonkey2.1 a1+)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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.
Version: unspecified → Trunk
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
Flags: blocking-seamonkey2.1a1+

Updated

7 years ago
Keywords: helpwanted

Comment 2

7 years ago
Could we get some traction here? Our 2.1a1 code freeze is in a week and this is a blocker!
blocking-seamonkey2.1: --- → a1+
Flags: blocking-seamonkey2.1a1+
(Assignee)

Comment 3

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

Updated

7 years ago
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Keywords: helpwanted
(Assignee)

Comment 4

7 years ago
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.
Attachment #442350 - Flags: superreview?(mnyromyr)
Attachment #442350 - Flags: review?(neil)
Attachment #442350 - Flags: feedback?(stefanh)
(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
(Assignee)

Comment 6

7 years ago
> 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
(Assignee)

Updated

7 years ago
Attachment #442350 - Flags: superreview?(neil)
Attachment #442350 - Flags: superreview?(mnyromyr)
Attachment #442350 - Flags: review?(neil)
Attachment #442350 - Flags: review?(mnyromyr)
(Assignee)

Comment 7

7 years ago
Comment on attachment 442350 [details] [diff] [review]
Patch v1.0 Straight port.

For some reason I got r? and sr? reversed.
(Assignee)

Comment 8

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

Comment 9

7 years ago
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
Attachment #442350 - Flags: feedback?(stefanh) → feedback+
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?
(Assignee)

Comment 11

7 years ago
Created attachment 443798 [details] [diff] [review]
Patch v1.1 Address review comment.
Attachment #442350 - Attachment is obsolete: true
Attachment #443798 - Flags: superreview?(neil)
Attachment #443798 - Flags: review?(mnyromyr)
Attachment #442350 - Flags: superreview?(neil)
Attachment #442350 - Flags: review?(mnyromyr)
(Assignee)

Comment 12

7 years ago
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

7 years ago
(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

7 years ago
sure, that would be nice, thx for thinking of that.
(Assignee)

Comment 15

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

Switching review to IanN.
Attachment #443798 - Flags: review?(mnyromyr)
Attachment #443798 - Flags: review?(iann_bugzilla)
Attachment #443798 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #443798 - Flags: review?
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 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!
Attachment #443798 - Flags: superreview?(neil) → superreview+
(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?]
(Assignee)

Comment 19

7 years ago
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.
Attachment #443798 - Attachment is obsolete: true
Attachment #444308 - Flags: superreview+
Attachment #444308 - Flags: review?(iann_bugzilla)
Attachment #443798 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 20

7 years ago
>> 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

7 years ago
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.
Attachment #444308 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 22

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

Oops.
(Assignee)

Comment 23

7 years ago
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.
Attachment #444308 - Attachment is obsolete: true
Attachment #444339 - Flags: superreview+
Attachment #444339 - Flags: review+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
Whiteboard: [blocking 2.1a1+]

Comment 24

7 years ago
Pushed as http://hg.mozilla.org/comm-central/rev/e9d005c309d8 - please mark fixed with appropriate target milestone if that was all for this bug.
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [blocking 2.1a1+]
Target Milestone: --- → seamonkey2.1a1

Comment 25

7 years ago
--- 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)?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 26

7 years ago
Oops. Question.png => question-32.png. Will attach a followup patch if you don't get to it first.
(Assignee)

Comment 27

7 years ago
Created attachment 444437 [details] [diff] [review]
Patch v1.2b Fix OS X missing image.
Attachment #444437 - Flags: review?(stefanh)

Updated

7 years ago
Attachment #444437 - Flags: review?(stefanh)
Attachment #444437 - Flags: review+
Attachment #444437 - Flags: approval-seamonkey2.1a1?

Updated

7 years ago
Attachment #444437 - Flags: approval-seamonkey2.1a2+
Attachment #444437 - Flags: approval-seamonkey2.1a1?
Attachment #444437 - Flags: approval-seamonkey2.1a1+

Comment 28

7 years ago
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

7 years ago
http://hg.mozilla.org/comm-central/rev/519b9b0655ac
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED

Comment 30

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

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

Updated

7 years ago
Blocks: 568447
You need to log in before you can comment on or make changes to this bug.