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

VERIFIED FIXED in Thunderbird 14.0

Status

MailNews Core
Backend
--
minor
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: sgautherie, Assigned: rsx11m)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 14.0
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
+++ 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-
(Reporter)

Updated

5 years ago
Blocks: 720356
(Assignee)

Comment 1

5 years ago
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.
Attachment #604594 - Flags: review?(dbienvenu)
(Reporter)

Updated

5 years ago
Attachment #604594 - Flags: feedback+
(Reporter)

Updated

5 years ago
Assignee: nobody → rsx11m.pub
Status: NEW → ASSIGNED

Updated

5 years ago
Attachment #604594 - Flags: review?(dbienvenu) → review+
(Assignee)

Comment 2

5 years ago
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.
Attachment #604594 - Attachment is obsolete: true
Attachment #606088 - Flags: review+
Attachment #606088 - Flags: feedback+
(Assignee)

Comment 3

5 years ago
Push for trunk, please.
Keywords: checkin-needed
Whiteboard: [c-n: comm-central]
(Reporter)

Comment 4

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

Comment 5

5 years ago
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
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: comm-central]
Target Milestone: --- → Thunderbird 14.0
(Assignee)

Comment 8

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

Comment 9

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

Comment 10

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