Bug 43286 – Remove setNodeToDump from dump-as-markup.js

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ryosuke Niwa]

Now that noAutoDump is added, setNodeToDump is redundant.  We should make dump a little smarter not to print out the optional description, and remove setNodeToDump.

[Ryosuke Niwa]

Created attachment 63134 [details]
Patch

[Ojan Vafai]

Comment on attachment 63134 [details]
Patch

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.

[Ryosuke Niwa]

Created attachment 63271 [details]
Fixed per Ojan's comment. Asked him to take a look again

[Ojan Vafai]

Comment on attachment 63271 [details]
Fixed per Ojan's comment. Asked him to take a look again

> +    if (Markup._dumpCalls == 2 && Markup._firstCallDidNotHaveDescription) {
> +        var wrapper = document.getElementById('dump-as-markup-1');
> +        wrapper.insertBefore(document.createTextNode('\nDump of markup 1:\n'), wrapper.firstChild);
> +    }

> -    Markup._container.appendChild(document.createTextNode(markup));
> +    if (Markup._test_description && Markup._dumpCalls == 1)
> +        Markup._container.appendChild(document.createTextNode(Markup._test_description + '\n'))
> +
> +    var wrapper = document.createElement('span');
> +    wrapper.setAttribute('id', 'dump-as-markup-' + Markup._dumpCalls);

Nit: No need to use setAttribute with IDs. Can just do:
wrapper.id = 'dump-as-markup-' + Markup._dumpCalls;

Also, we could use classes instead of IDs. Then you can get the first one by calling document.getElementsByClassName(class)[0].

[Ryosuke Niwa]

Committed r64507: <http://trac.webkit.org/changeset/64507>

[WebKit Review Bot]

http://trac.webkit.org/changeset/64507 might have broken GTK Linux 64-bit Debug
The following changes are on the blame list:
http://trac.webkit.org/changeset/64506
http://trac.webkit.org/changeset/64507