Bug 96256

Summary: [WK2][WTR] WebKitTestRunner needs layoutTestController.setMinimumTimerInterval
Product: WebKit Reporter: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Component: WebKit2Assignee: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Severity: Normal CC: cdumez, gyuyoung.kim, kenneth, ossy, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 96396    
Bug Blocks:    
Description Flags
kenneth: review+, kenneth: commit-queue-
patch v2
patch v3
patch v4 none

Description Mikhail Pozdnyakov 2012-09-10 03:59:47 PDT
WTR needs WebKitTestRunner needs layoutTestController.setMinimumTimerInterval implementation so that 
can be unskipped
Comment 1 Mikhail Pozdnyakov 2012-09-10 04:58:39 PDT
Created attachment 163091 [details]
Comment 2 Chris Dumez 2012-09-10 05:10:49 PDT
Comment on attachment 163091 [details]

Looks sane.
Comment 3 Kenneth Rohde Christiansen 2012-09-10 05:16:23 PDT
Comment on attachment 163091 [details]

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

> Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:70
> +        void setMinimumTimerInterval(in double interval);

intervalMSecs ? or what is normally used...
Comment 4 Mikhail Pozdnyakov 2012-09-10 05:43:29 PDT
Created attachment 163099 [details]
patch v2

Added a comment to .idl file describing the measure of the given interval parameter for newly added setMinimumTimerInterval().
Comment 5 Kenneth Rohde Christiansen 2012-09-10 05:49:47 PDT
Comment on attachment 163099 [details]
patch v2

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

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:676
> +void TestRunner::setMinimumTimerInterval(double interval)

why not just use "double seconds"
Comment 6 Mikhail Pozdnyakov 2012-09-10 06:02:52 PDT
Created attachment 163106 [details]
patch v3

Used 'seconds' name for the interval parameter in cpp files.
Comment 7 Kenneth Rohde Christiansen 2012-09-10 08:19:10 PDT
Comment on attachment 163106 [details]
patch v3

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

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundle.cpp:234
> +void WKBundleSetMinimumTimerInterval(WKBundleRef bundleRef, WKBundlePageGroupRef pageGroupRef, double interval)


> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.h:128
> +    void setMinimumTimerInterval(WebPageGroupProxy*, double);

here you have no idea what double means, so it makes sense to add double seconds... I see it is not perfect elsewhere, but this is the right thing to do

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:109
> +    void setMinimumTimerInterval(double); // Interval specified in seconds.

double seconds
Comment 8 Mikhail Pozdnyakov 2012-09-11 03:03:38 PDT
Created attachment 163317 [details]
patch v4

Applied comments from Kenneth.
Comment 9 WebKit Review Bot 2012-09-11 04:21:31 PDT
Comment on attachment 163317 [details]
patch v4

Clearing flags on attachment: 163317

Committed r128169: <http://trac.webkit.org/changeset/128169>
Comment 10 WebKit Review Bot 2012-09-11 04:21:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Csaba Osztrogon√°c 2012-09-11 08:49:53 PDT
(In reply to comment #9)
> (From update of attachment 163317 [details])
> Clearing flags on attachment: 163317
> Committed r128169: <http://trac.webkit.org/changeset/128169>

It caused a regression - https://bugs.webkit.org/show_bug.cgi?id=96396
Could you check it, please?