Closed
Bug 983573
Opened 10 years ago
Closed 10 years ago
[Gaia] Refactoring webapp-manifest.js
Categories
(Firefox OS Graveyard :: Gaia::Build, defect)
Tracking
(feature-b2g:2.0)
People
(Reporter: gduan, Assigned: gduan)
References
Details
(Whiteboard: [p=5])
Attachments
(1 file)
Refactoring webapp-manifest and write test.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → gduan
Assignee | ||
Comment 1•10 years ago
|
||
WIP, https://github.com/mozilla-b2g/gaia/pull/17315 1. waiting for bug 897352 2. tests
Updated•10 years ago
|
Component: Gaia → Gaia::Build
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
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
Flags: needinfo?(yurenju.mozilla)
Comment 3•10 years ago
|
||
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)
Updated•10 years ago
|
Target Milestone: --- → 1.4 S6 (25apr)
Assignee | ||
Comment 4•10 years ago
|
||
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
Assignee | ||
Comment 5•10 years ago
|
||
waiting for travis.
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 8410080 [details]
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)
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8410080 [details]
PR to master
still have some pbs.
Attachment #8410080 -
Flags: review?(yurenju.mozilla)
Assignee | ||
Comment 8•10 years ago
|
||
Comment on attachment 8410080 [details]
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 9•10 years ago
|
||
Comment on attachment 8410080 [details]
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)
Updated•10 years ago
|
Target Milestone: 1.4 S6 (25apr) → 2.0 S1 (9may)
Assignee | ||
Comment 10•10 years ago
|
||
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)
Assignee | ||
Comment 11•10 years ago
|
||
Comment on attachment 8410080 [details]
PR to master
Hi Alex,
could u help me to check this patch?
Thanks.
Attachment #8410080 -
Flags: review?(poirot.alex)
Assignee | ||
Updated•10 years ago
|
Whiteboard: [p=5]
Comment 12•10 years ago
|
||
Comment on attachment 8410080 [details]
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)
Assignee | ||
Comment 13•10 years ago
|
||
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?
Flags: needinfo?(poirot.alex)
Comment 14•10 years ago
|
||
Comment on attachment 8410080 [details] 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+
Flags: needinfo?(poirot.alex)
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 8410080 [details] 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.
Attachment #8410080 -
Flags: review?(yurenju.mozilla)
Comment 16•10 years ago
|
||
Comment on attachment 8410080 [details]
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+
Assignee | ||
Comment 17•10 years ago
|
||
Thanks Yuren and Alex, merge into master https://github.com/mozilla-b2g/gaia/commit/42b468d6b18d8cd6ea73e23a269b40e22584f4fb
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
feature-b2g: --- → 2.0
You need to log in
before you can comment on or make changes to this bug.
Description
•