Throw in window.open() when an invalid URL is being passed

Throw in window.open() when an invalid URL is being passed

Intent to ship: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/lQ0Q2WOw8jc/NW2B83TgAgAJ

BUG=708135
R=foolip@chromium.org

Review-Url: https://codereview.chromium.org/2855263004
Cr-Commit-Position: refs/heads/master@{#469626}
Committed: https://chromium.googlesource.com/chromium/src/+/4451675d1f2349898d0a84cd25035296db70c6ea

[jochen (gone - plz use gerrit), 2017-05-04 17:38:11]

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

[commit-bot: I haz the power, 2017-05-04 17:38:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jochen (gone - plz use gerrit), 2017-05-04 19:03:28]

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

[commit-bot: I haz the power, 2017-05-04 19:03:45]

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-05-04 20:54:18]

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

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

Dry run: This issue passed the CQ dry run.

[foolip, 2017-05-05 11:21:29]

LGTM, the testing question I'll leave to you.

https://codereview.chromium.org/2855263004/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/dom/Window/window-open-invalid-url.html
(right):

https://codereview.chromium.org/2855263004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/Window/window-open-invalid-url.html:7:
assert_throws("SyntaxError", () => { self.open("http:"); });
This is the same as https://github.com/w3c/web-platform-tests/pull/5776, is it
OK to wait for that to be imported to avoid duplicate tests?

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

https://codereview.chromium.org/2855263004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Window.idl:66: [Custom, RaisesException]
Window? open(DOMString url, DOMString target, optional DOMString features);
FYI, I wondered if returning null is still possible, and it is: "If
tokenizedFeatures contains an entry with the key "noopener", then disown target
browsing context's opener and return null."

[jochen (gone - plz use gerrit), 2017-05-05 11:25:52]

https://codereview.chromium.org/2855263004/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/dom/Window/window-open-invalid-url.html
(right):

https://codereview.chromium.org/2855263004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/Window/window-open-invalid-url.html:7:
assert_throws("SyntaxError", () => { self.open("http:"); });
On 2017/05/05 at 11:21:29, foolip wrote:
> This is the same as https://github.com/w3c/web-platform-tests/pull/5776, is it
OK to wait for that to be imported to avoid duplicate tests?

I don't know why the wpt/url isn't yet included, and from the imports I've seen
so far, this might or might not be trivial to do :/

So I'd rather go with the duplicate test for now

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

https://codereview.chromium.org/2855263004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Window.idl:66: [Custom, RaisesException]
Window? open(DOMString url, DOMString target, optional DOMString features);
On 2017/05/05 at 11:21:29, foolip wrote:
> FYI, I wondered if returning null is still possible, and it is: "If
tokenizedFeatures contains an entry with the key "noopener", then disown target
browsing context's opener and return null."

correct, that should return null. Also note that the PR against html didn't
change the idl

[jochen (gone - plz use gerrit), 2017-05-05 11:26:51]

Description was changed from

==========
Throw in window.open() when an invalid URL is being passed

BUG=708135
R=foolip@chromium.org
==========

to

==========
Throw in window.open() when an invalid URL is being passed

Intent to ship:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/lQ0Q2WOw8jc/NW...

BUG=708135
R=foolip@chromium.org
==========

[foolip, 2017-05-05 11:28:55]

https://codereview.chromium.org/2855263004/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/dom/Window/window-open-invalid-url.html
(right):

https://codereview.chromium.org/2855263004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/Window/window-open-invalid-url.html:7:
assert_throws("SyntaxError", () => { self.open("http:"); });
On 2017/05/05 11:25:51, jochen wrote:
> On 2017/05/05 at 11:21:29, foolip wrote:
> > This is the same as https://github.com/w3c/web-platform-tests/pull/5776, is
it
> OK to wait for that to be imported to avoid duplicate tests?
> 
> I don't know why the wpt/url isn't yet included, and from the imports I've
seen
> so far, this might or might not be trivial to do :/
> 
> So I'd rather go with the duplicate test for now

Oh, importing that is https://codereview.chromium.org/2855023002/, reviewer is
OOO though. Duplicate test it is.

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

https://codereview.chromium.org/2855263004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Window.idl:66: [Custom, RaisesException]
Window? open(DOMString url, DOMString target, optional DOMString features);
On 2017/05/05 11:25:51, jochen wrote:
> On 2017/05/05 at 11:21:29, foolip wrote:
> > FYI, I wondered if returning null is still possible, and it is: "If
> tokenizedFeatures contains an entry with the key "noopener", then disown
target
> browsing context's opener and return null."
> 
> correct, that should return null. Also note that the PR against html didn't
> change the idl

Yeah, just verifying since sometimes the exception is the only thing missing to
never return null.

[jochen (gone - plz use gerrit), 2017-05-05 11:31:04]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2017-05-05 11:31:27]

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-05-05 12:20:11]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493983864812420,
"parent_rev": "4d49107dce95a5491acebfab01023a377ae03b58", "commit_rev":
"03d0ca7858b7df44481fcde02761ec9004658c4b"}

[commit-bot: I haz the power, 2017-05-05 12:20:29]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493983864812420,
"parent_rev": "70763ca5a8ee376df302e4a411aa1ab46303f69a", "commit_rev":
"4451675d1f2349898d0a84cd25035296db70c6ea"}

[commit-bot: I haz the power, 2017-05-05 12:20:42]

Description was changed from

==========
Throw in window.open() when an invalid URL is being passed

Intent to ship:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/lQ0Q2WOw8jc/NW...

BUG=708135
R=foolip@chromium.org
==========

to

==========
Throw in window.open() when an invalid URL is being passed

Intent to ship:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/lQ0Q2WOw8jc/NW...

BUG=708135
R=foolip@chromium.org

Review-Url: https://codereview.chromium.org/2855263004
Cr-Commit-Position: refs/heads/master@{#469626}
Committed:
https://chromium.googlesource.com/chromium/src/+/4451675d1f2349898d0a84cd2503...
==========

[commit-bot: I haz the power, 2017-05-05 12:20:43]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/4451675d1f2349898d0a84cd2503...