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
Product: WebKit
Classification: Unclassified
Component: WebKit Qt
: 528+ (Nightly build)
: All All
: P1 Critical
Assigned To: Nobody
: Qt, QtTriaged
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-24 08:19 PDT by Kristian Amlie
Modified: 2011-03-31 12:30 PDT (History)
2 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Kristian Amlie 2011-03-24 08:19:57 PDT
This is needed for Qt Modularization.
Comment 1 Kristian Amlie 2011-03-24 08:23:58 PDT
Created attachment 86776 [details]
Properly-detect-phonon-include-and-avoid-double-qtLibraryTarget-call
Comment 2 Benjamin Poulain 2011-03-24 10:27:45 PDT
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.
Comment 3 Kristian Amlie 2011-03-25 01:20:47 PDT
(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.
Comment 4 Kristian Amlie 2011-03-25 01:25:08 PDT
Created attachment 86908 [details]
Properly-detect-phonon-include-and-avoid-double-qtLibraryTarget-call-v2

Fixed ChangeLogs.
Comment 5 Alexis Menard (darktears) 2011-03-29 10:27:20 PDT
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.
Comment 6 Alexis Menard (darktears) 2011-03-31 06:19:37 PDT
Created attachment 87712 [details]
Changelog fix.
Comment 7 Kristian Amlie 2011-03-31 07:53:15 PDT
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!
Comment 8 WebKit Commit Bot 2011-03-31 12:27:17 PDT
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 WebKit Commit Bot 2011-03-31 12:30:46 PDT
Comment on attachment 87712 [details]
Changelog fix.

Clearing flags on attachment: 87712

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