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

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect)

Description

[Serge Gautherie (:sgautherie)]

+++ 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.*.

Comment 1

[rsx11m]

Attachment: Proposed patch

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.

Comment 2

[rsx11m]

Attachment: Patch for checkin

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.

Comment 3

[rsx11m]

Push for trunk, please.

Comment 4

[Serge Gautherie (:sgautherie)]

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.

Comment 5

[rsx11m]

Thanks, still learning.  ;-)

Comment 6

[Mark Banner (:standard8)]

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

Comment 7

[Mark Banner (:standard8)]

Checked in:

http://hg.mozilla.org/comm-central/rev/3ef810f3fceb

Comment 8

[rsx11m]

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.

Comment 9

[Serge Gautherie (:sgautherie)]

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

Comment 10

[Serge Gautherie (:sgautherie)]

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

And no other Application.* either.