Bug 69469

Summary: [GTK][V8] Add JSC compilation option to configure.ac
Product: WebKit Reporter: Nayan Kumar K <nayankk>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, mrobinson, roger.wang, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Linux   
Bug Depends on:    
Bug Blocks: 32452    
Attachments:
Description Flags
Configure JSC or V8
pnormand: review+
Configure JSC or V8
none
Resolved style issues
none
Configure JSC or V8 none

Description Nayan Kumar K 2011-10-05 15:03:43 PDT
This is the first of patches to port V8 to WebKit-GTK. Idea is to provide a configurable option to choose either JSC or V8 as JS backend.

This patch just add a command line switch called '--with-jsengine=[jsc/v8]' (for now only jsc is allowed) for compiling either JSC or V8 as JS backend. This defaults to JSC.

There are total 13 patches to cover whole V8 porting work (as per the current split) and all of them have been rebase'd and pushed to my gitorious branch at https://gitorious.org/~nayankk/webkit/nayankk-webkit, branch v8-new (Push is still on-going). I will create bugs and submit the patches here step-by-step.
Comment 1 Nayan Kumar K 2011-10-05 15:05:36 PDT
Created attachment 109869 [details]
Configure JSC or V8
Comment 2 Philippe Normand 2011-10-06 00:05:37 PDT
Comment on attachment 109869 [details]
Configure JSC or V8

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

Looks good, just one thing to fix before landing, thanks!

> GNUmakefile.am:199
> +if USE_JSC
> +global_cppflags += \
> +	-DWTF_USE_JSC=1
> +endif
> +

This define is not used yet, right? If not it should be introduced in the first patch that will use it.
Comment 3 Nayan Kumar K 2011-10-06 02:40:18 PDT
Created attachment 109935 [details]
Configure JSC or V8

Incorporated the review comments.
Comment 4 WebKit Review Bot 2011-10-06 02:42:41 PDT
Attachment 109935 [details] did not pass style-queue:

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

configure.ac:854:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Nayan Kumar K 2011-10-06 03:26:17 PDT
Created attachment 109938 [details]
Resolved style issues
Comment 6 WebKit Review Bot 2011-10-06 03:29:16 PDT
Attachment 109938 [details] did not pass style-queue:

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

ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Nayan Kumar K 2011-10-06 03:38:01 PDT
Created attachment 109940 [details]
Configure JSC or V8
Comment 8 Philippe Normand 2011-10-06 05:32:35 PDT
Comment on attachment 109940 [details]
Configure JSC or V8

I thought you'd land the previous patch yourself since you initially didn't flip the cq? flag.
Comment 9 WebKit Review Bot 2011-10-06 06:36:07 PDT
Comment on attachment 109940 [details]
Configure JSC or V8

Clearing flags on attachment: 109940

Committed r96808: <http://trac.webkit.org/changeset/96808>
Comment 10 WebKit Review Bot 2011-10-06 06:36:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Xan Lopez 2011-10-06 08:59:05 PDT
I don't understand what's the point of having this upstream when we haven't decided yet whether we'll allow V8 as an option or not. IMHO it only adds confusion for no gain at all. When (or if) a decision is made we can just land it, it's not a difficult patch at all.
Comment 12 Nayan Kumar K 2011-10-07 05:23:59 PDT
(In reply to comment #11)
> I don't understand what's the point of having this upstream when we haven't decided yet whether we'll allow V8 as an option or not. IMHO it only adds confusion for no gain at all. When (or if) a decision is made we can just land it, it's not a difficult patch at all.

I have started a discussion in mailing list to see community is willing to accept this work at this point of time. If we conclude not to push this work right now, we will revert this patch. :). 

Please feel free to share your opinions. Thanks.