Split BrowserWindow into two source files and headers. Remove unnecessary header includes.
Created attachment 66487 [details] proposed patch
Attachment 66487 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKitTools/MiniBrowser/qt/BrowserView.h:29: #ifndef header guard has wrong style, please use: BrowserView_h [build/header_guard] [5] WebKitTools/MiniBrowser/qt/BrowserView.h:33: Alphabetical sorting problem. [build/include_order] [4] WebKitTools/MiniBrowser/qt/BrowserView.h:34: Alphabetical sorting problem. [build/include_order] [4] WebKitTools/MiniBrowser/qt/BrowserView.cpp:29: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebKitTools/MiniBrowser/qt/BrowserView.cpp:31: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 5 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 66489 [details] proposed patch
Attachment 66489 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKitTools/MiniBrowser/qt/BrowserView.h:33: Alphabetical sorting problem. [build/include_order] [4] WebKitTools/MiniBrowser/qt/BrowserView.h:34: Alphabetical sorting problem. [build/include_order] [4] WebKitTools/MiniBrowser/qt/BrowserView.cpp:29: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebKitTools/MiniBrowser/qt/BrowserView.cpp:32: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 4 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Seems reasonable to me.
The include order in BrowserView.h should probably be: #include "WKRetainPtr.h" #include "qgraphicswkview.h" #include <QGraphicsView> and in BrowserView.cpp: #include "WKContext.h" #include <QGraphicsScene> Do not bother with config.h, WebKit2 does not need to include it (btw this should be fixed in check-webkit-style). Otherwise LGTM.
> Do not bother with config.h, WebKit2 does not need to include it (btw this should be fixed in check-webkit-style). No, this should be fixed in WebKit2.
Comment on attachment 66489 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=66489&action=prettypatch > WebKitTools/MiniBrowser/qt/BrowserView.cpp:74 > +void BrowserView::load(const QUrl& url) > +{ > +#if QT_VERSION >= QT_VERSION_CHECK(4, 6, 0) > + return m_item->load(QUrl::fromUserInput(url.toString())); > +#else > + return m_item->load(url); > +#endif > +} This function should really be taking a QString as argument, not a QUrl. QUrl::toString() and then passing it to QUrl::fromUserInput is asking for trouble :) That's not your fault though, just a comment on the side. I also think it's safe to remove the QT_VERSION_CHECK here.
(In reply to comment #8) > (From update of attachment 66489 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=66489&action=prettypatch > > > WebKitTools/MiniBrowser/qt/BrowserView.cpp:74 > > +void BrowserView::load(const QUrl& url) > > +{ > > +#if QT_VERSION >= QT_VERSION_CHECK(4, 6, 0) > > + return m_item->load(QUrl::fromUserInput(url.toString())); > > +#else > > + return m_item->load(url); > > +#endif > > +} > This function should really be taking a QString as argument, not a QUrl. QUrl::toString() and then passing it to QUrl::fromUserInput is asking for trouble :) > > That's not your fault though, just a comment on the side. I also think it's safe to remove the QT_VERSION_CHECK here. I'm going to file a new bug for these after landing!
Comment on attachment 66489 [details] proposed patch http://trac.webkit.org/changeset/66969 Clearing flags. Committed revision 66969.