| Summary: | Make compile with both OS(WINCE) and PLATFORM(QT) support | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kevin Funk <kevin.funk> | ||||||||||||||||||||||||
| Component: | WebKit Qt | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
| Severity: | Normal | CC: | abecsi, benjamin, hausmann, paroga, vestbo, webkit.review.bot | ||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||
|
Description
Kevin Funk
2012-08-31 01:21:27 PDT
Created attachment 162004 [details]
Patch
Attachment 162004 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/WTF/ChangeLog:3: Line contains tab character. [whitespace/tab] [5]
Source/WTF/ChangeLog:11: Line contains tab character. [whitespace/tab] [5]
Source/WTF/ChangeLog:13: Line contains tab character. [whitespace/tab] [5]
Source/WTF/WTF.pri:40: Line contains tab character. [whitespace/tab] [5]
Source/JavaScriptCore/ChangeLog:3: Line contains tab character. [whitespace/tab] [5]
Source/WebCore/ChangeLog:3: Line contains tab character. [whitespace/tab] [5]
Source/WebCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5]
Source/WebCore/ChangeLog:11: Line contains tab character. [whitespace/tab] [5]
Source/WebCore/ChangeLog:13: Line contains tab character. [whitespace/tab] [5]
Source/WebCore/ChangeLog:15: Line contains tab character. [whitespace/tab] [5]
Total errors found: 10 in 15 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 162018 [details]
Patch (fixed whitespace issues)
Attachment 162018 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/ChangeLog:3: Line contains tab character. [whitespace/tab] [5]
Total errors found: 1 in 15 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 162038 [details]
Patch (really fixed whitespace issues)
Comment on attachment 162038 [details] Patch (really fixed whitespace issues) View in context: https://bugs.webkit.org/attachment.cgi?id=162038&action=review > Source/WTF/wtf/unicode/icu/CollatorICU.cpp:45 > +#define strdup _strdup please include <wtf/StringExtras.h> instead (outside of OS(WINCE)). It implements this logic already. > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:36 > +#if !OS(WINCE) > #include <shlwapi.h> > +#endif please move conditional includes after the regular ones Created attachment 162447 [details]
Patch v2
Used StringExtras.h and reordered includes.
Comment on attachment 162447 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=162447&action=review Nothing controversial, I'm glad to see that the patch is overall simple and doesn't require a lot of changes. But there are a few things that should be cleaned up IMO before this can go in. > Source/WTF/WTF.pri:41 > +wince* { > + # for mt19937ar.c > + INCLUDEPATH += $${ROOT_WEBKIT_DIR}/Source/ThirdParty > +} Build flags are added to WTF.pri if they're needed for the build of WTF itself _and_ other libraries using WTF. It seems that mt19937ar.c is only included from wtf/RandomNumber.cpp and not anywhere outside of WTF. There I think that this change should go into WTF.pro instead. > Source/WebCore/WebCore.pri:234 > +win* { I think this can be just written as win32, without the wild card. > Source/WebCore/WebCore.pri:238 > +win32-* { Would this perhaps be easier to read if it was written as win32:!wince* { ... ? The current mix of win32, win32-* and win* is hard to read. It would be nice to reduce the number of combinations :) > Source/WebCore/WebCore.pri:259 > + DEFINES += HAVE_LOCALTIME_S=0 ENABLE_JIT=0 ENABLE_FTPDIR=0 This is definitely the wrong place for ENABLE_JIT=0. Is it really needed here? I don't see it being enabled for WINCE in wtf/Platform.h, but if it is, then that would be the place to ensure that it's disabled. I think HAVE_LOCALTIME_S can be removed entirely, I don't see any code in WebKit actually using that define. What's the reason for disabling the FTP directory parsing? Created attachment 163128 [details]
Patch (sanitized)
Fix outstanding issues with the patch.
Comment on attachment 163128 [details] Patch (sanitized) View in context: https://bugs.webkit.org/attachment.cgi?id=163128&action=review > Source/WTF/wtf/Platform.h:799 > +#define ENABLE_FTPDIR 0 please never define a ENABLE_* with null -> merge it into the #if around the #define ENABLE_FTPDIR 1 but anyway i don't think Platform is the correct place for feature defines -> they should go into the build system Will fix right away, I've noticed that Platform.h defines don't even give me the desired result. Created attachment 163289 [details]
Patch
* Removed changes to Platform.h
* Added feature defines to features.prf (qmake)
Comment on attachment 163289 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163289&action=review > Source/WebCore/WebCore.pri:244 > + } > + else { } else { > Source/WebCore/WebCore.pri:-281 > - Extra hunk Created attachment 163332 [details]
Patch (extra hunk removed)
Created attachment 163342 [details]
Patch (removed newline after '}')
Comment on attachment 163342 [details] Patch (removed newline after '}') Rejecting attachment 163342 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: patching file Tools/qmake/mkspecs/features/default_post.prf Hunk #1 succeeded at 148 (offset 4 lines). patching file Tools/qmake/mkspecs/features/features.prf Hunk #1 FAILED at 109. 1 out of 1 hunk FAILED -- saving rejects to file Tools/qmake/mkspecs/features/features.prf.rej patching file Tools/qmake/mkspecs/features/functions.prf Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Simon Haus..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/13833287 Created attachment 163833 [details]
Patch (rebased)
Comment on attachment 163833 [details] Patch (rebased) View in context: https://bugs.webkit.org/attachment.cgi?id=163833&action=review > Source/WebCore/WebCore.pri:-270 > - unrelated change > Tools/Tools.pro:12 > + !wince*:build?(drt): SUBDIRS += DumpRenderTree/qt/DumpRenderTree.pro why not make build?(drt) return the correct value? > Tools/qmake/mkspecs/features/default_post.prf:174 > +win32-msvc*|win32-icc|wince*: INCLUDEPATH += $$ROOT_WEBKIT_DIR/Source/JavaScriptCore/os-win32 is there any msvc test? Comment on attachment 163833 [details] Patch (rebased) View in context: https://bugs.webkit.org/attachment.cgi?id=163833&action=review >> Tools/Tools.pro:12 >> + !wince*:build?(drt): SUBDIRS += DumpRenderTree/qt/DumpRenderTree.pro > > why not make build?(drt) return the correct value? Yes, this can now go into finalizeConfigure of configure.prf, similar to how we disable things for production_build Created attachment 163856 [details]
Patch (sanitized)
* Removed build_drt on WinCE.
* Removed unrelated change
I'm not sure what is meant by "is there any msvc test?".
Attachment 163856 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Tools/ChangeLog:11: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 17 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #20) > Created an attachment (id=163856) [details] > Patch (sanitized) > > * Removed build_drt on WinCE. > * Removed unrelated change > > I'm not sure what is meant by "is there any msvc test?". Sorry for the unclear comment: in WebKit there is a #if COMPILER(MSVC), since the missing stdint.h is compiler specific and not platform specific. Created attachment 163857 [details]
Patch (fixed whitespace)
Comment on attachment 163857 [details]
Patch (fixed whitespace)
The world of porting and compilers is cruel and painful ;). I think this is pretty good and worth landing. Clearly an improvement and further changes can be easily done incrementally.
Comment on attachment 163857 [details] Patch (fixed whitespace) Clearing flags on attachment: 163857 Committed r128558: <http://trac.webkit.org/changeset/128558> All reviewed patches have been landed. Closing bug. |