[webkit-reviews] review granted: [Bug 32596] Refactor Window object tests to use a shared list of property names : [Attachment 44945] Updated patch (fix spelling error in ChangeLog)

Wed Dec 16 09:38:11 PST 2009

Darin Adler <darin at apple.com> has granted Kent Hansen <kent.hansen at nokia.com>'s
request for review:
Bug 32596: Refactor Window object tests to use a shared list of property names
https://bugs.webkit.org/show_bug.cgi?id=32596

Attachment 44945: Updated patch (fix spelling error in ChangeLog)
https://bugs.webkit.org/attachment.cgi?id=44945&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
What this removes is any test that checks that no extra properties on the
Window object are present. I would like to see us add one test for that. It's
good not to have multiple tests for it, but I think it's good to have one to
help remind people to keep the list of window properties up to date. That test
can have whatever platform exceptions we need coded into it.

> -FAIL inner.isInner.isInner should be true. Was false.
> -FAIL inner.isInner.constructor.isInner should be true. Was false.

Is it a good thing to skip testing this? Should we add something back so this
case is tested?

> +staticWindowProperties = [

I think this should say "var staticWindowProperties" or "const
staticWindowProperties" for better efficiency.

> diff --git a/LayoutTests/fast/js/global-constructors.html
b/LayoutTests/fast/js/global-constructors.html
> index d08d6f5..684a9ed 100644
> --- a/LayoutTests/fast/js/global-constructors.html
> +++ b/LayoutTests/fast/js/global-constructors.html
> @@ -7,6 +7,7 @@
>  <body>
>  <p id="description"></p>
>  <div id="console"></div>
> +<script src="../dom/resources/static-window-properties.js"></script>
>  <script src="script-tests/global-constructors.js"></script>
>  <script src="resources/js-test-post.js"></script>
>  </body>

Changing the test in this fashion means it should move into the fast/dom
directory. The old test could run in a non-browser context, so it made sense to
have it in fast/js, although we'd need different test results for that case.

I'm going to say r=me on this as-is, but ideally I'd like to see some
refinement and followup in future patches.