[webkit-reviews] review denied: [Bug 64297] [Qt][WK2] Add the Web Inspector to WebKit2 : [Attachment 105798] Patch to add Web Inspector to WebKit2 updated.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 31 13:45:36 PDT 2011


Noam Rosenthal <noam.rosenthal at nokia.com> has denied Genisim
<genisim at yahoo.com>'s request for review:
Bug 64297: [Qt][WK2] Add the Web Inspector to WebKit2
https://bugs.webkit.org/show_bug.cgi?id=64297

Attachment 105798: Patch to add Web Inspector to WebKit2 updated.
https://bugs.webkit.org/attachment.cgi?id=105798&action=review

------- Additional Comments from Noam Rosenthal <noam.rosenthal at nokia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=105798&action=review


> Source/WebKit2/ChangeLog:13
> +	   Implemented Qt platform methods for Web Inspector
> +	   One will activate Web Inspector from qdesktopwebview
> +	   and qtouchwebview views. With current patch Web Inspector
> +	   for both cases using qdesktopwebview
> +	   "Enable Developer Extras" and "Toggle WebInspector" features
> +	   implemented in QtWebPageProxy common for both views

Grammar for this Changelog entry is totally unreadable.

> Source/WebKit2/UIProcess/WebInspectorProxy.h:57
> +#if PLATFORM(QT)
> +#include <qdesktopwebview.h>
> +#include <qsgview.h>
> +
> +class QDesktopWebView;
> +class QObject;
> +#endif

If you include those files declaring the classes is redundant.

> Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:45
> +    ASSERT(m_inspectorView);

Please remove this assert.

> Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:57
> +    ASSERT(m_view);

Please remove this assert.

> Tools/MiniBrowser/qt/BrowserView.cpp:104
> +void BrowserView::enableDeveloperExtrasMode(bool enable)
> +{
> +    if (desktopWebView())
> +	   desktopWebView()->setDeveloperExtrasEnabled(enable);
> +    else
> +	   touchWebView()->setDeveloperExtrasEnabled(enable);
> +}
> +
> +void BrowserView::toggleWebInspectorMode(bool enable)
> +{
> +    if (desktopWebView())
> +	   desktopWebView()->setWebInspectorEnabled(enable);
> +    else
> +	   touchWebView()->setWebInspectorEnabled(enable);
> +}
> +

Naming consistency:
setDeveloperExtrasEnabled
setWebInspectorModeEnabled

> Tools/MiniBrowser/qt/BrowserWindow.cpp:119
> +    toggleWebInspector->connect(this,
SIGNAL(enteredDeveloperExtrasMode(bool)), SLOT(setEnabled(bool)));
> +    toggleWebInspector->connect(this, SIGNAL(enteredWebInspectorMode(bool)),
SLOT(setChecked(bool)));
> +    m_enableDeveloperExtras->connect(this,
SIGNAL(enteredWebInspectorMode(bool)), this,
SLOT(changeDeveloperExtrasMode(bool)));
> +    connect(this, SIGNAL(enteredWebInspectorMode(bool)), this,
SLOT(toggleWebInspectorMode(bool)));

Instead of doing this with signals and slot, please call those functions
explicitly in onDeveloperExtrasModeChanged or onWebInspectorModeChanged. will
be much more readable, and you won't need changeDeveloperExtrasMode.


More information about the webkit-reviews mailing list