<div><div class="gmail_quote">On Thu, Apr 19, 2012 at 10:35 PM, David Barr <span dir="ltr"><<a href="mailto:davidbarr@google.com">davidbarr@google.com</a>></span> wrote:<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

Regarding style, the change homogenizes the loop constructs within that method.</blockquote></div><div class="gmail_quote"><br></div><div class="gmail_quote">I don't think that's necessary an improvement. e.g. we don't have a style guide that mandates it.</div>

<div class="gmail_quote"><br></div><div class="gmail_quote"><div>I completely agree with Maciej here that if this is a reachable code, then the patch author should put a reasonable effort into creating a test case. <span style="font-family:arial,sans-serif;line-height:18px">And most importantly, these changes are clearly not "code cleanup".</span></div>

<div><br></div><div>On Thu, Apr 19, 2012 at 11:11 PM, David Levin <span dir="ltr"><<a href="mailto:levin@google.com">levin@google.com</a>></span> wrote:</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

I understand the other side as well that it would be good to figure out if it is really an issue and find a test to prove it. I guess this is more of what I think of as a BSD type of approach. It seems to be an area where reasonable people can disagree.</blockquote>

<div><br></div><div>The WebKit contribution guide lists this as a requirement (<a href="http://www.webkit.org/coding/contributing.html">http://www.webkit.org/coding/contributing.html</a>):</div><div><span style="color:rgb(51,51,51);font-family:'Lucida Grande',Verdana,Arial;font-size:12px;line-height:18px;background-color:rgb(255,255,255)">For any feature that affects the layout engine, a new regression test must be constructed. If you provide a patch that fixes a bug, that patch should also include the addition of a regression test that would fail without the patch and succeed with the patch. If no regression test is provided, the reviewer will ask you to revise the patch, so you can save time by constructing the test up front and making sure it's attached to the bug.</span></div>

<div><br></div><div>So I don't think we can "disagree" on this topic. I'm sympathetic to the argument that it's hard to come up with a test case for things like this, but then the patch author should clearly state that in the change log, and most importantly the reviewer should be asking that during the review.</div>

<div><br></div><div>- Ryosuke</div><div><br></div></div></div>