1185634 - SandboxPrivate shouldn't be public

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

I came across this comment in SandboxPrivate.h:

// This interface is public only because it is used in jsd.
// Once jsd is gone this file should be moved back to xpconnect/src.

jsd is gone, and it looks like, amazingly, nobody else started depending on it being public, so we live out the dream of bug 820170.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: part 1 - SandboxPrivate shouldn't be public.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Remove obsolete comment.

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 8636136 [details] [diff] [review]
part 1 - SandboxPrivate shouldn't be public.

This didn't really need to be split into two patches, but I was worried about whether a git file move plus a change would be properly picked up as a file move by hg.

This builds locally for me, and I audited to check that only xpcprivate.h includes this header, so that should be enough.

Comment 4

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8636136 [details] [diff] [review]
part 1 - SandboxPrivate shouldn't be public.

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

Thank you for doing this, I should have done this ages ago... if you have some more time, could you also move the file itself from xpconnect/public to xpconnect/src?

Comment 5

[Andrew McCreight [:mccr8]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #4)
> Thank you for doing this, I should have done this ages ago... if you have
> some more time, could you also move the file itself from xpconnect/public to
> xpconnect/src?

The patch does that. Splinter just doesn't show it for some reason:

diff --git a/js/xpconnect/public/SandboxPrivate.h b/js/xpconnect/src/SandboxPrivate.h
similarity index 100%
rename from js/xpconnect/public/SandboxPrivate.h
rename to js/xpconnect/src/SandboxPrivate.h

Comment 6

[Andrew McCreight [:mccr8]]

(In reply to Andrew McCreight [:mccr8] from comment #5)
> The patch does that. Splinter just doesn't show it for some reason:

I should have mentioned that, because it confused me, too. :)

As I said above, this compiles locally for me, and it is just a file move, so it shouldn't need a try run.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/36ef1a883f98
https://hg.mozilla.org/integration/mozilla-inbound/rev/df53edb6b9f7

Comment 8

[Gabor Krizsanits (INACTIVE)]

(In reply to Andrew McCreight [:mccr8] from comment #6)
> > The patch does that. Splinter just doesn't show it for some reason:

Ah yeah, it happened before yet I fell for it again :)

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/36ef1a883f98
https://hg.mozilla.org/mozilla-central/rev/df53edb6b9f7