Closed Bug 983573 Opened 8 years ago Closed 7 years ago
[Gaia] Refactoring webapp-manifest
Refactoring webapp-manifest and write test.
Assignee: nobody → gduan
Component: Gaia → Gaia::Build
Status: NEW → ASSIGNED
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
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!
Target Milestone: --- → 1.4 S6 (25apr)
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
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.
Comment on attachment 8410080 [details] [review] PR to master still have some pbs.
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 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.
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? Thanks.
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.
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 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!
Attachment #8410080 - Flags: review+
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 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 https://github.com/mozilla-b2g/gaia/commit/42b468d6b18d8cd6ea73e23a269b40e22584f4fb
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.