1093516 - [System2] Migrate AppUpdate/InstallDialog to SystemDialog

Status: RESOLVED WONTFIX

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

Comment 1

[(inactive after 6/18) Alive Kuo [:alive]]

Backlog Triage: Move it become system dialog is valuable because we could manage the UI by system dialog manager.

Comment 2

[(inactive after 6/18) Alive Kuo [:alive]]

Are you still on this? If yes, please assign it to yourself.

Comment 3

[Evan Tseng [:evanxd]]

Sure.
And looks like Bug 1111975 is a duplicate of this bug.

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Would you finish this this week? Thanks.

Comment 6

[Evan Tseng [:evanxd]]

Hi Tim,

We will have a patch for review(feedback) this week. Let's do our best to land it next week. :)

Comment 7

[Evan Tseng [:evanxd]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27456

WIP patch, and working on debugging issues of using system dialog module.

Comment 8

[Evan Tseng [:evanxd]]

Fixed the issue listed in Comment 7.

Continue to refactor `imeLayoutDialog`, `downloadCancelDialog`, `setupInstalledAppDialog` dialogs. Then remove the global dependencies with using `Service.request()`.

Comment 9

[Evan Tseng [:evanxd]]

Finished `downloadCancelDialog` and `setupInstalledAppDialog` dialogs.

Comment 10

[Evan Tseng [:evanxd]]

Finished the `imeLayoutDialog` dialog.

Comment 11

[Evan Tseng [:evanxd]]

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 12

[(inactive after 6/18) Alive Kuo [:alive]]

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

Comment 13

[Alberto Pastor [:albertopq]]

Evan, do you mind if I steal this bug? I have bandwidth to work on it now. Thanks!

Comment 14

[Evan Tseng [:evanxd]]

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[1]?

[1]: https://wiki.mozilla.org/User:Alive/System_Refactor_Plan

Comment 15

[Alberto Pastor [:albertopq]]

Yeah, sounds good! Thanks!

Comment 16

[Evan Tseng [:evanxd]]

Hi Alive,

After I update the patch for the comments, could you help to review the patch?

Thanks.

Comment 17

[(inactive after 6/18) Alive Kuo [:alive]]

(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.