[webkit-reviews] review granted: [Bug 83635] [Chromium] Chromium's LayoutTestController is missing setBackingScaleFactor : [Attachment 149582] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Terry Anderson
<tdanderson at chromium.org>'s request for review:
Bug 83635: [Chromium] Chromium's LayoutTestController is missing
setBackingScaleFactor
https://bugs.webkit.org/show_bug.cgi?id=83635

Attachment 149582: Patch
https://bugs.webkit.org/attachment.cgi?id=149582&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=149582&action=review


> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:715
> -    if (!m_waitUntilDone)
> -	   m_workQueue.processWorkSoon();
> +    m_workQueue.processWorkSoon();

This change doesn't cause problems for other tests?  It might be worth
understanding why the branch was here originally.

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2215
> +    WorkItemInvokeDefault(PassOwnArrayPtr<CppVariant> callbackArgs, uint32_t
numArgs) :
> +	   m_callbackArgs(callbackArgs), 
> +	   m_numArgs(numArgs) { }

This isn't quite in WebKit style.  The WebKit style would be something like:

WorkItemInvokeDefault(PassOwnArrayPtr<CppVariant> callbackArguments, uint32_t
numberOfArguments) :
    : m_callbackArguments(callbackArguments),
    , m_numberOfArguments(numberOfArguments)
{
}

The : and the , go in those funny locations to make adding and removing
properties easier.  Also, we prefer to use complete words in variable names
rather than abbreviations.

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2219
> +	   return m_callbackArgs[0].invokeDefault(m_callbackArgs.get(), 1,
invokeResult);

1 -> m_numArgs ?