983573 - [Gaia] Refactoring webapp-manifest.js

Status: RESOLVED FIXED

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

Description

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

Refactoring webapp-manifest and write test.

Comment 1

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

WIP,
https://github.com/mozilla-b2g/gaia/pull/17315
1. waiting for bug 897352 
2. tests

Comment 2

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

Hi Yuren,
could I have your comment on below commit ? If everything looks fine, I'll implement unit/integrate test for it.
Thanks.
https://github.com/cctuan/gaia/commit/820c48f2970363a15dae1af12a1dc11ea9ba6c24

Comment 3

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

discussed offline, this commit fixed weird logic in webapp-manifests.js and separate writing manifest.webapp for each app and writing webapps.json in profile!

and per our discussion, please migrate webapps.json and webapps-mapping.json in build_stage and use an export function to extract webapps.json in profile.

Thank you!

Comment 4

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

Create webapps_stage.json in stage for keyboard and homescreen and generate webapps.json to profile.
WIP:

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

Comment 5

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

Attachment: PR to master

waiting for travis.

Comment 6

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

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

Hi Yuren,
test case is done, and I move the copy-external-app task to copy-build-stage-data.js.

could you help me to review this patch? Thanks.

Comment 7

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

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

still have some pbs.

Comment 8

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

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

Hi Yuren,
could you help me to review this patch?

1. it generate webapp_stage.json under build_stage folder for homescreen and keyboard.js
2. then generate webapps.json to profile from webapp_stage.json
3. move copy external app logic to copy-build-stage-data.js
4. generate settings_stage.json to build_stage folder(settings.js)
5. copy settings_stage.json under build_stage to settings.json under profile
6. modify tasks' dependency and order of makefile

Comment 9

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

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

George, this review cannot be done today, please send review request to Alexandre Poirot (:ochameau) since I will be offline next week.

Comment 10

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

This patch has done below items.
1. generate webapps_stage.json under build_stage folder for keyboard.js and homescreen app
   (before: keyboard.js and homescreen app were using webapps.json from profile folder)
2. generate webapps.json under profile folder from webapps_stage.json
   (before: we generate webapps.json by webapps-manifest.js)
3. move the logic of copying external app to copy-build-stage-data task
   (before: we copy external app in webapp-manifest.js)
4. generate settings_stage.json under build_stage folder, then move to profile folder in copy-build-stage-data task
   (before: we create settings.json to profile directly)

Comment 11

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

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

Hi Alex,
could u help me to check this patch?
Thanks.

Comment 12

[Alexandre Poirot [:ochameau]]

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

Thanks for this refactoring! You are really brave to dig into this code :)

I miss the big picture over copy-build-stage-data goals and next steps.
It doesn't look like the build system is more efficient and do less copy/computing when it is not needed. So I'm wondering what is the goal of this patch in the refactoring plan.

It's not really clear how you define webapps-manifest and copy-build-stage-data.
What is the main purpose of webapps-manifest now (or after some more refactoring)?
What is going to be the goal of copy-build-stage-data (as it looks like it isn't just about manifests)?

For example, I don't know where manifestInterAppHostnames should be called. 
That sounds like something related to manifest, so "webapps-manifest" looks like a good spot for such operation??

I would like to have a bit more context in order to better review this patch.
Otherwise the modifications look good.

Comment 13

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

Comments addressed.

I refactor webapps-manifest for three purposes
1. make webapps-manifest clean and testable
2. webapps-manifest should only manage things about webapps.json, so I move copyExternalApp to copy-build-stage-data.js
3. when refactoring, I found out we can reuse webapps_stage.json for homescreen app and keyboard.js, so that we don't need webapps_mapping.json in profile.
4. as https://bugzilla.mozilla.org/show_bug.cgi?id=995900#c4 , we also need a settings_stage.json for further usage.
5. for the reason 2,3,4, I move those logic in copy-build-stage-data.js. The script is to copy necessary data from build_stage to profile folder.


I agree with you that we should move manifestInterAppHostnames to webapps-manifest. If that so, we should move this task after app-makefiles which will copy each app to stage folder. However, we need webapps_stage.json in keyboard.js and homescreen app before in app-makefiles. That's why I put it in copy-build-stage-data.js. Do you have other idea?

Comment 14

[Alexandre Poirot [:ochameau]]

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

I put various comments, most important ones are about inter app function.
It looks like a hack that has never been addressed.
Feel free to just comment about that and figure out with yuren how to eventually get rid of it as suggested here:
  https://github.com/mozilla-b2g/gaia/pull/18317#discussion_r12277993
Or you may also do something like this:
  https://github.com/mozilla-b2g/gaia/pull/18317#discussion_r12277524
that can help next refactoring steps.

In any case, the patch looks good and I tested it in multiple ways on device
(mostly PRODUCTION=1/0 and reset-gaia)
I also did some `diff -r` on profile generated out of master and out of your branch, and I have seen some fixes!
The inter app communication hack seems to be only working in your patch! Somehow on master findmydevice manifest keeps using app:// urls in connections while building the profile with DEBUG=1.
Also with some external apps like accuweather, on master, we have a buggy and useless folder in profile/webapps/accuweather.gaiamobile.org, with just a manifest.webapp file (without cache folder). We don't have it with your patch!

So LGTM. Thanks again for contributing to this code!

Comment 15

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

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

Hi Yuren,
as Alex suggested, I create a post-manifest task to do what bug 957418 does.
Could you help me to review it again?
Thanks.

Comment 16

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

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

George, looks great! I left some comments on github, please fix the nits before land it :D

thanks George & Alex!

Comment 17

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

Thanks Yuren and Alex,
merge into master
https://github.com/mozilla-b2g/gaia/commit/42b468d6b18d8cd6ea73e23a269b40e22584f4fb