[webkit-reviews] review granted: [Bug 86969] Match Firefox restrictions to window.blur and window.focus : [Attachment 143038] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted jochen at chromium.org's request for
review:
Bug 86969: Match Firefox restrictions to window.blur and window.focus
https://bugs.webkit.org/show_bug.cgi?id=86969

Attachment 143038: Patch
https://bugs.webkit.org/attachment.cgi?id=143038&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=143038&action=review


This looks fine, but please wait a bit before landing this patch in case folks
on webkit-dev have comments.

> LayoutTests/fast/dom/HTMLDocument/hasFocus.html:32
> -		   checkFocus(false, false);
> +		   checkFocus(true, false);

I'd add a comment about how window.blur() doesn't do anything anymore because
of popupunders.  Otherwise, someone reading this test might think this is
pretty odd.

> LayoutTests/fast/dom/Window/mozilla-focus-blur.html:19
> +    var log = document.getElementById('log');
> +    var escaped = message.replace(/</g, '<');
> +    if (isOk)
> +	   log.innerHTML += 'PASS: ' + escaped + '<br>';
> +    else
> +	   log.innerHTML += 'FAIL: ' + escaped + '<br>';

Why not just use document.createTextNode and appendChild() ?

> LayoutTests/fast/dom/Window/mozilla-focus-blur.html:22
> +function focusShouldNotChange(aAction, nextTest) {

Is there a better name we can use than aAction?  That looks like a name Mozilla
would use.

> LayoutTests/fast/dom/Window/mozilla-focus-blur.html:77
> +   
focusShouldNotChange2('data:text/html,\<script>opener.focus();opener.postMessag
e("", "*");\<\/script>', test4);

\<    <-- The \ isn't needed.