Implement client commands triggered by SimplePush

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: nsm, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa+])

Attachments

(2 attachments, 7 obsolete attachments)

Comment hidden (empty)
Whiteboard: [qa+]
Making the purpose of this bug a little clearer. This refers to implementing the initial set of commands -- track, erase, ring and lock -- and having them be triggered by SimplePush.
Summary: Implement hooks into SimplePush → Implement client commands triggered by SimplePush
Created attachment 8376389 [details] [review]
Pull request 16325 on mozilla-b2g/gaia

This bug is mostly about the changes in the System app, for which Etienne would be the primary reviewer, but as discussed by email it would be awesome if James could have a look at the client logic.
Attachment #8376389 - Flags: review?(jlal)
Attachment #8376389 - Flags: review?(etienne)
Comment on attachment 8376389 [details] [review]
Pull request 16325 on mozilla-b2g/gaia

I'm on PTO next week but I want to review this when I will come back.

There is a lot of things in the patch for the system app and the patch for the settings part. I want to understand them correctly before this stuff is landed since some parts are not crystal clear to me.
Attachment #8376389 - Flags: review?(21)
Comment on attachment 8376389 [details] [review]
Pull request 16325 on mozilla-b2g/gaia

Hey Guilherme,

It's really cool to see this patch shaping up, glad to see the integration test worked out :)

Let's make sure everything is ready for :21's review.

I know there was some talk about making findmydevice a separate hidden app. Is there anything blocking?
Since everything is driven by Alarm/Push it should be doable, and it will make landing this preffed-off a lot easier :)

If the issue is to hide the app, I'm sure we can add a new hidden_role [1].

If the issue is to make sure the app is launched at least once to make sure the alarm/push is set, we could keep ~5 lines of code in the system app for that :)

Also, even if it's not blocking, let's bug our UX/Design friends for a lockscreen message design, we might get it in time for landing.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/grid.js#L17
Attachment #8376389 - Flags: review?(etienne)
Where's my fox let's you (besides other things) remotely lock your phone and display a message on the lockscreen.

Currently we display black text on a white rectangle with rounded corners, I'm sure we can do better :)
Flags: needinfo?(firefoxos-ux-bugzilla)
> (In reply to Etienne Segonzac (:etienne) from comment #4)
> I know there was some talk about making findmydevice a separate hidden app.
> Is there anything blocking?

I'll get started on that now, and hopefully have something to show by tomorrow.
 
> Also, even if it's not blocking, let's bug our UX/Design friends for a
> lockscreen message design, we might get it in time for landing.

That would be cool, thanks for remembering that. The current design is based on a some early wireframes at bug 938896, but I certainly wouldn't mind changing it to something nicer.

Comment 7

5 years ago
Flagging Peko to advise on visual design for this aspect of Where's My Fox, and adding Jacqueline just so she knows this is in play.
Flags: needinfo?(pchen)
Flags: needinfo?(jsavory)
Flags: needinfo?(firefoxos-ux-bugzilla)
Comment on attachment 8376389 [details] [review]
Pull request 16325 on mozilla-b2g/gaia

Giving review to a more prompt reviewer (kgrandon)
Attachment #8376389 - Flags: review?(jlal) → review?(kgrandon)
Comment on attachment 8376389 [details] [review]
Pull request 16325 on mozilla-b2g/gaia

I added some comments on github - I was mostly testing for style. I will let Vivien take a look at the architecture of everything, and will do another pass once he has made some comments. Clearing review flag for now, but please re-request review once the patch is updated.
Attachment #8376389 - Flags: review?(kgrandon)
Comment on attachment 8376389 [details] [review]
Pull request 16325 on mozilla-b2g/gaia

I made some progress on splitting this into a separate app, and fixed all of Kevin's comments. Etienne, can you please have another look?

A couple of notes on separating into a new app:

- As Etienne and I had discused on IRC, I had to introduce a new 'lockscreen.lock-immediately' setting to replace the call to lockIfEnabled() that made the lockscreen lock.

- Since we need to attach listeners to the settings 'findmydevice.enabled' and 'findmydevice.registered' to control the whole client state machine, I actually had to set the app to be launched on boot. Please let me know if findmydevice_launcher.js is not the correct way to do it.

I haven't rebased the new changes for ease of reviewing, but I can do it if you think a cleaner history will make it easier for you.
Attachment #8376389 - Flags: review?(etienne)
Created attachment 8382257 [details] [diff] [review]
wimf.patch

I'm attaching the patch here as it is much easier to review it this way imho.
Attachment #8376389 - Attachment is obsolete: true
Attachment #8376389 - Flags: review?(etienne)
Attachment #8376389 - Flags: review?(21)
Attachment #8382257 - Flags: review?(etienne)
Attachment #8382257 - Flags: review?(21)
Created attachment 8382262 [details] [diff] [review]
findmydevice.patch

I thought github was smart enough to generate a nice patch but it is not. snif.
Attachment #8382257 - Attachment is obsolete: true
Attachment #8382257 - Flags: review?(etienne)
Attachment #8382257 - Flags: review?(21)
Attachment #8382262 - Flags: review?(etienne)
Attachment #8382262 - Flags: review?(21)
I started the review but I think my biggest question is do you really need to start the app at system startup ?

If yes it sounds a bit weird to me since the app can be killed when the device does not have enough free memory. And then you will ends up in a state similar to not having already opened the app.
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #13)
> I started the review but I think my biggest question is do you really need
> to start the app at system startup ?

I'm starting it on system startup so I can attach callbacks for changes to the 'findmydevice.enabled' setting, which is exposed in the Settings app, so we can register/ping the server when enabled. Is there a way to do that that does not involve launching on startup? Perhaps I can use the inter-app communication API to wake up the Find My Device app from the Settings app instead of relying on a setting?

I *think* that is the main reason right now, and if we can get around that, then it looks like I could work out the other necessary changes. I expect most of the other events (push and alarms) to wake up the app as necessary.

The only other issue I can think of right now is that we need to listen for Firefox Accounts logout events to disable Find My Device if that happens, and that means calling navigator.id.watch({...}) at some point. I might be able to work something out with the Firefox Accounts folks to have the app be woken up on logout, but in the worst case the call to navigator.id.watch({...}) could be done from the System app, right?
(In reply to Guilherme Gonçalves [:ggp] from comment #14)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #13)
> > I started the review but I think my biggest question is do you really need
> > to start the app at system startup ?
> 
> I'm starting it on system startup so I can attach callbacks for changes to
> the 'findmydevice.enabled' setting, which is exposed in the Settings app, so
> we can register/ping the server when enabled. Is there a way to do that that
> does not involve launching on startup? Perhaps I can use the inter-app
> communication API to wake up the Find My Device app from the Settings app
> instead of relying on a setting?
>

Inter app should wake up the app the way you want.
 
> I *think* that is the main reason right now, and if we can get around that,
> then it looks like I could work out the other necessary changes. I expect
> most of the other events (push and alarms) to wake up the app as necessary.
> 

yes, push and alarms will wake up the app.

> The only other issue I can think of right now is that we need to listen for
> Firefox Accounts logout events to disable Find My Device if that happens,
> and that means calling navigator.id.watch({...}) at some point. I might be
> able to work something out with the Firefox Accounts folks to have the app
> be woken up on logout, but in the worst case the call to
> navigator.id.watch({...}) could be done from the System app, right?

I don't know enough navigator.id. Let's ask ferjm.
Flags: needinfo?(ferjmoreno)
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #15)
> (In reply to Guilherme Gonçalves [:ggp] from comment #14)> 
> > The only other issue I can think of right now is that we need to listen for
> > Firefox Accounts logout events to disable Find My Device if that happens,
> > and that means calling navigator.id.watch({...}) at some point. I might be
> > able to work something out with the Firefox Accounts folks to have the app
> > be woken up on logout, but in the worst case the call to
> > navigator.id.watch({...}) could be done from the System app, right?
> 
> I don't know enough navigator.id. Let's ask ferjm.

The System app already receives 'onlogout' events after bug 952063. Check [1]. Maybe you can take advantage of that.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/fxa_manager.js#L132
Flags: needinfo?(ferjmoreno)
Comment on attachment 8382262 [details] [diff] [review]
findmydevice.patch

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

::: apps/findmydevice/js/lib/hawk.js
@@ +1,5 @@
> +/*
> +    HTTP Hawk Authentication Scheme
> +    Copyright (c) 2012-2013, Eran Hammer <eran@hueniverse.com>
> +    MIT Licensed
> +*/

We already have a HAWK implementation in Gecko [1][2], maybe we can use that code instead of adding all this code to Gaia. We can build an IAC based API using the System app to proxy the requests as we did with FxA or simply expose a certified only DOM API. The HAWK client might be needed by other certified apps in the future.

I am not saying that we should do this for MVP, but maybe this could be something to take into account for future versions.

[1] https://mxr.mozilla.org/mozilla-central/source/services/common/hawkrequest.js
[2] https://mxr.mozilla.org/mozilla-central/source/services/common/hawkclient.js
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #16)
> The System app already receives 'onlogout' events after bug 952063. Check
> [1]. Maybe you can take advantage of that.

Ah, I think I see what you mean. So it should be possible to use shared/js/fxa_iac_client.js to subscribe to FxA events the way I want. I'll see how far I can get taking inspiration on :_6a68's work on the Settings app [1], but this looks doable, and hopefully we can land this with FxA already :)

About the HAWK stuff, it would indeed be great if we could share code, but I agree we should leave it out of the scope of this bug. I filed bug 977552 for that.

1- https://github.com/6a68/gaia/blob/6eec0b06053b6e508b8ad559d7fb5d4e2d56948c/apps/settings/js/firefox_accounts/menu.js
Comment on attachment 8382262 [details] [diff] [review]
findmydevice.patch

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

Looks like this is in very good hands, clearing the r? for now.
Feel free to add it again if we have some tests/lockscreen related follow ups.
Attachment #8382262 - Flags: review?(etienne)
(In reply to Guilherme Gonçalves [:ggp] from comment #18)
> (In reply to Fernando Jiménez Moreno [:ferjm] from comment #16)
> > The System app already receives 'onlogout' events after bug 952063. Check
> > [1]. Maybe you can take advantage of that.
> 
> Ah, I think I see what you mean. So it should be possible to use
> shared/js/fxa_iac_client.js to subscribe to FxA events the way I want. I'll
> see how far I can get taking inspiration on :_6a68's work on the Settings
> app [1], but this looks doable, and hopefully we can land this with FxA
> already :)
> 

Note that you won't receive FxAccountsIACHelper notifications if your app is not running. What do you need to do after getting the onlogout event? If it is only changing the 'findmydevice.enabled' setting, you can do that from the System app.

> About the HAWK stuff, it would indeed be great if we could share code, but I
> agree we should leave it out of the scope of this bug. I filed bug 977552
> for that.
> 

Thanks!
I switched to Firefox Accounts and no longer require the app to be running at all times (which was nontrivial, but I think I got it to work). This was my approach:

- Listen for changes to 'findmydevice.enabled and for FxA logouts on the System app (using simply navigator.id.watch API, not the IACHelper). When 'findmydevice.enabled' becomes true, wake up Find My Device using IAC. When a logout happens, disable Find My Device (no need to wake it up, though).

- Refactor the initialization procedure for the app so that it loads all of its state, but doesn't contact the server, before the calls to mozSetMessageHandler. This ensures that, if the app is not running, it will only contact the server if needed and due to a system message waking it up. While the app is running, it also observes the 'findmydevice.registered' setting to it can re-register if necessary.

:vingtetun, please have a look at this when you get a chance.
(In reply to Guilherme Gonçalves [:ggp] from comment #21)
> I switched to Firefox Accounts and no longer require the app to be running
> at all times (which was nontrivial, but I think I got it to work).

Great!

> This was
> my approach:
> 
> - Listen for changes to 'findmydevice.enabled and for FxA logouts on the
> System app (using simply navigator.id.watch API, not the IACHelper). When
> 'findmydevice.enabled' becomes true, wake up Find My Device using IAC. When
> a logout happens, disable Find My Device (no need to wake it up, though).
> 

You don't need to use navigator.mozId.watch for this, you can just listen for 'mozFxAccountsUnsolChromeEvent' events, as in [1].

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/fxa_manager.js#L58
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #22)
> You don't need to use navigator.mozId.watch for this, you can just listen
> for 'mozFxAccountsUnsolChromeEvent' events, as in [1].

Sorry, why is this preferable? It would seem to me that, all other things being equal, using the documented and presumably more stable observer API would be better than listening for these internal events.

Of course I wouldn't mind changing it at all, I just don't see a compelling reason right now.
(In reply to Guilherme Gonçalves [:ggp] from comment #23)
> (In reply to Fernando Jiménez Moreno [:ferjm] from comment #22)
> > You don't need to use navigator.mozId.watch for this, you can just listen
> > for 'mozFxAccountsUnsolChromeEvent' events, as in [1].
> 
> Sorry, why is this preferable? It would seem to me that, all other things
> being equal, using the documented and presumably more stable observer API
> would be better than listening for these internal events.

First of all, because a single event listener is cheaper than using the whole mozId API .watch() process, specially in the System app, where the window is always alive and resources won't be released.

And in any case you don't really need all the .watch() functionality, only the 'onlogout' event, which BTW will always be fired after bug 945363 lands even if there is no change in the account status, right before firing 'onready'. So you will always be setting the 'findmydevice.enabled' value at [1] to false on every run if there is no account in the device, unless you check that the 'onready' event has not been fired at that point. I already expressed my concerns about this API at bug 945363, but I am afraid that it will not change for now.

So I think it is worth listening for 'mozFxAccountsUnsolChromeEvent' instead :).

[1] https://github.com/mozilla-b2g/gaia/pull/16325/files#diff-7e78b616bd4bff2333a5ac999fd46f6cR22
Makes sense, thanks for the information. I'll update the patch to use mozFxAccountsUnsolChromeEvent.
Created attachment 8386715 [details] [diff] [review]
findmydevice.patch
Attachment #8382262 - Attachment is obsolete: true
Attachment #8382262 - Flags: review?(21)
Attachment #8386715 - Flags: review?(21)
Comment on attachment 8382262 [details] [diff] [review]
findmydevice.patch

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

Those are the comments I got on the previous patch before you posted another one. I will look at the other patch again but those should be good to fix too.

::: apps/findmydevice/js/commands.js
@@ +21,5 @@
> +    var self = this;
> +
> +    this._ringer = new Audio();
> +    this._ringer.mozAudioChannel = 'content';
> +    this._ringer.loop = true;

var ringer = this._ringer = new Audio();
ringer.mozAudioChannel = 'content';
ringer.loop = true;

@@ +23,5 @@
> +    this._ringer = new Audio();
> +    this._ringer.mozAudioChannel = 'content';
> +    this._ringer.loop = true;
> +
> +    this._ringtoneURL = new SettingsURL();

var ringtoneURL = this._ringtoneURL = new SettingsURL();

@@ +62,5 @@
> +
> +  _setPermission: function fmdc_set_permission(permission, value) {
> +    if (!this._app) {
> +      return false;
> +    }

It's good to be careful but can it really happens ?

@@ +66,5 @@
> +    }
> +
> +    try {
> +      navigator.mozPermissionSettings.set(
> +        permission, value, this._app.manifestURL, this._app.origin, false);

var app = this._app;
navigator.mozPermissionSettings.set(permission, app.manifestURL,
  app.origin, false);

@@ +179,5 @@
> +    };
> +  },
> +
> +  ring: function fmdc_ring(args, reply) {
> +    var self = this;

s/var self = this;/var ringer = this._ringer;| and use that.

::: apps/findmydevice/js/findmydevice.js
@@ +102,5 @@
> +        self._enabled = true;
> +        self._processCommands(event.data);
> +        self.enabled = enabled;
> +      };
> +    });

Can we make this init method a bit more concise. By creating some methods on the FindMyDevice singleton for example. It will be easier to see what is is registered for and what are the actions of those registrations.

::: apps/findmydevice/js/requester.js
@@ +27,5 @@
> +    vreq.onsuccess = function fmdr_api_version_onsuccess() {
> +      version = this.result[self.API_VERSION_SETTING];
> +      self._url = baseURL + '/' + version;
> +    };
> +  },

Is there any particular reasons where you need findmydevice.api_url in the settings database ? Can't you just use the build/application-data.js mechanism ?

The reason I'm asking is because mozSettings is not really done for that. So we have violated it a few times in the past but if we can avoid it that's good.

@@ +66,5 @@
> +          {payload: xhr.response});
> +      }
> +
> +      if (!valid) {
> +        console.log('findmydevice ignoring response, HAWK failed!');

nit: remove that.

::: apps/findmydevice/manifest.webapp
@@ +1,4 @@
> +{
> +  "name": "Find My Device",
> +  "type": "certified",
> +  "role": "lostphone",

lostphone -> system

@@ +34,5 @@
> +          "app://test-findmydevice.gaiamobile.org/manifest.webapp",
> +          "http://test-findmydevice.gaiamobile.org:8080/manifest.webapp"
> +        ]
> +      }
> +    }

It scares me a little bit that we let that in production. If someone is able to make the dns lies and install an application with this domain name (and this application is not installed because it is only on testing devices), then he can wipe your phone afterward. It's OK as long IAC is certified only, but it will certainly be privileged one day. It would be nice to open a followup to fix that and insert those data dynamically in engineering build only.

::: apps/homescreen/js/grid.js
@@ +13,5 @@
>    var SAVE_STATE_TIMEOUT = 100;
>    var BASE_HEIGHT = 460; // 480 - 20 (status bar height)
>    var DEVICE_HEIGHT = window.innerHeight;
>  
> +  var HIDDEN_ROLES = ['system', 'input', 'homescreen', 'search', 'lostphone'];

no need to do that if you use the system role.

::: apps/settings/index.html
@@ +435,5 @@
> +          <li data-show-name="findmydevice.ui.enabled" hidden>
> +            <small id="findmydevice-desc" class="menu-item-desc"></small>
> +            <a id="menuItem-findmydevice" class="menu-item" href="#findmydevice"
> +              data-l10n-id="findmydevice">Find My Device</a>
> +          </li>

It's not clear to me where you are using this information.

::: apps/system/js/findmydevice_launcher.js
@@ +18,5 @@
> +  window.addEventListener('applicationready', function onAppReady() {
> +    window.removeEventListener('applicationready', onAppReady);
> +    launchFindMyDevice();
> +  });
> +}

As discussed we can probably remove this and expect the settings app to wake up the wimf app via IAC.

::: apps/system/js/lockscreen.js
@@ +379,5 @@
> +      (function(value) {
> +        if (value === true) {
> +          this.lockIfEnabled(value);
> +        }
> +    }).bind(this));

Should we consider using lockscreen.locked instead and make it lock/unlock the homescreen if it is changed ?

@@ +491,5 @@
> +  function ls_setLockMessage(val) {
> +    this.message.textContent = val;
> +    this.message.hidden = (val === '');
> +  },
> +

It stress me a little bit to add this code directly in the lockscreen implementation as lockscreen is supposed to be replaceable at some point in the future and that will break.

Can't we have a dedicated layer on top of the lockscreen ?

::: apps/system/manifest.webapp
@@ +77,5 @@
>       { "cdma-info-rec-received": "/index.html" },
>       { "nfc-manager-tech-discovered": "/index.html" },
>       { "nfc-manager-tech-lost": "/index.html" },
> +     { "nfc-manager-send-file": "/index.html" },
> +     { "push": "/index.html" }

This chance is probably not needed anymore.
Can you fix them and post a new patch that I can review again ?
Thanks for the review, :vingtetun! I've addressed all of them and will attach an updated patch shortly. Here are some of my comments and observations:

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #27)
> Comment on attachment 8382262 [details] [diff] [review]
> findmydevice.patch
> 
> Review of attachment 8382262 [details] [diff] [review]:
> -----------------------------------------------------------------
> Is there any particular reasons where you need findmydevice.api_url in the
> settings database ? Can't you just use the build/application-data.js
> mechanism ?

As discussed on IRC, I added a apps/findmydevice/build/build.js file and a corresponding Makefile to generate this configuration at build time. It seems to work, but someone more knowledgeable in the build system than me should have a close look.

> 
> ::: apps/settings/index.html
> @@ +435,5 @@
> > +          <li data-show-name="findmydevice.ui.enabled" hidden>
> > +            <small id="findmydevice-desc" class="menu-item-desc"></small>
> > +            <a id="menuItem-findmydevice" class="menu-item" href="#findmydevice"
> > +              data-l10n-id="findmydevice">Find My Device</a>
> > +          </li>
> 
> It's not clear to me where you are using this information.

What information? Do you mean the findmydevice.ui.enabled setting? That setting controls whether Find My Device appears on the Settings app menu, and if I understand it correctly, data-show-name is the standard way to hide an entry from the menu depending on the value of a setting. I copied this pattern from Firefox Accounts, anyway.

> 
> ::: apps/system/js/lockscreen.js
> @@ +379,5 @@
> > +      (function(value) {
> > +        if (value === true) {
> > +          this.lockIfEnabled(value);
> > +        }
> > +    }).bind(this));
> 
> Should we consider using lockscreen.locked instead and make it lock/unlock
> the homescreen if it is changed ?

lockscreen.locked is currently not used to control the lockscreen, and I didn't really feel comfortable changing that. The new setting, lockscreen.lock-immediately, came up when I was discussing this very issue with :etienne. It seems to me that refactoring this could be left as a followup, but if you insist we should probably needinfo? a lockscreen expert.

> 
> Can't we have a dedicated layer on top of the lockscreen ?
> 

I experimented a little with this. While it is possible to do it, making sure the message layer and lockscreen coexist peacefully can get messy. For example, it would be desirable to have the passcode and keyboard stay on top of the message as the user types the passcode to unlock the device. This seems rather hard to accomplish without somehow coupling the message and lockscreen implementation.

Unless you are very far along in making the lockscreen replaceable, I'd suggest leaving things as they are and dealing with this problem when it comes up later, once replacing the default lockscreen becomes a concrete possibility.
Created attachment 8387047 [details] [diff] [review]
findmydevice.patch
Attachment #8386715 - Attachment is obsolete: true
Attachment #8386715 - Flags: review?(21)
Attachment #8387047 - Flags: review?(21)
Created attachment 8388772 [details] [diff] [review]
findmydevice.patch

Refreshing the patch after rebasing on current Gaia. This also fixes the fact that the last patch had included config.js by accident.
Attachment #8387047 - Attachment is obsolete: true
Attachment #8387047 - Flags: review?(21)
Attachment #8388772 - Flags: review?(21)
Comment on attachment 8388772 [details] [diff] [review]
findmydevice.patch

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

I'm generally a bit worried about what's happens if the home is running out of memory. We will need to have some QA tests for that.

Otherwise this is a lot of work, thanks for your efforts. I basically reviewed the settings changes inside this big patch, so I'm going to close the bug for the settings part if thats fine for you ?

::: .jshintignore
@@ +32,5 @@
>  apps/music/js/metadata_scripts.js
>  apps/keyboard/js/imes/jszhuyin/tools/cook.js
>  apps/keyboard/build/keyboard-config.js
> +apps/findmydevice/js/lib/*
> +apps/findmydevice/js/config.js

I just realized that you have to add the config.js file to both .jshintignore and .gitignore. Can you file a followup to investigate if .jshintignore can use .gitignore as well ?

::: apps/findmydevice/build/build.js
@@ +5,5 @@
> +
> +var utils = require('utils');
> +
> +function execute(config) {
> +  var distDir = config.GAIA_DISTRIBUTION_DIR;

Move the declaration of |distDir| closer to where it is used (next to the utils.writeContent call).

@@ +14,5 @@
> +    'api_url': 'http://ec2-54-241-87-238.us-west-1.compute.amazonaws.com',
> +    'api_version': '0'
> +  };
> +
> +  utils.writeContent(init, 'FindMyDeviceConfig = ' +

Since this code is now part of the findmydevice app, there is likely no needs to prefix it with FindMyDevice. Just rename it |Config|.

::: apps/findmydevice/index.html
@@ +16,5 @@
> +    <script defer src="js/config.js"></script>
> +    <script defer src="js/requester.js"></script>
> +    <script defer src="js/commands.js"></script>
> +    <script defer src="js/findmydevice.js"></script>
> +  </head>

nit: It may be dumb but for my own sanity can you add a <body></body> after the </head> section. I know that the engine will create it for you but it break my heart to ask it to do it :)

::: apps/findmydevice/js/commands.js
@@ +3,5 @@
> +/* global DUMP */
> +
> +'use strict';
> +
> +var FindMyDeviceCommands = {

Since this code is now part of the findmydevice app, there is likely no needs to prefix it with FindMyDevice. Just rename it |Commands| and update the functions names with that.

@@ +6,5 @@
> +
> +var FindMyDeviceCommands = {
> +  _ringer: null,
> +
> +  _watchId: null,

Declare |_watchId| before the |track| method, and rename it to _watchPositionId (mostly because the first time I read the code I was wondering what it was).

@@ +8,5 @@
> +  _ringer: null,
> +
> +  _watchId: null,
> +
> +  _app: null,

You may be able to get rid of this one. See later.

@@ +15,5 @@
> +
> +  _lockscreenPassCodeEnabled: false,
> +
> +  init: function fmdc_init() {
> +    var self = this;

Declare |self| closer to where it is used. The rationale for that is usualy to not forgot some variables in the code. If someone delete the SettingsListener blocks, or move it elsewhere it is easy to forgot to remove the |self| variable.

So move it just above the SettingsListerner blocks.

@@ +46,5 @@
> +    this._app = null;
> +    var appreq = navigator.mozApps.getSelf();
> +    appreq.onsuccess = function fmdc_getapp_success() {
> +      self._app = this.result;
> +    };

Since Commands._app is only used by the _setPermission code, it may be better to keep it as a local variable inside the _setPermission method and add a callback mechanism to _setPermission that returns when the permission is set or not.

@@ +64,5 @@
> +      return false;
> +    }
> +
> +    return true;
> +  },

It makes me a bit sad that you have to explicitely set the geolocation permission to your app, with a perfectly valid use case. Can you file a followup to see if this can be configurable at build time ? It will makes me happy to not require an application other than settings to access this.

One other reasons why it seems weird to do it like this is, could be that the user can revoke the permission the geolocation permission in the Settings app (at least I think it can). I'm pretty sure that if the app is already doing some geolocation requests while the user turn the permission off, geolocation will continue to work, but that seems like a bug too.

@@ +67,5 @@
> +    return true;
> +  },
> +
> +  track: function fmdc_track(args, reply) {
> +    var duration = parseInt(args.d, 10);

Since we only use args.d, can't we just replace the |args| argument by |duration| and ensure the caller give it args.d directly ?

@@ +70,5 @@
> +  track: function fmdc_track(args, reply) {
> +    var duration = parseInt(args.d, 10);
> +
> +    // XXX we actually ignore nonzero durations for the
> +    // time being.

|time beeing| ? Can you file a followup bug and reference the bug number here instead of XXX

@@ +88,5 @@
> +      reply(false, 'mozPermissionSettings is missing');
> +      return;
> +    }
> +
> +    if (!this._setPermission('geolocation', 'allow')) {

Rename _setPermission to _setGeolocationPermission(successCallback, errorCallback)

@@ +107,5 @@
> +      }
> +    );
> +  },
> +
> +  erase: function fmdc_erase(args, reply) {

|args| does not seems used ?

@@ +120,5 @@
> +          ds.delete(file.name);
> +        }
> +
> +        if (!this.done) {
> +          this.continue();

if (!this.done) {
  this.continue();
  return;
}

And then avoid one indentation level for the following block.

@@ +126,5 @@
> +          DUMP('done wiping ' + target);
> +          if (++wiped == toWipe.length) {
> +            DUMP('all targets wiped, starting factory reset!');
> +            navigator.mozPower.factoryReset();
> +            reply(true); // testing only

Does the |reply| callback is used for testing all over the places, or is it only in the case of this particular method ? This comment makes me doubt.

@@ +139,5 @@
> +        wiped++;
> +      };
> +    }
> +
> +    for (var i = 0; i < toWipe.length; i++) {

toWipe.forEach(function wipeDirectory(name) {
  ...
}

?

@@ +148,5 @@
> +    }
> +  },
> +
> +  lock: function fmdc_lock(args, reply) {
> +    var message = args.m, passcode = args.c;

Any reasons to not pass directly message, passcode as parameters ?

@@ +167,5 @@
> +    }
> +
> +    SettingsListener.getSettingsLock().set(settings).onsuccess = function() {
> +      reply(true);
> +    };

Don't you want to reply(false) on a failure then ?

@@ +172,5 @@
> +  },
> +
> +  ring: function fmdc_ring(args, reply) {
> +    var ringer = this._ringer;
> +    var duration = parseInt(args.d, 10);

Basically same comment as the |track| method:
Since we only use args.d, can't we just replace the |args| argument by |duration| and ensure the caller give it args.d directly ?

@@ +180,5 @@
> +      ringer.currentTime = 0;
> +    }
> +
> +    // are we already ringing?
> +    if (!this._ringer.paused) {

s/this._ringer/ringer/

@@ +196,5 @@
> +      'audio.volume.content': 15
> +    }).onsuccess = function() {
> +      ringer.play();
> +      reply(true);
> +    };

Returns false on onerror ?

::: apps/findmydevice/js/findmydevice.js
@@ +6,5 @@
> +/* global DUMP */
> +
> +'use strict';
> +
> +var FindMyDevice = {

Maybe a better name as this one does not means much.

@@ +9,5 @@
> +
> +var FindMyDevice = {
> +  _state: null,
> +
> +  _enabled: null,

_enabled is likely going to be a boolean. Let's initialize it with |false|.

@@ +11,5 @@
> +  _state: null,
> +
> +  _enabled: null,
> +
> +  _registered: null,

_registered is likely going to be a boolean. Let's initialize it with |false|.

Also maybe those 2 properties would better fit declared next to the helper that will affects their values (registeredHelper and enabledHelper) to see more clearly the relationship ?

@@ +21,5 @@
> +  _alarmId: null,
> +
> +  _requester: FindMyDeviceRequester,
> +
> +  _commands: FindMyDeviceCommands,

If FindMyDeviceRequester is renamed Requester and FindMyDeviceCommands is renamed Commands, do you still need those properties ?

If you prefer to keep those 2, let's add |_config: Config,| as well to stay consistent.

@@ +29,5 @@
> +  _enabledHelper: SettingsHelper('findmydevice.enabled'),
> +
> +  init: function fmd_init() {
> +    var self = this;
> +    var settings = navigator.mozSettings;

Declare |settings| closer to where it is used (next to the settings.addObserver call)

@@ +37,5 @@
> +      audience: FindMyDeviceConfig.api_url,
> +      onready: function fmd_fxa_onready() {
> +        self._initSettings(function fmd_fxa_initialized() {
> +          self._initMessageHandlers();
> +        });

Maybe a |_onReady| method on the FindMyDevice singleton would makes it more consistent with the _onLogin part ?

@@ +39,5 @@
> +        self._initSettings(function fmd_fxa_initialized() {
> +          self._initMessageHandlers();
> +        });
> +      },
> +      onlogin: self._onLogin.bind(self),

s/self/this/g

@@ +42,5 @@
> +      },
> +      onlogin: self._onLogin.bind(self),
> +      onlogout: function fmd_fxa_onlogout() {
> +        self._enabledHelper.set(false);
> +      }

Maybe a |_onLogout| method on the FindMyDevice singleton would makes it more consistent with the _onLogin part ?

@@ +57,5 @@
> +
> +      // No need to contact the server here if we've been enabled,
> +      // since that means we'll get a wake up request from the System
> +      // app
> +    });

Any reasons to not use the SettingsListener helper for these 2 settings calls ?

@@ +64,5 @@
> +  _initSettings: function fmd_init_settings(callback) {
> +    var self = this;
> +
> +    this._registeredHelper.get(function fmd_get_registered(value) {
> +      self._onRegisteredStateChanged(value, loadEnabledSetting);

The _onRegisteredStateChanged and this method does not check if |value != this._registered|. Do we really need to do the work if it has not changed ?

@@ +72,5 @@
> +      self._enabledHelper.get(function fmd_get_enabled(value) {
> +        self._enabled = value;
> +        callback && callback();
> +      });
> +    }

If you use the SettingsListener in the init method then self._enabled will be set to the correct value and you can likely directly pass |callback| in the self._onRegisteredStateChanged above, or did I missed something ?

@@ +108,5 @@
> +      }
> +    });
> +  },
> +
> +  _onRegisteredStateChanged: function fmd_register_changed(value, callback) {

Start with:

if (!value) {
  this._registered = false;
  callback && callback();
  return;
}

var self = this;
asyncStorage.getItem('findmydevice-state', function(state) {
...
});

@@ +115,5 @@
> +      asyncStorage.getItem('findmydevice-state', function(state) {
> +        self._registered = true;
> +        self._state = state;
> +        self._requester.setHawkCredentials(
> +          self._state.deviceid, self._state.secret);

s/self._state/state/g

@@ +155,5 @@
> +    navigator.mozId.request();
> +  },
> +
> +  _onLogin: function fmd_on_login(assertion) {
> +    var self = this;

Move the |self| declaration closer to where it is used (in this method you don't need if if you return early too, so that makes even more sense).

@@ +184,5 @@
> +    };
> +  },
> +
> +  _requestRegistration: function fmd_request_registration(assertion, endpoint) {
> +    var self = this;

Move the |self| declaration closer to where it is used.

@@ +224,5 @@
> +    }
> +
> +    if (this._alarmId !== null) {
> +      this._unscheduleAlarm();
> +    }

It seems like we can ends up with multiples alarms if the app has been close after an alarm has been set up, and it is reopened afterward. You probably want to retrieve the alarm from the API first.

See https://developer.mozilla.org/en-US/docs/Web/API/MozAlarmsManager.getAll

As a result I wonder if the _alarmId property is needed anymore.

@@ +238,5 @@
> +  _unscheduleAlarm: function fmd_unschedule_alarm() {
> +    if (this._alarmId !== null) {
> +      navigator.mozAlarms.remove(this._alarmId);
> +      this._alarmId = null;
> +    }

Once you moved to MozAlarmsManager.getAll you can probably pass the id as a parameter instead of relying on a property.

@@ +242,5 @@
> +    }
> +  },
> +
> +  _replyAndFetchCommands: function fmd_reply_and_fetch() {
> +    this._reply.has_passcode = this._commands._deviceHasPasscode();

Maybe _deviceHasPasscode should not be prefixed with _ if the rationale is to access it from the outside world.

@@ +253,5 @@
> +    this._reply = {};
> +  },
> +
> +  _processCommands: function fmd_process_commands(cmdobj) {
> +    if (cmdobj === null || (!this.enabled && cmdobj.testing !== true)) {

It seems a bit strange to me that the testing will check something that works differently in production ?

@@ +279,5 @@
> +        DUMP('command ' + cmd + ', args ' + JSON.stringify(args));
> +        this._commands[commandsToMethods[cmd]](args, cb);
> +      } else {
> +        this._replyCallback(cmd, false, 'command not available');
> +      }

if (!(cmd in commandsToMethods)) {
   this._replyCallback(cmd, false, 'command not available');
  continue;
}

::: apps/findmydevice/js/lib/hawk.js
@@ +1,5 @@
> +/*
> +    HTTP Hawk Authentication Scheme
> +    Copyright (c) 2012-2013, Eran Hammer <eran@hueniverse.com>
> +    MIT Licensed
> +*/

I'm not going to reviewed this as this is a third party lib.

I'm still not sure to understand why we use a JS third party lib instead of a encrypted communication channel over TCP. Will needinfo some network folks to have their opinion as I really have no ideas about if this is a good or a bad thing.

::: apps/findmydevice/js/requester.js
@@ +3,5 @@
> +/* exported FindMyDeviceRequester */
> +
> +'use strict';
> +
> +var FindMyDeviceRequester = {

Since this code is now part of the findmydevice app, there is likely no needs to prefix it with FindMyDevice. Just rename it |Requester| and update the functions names with that.

@@ +53,5 @@
> +      if (!valid) {
> +        return;
> +      }
> +
> +      if (xhr.status == 200 && onsuccess) {

I thought that onload means more or less 200 already ? (while it means 0 for local files).

@@ +63,5 @@
> +
> +    xhr.onerror = function fmd_xhr_onerror() {
> +      if (onerror) {
> +        onerror(xhr);
> +      }

For what it worth, some people are lazy and for such things they just do |onerror && onerror(xhr);|. I'm not saying this is better, this is just to share :)

::: apps/findmydevice/manifest.webapp
@@ +44,5 @@
> +          "app://test-findmydevice.gaiamobile.org/manifest.webapp",
> +          "http://test-findmydevice.gaiamobile.org:8080/manifest.webapp"
> +        ]
> +      }
> +    }

As I mentioned, beeing a bit paranoiac, makes me feels like this a bit unsecure. Would be nice to make sure this rule is not shipped in production in a followup.

For example the manifest.webapp file can be cleared during the copy from the Makefile.

::: apps/findmydevice/test/unit/findmydevice_test.js
@@ +28,5 @@
> +  mocksForFindMyDevice.attachTestHelpers();
> +
> +  var subject;
> +  setup(function(done) {
> +    realL10n = navigator.mozL10n;

It's sad that you have to do that. mozL10n is an API exposed by shared/js/l10n.js. You should not have to do that but that's purely a bug on our test infrastructure side.

@@ +46,5 @@
> +        this.factoryResetCalled = true;
> +      }
> +    };
> +
> +    window.DUMP = function() {};

Add a comment explaining that it is a replacement for the shared/js/dump.js library.

@@ +82,5 @@
> +    MockSettingsListener.mCallbacks['dialer.ringtone'](ringtone);
> +
> +    subject.ring({d: duration}, function(retval) {
> +      var lock = MockSettingsListener.getSettingsLock().locks.pop();
> +

var ringer = subject.ringer; and use that.

@@ +92,5 @@
> +
> +      setTimeout(function() {
> +        assert.equal(subject._ringer.paused, true, 'must have stopped');
> +        done();
> +      }, duration * 1000);

setTimeout inside tests is known to fail randomly. Can you use a fake clock ? Look at the usage of useFakeTimers and clock in apps/system/test/unit/*

@@ +101,5 @@
> +    subject.erase({}, function(retval, error) {
> +      for (var i = 0; i < MockDeviceStorage.instances.length; i++) {
> +        // check that we deleted everything on the device storage
> +        assert.deepEqual(MockDeviceStorage.instances[i].entries, []);
> +      }

Create a local |instances| variable and use that.

@@ +147,5 @@
> +    navigator.mozPower = realMozPower;
> +
> +    delete window.DUMP;
> +  });
> +});

It would be nice to have more tests coverage for the FindMyDevice blocks. Some specific test files for the FindMyDeviceCommands bits, FindMyDeviceRequester bits too would be appreciated.

Thanks for all the Mocks thought :)

::: apps/settings/elements/findmydevice.html
@@ +9,5 @@
> +    <div>
> +      <div id="findmydevice-signin">
> +        <ul>
> +          <li class="description" data-l10n-id="findmydevice-signup-description">
> +          FindMyDevice allows you to locate, lock or erase your phone.

You need to add the data-l10n-ids (for all this file) to apps/settings/locales/settings.en-US.properties

::: apps/settings/js/findmydevice.js
@@ +2,5 @@
> +
> +'use strict';
> +
> +var FindMyDevice = {
> +  // When the FxA login callback is called, we need know if the

s/need know/need to know/ ?

@@ +12,5 @@
> +    var self = this;
> +
> +    var lock = SettingsListener.getSettingsLock();
> +    lock.get('findmydevice.api_url').onsuccess = function() {
> +      var api = this.result['findmydevice.api_url'];

You probably want to use the same mechanism than apps/findmydevice/build/build.j for that.

::: apps/system/js/findmydevice_launcher.js
@@ +22,5 @@
> +  }
> +
> +  if (event.detail.eventName === 'onlogout') {
> +    var helper = SettingsHelper('findmydevice.enabled');
> +    helper.set(false);

SettingsHelper('findmydevice.enabled').set(false);

::: apps/system/js/lockscreen.js
@@ +487,5 @@
>      }
>    };
>  
> +  LockScreen.prototype.setLockMessage =
> +  function ls_setLockMessage(val) {

I wished it fits on one line :(

::: apps/system/manifest.webapp
@@ +112,5 @@
> +        "manifestURLs": [
> +          "app://test-findmydevice.gaiamobile.org/manifest.webapp",
> +          "http://test-findmydevice.gaiamobile.org:8080/manifest.webapp"
> +        ]
> +      }

Is this still needed ?

::: build/config/common-settings.json
@@ +46,5 @@
>     "devtools.overlay": false,
>     "devtools.eventlooplag.threshold": 100,
>     "feedback.url": "https://input.allizom.org/api/v1/feedback/",
> +   "findmydevice.api_url": "http://ec2-54-241-87-238.us-west-1.compute.amazonaws.com",
> +   "findmydevice.api_version": "0",

Let's get rid of the first 2 settings.

::: test_apps/test-findmydevice/index.html
@@ +3,5 @@
> +
> +  <head>
> +    <meta name="viewport" content="width=device-width, user-scalable=no, initial-scale=1">
> +    <meta charset="utf-8">
> +    <title>Template</title>

s/Template/Find My Device Test App/g
Attachment #8388772 - Flags: review?(21)
Attachment #8388772 - Flags: review-
Attachment #8388772 - Flags: feedback+
Actually the Necko team is in Paris so I will discuss with them tonight to understand Hawk versus encryted communication over TCP.
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #33)
> Actually the Necko team is in Paris so I will discuss with them tonight to
> understand Hawk versus encryted communication over TCP.

You may also want to needinfo? jrconlin if you still have questions about HAWK after talking to the Necko team. I believe he was the one who initially proposed using it. As I understand it, HAWK is supposed to be used in addition to TLS when talking to the server, providing an extra MAC of the request contents.
Thank you so much for the very thorough review!

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #32)
> I'm going to close the bug for the settings part if thats fine for you ?

Absolutely, feel free to do that.

> Can you file a followup to investigate if .jshintignore can use .gitignore as well ?

File bug 982607.

> Can you file a followup to see if this can be configurable at build time ?

I'm not totally sure how this would work, but then again I don't know much about permissions.

As far as I know, permissions can be changed at run time by the user, right? My intention with this part of the code was to make sure the geolocation permission was set to 'allow' before we started tracking, so I don't quite see how a build time solution would help.

I do agree with your reasoning in general though, so a better solution would be very welcome :)

> 
> One other reasons why it seems weird to do it like this is, could be that
> the user can revoke the permission the geolocation permission in the
> Settings app (at least I think it can). I'm pretty sure that if the app is
> already doing some geolocation requests while the user turn the permission
> off, geolocation will continue to work, but that seems like a bug too.

Filed bug 982611 to investigate this later.

> 
> @@ +70,5 @@
> > +  track: function fmdc_track(args, reply) {
> > +    var duration = parseInt(args.d, 10);
> > +
> > +    // XXX we actually ignore nonzero durations for the
> > +    // time being.
> 
> |time beeing| ? Can you file a followup bug and reference the bug number
> here instead of XXX
> 

Sorry, that was a misleading comment. We actually don't plan on changing this behavior in the
foreseeable future (and I just noticed the server documentation on the wiki is outdated in that
regard). I rephrased the comment.
 
> Does the |reply| callback is used for testing all over the places, or is it
> only in the case of this particular method ? This comment makes me doubt.

Updated the comment to make it clearer, but for the record, this specific reply() call is the only one used for testing as that's the only circumstance in which we expect factoryReset() to return.

> 
> @@ +39,5 @@
> > +        self._initSettings(function fmd_fxa_initialized() {
> > +          self._initMessageHandlers();
> > +        });
> > +      },
> > +      onlogin: self._onLogin.bind(self),
> 
> s/self/this/g

I don't think that's possible, as |this| refers to the navigator.mozId.watch() options object in this context.


> Any reasons to not use the SettingsListener helper for these 2 settings
> calls ?

(sorry, I don't think I have a short answer for this)

We don't expect 'findmydevice.registered' to change at all while we're not running, but if we are running and it turns false, this means there was an error while contacting the server, so we need to attempt to re-register. On the other hand, when the app is being initialized due to a system message, we do not want to contact the server even if 'findmydevice.registered' is false, as the system message handler will do that for us.

So my reasoning was that it would be useful to separate what happens when we're first initializing from what happens when 'findmydevice.registered' changes at run time, as in the latter case we want to also contact the server. Breaking the SettingsListener call into a get() for initialization and an observe() for other changes seemed like the cleanest way to do it.

The above argument doesn't apply for 'findmydevice.enabled', as for this setting the logic is the same both cases (we simply update _enabled). However, when the app is being woken up by a system message, we want to make sure _state, _registered and _enabled are fully initialized before attaching handlers to system messages, and once again breaking the SettingsListener call made it easier to reason about the order in which things get initialized: you'll notice that in the current code we always initialize Firefox Accounts, then _registered, then _state, then _enabled, and only then do we process system messages.

All of this is of course towards the end goal of ensuring the whole app is initialized when a system message handler gets to run, regardless of whether the app had been previously running or was just woken up.

> @@ +253,5 @@
> > +    this._reply = {};
> > +  },
> > +
> > +  _processCommands: function fmd_process_commands(cmdobj) {
> > +    if (cmdobj === null || (!this.enabled && cmdobj.testing !== true)) {
> 
> It seems a bit strange to me that the testing will check something that
> works differently in production ?
> 

This is just so we can test commands without first going through the registering/enabling path. I added a clarifying comment for now, but of course I'm open to changing this if you disagree.

> @@ +53,5 @@
> > +      if (!valid) {
> > +        return;
> > +      }
> > +
> > +      if (xhr.status == 200 && onsuccess) {
> 
> I thought that onload means more or less 200 already ? (while it means 0 for
> local files).
> 

Not quite. onload still gets triggered for other status codes such as 401, which is the one we're specifically worried about.

> 
> ::: apps/findmydevice/manifest.webapp
> @@ +44,5 @@
> > +          "app://test-findmydevice.gaiamobile.org/manifest.webapp",
> > +          "http://test-findmydevice.gaiamobile.org:8080/manifest.webapp"
> > +        ]
> > +      }
> > +    }
> 
> As I mentioned, beeing a bit paranoiac, makes me feels like this a bit
> unsecure. Would be nice to make sure this rule is not shipped in production
> in a followup.

Definitely. I filed bug 983363 to look into this, thanks for pointing it out.

> It would be nice to have more tests coverage for the FindMyDevice blocks.
> Some specific test files for the FindMyDeviceCommands bits,
> FindMyDeviceRequester bits too would be appreciated.

I agree it could be a good idea to add tests that exercise the requester. I can look into adding them in a followup if you can live without them for now.

> ::: apps/system/manifest.webapp
> @@ +112,5 @@
> > +        "manifestURLs": [
> > +          "app://test-findmydevice.gaiamobile.org/manifest.webapp",
> > +          "http://test-findmydevice.gaiamobile.org:8080/manifest.webapp"
> > +        ]
> > +      }
> 
> Is this still needed ?
> 

It's not, thanks for catching this, I completely overlooked it!

I'll attach an updated patch and an interdiff in a moment.
Created attachment 8390846 [details] [diff] [review]
findmydevice.patch
Attachment #8388772 - Attachment is obsolete: true
Attachment #8390846 - Flags: review?(21)
(In reply to Guilherme Gonçalves [:ggp] from comment #35)
 > (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #32)

> > Can you file a followup to see if this can be configurable at build time ?
> 
> I'm not totally sure how this would work, but then again I don't know much
> about permissions.
> 
> As far as I know, permissions can be changed at run time by the user, right?
> My intention with this part of the code was to make sure the geolocation
> permission was set to 'allow' before we started tracking, so I don't quite
> see how a build time solution would help.
> 
> I do agree with your reasoning in general though, so a better solution would
> be very welcome :)

Basically if there is a build time solution, we can generate a config file that could be used by the settings app to *not* show some permissions for some app. Then at build time we can give the permission to this app once and that's it. It seems to me that this app really wants to have the permission by default and to never be changed.
  
> 
> > Any reasons to not use the SettingsListener helper for these 2 settings
> > calls ?
> 
> (sorry, I don't think I have a short answer for this)
> 
> We don't expect 'findmydevice.registered' to change at all while we're not
> running, but if we are running and it turns false, this means there was an
> error while contacting the server, so we need to attempt to re-register. On
> the other hand, when the app is being initialized due to a system message,
> we do not want to contact the server even if 'findmydevice.registered' is
> false, as the system message handler will do that for us.
> 
> So my reasoning was that it would be useful to separate what happens when
> we're first initializing from what happens when 'findmydevice.registered'
> changes at run time, as in the latter case we want to also contact the
> server. Breaking the SettingsListener call into a get() for initialization
> and an observe() for other changes seemed like the cleanest way to do it.
> 
> The above argument doesn't apply for 'findmydevice.enabled', as for this
> setting the logic is the same both cases (we simply update _enabled).
> However, when the app is being woken up by a system message, we want to make
> sure _state, _registered and _enabled are fully initialized before attaching
> handlers to system messages, and once again breaking the SettingsListener
> call made it easier to reason about the order in which things get
> initialized: you'll notice that in the current code we always initialize
> Firefox Accounts, then _registered, then _state, then _enabled, and only
> then do we process system messages.
> 
> All of this is of course towards the end goal of ensuring the whole app is
> initialized when a system message handler gets to run, regardless of whether
> the app had been previously running or was just woken up.

Sounds OK then. Thanks for the explanations.

> 
> > @@ +253,5 @@
> > > +    this._reply = {};
> > > +  },
> > > +
> > > +  _processCommands: function fmd_process_commands(cmdobj) {
> > > +    if (cmdobj === null || (!this.enabled && cmdobj.testing !== true)) {
> > 
> > It seems a bit strange to me that the testing will check something that
> > works differently in production ?
> > 
> 
> This is just so we can test commands without first going through the
> registering/enabling path. I added a clarifying comment for now, but of
> course I'm open to changing this if you disagree.
>

I think I disagree but I not going to block the landing for that. Feel free to open a followup.
 
> > @@ +53,5 @@
> > > +      if (!valid) {
> > > +        return;
> > > +      }
> > > +
> > > +      if (xhr.status == 200 && onsuccess) {
> > 
> > I thought that onload means more or less 200 already ? (while it means 0 for
> > local files).
> > 
> 
> Not quite. onload still gets triggered for other status codes such as 401,
> which is the one we're specifically worried about.
> 

Oh. I like to learn new things :)

> > It would be nice to have more tests coverage for the FindMyDevice blocks.
> > Some specific test files for the FindMyDeviceCommands bits,
> > FindMyDeviceRequester bits too would be appreciated.
> 
> I agree it could be a good idea to add tests that exercise the requester. I
> can look into adding them in a followup if you can live without them for now.
>

Let's open a followup.
Comment on attachment 8390847 [details] [diff] [review]
interdiff.patch

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

Thanks for the interdiff. Looks good to me now :)

::: apps/findmydevice/js/commands.js
@@ +65,3 @@
>  
> +    appreq.onerror = errorCallback;
> +    return;

nit: the return is probably useless here.
Attachment #8390847 - Flags: review+
Comment on attachment 8390846 [details] [diff] [review]
findmydevice.patch

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

Just to add the stamp on the real patch.
Attachment #8390846 - Flags: review?(21) → review+
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #39)
> It seems to me that this app really wants to have the permission
> by default and to never be changed.

Filed bug 984849.

> > > @@ +253,5 @@
> > > > +    this._reply = {};
> > > > +  },
> > > > +
> > > > +  _processCommands: function fmd_process_commands(cmdobj) {
> > > > +    if (cmdobj === null || (!this.enabled && cmdobj.testing !== true)) {
> > > 
> > > It seems a bit strange to me that the testing will check something that
> > > works differently in production ?
> > > 
> > 
> > This is just so we can test commands without first going through the
> > registering/enabling path. I added a clarifying comment for now, but of
> > course I'm open to changing this if you disagree.
> >
> 
> I think I disagree but I not going to block the landing for that. Feel free
> to open a followup.

Filed bug 984853.

> > > It would be nice to have more tests coverage for the FindMyDevice blocks.
> > > Some specific test files for the FindMyDeviceCommands bits,
> > > FindMyDeviceRequester bits too would be appreciated.
> > 
> > I agree it could be a good idea to add tests that exercise the requester. I
> > can look into adding them in a followup if you can live without them for now.
> >
> 
> Let's open a followup.

Filed bug 984855.
Created attachment 8392873 [details] [diff] [review]
findmydevice.patch r=vingtetun

Refreshing the patch after fixing a couple of typos and rebasing on top of gaia master. The tests are green as well: https://github.com/mozilla-b2g/gaia/pull/16325

There is, however, one nontrivial change I had to make: my handling of 'findmydevice.registered' was actually wrong. Consider this scenario, for example:

1) We lose our push endpoint and get a 'push-register' system message, which means our current registration state is invalid;
2) We set 'findmydevice.registered' to false and attempt to re-register with the server, but that fails (perhaps we're offline), so we set an alarm to retry in, say, 4 minutes;
3) The app gets killed;
4) When the alarm causes the app to launch again, we see that 'findmydevice.registered' is false, which means we need to re-register like we wanted to. However, we still want to load the (old) state despite that, so we can re-use the deviceid when talking to the server.

So it seems to me that the desired behavior here is:

- always load _state on initialization, regardless of the value of _registered, since we'll need any old state when re-registering;
- while the app is running, reload _state if _registered turns true, since that means we successfully (re-)registered;
- as before, if _registered turns false while the app is running, we want to start a re-registration reusing the deviceid from the current _state.

:vingtetun, I don't really think this is a major change, so I'm carrying over the r+, but please feel free to review again if you don't agree. I can attach an interdiff of this change as well, if needed.

Also, how should we proceed in landing this? Should I r? anyone else, or maybe ping someone to merge the pull request?
Attachment #8390846 - Attachment is obsolete: true
Attachment #8392873 - Flags: review+
needinfo?-ing just to make sure this gets looked at now that we're past reviews :)
Flags: needinfo?(21)
(In reply to Guilherme Gonçalves [:ggp] from comment #44)
> needinfo?-ing just to make sure this gets looked at now that we're past
> reviews :)

Oh! Do you mean you need someone to land the patches ? (please needinfo me again if you need that).
Flags: needinfo?(21)
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #45)
> (In reply to Guilherme Gonçalves [:ggp] from comment #44)
> > needinfo?-ing just to make sure this gets looked at now that we're past
> > reviews :)
> 
> Oh! Do you mean you need someone to land the patches ? (please needinfo me
> again if you need that).

Yep. The needinfo? was about the small change in comment 43, but mostly because I don't really know how to get this landed. Can you please do that? :)
Flags: needinfo?(21)
(In reply to Guilherme Gonçalves [:ggp] from comment #46)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #45)
> > (In reply to Guilherme Gonçalves [:ggp] from comment #44)
> > > needinfo?-ing just to make sure this gets looked at now that we're past
> > > reviews :)
> > 
> > Oh! Do you mean you need someone to land the patches ? (please needinfo me
> > again if you need that).
> 
> Yep. The needinfo? was about the small change in comment 43, but mostly
> because I don't really know how to get this landed. Can you please do that?
> :)

Please submit a PR against Gaia master and ping me when Travis is green. Then I will just have to push the 'Merge' button!
Flags: needinfo?(21)
:flod brought up the issue of getting a copy review for the strings in the Settings app.

The strings we currently have were copied from the wireframes attached to bug 938896. As I understand it, these strings are supposed to be temporary, and will be changed when we settle on the final UX for this.

As per :flod's suggestion, I'm adding a needinfo? to fxosux to figure this out, and cc-ing :matej, as he might be able to provide the review if necessary before landing.
Flags: needinfo?(firefoxos-ux-bugzilla)
Stephany is currently undertaking a copy audit of all strings in FxOS. I've set a needinfo? for her here as well. Thanks.
Flags: needinfo?(swilkes)
l10n team: GGP can land, right? We shouldn't be changing the meaning of strings/copy at this point. Scope is super clear for 1.5 for this feature and if we do, it would be very minor. We're hoping to land this feature asap...Thank you.
Flags: needinfo?(community)
Better to needinfo a real person than an alias :-)

Just to give some background: I asked if this patch already had copy review because I noticed a typo and the button with a strange case.
https://github.com/mozilla-b2g/gaia/pull/16325#discussion_r10987241

You're absolutely free to land this feature, just be aware that significant changes to strings will require new IDs, that's all I'm asking. And it's not just about meaning, it's about why you're changing a string and if you want localizers to be aware of it.
Flags: needinfo?(community)
Per bug 938896 comment 10, this was reverted. Also, please make sure your commit messages point to the correct bug number when landing so we don't need to dig through a bunch of different bugs and comments to figure out the right place to comment.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 54

5 years ago
Erin, no worries - the copy audit is just starting now, so we won't be changing copy right now, but later.
Flags: needinfo?(firefoxos-ux-bugzilla)
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Duplicate of this bug: 938905
Duplicate of this bug: 938898
Removing flag as bug has been fixed.
Flags: needinfo?(jsavory)
Since the bug was fixed.
clear needinfo.
Flags: needinfo?(pchen)

Updated

4 years ago
Flags: needinfo?(swilkes)
You need to log in before you can comment on or make changes to this bug.