Last Comment Bug 811193 - nsIUrlListManager.safeLookup not updated with bug 775796
: nsIUrlListManager.safeLookup not updated with bug 775796
Status: RESOLVED FIXED
: addon-compat, dev-doc-needed
Product: Toolkit
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla20
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on: 813691
Blocks: 775796
  Show dependency treegraph
 
Reported: 2012-11-12 19:44 PST by Hector Zhao [:hectorz]
Modified: 2012-11-28 12:20 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
-
affected
-
fixed
fixed


Attachments
Patch (1.76 KB, patch)
2012-11-13 08:19 PST, Mounir Lamouri (:mounir)
jonas: superreview+
bzhao: feedback+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Review

Description Hector Zhao [:hectorz] 2012-11-12 19:44:00 PST
nsIUrlListManager.safeLookup is said to be the exception free wrapper[1] for nsIUrlClassifierDBService.lookup, but it was not changed with bug 775796 where the first argument to lookup switched from string to principal, and stops working since fx 17.

[1]: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/content/listmanager.js#309
Comment 1 Mounir Lamouri (:mounir) 2012-11-13 08:19:16 PST
Created attachment 681049 [details] [diff] [review]
Patch
Comment 2 Hector Zhao [:hectorz] 2012-11-13 16:24:00 PST
Comment on attachment 681049 [details] [diff] [review]
Patch

Review of attachment 681049 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not experienced enough to do a formal review, but with the changes in (https://hg.mozilla.org/mozilla-central/diff/88af948f119e/toolkit/components/url-classifier/nsIUrlClassifierDBService.idl) as a reference, this patch looks good to me.

Maybe a testcase should be included to make sure this doesn't happen again?
Comment 4 Phil Ringnalda (:philor) 2012-11-19 22:23:36 PST
https://hg.mozilla.org/mozilla-central/rev/8ab84754b536
Comment 5 Mark Banner (:standard8) 2012-11-21 15:23:25 PST
Bug 775796 may provide some hints
Comment 6 Mark Banner (:standard8) 2012-11-21 15:24:00 PST
Opps sorry commented on wrong bug.
Comment 7 Hector Zhao [:hectorz] 2012-11-21 17:02:22 PST
Not really sure, but I remember once being told these flags and keywords should be set.
Comment 8 Mounir Lamouri (:mounir) 2012-11-22 03:31:15 PST
Comment on attachment 681049 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 775796
User impact if declined: developers will no longer be able to use nsIUrlListManager.safeLookup
Risk to taking this patch (and alternatives if risky): there is only an UUID change, this is the only risk (there is no other code change). We might break addon compat but I believe we are soon enough in the m-a and m-b cycle that we could land this quite safely.
String or UUID changes made by this patch: yes, nsIUrlListManager
Comment 9 Mounir Lamouri (:mounir) 2012-11-22 03:33:10 PST
Note that we might think to land this in ESR (and Firefox 17?) if addons happen to be broken because of this.
Comment 10 Alex Keybl [:akeybl] 2012-11-26 15:43:48 PST
(In reply to Mounir Lamouri (:mounir) from comment #8)
> [Approval Request Comment]
> User impact if declined: developers will no longer be able to use
> nsIUrlListManager.safeLookup

This appears to be a regression in FF17. Do we know of any add-ons broken by bug 775796? We need to be able to weigh maintaining the status quo for another 6 weeks versus addon compatibility impact.

> String or UUID changes made by this patch: yes, nsIUrlListManager

Including Jorge to help make the call on addon compatibility impact to beta here.

We'll approve for Aurora in the meantime.
Comment 11 Jorge Villalobos [:jorgev] 2012-11-26 16:52:59 PST
I don't see any add-ons using safeLookup. Changing the interface now sounds okay to me for beta, but there is no urgency to push this through for add-ons.

I do see a couple of add-ons using functions that changed on bug 775796, which wasn't flagged addon-compat :\. However, they are very few and I can contact the developers directly.
Comment 12 Mounir Lamouri (:mounir) 2012-11-27 13:17:27 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/271ed0c9bc96
Comment 13 Alex Keybl [:akeybl] 2012-11-28 12:20:27 PST
Comment on attachment 681049 [details] [diff] [review]
Patch

Given Jorge's comment, let's leave this as is for FF18 (same as FF17).

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