[Qt] No way to programatically specify element to focus the inspector

• Component: WebKit Qt
• Status: RESOLVED INVALID

[Eugene Ostroukhov]

I am trying to embed QWebView into an IDE and provide some advanced integration. One of the tasks I have is to focus Web Inspector on an element that was passed from the outside. I was unable to find a way to do it - QWebPage::triggerAction(QWebPage::InspectElement) requires a click on necessary element. To me, issuing the mouse click on an already known element looks like an overkill.

I suggest adding a method QWebPage::inspect(QWebElement)

[Eugene Ostroukhov]

Created attachment 69855 [details]
Patch that adds inspect(QWebElement) method to QWebPage

Actually, on second thought, would it make sense to add this method to QWebElement directly?
It looks like QWebView also needs some element highlighting (i.e. for web inspector) - would it make sense to add similar "highlight" method to QWebPage, QWebFrame or QWebElement?

[Andreas Kling]

Comment on attachment 69855 [details]
Patch that adds inspect(QWebElement) method to QWebPage

View in context: https://bugs.webkit.org/attachment.cgi?id=69855&action=review

> WebKit/qt/Api/qwebpage.cpp:2278
>              if (!d->hitTestResult.isNull()) {
> -                d->getOrCreateInspector(); // Make sure the inspector is created
> -                d->inspector->show(); // The inspector is expected to be shown on inspection
> -                d->page->inspectorController()->inspect(d->hitTestResult.d->innerNonSharedNode.get());
> +                inspect(d->hitTestResult.element());
>              }

Coding style, the braces should be removed since only one statement remains inside the if-block.

> WebKit/qt/Api/qwebpage.cpp:2303
> +    This function can be called to focus Web Inspector on specified element. Web
> +    Inspector will become visible if it was not. Document element may be passed.

English, would read better as:
This function focuses the inspector on the specified \a element.
The inspector becomes visible if it wasn't already.

> WebKit/qt/Api/qwebpage.cpp:2305
> +void QWebPage::inspect(const QWebElement& element) {

Coding style, { should be by itself on the next line.

> WebKit/qt/Api/qwebpage.cpp:2309
> +	d->getOrCreateInspector(); // Make sure the inspector is created
> +	d->inspector->show(); // The inspector is expected to be shown on inspection
> +	d->page->inspectorController()->inspect(element.m_element);

Coding style, indents should be 4 spaces.

> WebKit/qt/Api/qwebpage.h:351
> +	void inspect(const QWebElement& element);

Coding style, indents should be 4 spaces.

[Eugene Ostroukhov]

Created attachment 70202 [details]
Updated patch according to suggestions in comment #2

[Alexis Menard (darktears)]

Comment on attachment 70202 [details]
Updated patch according to suggestions in comment #2

View in context: https://bugs.webkit.org/attachment.cgi?id=70202&action=review

I don't have any problem with the feature. The patch looks ok to me (except the extra spaces). But should we ifdef out completely the API if we don't build webkit with the inspector, not just the implementation. It's better than to let a method which will do anything and just add an extra symbol. No?

> WebKit/qt/Api/qwebpage.h:350
> +	

Why those spaces?

[Simon Hausmann]

(In reply to comment #4)
> (From update of attachment 70202 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70202&action=review
> 
> I don't have any problem with the feature. The patch looks ok to me (except the extra spaces). But should we ifdef out completely the API if we don't build webkit with the inspector, not just the implementation. It's better than to let a method which will do anything and just add an extra symbol. No?

Given that we don't have a preprocessor define that is visible in the public
API to determine if the inspector is available or not, I don't think we have
a choice here :)

I'm personally okay with this approach.

[Andreas Kling]

Comment on attachment 70202 [details]
Updated patch according to suggestions in comment #2

View in context: https://bugs.webkit.org/attachment.cgi?id=70202&action=review

> WebKit/qt/Api/qwebpage.cpp:2136
> -                d->getOrCreateInspector(); // Make sure the inspector is created
> -                d->inspector->show(); // The inspector is expected to be shown on inspection
> -                d->page->inspectorController()->inspect(d->hitTestResult.d->innerNonSharedNode.get());
> -            }
> +            if (!d->hitTestResult.isNull())
> +                inspect(d->hitTestResult.element());

This breaks inspecting a text node from the right-click menu, since text nodes cannot be represented by QWebElement.

> WebKit/qt/Api/qwebpage.cpp:2162
> +/*!
> +    This function focuses the inspector on the specified \a element.
> +    The inspector becomes visible if it wasn't already.
> + */

Focuses is not exactly the right word. It should also mention that it may open the inspector if it's not already open.
Also needs a \since 4.8 doc tag.

> WebKit/qt/Api/qwebpage.cpp:2166
> +    d->getOrCreateInspector(); // Make sure the inspector is created

No need for the comment.

> WebKit/qt/Api/qwebpage.h:351
> +    void inspect(const QWebElement& element);

This API is not good enough. We also want to be able to inspect() text nodes.
There is currently no QtWebKit class that wraps a WebCore::Node, so we'll need that first.
There was a discussion on webkit-qt a while ago about this.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.