1179243 - Gaia changes for OpenMobile Application Compatibility Layer

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Comment 1

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: [PR 2.1s] acl integration

Kevin, could you take a look at this PR. Please see bug 1169426 for background. But the basic functionality is that the acl-daemon dev app should be able to execute commands against the system app. It mimics the communication channel, and there are buttons for each of the actions (like run app, close app, send notification, etc). Let me know if you have any questions about the details.

Comment 2

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: [2.0m PR] with updates from review

Comment 3

[Kevin Grandon :kgrandon]

Comment on attachment 8628551 [details] [review]
[PR 2.1s] acl integration

I wasn't really part of the discussions around implementation or partnership here, so a few things are confusing to me. From what I've been told is that we are unable to actually build and test this ourselves on devices? This seems like a strategic mistake for Mozilla to be implementing a workflow like this, and normally I would be against something like this out of principle.

The code does look fine though, and seems like it should work. While I can't actually test this, I will leave my principles at the door and give you an R+ because it seems like there is monumental pressure to get this landed. The feedback- is for the general approach/workflow and not your code, so feel free to land. It's just my silent form of protest and I'm hoping someone will play me a tiny violin.

Comment 4

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Kevin Grandon (PTO) :kgrandon from comment #3)
> Comment on attachment 8628551 [details] [review]
> [PR 2.1s] acl integration
> 
> I wasn't really part of the discussions around implementation or partnership
> here, so a few things are confusing to me. From what I've been told is that
> we are unable to actually build and test this ourselves on devices? This
> seems like a strategic mistake for Mozilla to be implementing a workflow
> like this, and normally I would be against something like this out of
> principle.

Completely agreed. Pushing to land this before we have a build to test is extremely risky and a backwards way of operating. I'm sorry to stick this on you, and I appreciate your timely review.

Comment 5

[[:fabrice] Fabrice Desré]

fwiw, I initially asked this to not ship in default builds. I'm not sure how easy it is to do with gaia build system though.

Comment 6

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Fabrice Desré [:fabrice] from comment #5)
> fwiw, I initially asked this to not ship in default builds. I'm not sure how
> easy it is to do with gaia build system though.

We do have a setting that disables this code if you don't have the right build flag. But yes, we do still include the code for default builds.

Comment 7

[Michael Henretty [:mikehenrty][:mhenretty]]

v2.0m: https://github.com/mozilla-b2g/gaia/commit/5bb23eb0c3d6cb14a1be5c8493b8e96b49ea2a8c

Comment 8

[Michael Henretty [:mikehenrty][:mhenretty]]

v2.1s https://github.com/mozilla-b2g/gaia/commit/c1a9fe0710a214df0dcabe89e45f3a9c41c282f9

Last patch landed. Let's open a new bug when we want to integrate this into master.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

(In reply to Michael Henretty [:mhenretty] from comment #8)
> v2.1s
> https://github.com/mozilla-b2g/gaia/commit/
> c1a9fe0710a214df0dcabe89e45f3a9c41c282f9
> 
> Last patch landed. Let's open a new bug when we want to integrate this into
> master.

This is causing Gaia unit test and Linter failures on v2.1s:
https://treeherder.mozilla.org/logviewer.html#?job_id=19866&repo=mozilla-b2g34_v2_1s
https://treeherder.mozilla.org/logviewer.html#?job_id=19870&repo=mozilla-b2g34_v2_1s

We don't have automation on v2.0m, so I have no idea if it's affected as well.

Comment 11

[GitHub Autolander Bot]

Attachment: [gaia] mozilla-b2g:bug-1179243-fix-tests > mozilla-b2g:v2.1s

Comment 12

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: [v2.1s PR] Test fixes

This fixes the unit test, and linter test. The pre-commit linter hook didn't catch this because newer versions of jshint don't detect the 'Notification' keyword as a global since it's a DOM object now. As for the unit test failure, I fixed it, but now I need to run this on try. I'm looking into that now.

Comment 13

[Michael Henretty [:mikehenrty][:mhenretty]]

On the bright-side, neither of these errors are actual bugs. So I don't think we need to upload to v2.0m. In the meantime, I'm running as many of the tests locally as I can before getting review for this.

Comment 14

[Michael Henretty [:mikehenrty][:mhenretty]]

Also to note, after speaking with :gaye there is no easy way to get a full test run of this code on v2.1s since we don't have a taskcluster gaia-try config for this. Hence the local testing.

Comment 15

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8631766 [details] [review]
[v2.1s PR] Test fixes

Spoke with kgrandon over IRC about this. r+ is good to carry. I was able to repro the failures locally and fixed them. It appears to be working fine now, and since 2.1s is not too active I'm going to land and just watch the tests.

Comment 16

[Michael Henretty [:mikehenrty][:mhenretty]]

v2.1s: https://github.com/mozilla-b2g/gaia/commit/8a614350a1f45f28e5a300bbd0233ee6d0a4738a