Make Window#open arguments optional to match the spec

Make Window#open arguments optional to match the spec

BUG=701557

[lunalu1, 2017-03-15 20:49:31]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 20:50:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 22:04:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 22:04:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lunalu1, 2017-03-16 15:25:30]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[lunalu1, 2017-03-16 15:25:44]

lunalu@chromium.org changed reviewers:
+ foolip@chromium.org

[lunalu1, 2017-03-16 15:25:45]

PTAL

[commit-bot: I haz the power, 2017-03-16 15:25:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 17:11:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-16 17:11:35]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2755773002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/Window.idl (right):

https://codereview.chromium.org/2755773002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Window.idl:65: // TODO(lunalu): Replace
Window by WindowProxy.
There's already a generic WindowProxy FIXME further up in this file.

https://codereview.chromium.org/2755773002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Window.idl:66: [Custom] Window?
open(optional DOMString url = "about:blank", optional DOMString target =
"_blank", [TreatNullAs=EmptyString] optional DOMString features = "");
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]

Please see comments replied inline.

https://codereview.chromium.org/2755773002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/Window.idl (right):

https://codereview.chromium.org/2755773002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Window.idl:65: // TODO(lunalu): Replace
Window by WindowProxy.
On 2017/03/21 08:09:34, foolip_UTC9 wrote:
> There's already a generic WindowProxy FIXME further up in this file.

My bad. I will just remove this TODO then.

https://codereview.chromium.org/2755773002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Window.idl:66: [Custom] Window?
open(optional DOMString url = "about:blank", optional DOMString target =
"_blank", [TreatNullAs=EmptyString] optional DOMString features = "");
On 2017/03/21 08:09:34, foolip_UTC9 wrote:
> 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?

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

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

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

[lunalu1, 2017-03-21 21:51:58]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 21:52:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 00:30:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 00:30:25]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2755773002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/Window.idl (right):

https://codereview.chromium.org/2755773002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Window.idl:66: [Custom] Window?
open(optional DOMString url = "about:blank", optional DOMString target =
"_blank", [TreatNullAs=EmptyString] optional DOMString features = "");
On 2017/03/21 19:49:45, loonybear wrote:
> On 2017/03/21 08:09:34, foolip_UTC9 wrote:
> > 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?
> 
> 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

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.

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

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]

Please see comments inline.

https://codereview.chromium.org/2755773002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/Window.idl (right):

https://codereview.chromium.org/2755773002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Window.idl:66: [Custom] Window?
open(optional DOMString url = "about:blank", optional DOMString target =
"_blank", [TreatNullAs=EmptyString] optional DOMString features = "");
On 2017/03/22 05:31:20, foolip_UTC9 wrote:
> On 2017/03/21 19:49:45, loonybear wrote:
> > On 2017/03/21 08:09:34, foolip_UTC9 wrote:
> > > 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?
> > 
> > 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
> 
> 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.
> 
> > > 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.
> > 
> > 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
> 
> 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.

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?

https://codereview.chromium.org/2755773002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Window.idl:66: [Custom] Window?
open(optional DOMString url = "about:blank", optional DOMString target =
"_blank", [TreatNullAs=EmptyString] optional DOMString features = "");
On 2017/03/22 05:31:20, foolip_UTC9 wrote:
> On 2017/03/21 19:49:45, loonybear wrote:
> > On 2017/03/21 08:09:34, foolip_UTC9 wrote:
> > > 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?
> > 
> > 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
> 
> 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.
> 
> > > 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.
> > 
> > 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
> 
> 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.

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?

[lunalu1, 2017-03-27 15:30:52]

wpt tests added in: https://github.com/w3c/web-platform-tests/pull/5229

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

https://codereview.chromium.org/2755773002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/Window.idl (right):

https://codereview.chromium.org/2755773002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Window.idl:66: [Custom] Window?
open(optional DOMString url = "about:blank", optional DOMString target =
"_blank", [TreatNullAs=EmptyString] optional DOMString features = "");
On 2017/03/24 19:37:31, loonybear wrote:
> On 2017/03/22 05:31:20, foolip_UTC9 wrote:
> > On 2017/03/21 19:49:45, loonybear wrote:
> > > On 2017/03/21 08:09:34, foolip_UTC9 wrote:
> > > > 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?
> > > 
> > > 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
> > 
> > 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.
> > 
> > > > 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.
> > > 
> > > 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
> > 
> > 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.
> 
> 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?

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

[foolip, 2017-03-30 05:05:48]

Can you do the wpt changes as part of this CL?

[lunalu1, 2017-03-30 18:28:17]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[lunalu1, 2017-03-30 18:28:32]

I added the wpt tests for window.open. PTAL

[commit-bot: I haz the power, 2017-03-30 18:29:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 20:05:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 20:05:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lunalu1, 2017-04-03 14:57:42]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 14:57:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 16:44:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-03 16:44:54]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[foolip, 2017-04-04 03:50:49]

https://codereview.chromium.org/2755773002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/html/browsers/the-window-object/window-open.html
(right):

https://codereview.chromium.org/2755773002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/html/browsers/the-window-object/window-open.html:8:
assert_equals(window.open().location.href, 'about:blank');
Can you also test null and undefined as the first argument?

[lunalu1, 2017-04-04 18:08:20]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[lunalu1, 2017-04-04 18:08:40]

I updated the layout tests.
Please take a second look.

[commit-bot: I haz the power, 2017-04-04 18:09:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 20:10:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 20:10:38]

Dry run: This issue passed the CQ dry run.

[foolip, 2017-04-05 04:33:56]

https://codereview.chromium.org/2755773002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/html/browsers/the-window-object/window-open.html
(right):

https://codereview.chromium.org/2755773002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/html/browsers/the-window-object/window-open.html:10:
assert_equals(window.open('null').location.href, 'about:blank');
Here I meant to test using the literal null, not a string, because I suspect
that will reveal a bug in our custom bindings.

However, it also seems like these tests can't be synchronous, because it seems
like location.href is always 'about:blank' initially and then the window is
navigated to the new url asynchronously. That's the "Otherwise, navigate" at
step 8 of https://html.spec.whatwg.org/multipage/browsers.html#dom-open

So, for the first two of these, I think you'll have to do something like this:

async_test(t => {
  const win = window.open();
  assert_equals(win.location.href, 'about:blank');
  win.onload = t.step_func_done(() => {
    assert_equals(win.location.href, 'about:blank');
  });
});

And for the null case, the inner assert should probably be
assert_true(win.location.href.endsWith('null'));

What I hope to find out is which browsers will fail the window.open(null) case.
It could be that we should change the spec if everyone treats it as
'about:blank' rather than stringifying it to 'null'.