Make Window#open arguments optional to match the spec

third_party/WebKit/Source/core/frame/Window.idl

Index: third_party/WebKit/Source/core/frame/Window.idl
diff --git a/third_party/WebKit/Source/core/frame/Window.idl b/third_party/WebKit/Source/core/frame/Window.idl
index c86c8b21790ac6626c5f682a9df2207544a54917..bc3cacdffe45b6217cc4805b4d7fb70d812a72e9 100644
--- a/third_party/WebKit/Source/core/frame/Window.idl
+++ b/third_party/WebKit/Source/core/frame/Window.idl
@@ -62,8 +62,8 @@
     [CrossOrigin, Custom=Setter] attribute Window opener;
     [Replaceable, CrossOrigin] readonly attribute Window? parent;
     [CheckSecurity=ReturnValue, Custom=Getter] readonly attribute Element? frameElement;
-    // FIXME: open() should have 4 optional arguments with defaults.
-    [Custom] Window? open(DOMString url, DOMString target, optional DOMString features);
+    // TODO(lunalu): Replace Window by WindowProxy.

[foolip, 2017/03/21 08:09:34]

There's already a generic WindowProxy FIXME further up in this file.

[lunalu1, 2017/03/21 19:49:45]

My bad. I will just remove this TODO then.

+    [Custom] Window? open(optional DOMString url = "about:blank", optional DOMString target = "_blank", [TreatNullAs=EmptyString] optional DOMString features = "");

[foolip, 2017/03/21 08:09:34]

Because of the [Custom] extended attribute, this won't have any effect on the
behavior, but will change window.open.length, can you add a test for that? If
there's already an idlharness.js test in web-platform-tests that includes this
bit of IDL but it still doesn't result in a failing test, then I think we just
need to teach idlharness.js about asserting the length of methods to get more
coverage like this. Can you check?

Also, the custom bindings notably use for the 1st and 3rd argument, which has
the equivalent effect as adding [TreatNullAs=EmptyString] to the first argument.
That would affect what open(null) and open(undefined) does, can you test that a
bit? It could be that we need to change the spec here as well if many
implementations behave like Blink seems to.

[lunalu1, 2017/03/21 19:49:45]

There isn't a test for window.open.length. But I am thinking about adding it to
https://github.com/w3c/web-platform-tests/tree/8c1d0274b5c7fb648a54019adceae8...
(Although I think it is a little bit silly to have a test that just checks the
length?)

There is a test in Blink fast/js/function-length.idl that tests
window.open.length
Do you mean to change it to be 
[TreatNullAs=EmptyString] optional DOMString url = "", [TreatNullAs=EmptyString]
optional DOMString target = ""
?

I will check with Browserstack how other vendors behave and write tests about
this, in some wpt directory

[foolip, 2017/03/22 05:31:20]

It is a bit silly, yes, and something I think idlharness.js should eventually
cover. I've confirmed that the other 3 engines already have open.length==0, so
there wouldn't be much practical benefit in testing in in web-platform-tests.
Just the function-length.html change will be enough for this CL I think.
I missed that there's some null/undefined handling for the target argument as
well in V8Window::openMethodCustom. I don't think there's any existing Web IDL
syntax that would capture the current behavior, AFAICT our behavior is per this
made-up IDL:

Window? open([TreatNullAs="about:blank"] optional DOMString url = "about:blank",
[TreatNullAs="_blank"] optional DOMString target = "_blank",
[TreatNullAs=EmptyString] optional DOMString features = "");

For the first two arguments, I think you can test the behavior by looking at the
returned window's location.href and name attributes. For the features argument,
I don't know, but I think that already effectively has the per-spec behavior.

[lunalu1, 2017/03/24 19:37:31]

Very interesting found: Chrome currently already behaves as if "" or null or
nothing
is passed to window.open(), location.href will show "about:blank" and name will
show
"".
I checked with FireFox and Safari, they all share the same behaviour. Edge has
some
weird behaviour. 

Should I write this test in wpt?

[lunalu1, 2017/03/24 19:37:31]

I tried to test for window.open().name. It doesn't seem like the name property
is 
being saved even though it does opens a new window in "_blank" tab. I can't
think of 
an easy way to justify it. Only thing I can think of is to manually test it.

So should we only test href for now?

[foolip, 2017/03/30 05:04:50]

This is complicated a bit by
https://html.spec.whatwg.org/multipage/browsers.html#the-rules-for-choosing-a...,
the target argument isn't copied directly to the name of the opened window. For
example, it says "If the given browsing context name is not _blank, then the new
auxiliary browsing context's name must be the given browsing context name
(otherwise, it has no name)"

So the test would be that open(url, undefined).name==="", but that
open(url,null).name==="null".

     // FIXME: These getters should not have [NotEnumerable].
     [NotEnumerable, CrossOrigin] getter Window (unsigned long index);
     [Custom, NotEnumerable, CrossOrigin] getter object (DOMString name);