Bug 40050 – [Qt] Upstream the WebKit QML integration plugin

• Component: WebKit Qt
• Status: CLOSED FIXED

[Simon Hausmann]

Qt 4.7 provides an integration of WebKit into QML. The integration is provided through a plugin located in the src/imports/webkit directory. It is self-contained and only depends on public QtDeclarative and QtWebKit API.

In order to implement new optimizations and in order to reduce the dependency of code inside Qt on WebKit, the QML WebKit integration should be moved into the upstream QtWebKit development.

A possible location would be WebKit/qt/declarative

[Kenneth Rohde Christiansen]

So what is the idea?

It is yet another view of a QWebPage, not benefiting from accelerated compositing, it has another kind of tiling implementation, etc.

Can we somehow reuse or sync this with the the QGraphicsWebView?

[Alexis Menard (darktears)]

No problem Kenneth the QML component is using QGraphicsWebView. It's just add some properties and some basic interactions.

[Kenneth Rohde Christiansen]

(In reply to comment #2)
> No problem Kenneth the QML component is using QGraphicsWebView. It's just add some properties and some basic interactions.

That is great! I wonder if we need to enforce some settings, like you surely want to use it with tiling enabled right, frame flattening etc. We need good defaults, but I'm not quite sure how to deal with this yet.

[Simon Hausmann]

(In reply to comment #3)
> (In reply to comment #2)
> > No problem Kenneth the QML component is using QGraphicsWebView. It's just add some properties and some basic interactions.
> 
> That is great! I wonder if we need to enforce some settings, like you surely want to use it with tiling enabled right, frame flattening etc. We need good defaults, but I'm not quite sure how to deal with this yet.

The current implementation - which you can find in Qt's 4.7 branch - uses the tiled backing store. That replaces the previous tiling implementation based on Qt Declarative's QDeclarativePaintedItem.

One thing QDeclarativeWebView should set though - and it doesn't right now - is the frame flattening.

[Kenneth Rohde Christiansen]

What about the viewport hints (viewpost meta tag)?

Some of our documentation like in QWebSettings says that for instance FrameFlattening is disabled by default. Should we remove these from the docs?

[Alexis Menard (darktears)]

Created attachment 57756 [details]
Patch for the import

[WebKit Review Bot]

Attachment 57756 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
WebKit/qt/declarative/qdeclarativewebview.cpp:21:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/qt/declarative/qdeclarativewebview.cpp:43:  MAX_DOUBLECLICK_TIME is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/qt/declarative/plugin.cpp:20:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/qt/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
WebKit/qt/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
WebKit/qt/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
WebKit/qt/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
WebKit/qt/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 9 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 57756 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/2979031

[Kenneth Rohde Christiansen]

Comment on attachment 57756 [details]
Patch for the import

OK, here are some initial style explanations. Please understand these carefully and fix them through-out the code.

There is a style bot checking the style, but it only checks a subset so please read the style guide on www.webkit.org


> diff --git a/ChangeLog b/ChangeLog
> index ef17c28..e502688 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,14 @@
> [phone]  Alexis Menard  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [Qt] Upstream the WebKit QML integration plugin
> +        https://bugs.webkit.org/show_bug.cgi?id=40050
> +
> +		Add to the build the QML WebKit integration plugin.

Wrong indentation


> +
> +        [Qt] Upstream the WebKit QML integration plugin
> +        https://bugs.webkit.org/show_bug.cgi?id=40050
> +
> +		Add to the Qt port the QML webkit integration plugin.
> +		QDeclarativeWebView is creating the item that is added
> +		to the QGraphicsScene. The .h file is not meant to be
> +		public.
> +
> +		QmlDir is describing the plugin.

Wrong indentation again. Plus it is called WebKit, not webkit.


> +
> +#include "qdeclarativewebview_p.h"
> +#include "qdeclarativewebview_p_p.h"

A private of a private? :)

> +
> +#include <QtDeclarative/qdeclarative.h>
> +#include <QtDeclarative/qdeclarativeextensionplugin.h>
> +
> +QT_BEGIN_NAMESPACE
> +
> +class WebKitQmlPlugin : public QDeclarativeExtensionPlugin {
> +    Q_OBJECT
> +public:
> +    virtual void registerTypes(const char *uri)

* should be left aligned.

> +
> +static const int MAX_DOUBLECLICK_TIME = 500; // XXX need better gesture system

In WebKit do not use capitals for globals. maxDoubleClickTime is fine. 
Do not use XXX, but use FIXME:

> +
> +class QDeclarativeWebViewPrivate {
> +public:
> +    QDeclarativeWebViewPrivate(QDeclarativeWebView* qq)
> +      : q(qq), page(0), preferredwidth(0), preferredheight(0),
> +            progress(1.0), status(QDeclarativeWebView::Null), pending(PendingNone),
> +            newWindowComponent(0), newWindowParent(0),
> +            pressTime(400),
> +            rendering(true)


Put each on one line and add the comma (,) to the right, like

    : q(qq)
    , page(0)
    , etc


> +    int pressTime; // milliseconds before it's a "hold"

What is a hold? could the name be improed?

> +
> +    static void windowObjectsAppend(QDeclarativeListProperty<QObject> *prop, QObject *o)

* is aligned wrongly

> +    {
> +        static_cast<QDeclarativeWebViewPrivate *>(prop->data)->windowObjects.append(o);

No space between Private and *

> +        static_cast<QDeclarativeWebViewPrivate *>(prop->data)->updateWindowObjects();
> +    }
> +
> +    void updateWindowObjects();
> +    QObjectList windowObjects;
> +
> +    bool rendering;
> +};
> +
> +/*!
> +    \qmlclass WebView QDeclarativeWebView
> +  \since 4.7

Something went wrong with indentation here

> +void QDeclarativeWebView::doLoadFinished(bool ok)
> +{
> +
> +    if (title().isEmpty())
> +        pageUrlChanged(); // XXX bug 232556 - pages with no title never get urlChanged()

Qt bug? Again do not use XXX and please add the full url.

> +void QDeclarativeWebView::setPreferredWidth(int iw)

iw? Use better variable names, like 'width


> +void QDeclarativeWebView::setPreferredHeight(int ih)

Here as well

> +QVariant QDeclarativeWebView::evaluateJavaScript(const QString &scriptSource)

& is aligned wrongly

> +{
> +    return this->page()->mainFrame()->evaluateJavaScript(scriptSource);
> +}
> +
> +void QDeclarativeWebView::propagateFocusToWebPage(bool hasFocus)
> +{
> +    QFocusEvent e(hasFocus ? QEvent::FocusIn : QEvent::FocusOut);

write e out as event or at least 'ev'

> +    page()->event(&e);


> +void QDeclarativeWebView::updateContentsSize()
> +{
> +    if (d->page)
> +        d->page->setPreferredContentsSize(QSize(
> +            d->preferredwidth>0 ? d->preferredwidth : width(),
> +            d->preferredheight>0 ? d->preferredheight : height()));
> +}
ly
According to the style guide this will need braces and the contents of the if textually spans multiple lines (incl comments if any)


> +
> +void QDeclarativeWebView::geometryChanged(const QRectF &newGeometry,
> +                                 const QRectF &oldGeometry)

& alignment

> +{
> +    if (newGeometry.size() != oldGeometry.size() && d->page) {
> +        QSize cs = d->page->preferredContentsSize();

cs is a badly named variable

[Alexis Menard (darktears)]

Created attachment 57769 [details]
Version 2

Fixes coding style and other feedback.

[WebKit Review Bot]

Attachment 57769 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/qt/declarative/qdeclarativewebview.cpp:21:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/qt/declarative/plugin.cpp:20:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexis Menard (darktears)]

Created attachment 57770 [details]
Version 3

Manage to get some URL for the bugs inside the code.

[WebKit Review Bot]

Attachment 57770 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/qt/declarative/qdeclarativewebview.cpp:21:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/qt/declarative/plugin.cpp:20:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 57770 [details]
Version 3

WebKit/qt/declarative/qdeclarativewebview.cpp:226
 +  
unneeded newline :-)

WebKit/qt/declarative/qdeclarativewebview.cpp:246
 +      if ((d->url.isEmpty() && page()->mainFrame()->url() != QUrl(QLatin1String("about:blank")))
Why do you control an url locally? Why not always use the url from the mainFrame? you could even make an inline accessor for it.

WebKit/qt/declarative/qdeclarativewebview.cpp:287
 +  void QDeclarativeWebView::setUrl(const QUrl &url)
& should be to the left

WebKit/qt/declarative/qdeclarativewebview.cpp:314
 +  int QDeclarativeWebView::preferredWidth() const
Be careful, we already have an API called setPreferredSize or something similar, which actually forces layout at that size and not of the max(preferred, min required my contents)

WebKit/qt/declarative/qdeclarativewebview.cpp:319
 +  void QDeclarativeWebView::setPreferredWidth(int width)
Is this preferredwidth/height a QML concept?

WebKit/qt/declarative/qdeclarativewebview.cpp:361
 +  void QDeclarativeWebView::propagateFocusToWebPage(bool hasFocus)
The hasFocus seems confusing here given the name of the method. It should like a setting, but it is not. Simon?

WebKit/qt/declarative/qdeclarativewebview.cpp:389
 +                                   const QRectF& oldGeometry)
function definitions should never include newline, thus they should be on one line only :-) even if that line spans 300 chars!

WebKit/qt/declarative/qdeclarativewebview.cpp:393
 +          if (widthValid())
widthValid seems like a strange name, what about hasValidWidth? widthIsValid? I prefer the former

WebKit/qt/declarative/qdeclarativewebview.cpp:455
 +      for (int ii = 0; ii < windowObjects.count(); ++ii) {
int ii? Why not use i? or it? or something more saying

WebKit/qt/declarative/qdeclarativewebview.cpp:457
 +          QDeclarativeWebViewAttached* attached = static_cast<QDeclarativeWebViewAttached *>(qmlAttachedPropertiesObject<QDeclarativeWebView>(object));
Sometimes we make casting methos like toQDeclarativeWebViewAttached(QObject* object). This can be quite handy for adding ASSERTS to the cast, plus it makes the code look cleaner. It should be inline though

WebKit/qt/declarative/qdeclarativewebview.cpp:478
 +  QMouseEvent* QDeclarativeWebView::sceneMouseEventToMouseEvent(QGraphicsSceneMouseEvent* e)
For consistency use ev' as we are trying to use that elsewhere.

WebKit/qt/declarative/qdeclarativewebview.cpp:478
 +  QMouseEvent* QDeclarativeWebView::sceneMouseEventToMouseEvent(QGraphicsSceneMouseEvent* e)
Oh btw, we have the other methods for handling mouse etc in the QWebPagePrivate, I believe. I guess this code belongs there.

WebKit/qt/declarative/qdeclarativewebview.cpp:511
 +      \qmlsignal WebView::onDoubleClick(clickx,clicky)
space after , ?

WebKit/qt/declarative/qdeclarativewebview.cpp:539
 +  bool QDeclarativeWebView::heuristicZoom(int clickX, int clickY, qreal maxzoom)
maxzoom should be maxZoom. Btw if you are using tiling you DO NOT want to use zoom, you want to use SCALE instead :)

WebKit/qt/declarative/qdeclarativewebview.cpp:543
 +      qreal ozf = contentsScale();
ozf? I have no idea what that is short for.

WebKit/qt/declarative/qdeclarativewebview.cpp:544
 +      QRect showarea = elementAreaAt(clickX, clickY, d->preferredwidth / maxzoom, d->preferredheight / maxzoom);
showArea... that is now we name variables.

WebKit/qt/declarative/qdeclarativewebview.cpp:549
 +          QRectF r(showarea.left()*z, showarea.top() * z, showarea.width() * z, showarea.height() * z);
showarea.left()*z should have spaces around the *

WebKit/qt/declarative/qdeclarativewebview.cpp:571
 +  void QDeclarativeWebView::setPressGrabTime(int ms)
I think WebKit normally uses millis, you can do a greb and check.

WebKit/qt/declarative/qdeclarativewebview.cpp:599
 +    */
Comments in code uses // and starts with a capital and ends with a punctuation mark (dot). I personally try to make them stay within 100 chars, but that is just me.

WebKit/qt/declarative/qdeclarativewebview.cpp:743
 +  
Why so many newlines here ?

WebKit/qt/declarative/qdeclarativewebview.cpp:750
 +      return page()->mainFrame()->icon().pixmap(QSize(256, 256));
Where does that size come from? Make it either a global value with a descriptive name or add a comment.

WebKit/qt/declarative/qdeclarativewebview.cpp:758
 +  void QDeclarativeWebView::setZoomFactor(qreal factor)
I still guess that we want to enforce tiling and use scale instead of zoom factor.

WebKit/qt/declarative/qdeclarativewebview.cpp:780
 +  void QDeclarativeWebView::setStatusText(const QString& s)
call s for text

WebKit/qt/declarative/qdeclarativewebview.cpp:798
 +  
unneeded newline

WebKit/qt/declarative/qdeclarativewebview.cpp:799
 +      if (!d->page) {
You can avoid indentating the below lines by saying
if (d->page)
    return d->page;

instead, always a good thing.

WebKit/qt/declarative/qdeclarativewebview.cpp:856
 +  QDeclarativeWebSettings* QDeclarativeWebView::settingsObject() const
we need to be VERY careful with which settings we really want to expose to QML.

WebKit/qt/declarative/qdeclarativewebview.cpp:862
 +  void QDeclarativeWebView::setPage(QWebPage* page)
How is the whole PageClient handled? This needs to be thought out!

WebKit/qt/declarative/qdeclarativewebview.cpp:923
 +            const QByteArray &body)
Function definition on one line please

WebKit/qt/declarative/qdeclarativewebview.cpp:945
 +  void QDeclarativeWebView::setHtml(const QString &html, const QUrl &baseUrl)
& placement

WebKit/qt/declarative/qdeclarativewebview.cpp:958
 +  void QDeclarativeWebView::setContent(const QByteArray &data, const QString &mimeType, const QUrl &baseUrl)
& placement

WebKit/qt/declarative/qdeclarativewebview.cpp:999
 +                      delete nobj;
newObject?

WebKit/qt/declarative/qdeclarativewebview.cpp:1011
 +              }
This is a one line, thus must not have braces :-) yeah I know, different from Qt style guide.

WebKit/qt/declarative/qdeclarativewebview.cpp:1105
 +          maxwidth = INT_MAX;
I think we normally use the max from stl.

WebKit/qt/declarative/qdeclarativewebview.cpp:1105
 +          maxwidth = INT_MAX;
should be maxWidth.

WebKit/qt/declarative/qdeclarativewebview.cpp:1112
 +      return rv;
rv?

WebKit/qt/declarative/qdeclarativewebview.cpp:1133
 +      qWarning() << sourceID << ':' << lineNumber << ':' << message;
Do you really want to upstream this? it seems like debugging code

WebKit/qt/declarative/qdeclarativewebview.cpp:1136
 +  QString QDeclarativeWebPage::chooseFile(QWebFrame *originatingFrame, const QString& oldFile)
* placement

Still a lot to check, but this is it for now.

[Kenneth Rohde Christiansen]

Jesus, can you please look at the WebPageClient issues and how we should go about them? or at least talk to Alexis about it.

[Jesus Sanchez-Palencia]

(In reply to comment #15)
> Jesus, can you please look at the WebPageClient issues and how we should go about them? or at least talk to Alexis about it.

I looked at the patch and I think that nothing else is needed from the PageClient perspective since you use a QGraphicsWebView and you call its setPage. In this function a PageClient is created for the view, so right now your view might be able to use Accelerated Compositing and Tiling just out of the box, but this need to be tested. In the worst case scenario there will be some hooks missing somewhere, but nothing that taking a look at QtTestBrowser and yberbrowser wouldn't point out.

About the patch itself, I had to remove the "private-private" includes (something_p_p.h) from two files and there was a duplicate forward declaration 'class QDeclarativeSomething;' in the .cpp. :)

[Alexis Menard (darktears)]

Created attachment 58989 [details]
version 4

Refactor, remove the private private header and change the way the items are organized.

[WebKit Review Bot]

Attachment 58989 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/qt/declarative/qdeclarativewebview.cpp:21:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/qt/declarative/plugin.cpp:20:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 58989 [details]
version 4

Almost there.

WebKit/qt/ChangeLog:8
 +          Add to the Qt port the QML WebKit integration plugin.
now in English please :-) "Add the QML WebKit integration plugin to the Qt port"

WebKit/qt/ChangeLog:9
 +          QDeclarativeWebView is creating the item and expose
exposes ?

WebKit/qt/declarative/qdeclarativewebview.cpp:44
 +        : q(qq)
wrong indentation :-) indentation is a multiply of 4

WebKit/qt/declarative/qdeclarativewebview.cpp:47
 +        , progress(1.0)
is starts fully loaded?

WebKit/qt/declarative/qdeclarativewebview.cpp:52
 +        , rendering(true)
isRendering

WebKit/qt/declarative/qdeclarativewebview.cpp:58
 +      QUrl url; // page url might be different if it has not loaded yet
might be? Page URL is different if the page hasn't been loaded yet.

comments start with capital and ends with a punctuation mark.

WebKit/qt/declarative/qdeclarativewebview.cpp:61
 +      int preferredwidth, preferredheight;
preferred_W_idth

WebKit/qt/declarative/qdeclarativewebview.cpp:88
 +      , pressTime(400)
pressTimeMillis

WebKit/qt/declarative/qdeclarativewebview.cpp:223
 +      QWebPage* wp = new QDeclarativeWebPage(this);
wp? maybe just use page

WebKit/qt/declarative/qdeclarativewebview.cpp:279
 +  void QDeclarativeWebView::doLoadProgress(int p)
progress. Btw, how can you "do a load progress"? is this like a onLoadProgressChange ?

WebKit/qt/declarative/qdeclarativewebview.cpp:503
 +  void QDeclarativeWebView::setRenderingEnabled(bool enabled)
any reason this is called rendering and not painting?

WebKit/qt/declarative/qdeclarativewebview.cpp:627
 +      return page()->mainFrame()->icon().pixmap(QSize(256, 256));
won't this look terrible? Most icons are quite small and never uses at this size

WebKit/qt/declarative/qdeclarativewebview.cpp:678
 +      \qmlproperty bool WebView::settings.developerExtrasEnabled
do you support the web inspector with qml???

WebKit/qt/declarative/qdeclarativewebview.cpp:729
 +  
why a newline here?

WebKit/qt/declarative/qdeclarativewebview.cpp:817
 +  QDeclarativeWebView* QDeclarativeWebView::createWindow(QWebPage::WebWindowType type)
can't this method be refactored a bit ?

WebKit/qt/declarative/qdeclarativewebview.cpp:939
 +          maxWidth = INT_MAX;
use stl constants

WebKit/qt/declarative/qdeclarativewebview_p.h:160
 +                const QByteArray &body = QByteArray());
keep on one line.

WebKit/qt/declarative/qdeclarativewebview_p.h:169
 +      QDeclarativeWebSettings *settingsObject() const;
* placement

WebKit/qt/declarative/qdeclarativewebview_p.h:179
 +      void setNewWindowComponent(QDeclarativeComponent *newWindow);
here as well

WebKit/qt/declarative/qdeclarativewebview_p.h:178
 +      QDeclarativeComponent *newWindowComponent() const;
here

WebKit/qt/declarative/qdeclarativewebview_p.h:232
 +                                   const QRectF &oldGeometry);
keep on one line

WebKit/qt/declarative/qdeclarativewebview_p.h:260
 +      void setWindowObjectName(const QString &n)
n? baad name :-) also wrong placement of  &

[Alexis Menard (darktears)]

Created attachment 58994 [details]
Version 5

[WebKit Review Bot]

Attachment 58994 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/qt/declarative/qdeclarativewebview.cpp:21:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/qt/declarative/plugin.cpp:20:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

Committed r61325: <http://trac.webkit.org/changeset/61325>

[Simon Hausmann]

Revision r61325 cherry-picked into qtwebkit-2.0 with commit c6f08f4c13f88491a5d1ae1794c72166af0c26ba