[webkit-reviews] review requested: [Bug 38424] add support for text/html-sandboxed on sandboxed iframes : [Attachment 65708] Patch with more tests

[bugzilla-daemon at webkit.org]

Patrik Persson <patrik.j.persson at ericsson.com> has asked  for review:
Bug 38424: add support for text/html-sandboxed on sandboxed iframes
https://bugs.webkit.org/show_bug.cgi?id=38424

Attachment 65708: Patch with more tests
https://bugs.webkit.org/attachment.cgi?id=65708&action=review

------- Additional Comments from Patrik Persson <patrik.j.persson at ericsson.com>
(In reply to comment #17)

Thanks for the fast reply. We didn't mean to intrude on your night sleep
(judging by the time of your message)...

> Can you add a test for loading a text/html-sandboxed document inside a plain
iframe in a sandboxed iframe?

Added
(LayoutTests/http/tests/security/sandboxed-iframe-inherited-html-sandboxed.html
)

> Do we still need to call closeRemainingWindowsWhenDone to clean up properly? 
(I might have gotten the name wrong.)

Added call to setCloseRemainingWindowsWhenComplete() in
html-sandboxed-mainwindow.html.

> nbr => generally we don't like these sorts of abbreviations.

Changed to numberOfTrustedWindows.

> LayoutTests/http/tests/security/html-sandboxed-mainwindow.html:31
>  +	  for (var i = 0; i < 5; i++) {
> Why do we open so many windows?  That seems like of silly.

I added a comment in the test case to explain our reasoning. Essentially, we're
trying to determine whether an asynchronous, unbounded event (rendering a
loaded document in a new window) was prevented. So, how long do we wait? We
came up with this (admittedly silly) solution of a set of very similar, but
allowed, windows. If all those allowed windows were loaded, and the disallowed
one wasn't, we should be fine.

Yes, silly indeed. We'd love to figure out a more direct way of testing this.