Closed Bug 734544 Opened 10 years ago Closed 10 years ago

Replace last Application.prefs uses by Services.prefs, in MailNews Core

Categories

(MailNews Core :: Backend, defect)

defect
Not set
minor

Tracking

(Not tracked)

VERIFIED FIXED
Thunderbird 14.0

People

(Reporter: sgautherie, Assigned: rsx11m.pub)

References

()

Details

Attachments

(2 files)

+++ This bug was initially created as a clone of Bug #724450 +++

"Found 3 matching lines in 2 files"

They all already have default values, so we can just use Services.*.
Flags: in-testsuite-
Blocks: 720356
Attached patch Proposed patchSplinter Review
The patch for this is a straightforward adaption from what was done for SeaMonkey with the following rationale (see mnyromyr's bug 724450 comment #12):

 - Services.prefs is "lighter" than Application.prefs (code simplification);

 - defaults for these prefs are defined in mailnews.js and libpref, a missing
   default can be considered a user/configuration error and throw an exception;

 - no redundancy maintaining the defaults both in prefs files and the code.
Attachment #604594 - Flags: review?(dbienvenu)
Attachment #604594 - Flags: feedback+
Assignee: nobody → rsx11m.pub
Status: NEW → ASSIGNED
Attachment #604594 - Flags: review?(dbienvenu) → review+
Thanks for the review. I don't know if I read the instructions linked to in Ryan's bug 724450 comment #16 correctly, but here is an updated patch with the commit message modified to reflect the review/feedback credits.
Attachment #604594 - Attachment is obsolete: true
Attachment #606088 - Flags: review+
Attachment #606088 - Flags: feedback+
Push for trunk, please.
Keywords: checkin-needed
Whiteboard: [c-n: comm-central]
Comment on attachment 606088 [details] [diff] [review]
Patch for checkin

(In reply to rsx11m from comment #2)
> I don't know if I read the instructions linked to in
> Ryan's bug 724450 comment #16 correctly, but here is an updated patch with
> the commit message modified to reflect the review/feedback credits.

You did.
Though the most important is author and description (as in your first patch here): reviews can usually be automatically/easily added, unless non-obvious.

I think we usually don't forward-mark r+/f+ on attachments anymore, though we used to in the past: I assume one can include them in the patch description as need be.
Attachment #606088 - Flags: review+
Attachment #606088 - Flags: feedback+
Thanks, still learning.  ;-)
(In reply to Serge Gautherie (:sgautherie) from comment #4)
> I think we usually don't forward-mark r+/f+ on attachments anymore, though
> we used to in the past: I assume one can include them in the patch
> description as need be.

I've not heard that, I think it is fine to forward mark reviews - it highlights what's being taken into account.

Oh and normally we don't put feedback into the comments for landing - just reviews.
Checked in:

http://hg.mozilla.org/comm-central/rev/3ef810f3fceb
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: comm-central]
Target Milestone: --- → Thunderbird 14.0
Comment on attachment 604594 [details] [diff] [review]
Proposed patch

(In reply to Mark Banner (:standard8) from comment #6)
> I've not heard that, I think it is fine to forward mark reviews - it
> highlights what's being taken into account.

Ok, thus I'll keep the original patch visible for reference as a compromise.
Attachment #604594 - Attachment is obsolete: false
(In reply to Mark Banner (:standard8) from comment #6)

> I've not heard that, I think it is fine to forward mark reviews - it
> highlights what's being taken into account.

2 stories then: mine is it confuses things, whereas patch description + leaving r+ patches visible is a similar/better compromise :-|

> Oh and normally we don't put feedback into the comments for landing - just
> reviews.

2 stories too...

***

V.Fixed, per MXR search.

Ftr, no Application.prefs remains in MNCore atm.
Status: RESOLVED → VERIFIED
(In reply to Serge Gautherie (:sgautherie) from comment #9)
> Ftr, no Application.prefs remains in MNCore atm.

And no other Application.* either.
You need to log in before you can comment on or make changes to this bug.