Bug 61958

Created attachment 95803 [details] Patch

This bug was previously created to upstream WorkQueueItem only, but since it is pretty small, I'm using it to upstream other smaller DRT bits (PixelDumpSupport, GCController and WorkQueueItem).

Attachment 95803 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/e..." exit_code: 1 Tools/DumpRenderTree/efl/PixelDumpSupportEfl.cpp:35: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 95803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95803&action=review Same comments, basically. This is old c code, in a c++ file. Unless EFL style strictly requires old-looking c code, with bad memory managmeent, we should move thsi all to smart pointers and modern C++ styling. > Tools/DumpRenderTree/efl/GCControllerEfl.cpp:5 > + * Copyright (C) 2007 Eric Seidel <eric@webkit.org> > + * Copyright (C) 2011 ProFUSION Embedded Systems > + * Copyright (C) 2011 Samsung Electronics > + * I doubt you need all these copyrights. > Tools/DumpRenderTree/efl/PixelDumpSupportEfl.cpp:45 > + cairo_surface_t* surface; > + cairo_t* context; > + int width, height; > + Ewk_View_Smart_Data* smartData; > + Ewk_View_Private_Data* privateData; > + Do we really need to follow this old-C limitiation for c++ files? I doubt you can even find a C compiler which can't handle inline declarations these days. :) > Tools/DumpRenderTree/efl/PixelDumpSupportEfl.cpp:50 > + context = cairo_create(surface); No Cairo smart pointers? The Gtk Folks might know if we have one. > Tools/DumpRenderTree/efl/WorkQueueItemEfl.cpp:36 > +char* JSStringCopyUTF8CString(JSStringRef jsString) > +{ > + size_t dataSize = JSStringGetMaximumUTF8CStringSize(jsString); > + char* utf8 = static_cast<char*>(malloc(dataSize)); > + JSStringGetUTF8CString(jsString, utf8, dataSize); > + > + return utf8; > +} Woh. so this is what youv'e been using everywhere. I thoguth it was actually part of the JS API. We could easily use WTF::String here. Since other ports do. Not a requirement, but there are definitely smarter string classes to use besides char* :)

Created attachment 97111 [details] Fixed patch with suggested changes. Thanks for the review. The attached patch should solve most of the issues you've brought up. The only downside is that we've had to create our own WorkQueueItemEfl.h (just like the Qt port does). This patch should obsolete the previous one, I just don't have permissions to do that.

Attachment 97111 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 97111 [details] Fixed patch with suggested changes. Attachment 97111 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/8837601

Looking at the error log, it does not seem to be related to this patch -- try removing WebKit/libewebkit.so.0.1.0 manually.

Comment on attachment 97111 [details] Fixed patch with suggested changes. I've restarted and cleaned up the EWS; cq? again.

Created attachment 97463 [details] Try to please the EWS bots. This is essentially the same patch, but will hopefully make the EWS bots happier.

CC'ing eseidel.

Attachment 97463 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/e..." exit_code: 1 Tools/DumpRenderTree/efl/WorkQueueItemEfl.cpp:29: Alphabetical sorting problem. [build/include_order] [4] Tools/DumpRenderTree/efl/WorkQueueItemEfl.h:39: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 2 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 97465 [details] Yet another attempt at pleasing the bots. Oops, I thought I had run check-webkit-style before sending. *sigh*

Comment on attachment 97465 [details] Yet another attempt at pleasing the bots. OK.

Comment on attachment 97465 [details] Yet another attempt at pleasing the bots. Clearing flags on attachment: 97465 Committed r89053: <http://trac.webkit.org/changeset/89053>

All reviewed patches have been landed. Closing bug.