Bug 88313

Created attachment 145809 [details] Patch

Created attachment 145813 [details] Patch Typo fixed. Thanks GTK EWS. :)

Comment on attachment 145813 [details] Patch This is like a sledge-hammer :). Is it perhaps cleaner to do NOMINMAX only in those .cpp files where the clash occurs? That seems to be the more common solution and I think preferred if for example it only happens in qwebframe.cpp.

(In reply to comment #3) > (From update of attachment 145813 [details]) > This is like a sledge-hammer :). Is it perhaps cleaner to do NOMINMAX only in those .cpp files where the clash occurs? That seems to be the more common solution and I think preferred if for example it only happens in qwebframe.cpp. :) As far as I remember we need this for 5-10 files not only one. Let me check how many files need it.

problematic source files: - Source/WebKit/qt/Api/qwebview.cpp - Source/WebKit/qt/Api/gqraphicswebview.cpp - Source/WebKit/qt/Api/qwebpage.cpp - Source/WebKit/qt/Api/qwebhistory.cpp - Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp Patch is coming soon.

Created attachment 146300 [details] Patch Add NOMINMAX only to problematic sources instead of using sledgehammer :)

(In reply to comment #6) > Created an attachment (id=146300) [details] > Patch > > Add NOMINMAX only to problematic sources instead of using sledgehammer :) In most parts of the project we already use the sledgehammer approach. In config.h's you can find this - under #if OS(WINDOWS) // If we don't define these, they get defined in windef.h. // We want to use std::min and std::max. #ifndef max #define max max #endif #ifndef min #define min min #endif What I don't know is that which config.h is used in Source/WebKit/qt and why it does not have this. Furthermore, adding NOMINMAX seems more elegant than this ifdef hackery but I'm not sure it would work on every platform. I favor fixing it in the appropriate config.h for consistency.

WebCore/config.h is used by WebKit/qt. We "#define max max" correctly in config.h but the problem is that qdatetime.h does "#undef max" before WinDef.h gets included. How the hell did this end up in a public Qt header...

(In reply to comment #8) > WebCore/config.h is used by WebKit/qt. > > We "#define max max" correctly in config.h but the problem is that qdatetime.h does "#undef max" before WinDef.h gets included. > > How the hell did this end up in a public Qt header... Well spotted!! And pretty recently changed, too: https://codereview.qt-project.org/#change,13166 The header file uses std::min/max in an inline function, but yeah, the fix is there IMHO.

Work in progress at https://codereview.qt-project.org/#change,28549 - If we can't get that into Qt, then we have to change the existing workaround in WebKit to use NOMINMAX instead of #define min min/max max

(In reply to comment #10) > Work in progress at https://codereview.qt-project.org/#change,28549 - If we can't get that into Qt, then we have to change the existing workaround in WebKit to use NOMINMAX instead of #define min min/max max I tried it, it works for me without workaround for WebKit. ;-)

(In reply to comment #11) > (In reply to comment #10) > > Work in progress at https://codereview.qt-project.org/#change,28549 - If we can't get that into Qt, then we have to change the existing workaround in WebKit to use NOMINMAX instead of #define min min/max max > > I tried it, it works for me without workaround for WebKit. ;-) Great. The change is slowly making its way into Qt. Most of the submodules are patched already and the qtbase change is staged (after the n-th try :) Let's close this as INVALID then, because the root cause for this bug is an upstream issue that disabled the existing workaround.

Comment on attachment 146300 [details] Patch Clearing review