With bug 789224 in place, there are at least a few places we could easily get rid of references to "UniversalXPConnect". The purpose would be to make the code more understandable. - Eliminate nsContentUtils::CallerHasUniversalXPConnect by inlining its definition. - Eliminate IsUniversalXPConnectCapable in nsDOMWindowUtils.cpp by inlining its definition

The patches remove all instances of calls, but just waiting for TBPL to confirm all builds are successful. https://tbpl.mozilla.org/?tree=Try&rev=08254ce176db

Comment on attachment 674876 [details] [diff] [review] Eliminate IsUniversalXPConnectCapable Review of attachment 674876 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.

This isn't ready to check in yet. :)

Comment on attachment 674874 [details] [diff] [review] eliminate CallerHasUniversalXPConnect Review of attachment 674874 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I noticed a few more surrounding comments that should be updated. I'll r+ it when Bobby gets a chance to weigh in. We should get bholley to give a thumbs up to this. We want to wait until we are sure bug 789224 is going to stick, and he may also think my idea is a bad one. :) You should put the bug number and reviewer in the patch message for both patches to make it easier for people who check in the patch for you (I'm assuming you don't have level 3 given that you added the checkin-needed flag earlier). So, something like this: Bug 804164 - Eliminate CallerHasUniversalXPConnect with IsCallerChrome. r=mccr8 ::: content/base/src/nsContentUtils.cpp @@ +1605,5 @@ > return true; > } > > // The subject doesn't subsume aPrincipal. Allow access only if the subject > // has UniversalXPConnect. nit: Please update this comment to say "is chrome" instead of "has UniversalXPConnect" or something. @@ +1781,5 @@ > > bool > nsContentUtils::IsCallerTrustedForRead() > { > + return IsCallerChrome(); Hmm. I wonder if IsCallerTrustedForRead/Write should get the same treatment. Anyways, that can be a followup bug, if it is actually the right thing to do. ::: content/events/src/nsDOMDataTransfer.cpp @@ -446,1 @@ > // UniversalXPConnect privileges can always read the data. During the nit: update the comment to say "chrome privileges" instead. ::: content/html/content/src/nsHTMLInputElement.cpp @@ +1301,2 @@ > // setting the value of a "FILE" input widget requires the > // UniversalXPConnect privilege nit: please update this comment to say "chrome privilege" instead of "the UniversalXPConnect privilege" instead. ::: dom/base/nsGlobalWindow.cpp @@ +2600,5 @@ > { > NS_ASSERTION(GetScriptableTop() && > GetScriptableTop()->GetCurrentInnerWindowInternal() == this, > "DialogsAreBeingAbused called with invalid window"); > nit: while you are here, please remove the trailing whitespace on this blank line, and on the blank line below. ::: layout/style/nsComputedDOMStyle.cpp @@ -537,5 @@ > "should not have pseudo-element data"); > } > > // mExposeVisitedStyle is set to true only by testing APIs that > // require UniversalXPConnect. nit: please change this to "chrome privilege" instead of "UniversalXPConnect" or something.

Oh, and I can just check these patches in for you when the time comes.

Comment on attachment 674874 [details] [diff] [review] eliminate CallerHasUniversalXPConnect > // The subject doesn't subsume aPrincipal. Allow access only if the subject > // has UniversalXPConnect. >- return CallerHasUniversalXPConnect(); >+ return IsCallerChrome(); Fix comment. > bool > nsContentUtils::IsCallerTrustedForRead() > { >- return CallerHasUniversalXPConnect(); >+ return IsCallerChrome(); > } > > bool > nsContentUtils::IsCallerTrustedForWrite() > { >- return CallerHasUniversalXPConnect(); >+ return IsCallerChrome(); > } These functions should be eliminated, and their definitions inlined as well. I know mccr8 suggested doing this in a separate bug, but there's no time like the present. > // mExposeVisitedStyle is set to true only by testing APIs that > // require UniversalXPConnect. > NS_ABORT_IF_FALSE(!mExposeVisitedStyle || >- nsContentUtils::CallerHasUniversalXPConnect(), >+ nsContentUtils::IsCallerChrome(), > "mExposeVisitedStyle set incorrectly"); Update comment.

Thanks Bobby! Nikhil, update your patch to address our comments (including inlining IsCallerTrustedForRead and IsCallerTrustedForWrite) and then I'll review it again. The patch doesn't seem very dangerous, so I wouldn't bother with a try run again: just make sure it builds locally and runs for at least long enough to open a page or two. :)

Incidentally, dom/base/nsJSEnvironment.cpp has "CheckUniversalXPConnectForTraceMalloc()", I think that should be changed eventually. Working on comments for now. Same for: dom/base/nsDOMWindowUtils.cpp "IsUniversalXPConnectCapable()"

sorry, "Same for" doesn't apply in the above comment, I just fixed that

Comment on attachment 675218 [details] [diff] [review] eliminate CallerHasUniversalXPConnect Review of attachment 675218 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/src/nsHTMLInputElement.cpp @@ +1056,2 @@ > // setting the value of a "FILE" input widget requires the > // UniversalXPConnect privilege nit: this comment needs to be updated. I can just do this before I land the patch.

Comment on attachment 675221 [details] [diff] [review] Part 3 - Eliminate IsCallerTrustedForRead Review of attachment 675221 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.

Comment on attachment 675222 [details] [diff] [review] Part 4 - Eliminate IsCallerTrustedForWrite Review of attachment 675222 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Would you like me to land this now?

Yes please

Thanks for fixing this! remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/cad381167be7 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e21d04fa311b remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5b6d7523e355 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ca811b550726

https://hg.mozilla.org/mozilla-central/rev/cad381167be7 https://hg.mozilla.org/mozilla-central/rev/e21d04fa311b https://hg.mozilla.org/mozilla-central/rev/5b6d7523e355 https://hg.mozilla.org/mozilla-central/rev/ca811b550726