(inactive after 6/18) Alive Kuo [:alive] Reporter
	Description • 11 years ago

No matter we're having a standalone LockScreen app or not,
it's better if we could move current lockscreen into scope of window management.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 1 • 11 years ago

Since this bug relevant to LockScreen and is a facility change, I would start to survey it.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 2 • 11 years ago

Implemented the LockScreenWindow and LockScreenWindowManager.

The LockScreenWindowManager would also create the LockScreen and LockScreenWindow. Because this is really a simple case so I don't implement the factory.

And there're some code mocking the iframe's behavior and do some hacking tricks in the LockScreenWindow. If I done it improperly, please give me some advices.

We can remove these hacks after we put a really app-ize LockScreen inside the iframe. If we complete it, we can adjust these two constructors with:

1. Remove the LockScreen HTML template. Because the HTML would be provided by the app's entry file.
2. Remove the mocking functions and properties inside the LockScreenWindow.
3. LockScreenWindow would no need to create both LockScreen and LockScreenWindow. The former would be created inside the app while it got loaded.

	(inactive after 6/18) Alive Kuo [:alive] Reporter
	Comment 3 • 11 years ago

Comment on attachment 8384469 [details]
Patch

Cancel review for singleton of lockscreenWindow. Don't need to have multiple running lockscreen Window I think.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 4 • 11 years ago

Comment on attachment 8384469 [details]
Patch

Fixed the patch and set review again.

	(inactive after 6/18) Alive Kuo [:alive] Reporter
	Comment 5 • 11 years ago

Comment on attachment 8384469 [details]
Patch

Some tests are failing please check before sending review.
Also some more nits.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 6 • 11 years ago

Passed all relevant Travis tests:

https://travis-ci.org/mozilla-b2g/gaia/builds/20506205

The errors it shows are irrelevant to lockscreen. I'll push another commit to re-run those timeout issue.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 7 • 11 years ago

Comment on attachment 8384469 [details]
Patch

Since the patch passed all related tests, reset the view flag.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 8 • 10 years ago

I'll try to land this tomorrow after solving all conflicts.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 9 • 10 years ago

The six questions:

> Is this a good idea?
Yes. While we're refactoring System to manage these window-like
components with managers, this is a good idea to achieve that. Beside
that, this is also important for lockscreen-as-an-app, while the later
bug needs a window to manage the LockScreen app.

> What problem does it solve (and for whom)?
As above.

> Is it high priority?
Yes. Because this blocks the LockScreen refactoring plan I mentioned
in dev-gaia. And this also blocks the lockscreen-as-an-app bug, which
is one part of our 1.5 plan.

> Are there any risks involved in doing this?
Unknown regressions

> Who should I ask for input? Who should at least know that this is happening?
- Tim as the owner of Gaia, and the lockscreen reviewer
- Alive as the owner of System and reviewer

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 10 • 10 years ago

Travis now is only failed at irrelevant tests (APP=search)

https://travis-ci.org/mozilla-b2g/gaia/jobs/21913007

I would test it again at another clean Gaia. If it still fail, I would land the code after resolved the last conflicts (from the SystemDialog).

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 11 • 10 years ago

I've found the error would occur even with the clean Gaia (re-cloned from mozilla/b2g and sticked on the master branch). So I attached the screenshot and would land the patch later.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 12 • 10 years ago

The same patch but would be landed in master. Not obsolete the original one to keep the review logs.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 13 • 10 years ago

master:

https://github.com/mozilla-b2g/gaia/commit/7a62da7f3bd1b3bbb80e6358d8e201b2a1c53bc5

	Kevin Grandon :kgrandon
	Comment 14 • 10 years ago

It seems you had some unrelated changes that may have slipped in here, possibly due to a conflict. See: https://github.com/mozilla-b2g/gaia/pull/17791/files#diff-8a999d20affe1158e168ecc8af594dcaR160

This resulted in several failing marionette tests, so we had to back this out. Please fix, wait for a travis green, and re-land.

https://github.com/mozilla-b2g/gaia/commit/4d8b4d707ea9ced89e5f6676a34fe7b03fb4bab7

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 15 • 10 years ago

Thanks Kevin. I now start to survey which parts cause the search app failed. If it's not the failed tests you mentioned, please tell me.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 16 • 10 years ago

Kevin: Travis now is all green:

https://travis-ci.org/mozilla-b2g/gaia/builds/21990564

So I would land it again. Cross finger!

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 17 • 10 years ago

New patch after the back-out.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 18 • 10 years ago

master: https://github.com/mozilla-b2g/gaia/commit/f9a44b5e3cbea1e48b12716aceb0e365fec9e18e

	Ahmed Nefzaoui [:Nefzaoui]
	Comment 19 • 10 years ago

Something is wrong here, after the first unlock there's no clock in the status bar and first icon on the right is battery. I know it's related to this because when the patch were backed out the clock near the battery icon appeared again.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 20 • 10 years ago

Thanks for remind. I've tested it on my device and it actually happen (so sad). One more thing I can confirm is if we boot the device without LockScreen, the clock would appear. But after I set it on via Settings, the clock on the status bar would disappear, and never appear again.

I think I need to backout this first, and then to fix it again.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 21 • 10 years ago

reverted:

https://github.com/mozilla-b2g/gaia/commit/aa17def17754097b5004b370ebe26b557cdc9586

	Ahmed Nefzaoui [:Nefzaoui]
	Comment 22 • 10 years ago

Sorry for bringing such annoying/upsetting news, I just didn't want it to go unnoticed.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 23 • 10 years ago

No problem. It benefits me that someone can remind me which part may be broken.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 24 • 10 years ago

Fixed the asynchrnous LockScreen and LockScreenWindow states in LockScreenWindowManager.

Waiting Travis.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 25 • 10 years ago

Fixed the failed test of FindMyDevice. Waiting Travis.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 26 • 10 years ago

Travis is green:

https://travis-ci.org/mozilla-b2g/gaia/builds/22156264

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 27 • 10 years ago

Merged again,

master: https://github.com/mozilla-b2g/gaia/commit/32581663a25782810518340b271f2a93de3677f0

	Carsten Book [:Tomcat]
	Comment 28 • 10 years ago

Hi Greg,

sorry had to backout/revert this change for a test failure like https://tbpl.mozilla.org/php/getParsedLog.php?id=37203345&tree=B2g-Inbound

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 29 • 10 years ago

I believe this can be solve with one line fix in the test file (didn't require the shared library). But Travis should told me this...

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 30 • 10 years ago

Let's try again!

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 31 • 10 years ago

Travis green again:

https://travis-ci.org/mozilla-b2g/gaia/builds/22183936

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 32 • 10 years ago

master: https://github.com/mozilla-b2g/gaia/commit/680f2b0f22dac1eea680158c419a1c7d8a0ad70e

	Dale Harvey (:daleharvey)
	Comment 33 • 10 years ago

It looks like this caused a regression - https://bugzilla.mozilla.org/show_bug.cgi?id=992466

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 34 • 10 years ago

I've solved the issue at the Bug 992466. The patch now is set 'r?' .

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 35 • 10 years ago

The patch is now landed.

	Zac C (:zac)
	Comment 36 • 10 years ago

Some really poor test code here, potential infinite loop and a repeated action which could mask real failures.

https://github.com/mozilla-b2g/gaia/commit/9f21d2c750b4c642f77bd3bf638f0f2fea93fb08#diff-1c89300cc68e8b8cf66db00946358182R38

This should not have been done this way. Basically just how it was before was best practice and correct. Your change is just masking some other kind of problem.

Snowman, can you fix it?

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 37 • 10 years ago

The problem is the error (before I added the code) would only occur on Travis, and the patch would pass the test perfectly on my local machine (any time, every run). So I think before I "masked" it, there is something already wrong, but I lacked the skill to debug with Travis in such case, so the code you saw now is the only found that I've found.

And I remembered the problem is, even after I successfully execute the |_slide_to_unlock|, the panel would still not display. But if I inserted two or more |_slide_to_unlock|, the panel would show. I remembered this is tough because if you check the code of the |_slide_to_unlock| function, the |find_element| it called should guarantee the element can be manipulate, or there would be some error at the line the Action chain manipulate it. So obviously the unlocking function done it's work at the first time, but the consequence of its effort not happened, although it actually done at the local console.

Anyway, I'll fire a bug to try to fix it again. I would log any discovery to record any strange situation here.

	Zac C (:zac)
	Comment 38 • 10 years ago

Thanks Greg!

	Jason Smith [:jsmith]
	Comment 39 • 10 years ago

This patch has caused too many critical regressions (see the dependencies). Let's get this backed out.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 40 • 10 years ago

I gather some of these bugs happened because we're now in an embarrassing situation: we have an ordinary window class just like other windows in the System app, but we don't have the corresponding isolating LockScreen app. So some events now handled by the LockScreen class would need to be handled by the window as well, and some events should be migrated to be handled only by the window class. I think my options is:

1. Back-out this patch and fix the regressions mentioned by Jason, and landed it again

This would at least solve the current regressions. However after my re-landing, new regressions may still appear due to the reason above, because I can't do so detailed tests as QA team did (and thanks for that). I can only let the patch pass all tests again as it already did, and "looks fine" on my local device (test all STR of all discovered regressions on this bug).

Of course the new regressions should be fewer compares to it before the back-out. However I may need re-run this back-out & regressions flow several times until the thing finally get stable enough. I'm afraid that we may both suffer from this.

2. Jump to the next step to make LockScreen as an app and land it with the window at the same time

If regressions are really caused (as I gathered) by the lacking of corresponding LockScreen app (so I must do so many workaround), these problem may be exposed more easily if we just jump to make LockScreen as an app, and land the both bug at the same time. Because at the early testing stage I may be better to find these problems because there would be no ambiguous questions that the events should be handled by whom.

The risk is this would force me to re-schedule the whole LockScreen refactoring plan, and may result in more bugs from those components that related with the LockScreen. However if LockScreenWindow continuously cause regressions due to the above reason, I think the schedule would still be delayed.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 41 • 10 years ago

(In reply to Jason Smith [:jsmith] from comment #39)
> This patch has caused too many critical regressions (see the dependencies).
> Let's get this backed out.

Greg have discussed with me offline and I think base on the current status I disagree with the statement above.

Yes, there are 4 existing regression bug, but none of them are smoketest breakage, and two of them are already fixed, 1 under review, and 1 non-reproducible locally. I think we are on track on handling them, judging by the turnaround time of these bugs and the current project schedule. We can re-evaluate if the situation changes (# of regression bugs explodes or schedule changes again, like 1.4).

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 42 • 10 years ago

... and now we are down to 3 as I type.

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)
	Comment 43 • 10 years ago

Adding to the pile: This patch breaks debug b2g desktop builds, outlined in bug 994900. Backing it out restores our ability to use B2G Desktop in debug.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 44 • 10 years ago

Got it. Debugging and NI to know the detail STR.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 45 • 10 years ago

I've encountered problem to reproduce the Bug 994900. Kyle and me now trying to solve this inconsistent result in the bug.