573649 - Jettison Fennec's PromptService.js

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: 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.]

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 3

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 (WIP, m-c)

Comment 4

[Justin Dolske [:Dolske]]

Attachment: 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.

Comment 5

[Justin Dolske [:Dolske]]

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.

Comment 6

[BMO Automation]

Closing all opened bug in a graveyard component