Closed Bug 900707 Opened 6 years ago Closed 6 years ago

Add macro for b2g permission code

Categories

(Core :: DOM: Core & HTML, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: evilpie, Assigned: evilpie)

References

Details

Attachments

(3 files, 4 obsolete files)

With e10s enabled we suddenly started running b2g only permission checks. We should have an easily findable way to disabling these on desktop for now.
I talked to some of the build folks and they suggest something like #ifdef MOZ_B2G && MOZ_PERMISSION. Arguably there are not to many places to fix anyway.
Assignee: nobody → evilpies
Attached patch permission-configure v1 (obsolete) — Splinter Review
So here is the idea of this patch, have MOZ_APP_PERMISSIONS be always true on B2G and fail is somebody disables MOZ_PERMISSIONS while enabled app-permissions. I have never done anything with configure.in, so I don't really know what I'm doing.
Attachment #789094 - Flags: review?(mh+mozilla)
Attached patch app-permissionsSplinter Review
This patch actually uses the new define to disable most of the permission code that I know is causing problems.
Attachment #789095 - Flags: review?(jonas)
Attachment #789095 - Flags: review?(felipc)
Comment on attachment 789094 [details] [diff] [review]
permission-configure v1

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

::: configure.in
@@ +5798,5 @@
> +if test -n "$MOZ_APP_PERMISSIONS"; then
> +    if test -n "$MOZ_PERMISSIONS"; then
> +        AC_DEFINE(MOZ_APP_PERMISSIONS)
> +    else
> +        AC_MSG_ERROR([You need to enable MOZ_PERMISSIONS for MOZ_APP_PERMISSIONS])

--enable-permissions for --enable-app-permissions.

That being said, is there really a need for a configure option? If you need testing on non-b2g environments, you can still set MOZ_APP_PERMISSIONS in .mozconfig.
Attachment #789094 - Flags: review?(mh+mozilla) → review+
That sounds good, I don't think we really need a flag for this.
Comment on attachment 789095 [details] [diff] [review]
app-permissions

Looks reasonable to me, but I'm not really a peer.. I suppose you can land if you're eager to, as it's all ifdef'ed code, but I'll leave the pending review flag for Jonas so he has a chance to take a look at it.

(Please run on tryserver first to make sure it doesn't break b2g/android!)

And don't forget to address Glandium's comment 4
Attachment #789095 - Flags: review?(felipc) → review+
Comment on attachment 789095 [details] [diff] [review]
app-permissions

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

I'm not sure this is "app" specific. But rather specific to if we want child processes to run in security sandboxes or not. So maybe use a macro name that indicates that better.
Attachment #789095 - Flags: review?(jonas) → review+
Attached patch permission-configure v2 (obsolete) — Splinter Review
I tried removing this pref, but somehow this looks really weird now. Any suggestions on how to make this nicer?
Attachment #789094 - Attachment is obsolete: true
Attachment #791523 - Flags: review?(mh+mozilla)
Comment on attachment 791523 [details] [diff] [review]
permission-configure v2

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

::: configure.in
@@ +5803,5 @@
> +dnl Child permissions, currently only used for b2g
> +dnl ========================================================
> +if test -n "$MOZ_B2G"; then
> +    if test -n "$MOZ_PERMISSIONS"; then
> +        AC_DEFINE(MOZ_CHILD_PERMISSIONS)

Please don't use an AC_DEFINE for something that is only used in one place.
Do MOZ_CHILD_PERMISSIONS=1, and let AC_SUBST deal with it (which, without such a value would have achieved nothing)

Then, in the appropriate Makefile.in, add:
ifdef MOZ_CHILD_PERMISSIONS
DEFINES += -DMOZ_CHILD_PERMISSIONS
endif
Attachment #791523 - Flags: review?(mh+mozilla) → review-
Thanks for reviewing. Is this more what you were looking for? At some point I really need to try learning this...
Attachment #791523 - Attachment is obsolete: true
Attachment #792578 - Flags: review?(mh+mozilla)
Attachment #792578 - Flags: review?(mh+mozilla) → review+
Looks like we do test some app permission code on Desktop: https://tbpl.mozilla.org/?tree=Try&rev=cb6390c3d913. I guess we could just disable the tests?
Attached patch disable tests (obsolete) — Splinter Review
Okay let's do it then.
Attachment #793635 - Flags: review?(gavin.sharp)
Shouldn't you use MOZ_APP_PERMISSIONS (or whatever you renamed it to to address sicking's comment) rather than MOZ_B2G? Probably best to get someone who knows this test to review.
Attachment #793635 - Attachment is obsolete: true
Attachment #793635 - Flags: review?(gavin.sharp)
Attached patch disable tests v2 (obsolete) — Splinter Review
Attachment #794038 - Flags: review?(bugs)
So basically the idea is to disable the test when we disable all the permission checks. In theory that would mean these would run on B2G, but this two tests are actually explicitly disabled with the b2g.json file. I don't think this is really my problem at the moment and I would like to land these patches.
Attachment #794038 - Attachment is obsolete: true
Attachment #794038 - Flags: review?(bugs)
Attachment #794040 - Flags: review?(bugs)
Comment on attachment 794040 [details] [diff] [review]
disable tests v2.1

rs+
Attachment #794040 - Flags: review?(bugs) → review+
Blocks: 908692
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.