552596 - Fix for bug 383009 is not embedding-friendly (breaks changing integer prefs in non-XUL apps)

Status: RESOLVED FIXED

(Toolkit :: Preferences, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

The patch in bug 383009 switched integer pref entry in about:config from using the embedding-friendly Prompt Service to window.opening a random XUL document.

As a result, non-XUL embedders like Camino open a giant XUL window with a text field when people try to modify integer prefs, and values entered in the giant window's text field don't ever get propagated back to the real about:config after clicking the giant window's OK button, which means it's impossible to change integer prefs in about:config.

Can we back out that patch and instead port whatever logic XPFE's about:config used to display a warning alert and not change the pref value (see the end of bug 383009 comment 0), since whatever that did used the Prompt Service and String Bundles, both of which are embedding-friendly?

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

...and ideally in a way that is or could be branch-friendly :-(, since our target right now is 1.9.2.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Why is the XUL window giant, and why doesn't it work? I don't really understand the embedding situation...

On the other hand, I don't really care about bug 383009 either. I would support backing it out entirely and WONTFIXing it.

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #2)
> Why is the XUL window giant

window.open without any size arguments opens a window the size of your last browser window (in my case full-height and about 900px wide).  My guess here is that window.openDialog is just a thin wrapper around window.open that supports some additional arguments, so that what gets sent down to Camino's window-creation code is essentially a window.open call without size.

> and why doesn't it work? I don't really understand the embedding situation...

I don't understand how XUL prompting and event flow work, but my guess is that somehow ending up with a full-fledged native window rather than a child window is breaking the connection between about:config and the new configInt window.

> On the other hand, I don't really care about bug 383009 either. I would support
> backing it out entirely and WONTFIXing it.

I think it's reasonable to try and prevent users from entering bad values where we can; I have a proto-patch I've been using locally that backs out bug 383009 and patches in the old XPFE integer validation code.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Back out bug 383009 and port XPFE code

This is the m-c version of the patch I've been using locally on 1.9.2 (there are some JS MIME type changes in configInt that cause patch to fail to entirely remove that file in the other repo; otherwise the patches are identical).

The patch entirely backs out the code from bug 383009 and then ports the XPFE integer validation code from http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/global/resources/content/config.js&rev=1.31&mark=598-608#583

I'm not sure what configInt was doing that this doesn't do (besides live validation); Mats seemed to have written a lot more code, but JS isn't one of the languages where I even pretend to have any proficiency.

Comment 5

[[e-mail]]

Comment on attachment 434123 [details] [diff] [review]
Back out bug 383009 and port XPFE code

>+        var err_title = gConfigBundle.getString("nan_title");
>+        var err_text = gConfigBundle.getString("nan_text");
Note that these strings are no longer available.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #5)
> (From update of attachment 434123 [details] [diff] [review])
> >+        var err_title = gConfigBundle.getString("nan_title");
> >+        var err_text = gConfigBundle.getString("nan_text");
> Note that these strings are no longer available.

Oh, bleh.  I guess we have them in Camino from our StringBundleOverride stuff.

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: v1.1, port XPFE strings, too

OK, we could do this for the trunk.  Not sure where that leaves us for 1.9.2, though; just back out bug 383009 entirely?

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 435085 [details] [diff] [review]
v1.1, port XPFE strings, too

So, this code has been running around in my tree while I've been putting out other fires for three months :-(

Let's go ahead and look at this patch (back-out the embedding-breaking stuff and replace it by porting the XPFE code+strings) for trunk, then, so I won't have to keep chasing this forever going forward.

(If we unbreak the trunk, then, worst-case scenario, I can probably hack something locally for branch, but I don't want to maintain a fork going forward.)

Neil, you reviewed bug 383009, so tagging you for r? here, too; please let me know if someone else should handle it, though.

Comment 9

[[e-mail]]

Comment on attachment 435085 [details] [diff] [review]
v1.1, port XPFE strings, too

This seems reasonable, as the toolkit dialog was deliberately designed to be near-indistinguishable from the prompt service anyway.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/mozilla-central/rev/b48e3441abcb

Thanks Neil et al!