[webkit-reviews] review denied: [Bug 33096] [Qt] DRT: Support evaluateInWebInspector(), setTimerProfilingEnabled() and display(). : [Attachment 47437] 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 47437: Updated Patch
https://bugs.webkit.org/attachment.cgi?id=47437&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>

> +void QWEBKIT_EXPORT qt_drt_webinspector_executeScript(QWebPage* page, long
callId, const QString &script)

Coding style nitpick :) (& placement)

>  void DumpRenderTree::open(const QUrl& aurl)
>  {
> +    static QUrl url(aurl);
> +
>      resetToConsistentStateBeforeTesting();
>  
> -    QUrl url = aurl;
> +    if (isWebInspectorTest(url))
> +	   layoutTestController()->closeWebInspector();
> +
> +    url = aurl;
> +
> +    if (isWebInspectorTest(url))
> +	   layoutTestController()->showWebInspector();
> +

Please don't use a global QUrl object to maintain whether the inspector should
be closed on the next run or not.

I suggest either a boolean (where the name indicates what's happening), or see
if I can tie it to some other property,
for example to whether DeveloperExtras are enabled or not. That is unless you'd
like to show it once the inspector tests being, keep it "visible" and then
close it once the last inspector test has finished.


It's a bit sad that we can't use the public API to properly hide/show the
inspector but have to resort to private drt hooks :(