Bug 35084

Changing platform to Other / Linux.

Comment on attachment 48993 [details] Add EFL port files in JavaScriptCore/efl Mark patch for review.

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.

Created attachment 49069 [details] Patch, with requested changes

Comment on attachment 49069 [details] Patch, with requested changes Marking patch for review.

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?

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

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

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

(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

(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

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

Created attachment 49195 [details] Remove EOwnPtr.h and move GOwnPtr/GRefPtr to wtf/gobject

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?

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!

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

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

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

Created attachment 49282 [details] Remove EOwnPtr.h and move GOwnPtr/GRefPtr to wtf/gobject Patch updated to SVN HEAD.

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.

Attachment 49282 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/299722

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>

All reviewed patches have been landed. Closing bug.

As EWS said in comment #21, this broke al GTK+ bots.

../../WebKit/gtk/webkit/webkitprivate.h:51:21: error: GOwnPtr.h: No such file or directory, seems to be the culprit

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.

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

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

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

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