Closed Bug 718255 Opened 13 years ago Closed 13 years ago

Merge nsIPrefBranch2 with nsIPrefBranch

Categories

(Core :: Preferences: Backend, defect)

Product:
Core
Component:
Preferences: Backend
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla13

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

(Keywords: addon-compat, dev-doc-complete)

Attachments

(8 files, 1 obsolete file)

part A, merge interfaces
(deleted), patch
benjamin
: review+
Gavin
: superreview+
Details | Diff | Splinter Review
part B, modules/
(deleted), patch
benjamin
: review+
Details | Diff | Splinter Review
part C, toolkit/
(deleted), patch
benjamin
: review+
Details | Diff | Splinter Review
part D, browser/ and mobile/
(deleted), patch
benjamin
: review+
Details | Diff | Splinter Review
part E, netwerk/
(deleted), patch
benjamin
: review+
Details | Diff | Splinter Review
part F, testing/ and layout/tools/reftest/
(deleted), patch
benjamin
: review+
Details | Diff | Splinter Review
part G, everything else
(deleted), patch
benjamin
: review+
Details | Diff | Splinter Review
part H
(deleted), patch
mak
: review+
Details | Diff | Splinter Review
It's time to get rid of nsIPrefBranch2. I'll merge it with nsIPrefBranch and leave an empty interface for compatibility. Benjamin, are you willing to do reviews for this?
Attached patch part A, merge interfaces (deleted) — — Splinter Review
Attachment #590145 - Flags: review?(benjamin)
Attached patch part B, modules/ (deleted) — — Splinter Review
Attachment #590146 - Flags: review?(benjamin)
Attached patch part C, toolkit/ (deleted) — — Splinter Review
Attachment #590148 - Flags: review?(benjamin)
Attached patch part D, browser/ and mobile/ (deleted) — — Splinter Review
Attachment #590149 - Flags: review?(benjamin)
Attached patch part E, netwerk/ (deleted) — — Splinter Review
Attachment #590150 - Flags: review?(benjamin)
Attachment #590151 - Flags: review?(benjamin)
Attached patch part G, everything else (deleted) — — Splinter Review
Attachment #590153 - Flags: review?(benjamin)
I will get to this review on Thurdsay, I'm traveling this week.
Attachment #590145 - Flags: review?(benjamin) → review+
Just to be sure: all these patches build on their own?
Attachment #590146 - Flags: review?(benjamin) → review+
Attachment #590148 - Flags: review?(benjamin) → review+
Attachment #590149 - Flags: review?(benjamin) → review+
Attachment #590150 - Flags: review?(benjamin) → review+
Attachment #590151 - Flags: review?(benjamin) → review+
Attachment #590153 - Flags: review?(benjamin) → review+
Comment on attachment 590145 [details] [diff] [review] part A, merge interfaces (In reply to Ms2ger from comment #9) > Just to be sure: all these patches build on their own? I sent it all to TryServer every time I started a new part. Only A and B actually change how things work.
Attachment #590145 - Flags: superreview?(dwitte)
Attachment #590145 - Flags: superreview?(dwitte) → superreview+
I'll need to follow this up with some new additions. Will do so after a few days in case there's stuff from other branches.
Whiteboard: [leave open after inbound merge]
It would be nice if a compatibility warning were emitted the first time an attempt is made to QI a preference branch to nsIPrefBranch2 so code that relies on it doesn't suddenly break without warning when it goes away.
Blocks: 727408
kmag, I don't think we have plans to remove nsIPrefBranch2 any time soon, so I don't think that the in-product compatibility warning is needed currently.
Attached patch part H (obsolete) (deleted) — — Splinter Review
Last one (hopefully)
Attachment #600535 - Flags: review?(benjamin)
Comment on attachment 600535 [details] [diff] [review] part H >diff --git a/toolkit/components/places/nsPlacesAutoComplete.js b/toolkit/components/places/nsPlacesAutoComplete.js >- if (aRegisterObserver) { >- let pb = this._prefs.QueryInterface(Ci.nsIPrefBranch2); >- pb.addObserver("", this, false); >- } >+ this._prefs.addObserver("", this, false); Don't you need to preserve the aRegisterObserver check?
Attachment #600535 - Flags: review?(benjamin) → review-
Attached patch part H (deleted) — — Splinter Review
Oh, uhh, whoops.
Attachment #600535 - Attachment is obsolete: true
Attachment #600613 - Flags: review?(gavin.sharp)
Attachment #600613 - Flags: review?(gavin.sharp) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8a1e874238a
Whiteboard: [leave open after inbound merge]
https://hg.mozilla.org/mozilla-central/rev/a8a1e874238a
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: