Bug 99964

Created attachment 169837 [details] Patch

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

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

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

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.

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.

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

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

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

Comment on attachment 169967 [details] Patch Clearing flags on attachment: 169967 Committed r132138: <http://trac.webkit.org/changeset/132138>

All reviewed patches have been landed. Closing bug.