[webkit-reviews] review denied: [Bug 99964] [chromium] introduce WebTask to the TestRunner library : [Attachment 169915] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied jochen at chromium.org's request for
review:
Bug 99964: [chromium] introduce WebTask to the TestRunner library
https://bugs.webkit.org/show_bug.cgi?id=99964

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

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


> Tools/DumpRenderTree/chromium/TestRunner/public/WebTask.h:36
> +#include "webkit/support/webkit_support.h"
> +
> +#include <vector>

These seem like strange dependencies to have in the API.  Can we remove them?

> Tools/DumpRenderTree/chromium/TestRunner/public/WebTask.h:43
> +// WebTask represents a task which can run by postTask() or
postDelayedTask().
> +// it is named "WebTask", not "Task", to avoid conflist with base/task.h.

conflist -> conflicts

This doen't really apply now that we're using the WebTestRunner namespace.

> Tools/DumpRenderTree/chromium/TestRunner/public/WebTask.h:44
> +class WebTask : public webkit_support::TaskAdaptor {

I hope we can avoid this base class here.  We might need to refactor things a
bit so that we can have a cleaner API (i.e., that doesn't depend on
webkit_support).  The TaskAdaptor class seems very simple...

> Tools/DumpRenderTree/chromium/TestRunner/public/WebTask.h:46
> +    WebTask(WebTaskList*);

explicit ?

> Tools/DumpRenderTree/chromium/TestRunner/public/WebTask.h:69
> +    std::vector<WebTask*> m_tasks;

Should this be a WebVector?  Maybe this class should have an m_private
implementation.

> Tools/DumpRenderTree/chromium/TestRunner/public/WebTask.h:74
> +// A task containing an object pointer of class T. Is is supposed that
> +// runifValid() calls a member function of the object pointer.
> +// Class T must have "WebTaskList* taskList()".

"Is is supposed" isn't grammatically correct. I might re-write this comment to
be clearer.

> Tools/DumpRenderTree/chromium/TestRunner/public/WebTask.h:75
> +template<class T> class WebMethodTask: public WebTask {

This line isn't in correct style.

> Tools/DumpRenderTree/chromium/TestRunner/public/WebTask.h:77
> +    WebMethodTask(T* object): WebTask(object->taskList()), m_object(object)
{ }

Bad style.