Bug 99964 – [chromium] introduce WebTask to the TestRunner library

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[chromium] introduce WebTask to the TestRunner library

Created attachment 169837 [details]
Patch

[Peter Beverloo (cr-android ews)]

Comment on attachment 169837 [details]
Patch

Attachment 169837 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14492357

[WebKit Review Bot]

Comment on attachment 169837 [details]
Patch

Attachment 169837 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14490343

Created attachment 169908 [details]
Patch

[WebKit Review Bot]

Attachment 169908 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/D..." exit_code: 1
Tools/DumpRenderTree/chromium/TestRunner/src/WebTask.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 169915 [details]
Patch

[Adam Barth]

Comment on attachment 169915 [details]
Patch

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.

[Adam Barth]

We need to think some more about how to turn this into a cleaner abstraction rather than just moving the code from Task.h to WebTask.h.

[Adam Barth]

Also, can you add this public directory to the watchlist file so that the other API reviewers will be automatically CCed on these changes?

Created attachment 169951 [details]
Patch

Comment on attachment 169915 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169915&action=review

>> Tools/DumpRenderTree/chromium/TestRunner/public/WebTask.h:36
>> +#include <vector>
> 
> These seem like strange dependencies to have in the API.  Can we remove them?

done

>> Tools/DumpRenderTree/chromium/TestRunner/public/WebTask.h:43
>> +// 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.

done

>> 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...

done.

I've moved postTask and postDelayedTask to the WebTestRunner. DRT then forwards the tasks to webkit_support, wrapping it in a TaskAdaptor when needed.

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

done

>> 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.

done

>> Tools/DumpRenderTree/chromium/TestRunner/public/WebTask.h:74
>> +// Class T must have "WebTaskList* taskList()".
> 
> "Is is supposed" isn't grammatically correct. I might re-write this comment to be clearer.

done

>> Tools/DumpRenderTree/chromium/TestRunner/public/WebTask.h:75
>> +template<class T> class WebMethodTask: public WebTask {
> 
> This line isn't in correct style.

done

> Tools/DumpRenderTree/chromium/TestRunner/public/WebTask.h:78
> +    virtual void run()

done

[Adam Barth]

Comment on attachment 169951 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169951&action=review

Much better.  Thank you!

> Tools/DumpRenderTree/chromium/Task.cpp:59
> +        delete m_task;

We can't use OwnPtr here?

> Tools/DumpRenderTree/chromium/TestRunner/public/WebTestDelegate.h:56
> +    virtual void postTask(WebTask*) = 0;
> +    virtual void postDelayedTask(WebTask*, long long ms) = 0;

I would add a comment explaining that the delegate takes ownership of the task object.

Created attachment 169961 [details]
Patch

Comment on attachment 169951 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169951&action=review

>> Tools/DumpRenderTree/chromium/Task.cpp:59
>> +        delete m_task;
> 
> We can't use OwnPtr here?

done

>> Tools/DumpRenderTree/chromium/TestRunner/public/WebTestDelegate.h:56
>> +    virtual void postDelayedTask(WebTask*, long long ms) = 0;
> 
> I would add a comment explaining that the delegate takes ownership of the task object.

done

[WebKit Review Bot]

Comment on attachment 169961 [details]
Patch

Rejecting attachment 169961 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
candidates are: WTF::OwnPtr<T>::OwnPtr(const WTF::OwnPtr<typename WTF::RemovePointer<T>::Type>&) [with T = WebTestRunner::WebTask]
Source/WTF/wtf/OwnPtr.h:50: note:                 WTF::OwnPtr<T>::OwnPtr(std::nullptr_t) [with T = WebTestRunner::WebTask]
Source/WTF/wtf/OwnPtr.h:49: note:                 WTF::OwnPtr<T>::OwnPtr() [with T = WebTestRunner::WebTask]
make: *** [out/Release/obj.target/DumpRenderTree/Tools/DumpRenderTree/chromium/Task.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/14496371

Created attachment 169967 [details]
Patch

[WebKit Review Bot]

Comment on attachment 169967 [details]
Patch

Clearing flags on attachment: 169967

Committed r132138: <http://trac.webkit.org/changeset/132138>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.