Closed Bug 718255 Opened 9 years ago Closed 9 years ago

Merge nsIPrefBranch2 with nsIPrefBranch

Categories

(Core :: Preferences: Backend, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

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

Attachments

(8 files, 1 obsolete file)

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?
Attachment #590145 - Flags: review?(benjamin)
Attached patch part B, modules/Splinter Review
Attachment #590146 - Flags: review?(benjamin)
Attached patch part C, toolkit/Splinter Review
Attachment #590148 - Flags: review?(benjamin)
Attachment #590149 - Flags: review?(benjamin)
Attached patch part E, netwerk/Splinter Review
Attachment #590150 - Flags: review?(benjamin)
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) — 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 HSplinter 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: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.