820582 - Add private browsing package.json flag

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Comment 1

[Erik Vold [:erikvold] (please needinfo? me)]

I am thinking about

Comment 2

[Erik Vold [:erikvold] (please needinfo? me)]

I am thinking about adding a private-browsing opt-in key to package.json which would mean modules like context-menu, widget, page-mod, and probably more modules would all need to be disabled on pb enabled windows by default, unless the developer opts-in to be used on those windows.

Can I get your thoughts on this Dave, Irakli, and anyone else?

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Would it be easier to require the addon to require("private-browsing") than to require adding things to package.json?

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Would it be easier to require the addon to require("private-browsing") than to require adding things to package.json?

Comment 5

[Jeff Griffiths (:canuckistani) (:⚡︎)]

As I stated on the list, I'm concerned that this flag doesn't give the user much choice as to whether an add-on should work or not in PB mode.

Comment 6

[Dave Townsend [:mossop]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #2)
> I am thinking about adding a private-browsing opt-in key to package.json
> which would mean modules like context-menu, widget, page-mod, and probably
> more modules would all need to be disabled on pb enabled windows by default,
> unless the developer opts-in to be used on those windows.
> 
> Can I get your thoughts on this Dave, Irakli, and anyone else?

Why those modules? By themselves they do nothing that is against the rules of private browsing. It'd be better to disabling things like the prefs modules which actually have the ability to store private data, but I'm not sure that is a great idea either.

AMO has a policy that add-ons be required to properly monitor PB state and behave accordingly, I think we should rely on that rather than trying to put in safeguards that aren't actually protecting the thing we need to protect.

Comment 7

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Dave Townsend (:Mossop) from comment #6)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #2)
> > I am thinking about adding a private-browsing opt-in key to package.json
> > which would mean modules like context-menu, widget, page-mod, and probably
> > more modules would all need to be disabled on pb enabled windows by default,
> > unless the developer opts-in to be used on those windows.
> > 
> > Can I get your thoughts on this Dave, Irakli, and anyone else?
> 
> Why those modules? By themselves they do nothing that is against the rules
> of private browsing. It'd be better to disabling things like the prefs
> modules which actually have the ability to store private data, but I'm not
> sure that is a great idea either.
> 
> AMO has a policy that add-ons be required to properly monitor PB state and
> behave accordingly, I think we should rely on that rather than trying to put
> in safeguards that aren't actually protecting the thing we need to protect.

Perhaps I was too concerned with avoiding add-on breakage, that is the only reason I had for bringing up this safeguard which looks more and more like a rabbit hole now.

I'm ok with simply changing the behavior of the panel module and reaching out to developers that use it too.

Comment 8

[Jeff Griffiths (:canuckistani) (:⚡︎)]

As per the discussion in triage today, we were thinking a default pref could be added to package.json for pwpb opt-in.

Comment 9

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/763

Pointer to Github pull-request

Comment 10

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 711204 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/763

Please note: There were bunch of pull requests containing each other, so comments may be little spread out.

Comment 11

[Erik Vold [:erikvold] (please needinfo? me)]

I added docs and got the other comments I believe, ready for another round now Irakli.

Comment 12

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from bug 839746 comment #4)

> Maybe property name can be made more descriptive though, like
> `supports-private-browsing: true, { supports: { private-browsing: true } }`
> or something, I'll let native ENG speakers bike-shed / decide on proper
> naming.

How about `{ permissions: [ "private-browsing" ] }` like chrome extensions:

http://developer.chrome.com/extensions/declare_permissions.html

Comment 13

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #12)
> (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from bug
> 839746 comment #4)
> 
> > Maybe property name can be made more descriptive though, like
> > `supports-private-browsing: true, { supports: { private-browsing: true } }`
> > or something, I'll let native ENG speakers bike-shed / decide on proper
> > naming.
> 
> How about `{ permissions: [ "private-browsing" ] }` like chrome extensions:
> 
> http://developer.chrome.com/extensions/declare_permissions.html

Maybe, I don't know to be honest, I don't in particular mind with previous option, it would be just great if you and wbamberg, mossop could figure correct naming.

Comment 14

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 711204 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/763

Looks good, just make sure to use full require paths as pointed out in review, and would be great if you can settle with Will / Mossop on property name before
landing it.

Comment 15

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

On the second thought we should probably follow the ff os lead here and copy their approach with app.manifest that is also similar to chrome extensions that you've suggested:

https://developer.mozilla.org/en-US/docs/Apps/Manifest?redirectlocale=en-US&redirectslug=OpenWebApps%2FThe_Manifest#permissions

Comment 16

[Alexandre Poirot [:ochameau]]

If we want to follow openwebapp manifest, (I'd vote for this), it would be something like this:

{
  "id": "my-addon-id",
  "permissions": {
    "private-browsing": {}
  }
}

It isn't just an array like chrome as we can pass optional arguments for each permission.
A common one is `access` property with read, write and readwrite possible values.

Comment 17

[Erik Vold [:erikvold] (please needinfo? me)]

I'm using the "permissions" property now, that and https://github.com/mozilla/addon-sdk/pull/763/files#diff-0 are the only changes, mind taking a quick review of that before I merge Irakli?

Comment 18

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/c2db6ffe65523b657f4e7c5a25da8a3bf8b47018
bug 820582: fix for tests that used loader w/out metadata

https://github.com/mozilla/addon-sdk/commit/b2ec37edb05d7b789ad780815ef88e44220a8b44
bug 820582: review updates, self module now exports isPrivateBrowsingSupported, and there is a test that this is true when it should be

https://github.com/mozilla/addon-sdk/commit/5b4fbc80d7c9f82122390ea5c0f54f5b308bccdf
Bug 820582: address review comment on diff in lib/sdk/loader/cuddlefish.js

https://github.com/mozilla/addon-sdk/commit/9fcbce9c79356c857f2c2830b0f384a72d229039
Bug 820582 adding baisc docs, more to come

https://github.com/mozilla/addon-sdk/commit/36d6971858670711017ced10d16168940c0b97e8
Bug 820582 - updating package.json to use permissions property

https://github.com/mozilla/addon-sdk/commit/7018afebad65d5529db30099bffa56e4a657b78b
Bug 820582: freezing metadata and metadata.permissions in bootstrap.js file now and no longer need to do this in the loader; freezing should only be done for add-on metadata

https://github.com/mozilla/addon-sdk/commit/9ded485783703a2447b46719bf60d60a64ba2c4c
bug 820582: updating package.json docs to reflect the new permissions key

https://github.com/mozilla/addon-sdk/commit/d1f3d5136fc2de16e2b8eae04dd8acebeafde42b
Merge pull request #763 from erikvold/820582

Fix Bug 820582 adding private-browsing flag to package.json r=@Gozala

Comment 19

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Forgot to clear needinfo after review.