[webkit-reviews] review denied: [Bug 42767] [Windows] Home hey doesn't work in first DIV inside a TABLE : [Attachment 62320] fixed per Ojan's comment

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 42767: [Windows] Home hey doesn't work in first DIV inside a TABLE
https://bugs.webkit.org/show_bug.cgi?id=42767

Attachment 62320: fixed per Ojan's comment
https://bugs.webkit.org/attachment.cgi?id=62320&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
Just a couple more small things.

> +++ LayoutTests/resources/dump-as-markup.js	(working copy)
> +    if (typeof opt_node == 'string') {
> +	   // Allow to specify the node by id and to pass the description in
the first argument
> +	   if (document.getElementById(opt_node))
> +	       opt_node = document.getElementById(opt_node);
> +	   else {
> +	       opt_node = null;
> +	       opt_description = opt_node;
> +	   }

Why do we have this else clause? If opt_node is a string and the node is not in
the document, we should just error.

> +    if (Markup._test_description && !Markup._container)
> +	   markup += Markup._test_description + '\n';
> +
> +    Markup._dumpCalls++;
> +
> +    // If dump is called not by notifyDone or dump has already been called
then
> +    // print out optional description because this is a test with multiple
calls of dump.
> +    if (!Markup._done || opt_description) {
> +	   if (!opt_description)
> +	       opt_description = "Dump of markup " + Markup._dumpCalls
> +	   if (Markup._container)

Checking Markup._container here and above is a bit confusing to me. How about
checking that Markup._dumpCalls > 1? That should have the same effect, right?

> +	       markup += '\n';
> +	   markup += '\n' + opt_description;

Nit: Can you add add a colon after the description? I think it makes the output
a bit more readable.

> -    Markup.dump();
> +    Markup._done = true;
> +
> +    // If dump has already been called, don't bother to dump again
I like this!

> +    if (!Markup._dumpCalls)
> +	   Markup.dump();