922128 - [Clock] Defer Panel loading and execution

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Clock, defect)

Description

[Mike Pennisi [:jugglinmike]]

Because the Clock application consistently opens to the "Alarm" panel, the code to display all other panels is not needed at initial load time. The resources for any other panel should not be loaded until one of the following events take place:

1. The user attempts to navigate to the panel
2. The Alarm panel's Clock face is displayed and fully functional

Deferring loading an execution in this way will improve the perceived start up time.

Comment 1

[Mike Pennisi [:jugglinmike]]

I'm making this as a dependent on Bug 907177 since independent integration tests will help mitigate the risk of deferred module loading.

Comment 2

[Mike Pennisi [:jugglinmike]]

Attachment: 922128-defer-panels.patch

There is one somewhat irregular aspect to this approach: I am wrapping Gaia's shared Template library (which only accepts DOM nodes) so that it will accept strings. This means that template functions cannot be optimized by the build process. I feel this is acceptable because this patch already introduces a lot of change, and I want to avoid accidentally introducing regressions that might come about from switching templating engines.

Addressing the template optimization in a separate bug allows this one to land sooner and isolates the opportunities for regressions.

I've also opened a pull request on GitHub:

https://github.com/mozilla-b2g/gaia/pull/12868

(This patch has a little more risk than the one we landed for bug 922126, so it may make sense to land the integration tests I wrote for bug 907177 first. Please also note that it blocks a blocker: bug 915023.)

Comment 3

[Mike Pennisi [:jugglinmike]]

Comment on attachment 817511 [details] [diff] [review]
922128-defer-panels.patch

Since I used this pull request to isolate a bug in the test environment (ultimately opening bug 928751), but I don't want to lose that work, I'm going to open a new pull request to track a new version of this patch.

Comment 4

[Mike Pennisi [:jugglinmike]]

Attachment: Pull request on GitHub

This incorporates the feedback I received on attachment 817511 [details] [diff] [review], excludes the changes I made to isolate bug 928751, and squashes the changes into a single commit.

Comment 5

[James Burke [:jrburke]]

Comment on attachment 819505 [details] [review]
Pull request on GitHub

Going off the previous reviews I did, this code seems to be the same, just squashed, and avoiding the mozSetMessageHandler issue by using a mock for the module that used it. r+ from me. The travis build for the pull request will likely turn green if you merge latest master and rebase on top of that.

Comment 6

[[e-mail], [e-mail]]

Comment on attachment 817511 [details] [diff] [review]
922128-defer-panels.patch

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

Hi Mike,

The refactor is pretty clear. I appreciate the architecture of deferring each panel loading. Since James has reviewed in detail, I focus on the functionality. After applied the patch, I'm not able to create new alarm, if there is already one existed alarm. Looks like some nits in the patch. Please take a look it again. Thanks.

Comment 7

[[e-mail], [e-mail]]

Comment on attachment 819505 [details] [review]
Pull request on GitHub

As comment 6 mentioned, I unset the review flag. Once the nits fixed, please set the review again to me. Thanks.

Comment 8

[Mike Pennisi [:jugglinmike]]

This is strange, I can't reproduce this behavior! Here are my steps:

1. Flash the latest build of Gecko
2. Install Clock from this branch
3. Open the Clock application
4. Create a new alarm
5. Force-quit the Clock application
6. Open the Clock application
7. Create another new alarm

As expected, I see two alarms listed in the Alarm panel at the end of this process.

Would you mind sharing some more information?

- Which device are you using? (I'm using an inari)
- What version of Gecko is installed? (I'm using v1.3)

I suspect there may be a database issue at play here. Instead of re-flashing Gecko, you may be able to resolve this issue by running `make delete-databases`

Comment 9

[[e-mail], [e-mail]]

Here is my build version:

Gaia:     b96bc1bef2eccd9ef3bec8ee58f303df823e3f51
Gecko:    http://hg.mozilla.org/mozilla-central/rev/21d97baadc05
BuildID   20131023040203
Version   27.0a1

My steps are as following:

1. Flash Gecko and Gaia to above build version.
2. Make reset-gaia with this branch
3. Launch Clock app
4. Create a new alarm with name A
5. Create a new alarm with name B
6. Delete alarm B
7. Create a new alarm **

At step 7, it will go into "Edit Alarm" A page. Then, I try to click "Close" button. It will be stuck in this page. And the "Close" button is active. You may try to add more alarms before try step 6. It could help you to reproduce the issue easily. I have tried to run 'make delete-databases' and re-launch Clock. I'm still able to reproduce the symphony.

Comment 10

[Mike Pennisi [:jugglinmike]]

Attachment: 922128-defer-panels-4.patch

Since this has been blocked by bug 907177 for so long, the patch has fallen behind `master`. I took some time to rebase it--see attached.

Comment 11

[James Burke [:jrburke]]

Given the difficulty of being able to debug intermittents, not enough insight to be able to reproduce locally too, I would rather see this code go ahead than wait indefinitely on the test issue outcome.

Comment 12

[Mike Pennisi [:jugglinmike]]

Attachment: 922128-defer-panels-5.patch

This patch rebases over the changes introduced by the patch for bug 898354.

@jrburke: I also want to see this landed. but I'm still a little concerned about risk from such a spanning change.

Comment 13

[Mike Pennisi [:jugglinmike]]

Attachment: 922128-defer-panels-6.patch

Rebasing this work over the latest Clock work again.

In order to unblock this bug, I've opened bug 961758. The resolution there should allow bug 907177 to land, which will in turn allow this to land.

Comment 14

[Mike Pennisi [:jugglinmike]]

Hey Macrus,

I'm picking this back up now that bug 907177 has landed. The trouble is, I'm moving a lot of code, and all new files are subject to the new linting rules. I could fix all the issues, but because the change set is already kind of spanning, I'm inclined to add them to the "approved failure" list and create a separate bug (blocked by this one) specifically for getting the code to pass lint.

What do you think of that approach?

Comment 15

[Marcus Cavanaugh [:mcav]]

Maybe you could steal the review for bug 972463 and then we can land that to fix the linter errors?

Comment 16

[Mike Pennisi [:jugglinmike]]

Attachment: Pull request on GitHub.com

Hey Marcus,

Nice work getting the codebase to pass lint! I've rebased over that work, and the tests are passing on TravisCI.

Comment 17

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8376607 [details] [review]
Pull request on GitHub.com

Awesome patch, Mike. This'll be great to have. It looks good to me, though it's not immediately clear from reading the patch how these changes interact with the RequireJS configuration; I made a comment about that -- it'd be nice to have some documentation somewhere (or maybe even a README update) about how the loading process works, etc., so that if we had to e.g. add/remove a panel, where we'd need to change the RequireJS config, etc.

Or, if all that stuff is covered well in some MDN or RequireJS docs, a pointer to that instead would be good enough for me.

The patch itself LGTM. I also have a patch impending as soon as the tree opens up, so depending on who lands first one of us will have to rebase.

Comment 18

[Marcus Cavanaugh [:mcav]]

So there's this ongoing Gaia tree closure right now, but I'd like to get this in as soon as it opens so that I don't end up with horrible git merges. If it's OK with you, I'll check Travis and merge when it opens, unless you'd like to do it or you get to it before I do.

Comment 19

[Mike Pennisi [:jugglinmike]]

Attachment: 922128-defer-panels-7.patch

Yeah, the timing on this tree closure is a little unfortunate (is there ever a good time for that?). I've had my finger on the "Merge" button for the past 24 hours :P

If you get the chance, then by all means: go for it! I've pushed the latest commit to GitHub (and updated the patch file attached to this bug) so that you can merge the version that includes your final review request.

Comment 20

[Marcus Cavanaugh [:mcav]]

Happy to report that after much Travis-wrangling, this has been landed on master:

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