Closed Bug 573130 Opened 9 years ago Closed 9 years ago

Override nsIPromptFactory, nsIPrompt, and nsIAuthPrompt(2?) for Fennec

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

Details

Attachments

(1 file, 5 obsolete files)

Recent Android builds do not use Fennec's PromptService, but instead use normal XUL prompts.  This includes javascript alerts, the XPI web-install-prompt, etc.

Possibly related to bug 563274.  On IRC, dolske suggests we may need to implement nsIPromptFactory and override @mozilla.org/prompter;1.
Blocks: 564097
Attached patch patch (obsolete) — Splinter Review
This patch adds an override of @mozilla.org/prompter;1 (with a non-functional stub implementation of nsIPromptFactory).  The patch also fixes a regression caused by bug 564097.

This doesn't fix the problem on Android.  I think that problem is probably caused by something else.  It happens even in code like http://mxr.mozilla.org/mobile-browser/source/components/XPIDialogService.js#67 which uses the old "prompt-service" contractID (which is correctly registered to Fennec's PromptService.js classID in compreg.dat).  And it does not happen in my desktop Linux build.  Still trying to figure it out.
Attachment #452372 - Flags: review?(mark.finkle)
Attachment #452372 - Flags: feedback?(dolske)
Comment on attachment 452372 [details] [diff] [review]
patch

>diff -r 2ec6c905f325 components/PromptService.js

> function promptService() {

I don't know why we didn't catch this a long time ago, but:

promptService -> PromptService
 
> promptService.prototype = {

Same...

>   classDescription: "Mobile Prompt Service",
>-  contractID: "@mozilla.org/embedcomp/prompt-service;1",
>+  contractID: "@mozilla.org/prompter;1",
>   classID: Components.ID("{9a61149b-2276-4a0a-b79c-be994ad106cf}"),
>   
>-  QueryInterface: XPCOMUtils.generateQI([Ci.nsIPromptService, Ci.nsIPromptService2]),
>- 
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIPromptService, Ci.nsIPromptService2, nsIPromptFactory]),
>+
>+  getPrompt: function (domWin, iid) {

aDOMWindow and aIID

>+    throw "Not implemented";

I guess we'll find out how badly we need to implement this

>   _getFallbackService: function() {
>-    return Components.classesByID["{a2112d6a-0e28-421f-b46a-25c0B308cbd0}"]
>+    return Components.classesByID["{1c978d25-b37f-43a8-a2d6-0c7a239ead87}"]
>                      .getService(Ci.nsIPromptService);

For the record, we use the fallback when we need to display a prompt, but the browser window is not visible. The profile manager (fennec -p) is an example.

>+function EmbedPrompter() {}
>+EmbedPrompter.prototype = new promptService();

PromptService

>-  return XPCOMUtils.generateModule([promptService]);
>+  return XPCOMUtils.generateModule([promptService, EmbedPrompter]);

Same...

So, what we have here is a patch that fixes some bugs, updates the fallback to m-c trunk and implements the snake's nest of a prompt system a little better. YAY!

But still doesn't quite fix the Android problem. CRAP!
Attachment #452372 - Flags: review?(mark.finkle) → review+
Duplicate of this bug: 573228
OS: Android → Linux
Hardware: ARM → All
Comment on attachment 452372 [details] [diff] [review]
patch

>diff -r 2ec6c905f325 components/PromptService.js

> promptService.prototype = {

>+  getPrompt: function (domWin, iid) {
>+    throw "Not implemented";

I would expect this to break all the prompting that goes through the various Get*Prompt* methods in nsWindowWatcher.cpp. One example: the dialog produced by a call to:

window.sidebar.addSearchEngine(null, null, null, null);

in content (can use a javascript: URI to test that easily).

I think you need to actually implement nsIPrompt/nsIAuthPrompt/nsIAuthPrompt2 (basically just by moving the existing code you have for nsIPromptService), delegate to that in your existing nsIPromptService[2] implementation, and then return it from this method. In other words, do everything that nsPrompter.js is doing, but with a different "ModalPrompter" implementation that uses your existing code.

I guess ideally we'd make it easier to just override the base prompt implementation (nsIPrompt etc.) without having to override all the other glue (nsIPromptService/nsIPromptFactory/etc.). I think that'd be pretty doable if we added an init(aWindow) to nsIPrompt and then retrieved its implementation via XPCOM in the nsPrompter.js code.
Attached patch patch v2 (obsolete) — Splinter Review
This contains a working implementation of nsIPromptFactory, which proxies to an implementation of nsIPrompt/nsIAuthPrompt/nsIAuthPrompt2.

There's some duplicated code marked with XXX.  Hopefully we can share more code with nsPrompter.js in the future, as suggested by dolske and gavin.
Attachment #452372 - Attachment is obsolete: true
Attachment #452867 - Flags: review?(mark.finkle)
Attachment #452867 - Flags: review?(dolske)
Attachment #452372 - Flags: feedback?(dolske)
Attached patch patch v3 (obsolete) — Splinter Review
Less magic (more explicit) proxy from PromptService to Prompt.
Attachment #452867 - Attachment is obsolete: true
Attachment #452886 - Flags: review?(mark.finkle)
Attachment #452886 - Flags: review?(dolske)
Attachment #452867 - Flags: review?(mark.finkle)
Attachment #452867 - Flags: review?(dolske)
Attached patch patch v4 (obsolete) — Splinter Review
Oops, some comments were in the wrong spots.  Move everything into the right section by interface.
Attachment #452886 - Attachment is obsolete: true
Attachment #452890 - Flags: review?(mark.finkle)
Attachment #452890 - Flags: review?(dolske)
Attachment #452886 - Flags: review?(mark.finkle)
Attachment #452886 - Flags: review?(dolske)
The Android-specific issue turned out to be unrelated to bug 563274 or nsIPromptFactory; see bug 573612 for details.  This bug is now about the new interfaces only; summary changed.
Status: NEW → ASSIGNED
OS: Linux → All
Summary: Fennec on Android using default (non-mobile) prompt-service → Override nsIPromptFactory, nsIPrompt, and nsIAuthPrompt(2?) for Fennec
See bug 573649 for an alternate approach that makes the nsPrompter.js code reusable in Fennec.
Comment on attachment 452890 [details] [diff] [review]
patch v4

This is mostly a rs=me, this looks like the proper way to get things working again (without more radical changes to PromptService.js) and nothing looks patently incorrect, but I didn't review the details in depth.

>+  alert: function() {
>+    this.callProxy("alert", arguments);
>+  },

Mmm, I should probably steal this pattern for toolkit. :)

>+XPCOMUtils.defineLazyGetter(Prompt, "bundle", function () {
>+  let bundleService = Cc["@mozilla.org/intl/stringbundle;1"].getService(Ci.nsIStringBundleService);
>+  return bundleService.createBundle("chrome://global/locale/commonDialogs.properties");
>+});

Hrm, does this work? I thought defineLazyGetter only worked for static objects (var ps = { func1: ... func2: ... }), and not when everything's put in the prototype. Though I guess you're using Prompt and not Prompt.prototype here, so maybe it all works for something doing |new Prompt()|.
Attachment #452890 - Flags: review?(dolske) → review+
(In reply to comment #10)
> >+XPCOMUtils.defineLazyGetter(Prompt, "bundle", function () {
> 
> Hrm, does this work? I thought defineLazyGetter only worked for static objects
> (var ps = { func1: ... func2: ... }), and not when everything's put in the
> prototype. Though I guess you're using Prompt and not Prompt.prototype here, so
> maybe it all works for something doing |new Prompt()|.

Yeah, I am ignoring the prototype here and adding the getter directly to the Prompt() object.  Then I refer to it as "Prompt.bundle" (not "this.bundle").
Ah, yeah, that's why it's working. Seems like an undesirable pattern, though, because it's easy to forget and use "this.bundle" and be surprised by it not working.
Blocks: 573635
Comment on attachment 452890 [details] [diff] [review]
patch v4

>diff -r 2ec6c905f325 components/PromptService.js

>+  callProxy: function(aMethod, aArguments) {
>+    let doc = this.getDocument();
>+    if (!doc) {
>+      let fallback = this._getFallbackService();
>+      return fallback[aMethod].apply(fallback, aArguments);
>+    }
>+    let domWin = arguments[0];

You want: let domWin = aArguments[0];

>+XPCOMUtils.defineLazyGetter(Prompt, "bundle", function () {
>+  let bundleService = Cc["@mozilla.org/intl/stringbundle;1"].getService(Ci.nsIStringBundleService);
>+  return bundleService.createBundle("chrome://global/locale/commonDialogs.properties");
>+});

You could just make this a global so it wouldn't confuse people

r+, but needs the domWin change before checkin
Attachment #452890 - Flags: review?(mark.finkle) → review+
Attached patch patch v5 (obsolete) — Splinter Review
Updates:
- fix arguments->aArguments
- fix return value for prompt(), confirm()
- move "bundle" and related methods to a separate PromptUtils object like in toolkit's nsPrompter.js.
Attachment #452890 - Attachment is obsolete: true
Attachment #454112 - Flags: review?(mark.finkle)
Attached patch patch v6Splinter Review
Fix getPrompt to get getDocument.
Attachment #454112 - Attachment is obsolete: true
Attachment #454114 - Flags: review?(mark.finkle)
Attachment #454112 - Flags: review?(mark.finkle)
Attachment #454114 - Flags: review?(mark.finkle) → review+
pushed:
http://hg.mozilla.org/mobile-browser/rev/1dfc26d1be89
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.