[webkit-reviews] review denied: [Bug 40050] [Qt] Upstream the WebKit QML integration plugin : [Attachment 57770] Version 3

[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 57770: Version 3
https://bugs.webkit.org/attachment.cgi?id=57770&action=review

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