Summary: | [Qt] Make QtWebkit2.2 compile and run on S60 Emulator | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hui Huang <hui_huang> | ||||||||
Component: | New Bugs | Assignee: | 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
Hui Huang
2011-07-12 14:29:50 PDT
Created attachment 100572 [details]
Proposed patch
Compiled and tested on S60 Emulator.
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"
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. 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 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 ? 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. Created attachment 100842 [details]
Patch with the changes made based on Laszlo's comments
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 ? 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 ? Created attachment 101034 [details]
New patch
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 ? (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 :( 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 ? Thanks, I'm adding the v3 patch to the branch. Once I finish it I'll add a comment here. 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. |