[webkit-reviews] review denied: [Bug 33096] [Qt] DRT: Support evaluateInWebInspector(), setTimerProfilingEnabled() and display(). : [Attachment 46743] Updated Patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Robert Hogan
<robert at roberthogan.net>'s request for review:
Bug 33096: [Qt] DRT: Support evaluateInWebInspector(),
setTimerProfilingEnabled() and display().
https://bugs.webkit.org/show_bug.cgi?id=33096

Attachment 46743: Updated Patch
https://bugs.webkit.org/attachment.cgi?id=46743&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
In general this looks like a really good patch! Excellent work with improving
our test coverage.



> +void QWEBKIT_EXPORT qt_drt_webinspector_executeScript(QWebPage *page, long
callId, const QString &script)
> +{
> +    if (!page->handle()->page->inspectorController())
> +	   return;
> +   
page->handle()->page->inspectorController()->evaluateForTestInFrontend(callId,
script);
> +}
> +
> +void QWEBKIT_EXPORT qt_drt_webinspector_close(QWebPage *page)
> +{
> +    if (!page->handle()->page->inspectorController())
> +	   return;
> +    page->handle()->page->inspectorController()->close();
> +}
> +
> +void QWEBKIT_EXPORT qt_drt_webinspector_show(QWebPage *page)
> +{
> +    if (!page->handle()->page->inspectorController())
> +	   return;
> +    page->handle()->page->inspectorController()->show();
> +}

This approach is fine with me, but the '*' position is incorrect (coding style
nitpick :)


> diff --git a/WebKit/qt/WebCoreSupport/InspectorClientQt.cpp
b/WebKit/qt/WebCoreSupport/InspectorClientQt.cpp
> index 1caf96d..6e3bd5c 100644
> --- a/WebKit/qt/WebCoreSupport/InspectorClientQt.cpp
> +++ b/WebKit/qt/WebCoreSupport/InspectorClientQt.cpp
> @@ -120,6 +120,9 @@ void InspectorClientQt::showWindow()
>  void InspectorClientQt::closeWindow()
>  {
>  #if ENABLE(INSPECTOR)
> +    // QWebInspector::frontEnd is a copy of inspectorView which will be left
dangling
> +    // by m_frontend.set(0) in InspectorController::close() if we do not
clear it here.
> +    m_inspectedWebPage->d->getOrCreateInspector()->d->setFrontend(0);
>      m_inspectedWebPage->d->inspectorController()->setWindowVisible(false);
>  #endif

Jocelyn, does that look okay to you? (you have a better idea of the inspector
workings than I do :)

>
> +static bool isWebInspectorTest(const QUrl& url)
> +{
> +    if (url.toString().contains("inspector/"))

Is there an easier way to do this? Maybe just url.path().contains() instead of
the expensive toString() conversion?

r- because of the style bit, and I'd like to hear Jocelyn's opinion on the
InspectorClientQt.cpp change.