[webkit-reviews] review granted: [Bug 186373] Handle Storage Access API calls in the absence of an attached frame : [Attachment 342102] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 186373: Handle Storage Access API calls in the absence of an attached frame
https://bugs.webkit.org/show_bug.cgi?id=186373

Attachment 342102: Patch

https://bugs.webkit.org/attachment.cgi?id=342102&action=review




--- Comment #3 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 342102
  --> https://bugs.webkit.org/attachment.cgi?id=342102
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=342102&action=review

> LayoutTests/http/tests/storageAccess/has-storage-access-crash-expected.txt:8
> +TEST COMPLETE

Is it possible to write the logic for this test such that this is the last line
that is printed? If so, I suggest we do this for readability and consistency
with the output of other tests.

> LayoutTests/http/tests/storageAccess/has-storage-access-crash-expected.txt:9
> +Test that querying storage access API called on a detached frame doesn't
crash.

This repeats the description at the top.

> LayoutTests/http/tests/storageAccess/has-storage-access-crash-expected.txt:11
> +[object HTMLDocument]

How does a person interpret this output? Is it necessary to emit this? Can we
rewrite the test to emit a PASS/FAIL style message?

> LayoutTests/http/tests/storageAccess/has-storage-access-crash.html:4
> +    <script src="/js-test-resources/js-test.js"></script>

It seems excessive to use js-test for a test that just checks we don’t crash as
we are significantly underutilizing its functionality. It seems sufficient to
have a description in markup (like you have), have a PASS message in markup and
call window.testRunner..dumpAsText(), window.testRunner.waitUntilDone() and
window.testRunner.notifyDone() directly. Obviously if the test crashes then
there will not be a PASS message.

> LayoutTests/http/tests/storageAccess/has-storage-access-crash.html:10
> +	   var o2  = document.getElementById('test');

Extra space characters after o2. Is it necessary to embed the iframe in the
<div>? Would the test work if we inserted the iframe as a child of
document.body? Can we come up with a more descriptive name for this variable?
The quoting style in this line is inconsistent with the quoting style used
throughout this file. This is the only line in this file that uses a single
quoted string literal. I suggest that we use a double-quoted string literal for
consistency. Regardless we should pick one quoting style and stick with it
throughout this document.

> LayoutTests/http/tests/storageAccess/has-storage-access-crash.html:12
> +	   o2.appendChild(testFrame);

Is it necessary to dynamically create an iframe? I mean, could this test have
been written with a declarative iframe?

> LayoutTests/http/tests/storageAccess/has-storage-access-crash.html:14
> +	   testFrame.outerHTML = testFrameDocument;

Is this necessary?

> LayoutTests/http/tests/storageAccess/has-storage-access-crash.html:23
> +	   <p>Test that querying storage access API called on a detached frame
doesn't crash.</p>

Can we please remove this as it repetitive with the description() text?

>
LayoutTests/http/tests/storageAccess/request-storage-access-crash-expected.txt:
8
> +TEST COMPLETE

All of the comments made in has-storage-access-crash-expected.txt apply to this
file as well.

> LayoutTests/http/tests/storageAccess/request-storage-access-crash.html:10
> +	   var o2  = document.getElementById('test');

All of the comments made in has-storage-access-crash-expected.html aply to this
file as well.