[webkit-reviews] review denied: [Bug 40050] [Qt] Upstream the WebKit QML integration plugin : [Attachment 58989] version 4

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Alexis Menard
<alexis.menard at nokia.com>'s request for review:
Bug 40050: [Qt] Upstream the WebKit QML integration plugin
https://bugs.webkit.org/show_bug.cgi?id=40050

Attachment 58989: version 4
https://bugs.webkit.org/attachment.cgi?id=58989&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
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  &