Closed Bug 983573 Opened 8 years ago Closed 7 years ago

[Gaia] Refactoring webapp-manifest.js


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

Not set



2.0 S1 (9may)
feature-b2g 2.0


(Reporter: gduan, Assigned: gduan)



(Whiteboard: [p=5])


(1 file)

Refactoring webapp-manifest and write test.
Assignee: nobody → gduan
Component: Gaia → Gaia::Build
Hi Yuren,
could I have your comment on below commit ? If everything looks fine, I'll implement unit/integrate test for it.
Flags: needinfo?(yurenju.mozilla)
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!
Flags: needinfo?(yurenju.mozilla)
Target Milestone: --- → 1.4 S6 (25apr)
Create webapps_stage.json in stage for keyboard and homescreen and generate webapps.json to profile.
Attached file PR to master
waiting for travis.
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.
Attachment #8410080 - Flags: review?(yurenju.mozilla)
Comment on attachment 8410080 [details] [review]
PR to master

still have some pbs.
Attachment #8410080 - Flags: review?(yurenju.mozilla)
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
Attachment #8410080 - Flags: review?(yurenju.mozilla)
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.
Attachment #8410080 - Flags: review?(yurenju.mozilla)
Target Milestone: 1.4 S6 (25apr) → 2.0 S1 (9may)
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 on attachment 8410080 [details] [review]
PR to master

Hi Alex,
could u help me to check this patch?
Attachment #8410080 - Flags: review?(poirot.alex)
Whiteboard: [p=5]
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.
Attachment #8410080 - Flags: review?(poirot.alex)
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 , 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?
Flags: needinfo?(poirot.alex)
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:
Or you may also do something like this:
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/, 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!
Attachment #8410080 - Flags: review+
Flags: needinfo?(poirot.alex)
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?
Attachment #8410080 - Flags: review?(yurenju.mozilla)
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!
Attachment #8410080 - Flags: review?(yurenju.mozilla) → review+
Thanks Yuren and Alex,
merge into master
Closed: 7 years ago
Resolution: --- → FIXED
feature-b2g: --- → 2.0
You need to log in before you can comment on or make changes to this bug.