Bug 45173 – [Qt] Refactor MiniBrowser

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Zoltan Horvath]

Split BrowserWindow into two source files and headers. Remove unnecessary header includes.

[Zoltan Horvath]

Created attachment 66487 [details]
proposed patch

[WebKit Review Bot]

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.

[Zoltan Horvath]

Created attachment 66489 [details]
proposed patch

[WebKit Review Bot]

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.

[Balazs Kelemen]

Seems reasonable to me.

[Andras Becsi]

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.

[Balazs Kelemen]

> 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.

[Simon Hausmann]

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.

[Zoltan Horvath]

(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!

[Zoltan Horvath]

Comment on attachment 66489 [details]
proposed patch

http://trac.webkit.org/changeset/66969
Clearing flags.
Committed revision 66969.