[webkit-reviews] review granted: [Bug 193132] Refactor 'platforms' table to contain group information. : [Attachment 358308] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted dewei_zhu at apple.com's request for
review:
Bug 193132: Refactor 'platforms' table to contain group information.
https://bugs.webkit.org/show_bug.cgi?id=193132

Attachment 358308: Patch

https://bugs.webkit.org/attachment.cgi?id=358308&action=review




--- Comment #3 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 358308
  --> https://bugs.webkit.org/attachment.cgi?id=358308
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=358308&action=review

> Websites/perf.webkit.org/public/admin/platforms.php:99
> +    array_push($platform_group_options, array('platformgroup_id' => NULL,
'platformgroup_name' => '&ltNo Group&gt'));

Maybe just say "-"?

> Websites/perf.webkit.org/public/admin/platforms.php:103
> +	   $id = $platform_row['platform_id'];

Call intval here just be safe.

> Websites/perf.webkit.org/public/v3/models/analysis-task.js:180
> +	   const fetchingMeasurementSetForOtherPlatformInSameGroupPromises =
this._commitSetForOtherPlatformsInSameGroup();

This is awfully long variable name. Also, this isn't fetching measurement set
so we probably shouldn't call it that.
How about commitSetsInSamePlatformGroupPromise?

> Websites/perf.webkit.org/public/v3/models/analysis-task.js:198
> +    _commitSetForOtherPlatformsInSameGroup()

"SameGroup" is a bit ambiguous here because there is also test group.
I think I'd prefer call this _commitSetsInSamePlatformGroup.