Bug 95536 – Make compile with both OS(WINCE) and PLATFORM(QT) support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Kevin Funk]

Patches for qmake / source files following.

[Kevin Funk]

Created attachment 162004 [details]
Patch

[WebKit Review Bot]

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.

[Kevin Funk]

Created attachment 162018 [details]
Patch (fixed whitespace issues)

[WebKit Review Bot]

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.

[Kevin Funk]

Created attachment 162038 [details]
Patch (really fixed whitespace issues)

[Patrick R. Gansterer]

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

[Kevin Funk]

Created attachment 162447 [details]
Patch v2

Used StringExtras.h and reordered includes.

[Simon Hausmann]

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?

[Kevin Funk]

Created attachment 163128 [details]
Patch (sanitized)

Fix outstanding issues with the patch.

[Patrick R. Gansterer]

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

[Kevin Funk]

Will fix right away, I've noticed that Platform.h defines don't even give me the desired result.

[Kevin Funk]

Created attachment 163289 [details]
Patch

* Removed changes to Platform.h
* Added feature defines to features.prf (qmake)

[Tor Arne Vestbø]

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

[Kevin Funk]

Created attachment 163332 [details]
Patch (extra hunk removed)

[Kevin Funk]

Created attachment 163342 [details]
Patch (removed newline after '}')

[WebKit Review Bot]

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

[Kevin Funk]

Created attachment 163833 [details]
Patch (rebased)

[Patrick R. Gansterer]

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?

[Tor Arne Vestbø]

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

[Kevin Funk]

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?".

[WebKit Review Bot]

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.

[Patrick R. Gansterer]

(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.

[Kevin Funk]

Created attachment 163857 [details]
Patch (fixed whitespace)

[Simon Hausmann]

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.

[WebKit Review Bot]

Comment on attachment 163857 [details]
Patch (fixed whitespace)

Clearing flags on attachment: 163857

Committed r128558: <http://trac.webkit.org/changeset/128558>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.