Bug 104909

Created attachment 179263 [details] Patch

Tell me if I'm overseeing cases where we would need to override this in the WebKit's configure step. This patch is only valid if we can agree on this.

If we do this, then we must also change the way we link against libpng. IOW: Currently we use pkg-config to _discover_ the presence of libpng/libjpeg and we also use to determine the include search paths, compiler flags, library search path and library name (through PKGCONFIG += libpng) If we use the system-png to determine the presence, then we should also link against it like Qt, i.e. rely other ways of determining the include search path for example. From qpnghandler.pri: contains(QT_CONFIG, system-png) { if(unix|win32-g++*): LIBS_PRIVATE += -lpng else:win32: LIBS += libpng.lib } Also, if we do this, then we should do it consistently also for the other third party libraries like sqlite (which I recently just changed over from system-sqlite to use pkg-config ;-)

It would also be good if this bug report would explain the reasoning for this change, so that if we decide to go for it and later come back we have the arguments in this bug report :)

Comment on attachment 179263 [details] Patch Attachment 179263 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15316281

Comment on attachment 179263 [details] Patch Yep so I agree that this is not a good solution, but the current situation might soon be problematic too. I talked with Tobias a bit and we'll run into problems if Qt is configured to use any other version than the system default path's libraries and that we try to load two versions of the same library as a result of WebKit having its own system dependencies. This hasn't been a problem before since we always religiously tried not to use anything outside qt/src/3rdparty. A solution would be to delegate all our configuration work to Qt and get the result down to every modules. "Oh you need sqlite? Here I did the work for you, just add this and this to your build flags." If necessary, the need behind this patch is mostly explained in the ChangeLog.

(In reply to comment #6) > (From update of attachment 179263 [details]) > Yep so I agree that this is not a good solution, but the current situation might soon be problematic too. I think it depends. The current solution for example is for example rather suitable for cross-compilation with sysroots and pkg-config. > I talked with Tobias a bit and we'll run into problems if Qt is configured to use any other version than the system default path's libraries and that we try to load two versions of the same library as a result of WebKit having its own system dependencies. > This hasn't been a problem before since we always religiously tried not to use anything outside qt/src/3rdparty. > > A solution would be to delegate all our configuration work to Qt and get the result down to every modules. > "Oh you need sqlite? Here I did the work for you, just add this and this to your build flags." Yes. Either we let Qt do all the work (discovery, build and link settings) or _both_ use an external mechanism like pkg-config. Right now we have a hybrid in the sense that Qt uses pkg-config for many libs but not all and WebKit always uses pkg-config. Wouldn't it be another option btw to configure Qt with -no-pkg-config? WebKit should obey that. Oh wait, the current situation is not how I described it in WebKit, it's in fact worse. We don't even use pkg-config to _discover_ libpng. Argh. > If necessary, the need behind this patch is mostly explained in the ChangeLog. Ooops, sorry, I was too quick on that one :)

Comment on attachment 179263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179263&action=review > Tools/qmake/mkspecs/features/features.prf:48 > - config_libjpeg: WEBKIT_CONFIG += use_libjpeg > - else: CONFIGURE_WARNINGS += "JPEG library not found, QImageDecoder will decode JPEG images" > - > - config_libpng: WEBKIT_CONFIG += use_libpng > - else: CONFIGURE_WARNINGS += "PNG library not found, QImageDecoder will decode PNG images" > + # We can't use Qt's 3rdparty sources for libjpeb and libpng outside of qtbase, but if Qt > + # is using the system libraries, use them to take advantage of the WebCore image decoders as well. > + contains(QT_CONFIG, system-jpeg): WEBKIT_CONFIG += use_libjpeg > + contains(QT_CONFIG, system-png): WEBKIT_CONFIG += use_libpng Shouldn't we still issue a warning? I mean, the choice of _not_ having system-jpeg comes at a performance cost that the CONFIGURE_WARNINGS += should sort of convey, no? Otherwise this is clearly an improvement over the current situation. Although I'm starting to think that we should use pkg-config instead :)

Created attachment 179466 [details] Patch Ok, I'm still concerned that this might break things for common mortals in the future in favor of the few that needs to package redistributables, but until then this can be considered a step forward. This patch version only re-add the warnings.

Comment on attachment 179466 [details] Patch Attachment 179466 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15314641

Comment on attachment 179466 [details] Patch Attachment 179466 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15309690

Comment on attachment 179466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179466&action=review r=me. It's strange that the EWS is complaining. Land with care manually I'd say :) > Tools/qmake/mkspecs/features/features.prf:45 > + # We can't use Qt's 3rdparty sources for libjpeb and libpng outside of qtbase, but if Qt libjpeb -> libjpeg

Created attachment 182542 [details] Patch Fixed the type and re-uploading to see if the EWS is still broken.

Comment on attachment 182542 [details] Patch Attachment 182542 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15853497 New failing tests: svg/as-image/img-relative-height.html

Comment on attachment 182542 [details] Patch It seems like it works now on the Qt bots, CQing it.

Comment on attachment 182542 [details] Patch Rejecting attachment 182542 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue /mnt/git/webkit-commit-queue/Tools/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/15841485

(In reply to comment #15) > (From update of attachment 182542 [details]) > It seems like it works now on the Qt bots, CQing it. We always build Qt with "-qt-libpng -qt-libjpeg". I don't know why, but somebody told us to use it a long time ago, and we've never changed/reconsidered config options. Now we use WebCore's imagedecoders and this change will force us to use qt's imagedecoders. I'm not sure if the qmake build system handles this situation correctly ...

Comment on attachment 182542 [details] Patch Clearing flags on attachment: 182542 Committed r139609: <http://trac.webkit.org/changeset/139609>

All reviewed patches have been landed. Closing bug.

(In reply to comment #17) > We always build Qt with "-qt-libpng -qt-libjpeg". I don't know why, > but somebody told us to use it a long time ago, and we've never > changed/reconsidered config options. > > Now we use WebCore's imagedecoders and this change will force us > to use qt's imagedecoders. I'm not sure if the qmake build system > handles this situation correctly ... In that case it will switch to the qt's image decoders yep. It would be better if WebKit continued using the system libraries. If we would now use the system libpng and libjpeg, it feels to me that any advantage that those configure switches would give was lost when we decided that WebKit would use the system libraries. This is also an example why I think it's nice to have WebKit follow Qt's behavior. Could you try removing those configure switches or maybe try to remember what would be the benefit of keeping them?

(In reply to comment #20) > Could you try removing those configure switches or maybe try to remember what would be the benefit of keeping them? Sure, will check near the next Qt update. But as far as I remember the benefit was that we didn't want to add libjpeg/libpng as build dependency long long time ago near Qt 4.5 / 4.6. (2-3 years before)

I tried the released Qt 5.0.0 without "-qt-libpng -qt-libjpeg" configure options, and tests work fine with system libpng/libjpeg. We had to skip 3 tests because of http://trac.webkit.org/changeset/139609: - fast/events/mouse-cursor-multiframecur.html - fast/events/mouse-cursor.html - fast/images/icon-decoding.html Will unskip after Qt update.