[webkit-reviews] review denied: [Bug 88260] Make a builder group support+expect multiple loads. : [Attachment 145634] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jun 4 15:35:21 PDT 2012
Ojan Vafai <ojan at chromium.org> has denied Chase Phillips <cmp at chromium.org>'s
request for review:
Bug 88260: Make a builder group support+expect multiple loads.
https://bugs.webkit.org/show_bug.cgi?id=88260
Attachment 145634: Patch
https://bugs.webkit.org/attachment.cgi?id=145634&action=review
------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=145634&action=review
Thanks for following up on this. I completely forgot about it. :( This looks
good. Just a few minor things to cleanup. FYI, when you next upload it, if you
use --request-commit in the webkit-patch command it will set cq? indicating
that you'd like the reviewer to put the patch in the commit queue once they
approve it.
> Tools/ChangeLog:7
> +
This could use a little bit more description (a couple short sentences) about
the changes being made, e.g. something like:
Each BuilderGroup now keeps track of how many sub-groups it still expects to
load. We only consider the builder group loaded once all the sub-groups have
loaded for failed to load.
> Tools/TestResultServer/static-dashboards/builders.js:51
> +CHROMIUM_LINUX_BUILDER_MASTER = new BuilderMaster('ChromiumMac',
'http://build.chromium.org/p/chromium.linux/');
s/ChromiumMac/ChromiumLinux
> Tools/TestResultServer/static-dashboards/builders.js:74
> + this.groups = 0;
> + this.expectedGroups = expectedGroups;
Can we kill the expectedGroups argument? Instead, in requestBuilderList you
increment expectedGroups. Then the rest of the code just works without needing
to hard-code the number of expected groups.
> Tools/TestResultServer/static-dashboards/builders.js:75
> + this.append(builders);
Do we ever pass in a non-empty builders to the constructor? Can we just remove
the argument entirely? Then we also wouldn't need the length check at the
beginning of append.
> Tools/TestResultServer/static-dashboards/builders.js:81
> + if (flags & BuilderGroup.DEFAULT_BUILDER)
> + this.defaultBuilder = builder;
While you're modifying this code, would you mind putting a FIXME here to remove
this? AFAIK, we don't actually use DEFAULT_BUILDER in any meaningful way
anymore (we always just default to the first builder in alphabetical order).
> Tools/TestResultServer/static-dashboards/builders.js:100
> + if (this.groups >= this.expectedGroups)
> + return true;
> + return false;
Nit: I'd just merge this all into one line:
return this.groups >= this.expectedGroups;
> Tools/TestResultServer/static-dashboards/builders.js:127
> + builderGroups[groupName] = builderGroup;
Needs 4 space indent.
> Tools/TestResultServer/static-dashboards/builders.js:210
> + g_handleBuildersListLoaded();
Needs 4 space indent.
> Tools/TestResultServer/static-dashboards/flakiness_dashboard_unittests.js:648
> + '@ToT - dummy.org': new BuilderGroup(BuilderGroup.TOT_WEBKIT, [],
3),
This will obviously need to change given my request to remove expectedGroups
from the constructor, but I'm OK with the tests just setting
builderGroup.expectedGroups = 3. Somehow the hard-coding bothers me less in a
test. :)
> Tools/TestResultServer/static-dashboards/run-unittests.html:57
> +LAYOUT_TESTS_BUILDER_GROUPS[groupName] = new
BuilderGroup(BuilderGroup.TOT_WEBKIT, [], 4);
Ditto here. This can set LAYOUT_TESTS_BUILDER_GROUPS[groupName].expectedGroups
= 4.
More information about the webkit-reviews
mailing list