Bug 42049

Summary: Implement waitUntilDone and notifyDone for WebKitTestRunner
Product: WebKit Reporter: Maciej Stachowiak <mjs>
Component: New BugsAssignee: Maciej Stachowiak <mjs>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch mitz: review+

Description Maciej Stachowiak 2010-07-11 16:58:08 PDT
Implement waitUntilDone and notifyDone for WebKitTestRunner
Comment 1 Maciej Stachowiak 2010-07-11 19:13:09 PDT
Created attachment 61187 [details]
Patch
Comment 2 mitz 2010-07-11 19:29:05 PDT
Comment on attachment 61187 [details]
Patch

> +static const CFTimeInterval waitToDumpWatchdogInterval = 30.0;

Current style is to omit the “.0”.

> +void LayoutTestController::setWaitToDump(bool waitUntilDone)
> +{
> +    m_waitToDump = waitUntilDone;
> +    if (m_waitToDump && !m_waitToDumpWatchdog) {
> +        m_waitToDumpWatchdog.adoptCF(CFRunLoopTimerCreate(kCFAllocatorDefault, CFAbsoluteTimeGetCurrent() + waitToDumpWatchdogInterval, 
> +                                                      0, 0, 0, waitUntilDoneWatchdogFired, NULL));
> +        CFRunLoopAddTimer(CFRunLoopGetCurrent(), m_waitToDumpWatchdog.get(), kCFRunLoopCommonModes);
> +    }
> +}

It’s not clear that this method does the right thing when waitUntilDone is false. Fortunately, it’s always passed true. (In particular, if it were ever possible for this to get called with false, wouldn’t it need to invalidate the watchdog timer and dump if the page has already finished loading?). Anyway, this is the same in DRT.

> +static JSValueRef waitUntilDoneCallback(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)
> +{
> +    // Has mac & windows implementation
> +    LayoutTestController* controller = static_cast<LayoutTestController*>(JSObjectGetPrivate(thisObject));
> +    controller->setWaitToDump(true);
> +
> +    return JSValueMakeUndefined(context);
> +}
> +
> +static JSValueRef notifyDoneCallback(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)
> +{
> +    LayoutTestController* controller = static_cast<LayoutTestController*>(JSObjectGetPrivate(thisObject));
> +    controller->notifyDone();
> +    return JSValueMakeUndefined(context);
> +}

I am not sure why one of these deserves a comment and an empty line before the return statement and the other doesn’t.

> +    bool m_waitToDump; // True if waitUntilDone() has been called, but notifyDone() has not yet been called.

Shouldn’t this member variable be initialized to false in the constructor?
Comment 3 mitz 2010-07-11 19:33:07 PDT
Comment on attachment 61187 [details]
Patch

r+ based on changes Maciej said he’d make before landing
Comment 4 Maciej Stachowiak 2010-07-11 19:39:37 PDT
Created attachment 61188 [details]
Patch
Comment 5 mitz 2010-07-11 19:41:54 PDT
Comment on attachment 61188 [details]
Patch


> +
> +2010-07-11  Maciej Stachowiak  <mjs@apple.com>
> +
> +        * WebKitTestRunner/InjectedBundle/LayoutTestController.cpp:
> +

Stray changelog entry.

> +static const CFTimeInterval waitToDumpWatchdogInterval = 30.0;

Current style is to omit the “.0”.

r=me
Comment 6 Maciej Stachowiak 2010-07-11 20:41:28 PDT
Committed r63061: <http://trac.webkit.org/changeset/63061>