Bug 35087 - New port: EFL; adding files to WebCore/*/efl
: New port: EFL; adding files to WebCore/*/efl
Status: RESOLVED FIXED
: WebKit
Platform
: 528+ (Nightly build)
: Other Linux
: P2 Enhancement
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-02-18 03:37 PST by
Modified: 2010-03-06 21:35 PST (History)


Attachments
Add EFL port files in WebCore/page/efl (10.35 KB, patch)
2010-02-18 03:37 PST, Leandro Pereira
oliver: review-
Review Patch | Details | Formatted Diff | Diff
Add EFL port files in WebCore/platform/efl (171.04 KB, patch)
2010-02-18 03:38 PST, Leandro Pereira
oliver: review-
Review Patch | Details | Formatted Diff | Diff
Add EFL port files in WebCore/platform/graphics/efl (75.87 KB, patch)
2010-02-18 03:39 PST, Leandro Pereira
oliver: review-
Review Patch | Details | Formatted Diff | Diff
Add EFL port files in WebCore/platform/text/efl (1.46 KB, patch)
2010-02-18 03:40 PST, Leandro Pereira
oliver: review-
Review Patch | Details | Formatted Diff | Diff
Add EFL port files in WebCore/platform/text/efl (1.92 KB, patch)
2010-02-22 03:58 PST, Leandro Pereira
no flags Review Patch | Details | Formatted Diff | Diff
Add EFL port files in WebCore/platform/graphics/efl (1.99 KB, patch)
2010-02-22 03:59 PST, Leandro Pereira
no flags Review Patch | Details | Formatted Diff | Diff
Add EFL port files in WebCore/platform/text/efl (1.99 KB, patch)
2010-02-22 04:07 PST, Leandro Pereira
kenneth: review+
commit-queue: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Add EFL port files in WebCore/platform/graphics/efl (77.29 KB, patch)
2010-02-22 10:54 PST, Leandro Pereira
no flags Review Patch | Details | Formatted Diff | Diff
Add EFL port files in WebCore/platform/efl (159.50 KB, patch)
2010-02-22 11:03 PST, Leandro Pereira
eric: review-
Review Patch | Details | Formatted Diff | Diff
Patch 1/7 to WebCore/platform/efl (24.58 KB, patch)
2010-02-25 12:34 PST, Leandro Pereira
no flags Review Patch | Details | Formatted Diff | Diff
Patch 2/7 to WebCore/platform/efl (21.33 KB, patch)
2010-02-25 12:34 PST, Leandro Pereira
no flags Review Patch | Details | Formatted Diff | Diff
Patch 3/7 to WebCore/platform/efl (22.79 KB, patch)
2010-02-25 12:35 PST, Leandro Pereira
no flags Review Patch | Details | Formatted Diff | Diff
Patch 4/7 to WebCore/platform/efl (24.98 KB, patch)
2010-02-25 12:35 PST, Leandro Pereira
no flags Review Patch | Details | Formatted Diff | Diff
Patch 5/7 to WebCore/platform/efl (22.61 KB, patch)
2010-02-25 12:36 PST, Leandro Pereira
no flags Review Patch | Details | Formatted Diff | Diff
Patch 6/7 to WebCore/platform/efl (31.79 KB, patch)
2010-02-25 12:36 PST, Leandro Pereira
no flags Review Patch | Details | Formatted Diff | Diff
Patch 7/7 to WebCore/platform/efl (13.60 KB, patch)
2010-02-25 12:37 PST, Leandro Pereira
no flags Review Patch | Details | Formatted Diff | Diff
Patch 1/1 to WebCore/page/efl (10.87 KB, patch)
2010-02-26 06:46 PST, Leandro Pereira
no flags Review Patch | Details | Formatted Diff | Diff
Patch 2/7 to WebCore/platform/efl (21.27 KB, patch)
2010-02-26 11:53 PST, Leandro Pereira
no flags Review Patch | Details | Formatted Diff | Diff
Patch 3/7 to WebCore/platform/efl (22.88 KB, patch)
2010-02-26 12:24 PST, Leandro Pereira
no flags Review Patch | Details | Formatted Diff | Diff
Patch 4/7 to WebCore/platform/efl (24.89 KB, patch)
2010-02-26 12:31 PST, Leandro Pereira
no flags Review Patch | Details | Formatted Diff | Diff
Patch 1/3 to WebCore/platform/graphics/efl (39.79 KB, patch)
2010-02-26 13:07 PST, Leandro Pereira
no flags Review Patch | Details | Formatted Diff | Diff
Patch 2/3 to WebCore/platform/graphics/efl (29.12 KB, patch)
2010-02-26 13:07 PST, Leandro Pereira
no flags Review Patch | Details | Formatted Diff | Diff
Patch 3/3 to WebCore/platform/graphics/efl (8.78 KB, patch)
2010-02-26 13:08 PST, Leandro Pereira
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-02-18 03:37:13 PST
Created an attachment (id=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 From 2010-02-18 03:38:13 PST -------
Created an attachment (id=48995) [details]
Add EFL port files in WebCore/platform/efl
------- Comment #2 From 2010-02-18 03:39:11 PST -------
Created an attachment (id=48996) [details]
Add EFL port files in WebCore/platform/graphics/efl
------- Comment #3 From 2010-02-18 03:40:15 PST -------
Created an attachment (id=48997) [details]
Add EFL port files in WebCore/platform/text/efl
------- Comment #4 From 2010-02-18 03:47:52 PST -------
Changing platform to Other / Linux.
------- Comment #5 From 2010-02-18 03:59:48 PST -------
(From update of attachment 48997 [details])
Mark patch for review.
------- Comment #6 From 2010-02-18 03:59:55 PST -------
(From update of attachment 48994 [details])
Mark patch for review.
------- Comment #7 From 2010-02-18 04:00:00 PST -------
(From update of attachment 48995 [details])
Mark patch for review.
------- Comment #8 From 2010-02-18 04:00:07 PST -------
(From update of attachment 48996 [details])
Mark patch for review.
------- Comment #9 From 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 From 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 From 2010-02-18 22:31:25 PST -------
(From update of attachment 48997 [details])
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 From 2010-02-18 22:43:45 PST -------
(From update of attachment 48996 [details])
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 From 2010-02-18 22:45:20 PST -------
(From update of attachment 48994 [details])
Only issue with this patch is the absence of a changelog
------- Comment #14 From 2010-02-18 23:05:23 PST -------
(From update of attachment 48995 [details])
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 From 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 From 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 From 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 From 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 From 2010-02-22 03:58:04 PST -------
Created an attachment (id=49200) [details]
Add EFL port files in WebCore/platform/text/efl

New patch, with ChangeLog entry.
------- Comment #20 From 2010-02-22 03:59:21 PST -------
Created an attachment (id=49201) [details]
Add EFL port files in WebCore/platform/graphics/efl

Now with the correct ChangeLog entry.
------- Comment #21 From 2010-02-22 04:05:33 PST -------
(From update of attachment 49201 [details])
> +        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 From 2010-02-22 04:07:53 PST -------
Created an attachment (id=49202) [details]
Add EFL port files in WebCore/platform/text/efl

Previous patch was submitted with wrong description.
------- Comment #23 From 2010-02-22 06:10:02 PST -------
(From update of attachment 49202 [details])
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 From 2010-02-22 10:54:57 PST -------
Created an attachment (id=49226) [details]
Add EFL port files in WebCore/platform/graphics/efl

Adding revised patch, with ChangeLog entry.
------- Comment #25 From 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 From 2010-02-22 11:03:24 PST -------
Created an attachment (id=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 From 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 From 2010-02-22 11:39:54 PST -------
(From update of attachment 49201 [details])
Clearing flags from this obsolete patch.
------- Comment #29 From 2010-02-22 11:44:46 PST -------
(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.

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 From 2010-02-22 11:45:56 PST -------
(In reply to comment #29)
> (From update of attachment 49227 [details] [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 From 2010-02-23 10:23:28 PST -------
(In reply to comment #29)
> (From update of attachment 49227 [details] [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 From 2010-02-25 12:34:30 PST -------
Created an attachment (id=49519) [details]
Patch 1/7 to WebCore/platform/efl
------- Comment #33 From 2010-02-25 12:34:56 PST -------
Created an attachment (id=49520) [details]
Patch 2/7 to WebCore/platform/efl
------- Comment #34 From 2010-02-25 12:35:21 PST -------
Created an attachment (id=49521) [details]
Patch 3/7 to WebCore/platform/efl
------- Comment #35 From 2010-02-25 12:35:52 PST -------
Created an attachment (id=49522) [details]
Patch 4/7 to WebCore/platform/efl
------- Comment #36 From 2010-02-25 12:36:26 PST -------
Created an attachment (id=49523) [details]
Patch 5/7 to WebCore/platform/efl
------- Comment #37 From 2010-02-25 12:36:56 PST -------
Created an attachment (id=49524) [details]
Patch 6/7 to WebCore/platform/efl
------- Comment #38 From 2010-02-25 12:37:32 PST -------
Created an attachment (id=49525) [details]
Patch 7/7 to WebCore/platform/efl
------- Comment #39 From 2010-02-25 12:43:15 PST -------
(From update of attachment 49519 [details])
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 From 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 From 2010-02-25 12:46:40 PST -------
(From update of attachment 49520 [details])

> +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 From 2010-02-25 12:49:26 PST -------
(From update of attachment 49521 [details])

> +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 From 2010-02-25 12:54:50 PST -------
(From update of attachment 49522 [details])

> +
> +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 From 2010-02-25 12:58:22 PST -------
(From update of attachment 49524 [details])
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 From 2010-02-25 12:59:28 PST -------
(From update of attachment 49525 [details])
style issues according to the bot
------- Comment #46 From 2010-02-25 13:15:27 PST -------
(In reply to comment #45)
> (From update of attachment 49525 [details] [details])
> style issues according to the bot

All ports have the same issue.
------- Comment #47 From 2010-02-25 16:00:53 PST -------
(In reply to comment #46)
> (In reply to comment #45)
> > (From update of attachment 49525 [details] [details] [details])
> > style issues according to the bot
> 
> All ports have the same issue.

And it is not fixable?
------- Comment #48 From 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 From 2010-02-26 06:46:55 PST -------
Created an attachment (id=49579) [details]
Patch 1/1 to WebCore/page/efl
------- Comment #50 From 2010-02-26 11:53:02 PST -------
Created an attachment (id=49606) [details]
Patch 2/7 to WebCore/platform/efl
------- Comment #51 From 2010-02-26 12:17:28 PST -------
(From update of attachment 49606 [details])
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 From 2010-02-26 12:24:54 PST -------
Created an attachment (id=49613) [details]
Patch 3/7 to WebCore/platform/efl
------- Comment #53 From 2010-02-26 12:31:36 PST -------
Created an attachment (id=49614) [details]
Patch 4/7 to WebCore/platform/efl
------- Comment #54 From 2010-02-26 13:07:10 PST -------
Created an attachment (id=49628) [details]
Patch 1/3 to WebCore/platform/graphics/efl
------- Comment #55 From 2010-02-26 13:07:46 PST -------
Created an attachment (id=49629) [details]
Patch 2/3 to WebCore/platform/graphics/efl
------- Comment #56 From 2010-02-26 13:08:21 PST -------
Created an attachment (id=49630) [details]
Patch 3/3 to WebCore/platform/graphics/efl
------- Comment #57 From 2010-02-26 13:18:19 PST -------
(From update of attachment 49525 [details])
Ok, I will make an exception to the style issue then. Thanks for the explanation.
------- Comment #58 From 2010-02-26 17:51:47 PST -------
(From update of attachment 49519 [details])
Clearing flags on attachment: 49519

Committed r55328: <http://trac.webkit.org/changeset/55328>
------- Comment #59 From 2010-02-26 19:53:30 PST -------
(From update of attachment 49523 [details])
Clearing flags on attachment: 49523

Committed r55338: <http://trac.webkit.org/changeset/55338>
------- Comment #60 From 2010-02-26 20:43:02 PST -------
(From update of attachment 49524 [details])
Clearing flags on attachment: 49524

Committed r55340: <http://trac.webkit.org/changeset/55340>
------- Comment #61 From 2010-02-26 21:16:09 PST -------
(From update of attachment 49525 [details])
Clearing flags on attachment: 49525

Committed r55342: <http://trac.webkit.org/changeset/55342>
------- Comment #62 From 2010-02-26 21:32:49 PST -------
(From update of attachment 49579 [details])
Clearing flags on attachment: 49579

Committed r55343: <http://trac.webkit.org/changeset/55343>
------- Comment #63 From 2010-02-26 21:49:29 PST -------
(From update of attachment 49606 [details])
Clearing flags on attachment: 49606

Committed r55344: <http://trac.webkit.org/changeset/55344>
------- Comment #64 From 2010-02-26 22:06:08 PST -------
(From update of attachment 49613 [details])
Clearing flags on attachment: 49613

Committed r55345: <http://trac.webkit.org/changeset/55345>
------- Comment #65 From 2010-02-26 22:41:02 PST -------
(From update of attachment 49614 [details])

> +#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 From 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 From 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 From 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 From 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 From 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 From 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 From 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 From 2010-03-01 04:46:11 PST -------
(From update of attachment 49628 [details])
r- due to ChangeLog missing the (OOPS!!!) part.
------- Comment #74 From 2010-03-01 04:46:37 PST -------
(From update of attachment 49629 [details])
cq- due to ChangeLog missing the (OOPS!!!) part.
------- Comment #75 From 2010-03-01 04:47:42 PST -------
(From update of attachment 49628 [details])
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 From 2010-03-01 04:48:03 PST -------
(From update of attachment 49630 [details])
cq- due to ChangeLog missing the (OOPS!!!) part.
------- Comment #77 From 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 From 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 From 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 From 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 From 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 From 2010-03-04 06:51:25 PST -------
I am closing this bug, as other patches will have a bug report of their own.