1057198 - [LockScreen] Decoupling LockScreen with System before it becomes an iframe

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Lockscreen, defect)

Description

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

After a brief discussion with Tim, we decide to make LockScreen in an iframe first. In this way we can keep the current event-based communication model while we're fixing the layout and testing issues, so the risk of fixing the communication model and layout and testing issues at the same time can be reduced.

Comment 1

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

WIP Patch: https://github.com/mozilla-b2g/gaia/pull/23192

Comment 2

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

Attachment: Patch

Set review since I rebased on the newest master and fixed all tests failed at the previous version. I think review can be processed while I continue to rebase it.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Do we need to wait for bug 1063026?

Comment 4

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8484810 [details] [review]
Patch

1st round: several comments.

Comment 5

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

Comment on attachment 8484810 [details] [review]
Patch

Fixed those addressed by Alive; move files to a new directory as Tim recommends, although this broke the background image and adding the extra on-going work to find out the root cause. Even so, I think to review it is still OK, because this should be only a resource path error.

Another issue is I need to sync media playback widget style changing from a Utility Tray patch manually (again; if God save us to let there isn't another 'again'). The bug and patch is: Bug 1063026.

Comment 6

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

I've found to move files from System app to a system/lockscreen directory brings lots of work to do, especially the tests. And, I've found we're playing the same game again: while we all agree to do as few changes as possible, so the patch can be landed sooner to avoid serious conflicts and test failures, we still add the extra requirements to fix them all at one patch. For this case, to move files of LockScreen into another directory is 100% reasonable, but it cause lots of trouble that I need to fix them all before the patch is OK.

Another thing I now worry about is to play this game make me almost have no time to care about other issues, and my leader and colleagues may estimate my performance according to the few contributions, especially their work usually are irrelevant to mine, and I'm just an untitled developer with poor prestige, not super star with great title and everybody would forgive his/her absence since he/she is of course doing some awesome work. Also, while others may resolve bunch of bugs and earn the prestige among us, I may still struggle with tests and conflicts of this patch and looks like silent on Bugzilla or other channels, just like the previous two failed patches. So, as a defensive step, I would start to record everything about this patch everyday, including brief summaries about how much requirements have been added, what tests have been fixed and which now are still failed, and how long I spent on them.

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8484810 [details] [review]
Patch

I didn't read the integration test of lockscreen*.. r=me with nits.

Comment 8

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

Attachment: Log: 9/11

Today's log file is here.

Comment 9

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

Summary:

Total: about 6 hours.
Gij failures (not completed): about 3 hours
Gu failures (done): about 2 hours
Wallpaper issue: about 1 hour

Issues not handled: Alive' comments, daily rebasing (no time to do that).

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Greg, you are not doing a bad job. There are always unforeseen issues or small nits to be improved. We call this software development. Please keep up the good work and we will surely get through this.

Comment 11

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

Spent lots of time to check on Ubutu the test gone crazy:

1. AppWindowManager.activeApp.ready seems never ready, so the closeApp would not be invoked
2. Even closing, isActive is still false
3. According to the previous result, it is false because the instance is null, means it's not registered, or instanced
4. But on the B2G desktop there is a LockScreen. It can be manipulated, although the unlocking function not works

As a result:

1. To see why activeApp is never ready
2. To see why the instance is never be registered
3. It works on Mac, only broken on Ubuntu, and it doesn't work even after re-download B2G desktop
4. To see why the instance is never be registered, but there is still a LockScreen, although the event forwarding seems broken

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8484810 [details] [review]
Patch

Alive's review is good here. Land it land it \o/.

Comment 13

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

Attachment: Log: 9/15

Summary:

Gij problem: 3 hours
Alive's issues: 2 hours

Undone:

Manually rebasing the media playback adjustments.
To see if Gij on Gaia-Try is green.

Comment 14

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

The Bug need to be merged manually is Bug 1063026.

Comment 15

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

Attachment: Log: 9/16

Summary:

Fix Gij: about 5 hours
Rebasing & unit tests: about 1 hour

Rest:

Check UI tests on Ubuntu
Re-trigger Gij to verify the result

Comment 16

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

Current situation:

1. Gu still failed with lots of timeouts. I don't know why this happen since unit tests are isolated with different app and iframe as sandbox. There is only one failure related to System app, and it even fails to reproduce on my Mac.

2. It conflicts again. Two conflicts in the same day. This makes me suspect if we can actually patch things like this "incrementally".

Comment 17

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

Attachment: Log: 9/17

Summary: Try to figure out why test failed: about 2 hours
Stoping to handle this patch directly. I would start to dive into the test, and seek to fix them in the current master first.

Comment 18

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

Today I fixed the some Gij failures of as-div-patch. Unfortunately, I need to handle other bugs so I didn't fix them all today.

Comment 19

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

Since as-iframe encountered serious testing issues, this bug should be a decoupling bug, which would land all functions except real iframe in. Some functions may be landed as dummy functions that would be implemented after the final as-iframe bug.

Comment 20

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

Attachment: Log: 9/23

Summary:
Resolve the Gij failures (#1): 3 hours
Rebasing and daily maintenance: 1 hour

Todo:
Remove nonexistent elements in LockScreen
Fix the 47 Gij errors.

Comment 21

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

Comment on attachment 8484810 [details] [review]
Patch

Yuren: as we discussed, please review or feedback the build part, which would move lockscreen.html into System app's index.html.

Comment 22

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Comment on attachment 8484810 [details] [review]
Patch

set feedback to ricky since he will maintain build system on taipei side.

Comment 23

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

Comment on attachment 8484810 [details] [review]
Patch

Since the main goal of this bug changed, I discussed with Alive and he may give some new opinions in the new review.

Comment 24

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8484810 [details] [review]
Patch

r+ as a intervening patch to real iframe.

Comment 25

[Ricky Chien [:rickychien] (inactive)]

Behavior on apps/system/build/build.js seems correct for me. We should wait Yuren to review this part.

Comment 26

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

Comment on attachment 8484810 [details] [review]
Patch

After discussed with Yuren, he agreed that I can set George as an additional reviewer, and either Yuren or George reviewed the patch, another one can be cleared.

Comment 27

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 8484810 [details] [review]
Patch

LGTM for system/build/build.js part. r=gduan

Comment 28

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

The patch only failed with one timeout at an irrelevant Calendar test:

https://tbpl.mozilla.org/?rev=b651f3851b2f4ad505f81e9d10dfe15cff7487d6&tree=Gaia-Try

And it passed all Gaia-Try test at the previous run:

https://tbpl.mozilla.org/?rev=0664756b3c2ef395d4df7c534ed0d3cddb80361f&tree=Gaia-Try

So I would land this patch.

Comment 29

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

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

Comment 30

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

Let's see if this patch can stay on master.

Comment 31

[Tony Chung [:tchung]]

Greg,

your patch in Bug 1057198 has broken our Gaia automation tests on device.  can you please revert the changes on master and fix so we can test again?

Thanks
Tony
----------------------------------------
On Sep 29, 2014, at 7:10 AM, Viorela Ioia <[e-mail]> wrote:

Hello all,

We are not able to write a proper report on v2.2, because tests are failing due to commit https://github.com/mozilla-b2g/gaia/commit/369e3579af3f41fcb9c9c3508d6358ff7b797b64 not landing in the build.

In exchange, we generated a report based on the latest b2g-inbound build, where the above commit was included. 
We have no failing test in the b2g-inbound build.

Cheers,
    Viorela

Comment 32

[Michael Henretty [:mikehenrty][:mhenretty]]

Hrm, it looks to me like this commit is actually fixing the tests rather than breaking them. Viorela, have the tests you are running against device been modified to work specifically this patch?

In any case, if I am understanding correctly, this issue will be fixed when this patch lands in QA's build. Viorela, can you confirm?

Comment 33

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

Sorry, I'm confused. This is what I've got so far

1. On TBPL it seems good with my patch, since from the inbound page [0] it looks OK after my patch, and no sheriff backed out my patch during these hours.

2. From your forwarding comment:

"We are not able to write a proper report on v2.2, because tests are failing due to commit https://github.com/mozilla-b2g/gaia/commit/369e3579af3f41fcb9c9c3508d6358ff7b797b64 not landing in the build"

I don't know exactly what this means. If the test failed "due to the commit not landing in the build", then, the problem seems could be solved with my commit landing into the test build. And since the master is still pre-v2.2, which even not been announced to start (we're now still in v2.1 stabilization, as far as I know), I think there is no uplifting issue, unless I miss something that different between testing and developing working flow.

If my patch broke anything either on master (but inbound seems good from TBPL) or "testing v2.2 build" (I still have no idea what it means), there should be errors that I can revert the patch according to that. But from the information the comment, TBPL and Gaia log shows, I have no clue to do that. If I miss anything I should know, please tell me. Thanks. 

[0] https://tbpl.mozilla.org/?tree=B2g-Inbound

Comment 34

[Viorela Ioia [:viorela]]

(In reply to Michael Henretty [:mhenretty] from comment #32)
> Hrm, it looks to me like this commit is actually fixing the tests rather
> than breaking them. Viorela, have the tests you are running against device
> been modified to work specifically this patch?
> 
> In any case, if I am understanding correctly, this issue will be fixed when
> this patch lands in QA's build. Viorela, can you confirm?

Michael, that is correct, the issue was solved by this commit landing into the build.
The problem here is that the changes made in this patch landed in our tests, but not in the build on which the tests were run. 
The reason why I wrote a report based on the b2g-i is that the commit above was included. 
So there's no need to back this out, tests are passing on latest v2.2 build (where this commit was included): http://jenkins1.qa.scl3.mozilla.com/job/flame-kk-319.mozilla-central.ui.functional.smoke/14/HTML_Report/

Sorry for the misunderstanding here.