1184404 - support additional navigator.getFeature keys for OpenMobile ACL

Status: RESOLVED FIXED

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

Description

[Bill Walker [:bwalker] [@wfwalker]]

This bug is to track the extension of the existing getFeature support for OpenMobile ACL. The proposal here is to allow apps with the "external-app" permission to call on any property matching getFeature("acl.*") and have the implementation retrieve the corresponding values from the android property_get.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Which getFeature is this about?

Comment 2

[Johnny Stenback (:jst)]

navigator.getFeature()

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

What's wrong with <extapp>.getCustomFeature("feature-foo")?

Comment 4

[Bill Walker [:bwalker] [@wfwalker]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #3)
> What's wrong with <extapp>.getCustomFeature("feature-foo")?

Hamzata, would you be OK with calling getCustomFeature() from within your JS code to access these other values?

Comment 5

[Hamzata Diallo]

(In reply to Bill Walker [:bwalker] [@wfwalker] from comment #4)
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #3)
> > What's wrong with <extapp>.getCustomFeature("feature-foo")?
> 
> Hamzata, would you be OK with calling getCustomFeature() from within your JS
> code to access these other values?

Bill, that is fine. The idea was to minimize the changes by leveraging the implementation of getFeature() we already have. But please remember that we can't use the plugin because ACL will might not be installed by the time we make this call.

Comment 6

[William Chen [:wchen]]

Attachment: Support additional navigator.getFeature keys for OpenMobile ACL

Comment 7

[Bill Walker [:bwalker] [@wfwalker]]

(In reply to William Chen [:wchen] from comment #6)
> Created attachment 8634580 [details] [diff] [review]
> Support additional navigator.getFeature keys for OpenMobile ACL

Awesome! One question -- this would require Firefox Marketplace to list "external-app" in its privileges when it calls getFeature("acl.version"), right?

Comment 8

[William Chen [:wchen]]

No, in the patch I uploaded, acl.version is a special case and doesn't require the "external-app" permission.

Comment 9

[Bill Walker [:bwalker] [@wfwalker]]

(In reply to William Chen [:wchen] from comment #8)
> No, in the patch I uploaded, acl.version is a special case and doesn't
> require the "external-app" permission.

I see it now. I misread the patch. Triple awesome.

Comment 10

[Didem Ersoz [:didem]]

Hamzata,

William already created the patch for the issue we discussed yesterday. Could you test it and inform us that it works. So that, once we get your ok, we can land it in the 2.1s and 2.0m branches. 

thanks,
didem

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8634580 [details] [diff] [review]
Support additional navigator.getFeature keys for OpenMobile ACL

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

Not using the thing that we custom built for this exact use case is dumb, but w/e.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bcf82e1e10a

Comment 13

[William Chen [:wchen]]

Comment on attachment 8634580 [details] [diff] [review]
Support additional navigator.getFeature keys for OpenMobile ACL

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 1169474
User impact if declined: It may be harder to ship ACL without the changes.
Testing completed: Manually tested on device.
Risk to taking this patch (and alternatives if risky): Low risk, changes are small and only compiles in Gonk. Affects apps with "external-app" permission which is only available to OpenMobile apps.
String or UUID changes made by this patch: None

Comment 14

[Gregor Wagner [:gwagner]]

Comment on attachment 8634580 [details] [diff] [review]
Support additional navigator.getFeature keys for OpenMobile ACL

Approving since Fabrice is out.

Comment 15

[Gregor Wagner [:gwagner]]

Adding Josh and Mahe so they are aware of the approvals.

Comment 16

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

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/367b6867e3ff
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/367b6867e3ff
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/29d54b125d3a
http://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/29d54b125d3a
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/e2d1f1f55803

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/1bcf82e1e10a

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/e2d1f1f55803

Comment 19

[Hamzata Diallo]

(In reply to Didem Ersoz [:didem] from comment #10)
> Hamzata,
> 
> William already created the patch for the issue we discussed yesterday.
> Could you test it and inform us that it works. So that, once we get your ok,
> we can land it in the 2.1s and 2.0m branches. 
> 
> thanks,
> didem

For clearing the needinfo flag...