[webkit-reviews] review granted: [Bug 97619] webkit-patch rebaseline-expectations is broken : [Attachment 165708] remove debugger call

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 97619: webkit-patch rebaseline-expectations is broken
https://bugs.webkit.org/show_bug.cgi?id=97619

Attachment 165708: remove debugger call
https://bugs.webkit.org/attachment.cgi?id=165708&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=165708&action=review


This is fine. It's kind of a bandaid solution though since the other
rebaselining commands will have this problem, no? If we make [ Skip ] required
and disallow [ Skip WontFix Pass ] (i.e. it should just be [ WontFix ]). Then
these problems will just go away once we update the existing lines once, right?


> Tools/ChangeLog:14
> +	   1) the transformation into and out of the old syntax (which is
> +	   still used internally) is somewhat lossy, e.g., we're not
> +	   preserving the case of Bug(x) identifiers. Also, we can't
> +	   tell if the input was [ WontFix ] or [ Skip WontFix Pass ]

Don't we only write out the new syntax? In that case, can't we always write out
[ WontFix ]? I suppose it's annoying to have ones patch modify unrelated bits.

> Tools/ChangeLog:17
> +	   2) the new syntax is more lenient, allowing for multiple ways to
> +	   specify the same result, e.g., "[ Skip ]" may or may not be
> +	   missing.

I think we should do away with this. It seemed nice at first, but now that I
actually see it in the file, I think it's just confusing. We should require [
Skip ].