Bug 42049 - Implement waitUntilDone and notifyDone for WebKitTestRunner
Summary: Implement waitUntilDone and notifyDone for WebKitTestRunner
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Maciej Stachowiak
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-11 16:58 PDT by Maciej Stachowiak
Modified: 2010-07-11 20:41 PDT (History)
0 users

See Also:


Attachments
Patch (11.76 KB, patch)
2010-07-11 19:13 PDT, Maciej Stachowiak
no flags Details | Formatted Diff | Diff
Patch (12.27 KB, patch)
2010-07-11 19:39 PDT, Maciej Stachowiak
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>