Bug 35087 – New port: EFL; adding files to WebCore/*/efl

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Leandro Pereira]

Created attachment 48994 [details]
Add EFL port files in WebCore/page/efl

This patch is part of a series of patches to add an updated version of the EFL
port to the WebKit tree.

This particular patch adds files to WebCore/platform/efl, WebCore/platform/graphics/efl,
WebCore/platform/text/efl and WebCore/page/efl directories. Changes to the port-independent
parts and build system changes, will come later.

(See bug #35059 and bug #35084 for the first two patches in this series.)

It should apply cleanly on SVN rev 54651.

[Leandro Pereira]

Created attachment 48995 [details]
Add EFL port files in WebCore/platform/efl

[Leandro Pereira]

Created attachment 48996 [details]
Add EFL port files in WebCore/platform/graphics/efl

[Leandro Pereira]

Created attachment 48997 [details]
Add EFL port files in WebCore/platform/text/efl

[Leandro Pereira]

Changing platform to Other / Linux.

[Leandro Pereira]

Comment on attachment 48997 [details]
Add EFL port files in WebCore/platform/text/efl

Mark patch for review.

[Leandro Pereira]

Comment on attachment 48994 [details]
Add EFL port files in WebCore/page/efl

Mark patch for review.

[Leandro Pereira]

Comment on attachment 48995 [details]
Add EFL port files in WebCore/platform/efl

Mark patch for review.

[Leandro Pereira]

Comment on attachment 48996 [details]
Add EFL port files in WebCore/platform/graphics/efl

Mark patch for review.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Last 3072 characters of output:
/naming] [4]
WebCore/platform/efl/KeyboardCodes.h:488:  VK_OEM_3 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/efl/KeyboardCodes.h:491:  VK_OEM_4 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/efl/KeyboardCodes.h:494:  VK_OEM_5 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/efl/KeyboardCodes.h:497:  VK_OEM_6 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/efl/KeyboardCodes.h:500:  VK_OEM_7 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/efl/KeyboardCodes.h:503:  VK_OEM_8 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/efl/KeyboardCodes.h:506:  VK_OEM_102 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/efl/KeyboardCodes.h:509:  VK_PROCESSKEY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/efl/KeyboardCodes.h:512:  VK_PACKET is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/efl/KeyboardCodes.h:515:  VK_ATTN is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/efl/KeyboardCodes.h:518:  VK_CRSEL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/efl/KeyboardCodes.h:521:  VK_EXSEL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/efl/KeyboardCodes.h:524:  VK_EREOF is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/efl/KeyboardCodes.h:527:  VK_PLAY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/efl/KeyboardCodes.h:530:  VK_ZOOM is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/efl/KeyboardCodes.h:533:  VK_NONAME is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/efl/KeyboardCodes.h:536:  VK_PA1 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/efl/KeyboardCodes.h:539:  VK_OEM_CLEAR is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/efl/KeyboardCodes.h:541:  VK_UNKNOWN is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/efl/ScrollbarEfl.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/efl/PasteboardEfl.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 174 in 37 files


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

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/efl/IntPointEfl.cpp:34:  Evas_Point is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/graphics/efl/FloatRectEfl.cpp:34:  Eina_Rectangle is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/graphics/efl/FontCustomPlatformData.h:29:  cairo_font_face_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/graphics/efl/MediaPlayerPrivateGStreamer.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/efl/VideoSinkGStreamer.h:65:  parent_class is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/graphics/efl/VideoSinkGStreamer.h:76:  webkit_video_sink_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/graphics/efl/VideoSinkGStreamer.h:79:  webkit_video_sink_set_surface is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/graphics/efl/IntRectEfl.cpp:34:  Eina_Rectangle is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 8 in 17 files


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

[Oliver Hunt]

Comment on attachment 48997 [details]
Add EFL port files in WebCore/platform/text/efl

All of these patches need changelogs, so they will be r-'d on that alone, however as the larger ones are likely to require changes anyway that shouldn't be a problem :D

That said this specific patch only needs a changelog to be landable.

[Oliver Hunt]

Comment on attachment 48996 [details]
Add EFL port files in WebCore/platform/graphics/efl

The vast majority of this patch is code copied from the Gtk equivalents, really this should be shared code.  For that reason i suggest splitting this into two distinct patches.

One which contains the EFL changes to core WebCore types (IntRect, etc), and one that adds the EFL specific code to the Gtk platform code.

It's possible that some of the gtk code should be renamed from (for instance) FontPlatformDataGtk to FontPlatformDataCairo, and MediaPlayerGtk to MediaPlayerGStreamer, or at least the common cairo and gstreamer code be split into gstreamer and cairo specific files, with separate efl and gtk files for those pieces of functionality that aren't sharable.  The current copy-paste model seems likely to reduce the ability of the Efl port to benefit from work done on the Cairo and Gtk ports.  It may be worth discussing these file renames, etc with Gustavo, Philippe, and Ken in #webkit-gtk

[Oliver Hunt]

Comment on attachment 48994 [details]
Add EFL port files in WebCore/page/efl

Only issue with this patch is the absence of a changelog

[Oliver Hunt]

Comment on attachment 48995 [details]
Add EFL port files in WebCore/platform/efl

static HashMap<String, String> cookieJar;

In general we try to avoid raw globals in WebKit as they cause problems on some platforms (eg. windows) when loading webkit as a library.  But that's not too much of an issue (especially given eventually this should be replaced by a better, persistent storage)

class Cursors {
protected:
     Cursors()
         : PointerCursor("cursor/pointer"),
...
The comma should be at the beginning of the next line rather than the end of the current

It looks like KeyboardCodes, KeyboardCodeMIMETypeRegistry are also candidates for refactoring to allow multiple ports to share code, which it is probably worth looking at before landing these patches, as otherwise the unshared code is unlikely to ever become shared.

RenderTheme implementation has a lot of incorrect style usage, ( for a call always goes after the function, not on the next line.

By and large these patches look okay.  My real concern is the degree of direct copying of code rather than reuse of a shared implementation.  I think for the most part that's what will need to be fixed in order to get this landed.

[Gustavo Sverzut Barbieri]

(In reply to comment #14)
> By and large these patches look okay.  My real concern is the degree of direct
> copying of code rather than reuse of a shared implementation.  I think for the
> most part that's what will need to be fixed in order to get this landed.

Yes, we noticed this. We actually improved a bit over the initial port done by INdT, but after some time it was not so simple to detect what was copied and just renamed and what was actually changed and how -- thus we still have couple of functions as you mentioned.

It is in our plans to progressively improve the whole thing by progressively merging with Gtk, renaming from "*Gtk.{cpp,h}" to Cairo or related. This way we avoid the burden we have been taking for a while, benefiting when API changes and not need to hunt the changes in our side.

Our hope is to get the files included and take part in the process as soon as possible. Later on apply such changes that may impact other ports (Gtk) directly, as they will need to take action and patches may hang in bugzilla for long time, requiring lots of work to keep it updated :-)

[Leandro Pereira]

(In reply to comment #14)
> class Cursors {
> protected:
>      Cursors()
>          : PointerCursor("cursor/pointer"),
> ...
> The comma should be at the beginning of the next line rather than the end of
> the current

check-webkit-style didn't warn me about this.

> 
> It looks like KeyboardCodes, KeyboardCodeMIMETypeRegistry are also candidates
> for refactoring to allow multiple ports to share code, which it is probably
> worth looking at before landing these patches, as otherwise the unshared code
> is unlikely to ever become shared.
> 

I've removed KeyboardCodes, as it happened to be almost identical to the shared one.

>
> RenderTheme implementation has a lot of incorrect style usage, ( for a call
> always goes after the function, not on the next line.
> 

check-webkit-style didn't warn me about this too.

[Kenneth Rohde Christiansen]

> check-webkit-style didn't warn me about this.

That script isn't perfect and only meant to help you. Please check the style guide and feel free to supply patches to the script to make it better.

[Leandro Pereira]

(In reply to comment #17)
> > check-webkit-style didn't warn me about this.
> 
> That script isn't perfect and only meant to help you. Please check the style
> guide and feel free to supply patches to the script to make it better.

Yes, I know about that. But WebKit's a monster, you know :)

[Leandro Pereira]

Created attachment 49200 [details]
Add EFL port files in WebCore/platform/text/efl

New patch, with ChangeLog entry.

[Leandro Pereira]

Created attachment 49201 [details]
Add EFL port files in WebCore/platform/graphics/efl

Now with the correct ChangeLog entry.

[Kenneth Rohde Christiansen]

Comment on attachment 49201 [details]
Add EFL port files in WebCore/platform/graphics/efl

> +        See https://bugs.webkit.org/show_bug.cgi?id=35087 for details.

Just having the bug link would be enough. As a short cut you could write webkit.org/b/35087, but r=me

[Leandro Pereira]

Created attachment 49202 [details]
Add EFL port files in WebCore/platform/text/efl

Previous patch was submitted with wrong description.

[WebKit Commit Bot]

Comment on attachment 49202 [details]
Add EFL port files in WebCore/platform/text/efl

Rejecting patch 49202 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Kenneth Rohde Christiansen', '--force']" exit_code: 2
patching file WebCore/platform/text/efl/TextBreakIteratorInternalICUEfl.cpp
patching file WebCore/ChangeLog
patch: **** malformed patch at line 17:          Reviewed by Nate Chapin.


Full output: http://webkit-commit-queue.appspot.com/results/298760

[Leandro Pereira]

Created attachment 49226 [details]
Add EFL port files in WebCore/platform/graphics/efl

Adding revised patch, with ChangeLog entry.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/efl/IntPointEfl.cpp:24:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
WebCore/platform/graphics/efl/MediaPlayerPrivateGStreamer.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 18 files


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

[Leandro Pereira]

Created attachment 49227 [details]
Add EFL port files in WebCore/platform/efl

Patch with coding style fixes and ChangeLog entry. It is still large, but there isn't a logical way to split it up.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/efl/FileSystemEfl.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/efl/WidgetEfl.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/efl/TemporaryLinkStubs.cpp:32:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/platform/efl/TemporaryLinkStubs.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/efl/RenderThemeEfl.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/efl/PasteboardEfl.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/efl/ScrollbarEfl.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 7 in 37 files


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

[Eric Seidel (no email)]

Comment on attachment 49201 [details]
Add EFL port files in WebCore/platform/graphics/efl

Clearing flags from this obsolete patch.

[Eric Seidel (no email)]

Comment on attachment 49227 [details]
Add EFL port files in WebCore/platform/efl

Half of your files are LGPL the other half are BSD.  Is this intentional?

Also, I don't understand why you're listing any copyright from 2008 (or before 2010).  If the work wasn't published, I'm not sure how it can have copyright.

I think normally we leave a blank line here, but I don't think there is an official style:
+namespace WebCore {
+PassRefPtr<Clipboard> Editor::newGeneralClipboard(ClipboardAccessPolicy policy)

This patch would be much easier to handle in smaller pieces.

See examples from other recent ports, like BREW or Haiku, how they posted smaller patches grouped by related files.  Personally I target my patches to be < 20k, although for simple patches like these that's not always needed.  The problem here is that some of this patch is simple,a nd some of it actually has real logic.  Would be much better to post the simple patches of just "notImplemented()" code in a separate patch.

[Eric Seidel (no email)]

(In reply to comment #29)
> (From update of attachment 49227 [details])
> Half of your files are LGPL the other half are BSD.  Is this intentional?
> 
> Also, I don't understand why you're listing any copyright from 2008 (or before
> 2010).  If the work wasn't published, I'm not sure how it can have copyright.

My comment was about how this patch seems to *add* a copyright from 2008, obviously when copying files you should maintain copyrights.

[Leandro Pereira]

(In reply to comment #29)
> (From update of attachment 49227 [details])
> Half of your files are LGPL the other half are BSD.  Is this intentional?
> 

Files that were copied retained their original licenses. But we'll review all files since everything originally written by us should be LGPL-licensed.

>
> Also, I don't understand why you're listing any copyright from 2008 (or before
> 2010).  If the work wasn't published, I'm not sure how it can have copyright.
> 

This port is based on previous work by INdT, which is already published. Files based on their work have their copyright intact.

> 
> This patch would be much easier to handle in smaller pieces.
> 

I'll try to find a way to split them so they're easier to grasp.

[Leandro Pereira]

Created attachment 49519 [details]
Patch 1/7 to WebCore/platform/efl

[Leandro Pereira]

Created attachment 49520 [details]
Patch 2/7 to WebCore/platform/efl

[Leandro Pereira]

Created attachment 49521 [details]
Patch 3/7 to WebCore/platform/efl

[Leandro Pereira]

Created attachment 49522 [details]
Patch 4/7 to WebCore/platform/efl

[Leandro Pereira]

Created attachment 49523 [details]
Patch 5/7 to WebCore/platform/efl

[Leandro Pereira]

Created attachment 49524 [details]
Patch 6/7 to WebCore/platform/efl

[Leandro Pereira]

Created attachment 49525 [details]
Patch 7/7 to WebCore/platform/efl

[Kenneth Rohde Christiansen]

Comment on attachment 49519 [details]
Patch 1/7 to WebCore/platform/efl

WebCore/platform/efl/MIMETypeRegistryEfl.cpp looks like it could be shared across ports. You could consider this in a future patch.

But apart from that, looks fine.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/efl/TemporaryLinkStubs.cpp:32:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 7 files


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

[Kenneth Rohde Christiansen]

Comment on attachment 49520 [details]
Patch 2/7 to WebCore/platform/efl


> +namespace {

Why this?


> +Cursors* Cursors::self()
> +{
> +    if (!s_self)
> +        s_self = new Cursors();
> +
> +    return s_self;
> +}
> +
> +}
> +

and why end it here?
> +namespace WebCore {
> +
> +String submitButtonDefaultLabel()
> +{
> +    return String::fromUTF8("Submit");
> +}

EFL provides no translation support?

[Kenneth Rohde Christiansen]

Comment on attachment 49521 [details]
Patch 3/7 to WebCore/platform/efl


> +IntSize dragImageSize(DragImageRef)
> +{
> +    return IntSize(0, 0);
> +}
> +
> +void deleteDragImage(DragImageRef)
> +{
> +}
> +
> +DragImageRef scaleDragImage(DragImageRef image, FloatSize)
> +{
> +    return image;
> +}
> +
> +DragImageRef dissolveDragImageToFraction(DragImageRef image, float)
> +{
> +    return image;
> +}
> +

Shouldn't you add notImplemented to these?

[Kenneth Rohde Christiansen]

Comment on attachment 49522 [details]
Patch 4/7 to WebCore/platform/efl


> +
> +class ScrollbarThemeEfl : public ScrollbarTheme {
> +public:
> +    virtual ~ScrollbarThemeEfl();
> +
> +    virtual int scrollbarThickness(ScrollbarControlSize = RegularScrollbar);
> +
> +    virtual void registerScrollbar(Scrollbar* scrollbar);
> +    virtual void unregisterScrollbar(Scrollbar* scrollbar);
> +
> +
> +};

excessive new lines, please remove.

[Kenneth Rohde Christiansen]

Comment on attachment 49524 [details]
Patch 6/7 to WebCore/platform/efl

In the future, please group the files, like the RenderThemeQt.h and RenderTheme.cpp together. I believe I saw RenderThemeQt.h in an earlier patch.

[Kenneth Rohde Christiansen]

Comment on attachment 49525 [details]
Patch 7/7 to WebCore/platform/efl

style issues according to the bot

[Leandro Pereira]

(In reply to comment #45)
> (From update of attachment 49525 [details])
> style issues according to the bot

All ports have the same issue.

[Kenneth Rohde Christiansen]

(In reply to comment #46)
> (In reply to comment #45)
> > (From update of attachment 49525 [details] [details])
> > style issues according to the bot
> 
> All ports have the same issue.

And it is not fixable?

[Leandro Pereira]

(In reply to comment #47)
> > 
> > All ports have the same issue.
> 
> And it is not fixable?
>

It is software, it's almost always fixable.  :)

The easiest way (although not the best) to fix this would be adding an exception for the build/include_order rule for platform/*/TemporaryLinkStubs.cpp.

Prototypes for methods defined in this file are in SSLKeyGenerator.h (WebCore::getSupportedKeySizes and WebCore::signedPublicKeyAndChallengeString) and SystemTime.h (WebCore::userIdleTime), both in platform/.

The proper way to fix this would be implementing things currently inside TemporaryLinkStubs.cpp in two other files: SSLKeyGenerator${PORT}.cpp and SystemTime${PORT}.cpp. Some ports already implement one and/or another, and both have their respective port-independent header files.

However, I cannot fix this in all ports, as I'll be moving things around and I'm not familiar with all build systems.

[Leandro Pereira]

Created attachment 49579 [details]
Patch 1/1 to WebCore/page/efl

[Leandro Pereira]

Created attachment 49606 [details]
Patch 2/7 to WebCore/platform/efl

[Leandro Pereira]

Comment on attachment 49606 [details]
Patch 2/7 to WebCore/platform/efl

The anonymous namespace inside CursorEfl.cpp is there so the Cursors class isn't available anywhere else. This class is used only inside this file so that all the cursors are allocated once.

[Leandro Pereira]

Created attachment 49613 [details]
Patch 3/7 to WebCore/platform/efl

[Leandro Pereira]

Created attachment 49614 [details]
Patch 4/7 to WebCore/platform/efl

[Leandro Pereira]

Created attachment 49628 [details]
Patch 1/3 to WebCore/platform/graphics/efl

[Leandro Pereira]

Created attachment 49629 [details]
Patch 2/3 to WebCore/platform/graphics/efl

[Leandro Pereira]

Created attachment 49630 [details]
Patch 3/3 to WebCore/platform/graphics/efl

[Kenneth Rohde Christiansen]

Comment on attachment 49525 [details]
Patch 7/7 to WebCore/platform/efl

Ok, I will make an exception to the style issue then. Thanks for the explanation.

[WebKit Commit Bot]

Comment on attachment 49519 [details]
Patch 1/7 to WebCore/platform/efl

Clearing flags on attachment: 49519

Committed r55328: <http://trac.webkit.org/changeset/55328>

[WebKit Commit Bot]

Comment on attachment 49523 [details]
Patch 5/7 to WebCore/platform/efl

Clearing flags on attachment: 49523

Committed r55338: <http://trac.webkit.org/changeset/55338>

[WebKit Commit Bot]

Comment on attachment 49524 [details]
Patch 6/7 to WebCore/platform/efl

Clearing flags on attachment: 49524

Committed r55340: <http://trac.webkit.org/changeset/55340>

[WebKit Commit Bot]

Comment on attachment 49525 [details]
Patch 7/7 to WebCore/platform/efl

Clearing flags on attachment: 49525

Committed r55342: <http://trac.webkit.org/changeset/55342>

[WebKit Commit Bot]

Comment on attachment 49579 [details]
Patch 1/1 to WebCore/page/efl

Clearing flags on attachment: 49579

Committed r55343: <http://trac.webkit.org/changeset/55343>

[WebKit Commit Bot]

Comment on attachment 49606 [details]
Patch 2/7 to WebCore/platform/efl

Clearing flags on attachment: 49606

Committed r55344: <http://trac.webkit.org/changeset/55344>

[WebKit Commit Bot]

Comment on attachment 49613 [details]
Patch 3/7 to WebCore/platform/efl

Clearing flags on attachment: 49613

Committed r55345: <http://trac.webkit.org/changeset/55345>

[Holger Freyther]

Comment on attachment 49614 [details]
Patch 4/7 to WebCore/platform/efl


> +#include "config.h"
> +#include "KURL.h"
> +
> +#include "CString.h"
> +
> +#include <Ecore.h>
> +
> +namespace WebCore {
> +
> +String KURL::fileSystemPath() const
> +{
> +    char* filename = (char*)m_string.utf8().data();
> +    if (!filename)
> +        return String();
> +
> +    String path = String::fromUTF8(filename);
> +    free(filename);
> +    return path;


Sorry, I stopped here reading. The memory returned by .data() will be dead shortly afterwards, calling free (filename) is certainly not right. The whole method seems to be bogus though. :)

[Nikolas Zimmermann]

I am a bit worried about the last commits. The ChangeLog says "Reviewed by Nobody". We never commit ChangeLog, w/o listing the reviewer. As the comit queue also cleared the review flags, I am unable to figure out who gave r+ to these patches.

Please fix the ChangeLogs, before landing anything else.

[Kenneth Rohde Christiansen]

(In reply to comment #66)
> I am a bit worried about the last commits. The ChangeLog says "Reviewed by
> Nobody". We never commit ChangeLog, w/o listing the reviewer. As the comit
> queue also cleared the review flags, I am unable to figure out who gave r+ to
> these patches.
> 
> Please fix the ChangeLogs, before landing anything else.

Apparently the ChangeLog was malformed and the commit queue committed it anyway without the substitution. Eric talked about rolling it out. I think that would be fine.

[Kenneth Rohde Christiansen]

I'm going to roll out the patches.

Leandro, please create some new bugs for each part of the initial landing of the port, because this bug is getting a bit out of hands

[Gustavo Noronha (kov)]

(In reply to comment #68)
> I'm going to roll out the patches.

If the problem is only the Reviewed by NOBODY, and the patches have actually been reviewed, I don't think we need to rollout. Just fixing the changelog entries should be enough.
 
> Leandro, please create some new bugs for each part of the initial landing of
> the port, because this bug is getting a bit out of hands

Agreed.

[Gustavo Noronha (kov)]

(In reply to comment #66)
> I am a bit worried about the last commits. The ChangeLog says "Reviewed by
> Nobody". We never commit ChangeLog, w/o listing the reviewer. As the comit
> queue also cleared the review flags, I am unable to figure out who gave r+ to
> these patches.

The history link in the summary of the bug keeps the r?->r+->- history, so it's verifiable that Kenneth has given r+ to the patches. I agree with fixing the changelogs so that they get merged correctly, though.

[Kenneth Rohde Christiansen]

(In reply to comment #68)
> I'm going to roll out the patches.
> 
> Leandro, please create some new bugs for each part of the initial landing of
> the port, because this bug is getting a bit out of hands

Rolling out using webkit-patch just failed misserable... I'm now back in 2008
in my history and now I have no webkit-patch as that didn't exist then :-). So
much for automated tools.

Should we rollout or just change the ChangeLogs ?

[Kenneth Rohde Christiansen]

OK, the cq didn't set the reviewed by name, as the ChangeLogs were created manually (though with spaces as required)

The issues comes from the (OOPS!!!) being left out.

[Kenneth Rohde Christiansen]

Comment on attachment 49628 [details]
Patch 1/3 to WebCore/platform/graphics/efl

r- due to ChangeLog missing the (OOPS!!!) part.

[Kenneth Rohde Christiansen]

Comment on attachment 49629 [details]
Patch 2/3 to WebCore/platform/graphics/efl

cq- due to ChangeLog missing the (OOPS!!!) part.

[Kenneth Rohde Christiansen]

Comment on attachment 49628 [details]
Patch 1/3 to WebCore/platform/graphics/efl

cq- due to ChangeLog missing the (OOPS!!!) part. The patch as such can be reviewed, but as the author is not a committer will have to be landed manually or though the commit-queue, essentially making this a r-

[Kenneth Rohde Christiansen]

Comment on attachment 49630 [details]
Patch 3/3 to WebCore/platform/graphics/efl

cq- due to ChangeLog missing the (OOPS!!!) part.

[Kenneth Rohde Christiansen]

Leandro could you please clone this bug and create other ones? You might have to clean the flags, I'm not really sure. Just check afterward if the r? patches are still showing up in the review queue

[Leandro Pereira]

(In reply to comment #77)
> Leandro could you please clone this bug and create other ones? You might have
> to clean the flags, I'm not really sure. Just check afterward if the r? patches
> are still showing up in the review queue

I've cloned this bug as bug #35531.

I'm cleaning the flags for r? patches, and then create a bug report for each directory.

[Eric Seidel (no email)]

(In reply to comment #71)
> Rolling out using webkit-patch just failed misserable... I'm now back in 2008
> in my history and now I have no webkit-patch as that didn't exist then :-). So
> much for automated tools.

I would love to know more about this failure.  "rollout" takes a revision argument.  Did you pass it a bug number instead?

The suggestion of rolling them out was mostly in response to Oliver Hunt emailing me about the bad commit.  As the administrator of the commit-queue it doesn't make much sense for me to do anything other than roll out patches when they're wrong, as I'm often completely removed from the bugs in question (and there sure are a lot of them per day!)

I think a commit to just fix the changeLog would be fine.  These aren't particularly important patches in the grand scheme of things.

[Eric Seidel (no email)]

(In reply to comment #79)
> These aren't
> particularly important patches in the grand scheme of things.

I mean no disrespect to these patches of course. :)  I just meant to point out that these only affect one (incomplete) port and do not change security issues, or core features which affect all ports.

[Kenneth Rohde Christiansen]

> I would love to know more about this failure.  "rollout" takes a revision
> argument.  Did you pass it a bug number instead?

I did pass the revision, but I believe without the r. Can that be the problem?

> The suggestion of rolling them out was mostly in response to Oliver Hunt
> emailing me about the bad commit.  As the administrator of the commit-queue it
> doesn't make much sense for me to do anything other than roll out patches when
> they're wrong, as I'm often completely removed from the bugs in question (and
> there sure are a lot of them per day!)

Yes, I agree with that.

> I think a commit to just fix the changeLog would be fine.  These aren't
> particularly important patches in the grand scheme of things.

I have done so.

[Leandro Pereira]

I am closing this bug, as other patches will have a bug report of their own.