Bug 45173 - [Qt] Refactor MiniBrowser
Summary: [Qt] Refactor MiniBrowser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Zoltan Horvath
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-03 06:45 PDT by Zoltan Horvath
Modified: 2010-09-08 04:21 PDT (History)
3 users (show)

See Also:


Attachments
proposed patch (8.99 KB, patch)
2010-09-03 06:48 PDT, Zoltan Horvath
no flags Details | Formatted Diff | Diff
proposed patch (9.21 KB, patch)
2010-09-03 07:01 PDT, Zoltan Horvath
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zoltan Horvath 2010-09-03 06:45:59 PDT
Split BrowserWindow into two source files and headers. Remove unnecessary header includes.
Comment 1 Zoltan Horvath 2010-09-03 06:48:42 PDT
Created attachment 66487 [details]
proposed patch
Comment 2 WebKit Review Bot 2010-09-03 06:53:39 PDT
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.
Comment 3 Zoltan Horvath 2010-09-03 07:01:01 PDT
Created attachment 66489 [details]
proposed patch
Comment 4 WebKit Review Bot 2010-09-03 07:07:07 PDT
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.
Comment 5 Balazs Kelemen 2010-09-03 07:30:44 PDT
Seems reasonable to me.
Comment 6 Andras Becsi 2010-09-03 07:48:51 PDT
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.
Comment 7 Balazs Kelemen 2010-09-03 08:04:54 PDT
> 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 8 Simon Hausmann 2010-09-08 01:47:50 PDT
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.
Comment 9 Zoltan Horvath 2010-09-08 02:04:19 PDT
(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 10 Zoltan Horvath 2010-09-08 04:20:41 PDT
Comment on attachment 66489 [details]
proposed patch

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