987487 - [Gaia] [Build] Refactoring webapp-optimize.js and unit tests

Status: RESOLVED FIXED

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

Description

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

Refactoring webapp-optimize.js and write unit tests for it.

Comment 1

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

WIP:
https://github.com/cctuan/gaia/commit/45604e491b5619ea59d9951d6d41ae3e294d843b

1. refactoring for better test coverage
2. optimize files in build_stage
3. remove unnecessary files after merged
4. add interface for css minifiy

Comment 2

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

Attachment: PR to master

Hi Yuren,
could you help me to check this patch's arch?

Comment 3

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

Comment on attachment 8421535 [details] [review]
PR to master

Finally I have read all webapp-optimize.js.

please address issues which mentioned on github, and set review flag next time.

BTW, please don't squash commits for now to help me review this huge pr.

Comment 4

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

Follow-up - bug 1012464 : move optimize_embedGlobals to other place.

Comment 5

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

Issues are addressed and unit tests are done.
waiting for travis.

Comment 6

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

Comment on attachment 8421535 [details] [review]
PR to master

Hi Yuren,
could you check this patch?
Travis report failed, but I think it's due to other commit which use resource of shared/page.

Comment 7

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

Travis is all green now.

Comment 8

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

Comment on attachment 8421535 [details] [review]
PR to master

r=yurenju if issues mentioned on github are fixed.

Comment 9

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

thanks Yuren!
Merge to master,
https://github.com/mozilla-b2g/gaia/commit/619530c7cee4b30c4aab751d4e62117783d4e97a

Comment 10

[Anthony Ricaud (:rik)]

I had to back this out since it broke a lot of apps: https://github.com/mozilla-b2g/gaia/commit/514cc1e245e38efe2ba3111535b91efa89b79ae9

Comment 11

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

Attachment: PR to master 2

I found that I didn't optimize html inside shared/page.

Issues should be fixed now, waiting for travis and tbpl try report.

Comment 12

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

Travis all pass:
https://travis-ci.org/mozilla-b2g/gaia/builds/26029959

Gaia-try:
There's a Goop failing test, but it seems it fails everywhere.
https://tbpl.mozilla.org/?tree=Gaia-Try#push-2628

try-server:
pip-install error? I think it's not related to me.
https://tbpl.mozilla.org/?tree=Try&rev=22aebf2c3d2e

Comment 13

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

Comment on attachment 8428508 [details] [review]
PR to master 2

Hi Yuren,
I've put my fix in this patch. However, they're merged together by accident.
The main change I've made is,
1. add files array shared/pages for scanning.
2. modify unit tests

Could you check this patch again?

Comment 14

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

(In reply to George Duan [:gduan] [:喬智] from comment #12)
> 
> Travis all pass:
> https://travis-ci.org/mozilla-b2g/gaia/builds/26029959
> 
> Gaia-try:
> There's a Goop failing test, but it seems it fails everywhere.
> https://tbpl.mozilla.org/?tree=Gaia-Try#push-2628
and also Gi test.
> 
> try-server:
> pip-install error? I think it's not related to me.
> https://tbpl.mozilla.org/?tree=Try&rev=22aebf2c3d2e

Comment 15

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

Comment on attachment 8428508 [details] [review]
PR to master 2

cancel revuew request since George are working on fixing integration test

Comment 16

[howie [:howie]]

move to 2.1

Comment 17

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

Comment on attachment 8428508 [details] [review]
PR to master 2

Hi Yuren,

could you review this patch again?

Here are some changes I made,
1. remove player.js from music/index.html, since it's loaded by LazyLoad.
2. add single_file_optimize_blacklist to ignore communication app. we'll figure out which script cannot be minified in comms app.
3. timeout extend in comms app and test again and pass.
4. Test in GAIA_OPTIMIZE=1 on travis. I'll remove it once all pass after I merge all commits into one.

Comment 18

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

Comment on attachment 8428508 [details] [review]
PR to master 2

looks good, r=yurenju.

please file two follow-up bugs before landing:
1. remove logic for handling shared/pages
2. add a optimize configration file per app, we shouldn't specify certain app in webapp-optimize.js

Comment 19

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

test with |make GAIA_OPTIMIZE=1| got all green.
https://travis-ci.org/cctuan/gaia/builds/26354676

Now I'm going to remove GAIA_OPTIMIZE=1 from travis test.

Comment 20

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

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

Comment 21

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

Merge into master,
https://github.com/mozilla-b2g/gaia/commit/d5c9689b19d8871bc8e03d959db4959a90041460

Comment 22

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

based on https://github.com/mozilla-b2g/gaia/pull/19627#discussion_r13170970
Since it may break some apps which use r.js to optimize itself, I file another bug 1018108 to solve it.

Comment 23

[:gerard-majax]

Dialer's call log is broken for me on Flame with this landed.

Comment 24

[:gerard-majax]

With gaia at e33061348a5134923252a8bddc835000e4fcfa9c

> $ git revert --no-commit -m 1 d5c9689b19d8871bc8e03d959db4959a90041460

I get call log to work again on my Flame. I'm just performing:

> $ make MOZILLA_OFFICIAL=1 PRODUCTION=1 REMOTE_DEBUGGER=1 NOFTU=1 GAIA_DEV_PIXELS_PER_PX=1.5 GAIA_OPTIMIZE=1 install-gaia

Comment 25

[Jason Smith [:jsmith]]

George - This is way too risky to land for 2.0. We need this backed out of master. When master becomes 2.1, then try to landing this again with the fix to the issue that Alexandre hit.

Comment 26

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

Hi Alex,
I tried buri, unagi, inari and frame, with your command at e33061348a5134923252a8bddc835000e4fcfa9c.

> > $ make MOZILLA_OFFICIAL=1 PRODUCTION=1 REMOTE_DEBUGGER=1 NOFTU=1 GAIA_DEV_PIXELS_PER_PX=1.5 GAIA_OPTIMIZE=1 install-gaia

and found the call log works just fine.
I guess you're using command line to flash the device. Could you execute |make clean| and |git clean -fd| and run again ? Perhaps also attach the screenshot when pb happens. If it still is, I'll backout it!
Thanks.

Comment 27

[:gerard-majax]

(In reply to George Duan [:gduan] [:喬智] from comment #26)
> Hi Alex,
> I tried buri, unagi, inari and frame, with your command at
> e33061348a5134923252a8bddc835000e4fcfa9c.
> 
> > > $ make MOZILLA_OFFICIAL=1 PRODUCTION=1 REMOTE_DEBUGGER=1 NOFTU=1 GAIA_DEV_PIXELS_PER_PX=1.5 GAIA_OPTIMIZE=1 install-gaia
> 
> and found the call log works just fine.
> I guess you're using command line to flash the device. Could you execute
> |make clean| and |git clean -fd| and run again ? Perhaps also attach the
> screenshot when pb happens. If it still is, I'll backout it!
> Thanks.

No, I'm not using the command line to install gaia. I'm building and flashing complete system.img for Flame.

Comment 28

[:gerard-majax]

> alex@portable-alex:~/codaz/Mozilla/b2g/devices/Flame/B2G/gaia$ git status .
> # HEAD detached at e330613
> # Untracked files:
> #   (use "git add <file>..." to include in what will be committed)
> #
> #	.b2g.mk
> alex@portable-alex:~/codaz/Mozilla/b2g/devices/Flame/B2G/gaia$ cat .b2g.mk 
> PRODUCTION=1 GAIA_DEV_PIXELS_PER_PX=1.5 B2G_SYSTEM_APPS=1
< alex@portable-alex:~/codaz/Mozilla/b2g/devices/Flame/B2G/gaia$

Comment 29

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

Alex, I can't reproduce this issue, could you try this command? maybe there are some old files in your device with |install-gaia|.

make MOZILLA_OFFICIAL=1 PRODUCTION=1 REMOTE_DEBUGGER=1 NOFTU=1 GAIA_DEV_PIXELS_PER_PX=1.5 GAIA_OPTIMIZE=1 reset-gaia

Comment 30

[:gerard-majax]

(In reply to Yuren [:yurenju] from comment #29)
> Alex, I can't reproduce this issue, could you try this command? maybe there
> are some old files in your device with |install-gaia|.
> 
> make MOZILLA_OFFICIAL=1 PRODUCTION=1 REMOTE_DEBUGGER=1 NOFTU=1
> GAIA_DEV_PIXELS_PER_PX=1.5 GAIA_OPTIMIZE=1 reset-gaia

In which case it means this patch will break upgrade path.

Comment 31

[:gerard-majax]

Still reproducing after |git clean -fdx| and producing a fresh new profile.

Comment 32

[:gerard-majax]

Attachment: call-log.mp4

Video of the issue.

Comment 33

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

(In reply to Alexandre LISSY :gerard-majax from comment #32)
> Created attachment 8433102 [details]
> call-log.mp4
> 
> Video of the issue.
I've seen this issue on my first landing, because I didn't put files from shared/page into optimize process, so some necessary files are removed unexpectedly. But I already fixed it on my 2nd patch.

Comment 34

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

It can reproduced on below pvt build.
flame-mozilla-central-20140602072051-ril01.02.00.019.102.zip

but cannot on 
flame-mozilla-central-20140602160205-ril01.02.00.019.102.zip

Comment 35

[:gerard-majax]

(In reply to George Duan [:gduan] [:喬智] from comment #34)
> It can reproduced on below pvt build.
> flame-mozilla-central-20140602072051-ril01.02.00.019.102.zip
> 
> but cannot on 
> flame-mozilla-central-20140602160205-ril01.02.00.019.102.zip

Good news at least :). Can you get the git head of Gaia in each ?

Comment 36

[Anthony Ricaud (:rik)]

So let's go back a bit here.

George: Could you explain the purpose of this refactor? What is it helping? I can see that Settings, Email, Camera, Clock, Settings and Communications are now blacklisted. This will affect the start time of those apps. And if it doesn't, why are we aggregating things in the first place?

If it does affect start time, we should back out this patch.

Comment 37

[Kevin Grandon :kgrandon]

Yeah, we should definitely not be adding additional files to the blacklist. If anything, removing any existing files from the blacklist should be top priority.

Comment 38

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

Hi Rik,
I put those app in black list due to they're already optimized by r.js, so it's not necessary to optimize they again. In the future, we'll put optimize task in each app itself and let app decide when to optimize, zip, load outside resource(like shared).

Comment 39

[Anthony Ricaud (:rik)]

We had to close the tree because of this. So backing out again. It at least caused bug 1019607, bug 1019205 and bug 1019639. Therefore, this is not allowed to land again in 2.0.

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

Comment 40

[Anthony Ricaud (:rik)]

(In reply to George Duan [:gduan] [:喬智] from comment #38)
> Hi Rik,
> I put those app in black list due to they're already optimized by r.js, so
> it's not necessary to optimize they again. In the future, we'll put optimize
> task in each app itself and let app decide when to optimize, zip, load
> outside resource(like shared).
I'm not aware of Communications being optimised by r.js (I have no idea what r.js is). Could you point me to the code doing that?

Comment 41

[Anthony Ricaud (:rik)]

Also, before landing this again, can we look into an automated way to test optimised builds?

Comment 42

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

we can compare ASTs tree to verify it just like bug 903149 for js minify.

http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/js-compare-ast.js

Comment 43

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

more information, we can use |require('chrome').Cu.import("resource://gre/modules/reflect.jsm")| to get Relect parser.

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Parser_API

Comment 44

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

Please set feedback? to me when the next patch is ready, thanks!

Comment 45

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

Attachment: PR to master 3

waiting for tbpl first.

Comment 46

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

Comment on attachment 8442579 [details] [review]
PR to master 3

It seems tbpl tests already pass but there're still some failure which may not related to my patch.

Hi Yuren,
I've done below modifications in PR to master 3 compared to previous one.

1. rebase to master (move webapp-optimize to post-app)
2. do not remove files that have already been merged. I will file a new bug to add this feature back.
3. conflict with master (add PRETRANSLATION_BLACKLIST)

Please kindly check, thanks!

Hi Lissy,
I would need your feedback on this patch and see if those bugs can still be reproduced.

Hi Tim,
as comment 44, could I also have your feedback?

Comment 47

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

File bug 1027985 for unit test of optimized js file.

Comment 48

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

since we can't reproduce the issue which :gerard-majax mentioned, I would like to get :gerard-majax's feedback for this pr.

Comment 49

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

Hi Jason,

I think I already found the failure cause of previous commit (item 2 of comment 46), so I update my patch and hopefully reland it again.

https://travis-ci.org/mozilla-b2g/gaia/builds/27931994
1. I've put GAIA_OPTIMIZE=1 in marionette test if it seems to pass, except gaia_ui_tests SUITE=unit, I don't think it's related to it, so I rerun it.

https://tbpl.mozilla.org/?tree=Gaia-Try&rev=04d3892151936fedb973c3cc0fb4626b36c6e418
2. tbpl pass, except month_view_test.js (common issue , bug 1027539)

3. Comms app works fine on my side with |make production|

4. seek help(testing) from those who can reproduce bug from previous patch.

I would like to try run marionette for 20 times in weekend and see if it's stable.
Sometime some bugs I cannot reproduce on my site, so, would you mind to take a look of my patch?

Comment 50

[Jason Smith [:jsmith]]

Edward - Can you help test this patch?

Comment 51

[:gerard-majax]

Sorry if it may sound harsh, but can't you just fix bug 1027985 prior to landing this? I feel like it's the safer way.

Comment 52

[Edward Chen[:edchen]]

Hi Jason,

When the bug fixed, I will verify it.

Comment 53

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

Comment on attachment 8442579 [details] [review]
PR to master 3

I feel having comment 46 point 2 makes this bug safe to land on v2.1.

Comment 55

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

Put it into my third commit.

(In reply to Alexandre LISSY :gerard-majax from comment #51)
> Sorry if it may sound harsh, but can't you just fix bug 1027985 prior to
> landing this? I feel like it's the safer way.

Comment 56

[Anthony Ricaud (:rik)]

Have you verified that the test added in the third commit is red when running against the two first commits that landed?

Comment 57

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

Hi Rik,
most of failures randomly happen, so I rerun them.

Hi Lissy,
I've put it in third commit.

Comment 58

[:gerard-majax]

(In reply to George Duan [:gduan] [:喬智] from comment #57)
> Hi Rik,
> most of failures randomly happen, so I rerun them.
> 
> Hi Lissy,
> I've put it in third commit.

Give the status of this bug: several people unable to reproduce when one was, then everyone getting, what makes you confident that the fact that it works for me will be okay for everyone? I may just have been lucky triggering the bug the other day.

Comment 59

[Alexandre Poirot [:ochameau]]

Comment on attachment 8442579 [details] [review]
PR to master 3

Does this refactoring is expected to modify build system output?
(I would expect code move/refactoring to not change it!)

I'm asking that because it does change its output significantly!
(If you generate a profile without and then with your patch and do "diff -r profile_without profile_with", you will see many differences)

Can you look at those and justify them?

For example, I can see in html pages:
  <link rel="prefetch" type="application/l10n" href="/locales-obj/{{locale}}.json">
with your patch that I don't get without.

Also with your patch the html is more strict. All attributes with no value now have an empty string.
So that:
  <script defer src="/shared/js/l10n.js"></script>
becomes:
   <script defer="" src="/shared/js/l10n.js"></script>

I also see some tag like these one to disappear:
  <link rel="import" href="/elements/errors.html">

Given the differences in profile output you are introducing I don't feel confident landing this patch without at least some explanations.

Comment 60

[Alexandre Poirot [:ochameau]]

Attachment: build stage diff without/with this patch

Comment 61

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

Comment on attachment 8442579 [details] [review]
PR to master 3

discussed with George offline, we should use diff to check these three scenario:
1. make without any parameter
2. GAIA_OPTIMIZE=1 make
3. MOZILLA_OFFICIAL=1 make

and ensure everything is same as we expected

Comment 62

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

Hi Alex,
You're comparing files in build_stage instead of profile.
After this commit, the html in build_stage should be optimized, and I think it makes sense.

(In reply to Alexandre Poirot (:ochameau) from comment #59)
> Comment on attachment 8442579 [details] [review]
> PR to master 3
> 
> Does this refactoring is expected to modify build system output?
> (I would expect code move/refactoring to not change it!)
Yes , you're right.
> 
> I'm asking that because it does change its output significantly!
> (If you generate a profile without and then with your patch and do "diff -r
> profile_without profile_with", you will see many differences)
> 
> Can you look at those and justify them?
> 
> For example, I can see in html pages:
>   <link rel="prefetch" type="application/l10n"
> href="/locales-obj/{{locale}}.json">
see https://github.com/mozilla-b2g/gaia/blob/master/build/webapp-optimize.js#L447
It also happens in master branch. I'm not quire sure why it's there, so I keep it the same.

> with your patch that I don't get without.
> 
> Also with your patch the html is more strict. All attributes with no value
> now have an empty string.
> So that:
>   <script defer src="/shared/js/l10n.js"></script>
> becomes:
>    <script defer="" src="/shared/js/l10n.js"></script>
see https://github.com/mozilla-b2g/gaia/blob/master/build/webapp-optimize.js#L247
It also happens in master branch.

> 
> I also see some tag like these one to disappear:
>   <link rel="import" href="/elements/errors.html">
see https://github.com/mozilla-b2g/gaia/blob/master/build/webapp-optimize.js#L372
Please also check master.
> 
> Given the differences in profile output you are introducing I don't feel
> confident landing this patch without at least some explanations.

Comment 63

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

something what I found:
1. css content has been embed into index.html in application.zip of homescreen
2. the order of file content has been changed for a lot of files in locale-obj, but looks content is the same.
3. lang="en-US" dir="ltr" added into index.html in application.zip of keyboard:
4. content of net_error.html has been changed.

I squash 3 commit in pull request 20722[1] and compare to previous commit

[1] https://github.com/mozilla-b2g/gaia/pull/20722

Comment 64

[Alexandre Poirot [:ochameau]]

(In reply to George Duan [:gduan] [:喬智] from comment #62)
> (In reply to Alexandre Poirot (:ochameau) from comment #59)
> > Comment on attachment 8442579 [details] [review]
> > PR to master 3
> > 
> > Does this refactoring is expected to modify build system output?
> > (I would expect code move/refactoring to not change it!)
> Yes , you're right.

Oh right, I missed that, I find the exact same diffs Yuren found in comment 63.
It would be worth explaining why we have some diffs.

Comment 65

[Alexandre Poirot [:ochameau]]

Attachment: diff-build.sh

If that can help... here is a bash script to do some diffs on profiles.
Just pass it two revision number and it will display a diff, even for files being zipped:
./diff-build.sh ab6cf9f47e9a4ac7159dd9bd3fb0685d48553535 6bd60af83260095768e703233b9bac8239d22db3

Comment 66

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

(In reply to Yuren [:yurenju] from comment #63)
> something what I found:
> 1. css content has been embed into index.html in application.zip of
> homescreen
It's my fault. I didn't remove  the white list to build/config/optimize_config.json.
New commit has fixed this problem.

> 2. the order of file content has been changed for a lot of files in
> locale-obj, but looks content is the same.
> 3. lang="en-US" dir="ltr" added into index.html in application.zip of
> keyboard:
I didn't resolve previous conflict well. 
New commit has fixed this problem.
> 4. content of net_error.html has been changed.
The time we declare window.SYSTEM_MANIFEST is different, but I think it's ok.
> 
> I squash 3 commit in pull request 20722[1] and compare to previous commit
> 
> [1] https://github.com/mozilla-b2g/gaia/pull/20722

Hi Yuren,

could you check my 4th commit and see if there's any problem?

Comment 67

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

Comment on attachment 8442579 [details] [review]
PR to master 3

for last pr, I only saw some different for profile directory:
1. properties order for files in locale-obj are different but it's fine because we have same properties and content.
2. "<html lang="en-US" dir="ltr">" is added into communications/contacts/test/unit/mock_contacts_index.html but it's fine because we should remove all test files in zip but we only remove test directory from first level of app directory, so we didn't remove communications/contacts/test/.
3. gaia_commit.txt in settings is different and that's what we expected.
4. "window.SYSTEM_MANIFEST="app://system.gaiamobile.org/manifest.webapp";" is added into net_error.html and that is an expected change.

for optimization verification, we have an integration test to catch it by |GAIA_OPTIMIZE=1 DEBUG_BUILD=1 make|

this pr is addressed all issue I mentioned on this bug, so r=yurenju, I also want to get feedback from :gerard-majax to confirm the previous issue wouldn't happend on this pr.

Comment 68

[:gerard-majax]

Comment on attachment 8442579 [details] [review]
PR to master 3

Again, what makes you think that I will be lucky enough again to reproduce the issue I got lucky to reproduce at first, if it's still bogus ?

Comment 69

[:gerard-majax]

I mean, as far as I know, there was no statement of WHY it failed at first and no explanation of why I was the only one seeing it for a couple of days before it explodes the whole tree. Therefore I don't think it's useful that I give it a try and say "it works" if we don't know the real root of this and we can make sure I'm not producing a false negative.

Comment 70

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

:gerard-majax, sorry I didn't notice your comment on comment 58.

as our experiment, all changes in profile directory are what we expected, so we are ready to land this pr. George, please find a good day to land this change when you feel the day is a lucky day.

cross your fingers!

Comment 71

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

Comment on attachment 8442579 [details] [review]
PR to master 3

Hi Alex,
Like comment 67, I think most part of the diff output is expected.

may I have your feedback again?
Thanks.

Comment 72

[Alexandre Poirot [:ochameau]]

Comment on attachment 8442579 [details] [review]
PR to master 3

Please yuren, review more carefully:

* The AST comparator is completely broken in many ways :/
This is a useful feature to test this patch, it is useful to run it against this patch, but given its current state I think you should land it in a distinct bug/revision.

Here is a snippet that works (based on the code Yuren highlighted in comment 42:

  let astA = JSON.stringify(Reflect.parse(content, {loc: 0})); 
  let astB = JSON.stringify(Reflect.parse(minifiedContent, {loc: 0})); 
  if (astA === astB) { 
    utils.writeContent(file, minifiedContent); 
  } else { 
    dump("failed to optimize: "+file.path+"\n"); 
  }

* There is some leftover comments and dead code

* cssmin has no effect on build system output, I don't think there is much value in using it (yet). I don't know what it has no effect, but it is only used for net_error.html. Also we shouldn't introduce such optimization without any proof of significant value.


About refactoring, a great way to proceed is to first refactor, while keeping the exact same features/behavior/output. Then do some followups to enable new stuff. I think you made a good work here by keeping existing code, but I would prefer to keep optimizing all JS file in this revision (drop optimizeFiles for a later path) and optimize CSS (stop using cssmin) for another patch!
Also the same comment about cssmin applies to optimizeFiles. We should come with some proof on how valuable such move is before landing it.

Comment 73

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

Hi Alex, thanks your comment, I will review more carefully next time.

Comment 74

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

Comment on attachment 8442579 [details] [review]
PR to master 3

Hi Alex,
I've addressed issues you mentioned.
1. As comment 51 suggested, we should do compare js and minfied-js in the same bug.
2. I've removed css optimizing thing from this patch.

Could you kindly check again? Thanks.

Comment 75

[Alexandre Poirot [:ochameau]]

(In reply to George Duan [:gduan] [:喬智] from comment #74)
> Comment on attachment 8442579 [details] [review]
> 1. As comment 51 suggested, we should do compare js and minfied-js in the
> same bug.

We wouldn't have to compare js and minified js if we weren't minifying js in this patch.
I whish we could land just the refactoring, without introducing any major new feature in webapp-optimize.
Today, on master, we don't minify JS files individualy, we only minify the aggregated files (gaia_build*.js).
I would prefer landing the individual minification in a followup, with some benchmarks to prove the value of it!
And given that you only compare the individual minifications... we would not land the current AST check you implemented in this patch.

Instead, to address comment 51, you can try to compare the minification of gaia_buid*.js scripts instead.

Also I'm wondering about .travis.yml, are we going to land that, or is this just for testing this patch and going to be removed before landing?

Otherwise, that is my last comment about this patch, I just would like to test a last time the final patch before landing.

Comment 76

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

ok.
1. I would also remove individual js minifying to follow-up bug.
2. ** Try to write test for comparing gaia_build*.js. 
3. .travis.yml is only for test .

Comment 77

[Alexandre Poirot [:ochameau]]

Comment on attachment 8442579 [details] [review]
PR to master 3

Looks good, thanks for all your work on this patch, and the various followups ;)

I verified build system output on regular builds, production=1 and multilocale
and can't see any unexplained/bad difference anymore.
Also tested on device.

I only have one minor comment about tests folder blacklisting over here:
https://github.com/mozilla-b2g/gaia/pull/20722#discussion_r14574301

Comment 78

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

Thanks, everyone.
tbpl is all green
https://tbpl.mozilla.org/?rev=e4ccba5838256367a7b7396f87d1baa938a4a9fa&tree=Gaia-Try

Merge to master
https://github.com/mozilla-b2g/gaia/commit/d944113b6b261011a6f7b00c57881a18648d0c3a