[webkit-reviews] review granted: [Bug 89697] [JSC] CSSStyleDeclaration report incorrect descriptor : [Attachment 208111] Patch

Sun Apr 13 23:18:40 PDT 2014

Benjamin Poulain <benjamin at webkit.org> has granted Manuel Rego Casasnovas
<rego at igalia.com>'s request for review:
Bug 89697: [JSC] CSSStyleDeclaration report incorrect descriptor
https://bugs.webkit.org/show_bug.cgi?id=89697

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=208111&action=review

The patch looks good. Some issue with the test. This will need a rebaseline.

> LayoutTests/ChangeLog:12
> +	   * fast/js/cssstyledeclaration-properties-descriptor-expected.txt:
Added.
> +	   * fast/js/cssstyledeclaration-properties-descriptor.html: Added.
> +	   * fast/js/script-tests/cssstyledeclaration-properties-descriptor.js:
Added.

The test is at the wrong place, it should be in
LayoutTests//fast/dom/CSSStyleDeclaration

> LayoutTests/fast/js/cssstyledeclaration-properties-descriptor.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Please use the HTML5 doctype.

> LayoutTests/fast/js/cssstyledeclaration-properties-descriptor.html:4
> +<script src="resources/js-test-pre.js"></script>

This won't work anymore, the path has changed. :(

> LayoutTests/fast/js/cssstyledeclaration-properties-descriptor.html:7
> +<script
src="script-tests/cssstyledeclaration-properties-descriptor.js"></script>

Please put the test code here instead of using a separate file.

The separate file thingy is the old style of WebKit. It makes it more painful
to work on tests.

>
LayoutTests/fast/js/script-tests/cssstyledeclaration-properties-descriptor.js:3

> +description(
> +"This tests the descriptor of CSSStyleDeclaration properties."
> +);

This should be on a single line.