Bug 56649 – [Qt][V8] Use qtscript-staging's shipped version of V8 when building with --v8

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Andras Becsi]

Since http://qt.gitorious.org/qt/qt-script-ng is public now, we can use its version of V8 and the provided functionality to build QtWebKit+V8. Since the version shipped is an older version V8, we need a few ifdef's to make it work, but those should be removed soon, since the work on QtScript is ongoing.

[Andras Becsi]

Created attachment 86171 [details]
proposed patch

[Andras Becsi]

After this patch we can restart the QtWebKit+V8 bot again to use QtScript-NG, and revive our DRT with V8.

[Andras Becsi]

Comment on attachment 86171 [details]
proposed patch

Marking the patch obsolete, since the v8 tree in QtScript is going to be updated in the next few weeks.

[Peter Varga]

Created attachment 94057 [details]
proposed patch v2

Based on the previous patch of Andras Becsi <[e-mail]>.

[Andras Becsi]

Comment on attachment 94057 [details]
proposed patch v2

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

Thanks for taking this.
The patch looks good to me, just a few nitpicks which can be fixed when you land it.

> Source/WebCore/ChangeLog:9
> +        Use the provided V8 and functionality of
> +        https://bugs.webkit.org/show_bug.cgi?id=56649 to build QtWebKit+V8.

Here you probably wanted to add the link to the qtscript repo.

> Source/WebCore/WebCore.pri:37
> +    lessThan(QT_MAJOR_VERSION, 5) {
> +        warning("To build QtWebKit+V8 you need qtscript-staging's v8 branch. (See: http://qt.gitorious.org/+qt-developers/qt/qtscript-staging)")
> +    }

This could be an error instead of a warning since it will not build without the specified Qt version, and a warning is easily overseen.

> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:54
> +#if USE(V8)
> +#include <wtf/UnusedParam.h>
> +#endif

Did it cause problems in normal builds without the guard? This does not need to be guarded you can just add the include.

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:44
> +#if USE(V8)
> +#include <wtf/UnusedParam.h>
> +#endif

Ditto.

> Source/WebCore/page/PageSerializer.cpp:59
> +#if USE(V8)
> +#include <wtf/text/CString.h>
> +#endif

Ditto.

> Source/WebCore/page/qt/FrameQt.cpp:27
> +#if USE(V8)
> +#include "Document.h"
> +#endif

Ditto

> Source/WebCore/storage/StorageEventDispatcher.cpp:33
> +#if USE(V8)
> +#include "Document.h"
> +#endif

Ditto.

> Source/WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:37
> +#if USE(V8)
> +#include "Document.h"
> +#endif

Ditto.

> Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:70
> +#if USE(V8)
> +#include "ScriptSourceCode.h"
> +#endif

Ditto.

[Peter Varga]

Created attachment 94060 [details]
proposed patch v2.1

Fix the mentioned issues.

[Simon Hausmann]

Comment on attachment 94060 [details]
proposed patch v2.1

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

> Source/WebCore/WebCore.pri:37
> +    !exists($${V8_LIB_DIR}$${QMAKE_DIR_SEP}libv8.a): error("Cannot build with V8. Needed library $${V8_LIB_DIR}$${QMAKE_DIR_SEP}libv8.a does not exist.")

Hmm, are you sure that this check works with shadow-builds of qtscript-staging? (this can be fixed later though)

> Source/WebCore/bindings/v8/ScriptController.cpp:313
> +#if !PLATFORM(QT)
> +// FIXME: http://code.google.com/p/v8/source/detail?r=7753
> +//        https://bugs.webkit.org/show_bug.cgi?id=60384
> +

Is this because of Qt's copy of v8 being outdated?

> Source/WebKit/qt/Api/qwebframe.cpp:650
> +    engine->globalObject().property(QString::fromLatin1("window")).setProperty(name, v);

It would be cleaner to use QLatin1String here. </nitpick> :)

[Simon Hausmann]

Comment on attachment 94060 [details]
proposed patch v2.1

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

The patch in general looks good, r=me. I think the FIXME comment should be fixed though before landing (hence cq-).

>> Source/WebCore/bindings/v8/ScriptController.cpp:313
>> +
> 
> Is this because of Qt's copy of v8 being outdated?

Please replace this comment with a comment explaining why this code is currently disabled for PLATFORM(QT)
and a link to a new bug in bugs.webkit.org that tracks fixing this once the v8 copy in Qt is updated.

[Peter Varga]

(In reply to comment #7)

> Hmm, are you sure that this check works with shadow-builds of qtscript-staging? (this can be fixed later though)

We don't know how the modularized Qt deployment will work thus this solution is meant to be temporary.
Currently we need to copy or make a symlink to the source code into the prefix directory since the V8 source code (include/v8.h) and the library are both in qtscript-staging's src directory. As soon as we know enough about the modularized Qt deployment structure we can modify these checks.


> Is this because of Qt's copy of v8 being outdated?

Yes, I have opened a bug: https://bugs.webkit.org/show_bug.cgi?id=61187

[Peter Varga]

(In reply to comment #8)
> (From update of attachment 94060 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94060&action=review
> 
> The patch in general looks good, r=me. I think the FIXME comment should be fixed though before landing (hence cq-).
> 
> >> Source/WebCore/bindings/v8/ScriptController.cpp:313
> >> +
> > 
> > Is this because of Qt's copy of v8 being outdated?
> 
> Please replace this comment with a comment explaining why this code is currently disabled for PLATFORM(QT)
> and a link to a new bug in bugs.webkit.org that tracks fixing this once the v8 copy in Qt is updated.

Fixed patch landed in: http://trac.webkit.org/changeset/86949

Closing bug.

[Peter Varga]

Comment on attachment 94060 [details]
proposed patch v2.1

Clearing flags.