Closed Bug 987487 Opened 7 years ago Closed 7 years ago

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

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.0 S5 (4july)

People

(Reporter: gduan, Assigned: gduan)

References

Details

(Whiteboard: [p=5])

Attachments

(5 files, 1 obsolete file)

Refactoring webapp-optimize.js and write unit tests for it.
Assignee: nobody → gduan
Status: NEW → ASSIGNED
Whiteboard: [p=5]
Target Milestone: --- → 2.0 S2 (23may)
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
Attached file PR to master
Hi Yuren,
could you help me to check this patch's arch?
Attachment #8421535 - Flags: feedback?(yurenju.mozilla)
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.
Attachment #8421535 - Flags: feedback?(yurenju.mozilla) → feedback+
Follow-up - bug 1012464 : move optimize_embedGlobals to other place.
Issues are addressed and unit tests are done.
waiting for travis.
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.
Attachment #8421535 - Flags: review?(yurenju.mozilla)
feature-b2g: --- → 2.0
Travis is all green now.
Comment on attachment 8421535 [details] [review]
PR to master

r=yurenju if issues mentioned on github are fixed.
Attachment #8421535 - Flags: review?(yurenju.mozilla) → review+
thanks Yuren!
Merge to master,
https://github.com/mozilla-b2g/gaia/commit/619530c7cee4b30c4aab751d4e62117783d4e97a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
I had to back this out since it broke a lot of apps: https://github.com/mozilla-b2g/gaia/commit/514cc1e245e38efe2ba3111535b91efa89b79ae9
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 1015188
Depends on: 1015196
Depends on: 1015183
Attached file 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.
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)

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 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?
Attachment #8428508 - Flags: review?(yurenju.mozilla)
(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 on attachment 8428508 [details] [review]
PR to master 2

cancel revuew request since George are working on fixing integration test
Attachment #8428508 - Flags: review?(yurenju.mozilla)
move to 2.1
feature-b2g: 2.0 → 2.1
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.
Attachment #8428508 - Flags: review?(yurenju.mozilla)
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
Attachment #8428508 - Flags: review?(yurenju.mozilla) → review+
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.
Merge into master,
https://github.com/mozilla-b2g/gaia/commit/d5c9689b19d8871bc8e03d959db4959a90041460
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
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.
Dialer's call log is broken for me on Flame with this landed.
Flags: needinfo?(gduan)
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
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.
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.
Flags: needinfo?(gduan) → needinfo?(lissyx+mozillians)
(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.
Flags: needinfo?(lissyx+mozillians)
> 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$
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 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.
Still reproducing after |git clean -fdx| and producing a fresh new profile.
Flags: needinfo?(gduan)
Attached video call-log.mp4
Video of the issue.
(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.
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
Flags: needinfo?(gduan)
(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 ?
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.
Flags: needinfo?(gduan)
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.
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).
Flags: needinfo?(gduan)
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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?
Also, before landing this again, can we look into an automated way to test optimised builds?
Depends on: 1019650
Depends on: 1019639
Depends on: 1019228
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
Please set feedback? to me when the next patch is ready, thanks!
feature-b2g: 2.1 → ---
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)
Attached file PR to master 3
waiting for tbpl first.
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?
Attachment #8442579 - Flags: review?(yurenju.mozilla)
Attachment #8442579 - Flags: feedback?(timdream)
Attachment #8442579 - Flags: feedback?(lissyx+mozillians)
File bug 1027985 for unit test of optimized js file.
since we can't reproduce the issue which :gerard-majax mentioned, I would like to get :gerard-majax's feedback for this pr.
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?
Flags: needinfo?(jsmith)
Edward - Can you help test this patch?
Flags: needinfo?(jsmith) → needinfo?(edchen)
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.
Hi Jason,

When the bug fixed, I will verify it.
Flags: needinfo?(edchen)
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.
Attachment #8442579 - Flags: feedback?(timdream) → feedback+
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
Duplicate of this bug: 1027985
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.
Have you verified that the test added in the third commit is red when running against the two first commits that landed?
Hi Rik,
most of failures randomly happen, so I rerun them.

Hi Lissy,
I've put it in third commit.
Flags: needinfo?(lissyx+mozillians)
(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.
Flags: needinfo?(lissyx+mozillians)
Attachment #8442579 - Flags: feedback?(lissyx+mozillians)
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.
Attachment #8442579 - Flags: feedback-
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
Attachment #8442579 - Flags: review?(yurenju.mozilla)
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.
Flags: needinfo?(poirot.alex)
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
(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.
Flags: needinfo?(poirot.alex)
Attached file 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
Attachment #8446502 - Attachment is obsolete: true
(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?
Flags: needinfo?(yurenju.mozilla)
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.
Attachment #8442579 - Flags: review+
Attachment #8442579 - Flags: feedback?(lissyx+mozillians)
Flags: needinfo?(yurenju.mozilla)
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 ?
Attachment #8442579 - Flags: feedback?(lissyx+mozillians)
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.
: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 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.
Attachment #8442579 - Flags: feedback- → feedback?(poirot.alex)
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.
Attachment #8442579 - Flags: review-
Attachment #8442579 - Flags: review+
Attachment #8442579 - Flags: feedback?(poirot.alex)
Hi Alex, thanks your comment, I will review more carefully next time.
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.
Attachment #8442579 - Flags: review- → review?(poirot.alex)
(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.
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 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
Attachment #8442579 - Flags: review?(poirot.alex) → review+
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
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
No longer blocks: 1050670
You need to log in before you can comment on or make changes to this bug.