Closed Bug 539066 Opened 10 years ago Closed 10 years ago

port bug 151244 to seamonkey, so return receipts keep working

Categories

(SeaMonkey :: MailNews: Message Display, defect)

defect
Not set

Tracking

(blocking-seamonkey2.1 a1+)

RESOLVED FIXED
seamonkey2.1a1
Tracking Status
blocking-seamonkey2.1 --- a1+

People

(Reporter: mvl, Assigned: philip.chee)

References

Details

Attachments

(2 files, 3 obsolete files)

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+
Keywords: helpwanted
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+
I'll make a stab at it (assuming that I can get trunk to build)
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Keywords: helpwanted
Attached patch Patch v1.0 Straight port. (obsolete) — Splinter Review
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
> 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
Attachment #442350 - Flags: superreview?(neil)
Attachment #442350 - Flags: superreview?(mnyromyr)
Attachment #442350 - Flags: review?(neil)
Attachment #442350 - Flags: review?(mnyromyr)
Comment on attachment 442350 [details] [diff] [review]
Patch v1.0 Straight port.

For some reason I got r? and sr? reversed.
> the xpcshell test from bug 151244 ran successfully.
But appears to test only the backend.
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?
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)
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.
(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?
sure, that would be nice, thx for thinking of that.
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?
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?]
Attached patch Patch v1.2 Fix Nits. sr=Neil (obsolete) — Splinter Review
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)
>> 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 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+
> I'm guessing the use of Warning still on classic is a typo?
> r=me with that explained/changed.

Oops.
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+
Keywords: checkin-needed
Whiteboard: [blocking 2.1a1+]
Pushed as http://hg.mozilla.org/comm-central/rev/e9d005c309d8 - please mark fixed with appropriate target milestone if that was all for this bug.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [blocking 2.1a1+]
Target Milestone: --- → seamonkey2.1a1
--- 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 → ---
Oops. Question.png => question-32.png. Will attach a followup patch if you don't get to it first.
Attachment #444437 - Flags: review?(stefanh)
Attachment #444437 - Flags: review?(stefanh)
Attachment #444437 - Flags: review+
Attachment #444437 - Flags: approval-seamonkey2.1a1?
Attachment #444437 - Flags: approval-seamonkey2.1a2+
Attachment #444437 - Flags: approval-seamonkey2.1a1?
Attachment #444437 - Flags: approval-seamonkey2.1a1+
Comment on attachment 444437 [details] [diff] [review]
Patch v1.2b Fix OS X missing image.

a=me for whichever it can be done for.
http://hg.mozilla.org/comm-central/rev/519b9b0655ac
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Not that a2 needs approval when the tree reopens ;-)

Still, I updated the a1 release config to include this one.
Blocks: 568447
You need to log in before you can comment on or make changes to this bug.