Bug 35084 - New port: EFL; adding files to JavaScriptCore/wtf/efl
Summary: New port: EFL; adding files to JavaScriptCore/wtf/efl
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-18 03:14 PST by Leandro Pereira
Modified: 2010-03-06 21:33 PST (History)
12 users (show)

See Also:


Attachments
Add EFL port files in JavaScriptCore/efl (6.90 KB, patch)
2010-02-18 03:14 PST, Leandro Pereira
oliver: review-
Details | Formatted Diff | Diff
Patch, with requested changes (6.28 KB, patch)
2010-02-19 05:44 PST, Leandro Pereira
no flags Details | Formatted Diff | Diff
Remove EOwnPtr.h and move GOwnPtr/GRefPtr to wtf/gobject (28.27 KB, patch)
2010-02-22 03:20 PST, Leandro Pereira
kenneth: review+
Details | Formatted Diff | Diff
Remove EOwnPtr.h and move GOwnPtr/GRefPtr to wtf/gobject (26.82 KB, patch)
2010-02-23 05:54 PST, Leandro Pereira
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Leandro Pereira 2010-02-18 03:14:40 PST
Created attachment 48993 [details]
Add EFL port files in JavaScriptCore/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 JavaScriptCore/wtf/efl directory. Changes
to the port-independent parts and build system changes, will come later.

(See bug #35059 for the first patch in this series.)

It should apply cleanly on SVN rev 54651.
Comment 1 Leandro Pereira 2010-02-18 03:48:44 PST
Changing platform to Other / Linux.
Comment 2 Leandro Pereira 2010-02-18 03:59:36 PST
Comment on attachment 48993 [details]
Add EFL port files in JavaScriptCore/efl

Mark patch for review.
Comment 3 Oliver Hunt 2010-02-18 22:28:01 PST
Comment on attachment 48993 [details]
Add EFL port files in JavaScriptCore/efl

Welcome to WebKit :D

This patch is basically fine, but you shouldn't add JavaScriptCore/wtf/efl/EOwnPtr.cpp as it's an (effectively) empty file.

You also need a changelog, to create it use the prepare-ChangeLog script, and add appropriate description.

I'll r- this patch and wait for an updated patch with a changelog and the superfluous file removed.
Comment 4 Leandro Pereira 2010-02-19 05:44:22 PST
Created attachment 49069 [details]
Patch, with requested changes
Comment 5 Leandro Pereira 2010-02-19 08:40:28 PST
Comment on attachment 49069 [details]
Patch, with requested changes

Marking patch for review.
Comment 6 Kenneth Rohde Christiansen 2010-02-19 08:56:28 PST
Comment on attachment 49069 [details]
Patch, with requested changes

So what do you use the EOwnPtr for ? basically it is a define for the GOwnPtr that works together with the GObject I suppose.

Also the method freeOwnerGPtr does nothing so far. It actually just contains out commented code.

I think it would be nice to fix this before committing the patch, or leaving out the EOwnPtr for now.

Could you please clarify this?
Comment 7 Leandro Pereira 2010-02-19 09:19:46 PST
(In reply to comment #6)
> (From update of attachment 49069 [details])
> So what do you use the EOwnPtr for ? basically it is a define for the GOwnPtr
> that works together with the GObject I suppose.
> 

Currently the soup network backend includes GOwnPtr, which is only defined in JSC/wtf/gtk. Since GOwnPtr is used elsewhere throughout the GTK+ port and it doesn't actually rely on GTK+, I don't know what to do.

The EFL port currently copies files from other ports. Its been like this since it was first developed, and we're rewriting/using common implementations as much as time permits. It is on our todo list to improve this gradually, though.
Comment 8 Gustavo Noronha (kov) 2010-02-19 09:57:53 PST
(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 49069 [details] [details])
> > So what do you use the EOwnPtr for ? basically it is a define for the GOwnPtr
> > that works together with the GObject I suppose.
> > 
> 
> Currently the soup network backend includes GOwnPtr, which is only defined in
> JSC/wtf/gtk. Since GOwnPtr is used elsewhere throughout the GTK+ port and it
> doesn't actually rely on GTK+, I don't know what to do.
> 
> The EFL port currently copies files from other ports. Its been like this since
> it was first developed, and we're rewriting/using common implementations as
> much as time permits. It is on our todo list to improve this gradually, though.

As we discussed briefly in #webkit-gtk, we go out of our way to make sure JSC does not depend on GTK+, so GOwnPtr is only related to gobject, not GTK+. I believe it is fine to rename JavaScriptCore/wtf/gtk to JavaScriptCore/wtf/gobject, to make it clearer that it is not specific to the GTK+ port, but to any gobjects-using port.

I'm not sure whether you will use our Threading, and MainThread implementations, though, but feel free to move GOwnPtr, and GRefPtr to a gobject directory (as long as you fix the paths in our build system, it won't be a problem for us at all).
Comment 9 Kenneth Rohde Christiansen 2010-02-19 10:00:48 PST
Should that be gobject or glib?

> As we discussed briefly in #webkit-gtk, we go out of our way to make sure JSC
> does not depend on GTK+, so GOwnPtr is only related to gobject, not GTK+. I
> believe it is fine to rename JavaScriptCore/wtf/gtk to
> JavaScriptCore/wtf/gobject, to make it clearer that it is not specific to the
> GTK+ port, but to any gobjects-using port.
> 
> I'm not sure whether you will use our Threading, and MainThread
> implementations, though, but feel free to move GOwnPtr, and GRefPtr to a
> gobject directory (as long as you fix the paths in our build system, it won't
> be a problem for us at all).
Comment 10 Gustavo Noronha (kov) 2010-02-19 10:43:10 PST
(In reply to comment #9)
> Should that be gobject or glib?

glib does sound more on the mark, tbh, but I don't care either way
Comment 11 Leandro Pereira 2010-02-19 11:09:07 PST
(In reply to comment #10)
> (In reply to comment #9)
> > Should that be gobject or glib?
> 
> glib does sound more on the mark, tbh, but I don't care either way

We do have our own implementation of MainThread and other ports (if they ever depend on the soup backend) might have it also, so I propose the following:

1) let MainThreadGtk.cpp and ThreadingGtk.cpp inside JSC/wtf/gtk
2) move GOwnPtr.{cpp,h} and GRefPtr.{cpp,h} to JSC/wtf/glib
3) change GTK+'s build system to reflect this change
Comment 12 Xan Lopez 2010-02-19 13:57:33 PST
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Should that be gobject or glib?
> > 
> > glib does sound more on the mark, tbh, but I don't care either way
> 
> We do have our own implementation of MainThread and other ports (if they ever
> depend on the soup backend) might have it also, so I propose the following:
> 
> 1) let MainThreadGtk.cpp and ThreadingGtk.cpp inside JSC/wtf/gtk
> 2) move GOwnPtr.{cpp,h} and GRefPtr.{cpp,h} to JSC/wtf/glib
> 3) change GTK+'s build system to reflect this change

This seems sane. I'd say 'gobject' sounds better to describe a "platform" than 'glib', since you can use glib without gobject but what really defines the platform is the latter.
Comment 13 Leandro Pereira 2010-02-22 03:20:33 PST
Created attachment 49195 [details]
Remove EOwnPtr.h and move GOwnPtr/GRefPtr to wtf/gobject
Comment 14 Kenneth Rohde Christiansen 2010-02-22 05:14:36 PST
Comment on attachment 49195 [details]
Remove EOwnPtr.h and move GOwnPtr/GRefPtr to wtf/gobject

Eric, if this is landed with the commit-queue, will it show the files as moved?
Comment 15 Eric Seidel (no email) 2010-02-22 10:45:12 PST
What's an EFL port?

It would be best if you could send a note to webkit-dev introducing yourself and your port and explain who is going to maintain the port and why WebKit should accept the port into our tree.

You might also want to read:
http://trac.webkit.org/wiki/SuccessfulPortHowTo

Maybe you already sent a note and I missed it?

It's very rare that webkit "rejects" a port, but it is important that behind each port there be a real maintenance effort, as poorly maintained partial ports have fallen under the "science project" non-goal and been removed after being abandoned in the past.  (Again, this is rare.)
https://webkit.org/projects/goals.html

As Oliver said, welcome to WebKit!
Comment 16 Kenneth Rohde Christiansen 2010-02-22 10:58:09 PST
(In reply to comment #15)
> What's an EFL port?
> 
> It would be best if you could send a note to webkit-dev introducing yourself
> and your port and explain who is going to maintain the port and why WebKit
> should accept the port into our tree.

> Maybe you already sent a note and I missed it?


I believe that it was actually announced on the mailing list.
Comment 17 Gustavo Noronha (kov) 2010-02-22 15:18:05 PST
(In reply to comment #14)
> (From update of attachment 49195 [details])
> Eric, if this is landed with the commit-queue, will it show the files as moved?

Subversion is kind of sucky in tracking file movements inside the repository. I wouldn't worry about this. Git will save us all, if we need this at a future point.
Comment 18 Gustavo Noronha (kov) 2010-02-22 15:21:24 PST
(In reply to comment #17)
> (In reply to comment #14)
> > (From update of attachment 49195 [details] [details])
> > Eric, if this is landed with the commit-queue, will it show the files as moved?
> 
> Subversion is kind of sucky in tracking file movements inside the repository. I
> wouldn't worry about this. Git will save us all, if we need this at a future
> point.

Having said that, the EWS scripts didn't like the patch, so I think it's unlikely that the cq will be able to handle it:

https://webkit-commit-queue.appspot.com/results/298648

You may need to rebase the patch on trunk.
Comment 19 Leandro Pereira 2010-02-23 05:54:13 PST
Created attachment 49282 [details]
Remove EOwnPtr.h and move GOwnPtr/GRefPtr to wtf/gobject

Patch updated to SVN HEAD.
Comment 20 WebKit Review Bot 2010-02-23 05:57:52 PST
Attachment 49282 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Skipping input 'JavaScriptCore/wtf/gtk/GRefPtr.h': Can't open for reading
JavaScriptCore/wtf/gobject/GRefPtr.h:23:  #ifndef header guard has wrong style, please use: GRefPtr_h  [build/header_guard] [5]
JavaScriptCore/wtf/gobject/GRefPtr.h:44:  More than one command on the same line in if  [whitespace/parens] [4]
JavaScriptCore/wtf/gobject/GRefPtr.h:45:  More than one command on the same line in if  [whitespace/parens] [4]
JavaScriptCore/wtf/gobject/GRefPtr.h:46:  More than one command on the same line in if  [whitespace/parens] [4]
JavaScriptCore/wtf/gobject/GRefPtr.h:48:  More than one command on the same line in if  [whitespace/parens] [4]
Skipping input 'JavaScriptCore/wtf/gtk/GOwnPtr.h': Can't open for reading
Skipping input 'JavaScriptCore/wtf/gtk/GOwnPtr.cpp': Can't open for reading
Skipping input 'JavaScriptCore/wtf/gtk/GRefPtr.cpp': Can't open for reading
Total errors found: 5 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 WebKit Review Bot 2010-02-23 05:59:39 PST
Attachment 49282 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/299722
Comment 22 WebKit Commit Bot 2010-02-23 06:22:01 PST
Comment on attachment 49282 [details]
Remove EOwnPtr.h and move GOwnPtr/GRefPtr to wtf/gobject

Clearing flags on attachment: 49282

Committed r55149: <http://trac.webkit.org/changeset/55149>
Comment 23 WebKit Commit Bot 2010-02-23 06:22:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 24 Xan Lopez 2010-02-23 06:30:37 PST
As EWS said in comment #21, this broke al GTK+ bots.
Comment 25 Kenneth Rohde Christiansen 2010-02-23 06:37:15 PST
../../WebKit/gtk/webkit/webkitprivate.h:51:21: error: GOwnPtr.h: No such file or directory, seems to be the culprit
Comment 26 Kenneth Rohde Christiansen 2010-02-23 06:40:47 PST
I guess all these needs fixing:

JavaScriptCore/JavaScriptCore.gypi:            'wtf/gtk/GOwnPtr.cpp',
JavaScriptCore/JavaScriptCore.gypi:            'wtf/gtk/GOwnPtr.h',
JavaScriptCore/JavaScriptCore.gypi:            'wtf/gtk/MainThreadGtk.cpp',
JavaScriptCore/JavaScriptCore.gypi:            'wtf/gtk/ThreadingGtk.cpp',
JavaScriptCore/wtf/Threading.h:#include <wtf/gtk/GOwnPtr.h>
JavaScriptCore/wtf/unicode/glib/UnicodeGLib.h:#include <wtf/gtk/GOwnPtr.h>
WebCore/platform/KURL.cpp:#include <wtf/gtk/GOwnPtr.h>
WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:#include <wtf/gtk/GOwnPtr.h>
WebCore/platform/gtk/DataObjectGtk.h:#include <wtf/gtk/GRefPtr.h>
WebCore/platform/text/TextEncoding.cpp:#include <wtf/gtk/GOwnPtr.h>
WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:#include <wtf/gtk/GOwnPtr.h>
WebCore/platform/text/gtk/TextCodecGtk.cpp:#include <wtf/gtk/GOwnPtr.h>
WebKit/gtk/webkit/webkitwebview.cpp:#include <wtf/gtk/GOwnPtr.h>

Xan, feel free to rollout.
Comment 27 Gustavo Noronha (kov) 2010-02-23 07:39:24 PST
(In reply to comment #24)
> As EWS said in comment #21, this broke al GTK+ bots.

Would be good to have the cq look at EWS bots before doing the commit, for these patches.
Comment 28 Gustavo Noronha (kov) 2010-02-23 07:41:53 PST
(In reply to comment #26)
> I guess all these needs fixing:
> 
> JavaScriptCore/JavaScriptCore.gypi:            'wtf/gtk/GOwnPtr.cpp',
> JavaScriptCore/JavaScriptCore.gypi:            'wtf/gtk/GOwnPtr.h',
> JavaScriptCore/JavaScriptCore.gypi:            'wtf/gtk/MainThreadGtk.cpp',
> JavaScriptCore/JavaScriptCore.gypi:            'wtf/gtk/ThreadingGtk.cpp',
> JavaScriptCore/wtf/Threading.h:#include <wtf/gtk/GOwnPtr.h>
> JavaScriptCore/wtf/unicode/glib/UnicodeGLib.h:#include <wtf/gtk/GOwnPtr.h>
> WebCore/platform/KURL.cpp:#include <wtf/gtk/GOwnPtr.h>
> WebCore/platform/graphics/gtk/MediaPlayerPrivateGStreamer.cpp:#include
> <wtf/gtk/GOwnPtr.h>
> WebCore/platform/gtk/DataObjectGtk.h:#include <wtf/gtk/GRefPtr.h>
> WebCore/platform/text/TextEncoding.cpp:#include <wtf/gtk/GOwnPtr.h>
> WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:#include <wtf/gtk/GOwnPtr.h>
> WebCore/platform/text/gtk/TextCodecGtk.cpp:#include <wtf/gtk/GOwnPtr.h>
> WebKit/gtk/webkit/webkitwebview.cpp:#include <wtf/gtk/GOwnPtr.h>

Most of the ones that matter for GTK+'s main configuration have been fixed (we focused on those to get the bots building again before today's commit stream opens up =D). I believe Leandro will work on a follow-up patch that fixes all the other usages.
Comment 29 Leandro Pereira 2010-02-23 09:57:25 PST
(In reply to comment #28)
> Most of the ones that matter for GTK+'s main configuration have been fixed (we
> focused on those to get the bots building again before today's commit stream
> opens up =D). I believe Leandro will work on a follow-up patch that fixes all
> the other usages.

Fixes were committed to SVN r55158.
Comment 30 Eric Seidel (no email) 2010-02-23 12:03:53 PST
The EWS bots stop processing after r+ is set.  So they're necessarily done before r+ is set if they're done at all. :)  EWS bots are really for reviewers. The commit-queue is its own separate beast.  They could be tied, but right now they aren't (intentionally).