Bug 57017 – [Qt] Properly detect phonon include, and avoid double qtLibraryTarget() call

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

[Kristian Amlie]

This is needed for Qt Modularization.

[Kristian Amlie]

Created attachment 86776 [details]
Properly-detect-phonon-include-and-avoid-double-qtLibraryTarget-call

[Benjamin Poulain]

Comment on attachment 86776 [details]
Properly-detect-phonon-include-and-avoid-double-qtLibraryTarget-call

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.

[Kristian Amlie]

(In reply to comment #2)
> (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)?

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.

[Kristian Amlie]

Created attachment 86908 [details]
Properly-detect-phonon-include-and-avoid-double-qtLibraryTarget-call-v2

Fixed ChangeLogs.

[Alexis Menard (darktears)]

Comment on attachment 86908 [details]
Properly-detect-phonon-include-and-avoid-double-qtLibraryTarget-call-v2

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.

[Alexis Menard (darktears)]

Created attachment 87712 [details]
Changelog fix.

[Kristian Amlie]

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])
> 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!

[WebKit Commit Bot]

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: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 87712 [details]
Changelog fix.

Clearing flags on attachment: 87712

Committed r82605: <http://trac.webkit.org/changeset/82605>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.