Bug 57017 - [Qt] Properly detect phonon include, and avoid double qtLibraryTarget() call
: [Qt] Properly detect phonon include, and avoid double qtLibraryTarget() call
Status: RESOLVED FIXED
: WebKit
WebKit Qt
: 528+ (Nightly build)
: All All
: P1 Critical
Assigned To:
:
: Qt, QtTriaged
:
:
  Show dependency treegraph
 
Reported: 2011-03-24 08:19 PST by
Modified: 2011-03-31 12:30 PST (History)


Attachments
Properly-detect-phonon-include-and-avoid-double-qtLibraryTarget-call (2.88 KB, patch)
2011-03-24 08:23 PST, Kristian Amlie
no flags Review Patch | Details | Formatted Diff | Diff
Properly-detect-phonon-include-and-avoid-double-qtLibraryTarget-call-v2 (2.71 KB, patch)
2011-03-25 01:25 PST, Kristian Amlie
alexis: review-
alexis: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Changelog fix. (2.22 KB, patch)
2011-03-31 06:19 PST, Alexis Menard (darktears)
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-03-24 08:19:57 PST
This is needed for Qt Modularization.
------- Comment #1 From 2011-03-24 08:23:58 PST -------
Created an attachment (id=86776) [details]
Properly-detect-phonon-include-and-avoid-double-qtLibraryTarget-call
------- Comment #2 From 2011-03-24 10:27:45 PST -------
(From update of attachment 86776 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=86776&action=review

Why do you add the isEmpty(QT_SOURCE_TREE)?

> Source/WebCore/ChangeLog:5
> +        Properly detect phonon include, and avoid double qtLibraryTarget() call

Unecessary, you just copied the bug title.

> Source/WebKit/qt/ChangeLog:3
> +        Reviewed by NOBODY (OOPS!).

Unecessary, you just copied the bug title.
------- Comment #3 From 2011-03-25 01:20:47 PST -------
(In reply to comment #2)
> (From update of attachment 86776 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86776&action=review
> 
> Why do you add the isEmpty(QT_SOURCE_TREE)?

Because if the QtKernel sources are available (as indicated by the existence of QT_SOURCE_TREE), then qtLibraryTarget will already have been called from within qbase.pri. This can happen in the case where you build the modularized Qt modules in development mode, where the modules can see each others sources. If building for release, then this will not be the case, and the qtLibraryTarget needs to be called.

Long term, the reliance on qbase.pri in QT_SOURCE_TREE should be removed, but this will be some additional work.
------- Comment #4 From 2011-03-25 01:25:08 PST -------
Created an attachment (id=86908) [details]
Properly-detect-phonon-include-and-avoid-double-qtLibraryTarget-call-v2

Fixed ChangeLogs.
------- Comment #5 From 2011-03-29 10:27:20 PST -------
(From update of attachment 86908 [details])
Sorry to be picky Kristian but the changelog is not properly filled :(.

http://trac.webkit.org/wiki/QtWebKitContrib

Tells you how to do it.

In your case :
Precede the short description with "[Qt]" if your patch is specific to the Qt port of WebKit.
The bug link should be right under the description and then after the explanation comes with a . at the end of the sentence.

Thanks for the patch though.
------- Comment #6 From 2011-03-31 06:19:37 PST -------
Created an attachment (id=87712) [details]
Changelog fix.
------- Comment #7 From 2011-03-31 07:53:15 PST -------
Hey, sorry for the late response. I only just now got back to actual coding. :-P

(In reply to comment #5)
> (From update of attachment 86908 [details] [details])
> Sorry to be picky Kristian but the changelog is not properly filled :(.
> 
> http://trac.webkit.org/wiki/QtWebKitContrib
> 
> Tells you how to do it.
> 
> In your case :
> Precede the short description with "[Qt]" if your patch is specific to the Qt port of WebKit.
> The bug link should be right under the description and then after the explanation comes with a . at the end of the sentence.
> 
> Thanks for the patch though.

I guess I misinterpreted Benjamin's comment to mean "remove the bug description, and leave a standalone description". Thanks for fixing that though!
------- Comment #8 From 2011-03-31 12:27:17 PST -------
The commit-queue encountered the following flaky tests while processing attachment 87712 [details]:

fast/workers/storage/use-same-database-in-page-and-workers.html bug 50995 (author: dumi@chromium.org)
The commit-queue is continuing to process your patch.
------- Comment #9 From 2011-03-31 12:30:46 PST -------
(From update of attachment 87712 [details])
Clearing flags on attachment: 87712

Committed r82605: <http://trac.webkit.org/changeset/82605>
------- Comment #10 From 2011-03-31 12:30:50 PST -------
All reviewed patches have been landed.  Closing bug.