Bug 97619 - webkit-patch rebaseline-expectations is broken
Summary: webkit-patch rebaseline-expectations is broken
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-25 17:44 PDT by Dirk Pranke
Modified: 2012-09-28 13:27 PDT (History)
6 users (show)

See Also:


Attachments
Patch (5.26 KB, patch)
2012-09-25 17:49 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
remove debugger call (5.28 KB, patch)
2012-09-25 18:09 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
remove the --platform arg (2.40 KB, patch)
2012-09-25 18:55 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
Patch (4.00 KB, patch)
2012-09-28 13:02 PDT, Dirk Pranke
ojan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2012-09-25 17:44:31 PDT
webkit-patch rebaseline-expectations is broken
Comment 1 Dirk Pranke 2012-09-25 17:49:19 PDT
Created attachment 165705 [details]
Patch
Comment 2 Dirk Pranke 2012-09-25 17:54:01 PDT
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?
Comment 3 Dirk Pranke 2012-09-25 18:09:15 PDT
Created attachment 165708 [details]
remove debugger call
Comment 4 Ojan Vafai 2012-09-25 18:39:10 PDT
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 ].
Comment 5 Dirk Pranke 2012-09-25 18:55:37 PDT
Created attachment 165714 [details]
remove the --platform arg
Comment 6 Dirk Pranke 2012-09-25 18:56:33 PDT
moved the --platform arg patch into bug 97621.
Comment 7 Dirk Pranke 2012-09-25 18:58:18 PDT
(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.
Comment 8 Ojan Vafai 2012-09-26 09:59:18 PDT
(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.
Comment 9 Dirk Pranke 2012-09-28 13:02:39 PDT
Created attachment 166299 [details]
Patch
Comment 10 Dirk Pranke 2012-09-28 13:03:15 PDT
okay, I've convinced myself about the correctness of this, and added a test. Please take a look?
Comment 11 Dirk Pranke 2012-09-28 13:27:39 PDT
Committed r129941: <http://trac.webkit.org/changeset/129941>