Bug 101081 – webkit-patch rebaseline is broken

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

webkit-patch rebaseline is broken

[Dirk Pranke]

Created attachment 172100 [details]
Patch

[Ojan Vafai]

Comment on attachment 172100 [details]
Patch

I don't understand this fix. Can you explain it more in the ChangeLog?

[Dirk Pranke]

Created attachment 172149 [details]
clarify bugfix in changelog

[Ojan Vafai]

Comment on attachment 172149 [details]
clarify bugfix in changelog

Not sure whether I prefer this or changing AbstractParallelRebaselineCommand to check:
if hasattr(options, "results_directory"):

They're both a little gross.

Another option would be to have AbstractParallelRebaselineCommand._rebaseline not take an options argument at all and just take the individual arguments it needs to know about. That'd probably be cleanest.

Up to you.

[Dirk Pranke]

(In reply to comment #4)
> (From update of attachment 172149 [details])
> Not sure whether I prefer this or changing AbstractParallelRebaselineCommand to check:
> if hasattr(options, "results_directory"):
> 
> They're both a little gross.
> 
> Another option would be to have AbstractParallelRebaselineCommand._rebaseline not take an options argument at all and just take the individual arguments it needs to know about. That'd probably be cleanest.
> 
> Up to you.

Yeah, I considered all of these options already. I really don't like the 'hasattr' approach (it's a real flaw in the way we handle all of the port-specific options in the layout_tests/port classes).

I originally wasn't passing the options object down and was passing the arguments individually but that was getting increasingly unwieldy.

I don't think this is far away from the idea that the options object has a set of default values, and then we provide command line flags to override them where relevant; that approach is cleaner, but the interface that the tool command constructors use to pass options back and forth don't give you a way to do this, unfortunately (we don't get direct access to the options argument until execute() is called, so we can't set default values anywhere).

So I prefer the way I ended up doing it. Hope that helps explain the approach.

[Ojan Vafai]

sgtm

[Dirk Pranke]

Committed r133382: <http://trac.webkit.org/changeset/133382>