[webkit-reviews] review granted: [Bug 43286] Remove setNodeToDump from dump-as-markup.js : [Attachment 63134] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 43286: Remove setNodeToDump from dump-as-markup.js
https://bugs.webkit.org/show_bug.cgi?id=43286

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
Just a few nits.

> +++ LayoutTests/resources/dump-as-markup.js	(working copy)
> -    // If dump is not called by notifyDone, then print out optional
description
> -    // because this test is manually calling dump.
> -    if (!Markup._done || opt_description) {
> +    if (Markup._dumpCalls > 1 || opt_description) {
>	   if (!opt_description)
>	       opt_description = "Dump of markup " + Markup._dumpCalls
>	   if (Markup._dumpCalls > 1)
> -	       markup += '\n';
> +	       markup += '\n'

Missing semi-colon.

>	   markup += '\n' + opt_description + ':\n';
> +    } else {
> +	   Markup._firstCallDidNotHaveDescription = true;
>      }

Single-line else statement should not use brackets.

> +    if (Markup._dumpCalls > 1 && Markup._firstCallDidNotHaveDescription) {
> +	   Markup._container.insertBefore(
> +	       document.createTextNode('\nDump of markup 1:\n'),
> +	       Markup._container.firstChild.nextSibling);

This looks to me like it will only work on tests that have descriptions, no?
How about wrapping the first dump in a span with an ID? Then you can
getElementById instead of relying on the DOM structure. Or you could give each
dump a classname and get do getElementsByClassName(class)[0]. Relying on the
DOM structure is too fragile I think.