[System2] Migrate AppUpdate/InstallDialog to SystemDialog

RESOLVED WONTFIX

Status

Firefox OS
Gaia::System
RESOLVED WONTFIX
4 years ago
5 months ago

People

(Reporter: alive, Unassigned)

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(tracking-b2g:backlog)

Details

(Whiteboard: [ft:system-platform])

Attachments

(1 attachment)

Comment hidden (empty)
blocking-b2g: --- → backlog
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.
Flags: needinfo?(evanxd)

Comment 3

4 years ago
Sure.
And looks like Bug 1111975 is a duplicate of this bug.
Flags: needinfo?(evanxd)

Updated

4 years ago
Assignee: nobody → evanxd

Updated

4 years ago
Duplicate of this bug: 1111975

Updated

4 years ago
Whiteboard: [ft:system-platform]
Would you finish this this week? Thanks.
Flags: needinfo?(evanxd)
Status: NEW → ASSIGNED
Target Milestone: --- → 2.2 S4 (23jan)

Comment 6

4 years ago
Hi Tim,

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

Comment 7

4 years ago
Created attachment 8550181 [details] [review]
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

4 years ago
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

4 years ago
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.
Attachment #8550181 - Flags: feedback?(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
Attachment #8550181 - Flags: feedback?(alive)
(Assignee)

Updated

3 years ago
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
Evan, do you mind if I steal this bug? I have bandwidth to work on it now. Thanks!
Flags: needinfo?(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
Flags: needinfo?(evanxd)
Yeah, sounds good! Thanks!

Updated

3 years ago
Target Milestone: 2.2 S4 (23jan) → ---
Hi Alive,

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

Thanks.
Flags: needinfo?(alegnadise+moz)
(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.
Flags: needinfo?(alegnadise+moz)

Updated

2 years ago
Assignee: evan → nobody
Status: ASSIGNED → NEW

Updated

5 months ago
Status: NEW → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.