Bug 183309 – Create analysis task should use build time as fallback when commit time is not available.

• Status: NEW
• Component: New Bugs

Create analysis task should use build time as fallback when commit time is not available.

Created attachment 334946 [details]
Patch

[EWS Watchlist]

Comment on attachment 334946 [details]
Patch

Attachment 334946 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6745646

New failing tests:
compositing/debug-borders-dynamic.html

[EWS Watchlist]

Created attachment 334947 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Ryosuke Niwa]

Comment on attachment 334946 [details]
Patch

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

> Websites/perf.webkit.org/public/privileged-api/create-analysis-task.php:97
> +    return $result ? ($result[0]['time'] ? $result[0]['time'] : $result[0]['build_time']) : null;

Can't we just do $result[0]['time'] || $result[0]['build_time'] ?

Comment on attachment 334946 [details]
Patch

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

>> Websites/perf.webkit.org/public/privileged-api/create-analysis-task.php:97
>> +    return $result ? ($result[0]['time'] ? $result[0]['time'] : $result[0]['build_time']) : null;
> 
> Can't we just do $result[0]['time'] || $result[0]['build_time'] ?

No. Logical operator will return true / false in PHP. http://php.net/manual/en/language.operators.logical.php

[Ryosuke Niwa]

Comment on attachment 334946 [details]
Patch

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

>>> Websites/perf.webkit.org/public/privileged-api/create-analysis-task.php:97
>>> +    return $result ? ($result[0]['time'] ? $result[0]['time'] : $result[0]['build_time']) : null;
>> 
>> Can't we just do $result[0]['time'] || $result[0]['build_time'] ?
> 
> No. Logical operator will return true / false in PHP. http://php.net/manual/en/language.operators.logical.php

Okay. In that case, do
$first_result = array_get($result, 0, array());
return array_key_exists('time', $first_result) : $first_result['time'] : $first_result['build_time'];

Comment on attachment 334946 [details]
Patch

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

>>>> Websites/perf.webkit.org/public/privileged-api/create-analysis-task.php:97
>>>> +    return $result ? ($result[0]['time'] ? $result[0]['time'] : $result[0]['build_time']) : null;
>>> 
>>> Can't we just do $result[0]['time'] || $result[0]['build_time'] ?
>> 
>> No. Logical operator will return true / false in PHP. http://php.net/manual/en/language.operators.logical.php
> 
> Okay. In that case, do
> $first_result = array_get($result, 0, array());
> return array_key_exists('time', $first_result) : $first_result['time'] : $first_result['build_time'];

Sounds a good idea. The 'time' key will always exist as long as there is a matching entry. Changing `array_key_exists('time', $first_result) : $first_result['time'] : $first_result['build_time']` to `$first_result['time'] ? $first_result['time'] : $first_result['build_time']` would make it work.

[Ryosuke Niwa]

(In reply to dewei_zhu from comment #7)
> Comment on attachment 334946 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334946&action=review
> 
> >>>> Websites/perf.webkit.org/public/privileged-api/create-analysis-task.php:97
> >>>> +    return $result ? ($result[0]['time'] ? $result[0]['time'] : $result[0]['build_time']) : null;
> >>> 
> >>> Can't we just do $result[0]['time'] || $result[0]['build_time'] ?
> >> 
> >> No. Logical operator will return true / false in PHP. http://php.net/manual/en/language.operators.logical.php
> > 
> > Okay. In that case, do
> > $first_result = array_get($result, 0, array());
> > return array_key_exists('time', $first_result) : $first_result['time'] : $first_result['build_time'];
> 
> Sounds a good idea. The 'time' key will always exist as long as there is a
> matching entry. Changing `array_key_exists('time', $first_result) :
> $first_result['time'] : $first_result['build_time']` to
> `$first_result['time'] ? $first_result['time'] :
> $first_result['build_time']` would make it work.

Okay. We should be careful for the case when time is 0 and NULL.

Comment on attachment 334946 [details]
Patch

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

>>>>>> Websites/perf.webkit.org/public/privileged-api/create-analysis-task.php:97
>>>>>> +    return $result ? ($result[0]['time'] ? $result[0]['time'] : $result[0]['build_time']) : null;
>>>>> 
>>>>> Can't we just do $result[0]['time'] || $result[0]['build_time'] ?
>>>> 
>>>> No. Logical operator will return true / false in PHP. http://php.net/manual/en/language.operators.logical.php
>>> 
>>> Okay. In that case, do
>>> $first_result = array_get($result, 0, array());
>>> return array_key_exists('time', $first_result) : $first_result['time'] : $first_result['build_time'];
>> 
>> Sounds a good idea. The 'time' key will always exist as long as there is a matching entry. Changing `array_key_exists('time', $first_result) : $first_result['time'] : $first_result['build_time']` to `$first_result['time'] ? $first_result['time'] : $first_result['build_time']` would make it work.
> 
> Okay. We should be careful for the case when time is 0 and NULL.

We've already have some code that assumes time is never to be 0. For example: line 29 of create-analysis-task.php `if (!$start_run_time || !$end_run_time || $start_run_time == $end_run_time)`

[Ryosuke Niwa]

(In reply to dewei_zhu from comment #9)
> Comment on attachment 334946 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334946&action=review
> 
> >>>>>> Websites/perf.webkit.org/public/privileged-api/create-analysis-task.php:97
> >>>>>> +    return $result ? ($result[0]['time'] ? $result[0]['time'] : $result[0]['build_time']) : null;
> >>>>> 
> >>>>> Can't we just do $result[0]['time'] || $result[0]['build_time'] ?
> >>>> 
> >>>> No. Logical operator will return true / false in PHP. http://php.net/manual/en/language.operators.logical.php
> >>> 
> >>> Okay. In that case, do
> >>> $first_result = array_get($result, 0, array());
> >>> return array_key_exists('time', $first_result) : $first_result['time'] : $first_result['build_time'];
> >> 
> >> Sounds a good idea. The 'time' key will always exist as long as there is a matching entry. Changing `array_key_exists('time', $first_result) : $first_result['time'] : $first_result['build_time']` to `$first_result['time'] ? $first_result['time'] : $first_result['build_time']` would make it work.
> > 
> > Okay. We should be careful for the case when time is 0 and NULL.
> 
> We've already have some code that assumes time is never to be 0. For
> example: line 29 of create-analysis-task.php `if (!$start_run_time ||
> !$end_run_time || $start_run_time == $end_run_time)`

I think those are all potential bugs.