922126 - [Clock] Introduce AMD and associated build process

Status: RESOLVED FIXED

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

Description

[Mike Pennisi [:jugglinmike]]

Formally define Clock's modules using AMD and implement an AMD-aware build process. This:

- Improves maintainability through:
  - Explicit module inter-dependencies
  - Simplified `index.html` file that does not require redundant data
- Simplifies unit testing
- Optimizes initial load time on the device by combining all necessary code into a single file

The E-mail application already takes this approach and should be considered an example of how AMD may be leveraged within Gaia.

Comment 1

[Mike Pennisi [:jugglinmike]]

Attachment: 922126-clock-amd.patch

Hi Tim!

There are many changes here, but they are mostly limited to the beginning and ending of each file. You can find more background about my process in Bug 915023 and the obsolete pull request for that bug [1]. You can also refer to the E-mail application as it exists in `master` today [2]--the build process is derived from that app.

[1] https://github.com/mozilla-b2g/gaia/pull/12406
[2] https://github.com/mozilla-b2g/gaia/tree/de6b143ed823fc195c8a802ed4f833cc88d489ba/apps/email

Comment 2

[James Burke [:jrburke]]

Comment on attachment 813662 [details] [diff] [review]
922126-clock-amd.patch

I put some feedback comments in the pull request here:

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

I suggest addressing at least the Makefile and .css @import comments before an official review is done, as I needed those to properly test in make DEBUG=1 mode.

Comment 3

[Mike Pennisi [:jugglinmike]]

Attachment: 922126-clock-amd-v2.patch

I've incorporated James' feedback on my initial patch. I've squashed this down into one commit for this patch file, but you can find the individual commits on the GitHub pull request I've opened for this bug:

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

From the previous attachment:

Hi Tim!

There are many changes here, but they are mostly limited to the beginning and ending of each file. You can find more background about my process in Bug 915023 and the obsolete pull request for that bug [1]. You can also refer to the E-mail application as it exists in `master` today [2]--the build process is derived from that app.

[1] https://github.com/mozilla-b2g/gaia/pull/12406
[2] https://github.com/mozilla-b2g/gaia/tree/de6b143ed823fc195c8a802ed4f833cc88d489ba/apps/email

Comment 4

[Mike Pennisi [:jugglinmike]]

Attachment: 922126-clock-amd-v3.patch

I've incorporated James' feedback on my initial patch, and I've included one minor update to better-facilitate running the unit tests in the browser. I've squashed this down into one commit for this patch file, but you can find the individual commits on the GitHub pull request I've opened for this bug:

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

From the original attachment:

Hi Tim!

There are many changes here, but they are mostly limited to the beginning and ending of each file. You can find more background about my process in Bug 915023 and the obsolete pull request for that bug [1]. You can also refer to the E-mail application as it exists in `master` today [2]--the build process is derived from that app.

[1] https://github.com/mozilla-b2g/gaia/pull/12406
[2] https://github.com/mozilla-b2g/gaia/tree/de6b143ed823fc195c8a802ed4f833cc88d489ba/apps/email

Comment 5

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

Comment on attachment 813758 [details] [diff] [review]
922126-clock-amd-v3.patch

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

Having jrburke review the patch would be enough :) Thanks!

Comment 6

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

Comment on attachment 813758 [details] [diff] [review]
922126-clock-amd-v3.patch

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

Having jrburke review the patch would be enough :) Thanks!

Comment 7

[James Burke [:jrburke]]

Comment on attachment 813758 [details] [diff] [review]
922126-clock-amd-v3.patch

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

I just had a few very small follow up comments in the pull request. With those addressed and tested locally, r+ from me. If a rebase with those changes against latest master passes travis on the pull request, it is good to merge.

Comment 8

[Mike Pennisi [:jugglinmike]]

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

James: thanks a million for all your help!

Comment 9

[Mike Pennisi [:jugglinmike]]

Requesting "koi+" because this bug blocks a blocker (bug 915023)

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/da309d2360df

Comment 11

[Dylan Oliver [:doliver]]

triage: koi+, blocks a regression fix

Comment 12

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1.2.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.2, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.2
  git cherry-pick -x  959ac2f692d85072ffdc3d16a041b5bf4735ae59
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 13

[Mike Pennisi [:jugglinmike]]

Uplifted 959ac2f692d85072ffdc3d16a041b5bf4735ae59 to:
v1.2: 2ae3c8130498dfb9934039843bc42ac9e786c364

Comment 14

[Mike Lee [:mlee]]

Setting Target Milestone for all FxOS Perf 1.2 issues fixed for 10.25.