webkit-patch rebaseline-expectations is broken
Created attachment 165705 [details] Patch
Note that this patch isn't quite ready to land yet. 1) it needs a test. 2) I'm not sure about my use of reconstitute_only_these yet 3) we probably need to be smarter about how we reconstitute lines where the new line says the same thing as the original line, only differently (e.g., if the original is 'foo.html' (no [Skip]), should we leave it like 'foo.html', or add the '[Skip]' to normalize the line?
Created attachment 165708 [details] remove debugger call
Comment on attachment 165708 [details] remove debugger call 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 ].
Created attachment 165714 [details] remove the --platform arg
moved the --platform arg patch into bug 97621.
(In reply to comment #4) > (From update of attachment 165708 [details]) > 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? > There's still a bug where we don't preserve the case of the Bug(X) modifiers, e.g., Bug(EFL) gets rewritten to Bug(efl), but that needs to be fixed independently, regardless. Otherwise, in theory, yes. > > 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. > Yes, modifying unrelated lines is exactly the problem. > > 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 ]. I'm not necessarily opposed to this, but others might be.
(In reply to comment #7) > > > 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 ]. > > I'm not necessarily opposed to this, but others might be. I think this is one of the things that was not resolved in the long discussions we had about this on webkit-dev. IIRC, noone was opposed to requiring it. I think we should try requiring it and see if anyone is upset.
Created attachment 166299 [details] Patch
okay, I've convinced myself about the correctness of this, and added a test. Please take a look?
Committed r129941: <http://trac.webkit.org/changeset/129941>