Last Comment Bug 734544 - Replace last Application.prefs uses by Services.prefs, in MailNews Core
: Replace last Application.prefs uses by Services.prefs, in MailNews Core
Status: VERIFIED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 14.0
Assigned To: rsx11m
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on:
Blocks: 720356
  Show dependency treegraph
 
Reported: 2012-03-09 17:48 PST by Serge Gautherie (:sgautherie)
Modified: 2012-03-15 18:25 PDT (History)
2 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed patch (2.46 KB, patch)
2012-03-09 19:10 PST, rsx11m
mozilla: review+
bugzillamozillaorg_serge_20140323: feedback+
Details | Diff | Splinter Review
Patch for checkin (2.49 KB, patch)
2012-03-14 20:01 PDT, rsx11m
no flags Details | Diff | Splinter Review

Description Serge Gautherie (:sgautherie) 2012-03-09 17:48:47 PST
+++ 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 2012-03-09 19:10:44 PST
Created attachment 604594 [details] [diff] [review]
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 2012-03-14 20:01:45 PDT
Created attachment 606088 [details] [diff] [review]
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 2012-03-14 20:02:47 PDT
Push for trunk, please.
Comment 4 Serge Gautherie (:sgautherie) 2012-03-14 20:28:36 PDT
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 2012-03-14 20:29:41 PDT
Thanks, still learning.  ;-)
Comment 6 Mark Banner (:standard8) 2012-03-15 02:32:20 PDT
(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) 2012-03-15 02:37:05 PDT
Checked in:

http://hg.mozilla.org/comm-central/rev/3ef810f3fceb
Comment 8 rsx11m 2012-03-15 06:11:27 PDT
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) 2012-03-15 18:24:35 PDT
(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) 2012-03-15 18:25:41 PDT
(In reply to Serge Gautherie (:sgautherie) from comment #9)
> Ftr, no Application.prefs remains in MNCore atm.

And no other Application.* either.

Note You need to log in before you can comment on or make changes to this bug.