[webkit-reviews] review granted: [Bug 70784] JS Test Harness: Make successfullyParsed optional : [Attachment 112284] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Erik Arvidsson <arv at chromium.org>'s
request for review:
Bug 70784: JS Test Harness: Make successfullyParsed optional
https://bugs.webkit.org/show_bug.cgi?id=70784

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=112284&action=review


This is awesome!

> LayoutTests/fast/js/kde/garbage-n.html:13
> +shouldBeTrue("hadSyntaxError");
> +hadSyntaxError = false;

As discussed in person, js-test-pre should just have a function like the
following that is called instead so that hadSyntaxError can be private and so
that people don't have to understand the guts of js-test-pre as much.

function shouldHaveHadSyntaxError()
{
    shouldBeTrue("hadSyntaxError");
    hadSyntaxError = false;
}

I'm fine with having a FIXME in js-test-pre and doing it in a followup cleanup
patch.

> LayoutTests/fast/js/resources/js-test-post-async.js:2
> +  successfullyParsed = true;

needs 4 space indent

> LayoutTests/fast/js/resources/js-test-pre.js:393
> +    // FIXME: Remove

A little more detail would be nice. "Remove once ...".

> LayoutTests/fast/multicol/column-span-parent-continuation-crash.html:13
> +	   layoutTestController.waitUntilDone();

this is indented too far now.

> LayoutTests/java/lc3/Constructors/construct-001-expected.txt:5
>  
> -FAIL successfullyParsed should be true. Threw exception ReferenceError:
Can't find variable: successfullyParsed
> +PASS successfullyParsed is true
>  

Can you add to the ChangeLog description why this new result is correct?