Bug 64391

Summary: [Qt] Make QtWebkit2.2 compile and run on S60 Emulator
Product: WebKit Reporter: Hui Huang <hui_huang>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ademar, benjamin, hui_huang, kling, laszlo.gombos, yael
Priority: P3 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: S60 Emulator   
OS: Other   
Attachments:
Description Flags
Proposed patch
benjamin: commit-queue-
Patch with the changes made based on Laszlo's comments
laszlo.gombos: review-, hui_huang: commit-queue-
New patch laszlo.gombos: review+, hui_huang: commit-queue-

Description Hui Huang 2011-07-12 14:29:50 PDT
Fix compiling errors with QtWebkit 2.2 WINSCW build. Make sure that QtTestBrowser runs on S60 Emulator.
Comment 1 Hui Huang 2011-07-12 15:11:54 PDT
Created attachment 100572 [details]
Proposed patch

Compiled and tested on S60 Emulator.
Comment 2 Andreas Kling 2011-07-13 12:14:33 PDT
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"
Comment 3 Hui Huang 2011-07-13 12:57:12 PDT
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.
Comment 4 Ademar Reis 2011-07-13 13:53:49 PDT
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.
Comment 5 Laszlo Gombos 2011-07-14 05:26:42 PDT
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 ?
Comment 6 Hui Huang 2011-07-14 11:51:21 PDT
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.
Comment 7 Hui Huang 2011-07-14 12:24:10 PDT
Created attachment 100842 [details]
Patch with the changes made based on Laszlo's comments
Comment 8 Laszlo Gombos 2011-07-14 13:16:31 PDT
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 ?
Comment 9 Hui Huang 2011-07-15 11:12:50 PDT
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 ?
Comment 10 Hui Huang 2011-07-15 13:23:55 PDT
Created attachment 101034 [details]
New patch
Comment 11 Laszlo Gombos 2011-07-19 00:24:48 PDT
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 ?
Comment 12 Yael 2011-07-19 04:49:53 PDT
(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 :(
Comment 13 Hui Huang 2011-07-19 07:10:51 PDT
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 ?
Comment 14 Ademar Reis 2011-07-19 14:30:28 PDT
Thanks, I'm adding the v3 patch to the branch. Once I finish it I'll add a comment here.
Comment 15 Ademar Reis 2011-07-20 12:26:51 PDT
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.