Last Comment Bug 806587 - Rename getNoAppCodebasePrincipal back to getCodebasePrincipal
: Rename getNoAppCodebasePrincipal back to getCodebasePrincipal
Status: RESOLVED FIXED
: addon-compat
Product: Core
Classification: Components
Component: Security: CAPS (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla19
Assigned To: Jonas Sicking (:sicking)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-29 15:20 PDT by Alex Keybl [:akeybl]
Modified: 2012-11-09 06:43 PST (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
+
fixed


Attachments
Patch to fix (4.71 KB, patch)
2012-10-30 10:17 PDT, Jonas Sicking (:sicking)
mounir: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Review

Description Alex Keybl [:akeybl] 2012-10-29 15:20:48 PDT
(Jonas Sicking (:sicking) from bug 774585 comment #31)
> I'd be fine with naming back getNoAppCodebasePrincipal to
> getCodebasePrincipal if it's really needed. But if it's not affecting too
> many addons I'd prefer to keep things as-is.

Let's do this due to the add-on compatibility impact.
Comment 1 Alex Keybl [:akeybl] 2012-10-29 15:21:18 PDT
Can we get this into tomorrow's beta to prevent any more add-on compat impact?
Comment 2 Jonas Sicking (:sicking) 2012-10-30 10:17:58 PDT
Created attachment 676659 [details] [diff] [review]
Patch to fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 774585
User impact if declined: some addons will break. Unclear which
Testing completed (on m-c, etc.): None
Risk to taking this patch (and alternatives if risky): Very low to none. The patch adds a very simple function which just forwards to an existing call.
String or UUID changes made by this patch: nsIScriptSecurityManager iid is changed.
Comment 3 Mounir Lamouri (:mounir) 2012-10-30 10:35:46 PDT
Comment on attachment 676659 [details] [diff] [review]
Patch to fix

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

r=me with the comments applied.

::: caps/idl/nsIScriptSecurityManager.idl
@@ +185,5 @@
> +    /**
> +     * DEPRECATED!
> +     * Legacy name for getNoAppCodebasePrincipal.
> +     */
> +    nsIPrincipal getCodebasePrincipal(in nsIURI uri);

Use [deprecated], please :)

https://developer.mozilla.org/en-US/docs/XPIDL

::: caps/src/nsScriptSecurityManager.cpp
@@ +2022,5 @@
>  NS_IMETHODIMP
> +nsScriptSecurityManager::GetCodebasePrincipal(nsIURI* aURI,
> +                                              nsIPrincipal** aPrincipal)
> +{
> +  return GetNoAppCodebasePrincipal(aURI, aPrincipal);

Stay consistent with other implementation and call "GetSimpleCodebasePrincipal".
Comment 4 Jonas Sicking (:sicking) 2012-10-30 10:40:24 PDT
> @@ +2022,5 @@
> >  NS_IMETHODIMP
> > +nsScriptSecurityManager::GetCodebasePrincipal(nsIURI* aURI,
> > +                                              nsIPrincipal** aPrincipal)
> > +{
> > +  return GetNoAppCodebasePrincipal(aURI, aPrincipal);
> 
> Stay consistent with other implementation and call
> "GetSimpleCodebasePrincipal".

That would be the wrong implementation as that would use the UNKNOWN_APP appid.
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-10-30 18:51:12 PDT
Landed with a presumed a+ from lsblakk per his request on IRC to get these landed on aurora/beta ASAP for 17b4.

https://hg.mozilla.org/releases/mozilla-aurora/rev/54f51455ee2e
https://hg.mozilla.org/releases/mozilla-beta/rev/ae08e43155c3
Comment 7 Ed Morley [:emorley] 2012-10-31 07:19:16 PDT
https://hg.mozilla.org/mozilla-central/rev/434c6bdec0bb
Comment 8 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-31 08:05:13 PDT
Comment on attachment 676659 [details] [diff] [review]
Patch to fix

yes, belated approval - i had been looking for it to get on central first. thank you for getting it into beta in time.
Comment 9 Ian Nartowicz 2012-11-09 06:43:45 PST
Beautiful.  All works again in 17.0b4.  Do we need a console message that this method is going away in favour of the new one?  Or is it going to stay?

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