Bug 64734

Created attachment 101175 [details] Patch

Comment on attachment 101175 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101175&action=review You still need a small description. The point is that someone should be able to understand why some code is like it is. This is valuable for the reviewer, and for developers in the future who will modify that code. > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) You should remove this line in this case.

Adding people in CC who know way more than me about this build process madness :)

Created attachment 101178 [details] Patch

Comment on attachment 101175 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101175&action=review This patch was tested outside the make-package case? Normal build with build-webkit --qt --release and WEBKIT_OUTPUT_DIR=/my/path build-webkit --qt --release. What about inside Qt? > Source/WebKit/qt/Api/DerivedSources.pro:47 > + CONFIG(standalone_package): PATH_TO_HEADER = ../../Source/WebKit/qt/Api/$$basename(HEADER) As I said on IRC, I don't think this is a good idea. How this will work with let say shadow build.

(In reply to comment #5) > (From update of attachment 101175 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=101175&action=review > > This patch was tested outside the make-package case? Normal build with build-webkit --qt --release and WEBKIT_OUTPUT_DIR=/my/path build-webkit --qt --release. What about inside Qt? > > > Source/WebKit/qt/Api/DerivedSources.pro:47 > > + CONFIG(standalone_package): PATH_TO_HEADER = ../../Source/WebKit/qt/Api/$$basename(HEADER) > > As I said on IRC, I don't think this is a good idea. How this will work with let say shadow build. WEBKITOUTPUTDIR sorry.

Created attachment 101180 [details] Patch

I tested this patch with a standalone tarball (generated by make-package.py, after applying the patch) as well as an internal build using build-webkit --qt --release. Didn't test anything else yet, but if required I can do so.

(In reply to comment #5) > (From update of attachment 101175 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=101175&action=review > > This patch was tested outside the make-package case? Normal build with build-webkit --qt --release and WEBKIT_OUTPUT_DIR=/my/path build-webkit --qt --release. What about inside Qt? > > > Source/WebKit/qt/Api/DerivedSources.pro:47 > > + CONFIG(standalone_package): PATH_TO_HEADER = ../../Source/WebKit/qt/Api/$$basename(HEADER) > > As I said on IRC, I don't think this is a good idea. How this will work with let say shadow build. No, that was a different issue (or not?). This change doesn't move the generated headers at all. It just changes the content of those headers to refer to an existing path. Currently they'd point to "../../WebKit" which would be a root-level WebKit folder, which doesn't exist after running make-package.py

(In reply to comment #9) > (In reply to comment #5) > > (From update of attachment 101175 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=101175&action=review > > > > This patch was tested outside the make-package case? Normal build with build-webkit --qt --release and WEBKIT_OUTPUT_DIR=/my/path build-webkit --qt --release. What about inside Qt? > > > > > Source/WebKit/qt/Api/DerivedSources.pro:47 > > > + CONFIG(standalone_package): PATH_TO_HEADER = ../../Source/WebKit/qt/Api/$$basename(HEADER) > > > > As I said on IRC, I don't think this is a good idea. How this will work with let say shadow build. > > No, that was a different issue (or not?). This change doesn't move the generated headers at all. It just changes the content of those headers to refer to an existing path. Currently they'd point to "../../WebKit" which would be a root-level WebKit folder, which doesn't exist after running make-package.py Could you apply it on the 4.8 branch and test to see if it doesn't break anything?

Created attachment 109022 [details] updated patch Updated patch, fixes tests to link to the just built libQtWebKit, rather than using the system wide installed one

I've tested this now on various configurations: - external build against Qt 4.7 - internal build for Qt 4.7 - external build against Qt 4.8 - internal build for Qt 4.8 - build-webkit against Qt 4.7 It works fine for me in all of the above cases.

Thanks for catching this! Unfortunately 2.2.0 is virtually out already (everything but the public announcement is ready, it's with the marketing department), but this fix will be included in 2.2.1. It's an important bug, but not critical enough to make us cancel the release: my understanding is that building the whole Qt package with a replaced QtWebKit works fine (standalone_package = false) and so does the build with build-webkit --qt. BTW, Qt-4.8.0 may include QtWebKit-2.2.0, QtWebKit-2.2.1 or even QtWebKit-2.2.0 + a couple of patches. It'll be up to them.

(In reply to comment #13) > Unfortunately 2.2.0 is virtually out already (everything but the public announcement is ready, it's with the marketing department), but this fix will be included in 2.2.1. That's great news :) > It's an important bug, but not critical enough to make us cancel the release: my understanding is that building the whole Qt package with a replaced QtWebKit works fine (standalone_package = false) and so does the build with build-webkit --qt. Yes, of course. This bug is in no way a showstopper as it is only affecting an edge case. 2.2.1 is totally sufficient for that. I'd suppose the only people who stumble over this are packagers who want to ship it together with Qt 4.7.x, and those can just as easily be pointed to this bugreport/patch until 2.2.1 is out.

If this is still an open bug, I think the patch should be rebased to requested for review and commit?

I can rebase the patch against latest QtWebKit 2.2, but I wonder how much use it still is. Will there be another release of the 2.2 branch? What about Qt5? I can imagine the patch is fully obsolete there, but I haven't looked at the modularization yet.

Not relevant any more