[webkit-reviews] review denied: [Bug 42767] [Windows] Home hey doesn't work in first DIV inside a TABLE : [Attachment 62262] fixes the bug

[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 62262: fixes the bug
https://bugs.webkit.org/attachment.cgi?id=62262&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
It would be nice if, in the cases where there are multiple dump calls, if each
one could be prefixed. Also, it would be good if dump took a second optional
argument for a description string. For example, I'd like the test expectations
for this test to be something like the following:

Tests whether home moves the caret to the beginning of line inside a
content-editable in an uneditable table.

Dump of markup 1:
<DIV id="test" contentEditable="true">
<#text>
</#text>
<DIV id="l1">
<#text>The caret is initially <selection-caret>here but should move to the
beginning of the line.</#text>
</DIV>
<#text>
</#text>
<DIV>
<#text>dummy text</#text>
</DIV>
<#text>
</#text>
</DIV>

Dump of markup 2:
<DIV id="test" contentEditable="true">
<#text>
</#text>
<DIV id="l1">
<#text><selection-caret>The caret is initially here but should move to the
beginning of the line.</#text>
</DIV>
<#text>
</#text>
<DIV>
<#text>dummy text</#text>
</DIV>
<#text>
</#text>
</DIV>

Where "Dump of markup *" is the default value for the optional description
string. If Markup.dump is only called once, without a description string, then
it should not print a description before dumping. Finally, if you make this
change, then rename Markup._description to Markup._test_description to avoid
confusion.

> +++ LayoutTests/resources/dump-as-markup.js	(working copy)

Your indentation is off in many places in this file.

> +	var markup = "";
> +
> +	if (Markup._description && !Markup._container)
> +		markup = Markup._description + '\n';

This should be += (imagine if a line got inserted above this one modifying
markup). 

> +	   Markup._container.style.cssText = 'width:100%';

I know you're just modifying existing code, but this should probably be
"Markup._container.style.width = '100%';". No reason to use cssText if we're
not actually setting multiple properties.