[webkit-reviews] review denied: [Bug 42674] WebKitTestRunner needs testRunner.queueLoad : [Attachment 166957] patch v2

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Mikhail Pozdnyakov
<mikhail.pozdnyakov at intel.com>'s request for review:
Bug 42674: WebKitTestRunner needs testRunner.queueLoad
https://bugs.webkit.org/show_bug.cgi?id=42674

Attachment 166957: patch v2
https://bugs.webkit.org/attachment.cgi?id=166957&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=166957&action=review


Only doing a shallow review, didn't attempt to check logic.

> Source/WebKit2/Shared/WebURL.h:57
> +	   KURL* absoluteURL = new KURL(*baseURL->m_parsedURL.get(), string);
> +
> +	   return adoptRef(new WebURL(absoluteURL, absoluteURL->string()));

I think that you should use OwnPtr/PassOwnPtr here. Adopting inside constructor
is opaque and error-prone.

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:810
> +    WKRetainPtr<WKURLRef> baseURLWK =
WKBundleFrameCopyURL(WKBundlePageGetMainFrame(InjectedBundle::shared().page()->
page()));
> +    WKRetainPtr<WKURLRef> urlWK = WKURLCreateWithBaseURL(baseURLWK.get(),
toWTFString(toWK(url)).utf8().data());
> +    WKRetainPtr<WKStringRef> urlStringWK = WKURLCopyString(urlWK.get());

These all leak. Results of "copy" and "create" functions should be adopted.

> Tools/WebKitTestRunner/TestInvocation.cpp:395
> +	   const uint64_t stepCount =
WKUInt64GetValue(static_cast<WKUInt64Ref>(messageBody));

As a matter of coding style, we don't use const local variables.

> Tools/WebKitTestRunner/TestInvocation.cpp:427
> +	   const bool isEmpty =
TestController::shared().workQueueManager().isWorkQueueEmpty();

As a matter of coding style, we don't use const local variables.

> Tools/WebKitTestRunner/WorkQueueManager.cpp:37
> +namespace {

As a matter of coding style, we don't use anonymous namespaces. These make
debugging harder, and name clashes that they facilitate make refactoring
harder.

> Tools/WebKitTestRunner/WorkQueueManager.cpp:89
> +	   { }

These braces should be on separate lines.

> Tools/WebKitTestRunner/WorkQueueManager.cpp:94
> +		   // FIXME: Use target.

This could use a little more verbosity. What is broken because of us not using
it?

> Tools/WebKitTestRunner/WorkQueueManager.cpp:103
> +	   WKRetainPtr<WKURLRef> mUrl;
> +	   String mTarget;

Coding style: m_url, m_target.

> Tools/WebKitTestRunner/WorkQueueManager.cpp:113
> +	   BackNavigationItem(unsigned howFarBackward):
mHowFarBackward(howFarBackward) { }

There should be a space before colon.

> Tools/WebKitTestRunner/WorkQueueManager.cpp:117
> +	   unsigned mHowFarBackward;

m_howFarBackward.

> Tools/WebKitTestRunner/WorkQueueManager.h:59
> +    WorkQueue mWorkQueue;
> +    bool mProcessing;

Coding style: m_workQueue, m_processing.

> Tools/WebKitTestRunner/win/WebKitTestRunner.vcproj:519
> -			>
> +			   >

Looks like this file should use tabs.

> Tools/WebKitTestRunner/win/WebKitTestRunner.vcproj:536
> +		   <File
> +			   RelativePath="..\WorkQueueManager.cpp"
> +			   >
> +		   </File>
> +		   <File
> +			   RelativePath="..\WorkQueueManager.h"
> +			   >
> +		   </File>

Looks like this file should use tabs.