Bug 44669 – [Qt] Make it possible to configure QtWebKit to use V8

• Status: RESOLVED
• Resolution: INVALID
• Component: New Bugs

[Henry Haverinen]

Make it possible to configure QtWebKit to use the V8 JavaScript engine instead of JavaScriptCore. At first, this would be useful for performance benchmarking and quality assurance.

[Henry Haverinen]

Qt bug tracker entry: http://bugreports.qt.nokia.com/browse/QTBUG-12502

[Vlad]

Created attachment 66261 [details]
V8 binding changes for QT platform (Symbian and Maemo)

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/bindings/v8/V8NPUtils.cpp:33:  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/bindings/v8/V8NPObject.cpp:33:  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/bindings/v8/V8Proxy.h:43:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/v8/NPV8Object.cpp:29:  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/bindings/v8/NPV8Object.cpp:122:  _NPN_CreateNoScriptObject is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/v8/custom/V8HTMLPlugInElementCustom.cpp:33:  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/bindings/v8/npruntime.cpp:29:  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/bindings/v8/V8Helpers.cpp:33:  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: 8 in 14 files


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

[Vlad]

Created attachment 66262 [details]
V8 related changes for QT platform: npruntime_internal.h Remove X11 artifacts

[Vlad]

Created attachment 66263 [details]
V8 related changes for QT platform: wtf/DateMath.* (Symbian and Maemo)

Store cachedUTCOffset and getDSTOffset as hidden properties of global object

[Vlad]

Created attachment 66265 [details]
V8 related changes for QT platform: eliminate ambiguous choice of CString.h and Animation.h for Symbian

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/text/PlatformString.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 2 files


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

[Vlad]

Created attachment 66266 [details]
V8 related changes for QT platform: Plugin changes

[Vlad]

Created attachment 66267 [details]
V8 related changes for QT platform: project file modifications

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/wtf/DateMath.cpp:965:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 2 files


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

[Vlad]

Created attachment 66268 [details]
V8 related changes for QT platform: Qt API implementation modifications

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/plugins/PluginView.h:45:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 5 files


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

[Vlad]

Created attachment 66270 [details]
V8 related changes for QT platform: DumpRenderTreeSupportQt and FrameLoaderClientQt

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:42:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:324:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:340:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 4 in 3 files


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

[Vlad]

Created attachment 66275 [details]
V8 related changes for QT platform: DumpRenderTreeSupportQt and FrameLoaderClientQt

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:25:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:42:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 3 files


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

[Early Warning System Bot]

Attachment 66262 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3944035

[Vlad]

Created attachment 66289 [details]
V8 related changes for QT platform: Plugin changes

[Vlad]

Created attachment 66291 [details]
V8 related changes for QT platform: npruntime_internal.h Remove X11 artifacts

[Vlad]

Created attachment 66292 [details]
V8 related changes for QT platform: eliminate ambiguous choice of CString.h and Animation.h for Symbian

[Vlad]

Created attachment 66293 [details]
V8 related changes for QT platform: wtf/DateMath.* (Symbian and Maemo)

[Early Warning System Bot]

Attachment 66291 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3969036

[Andreas Kling]

Hi Vlad, interesting work!

Three things before we can start proper reviews:

1. Some of your patches replace the entire contents of files with what seems to be mostly the same data. Could this be due to newline format mismatch perhaps? Needs to be fixed.

2. ChangeLog entries are missing. You can use WebKitTools/Scripts/prepare-ChangeLog to generate boilerplate entries. See https://trac.webkit.org/wiki/QtWebKitContrib for more information.

3. (Admittedly of lesser importance) There's both "#if USE(JSC) ... #elif USE(V8) ..." and "#if USE(JSC) ... #else ...". I think we should stick with the first variant.

[WebKit Review Bot]

Attachment 66261 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3949027

[Vlad]

First check in and relatevly big... :) Trying to fix those things now

(In reply to comment #23)
> Hi Vlad, interesting work!
> 
> Three things before we can start proper reviews:
> 
> 1. Some of your patches replace the entire contents of files with what seems to be mostly the same data. Could this be due to newline format mismatch perhaps? Needs to be fixed.
> 
> 2. ChangeLog entries are missing. You can use WebKitTools/Scripts/prepare-ChangeLog to generate boilerplate entries. See https://trac.webkit.org/wiki/QtWebKitContrib for more information.
> 
> 3. (Admittedly of lesser importance) There's both "#if USE(JSC) ... #elif USE(V8) ..." and "#if USE(JSC) ... #else ...". I think we should stick with the first variant.

[WebKit Review Bot]

Attachment 66291 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3927034

[WebKit Review Bot]

Attachment 66293 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3902036

[Vlad]

Created attachment 66319 [details]
V8 related changes for QT platform: webkit pr* files

[Vlad]

Created attachment 66320 [details]
V8 related changes for QT platform: webcore pr* files

[Vlad]

Created attachment 66321 [details]
V8 related changes for QT platform: javascript pr* files. Use wtf only

[Vlad]

Created attachment 66322 [details]
V8 related changes for QT platform: config.h

[Vlad]

Created attachment 66323 [details]
V8 related changes for QT platform: datemath mods

[Vlad]

Created attachment 66324 [details]
V8 related changes for QT platform: v8 bindings

[Vlad]

Created attachment 66325 [details]
V8 related changes for QT platform: npruntime_internal.h Remove X11 artifacts

[Vlad]

Created attachment 66326 [details]
V8 related changes for QT platform: eliminate ambiguous choice of CString.h and Animation.h for Symbian

[Vlad]

Created attachment 66327 [details]
V8 related changes for QT platform: Plugin changes

[Vlad]

Created attachment 66328 [details]
V8 related changes for QT platform: Qt API implementation modifications

[Vlad]

Created attachment 66329 [details]
V8 related changes for QT platform: QObject injection implementation

[Vlad]

Created attachment 66330 [details]
V8 related changes for QT platform: webcoresupport mods

[Adam Barth]

OMG.  So many patches attached to one bug.  Please consider using a master bug with a bunch of blocking bugs.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
Last 3072 characters of output:
t/v8/qt_instancev8.cpp:182:  Missing space before ( in foreach(  [whitespace/parens] [5]
WebCore/bridge/qt/v8/qt_instancev8.cpp:307:  Missing space before ( in for(  [whitespace/parens] [5]
WebCore/bridge/qt/v8/qt_instancev8.cpp:344:  Missing spaces around =  [whitespace/operators] [4]
WebCore/bridge/qt/v8/qt_instancev8.cpp:348:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bridge/qt/v8/qt_instancev8.cpp:352:  Missing space before ( in foreach(  [whitespace/parens] [5]
WebCore/bridge/qt/v8/qt_instancev8.cpp:353:  Should have a space between // and comment  [whitespace/comments] [4]
WebCore/bridge/qt/v8/qt_instancev8.cpp:357:  Missing spaces around =  [whitespace/operators] [4]
WebCore/bridge/qt/v8/qt_instancev8.cpp:360:  Should have a space between // and comment  [whitespace/comments] [4]
WebCore/bridge/qt/v8/qt_instancev8.cpp:380:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/bridge/qt/v8/qt_instancev8.cpp:427:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/bridge/qt/v8/qt_instancev8.cpp:440:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/bridge/qt/v8/qt_instancev8.cpp:498:  Missing spaces around =  [whitespace/operators] [4]
WebCore/bridge/qt/v8/qt_instancev8.cpp:510:  Extra space before )  [whitespace/parens] [2]
WebCore/bridge/qt/v8/qt_instancev8.cpp:519:  Missing space before ( in foreach(  [whitespace/parens] [5]
WebCore/bridge/qt/v8/qt_instancev8.cpp:534:  Declaration has space between type name and * in QObject *child  [whitespace/declaration] [3]
WebCore/bridge/qt/v8/qt_instancev8.cpp:542:  Extra space before )  [whitespace/parens] [2]
WebCore/bridge/qt/v8/qt_instancev8.cpp:549:  Missing spaces around =  [whitespace/operators] [4]
WebCore/bridge/qt/v8/qt_instancev8.cpp:557:  Should have a space between // and comment  [whitespace/comments] [4]
WebCore/bridge/qt/v8/qt_instancev8.cpp:583:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/bridge/qt/v8/qt_instancev8.cpp:586:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/bridge/qt/v8/qt_instancev8.cpp:596:  Declaration has space between type name and * in QObject *child  [whitespace/declaration] [3]
WebCore/bridge/qt/v8/qt_instancev8.cpp:629:  Declaration has space between type name and * in QObject *child  [whitespace/declaration] [3]
WebCore/bridge/qt/v8/qt_instancev8.cpp:578:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 195 in 6 files


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

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:77:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 4 files


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

[Early Warning System Bot]

Attachment 66325 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3915043

[WebKit Review Bot]

Attachment 66323 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3967042

[WebKit Review Bot]

Attachment 66325 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3949036

[Andreas Kling]

(In reply to comment #40)
> OMG.  So many patches attached to one bug.  Please consider using a master bug with a bunch of blocking bugs.

Second this.

Furthermore, separate patches need individual ChangeLogs explaining what's being changed and why - right now it's rather difficult to review these changes :-)

Here's an example of a very helpful ChangeLog entry: http://trac.webkit.org/changeset/66334

[Vlad]

Created attachment 66375 [details]
V8 related changes for QT platform: Plugin changes

[Vlad]

Created attachment 66378 [details]
V8 related changes for QT platform: wtf/DateMath.* (Symbian and Maemo)

[Balazs Kelemen]

Please close this bug (as invalid), create a new master bug, create one bug for each individual change and set them as blocker for the master bug. This is how we use bugzilla for changes that are closely related to each other.

[WebKit Review Bot]

Attachment 66375 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3924041

[Vlad]

Apperently I cannot set it to IGNORED myself.Henry can you do it?

(In reply to comment #49)
> Please close this bug (as invalid), create a new master bug, create one bug for each individual change and set them as blocker for the master bug. This is how we use bugzilla for changes that are closely related to each other.

[Andras Becsi]

Setting this bug invalid on the grounds of the previous comments.

[Simon Hausmann]

(In reply to comment #51)
> Apperently I cannot set it to IGNORED myself.Henry can you do it?

Now you can, too, Vlad :)

[Andreas Kling]

Comment on attachment 66319 [details]
V8 related changes for QT platform: webkit pr* files

Clearing review flags as these patches have been moved to separate bugs.

[Andreas Kling]

Comment on attachment 66320 [details]
V8 related changes for QT platform: webcore pr* files

Clearing review flags as these patches have been moved to separate bugs.

[Andreas Kling]

Comment on attachment 66321 [details]
V8 related changes for QT platform: javascript pr* files. Use wtf only

Clearing review flags as these patches have been moved to separate bugs.

[Andreas Kling]

Comment on attachment 66322 [details]
V8 related changes for QT platform: config.h

Clearing review flags as these patches have been moved to separate bugs.

[Andreas Kling]

Comment on attachment 66324 [details]
V8 related changes for QT platform: v8 bindings

Clearing review flags as these patches have been moved to separate bugs.

[Andreas Kling]

Comment on attachment 66326 [details]
V8 related changes for QT platform: eliminate ambiguous choice of CString.h and Animation.h for Symbian 

Clearing review flags as these patches have been moved to separate bugs.

[Andreas Kling]

Comment on attachment 66328 [details]
V8 related changes for QT platform: Qt API implementation modifications

Clearing review flags as these patches have been moved to separate bugs.

[Andreas Kling]

Comment on attachment 66329 [details]
V8 related changes for QT platform: QObject injection implementation

Clearing review flags as these patches have been moved to separate bugs.

[Andreas Kling]

Comment on attachment 66330 [details]
V8 related changes for QT platform: webcoresupport mods

Clearing review flags as these patches have been moved to separate bugs.

[Andreas Kling]

Comment on attachment 66375 [details]
V8 related changes for QT platform: Plugin changes

Clearing review flags as these patches have been moved to separate bugs.

[Andreas Kling]

Comment on attachment 66378 [details]
V8 related changes for QT platform: wtf/DateMath.* (Symbian and Maemo)

Clearing review flags as these patches have been moved to separate bugs.