[webkit-reviews] review denied: [Bug 88902] Web Inspector: add a way to get the remote inspector url for a given page. : [Attachment 147138] Patch

[bugzilla-daemon at webkit.org]

Jocelyn Turcotte <jocelyn.turcotte at nokia.com> has denied Alexis Menard
(darktears) <alexis.menard at openbossa.org>'s request for review:
Bug 88902: Web Inspector: add a way to get the remote inspector url for a given
page.
https://bugs.webkit.org/show_bug.cgi?id=88902

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

------- Additional Comments from Jocelyn Turcotte <jocelyn.turcotte at nokia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=147138&action=review


> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1302
> +    return
QUrl::fromUserInput(WebInspectorServer::shared().inspectorUrlForPageID(d_ptr->w
ebPageProxy->inspector()->remoteInspectionPageId()));

It would be cleaner to build a proper URL in
WebInspectorServer::inspectorUrlForPageID and use QUrl::QUrl(QString&) instead.


> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:274
> +    Q_PROPERTY(QUrl remoteInspectorUrl READ remoteInspectorUrl() CONSTANT
FINAL)

In some case it might be useful to have it non-constant. e.g. if the
developerExtraEnabled preference is set to true/false or if the inspector
server is closed. If we can't change the signature of the property later we
should have a notify signal already.

> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.cpp:98
> +String WebInspectorServer::inspectorUrlForPageID(int pageId)

pageId is unused.

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServer.cpp:67
> +    m_bindAddress = bindAddress;
> +    m_port = port;

This should be set only if (isNowListening).