Bug 127263

Summary: [EFL][WK2] Use nanoseconds in TimerWorkItem class consistently
Product: WebKit Reporter: Jinwoo Song <jinwoo7.song>
Component: WebKit EFLAssignee: Jinwoo Song <jinwoo7.song>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, bunhere, bw80.lee, cdumez, cmarcelo, commit-queue, gyuyoung.kim, gyuyoung.kim, lucas.de.marchi, rakuco
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Jinwoo Song 2014-01-19 22:16:41 PST
After r162276 and r162300, TimerWorkItem is getting nanoseconds as parameter.
So it would be better to use nanoseconds in TimerWorkItem for consistency.
Comment 1 Jinwoo Song 2014-01-19 22:23:43 PST
Created attachment 221614 [details]
Patch
Comment 2 WebKit Commit Bot 2014-01-19 22:24:56 PST
Attachment 221614 [details] did not pass style-queue:


ERROR: Source/WebKit2/Platform/efl/DispatchQueueWorkItemEfl.h:57:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/Platform/efl/DispatchQueueWorkItemEfl.h:66:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Gyuyoung Kim 2014-01-19 22:30:14 PST
Comment on attachment 221614 [details]
Patch

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

Looks make sense except for style nits and a new function in CurrentTime.

> Source/WTF/wtf/CurrentTime.h:64
> +inline double monotonicallyIncreasingTimeNS()

Should we add this function ? Can't we use this functionality by using monotonicallyIncreasingTimeMS() ?

It looks this new function need to be reviewed by other reviewer.
Comment 4 Jinwoo Song 2014-01-19 23:16:05 PST
Comment on attachment 221614 [details]
Patch

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

Regarding the style error, I got a guide from andersca to make a space after 'void'. It seems that the style-checker need to be updated for this rule.

>> Source/WTF/wtf/CurrentTime.h:64
>> +inline double monotonicallyIncreasingTimeNS()
> 
> Should we add this function ? Can't we use this functionality by using monotonicallyIncreasingTimeMS() ?
> 
> It looks this new function need to be reviewed by other reviewer.

It would be better not to add new function as it is only used in EFL port. I'll use monotonicallyIncreasingTime().
Comment 5 Jinwoo Song 2014-01-19 23:17:22 PST
Created attachment 221619 [details]
Patch
Comment 6 WebKit Commit Bot 2014-01-19 23:18:28 PST
Attachment 221619 [details] did not pass style-queue:


ERROR: Source/WebKit2/Platform/efl/DispatchQueueWorkItemEfl.h:57:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/Platform/efl/DispatchQueueWorkItemEfl.h:66:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Gyuyoung Kim 2014-01-20 00:47:07 PST
CC'ing Byungwoo
Comment 8 Gyuyoung Kim 2014-01-20 00:54:53 PST
Comment on attachment 221619 [details]
Patch

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

Byungwoo, any comment ?

> Source/WebKit2/Platform/efl/DispatchQueueEfl.cpp:245
> +    double timeOutSeconds = (m_timerWorkItems[0]->expirationTimeNanoSeconds() - monotonicallyIncreasingTime() * nanoSecondsPerSecond) / nanoSecondsPerSecond;

It looks "* nanoSecondsPerSecond) / nanoSecondsPerSecond;" is useless.
Comment 9 Jinwoo Song 2014-01-20 20:58:15 PST
Comment on attachment 221619 [details]
Patch

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

>> Source/WebKit2/Platform/efl/DispatchQueueEfl.cpp:245
>> +    double timeOutSeconds = (m_timerWorkItems[0]->expirationTimeNanoSeconds() - monotonicallyIncreasingTime() * nanoSecondsPerSecond) / nanoSecondsPerSecond;
> 
> It looks "* nanoSecondsPerSecond) / nanoSecondsPerSecond;" is useless.

monotonicallyIncreasingTime() is seconds but 'm_timerWorkItems[0]->expirationTimeNanoSeconds()' is nanoseconds.
Comment 10 Gyuyoung Kim 2014-01-21 00:43:32 PST
Comment on attachment 221619 [details]
Patch

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

>>> Source/WebKit2/Platform/efl/DispatchQueueEfl.cpp:245
>>> +    double timeOutSeconds = (m_timerWorkItems[0]->expirationTimeNanoSeconds() - monotonicallyIncreasingTime() * nanoSecondsPerSecond) / nanoSecondsPerSecond;
>> 
>> It looks "* nanoSecondsPerSecond) / nanoSecondsPerSecond;" is useless.
> 
> monotonicallyIncreasingTime() is seconds but 'm_timerWorkItems[0]->expirationTimeNanoSeconds()' is nanoseconds.

I'm sorry. I mistook this code. Yes, looks correct.
Comment 11 WebKit Commit Bot 2014-01-21 17:58:33 PST
Comment on attachment 221619 [details]
Patch

Clearing flags on attachment: 221619

Committed r162490: <http://trac.webkit.org/changeset/162490>
Comment 12 WebKit Commit Bot 2014-01-21 17:58:37 PST
All reviewed patches have been landed.  Closing bug.