Status

defect
RESOLVED DUPLICATE of bug 938901
6 years ago
5 years ago

People

(Reporter: nsm, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa+])

Attachments

(2 attachments)

Whiteboard: [qa+]
Bug 920455 is related, but should not be considered a blocker.
Depends on: 920455
The icon in the Settings app is, of course, not final (see bug 947985).
Attachment #8376392 - Flags: review?(21)
Depends on: 947985
Comment on attachment 8376392 [details] [review]
Pull request 16325 on mozilla-b2g/gaia

It's a bit painful to have to found in the PR what is relevant to the Settings app. Can you produce a patch with the changes I should reviewed and attach it to this bug ?
Attachment #8376392 - Flags: review?(21)
Definitely. I'm attaching the patch, but for the record, it corresponds to a
single commit: https://github.com/guilherme-pg/gaia/commit/171255d
Attachment #8376473 - Flags: review?(21)
Comment on attachment 8376473 [details] [diff] [review]
Integrate Find My Device into the Settings app.

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

I just started the review. I will be off next week but I would spend some time to understand the flow between the system app and settings.

::: apps/settings/js/findmydevice.js
@@ +2,5 @@
> +
> +'use strict';
> +
> +var FindMyDevice = {
> +  _personaIframe: null,

Any particular reasons why _personaIframe is a property of the object and not a variable in init, like the others ?

@@ +7,5 @@
> +
> +  init: function fmd_init() {
> +    var self = this;
> +    var _ = navigator.mozL10n.get;
> +    console.log('initializing findmydevice');

Remove this and all the others console.log stuffs.

@@ +17,5 @@
> +      desc.textContent = value ? _('enabled') : _('disabled');
> +
> +      var chbox = document.querySelector('#findmydevice-enabled input');
> +      chbox.checked = value;
> +      chbox.disabled = false;

nit: chbox -> checkbox

@@ +49,5 @@
> +
> +      SettingsListener.getSettingsLock().set({
> +        'findmydevice.enabled': true
> +      });
> +    }, false);

nit: remove |false|. This is the default.

@@ +60,5 @@
> +
> +      self._personaIframe = document.createElement('iframe');
> +      self._personaIframe.src = api + '/static/persona_iframe.html';
> +      console.log(self._personaIframe.src);
> +      document.body.appendChild(self._personaIframe);

I know that it is supposed to be temporary but this is really scary. You will basically inject remote content into a secure iframe. This iframe will also potentially never be removed if the there is no connection, or if there is a capturing proxy.

For my own education, how is it supposed to work with FxAccount ? (sorry my knowledge close to empty for FxAccount at the moment).

@@ +69,5 @@
> +      SettingsListener.getSettingsLock().set({
> +        'findmydevice.enabled': this.checked
> +      }).onerror = function() {
> +        chbox.disabled = false;
> +      };

nit: chbox -> checkbox

@@ +76,5 @@
> +    };
> +  }
> +};
> +
> +navigator.mozL10n.ready(FindMyDevice.init.bind(FindMyDevice));

It seems a bit odd that FindMyDevice is basically a big object that contains only an init() method. Would it be possible to move some of the callbacks into their own method of FindMyDevice ? That may help the readability of the code too.
> Any particular reasons why _personaIframe is a property of the object and
> not a variable in init, like the others ?

Not at the moment, but if you don't mind I'll keep it as a property when turning some callbacks into methods like you requested, so different methods can easily create and destroy the iframe.

> For my own education, how is it supposed to work with FxAccount ? (sorry my
> knowledge close to empty for FxAccount at the moment).

I certainly agree that the iframe solution is bad, not only for the reasons you mentioned, but also for causing the Persona dialog to load very slowly, or sometimes not at all. Firefox Accounts introduces two changes that will get us rid of the iframe, and possibly simplify the changes to the Settings app.

First, as soon as bug 947374 lands, certified apps will be able to specify an audience in the call to navigator.id.request(), which basically means we will no longer need to use an iframe at all (or, to be precise, I believe Firefox Accounts will handle the iframe for us). All we'll have to do is call navigator.id.request({audience:'findmydevice.mozilla.com'}), or whatever the domain ends up being for this feature.

Second, Persona currently only allows calls to navigator.id.request() from inside an event handler for user generated action (like a button click). That's why we currently have to log in from the Settings App just so we put the assertion in the 'findmydevice.assertion' setting, from where it can be read by the System app, which does all the rest. This restriction will be lifted with Firefox Accounts, so it should be possible to call navigator.id.request() from the System app and get rid of the setting, though I must admit I haven't considered my plan for that in too much detail yet.

Thanks for the comments, I'll have all the others fixed by the next iteration of the patch!
Comment on attachment 8376473 [details] [diff] [review]
Integrate Find My Device into the Settings app.

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

Since I basically reviewed this part into bug 938901. I don't think I need to review it again here.
Attachment #8376473 - Flags: review?(21)
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 938896
I'm not sure what your landing strategy is (I'd recommend a single squashed commit if possible), but this has been backed out due to the reasons outlined here: https://bugzilla.mozilla.org/show_bug.cgi?id=938896#c5
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
This was fixed as part of bug 938901, and it actually stuck after relanding quite some time ago.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 938901
You need to log in before you can comment on or make changes to this bug.