[webkit-reviews] review denied: [Bug 52039] Make rebaseline server usable with bot results : [Attachment 81166] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Mihai Parparita
<mihaip at chromium.org>'s request for review:
Bug 52039: Make rebaseline server usable with bot results
https://bugs.webkit.org/show_bug.cgi?id=52039

Attachment 81166: Patch
https://bugs.webkit.org/attachment.cgi?id=81166&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=81166&action=review

Sorry this fell through the cracks.

In general I'm supportive of this change.  It may be fine to leave this logic
on rebaseline server for now, but I definitely see other places wanting to
reuse this sooner rather than later.  Maybe LayoutTestResults is the right
place for it?  Not sure. NRWT doesn't have a "layout test results directory"
object, besides perhapse Koz's new FileSet (which isn't specific to layout
tests).

I think in the interst of readability we should go one more round and break
some of these long functions into smaller chunks.

I'll try to be much faster about the next round review.  My appologies again.

> Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:165
> +	   test_file = (self.query['test'][0])

Why a tuple here?

> Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:418
> +def _construct_results_json_from_directory(test_config):

I think this should be a separate class.  This is not specific to the
rebaseline server.  Only speicifc to the NRWT results directory format and the
results.json in memeory object model.

I forsee other code wanting to re-use this "generate a json object model from
this on-disk directory" logic.

We have LayoutTestResults which does the json object model genreation from an
ORWT directory (usign results.html).  One might argue thid should go there
instead of a new class.  Your call.

I also would have broken this function into smaller pieces.  It's a bit hard to
read at this length.

> Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:502
> +	   print 'Parsing unexpected_results.json...'

This parsing should be a helper function.  No need to make execute() 3 miles
long. :)