[webkit-reviews] review denied: [Bug 28806] [Qt] Make the WebInspector available as a widget class. : [Attachment 39174] Changes v0.3

[bugzilla-daemon at webkit.org]

Ariya Hidayat <ariya.hidayat at trolltech.com> has denied Jocelyn Turcotte
<jocelyn.turcotte at nokia.com>'s request for review:
Bug 28806: [Qt] Make the WebInspector available as a widget class.
https://bugs.webkit.org/show_bug.cgi?id=28806

Attachment 39174: Changes v0.3
https://bugs.webkit.org/attachment.cgi?id=39174&action=review

------- Additional Comments from Ariya Hidayat <ariya.hidayat at trolltech.com>
> +	   [Qt] Make the WebInspector available as a widget class.

Shall we mention it simple "as QWidget" to avoid confusion?

> +    void setAttachedWindow(bool);


> diff --git a/WebKit/qt/Api/qwebelement.h b/WebKit/qt/Api/qwebelement.h
> index bc6f8a9..0be8c5a 100644
> --- a/WebKit/qt/Api/qwebelement.h
> +++ b/WebKit/qt/Api/qwebelement.h
> @@ -142,6 +142,7 @@ private:
>      explicit QWebElement(WebCore::Node*);
>  
>      friend class QWebFrame;
> +    friend class QWebPage;
>      friend class QWebHitTestResult;
>      friend class QWebHitTestResultPrivate;

Do we need to add the new friend in this patch? Where is it necessary?

> +void QWebInspectorPrivate::setFrontend(QWidget* value)

"value" is a weird argument name.

> +++ b/WebKit/qt/Api/qwebinspector_p.h
> @@ -0,0 +1,48 @@
> +/*
> +    Copyright (C) 2008, 2009 Nokia Corporation and/or its subsidiary(-ies)
> +    Copyright (C) 2008 Holger Hans Peter Freyther

Just to confirm: although this is a new file, part of it was taken from another
code where Holger has his copyright?

> +	   * Api/qwebinspector.cpp: Added.
> +	   (QWebInspector::QWebInspector):
> +	   (QWebInspector::~QWebInspector):
> +	   (QWebInspector::setPage):
> +	   (QWebInspector::page):
> +	   (QWebInspector::dock):
> +	   (QWebInspector::undock):
> +	   (QWebInspector::isDocked):
> +	   (QWebInspector::sizeHint):
> +	   (QWebInspector::event):
> +	   (QWebInspector::resizeEvent):
> +	   (QWebInspector::showEvent):
> +	   (QWebInspector::hideEvent):
> +	   (QWebInspectorPrivate::setFrontend):
> +	   (QWebInspectorPrivate::setDocked):
> +	   (QWebInspectorPrivate::adjustFrontendSize):
> +	   * Api/qwebinspector.h: Added.
> +	   * Api/qwebinspector_p.h: Added.
> +	   (QWebInspectorPrivate::QWebInspectorPrivate):

Do we need to mention all the new functions? Probably fine with only listing
the new source files?

Final concern: don't we need a unit test (or even a manual test) for this?

r- until the issues are addressed.