Jettison Fennec's PromptService.js

NEW
Unassigned

Status

Fennec Graveyard
General
8 years ago
7 years ago

People

(Reporter: Dolske, Unassigned)

Tracking

Dependency tree / graph

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
Created attachment 452933 [details] [diff] [review]
Patch v.0 (WIP)

There's a lot of duplicated code here, especially since bug 563274 cleaned up the old prompt service implementation in /embedding. It's also kind of fragile and like to get out of sync with the /toolkit code... It would be really good to maximize usage of the /toolkit code, and have Fennec only provide just enough to open the dialog the way it wants (by importing the XUL into the existing window, instead of opening a new one).

Basic work would be to:
 * Modify nsPrompter.js to allow delegating to the application's AppPrompter
 * Implement a getAppPrompter in Fennec's browser.js [this would basically be the existing browser.importDialog() plus dialog.waitForClose(), so quite small.]
 * Fixup commonDialog.xul/js and selectDialog.xul/js to work properly in Fennec's importDialog() environment.

Would be good to have Fennec-side test coverage of this; if you're already able to run Toolkit tests then you pick those up for free here! [Err, well, we'd also need to tweak a few of the tests to know how to deal with Fennec's non-window dialogs.]
(In reply to comment #0)

>  * Fixup commonDialog.xul/js and selectDialog.xul/js to work properly in
> Fennec's importDialog() environment.

We really don't like (or want) dialogs that look like commonDialog.xul. We have dialogs that use dfferent text, different widget for checkbox, different button layout and if the dialog would be larger than the screen (easy on mobile) then the text area becomes pannable while the dialog size is constrained.

Just some bullet points we'd need to work on if we tried to share commonDialog.

The general idea in this bug is good though.

Updated

8 years ago
Blocks: 573635
Another fly in the ointment is electrolysis. Passing around DOM windows and
trying to get the chrome window from it won't work very well. See bug 573635.
No longer blocks: 573635
Depends on: 573635
(Reporter)

Comment 3

8 years ago
Created attachment 454674 [details] [diff] [review]
Patch v.1 (WIP, m-c)
Attachment #452933 - Attachment is obsolete: true
(Reporter)

Comment 4

8 years ago
Created attachment 454676 [details] [diff] [review]
Patch v.1 (WIP, mobile)

So, this is the basic concept. Almost working, except that the default dialog.xml binding doesn't work, because it's constructor adds a load listener, and load events never fire when inserting the <dialog> into an existing window/document.

Not what the next step should be here... Either hack dialog.xml to make it work in Fennec, or wait to see how we handle tab-modal dialogs in m-c and do the same here.
(Reporter)

Comment 5

8 years ago
I suppose an interm (and perhaps final) fix would be to add code to openPrompt() [introduced in this patch] to look at args.getProperty("type"), and glue the existing Fennec prompt UI to to that call.

That gives us the major benefit of not having Fennec override the prompt service, and Fennec completely controls it's own look and feel. The downsides are not sharing the commonDialog.js logic (which isn't exactly trivial), and that we're essentially making the property bag's (aka |args|) contents into a cross-product interface. So not prefect, but it seems _much_ better than the current state of things, and is a cleaner/simpler joint between the two.
(Reporter)

Updated

7 years ago
Depends on: 575485

Updated

7 years ago
Blocks: 627402

Updated

7 years ago
Blocks: 560395
You need to log in before you can comment on or make changes to this bug.