Bug 32452 – Port of v8 to work with Gtk webkit

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKitGTK

[Michael Emmel]

This is a large set of changes that allows the v8 js engine to work with the Gtk port including support for the chromedevtools debugger.

Its huge so I'm also posting to the mailing list looking for help on it.

[Michael Emmel]

Created attachment 44710 [details]
Patch to add support to V8

[Adam Barth]

From a brief look, i don't understand

- v8::Signature::New
+ v8::Signature::V8New

In general, WebCore isn't supposed to muck around with stuff in the v8:: namespace.  Why do we need to qualify this identifier if it's already properly scoped to V8?

[Michael Emmel]

(In reply to comment #2)
> From a brief look, i don't understand
> 
> - v8::Signature::New
> + v8::Signature::V8New
> 
> In general, WebCore isn't supposed to muck around with stuff in the v8::
> namespace.  Why do we need to qualify this identifier if it's already properly
> scoped to V8?

Well I filed a bug here.

http://code.google.com/p/v8/issues/detail?id=448&can=1&q=gtk&colspec=ID%20Type%20Status%20Priority%20Owner%20Summary%20HW%20OS%20Area%20Stars

It was rejected.

The problem is it turns into a nightmare to include V8 with Gtk which eventually includes X11 headers. One of the biggest issues is of course this. There is no real way around outside of a perhaps a full refactoring of the gtk port itself.

The problem of course is the C preprocessor trumps C++ namespaces.
I'd argue its wrong for the C preprocessor to do substitution inside a name-space declaration but thats probably a even larger can of worms.

[James Robinson]

Why did you neuter WebCore/bindings/v8/DerivedSourcesAllInOne.cpp ?

[Dimitri Glazkov (Google)]

Mike, is there a chance we could break this down into smaller patches? The big'un provides a good overview, but it's unlikely we'll be able to get this right when landing. Especially considering you're making changes to code that touches both Chromium and GTK ports.

[Adam Barth]

Comment on attachment 44710 [details]
Patch to add support to V8

I don't think we want to maintain a patch file against V8.  If you can't get the V8 project to rename their identifiers to avoid the C pre-processor, then we'll have to find another solution than doing it guerilla-style.

[Michael Emmel]

(In reply to comment #5)
> Mike, is there a chance we could break this down into smaller patches? The
> big'un provides a good overview, but it's unlikely we'll be able to get this
> right when landing. Especially considering you're making changes to code that
> touches both Chromium and GTK ports.

Trust me this is just a first go getting it out in public.

If someone wants to help I prob need a git server or something to work through all the changes.
I had no expectation for this to be landed. Plus of course it has patches to v8 and chromedevtools.

Obviously I'm trolling for help from google and other intrested parties on this :)

So think of it as a plea for help not a patch :)

[Michael Emmel]

(In reply to comment #6)
> (From update of attachment 44710 [details])
> I don't think we want to maintain a patch file against V8.  If you can't get
> the V8 project to rename their identifiers to avoid the C pre-processor, then
> we'll have to find another solution than doing it guerilla-style.

Well number one as far as V8 is concerned you can see the disaster develop if you reverted the renaming. However if you think about how headers are included esp given the X11 one are deep in gtk you can see why I found it rapidly turned into a hopeless mess. With the patch at least someone else could verify that not doing the renaming results in a intractable mess.

And yes the X11 developers who are responsible should be beaten if they are not retired but .. Its not like C does not support enums.

So thats the first hurdle.

The debugging support could readily be done as a sister patch but here I have changes to chromdevtools and also code seriously reworked from chromium itself along with some headers from chromium.

And this introduces a JSON native parser which should probably be bumped into some generic place to support native JSON.

Potentially lots of good stuff but as I said I need help.

[Adam Barth]

Is there some reason #undef can't solve the pre-processor problem?  I'd be very surprised if V8 took the renaming patch.  That sounds like the biggest problem you'll need to solve to unblock this effort.

[Michael Emmel]

(In reply to comment #9)
> Is there some reason #undef can't solve the pre-processor problem?  I'd be very
> surprised if V8 took the renaming patch.  That sounds like the biggest problem
> you'll need to solve to unblock this effort.

Yes I don't control the header include order.
the undef hack only works with simple include cycles the more complex ones make it practically impossible to insert a undef in the correct place.

I would not have done what I did if it was possible to do it a different way in any sane manner. Consider the header import  graph and I hope you can see it ends in cycles that make cutting impossible without significant refactoring of the code base which also would block the effort.

Its a catch-22 situation.

Its fairly easy to cause the problems simply reverse the definitions a second time in the v8.h header.

I can do that and post the errors.

[Michael Emmel]

One more comment I believe the Chromium team which worked with V8 took care to manage where the V8 headers where imported and how there code was arranged.

Perhaps someone from that team can chime in if so.

Thats not a realistic option if your integrating V8 into a large existing code base. My opinion right now is either you have to do some ugly hacks like I did or only use it in a project designed with the library in mind.

Neither are in my opinion good solutions for a very useful portable library.

And obviously I want to see v8 used more often.

[James Robinson]

What parts of the WebKit Gtk codebase need to interface directly with both V8 and X11?

[Michael Emmel]

I thought of one more thing to add.

In C++ generally you don't import dependencies in header files instead you use class declaration and import the correct header in the source while in C its the opposite and you have the big header graph. This difference between the languages is what makes simple undef solutions very hard to implement esp given the best place to put the undef would be in a system header which is not viable.

Heck if you prefix with V8 then the using shortcut in C++ works 99% of the time and its v8:: vs V8 two less chars to type and if you look I toggle using the predefines its optional so you don't have to use the prefixes only when you hit the situation I hit and had to resort to prefix's.

I'm by no means happy with what I had to do its just when you mix C/C++ sometimes the situation is not pretty.

[Michael Emmel]

(In reply to comment #12)
> What parts of the WebKit Gtk codebase need to interface directly with both V8
> and X11?

Eventually basically all of it since it imports gdk.

Proabably the pc file gives the best indication they are eventually imported intrinsically by most of the headers.

Name: GDK
Description: GTK+ Drawing Kit (${target} target)
Version: 2.18.3
Requires: gdk-pixbuf-2.0 pango pangocairo gio-2.0
Requires.private: fontconfig x11 xext xrender xinerama xi xrandr xcursor xfixes xcomposite xdamage cairo-xlib
Libs: -L${libdir} -lgdk-${target}-2.0
Cflags: -I${includedir}/gtk-2.0 -I${libdir}/gtk-2.0/include

[Michael Emmel]

I turned on turning off renaming I'll attach the output from the build.
Its been a while since I tried so this will help show the problem areas.

I'll give putting in undefs another try and see if I hit a catch22 case with the current code base.

Well it failed already. 

Ok I'll put something together showing how to test the build without the renaming.

But tomorrow I'm hungry and tired.

[Adam Barth]

There's at least an existence proof that it's possible because Chrome is a Gtk app and uses V8.  I don't mean to be negative.  I think it would be really cool to get this working.  I just want to make sure you're heading down a path that has a light at the end of the tunnel.

[Michael Emmel]

(In reply to comment #16)
> There's at least an existence proof that it's possible because Chrome is a Gtk
> app and uses V8.  I don't mean to be negative.  I think it would be really cool
> to get this working.  I just want to make sure you're heading down a path that
> has a light at the end of the tunnel.

No thats not correct the Gtk headers in Chromium are hidden deep behind a porting api and never interact with V8. There are no cross cases.

Obviously I've been through a significant amount of the Chromium code to pull out the debugging support so its important for someone from that team to explain why Chromium actually is not a Gtk application.

The closest equivalent to Chromium would probably be wxWidgets. In both cases the platform headers are fully insulated behind a comprehensive porting api.

To some extent you have to trust that I'm not doing this just because I want to be a pain in the ass its a serious problem.

I know in detail how chromium was constructed and designed. And thats not even including the ipc interfaces in arguing they are not even close to being a gtk app. Not only is gtk not exposed but its completely shielded via the ipc container.

[Michael Emmel]

Created attachment 44722 [details]
This patch reverses for the most part using prefixes


If this patch is applied you can generate the name collision errors and find that they become increasingly difficult to correct.

[Evan Martin]

BTW, the v8 conflicting with X headers thing has also been a problem for Chrome -- we have a few files where we have to jack around the include order.  (Now that I write this I am unclear where a file would use v8 and X11 at the same time, but IIRC it is because plugins use NPAPI which needs X and they also call v8 functions.)

It would have been nice for X to not have used True and False and if you have a time machine to fix that 10 years ago I would do it.  Since we can control where v8 is included, one option might be to "#define True V8True" around where you include that.

GTK normally doesn't need X headers; it's only gdkx.h that does.  So you could also do ifdef hacks around in the files that only touch those two.

[Evan Martin]

And yes, comment #17 is correct -- Chrome's Linux WebKit port doesn't use GTK.

[Evan Martin]

It would help me review this if you could post your git tree somewhere such that I could pull it.  repo.or.cz or github.com work.

[Michael Emmel]

(In reply to comment #19)
> BTW, the v8 conflicting with X headers thing has also been a problem for Chrome
> -- we have a few files where we have to jack around the include order.  (Now
> that I write this I am unclear where a file would use v8 and X11 at the same
> time, but IIRC it is because plugins use NPAPI which needs X and they also call
> v8 functions.)
> 
> It would have been nice for X to not have used True and False and if you have a
> time machine to fix that 10 years ago I would do it.  Since we can control
> where v8 is included, one option might be to "#define True V8True" around where
> you include that.
> 
> GTK normally doesn't need X headers; it's only gdkx.h that does.  So you could
> also do ifdef hacks around in the files that only touch those two.

Correct the NSAPI header had to be hacked in Chrome because of this. Also this is the one place in Chrome where you have collision.

As to your next statement thats not correct "GTK normally doesn't need X headers"
they are certainly included in the deep include chain in gtk. Actually gdk but its a chain of includes that lead to X11.

[Michael Emmel]

(In reply to comment #21)
> It would help me review this if you could post your git tree somewhere such
> that I could pull it.  repo.or.cz or github.com work.

Sure I could push it to one of those.

I think thats the best answer. Assuming no one has a strong preference I'll just pick one of those two and post the URL. Probably monday.

[Michael Emmel]

(In reply to comment #23)
> (In reply to comment #21)
> > It would help me review this if you could post your git tree somewhere such
> > that I could pull it.  repo.or.cz or github.com work.
> 
> Sure I could push it to one of those.
> 
> I think thats the best answer. Assuming no one has a strong preference I'll
> just pick one of those two and post the URL. Probably monday.

Just an update this is taking a bit longer than I anticipated. I had to take my kids to the doctor today for checkups then I had a lot of rejects patching the latest pull.

When its ready it will be here.
http://repo.or.cz/w/webbrowser.git

Should be tomorrow.

[Michael Emmel]

(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #21)
> > > It would help me review this if you could post your git tree somewhere such
> > > that I could pull it.  repo.or.cz or github.com work.
> > 
> > Sure I could push it to one of those.
> > 
> > I think thats the best answer. Assuming no one has a strong preference I'll
> > just pick one of those two and post the URL. Probably monday.
> 
> Just an update this is taking a bit longer than I anticipated. I had to take my
> kids to the doctor today for checkups then I had a lot of rejects patching the
> latest pull.
> 
> When its ready it will be here.
> http://repo.or.cz/w/webbrowser.git
> 
> Should be tomorrow.

Okay the branch is in who ever created this patch is insane ohh wait ...

Its at http://repo.or.cz/w/webbrowser.git on branch v8
And reached the stage of compiles for me and runs www.google.com

Should be enough for people interested in the work to play with it.

[Michael Emmel]

Hmm no feedback.

I'm now working on a proposed refactoring of the GNUMakefile to make them easier to modify more flexible. This will result in two patches. One showing the reorganization and the second with the actual v8 changes applied.

Hopefully this makes it easier to manage optional parts of the build for Gtk.
The assumption is the v8 option is probably the most aggressive change therefor if I can reorg where clean patches are easy to create for new features then its a useful change.

Next assuming no other feedback I'll attempt to create at least a partial alternative path using undef to try and include v8 headers without renaming.
The only obvious way I can think to do this is to wrap the offending native headers with one that does the undefs. This is the approach used inside WebKit in

npruntime_internal.h

So I'll create gtk_internal.h cairo_internal.h etc and see what happens.
Of course this was the approach I took originally before giving up but I don't see I have much choice but to offer a sample patch. There is a chance since I now have a functional build using renaming that I can find a simple way to remove renaming. Where when I first was trying to port all the other issues made it hard to come up with a clean approach.

Best case is it works worst case is at least I can provide a patch to help highlight the issues.

I'm certainly open to feedback and suggestions lacking that I'll do my best to try and present the alternatives.

[Michael Emmel]

Created attachment 45113 [details]
Another way to generate naming conflicts


Ok I though of another way to generate naming conflicts assuming the use of undef.

In this approach I use -include to force the conflicting headers to be include in general with attempts 
to use undef to resolve the conflicts.

This generates yet another set of problems with that approach.

I'm now back to suggesting that the assumption of C++ namespaces as sufficient to protect the use of common names is not a good one in general.

So your left with either removing the conflict via renaming or a case by case refactoring often in seemingly unrelated headers.

I'm back to my generic observation that a library should try and be as unobtrusive as possible and assume its own headers that its being included in unknown projects and do its best to minimize naming conflicts this would include the use of reasonable old style C prefixes to handle preprocessor issues in its public headers.

Certainly this rule is broken by ancient code but the ancient code is one of the reasons for the development of C++ namespaces in the first place, because of C namespace conflicts. However the old code is still around causing problems.

[Evan Martin]

(Sorry, in the middle of something else right now -- I hope to look at this after Christmas...)

[Michael Emmel]

(In reply to comment #5)
> Mike, is there a chance we could break this down into smaller patches? The
> big'un provides a good overview, but it's unlikely we'll be able to get this
> right when landing. Especially considering you're making changes to code that
> touches both Chromium and GTK ports.

Sorry I just saw your post. I'm having a compile problem with it.

It has to do with how the Gtk makefile works its one of the things I'm working on now. The problem is that derived sources are compiled and linked in using a general rule and I need to split this out. Once done I should be able to bring it back. Given all the other issues its on the lower end of the agenda at the moment. But something I'll fix actually as a side effect of my makefile refactoring. Mental sticky note added.

[Michael Emmel]

(In reply to comment #29)
> (In reply to comment #5)
> > Mike, is there a chance we could break this down into smaller patches? The
> > big'un provides a good overview, but it's unlikely we'll be able to get this
> > right when landing. Especially considering you're making changes to code that
> > touches both Chromium and GTK ports.
> 
> Sorry I just saw your post. I'm having a compile problem with it.
> 
> It has to do with how the Gtk makefile works its one of the things I'm working
> on now. The problem is that derived sources are compiled and linked in using a
> general rule and I need to split this out. Once done I should be able to bring
> it back. Given all the other issues its on the lower end of the agenda at the
> moment. But something I'll fix actually as a side effect of my makefile
> refactoring. Mental sticky note added.

Weird this was and attempt to reply to why I neutered the AllInOne file.
Bad click or bug in bugzilla ?

[Evan Martin]

I poked at this, but there is too much going on to be able to do anything useful with it.

Would it be possible to make:
1) a separate patch that refactors the makefiles to split out JSC from the other webcore files?  I can review and commit that without any v8 bits since it's a cleanup.
2) a patch that doesn't involve renaming everything?  that will keep the diff minimal so I can actually look at it.  (Even a patch that doesn't compile would be good.)

BTW, we spend some time looking at the GTK build this week and managed to reduce the slower build caused by JSC considerably:
  https://bugs.webkit.org/show_bug.cgi?id=32663
So if that happened to be the reason for looking into v8, it's no longer necessary.

[Michael Emmel]

(In reply to comment #31)
> I poked at this, but there is too much going on to be able to do anything
> useful with it.
> 
> Would it be possible to make:
> 1) a separate patch that refactors the makefiles to split out JSC from the
> other webcore files?  I can review and commit that without any v8 bits since
> it's a cleanup.
> 2) a patch that doesn't involve renaming everything?  that will keep the diff
> minimal so I can actually look at it.  (Even a patch that doesn't compile would
> be good.)
> 
> BTW, we spend some time looking at the GTK build this week and managed to
> reduce the slower build caused by JSC considerably:
>   https://bugs.webkit.org/show_bug.cgi?id=32663
> So if that happened to be the reason for looking into v8, it's no longer
> necessary.

Well first the remote debugging to allow debugging of javascript on an embedded device was the primary reason for the port. The eclipse plugin was patched to accept and ip address. Other factors are secondary. Longer term it would be nice to see at least a defacto standard for remote debugging of javascript but thats a big project. In general getting both engines to work across a variety of ports and platforms will encourage both to become better benefiting everyone.

Next I agree with your proposal. If this patch is to be accepted I think the following needs to be done.

1.) Finish refactoring the makefiles and submission of the refactor patch and the one with v8 showing a more modular and easier to modify set of makefiles.

Right now I've broken things out into a few more makefiles one per major component and am working on assigning a few variables per feature.


2.) A second patch with only the changes needed to build minus renaming and prob debugging support. In some cases I was able to replace engine specific code with some of the new generic script support. Here these changes need to be vetted and I think we can do a bit of work on ScriptController/GenericBinding to remove a lot of engine specific code.
Associated with this would be a patch with just the changes outside of v8 without renaming. Enough to pick up the headers at least.


4.) Let the powers that be decide about renaming these patches will help everyone including me take a deeper look at the issue. Renaming was invaluable  in getting something that worked however perhaps its not needed. Regardless a set of patches without renaming are a must.

Political Statement :)

In general for V8 it would be nice if it was easy to drop into any project that wanted to use javascript. I use Javascript engines outside of the browser world and I've found that the JavascriptCore API approach is very useful. Its incomplete and in some cases I've had to use the direct api but overall it won as far as ease of use.

A lot of people program in javascript today this large developer base can be leveraged providing javscript api's for all kinds of applications. Proposals for the newer versions of the language go a long way to removing some if its warts. Thus the language itself has a lot of potential. However the lack of a reasonably complete standard C/C++ api makes integration difficult. So from the big picture viewpoint I'd argue driving towards a unified C/C++ api should be considered. Certainly in some cases engine specific features will need to be used but they should be and exception not the rule. Browsers that allow multiple engines would also benefit for this expanded base.

I think its worthwhile to bring this point up now not for debate but because I think its useful to consider and its my underlying big picture view.

[Michael Emmel]

Finally and update.

I set to work refactoring the makefiles to make them modular enough to add large and different submodules aka v8 vs JavascriptCore.

It turned out to be a very involved task.

The work can be downloaded at:
http://repo.or.cz/w/webbrowser.git
git clone ssh://repo.or.cz/srv/git/webbrowser.git

The branch is makefile

It would still need quite a bit of clean up and several reviews.
I've not tested out of tree builds I suspect they are broken.
However nothing particularly major is left.

The basic idea is that the monolithic solution is simply becoming unmanageable and does not fit the way most large project using autotools.

Several modular makefiles are already included in the main one so its already a multi file solution.

What I have done is move all the processor flag setting into configure and split the makefiles so they are standalone not included in the main build.

What this does is make it much simpler to split out submodules especially and refactor a bit if needed. Right now its very close to what I would need to easily add V8.

It was quite a bit of work and more needs to be done to finish but its to the point that the basic idea can be accepted or rejected. 

When the build finished GtkLauncher is now in WebKit/WebKitTools/Programs

In doing the work I found a lot of interesting issues several features for example did not have switches and thus could not be turned on even if they existed in the Makefile. I suspect in several cases these are bugs highlighting the issue of maintainability.

With all the includes and flags now resolved in configure adding new modular standalone makefiles is fairly easy. And it would not be hard to make the final link a bit more modular if needed.

[Evan Martin]

Probably best to open a new bug for the Makefile refactoring.  Lemme know if you have and I can send it to the right people.

[Michael Emmel]

(In reply to comment #34)
> Probably best to open a new bug for the Makefile refactoring.  Lemme know if
> you have and I can send it to the right people.

Ok its in bug 33810.


On the V8 side I'll create a branch using my old makefile changes plus
V8 without the renaming to show issues with header conflicts.

With the new wrapper classes in WeCore/bindings I'm thinking that we should be able th hide V8 from the rest of WebKit eliminating most of the naming conflicts.

[Michael Emmel]

New patch with no renaming.

Location:
 http://repo.or.cz/w/webbrowser.git 
Branch:
v8norename

This is everything but the renaming and a few other changes.
The directory V8ExtraPatches has everything thats not applied.

I'm sorry I set on this for so long when the Makefile work went without comment I sort of lost my mojo.

One of the big reasons I did not submit is buried in the V8 rename patches are some useful changes that are needed. I wasted a lot of time trying to find them.
Regardless here it is and I think its good enough to serve as a start at porting.

[Nayan Kumar K]

Created attachment 102530 [details]
Porting V8 to WebKit-GTK

Patch to port V8 to WebKit-GTK

Few notes,
a). It is a big patch, based on review comments, I will re-structure the patches uploaded them for review in smaller patches. Hence review flag has not been set.
b). DumpRenderTree is yet to be implemented.

V8 compilation can be turned on using,

sh autogen.sh --with-jsengine=v8

This work can be found at https://gitorious.org/~nayankk/webkit/nayankk-webkit

Let me know your opinions and concerns.

And BTW, based on a quick performance test, WebKit-GTK with V8 shows significant improvement in performance as compared to SFX engine. This conclusion is based on V8 benchmarking suite (of google) in x86-64 machine.

[Adam Barth]

Comment on attachment 102530 [details]
Porting V8 to WebKit-GTK

View in context: https://bugs.webkit.org/attachment.cgi?id=102530&action=review

The general outline of this patch looks good.  I agree that we should break it into smaller pieces so that each piece can receive a high-quality review.

> Source/WebCore/bindings/v8/custom/V8InspectorFrontendHostCustom.cpp:145
> -#if !PLATFORM(QT)
> +#if !PLATFORM(QT) && !PLATFORM(GTK)

Maybe we should use a PLATFORM whitelist for these instead of a blacklist?

[Philippe Normand]

I tested this patch, some misc issues:

- it no longer applies cleanly on trunk
- some rules of Source/JavaScriptCore/GNUmakefile.am should appear in Source/JavaScriptCore/GNUmakefile.v8.am too (like the RegExpJitTables.h generation). Maybe have a common GNUMakefile for both v8 and jsc?
- Source/JavaScriptCore/yarr/YarrPattern.cpp build needs to depend on Source/JavaScriptCore/RegExpJitTables.h
- some bindings/v8/custom files missing in the GNUMakefile.list.am file
- proper libv8 detection missing in configure.ac
- FrameLoaderClient::didCreateIsolatedScriptContext now takes a WebCore::V8IsolatedContext* argument

Have you tested the distcheck build?
Please make sure to have a working clean build too, I advise to use build-webkit if you don't already.

[Rémi Duraffort]

(In reply to comment #37)

View in context: https://bugs.webkit.org/attachment.cgi?id=102530&action=review

> Source/WebCore/GNUmakefile.list.am:5820
>  

Looks suspecious IMHO. The endif should be after the section about USE_V8 so the test on ENABLE_WEB_AUDIO affects both USE_JSC and USE_V8

[Nayan Kumar K]

Here is the performance analysis between JSC and V8 on an x86 reference machine - http://xc0ffee.wordpress.com/2011/08/27/webkit-gtk-jsc-vs-v8/. 

I am also planning to benchmark the results on an arm reference board, will publish the results soon.

[Nayan Kumar K]

> 
> > Source/WebCore/bindings/v8/custom/V8InspectorFrontendHostCustom.cpp:145
> > -#if !PLATFORM(QT)
> > +#if !PLATFORM(QT) && !PLATFORM(GTK)
> 
> Maybe we should use a PLATFORM whitelist for these instead of a blacklist?

Sure, will incorporate this comment when I re-factor the whole patch

[Nayan Kumar K]

(In reply to comment #39)
> I tested this patch, some misc issues:
> 
> - it no longer applies cleanly on trunk
Yes, this patch is currently compatible with 91387 revision of WebKit.  

> - some rules of Source/JavaScriptCore/GNUmakefile.am should appear in Source/JavaScriptCore/GNUmakefile.v8.am too (like the RegExpJitTables.h generation). Maybe have a common GNUMakefile for both v8 and jsc?
> - Source/JavaScriptCore/yarr/YarrPattern.cpp build needs to depend on Source/JavaScriptCore/RegExpJitTables.h
Sorry, patch uploaded here missed the generation of RegExpJitTables.h. My gitorious branch https://gitorious.org/~nayankk/webkit/nayankk-webkit has the fix of this and couple of other issues.

> - some bindings/v8/custom files missing in the GNUMakefile.list.am file
> - proper libv8 detection missing in configure.ac
> - FrameLoaderClient::didCreateIsolatedScriptContext now takes a WebCore::V8IsolatedContext* argument
I will incorporate these comments when I re-factor the patch.

> 
> Have you tested the distcheck build?
> Please make sure to have a working clean build too, I advise to use build-webkit if you don't already.
Clean build works too. However, support for compiling v8 with build-webkit script is not yet implemented.

[Philippe Normand]

(In reply to comment #41)
> Here is the performance analysis between JSC and V8 on an x86 reference machine - http://xc0ffee.wordpress.com/2011/08/27/webkit-gtk-jsc-vs-v8/. 
> 
> I am also planning to benchmark the results on an arm reference board, will publish the results soon.

Looks like those pictures are hosted on a google account requiring Motorola access :P

[Philippe Normand]

(In reply to comment #43)

> Clean build works too. However, support for compiling v8 with build-webkit script is not yet implemented.

build-webkit will pass-through the options it doesn't handle to ./configure.
So build-webkit --gtk  --with-jsengine=v8 should work

[Nayan Kumar K]

Here is another blog post detailing the performance of V8 and JSC on an embedded ARM device - http://xc0ffee.wordpress.com/2011/09/07/webkit-gtk-jsc-vs-v8-performance-on-arm/

[Rémi Duraffort]

Created attachment 109470 [details]
Using V8 inside WebKit/Gtk

This patch is an updated version of the last patch posted by Nayan Kumar Konaje.

This patch should apply cleanly on r96484.

Comments are welcome.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Source/Ja..." exit_code: 1

Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.h:88:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.h:88:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testatk.c"
Source/WebCore/bindings/v8/custom/V8DirectoryEntryCustom.cpp:35:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Source/JavaScriptCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebCore/bindings/v8/custom/V8DirectoryEntrySyncCustom.cpp:35:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testdomdomwindow.c"
Source/WebKit/gtk/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testcopyandpaste.c"
Source/WebCore/platform/gtk/PlatformBridge.h:75:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/gtk/PlatformBridge.h:76:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/gtk/PlatformBridge.h:93:  The parameter name "npp" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:52:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:53:  Alphabetical sorting problem.  [build/include_order] [4]
ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testkeyevents.c"
Total errors found: 15 in 65 files


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

[Gyuyoung Kim]

Comment on attachment 109470 [details]
Using V8 inside WebKit/Gtk

Attachment 109470 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9928398

[Early Warning System Bot]

Comment on attachment 109470 [details]
Using V8 inside WebKit/Gtk

Attachment 109470 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9903985

[WebKit Review Bot]

Comment on attachment 109470 [details]
Using V8 inside WebKit/Gtk

Attachment 109470 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9929344

[Rémi Duraffort]

Created attachment 109484 [details]
Using V8 inside WebKit/Gtk

Update the patch and fix the building and style check issues reported by the buildbots.

[Gyuyoung Kim]

Comment on attachment 109484 [details]
Using V8 inside WebKit/Gtk

Attachment 109484 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9928451

[WebKit Review Bot]

Comment on attachment 109484 [details]
Using V8 inside WebKit/Gtk

Attachment 109484 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9929387

[Rémi Duraffort]

Created attachment 109615 [details]
Using V8 inside WebKit/Gtk

Second round of fixes that should fix at least EFL build (it does build in my freshly created environment)

[WebKit Review Bot]

Comment on attachment 109615 [details]
Using V8 inside WebKit/Gtk

Attachment 109615 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9936536

[Rémi Duraffort]

Created attachment 109624 [details]
Using V8 inside WebKit/Gtk

Another attempt to blindly fix compilation. It should now also work for cr-linux

[Rémi Duraffort]

Created attachment 109771 [details]
Using V8 inside WebKit/Gtk

Another attempt to blindly fix win32 build.

[Rémi Duraffort]

Now that every builds passes, could you review and comment the patch so we can try to get the patch integrated ?
Any comments are welcome.

[Adam Barth]

Comment on attachment 109771 [details]
Using V8 inside WebKit/Gtk

View in context: https://bugs.webkit.org/attachment.cgi?id=109771&action=review

This patch is basically fine.  I'll be happy to review it if you break it into smaller pieces and use a separate bug (each one blocking this one) for each patch.  There are a number of mistakes in this patch, but nothing that we can't clear up during review.

> ChangeLog:10
> +        This work is also copyrighted to Nayan Kumar Konaje <[e-mail]>

Does Nayan Kumar Konaje agree to the contributor license as well?

> Source/JavaScriptCore/GNUmakefile.v8.am:13
> +javascriptcore_sources += \

Can this be shared with another GNUmakefile ?  Every time we add a giant list of all files, we add a maintenance burden on the project.

> Source/WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:31
>  

This blank line should be removed.

> Source/WebCore/dom/CustomEvent.idl:32
> -        CustomConstructFunction,
> -        V8CustomConstructor
> +        CustomConstructFunction

I don't understand this change.

> Source/WebCore/platform/gtk/PlatformBridge.h:27
> +#ifndef PlatformBridge_h
> +#define PlatformBridge_h

There is no such thing as a PlatformBridge anymore.  It's called PlatformSupport instead.

> Source/WebCore/platform/gtk/PlatformBridge.h:72
> +// V8 bindings use the ARRAYSIZE_UNSAFE macro. This macro was copied
> +// from http://src.chromium.org/viewvc/chrome/trunk/src/base/basictypes.h
> +//
> +// ARRAYSIZE_UNSAFE performs essentially the same calculation as arraysize,
> +// but can be used on anonymous types or types defined inside
> +// functions. It's less safe than arraysize as it accepts some
> +// (although not all) pointers. Therefore, you should use arraysize
> +// whenever possible.
> +//
> +// The expression ARRAYSIZE_UNSAFE(a) is a compile-time constant of type
> +// size_t.
> +//
> +// ARRAYSIZE_UNSAFE catches a few type errors. If you see a compiler error
> +//
> +//   "warning: division by zero in ..."
> +//
> +// when using ARRAYSIZE_UNSAFE, you are (wrongfully) giving it a pointer.
> +// You should only use ARRAYSIZE_UNSAFE on statically allocated arrays.
> +//
> +// The following comments are on the implementation details, and can
> +// be ignored by the users.
> +//
> +// ARRAYSIZE_UNSAFE(arr) works by inspecting sizeof(arr) (the # of bytes in
> +// the array) and sizeof(*(arr)) (the # of bytes in one array
> +// element). If the former is divisible by the latter, perhaps arr is
> +// indeed an array, in which case the division result is the # of
> +// elements in the array. Otherwise, arr cannot possibly be an array,
> +// and we generate a compiler error to prevent the code from
> +// compiling.
> +//
> +// Since the size of bool is implementation-defined, we need to cast
> +// !(sizeof(a) & sizeof(*(a))) to size_t in order to ensure the final
> +// result has type size_t.
> +//
> +// This macro is not perfect as it wrongfully accepts certain
> +// pointers, namely where the pointer size is divisible by the pointee
> +// size. Since all our code has to go through a 32-bit compiler,
> +// where a pointer is 4 bytes, this means all pointers to a type whose
> +// size is 3 or greater than 4 will be (righteously) rejected.

Is this massive comment copy/pasted from somewhere?

> Source/WebCore/platform/gtk/PlatformSupport.h:33
> +typedef PlatformBridge PlatformSupport;

???  Why not just define it as PlatformSupport in the first place?

[Nayan Kumar K]

Hi Adam/Remi,

Thank you for showing interest in this patch.

We had discussion about pushing this patch to WebKit-GTK (https://lists.webkit.org/pipermail/webkit-gtk/2011-August/000622.html), and since community wasn't keen on this work, I didn't split this across multiple patches and re-submit.

I already have the patches split into multiple smaller patches here in my gitorious branch https://gitorious.org/~nayankk/webkit/nayankk-webkit/commits/v8, I will rebase them to latest trunk and re-submit them.

[Nayan Kumar K]

Comment on attachment 109771 [details]
Using V8 inside WebKit/Gtk

View in context: https://bugs.webkit.org/attachment.cgi?id=109771&action=review

>> Source/JavaScriptCore/GNUmakefile.v8.am:13
>> +javascriptcore_sources += \
> 
> Can this be shared with another GNUmakefile ?  Every time we add a giant list of all files, we add a maintenance burden on the project.

Sure. I will incorporate it when I re-factor the patch. We can try to get this patch in with at-most care taken in terms of code maintainability.

>> Source/WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:31
>>  
> 
> This blank line should be removed.

Sure.

>> Source/WebCore/platform/gtk/PlatformBridge.h:27
>> +#define PlatformBridge_h
> 
> There is no such thing as a PlatformBridge anymore.  It's called PlatformSupport instead.

Sure. I will modify it.

>> Source/WebCore/platform/gtk/PlatformBridge.h:72
>> +// size is 3 or greater than 4 will be (righteously) rejected.
> 
> Is this massive comment copy/pasted from somewhere?

Yes. It was taken from qt port.

[James Robinson]

(In reply to comment #61)
> Hi Adam/Remi,
> 
> Thank you for showing interest in this patch.
> 
> We had discussion about pushing this patch to WebKit-GTK (https://lists.webkit.org/pipermail/webkit-gtk/2011-August/000622.html), and since community wasn't keen on this work, I didn't split this across multiple patches and re-submit.
> 

If the WebKit GTK community isn't interested in this, then is it worth committing it?  Who will maintain this code going forward?

[Martin Robinson]

(In reply to comment #63)

> > We had discussion about pushing this patch to WebKit-GTK (https://lists.webkit.org/pipermail/webkit-gtk/2011-August/000622.html), and since community wasn't keen on this work, I didn't split this across multiple patches and re-submit.
> > 
> 
> If the WebKit GTK community isn't interested in this, then is it worth committing it?  Who will maintain this code going forward?

Personally, I do not mind making this a build option (demand seems to be very high) as long as someone will ensure that it keeps building and working. There has been some pushback from other members of the community. People seem to want this more and more as time goes on though.

[Nayan Kumar K]

(In reply to comment #63)
> (In reply to comment #61)
> > Hi Adam/Remi,
> > 
> > Thank you for showing interest in this patch.
> > 
> > We had discussion about pushing this patch to WebKit-GTK (https://lists.webkit.org/pipermail/webkit-gtk/2011-August/000622.html), and since community wasn't keen on this work, I didn't split this across multiple patches and re-submit.
> > 
> 
> If the WebKit GTK community isn't interested in this, then is it worth committing it?  Who will maintain this code going forward?

We are willing to maintain this code going forward, we believe its a good-to-have feature in webkit-gtk. Also, right now, v8 seem to perform better than jsc (http://xc0ffee.wordpress.com/).

For GTK community acceptance, I have cc'ed Alex and Martin to this bug.

[Martin Robinson]

(In reply to comment #65)

> We are willing to maintain this code going forward, we believe its a good-to-have feature in webkit-gtk. Also, right now, v8 seem to perform better than jsc (http://xc0ffee.wordpress.com/).

That's quite nice. Are you even willing to provide a build bot?

[Nayan Kumar K]

(In reply to comment #66)
> (In reply to comment #65)
> 
> > We are willing to maintain this code going forward, we believe its a good-to-have feature in webkit-gtk. Also, right now, v8 seem to perform better than jsc (http://xc0ffee.wordpress.com/).
> 
> That's quite nice. Are you even willing to provide a build bot?

Well, we cannot commit to provide a build bot at this point. Can we have the existing bot to run v8 builds as well?

[Rémi Duraffort]

(In reply to comment #61)
> Thank you for showing interest in this patch.
Thanks for creating the initial patch.

> I already have the patches split into multiple smaller patches here in my gitorious branch https://gitorious.org/~nayankk/webkit/nayankk-webkit/commits/v8, I will rebase them to latest trunk and re-submit them.

I will create a separate bug for the fixes I had to make in
V8DirectoryEntryCustom.cpp
V8HTMLAudioElementConstructor.cpp
V8HTMLAudioElementConstructor.h
V8ScriptProfileCustom.cpp
V8ScriptProfileNodeCustom.cpp
V8WebGLRenderingContextCustom.cpp
V8XSLTProcessorCustom.cpp
as these are v8 specific bugs and not related to the porting of V8 to WebKit/Gtk and will shrink (just a little bit) the patch.

[Rémi Duraffort]

(In reply to comment #68)
> I will create a separate bug for the fixes I had to make in
> V8HTMLAudioElementConstructor.cpp
> V8HTMLAudioElementConstructor.h
> V8XSLTProcessorCustom.cpp
> as these are v8 specific bugs and not related to the porting of V8 to WebKit/Gtk and will shrink (just a little bit) the patch.

The patch is included:
https://bugs.webkit.org/show_bug.cgi?id=69522

[Anders Carlsson]

V8 is gone from WebKit now, let's close this.