277798 - nsIWindowWatcher.openWindow inconsistently applies "dialog" feature

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Embedding: APIs, defect)

Description

[Benjamin Smedberg]

nsIWindowWatcher.openWindow has a strange little piece of code that applies the
"dialog" feature to a window only when there are arguments specified. This is
totally undocumented, and caused me very large headaches when debugging the new
command-line-handling code. We should apply the "dialog" feature at all times,
unless it is specifically overridden.

This is a small change in undocumented semantics to a frozen interface, so I
want to clear it with a decent number of people.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Note bug 160443.  It'd be good to fix it, one of these days... :(

That said, the change proposed here sounds ok to me assuming it's been checked
against "dialog" users in the tree...

Comment 2

[Benjamin Smedberg]

Attachment: make "dialog" the consistent default

Comment 3

[Benjamin Smedberg]

For the record, I did a fairly comprehensive LXR search for "openwindow", and
all the instances I found either specify "dialog=no" or do expect the dialog style.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 170835 [details] [diff] [review]
make "dialog" the consistent default

This needs ok from danm...

Comment 5

[Dan M]

Comment on attachment 170835 [details] [diff] [review]
make "dialog" the consistent default

Benjamin:
-    rv = OpenWindowJS(aParent, aUrl, aName, aFeatures, dialog, argc, argv,
+    rv = OpenWindowJS(aParent, aUrl, aName, aFeatures, PR_TRUE, argc, argv,

Mozilla's basic window opening method is always to use dialog semantics? It
seems to me you're trading an inconsistent surprise for a consistent one. Let's
not forget that people are using this frozen interface. We shouldn't change
lightly details of its implementation that will affect its calling conventions,
whether learned through documentation or through trial and error.

> We should apply the "dialog" feature at all times,
> unless it is specifically overridden.

To me this assertion seems unsupported. The function's name after all is
OpenWindow, not OpenDialog.

-	   rv = wwatch->OpenWindow(this, url.get(), name_ptr, options_ptr,
-				   aExtraArgument, getter_AddRefs(domReturn));

Fumbling aExtraArgument couldn't help but break every caller that uses it.

For my first objection, a furrowed brow. For my second, a big scratch of the
head. For my last, the boot. I didn't get a good feel for the problem being
addressed from its description in this bug, but this approach doesn't seem like
a good one, whatever the problem is.

Boris: Unlike bug 160443, its big brother, bug 195867, is getting attention.

Comment 6

[Benjamin Smedberg]

I'm not trying to change the semantics of the public DOM calls at all. The
problem is in the embedding interface.

Ditching aExtraArgument was a code error, I will correct it.

There is a basic problem when I can write the following two snippets:

ww.openWindow(null, "chrome://messenger/content/", "_blank",
              "chrome,dialog=no,all", cmdLine);

ww.openWindow(null, "chrome://messenger/content/", "_blank",
              "chrome,dialog=no,all", null);

And end up with radically different results. And since an audit of the existing
code assumes that any nsIWindowWatcher.openWindow call is going to produce a
dialog-style window, or explicitly overrides that with dialog=no, I don't think
any existing coders expect the inconsistency here. New patch forthcoming.

Comment 7

[Benjamin Smedberg]

*** Bug 309694 has been marked as a duplicate of this bug. ***

Comment 8

[andreas]

Is there any chance to get this issue fixed for Firefox 1.5.X?
Without having the choice to resize an initial window this makes the usage of Firefox 1.5+ for apps based on XUL pretty much worthless. An application _must_
be resizable in any way. The severity should be changed to "major".

Comment 9

[Carlos Hernandez]

I had the same issue on Fx1.5.0.2

http://forums.mozillazine.org/viewtopic.php?p=2268727

Comment 10

[Adam Rambousek]

(In reply to comment #8)
> Is there any chance to get this issue fixed for Firefox 1.5.X?
> Without having the choice to resize an initial window this makes the usage of
> Firefox 1.5+ for apps based on XUL pretty much worthless. An application _must_
> be resizable in any way. The severity should be changed to "major".
> 

I agree with this. The current behaviour make the applications unusable.

Comment 11

[Ilya]

Hi guys.
Look like this bug inverted in FireFox 1.5.0.4 (and 2.0 beta 1):
This command:
  var win = ww.openWindow( window, url, "_blank", "toolbar=yes,location=yes", null );

Will open dialog with toolbar and location field.

This one:
  var win = ww.openWindow( window, url, "_blank", "toolbar=yes,location=yes", argstring );

Will open dialog without toolbar and location field.
Alsow for first one all extensions will be initialized, for second one not.
So, I guess, the problem is dipper than wrong parameters passing.

Comment 13

[Benjamin Smedberg]

Marking a bunch of bugs in the "Embedding: APIs" component INCOMPLETE in preparation to archive that component. If I have done this incorrectly, please reopen the bugs and move them to a more correct component as we don't have "embedding" APIs any more.