Bug 97619 – webkit-patch rebaseline-expectations is broken

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

webkit-patch rebaseline-expectations is broken

[Dirk Pranke]

Created attachment 165705 [details]
Patch

[Dirk Pranke]

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?

[Dirk Pranke]

Created attachment 165708 [details]
remove debugger call

[Ojan Vafai]

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 ].

[Dirk Pranke]

Created attachment 165714 [details]
remove the --platform arg

[Dirk Pranke]

moved the --platform arg patch into bug 97621.

[Dirk Pranke]

(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.

[Ojan Vafai]

(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.

[Dirk Pranke]

Created attachment 166299 [details]
Patch

[Dirk Pranke]

okay, I've convinced myself about the correctness of this, and added a test. Please take a look?

[Dirk Pranke]

Committed r129941: <http://trac.webkit.org/changeset/129941>