Bug 61958 – [EFL] DumpRenderTree: Add GCController, PixelDumpsupport and WorkQueueItem

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

[Leandro Pereira]

[EFL] DRT: Add WorkQueueItemEfl.cpp.

[Leandro Pereira]

Created attachment 95803 [details]
Patch

[Leandro Pereira]

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

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

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 <[e-mail]>
> + * 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* :)

[Raphael Kubo da Costa (:rakuco)]

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.

[WebKit Review Bot]

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.

[Gyuyoung Kim]

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

[Raphael Kubo da Costa (:rakuco)]

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

[Leandro Pereira]

Comment on attachment 97111 [details]
Fixed patch with suggested changes.

I've restarted and cleaned up the EWS; cq? again.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 97463 [details]
Try to please the EWS bots.

This is essentially the same patch, but will hopefully make the EWS bots happier.

[Raphael Kubo da Costa (:rakuco)]

CC'ing eseidel.

[WebKit Review Bot]

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.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 97465 [details]
Yet another attempt at pleasing the bots.

Oops, I thought I had run check-webkit-style before sending. *sigh*

[Eric Seidel (no email)]

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

OK.

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.