webkit-patch rebaseline is broken
Created attachment 172100 [details] Patch
Comment on attachment 172100 [details] Patch I don't understand this fix. Can you explain it more in the ChangeLog?
Created attachment 172149 [details] clarify bugfix in changelog
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.
(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.
sgtm
Committed r133382: <http://trac.webkit.org/changeset/133382>