Remove the unnecessary l10n code from FMD



Firefox OS
3 years ago
3 years ago


(Reporter: gandalf, Assigned: gandalf)


Firefox Tracking Flags

(Not tracked)



(1 attachment)



3 years ago
It seems that findmydevice/index.html doesn't need l10n.js. That would save us from having to analyze it at build time.

Comment 1

3 years ago
Here's a branch with the code:

Unfortunately it breaks test/units/findmydevice_test.js in the way that I cannot fix because I cannot get those tests to run locally even without any changes :(

I could use your help guys!
Flags: needinfo?(lissyx+mozillians)
Flags: needinfo?(ggoncalves)
If you don't give us anything linked to a failure, I don't see how we can help. How urgent is this ? Can this wait a couple of days (still PTO for now) ?
Flags: needinfo?(lissyx+mozillians)
Flags: needinfo?(ggoncalves)
Flags: needinfo?(gandalf)

Comment 3

3 years ago
(In reply to Alexandre LISSY :gerard-majax from comment #2)
> How urgent is this ? Can this wait a couple of days (still PTO for now) ?

Not urgent. :) Feel free to ignore it until you're back :)

I'll provide the failure data when I rerun it.
I've opened a PR to check for gaia-try errors, and I'm triggering tests locally to see.
Triggering findmydevice_test.js, I see "line 1000: ReferenceError: Config is not defined"
Ok, we depend on config.js
Damn that's trivial: we used to avoid initializing the FMD object, with the part of code you removed:

> @@ -33,14 +31,6 @@ suite('FindMyDevice >', function() {
>    mocksForFindMyDevice.attachTestHelpers();
>    suiteSetup(function(done) {
> -    realL10n = navigator.mozL10n;
> -    navigator.mozL10n = window.MockL10n;
> -    sinon.stub(navigator.mozL10n, 'once', function(callback) {
> -      // we don't need to actually initialize FMD
> -      // for these unit tests, and it saves us from
> -      // mocking many objects
> -    });
> -
>      realMozId = navigator.mozId;
>      // attempting to stub only the request method of mozId,
>      // as in |sinon.stub(navigator.mozId, 'request', ...)|,
I've fixed the FMD code that made the bad breakage, now we just have two tests failing:

> ✓ [findmydevice-test/unit/findmydevice_test.js] ensure retry counter is reset on enable
> ✓ [findmydevice-test/unit/findmydevice_test.js] retryCount is not incremented on error if registered
> ✓ [findmydevice-test/unit/findmydevice_test.js] retryCount is incremented on error when not registered
> ✓ [findmydevice-test/unit/findmydevice_test.js] fields from coordinates are included in server response
> ✓ [findmydevice-test/unit/findmydevice_test.js] error message is included in the server response
> 1) [findmydevice-test/unit/findmydevice_test.js] request client id when invalidated
> ✓ [findmydevice-test/unit/findmydevice_test.js] refresh authentication when attempting to disable
> ✓ [findmydevice-test/unit/findmydevice_test.js] setting an alarm releases a wakelock
> ✓ [findmydevice-test/unit/findmydevice_test.js] contact the server on alarm
> ✓ [findmydevice-test/unit/findmydevice_test.js] report to the server when disabled
> ✓ [findmydevice-test/unit/findmydevice_test.js] track and ring are cancelled on LOCKSCREEN_CLOSED, passcode set
> ✓ [findmydevice-test/unit/findmydevice_test.js] track and ring continue on LOCKSCREEN_CLOSED, no passcode set
> 2) [findmydevice-test/unit/findmydevice_test.js] wakelocks are released on unregistered clientID change
> ✓ [findmydevice-test/unit/findmydevice_test.js] wakelocks are released when registering while already registering
>   [findmydevice-test/unit/findmydevice_test.js] findmydevice.current-clientid behavior
> ✓ [findmydevice-test/unit/findmydevice_test.js] invalidate client id when logged in
> ✓ [findmydevice-test/unit/findmydevice_test.js] invalidate client id when logged out
>   [findmydevice-test/unit/findmydevice_test.js] findmydevice.can-disable behavior
> ✓ [findmydevice-test/unit/findmydevice_test.js] set findmydevice.can-disable to false when logged out
> ✓ [findmydevice-test/unit/findmydevice_test.js] don't set findmydevice.can-disable on logout if not registered
> ✓ [findmydevice-test/unit/findmydevice_test.js] allow disabling when clientid matches the state
> ✓ [findmydevice-test/unit/findmydevice_test.js] disallow disabling when clientid doesn't match the state
>   [findmydevice-test/unit/findmydevice_test.js] set alarm on server interaction
> ✓ [findmydevice-test/unit/findmydevice_test.js] alarm is set on successful server response
> ✓ [findmydevice-test/unit/findmydevice_test.js] alarm is set on server response even when disabled
Repushed on my branch, and it's locally green.

Comment 10

3 years ago
(In reply to Alexandre LISSY :gerard-majax from comment #7)
> Damn that's trivial: we used to avoid initializing the FMD object, with the
> part of code you removed:

Yeah! I was trying to get around that with stupid flags... Your solution looks so clean :)

Thanks Alexandre!
Flags: needinfo?(gandalf)

Comment 11

3 years ago
What's the next step here?
Flags: needinfo?(lissyx+mozillians)
(In reply to Zibi Braniecki [:gandalf] from comment #11)
> What's the next step here?

I don't know, you picking up my fixes and landing this ? Please make it fast, I need to change code right here for bug 1103560.
Flags: needinfo?(lissyx+mozillians) → needinfo?(gandalf)
Forget what I said, my changes will be on apps/system/js/findmydevice_launcher.js


3 years ago
Assignee: nobody → gandalf
Flags: needinfo?(gandalf)

Comment 14

3 years ago
Created attachment 8555377 [details] [review]
pull request
Attachment #8555377 - Flags: review?(lissyx+mozillians)
Comment on attachment 8555377 [details] [review]
pull request

I should not be reviewing this, since I did the most dangerous fixes there :)
Attachment #8555377 - Flags: review?(lissyx+mozillians) → review?(guilherme.p.gonc+bmo)
Comment on attachment 8555377 [details] [review]
pull request

Thank you!
Attachment #8555377 - Flags: review?(guilherme.p.gonc+bmo) → review+

Comment 17

3 years ago
Last Resolved: 3 years ago
Resolution: --- → FIXED

Comment 18

3 years ago
Thanks for help Alexandre!
You need to log in before you can comment on or make changes to this bug.