Closed Bug 1093516 Opened 5 years ago Closed 2 years ago

[System2] Migrate AppUpdate/InstallDialog to SystemDialog

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:backlog)

RESOLVED WONTFIX
tracking-b2g backlog

People

(Reporter: alive, Unassigned)

References

Details

(Whiteboard: [ft:system-platform])

Attachments

(1 file)

No description provided.
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)
Sure.
And looks like Bug 1111975 is a duplicate of this bug.
Flags: needinfo?(evanxd)
Assignee: nobody → evanxd
Duplicate of this bug: 1111975
Whiteboard: [ft:system-platform]
Would you finish this this week? Thanks.
Flags: needinfo?(evanxd)
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. :)
Flags: needinfo?(evanxd)
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.
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)
blocking-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!
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)
Assignee: evan → nobody
Status: ASSIGNED → NEW
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.