Closed Bug 1093516 Opened 5 years ago Closed 2 years ago
[System2] Migrate App
Update/Install Dialog to System Dialog
46 bytes, text/x-github-pull-request
|Details | Review|
No description provided.
Backlog Triage: Move it become system dialog is valuable because we could manage the UI by system dialog manager.
Are you still on this? If yes, please assign it to yourself.
Sure. And looks like Bug 1111975 is a duplicate of this bug.
Would you finish this this week? Thanks.
Status: NEW → ASSIGNED
Target Milestone: --- → 2.2 S4 (23jan)
Hi Tim, We will have a patch for review(feedback) this week. Let's do our best to land it next week. :)
WIP patch, and working on debugging issues of using system dialog module.
Fixed the issue listed in Comment 7. Continue to refactor `imeLayoutDialog`, `downloadCancelDialog`, `setupInstalledAppDialog` dialogs. Then remove the global dependencies with using `Service.request()`.
Finished `downloadCancelDialog` and `setupInstalledAppDialog` dialogs.
Finished the `imeLayoutDialog` dialog.
Comment on attachment 8550181 [details] [review] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27456 Hi Alive, Could you give feedbacks for the patch? Currently, I created `AppInstallManager` module with `BaseModule`, and I used system dialog module for `AppInstallDialog`, `AppInstallCancelDialog`, `AppDownloadCancelDialog`, `SetupInstalledAppDialog`, and `ImeLayoutDialog` dialogs. That is our phase one for the refactor work. In the phase two, I will use `Service.request()` to remove the global object in the top of `app_install_manager.js`. Thanks.
Comment on attachment 8550181 [details] [review] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27456 Okay, I did a rough review around. Basically fine but the biggest problem is the module responsibility is vague. Notes: * The manager should not know detail of each dialogs, e.g. dom elements * The _start function of AIM does too many things - split it using BaseModule.EVENTS * Think this: which event should be handled in the manager and which event should be handled in the dialog? * Lazy load all dialogs on demand. One proposal is: 1. AppInstallManager only loads/starts AppInstallDialog when getting mozChromeEvent with evt.detail.type=applicationinstall 2. AppInstallDialog will loads/starts AppInstallCancelDialog if cancel button is pressed 3. AppInstallManager only loads/starts AppDownloadCancelDialog if the user click cancel somewhere 4. AppInstallManager only loads/starts ImeLayoutDialog once a new input app is installed
Evan, do you mind if I steal this bug? I have bandwidth to work on it now. Thanks!
Really sorry, Alberto, I'm working this. There is also a patch on review process. No update for a long time is because I was working on Bug 1100822. Sorry for that. How do you think if you help other refactor works? : https://wiki.mozilla.org/User:Alive/System_Refactor_Plan
Yeah, sounds good! Thanks!
Hi Alive, After I update the patch for the comments, could you help to review the patch? Thanks.
(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #16) > Hi Alive, > > After I update the patch for the comments, could you help to review the > patch? > > Thanks. Yes, please.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.