Bug 82195 - [Qt] Fix includes after QtDeclarative -> QtQML renaming
Summary: [Qt] Fix includes after QtDeclarative -> QtQML renaming
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Ádám Kallai
URL:
Keywords: Qt, QtTriaged
Depends on: 82887
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-26 06:11 PDT by Csaba Osztrogonác
Modified: 2012-04-05 09:54 PDT (History)
8 users (show)

See Also:


Attachments
Build fix patch draft (18.89 KB, patch)
2012-03-27 07:31 PDT, Ádám Kallai
no flags Details | Formatted Diff | Diff
proposed patch (22.01 KB, patch)
2012-03-29 07:26 PDT, Ádám Kallai
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
proposed patch (22.01 KB, patch)
2012-03-29 08:25 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2012-03-26 06:11:43 PDT
After QtDeclarative was renamed to QtQML in Qt5, we get warnings about it during 
building QtWebKit. That's why Simon disabled -Werror for Qt5-Webkit builds.

It's time to fix includes in WebKit and enable -Werror again.
( https://lists.webkit.org/pipermail/webkit-qt/2012-March/002586.html )

Ádám started working on it, patch is coming soon.
Comment 1 Ádám Kallai 2012-03-27 07:31:28 PDT
Created attachment 134058 [details]
Build fix patch draft

Replaced QtDeclarative with QtQml with the help of the script found at:
https://lists.webkit.org/pipermail/webkit-qt/2012-March/002586.html

I only modified the parts that caused warnings while building:
"#warning Header <QtDeclarative/qdeclarativelist.h> is deprecated. Please include <QtQml/qqmllist.h> instead."

I would like to know if it needs to be replaced everywhere in Webkit Source or just at the most crucial parts. I tested it on Qt-4.8 and Qt-5.0 and the build was successful. 
I'm not sure if it's the best solution. Any idea and help would be appreciated.
Comment 2 Csaba Osztrogonác 2012-03-27 07:41:04 PDT
Comment on attachment 134058 [details]
Build fix patch draft

View in context: https://bugs.webkit.org/attachment.cgi?id=134058&action=review

And could you enable -Werror with this patch? ( http://trac.webkit.org/changeset/110422/trunk/Tools/qmake/mkspecs/features/unix/default_post.prf )
If you enable it, and click on "Submit for EWS analysis" we can see if it really works with Qt 4.8 and Qt 5.0 too. ;)

> Source/WebKit/qt/declarative/plugin.cpp:20
> +#include "qglobal.h"

What about including config.h instead of qglobal.h?

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:27
> +//#include <QtDeclarative/qdeclarativelist.h>

Remove this commented line.
Comment 3 Ádám Kallai 2012-03-29 07:26:34 PDT
Created attachment 134577 [details]
proposed patch

I didn't notice unnecessary line in my Previous patch.
If I use 'config.h' then WebKit does not build with Qt5 so I need to use 'qglobal.h'.

I enabled -Werr in this patch.
Comment 4 Early Warning System Bot 2012-03-29 07:33:00 PDT
Comment on attachment 134577 [details]
proposed patch

Attachment 134577 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12214019
Comment 5 Csaba Osztrogonác 2012-03-29 08:06:19 PDT
(In reply to comment #4)
> (From update of attachment 134577 [details])
> Attachment 134577 [details] did not pass qt-wk2-ews (qt):
> Output: http://queues.webkit.org/results/12214019

OMG, I was sure if it builds for me. And now I notice that I missed to update Qt5 on the Qt-WK2 EWS. I'm doing it immediately.
Comment 6 Csaba Osztrogonác 2012-03-29 08:25:51 PDT
Created attachment 134593 [details]
proposed patch

same as 134577, let the Qt-WK2 EWS be green :)
Comment 7 Simon Hausmann 2012-03-30 00:12:05 PDT
Comment on attachment 134593 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134593&action=review

r=me. I'll let Ossy set cq+ for a safe landing :)

> Source/WebKit2/UIProcess/API/qt/qwebnavigationhistory_p.h:34
> -#include <qdeclarative.h>
> +#include <QtQml/qqml.h>

This is more of a nitpick, but there is no reason to use #include <ModuleName/class-or-header.h> in private header files or cpp files. There is a reason for adding the "redundant" module name in public header files, but for private header files and cpp files we can rely on the presence of QT += module-name in the .pro file and therefore use the shorter #include <ClassName> include style.
Comment 8 Csaba Osztrogonác 2012-03-30 00:25:07 PDT
Comment on attachment 134593 [details]
proposed patch

cq- to let Ádám fix what Simon mentioned before landing.
Comment 9 Kristóf Kosztyó 2012-03-30 04:09:20 PDT
patch landed in r112651 http://trac.webkit.org/changeset/112651
Comment 10 Csaba Osztrogonác 2012-04-02 05:15:43 PDT
Reopen, because I had to roll it out. Unfortunately we had to go back to the previous Qt5 and it doesn't work with it.
Comment 11 Csaba Osztrogonác 2012-04-05 09:54:47 PDT
Qt5 is updated, so I relanded it - http://trac.webkit.org/changeset/113330