938896 - [meta] FxOS client for Find my Device

Status: RESOLVED FIXED

(Firefox OS Graveyard :: FindMyDevice, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Tracker for Settings + Lock screen + custom client code.

Comment 1

[Erin Lancaster [:elan]]

Attachment: WMF Wireframes

WMF wireframes.

Comment 3

[Kevin Grandon :kgrandon]

Why were there so many unsquashed in the pull request that landed to master? The following unique commits were backed out: 938896, 938905, 938901, 938898.

Comment 4

[Kevin Grandon :kgrandon]

unique bug numbers i mean*

Comment 5

[Kevin Grandon :kgrandon]

Sorry to bring bad news, but this was backed out. This did not cause any specific regression, but it depended on bug 897352 which has been backed out. You can either fix the patch to not depend on that bug and land first, or coordinate with them on landing order.

https://github.com/mozilla-b2g/gaia/commit/8ad27d2f7d8b26d5d7551f232ce4a3650bcc1503

Comment 6

[Kevin Grandon :kgrandon]

Please squash these before landing again. If we do need to back it out again it makes it a lot easier. Thanks!

Comment 7

[Guilherme Gonçalves [:ggp]]

Attachment: findmydevice client r=vingtetun at bug 938901

Attaching the green pull request, rebased so it no longer depends on bug 897352, and squashed into a single commit referencing this bug as per :kgrandon's request.

The actual review happened in bug 938901, but I thought I'd attach the pull request here since this is the actual bug in the commit message. Apologies if this is confusing.

Comment 8

[Francesco Lodolo [:flod]]

Matej, how are we going to treat "Find My Device"? Is that supposed to be localized or kept in English like a trademark?

Comment 9

[Francesco Lodolo [:flod]]

Also another question: is the app's manifest supposed to be localized? 
https://github.com/mozilla-b2g/gaia/blob/master/apps/findmydevice/manifest.webapp

Because right now it's missing the expected structure. For example
https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/manifest.webapp#L42

Comment 10

[Carsten Book [:Tomcat]]

sorry had to revert this change for breaking tests like https://tbpl.mozilla.org/php/getParsedLog.php?id=37066302&tree=B2g-Inbound

Comment 11

[Kevin Grandon :kgrandon]

Hmm, this is strange, how did it happen?

It looks like the failing unit tests may have been created when this patch was landed today: https://github.com/mozilla-b2g/gaia/commit/c555cb7b8847f72e7c9f813812e53a2606ae98cf

Something is interacting strangely with MockLock. Oddly enough, I don't see that the test in question is actually including mock_lock_screen. My first guess is that the changes to mock_settings_listener.js in this pull request collided with the changes in bug 971554 to create a failing test =(

We should absolutely not be specifying a MockLock object inside of mock_settings_listener.js so I don't know how that ever landed.

Comment 12

[Guilherme Gonçalves [:ggp]]

(In reply to Kevin Grandon :kgrandon from comment #11) 
> My first guess is that the changes to mock_settings_listener.js in this pull request
> collided with the changes in bug 971554 to create a failing test =(

This does seem to be the case.

I had changed MockLock.set to return a new object that mocks a DOMRequest, but the test expected that DOMRequest to be accessible as MockLock.mCallbacks. At the time I made the change, no tests relied on that behavior, and the failing test was probably included between me getting a green test run and landing :(

I just changed my pull request so that MockLock.set now looks like this: https://github.com/guilherme-pg/gaia/blob/948641c/shared/test/unit/mocks/mock_settings_listener.js#L13 . With that, I just got a green test run again: https://github.com/mozilla-b2g/gaia/pull/17849 .

I also pushed to try to make sure, still waiting on the results.

Comment 13

[Matej Novak [:matej]]

(In reply to Francesco Lodolo [:flod] from comment #8)
> Matej, how are we going to treat "Find My Device"? Is that supposed to be
> localized or kept in English like a trademark?

It should be localized. It's a feature of the phone, like Settings or any of the apps.

Comment 14

[Axel Hecht [:Pike]]

Then we'll need to have an entry in the manifest.webapp like 
  "locales": {
    "en-US": {
      "name": "Find my Device",
      "description": "Find My Device helps find, lock and erase a lost phone"
    }
  }

Comment 15

[Matej Novak [:matej]]

(In reply to Axel Hecht [:Pike] from comment #14)
> Then we'll need to have an entry in the manifest.webapp like 
>   "locales": {
>     "en-US": {
>       "name": "Find my Device",
>       "description": "Find My Device helps find, lock and erase a lost phone"
>     }
>   }

We should follow FxOS conventions here, but in general, "Find my device" should be in sentence case.

Comment 16

[Guilherme Gonçalves [:ggp]]

Alright, green pull request and the try run looks good. I also included the manifest changes suggested by :flod, but kept it "Find My Device" as that's what's being used everywhere else. Of course, I don't really mind changing it later if necessary, just like we had agreed to do with the other strings.

:kgrandon, can you please merge this?

https://github.com/mozilla-b2g/gaia/pull/17849
https://tbpl.mozilla.org/?tree=Try&rev=fd44c14d62a4

Comment 17

[Kevin Grandon :kgrandon]

Sure thing: https://github.com/mozilla-b2g/gaia/commit/e7dd2bea50712389583bf989325b30f7097c5e8c

Nice work, let's hope this one sticks :)

Comment 18

[Greg Weng [:snowmantw][:gweng][:λ]]

Today I've found that FindMyDevice's test app would fail if I rebase LockScreenWindow on it. The reason is that the new LockScreen would exist only if the LockScreenWindow exist, which now need to be invoked by some events like |screenchange|. So I think there is something we need to sync-up if we don't want to conflict both our code.

I also told Ivan and Tim about this situation, because I need their help to sync-up this for both our team.

And I don't know whether the FindMyDevice would require to manipulate more the LockScreen message (the pop-up window) or not. If so, please note some refactoring bugs here:

Bug 937442 - LockScreenWindow
Bug 965105 - LockScreen widgets (part of the lockscreen refactoring plan)
Bug 960381 - LockScreen refactoring bug (meta bug)
Bug 898348 - LockScreen as an app (may change the communication way between FindMyDevice and LockScreen)

Comment 19

[Guilherme Gonçalves [:ggp]]

Hi Greg,

I'll unfortunately not be able to look into this issue in depth today, but I'll do it tomorrow.

To (superficially) answer your question, the lockscreen message was specifically added for Find My Device, and all we need is to be able to lock the screen at will and change the text in the pop-up.

We can probably talk in more detail once I've studied the changes you're trying to make :)

Comment 20

[Greg Weng [:snowmantw][:gweng][:λ]]

OK, if FindMyDevice now only require to show the lockscreen and set a message on it, it seems that I can change the current test first, and land my LockScreenWindow. This blocks lots of follow-up bugs and has been landed but pulled out due to regressions. So if my change broken FindMyDevice (beyond the test), please tell me.

Comment 21

[Greg Weng [:snowmantw][:gweng][:λ]]

And I saw you communicate FindMyDevice with System app using the old mozSettings trick (observe |lockscreen.lock-message| in LockScreen), which should be deprecated after we have IAC.

I would not fix this in the LockScreenWindow patch, but please take some time to design the communication model using the IAC API, and we can discuss this if you have any questions.

Comment 22

[Greg Weng [:snowmantw][:gweng][:λ]]

And I've found that if I follow the integration test manually on my real device, the test-FindMyDevice app would not show the LockScreen as the test showed. I don't know what happened, and still investigate it.

Comment 23

[Greg Weng [:snowmantw][:gweng][:λ]]

I (may) finally find out why the settings change would not invoke the correspond methods in LockScreen: the |window.lockScreen| now would only be instantiated after the first time we open a LockScreenWindow. But the test change the settings without open such window, so the |window.lockScreen| would receive nothing even after it got instantiated.

This is about how we manipulate the |lockscreen.enabled| value. In fact, now the enabling change should be handled by LockScreenWindowManager, not the LockScreen. I would try to find a way to let FindMyDevice work under such modification, at least for this test.

Comment 24

[Greg Weng [:snowmantw][:gweng][:λ]]

If I insert a |throw new Error('...')| line at the test app's click event handler, when the marionette run the B2G, the throwing code would not be trigger by the test. However, if I click with my mouse, the line now would be triggered. It's so weird.

Comment 25

[Greg Weng [:snowmantw][:gweng][:λ]]

Finally! I've successfully fixed the failed test. I'll land the LockScreenWindow after Travis all green.

Changes:

1. the test now need to set the value as enabled, and
2. directly open the lockscreen window (hacking), to
3. instantiate the |window.lockScreen|, and then
4. reset the value and close the widow to start the test

And the test now needs to wait the enabled value got changed, then open the window manually again. This is because if the enabled value is false, the window would not open, and the lockscreen element would not display.

Comment 26

[Michael:mtreese]

All dependent bugs resolved