Bug 35087

Summary: New port: EFL; adding files to WebCore/*/efl
Product: WebKit Reporter: Leandro Pereira <leandro>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: barbieri, commit-queue, eric, gustavo, gyuyoung.kim, joone.hur, kenneth, oliver, rakuco, tonikitoo, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Attachments:
Description Flags
Add EFL port files in WebCore/page/efl
oliver: review-
Add EFL port files in WebCore/platform/efl
oliver: review-
Add EFL port files in WebCore/platform/graphics/efl
oliver: review-
Add EFL port files in WebCore/platform/text/efl
oliver: review-
Add EFL port files in WebCore/platform/text/efl
none
Add EFL port files in WebCore/platform/graphics/efl
none
Add EFL port files in WebCore/platform/text/efl
kenneth: review+, commit-queue: commit-queue-
Add EFL port files in WebCore/platform/graphics/efl
none
Add EFL port files in WebCore/platform/efl
eric: review-
Patch 1/7 to WebCore/platform/efl
none
Patch 2/7 to WebCore/platform/efl
none
Patch 3/7 to WebCore/platform/efl
none
Patch 4/7 to WebCore/platform/efl
none
Patch 5/7 to WebCore/platform/efl
none
Patch 6/7 to WebCore/platform/efl
none
Patch 7/7 to WebCore/platform/efl
none
Patch 1/1 to WebCore/page/efl
none
Patch 2/7 to WebCore/platform/efl
none
Patch 3/7 to WebCore/platform/efl
none
Patch 4/7 to WebCore/platform/efl
none
Patch 1/3 to WebCore/platform/graphics/efl
none
Patch 2/3 to WebCore/platform/graphics/efl
none
Patch 3/3 to WebCore/platform/graphics/efl none

Description Leandro Pereira 2010-02-18 03:37:13 PST
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.
Comment 1 Leandro Pereira 2010-02-18 03:38:13 PST
Created attachment 48995 [details]
Add EFL port files in WebCore/platform/efl
Comment 2 Leandro Pereira 2010-02-18 03:39:11 PST
Created attachment 48996 [details]
Add EFL port files in WebCore/platform/graphics/efl
Comment 3 Leandro Pereira 2010-02-18 03:40:15 PST
Created attachment 48997 [details]
Add EFL port files in WebCore/platform/text/efl
Comment 4 Leandro Pereira 2010-02-18 03:47:52 PST
Changing platform to Other / Linux.
Comment 5 Leandro Pereira 2010-02-18 03:59:48 PST
Comment on attachment 48997 [details]
Add EFL port files in WebCore/platform/text/efl

Mark patch for review.
Comment 6 Leandro Pereira 2010-02-18 03:59:55 PST
Comment on attachment 48994 [details]
Add EFL port files in WebCore/page/efl

Mark patch for review.
Comment 7 Leandro Pereira 2010-02-18 04:00:00 PST
Comment on attachment 48995 [details]
Add EFL port files in WebCore/platform/efl

Mark patch for review.
Comment 8 Leandro Pereira 2010-02-18 04:00:07 PST
Comment on attachment 48996 [details]
Add EFL port files in WebCore/platform/graphics/efl

Mark patch for review.
Comment 9 WebKit Review Bot 2010-02-18 04:06:16 PST
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.
Comment 10 WebKit Review Bot 2010-02-18 04:07:09 PST
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.
Comment 11 Oliver Hunt 2010-02-18 22:31:25 PST
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.
Comment 12 Oliver Hunt 2010-02-18 22:43:45 PST
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
Comment 13 Oliver Hunt 2010-02-18 22:45:20 PST
Comment on attachment 48994 [details]
Add EFL port files in WebCore/page/efl

Only issue with this patch is the absence of a changelog
Comment 14 Oliver Hunt 2010-02-18 23:05:23 PST
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.
Comment 15 Gustavo Sverzut Barbieri 2010-02-19 02:58:59 PST
(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 :-)
Comment 16 Leandro Pereira 2010-02-19 11:20:53 PST
(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.
Comment 17 Kenneth Rohde Christiansen 2010-02-19 11:23:14 PST
> 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.
Comment 18 Leandro Pereira 2010-02-19 11:26:25 PST
(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 :)
Comment 19 Leandro Pereira 2010-02-22 03:58:04 PST
Created attachment 49200 [details]
Add EFL port files in WebCore/platform/text/efl

New patch, with ChangeLog entry.
Comment 20 Leandro Pereira 2010-02-22 03:59:21 PST
Created attachment 49201 [details]
Add EFL port files in WebCore/platform/graphics/efl

Now with the correct ChangeLog entry.
Comment 21 Kenneth Rohde Christiansen 2010-02-22 04:05:33 PST
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
Comment 22 Leandro Pereira 2010-02-22 04:07:53 PST
Created attachment 49202 [details]
Add EFL port files in WebCore/platform/text/efl

Previous patch was submitted with wrong description.
Comment 23 WebKit Commit Bot 2010-02-22 06:10:02 PST
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
Comment 24 Leandro Pereira 2010-02-22 10:54:57 PST
Created attachment 49226 [details]
Add EFL port files in WebCore/platform/graphics/efl

Adding revised patch, with ChangeLog entry.
Comment 25 WebKit Review Bot 2010-02-22 11:01:23 PST
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.
Comment 26 Leandro Pereira 2010-02-22 11:03:24 PST
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.
Comment 27 WebKit Review Bot 2010-02-22 11:08:35 PST
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.
Comment 28 Eric Seidel (no email) 2010-02-22 11:39:54 PST
Comment on attachment 49201 [details]
Add EFL port files in WebCore/platform/graphics/efl

Clearing flags from this obsolete patch.
Comment 29 Eric Seidel (no email) 2010-02-22 11:44:46 PST
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.
Comment 30 Eric Seidel (no email) 2010-02-22 11:45:56 PST
(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.
Comment 31 Leandro Pereira 2010-02-23 10:23:28 PST
(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.
Comment 32 Leandro Pereira 2010-02-25 12:34:30 PST
Created attachment 49519 [details]
Patch 1/7 to WebCore/platform/efl
Comment 33 Leandro Pereira 2010-02-25 12:34:56 PST
Created attachment 49520 [details]
Patch 2/7 to WebCore/platform/efl
Comment 34 Leandro Pereira 2010-02-25 12:35:21 PST
Created attachment 49521 [details]
Patch 3/7 to WebCore/platform/efl
Comment 35 Leandro Pereira 2010-02-25 12:35:52 PST
Created attachment 49522 [details]
Patch 4/7 to WebCore/platform/efl
Comment 36 Leandro Pereira 2010-02-25 12:36:26 PST
Created attachment 49523 [details]
Patch 5/7 to WebCore/platform/efl
Comment 37 Leandro Pereira 2010-02-25 12:36:56 PST
Created attachment 49524 [details]
Patch 6/7 to WebCore/platform/efl
Comment 38 Leandro Pereira 2010-02-25 12:37:32 PST
Created attachment 49525 [details]
Patch 7/7 to WebCore/platform/efl
Comment 39 Kenneth Rohde Christiansen 2010-02-25 12:43:15 PST
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.
Comment 40 WebKit Review Bot 2010-02-25 12:44:42 PST
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.
Comment 41 Kenneth Rohde Christiansen 2010-02-25 12:46:40 PST
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?
Comment 42 Kenneth Rohde Christiansen 2010-02-25 12:49:26 PST
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?
Comment 43 Kenneth Rohde Christiansen 2010-02-25 12:54:50 PST
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.
Comment 44 Kenneth Rohde Christiansen 2010-02-25 12:58:22 PST
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.
Comment 45 Kenneth Rohde Christiansen 2010-02-25 12:59:28 PST
Comment on attachment 49525 [details]
Patch 7/7 to WebCore/platform/efl

style issues according to the bot
Comment 46 Leandro Pereira 2010-02-25 13:15:27 PST
(In reply to comment #45)
> (From update of attachment 49525 [details])
> style issues according to the bot

All ports have the same issue.
Comment 47 Kenneth Rohde Christiansen 2010-02-25 16:00:53 PST
(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?
Comment 48 Leandro Pereira 2010-02-26 06:39:57 PST
(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.
Comment 49 Leandro Pereira 2010-02-26 06:46:55 PST
Created attachment 49579 [details]
Patch 1/1 to WebCore/page/efl
Comment 50 Leandro Pereira 2010-02-26 11:53:02 PST
Created attachment 49606 [details]
Patch 2/7 to WebCore/platform/efl
Comment 51 Leandro Pereira 2010-02-26 12:17:28 PST
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.
Comment 52 Leandro Pereira 2010-02-26 12:24:54 PST
Created attachment 49613 [details]
Patch 3/7 to WebCore/platform/efl
Comment 53 Leandro Pereira 2010-02-26 12:31:36 PST
Created attachment 49614 [details]
Patch 4/7 to WebCore/platform/efl
Comment 54 Leandro Pereira 2010-02-26 13:07:10 PST
Created attachment 49628 [details]
Patch 1/3 to WebCore/platform/graphics/efl
Comment 55 Leandro Pereira 2010-02-26 13:07:46 PST
Created attachment 49629 [details]
Patch 2/3 to WebCore/platform/graphics/efl
Comment 56 Leandro Pereira 2010-02-26 13:08:21 PST
Created attachment 49630 [details]
Patch 3/3 to WebCore/platform/graphics/efl
Comment 57 Kenneth Rohde Christiansen 2010-02-26 13:18:19 PST
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.
Comment 58 WebKit Commit Bot 2010-02-26 17:51:47 PST
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>
Comment 59 WebKit Commit Bot 2010-02-26 19:53:30 PST
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>
Comment 60 WebKit Commit Bot 2010-02-26 20:43:02 PST
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>
Comment 61 WebKit Commit Bot 2010-02-26 21:16:09 PST
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>
Comment 62 WebKit Commit Bot 2010-02-26 21:32:49 PST
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>
Comment 63 WebKit Commit Bot 2010-02-26 21:49:29 PST
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>
Comment 64 WebKit Commit Bot 2010-02-26 22:06:08 PST
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>
Comment 65 Holger Freyther 2010-02-26 22:41:02 PST
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. :)
Comment 66 Nikolas Zimmermann 2010-02-27 04:42:45 PST
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.
Comment 67 Kenneth Rohde Christiansen 2010-03-01 03:58:49 PST
(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.
Comment 68 Kenneth Rohde Christiansen 2010-03-01 04:17:22 PST
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
Comment 69 Gustavo Noronha (kov) 2010-03-01 04:34:51 PST
(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.
Comment 70 Gustavo Noronha (kov) 2010-03-01 04:37:17 PST
(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.
Comment 71 Kenneth Rohde Christiansen 2010-03-01 04:37:50 PST
(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 ?
Comment 72 Kenneth Rohde Christiansen 2010-03-01 04:43:28 PST
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.
Comment 73 Kenneth Rohde Christiansen 2010-03-01 04:46:11 PST
Comment on attachment 49628 [details]
Patch 1/3 to WebCore/platform/graphics/efl

r- due to ChangeLog missing the (OOPS!!!) part.
Comment 74 Kenneth Rohde Christiansen 2010-03-01 04:46:37 PST
Comment on attachment 49629 [details]
Patch 2/3 to WebCore/platform/graphics/efl

cq- due to ChangeLog missing the (OOPS!!!) part.
Comment 75 Kenneth Rohde Christiansen 2010-03-01 04:47:42 PST
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-
Comment 76 Kenneth Rohde Christiansen 2010-03-01 04:48:03 PST
Comment on attachment 49630 [details]
Patch 3/3 to WebCore/platform/graphics/efl

cq- due to ChangeLog missing the (OOPS!!!) part.
Comment 77 Kenneth Rohde Christiansen 2010-03-01 04:58:22 PST
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
Comment 78 Leandro Pereira 2010-03-01 09:57:52 PST
(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.
Comment 79 Eric Seidel (no email) 2010-03-01 10:40:12 PST
(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.
Comment 80 Eric Seidel (no email) 2010-03-01 10:42:33 PST
(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.
Comment 81 Kenneth Rohde Christiansen 2010-03-01 12:19:37 PST
> 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.
Comment 82 Leandro Pereira 2010-03-04 06:51:25 PST
I am closing this bug, as other patches will have a bug report of their own.