Bug 64391 – [Qt] Make QtWebkit2.2 compile and run on S60 Emulator

• Component: New Bugs
• Status: RESOLVED FIXED

[Hui Huang]

Fix compiling errors with QtWebkit 2.2 WINSCW build. Make sure that QtTestBrowser runs on S60 Emulator.

[Hui Huang]

Created attachment 100572 [details]
Proposed patch

Compiled and tested on S60 Emulator.

[Andreas Kling]

Comment on attachment 100572 [details]
Proposed patch

Is this for QtWebKit 2.2 or trunk? r- because the ChangeLog bug name doesn't match the apparent intention of putting it on trunk.
In that case it should be something like "Make QtWebKit build on WinSCW"

[Hui Huang]

This is not intended to be landed to trunk. Just following Ademar's advice to create a patch in Bugzilla to be integrated into QtWebkit2.2. I think this is how it was done for QtWebkit 2.1. The WINSCW patch wasn't landed to trunk either.

[Ademar Reis]

Kling: is there a way to remove your r- and go back to r? The patch is for 2.2 only, but it should be reviewed as usual.

[Laszlo Gombos]

Comment on attachment 100572 [details]
Proposed patch

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

Overall the patch looks good to me with some minor changes. As mentioned earlier this should only land in the 2.2 branch.

> Source/JavaScriptCore/wtf/PageAllocatorSymbian.h:72
> +const size_t largeReservationSize = 64*1024*1024;

Can you explain this change ?

> Source/WebCore/ChangeLog:5
> +        Fix compiling errors with QtWebkit 2.2 WINSCW build.

Can you add the [Qt] prefix for the title of the bug ? it is implied that this is Qt only if it only goes to the 2.2 branch, but would be good for consistency.

> Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1026
> +#if QT_VERSION >= 0x040800

This seems unrelated and perhaps should go to a different patch. Can you explain why this is needed ?

[Hui Huang]

Thanks for reviewing the patch. 

> > Source/JavaScriptCore/wtf/PageAllocatorSymbian.h:72
> > +const size_t largeReservationSize = 64*1024*1024;
> 
> Can you explain this change ?

The original value 96M causes S60 Emulator to crash on Symbian System error -4. New version of S60 Emulator doesn't seem to be able to allocate such a large chunk of memory. The problem is fixed after I reduced largeReservationSize to 64M.

> > Source/WebCore/ChangeLog:5
> > +        Fix compiling errors with QtWebkit 2.2 WINSCW build.
> 
> Can you add the [Qt] prefix for the title of the bug ? it is implied that this is Qt only if it only goes to the 2.2 branch, but would be good for consistency.

You are right. I will change it.

> 
> > Source/WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:1026
> > +#if QT_VERSION >= 0x040800
> 
> This seems unrelated and perhaps should go to a different patch. Can you explain why this is needed ?

This is a separate issue. QWebElement::render only takes one parameter before Qt4.7. QWebElement::render(QPainter * painter, const QRect &) was introduced in Qt 4.8. I will create a separate bug and introduce a separate patch to be landed to the trunk.

[Hui Huang]

Created attachment 100842 [details]
Patch with the changes made based on Laszlo's comments

[Laszlo Gombos]

Comment on attachment 100842 [details]
Patch with the changes made based on Laszlo's comments

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

r- for the likely non-WINSCW build break.

> Source/JavaScriptCore/wtf/PageAllocatorSymbian.h:71
> +// const size_t largeReservationSize = 96*1024*1024;

Please remove this line instead of commenting it out !

> Source/JavaScriptCore/wtf/text/AtomicString.h:176
> +    extern const JS_EXPORTDATA AtomicString xmlnsAtom1;

No need for the extra spaces here, please remove them !

> Source/JavaScriptCore/wtf/text/AtomicString.h:192
> +    extern const JS_EXPORTDATA AtomicString xmlnsAtom;

Ditto.

> Source/WebCore/css/CSSStyleSelector.h:212
> +        template <bool applyFirst>

Is this really needed ?

> Source/WebCore/page/PrintContext.cpp:57
>  

COMPILER(WINSCW) guard is missing from here. I think this would break non-WINSCW builds (including Symbian RVCT builds). As this patch no longer applies to trunk, I have no way telling if this patch breaks the build on e.g. Linux. Have you tried any non-WINSCW build with this patch ?

[Hui Huang]

Replied to the comments inline:
https://bugs.webkit.org/attachment.cgi?id=100842&action=review
Will submit a new patch.

(In reply to comment #8)
> (From update of attachment 100842 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100842&action=review
> 
> r- for the likely non-WINSCW build break.
> 
> > Source/JavaScriptCore/wtf/PageAllocatorSymbian.h:71
> > +// const size_t largeReservationSize = 96*1024*1024;
> 
> Please remove this line instead of commenting it out !
> 
> > Source/JavaScriptCore/wtf/text/AtomicString.h:176
> > +    extern const JS_EXPORTDATA AtomicString xmlnsAtom1;
> 
> No need for the extra spaces here, please remove them !
> 
> > Source/JavaScriptCore/wtf/text/AtomicString.h:192
> > +    extern const JS_EXPORTDATA AtomicString xmlnsAtom;
> 
> Ditto.
> 
> > Source/WebCore/css/CSSStyleSelector.h:212
> > +        template <bool applyFirst>
> 
> Is this really needed ?
> 
> > Source/WebCore/page/PrintContext.cpp:57
> >  
> 
> COMPILER(WINSCW) guard is missing from here. I think this would break non-WINSCW builds (including Symbian RVCT builds). As this patch no longer applies to trunk, I have no way telling if this patch breaks the build on e.g. Linux. Have you tried any non-WINSCW build with this patch ?

[Hui Huang]

Created attachment 101034 [details]
New patch

[Laszlo Gombos]

Comment on attachment 101034 [details]
New patch

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

This looks good to me for the 2.2 branch. 

One more nit

> Source/WebCore/css/CSSStyleSelector.h:212
> +        template <bool applyFirst>

Is this change really needed ?

[Yael]

(In reply to comment #11)
> (From update of attachment 101034 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101034&action=review
> 
> This looks good to me for the 2.2 branch. 
> 
> One more nit
> 
> > Source/WebCore/css/CSSStyleSelector.h:212
> > +        template <bool applyFirst>
> 
> Is this change really needed ?

Unfortunately yes :(

[Hui Huang]

I think Yael is right. This was taken from her WINSCW patch for QtWebkit 2.1. Without this line, it doesn't compile.

(In reply to comment #11)
> (From update of attachment 101034 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101034&action=review
> 
> This looks good to me for the 2.2 branch. 
> 
> One more nit
> 
> > Source/WebCore/css/CSSStyleSelector.h:212
> > +        template <bool applyFirst>
> 
> Is this change really needed ?

[Ademar Reis]

Thanks, I'm adding the v3 patch to the branch. Once I finish it I'll add a comment here.

[Ademar Reis]

Done: 0f2aaa057e79e01f6935e41c84d40cb23c6cc6f4

Everything appears to be normal (no regressions on buildbots). I'm closing as RESOLVED/FIXED because the title is explict about qtwebkit-2.2. If someone prefers RESOLVED/WONTFIX, feel free to change it.

Thanks.