Last Comment Bug 718255 - Merge nsIPrefBranch2 with nsIPrefBranch
: Merge nsIPrefBranch2 with nsIPrefBranch
Status: RESOLVED FIXED
: addon-compat, dev-doc-complete
Product: Core
Classification: Components
Component: Preferences: Backend (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Geoff Lankow (:darktrojan)
:
Mentors:
Depends on:
Blocks: 727408
  Show dependency treegraph
 
Reported: 2012-01-14 20:02 PST by Geoff Lankow (:darktrojan)
Modified: 2015-01-09 22:02 PST (History)
9 users (show)
geoff: in‑testsuite-
geoff: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part A, merge interfaces (11.08 KB, patch)
2012-01-20 02:05 PST, Geoff Lankow (:darktrojan)
benjamin: review+
gavin.sharp: superreview+
Details | Diff | Review
part B, modules/ (6.63 KB, patch)
2012-01-20 02:06 PST, Geoff Lankow (:darktrojan)
benjamin: review+
Details | Diff | Review
part C, toolkit/ (37.15 KB, patch)
2012-01-20 02:07 PST, Geoff Lankow (:darktrojan)
benjamin: review+
Details | Diff | Review
part D, browser/ and mobile/ (20.96 KB, patch)
2012-01-20 02:08 PST, Geoff Lankow (:darktrojan)
benjamin: review+
Details | Diff | Review
part E, netwerk/ (23.45 KB, patch)
2012-01-20 02:08 PST, Geoff Lankow (:darktrojan)
benjamin: review+
Details | Diff | Review
part F, testing/ and layout/tools/reftest/ (7.85 KB, patch)
2012-01-20 02:09 PST, Geoff Lankow (:darktrojan)
benjamin: review+
Details | Diff | Review
part G, everything else (29.96 KB, patch)
2012-01-20 02:09 PST, Geoff Lankow (:darktrojan)
benjamin: review+
Details | Diff | Review
part H (3.37 KB, patch)
2012-02-24 14:57 PST, Geoff Lankow (:darktrojan)
gavin.sharp: review-
Details | Diff | Review
part H (3.33 KB, patch)
2012-02-24 20:55 PST, Geoff Lankow (:darktrojan)
mak77: review+
Details | Diff | Review

Description Geoff Lankow (:darktrojan) 2012-01-14 20:02:14 PST
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?
Comment 1 Geoff Lankow (:darktrojan) 2012-01-20 02:05:21 PST
Created attachment 590145 [details] [diff] [review]
part A, merge interfaces
Comment 2 Geoff Lankow (:darktrojan) 2012-01-20 02:06:51 PST
Created attachment 590146 [details] [diff] [review]
part B, modules/
Comment 3 Geoff Lankow (:darktrojan) 2012-01-20 02:07:38 PST
Created attachment 590148 [details] [diff] [review]
part C, toolkit/
Comment 4 Geoff Lankow (:darktrojan) 2012-01-20 02:08:14 PST
Created attachment 590149 [details] [diff] [review]
part D, browser/ and mobile/
Comment 5 Geoff Lankow (:darktrojan) 2012-01-20 02:08:49 PST
Created attachment 590150 [details] [diff] [review]
part E, netwerk/
Comment 6 Geoff Lankow (:darktrojan) 2012-01-20 02:09:27 PST
Created attachment 590151 [details] [diff] [review]
part F, testing/ and layout/tools/reftest/
Comment 7 Geoff Lankow (:darktrojan) 2012-01-20 02:09:55 PST
Created attachment 590153 [details] [diff] [review]
part G, everything else
Comment 8 Benjamin Smedberg [:bsmedberg] 2012-01-23 07:47:00 PST
I will get to this review on Thurdsay, I'm traveling this week.
Comment 9 :Ms2ger 2012-02-06 07:24:48 PST
Just to be sure: all these patches build on their own?
Comment 10 Geoff Lankow (:darktrojan) 2012-02-06 15:02:43 PST
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.
Comment 12 Geoff Lankow (:darktrojan) 2012-02-13 16:09:50 PST
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.
Comment 14 Kris Maglione [:kmag] 2012-02-14 17:34:43 PST
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.
Comment 15 Benjamin Smedberg [:bsmedberg] 2012-02-15 05:47:29 PST
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.
Comment 16 Geoff Lankow (:darktrojan) 2012-02-24 14:57:51 PST
Created attachment 600535 [details] [diff] [review]
part H

Last one (hopefully)
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-24 16:39:35 PST
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?
Comment 18 Geoff Lankow (:darktrojan) 2012-02-24 20:55:01 PST
Created attachment 600613 [details] [diff] [review]
part H

Oh, uhh, whoops.
Comment 19 Geoff Lankow (:darktrojan) 2012-02-25 14:35:29 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8a1e874238a
Comment 20 Phil Ringnalda (:philor) 2012-02-26 16:20:47 PST
https://hg.mozilla.org/mozilla-central/rev/a8a1e874238a
Comment 22 editor.buzzfeed 2015-01-09 22:02:04 PST Comment hidden (spam)

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