[Qt] QWebInspector: improve QWebPage::webInspectorTriggered(QWebElement&) -> QWebInspector::show() connection

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Jocelyn Turcotte]

== Currently the way to connect an instantiated QWebInspector to a QWebPage context menu event is:
// ...
QWebPage *page = new QWebPage;
// ...

QWebInspector *inspector = new QWebInspector;
inspector->setPage(page);

connect(page, SIGNAL(webInspectorTriggered(const QWebElement&)), inspector, SLOT(show()));


== A way to lazily create the inspector on the first context menu event:
void UserClass::initialize() {
    // ...
    m_page = new QWebPage;
    // ...

    connect(page, SIGNAL(webInspectorTriggered(const QWebElement&)), userObject, SLOT(createAndShowInspector()));
}

void UserClass::createAndShowInspector() {
    if (!m_inspector) {
        m_inspector = new QWebInspector;
        m_inspector->setPage(m_page);
    }
    m_inspector->show();
}


== The problem is that the actual "set inspected element" is happening magically after the webInspectorTriggered signal has been emitted.
== So it might be cool to expose this behavior in the api.
== Two possible ways:
= 1. Add a setInspectedElement property setter
= Maybe we will add the getter in later versions, but this would require additional API in WebKit's web inspector.
= Also, what is actually the current inspectedElement is not totally obvious neither.
// ...
QWebPage *page = new QWebPage;
// ...

QWebInspector *inspector = new QWebInspector;
inspector->setPage(page);

connect(page, SIGNAL(webInspectorTriggered(const QWebElement&)), inspector, SLOT(show()));
connect(page, SIGNAL(webInspectorTriggered(const QWebElement&)), inspector, SLOT(setInspectedElement(const QWebElement&)));

= 2. Add a showElement(QWebElement&) slot
= Good thing: simple
= Bad thing: The showElement slot however has two responsibilities
= This might cause problem for a user that might call this method just to set the inspected element since show() will be called each time.
// ...
QWebPage *page = new QWebPage;
// ...

QWebInspector *inspector = new QWebInspector;
inspector->setPage(page);

connect(page, SIGNAL(webInspectorTriggered(const QWebElement&)), inspector, SLOT(showElement(const QWebElement&)));

[Tor Arne Vestbø]

After discussion, we're still not sure if this is the right API. Areas of concern:

 - Do we really need a way to provide lazy initialization? If not we don't need the webInspectorTriggered() signal. Also, without the signal you could always override the triggerAction

 - We want to keep the "magic"/default/99% case behavior of selecting the element you clicked on. The question is how to do it. Currently it's hard-coded, but if we add setInspectedElement() in the future as a slot we could add the webInspectorTriggered() signal and automatically connect it to setInspectedElement in setPage()

[Kenneth Rohde Christiansen]

webInspectorTriggered? wouldn't inspectorRequested be a better name?

No need for web either... we have methods like history() returning QWebHistory. page() returning QWebPage etc.

I also like requested more than triggered.

For setInspectedElement(), I would just go for inspectElement().

[Tor Arne Vestbø]

(In reply to comment #2)
> No need for web either... we have methods like history() returning QWebHistory.
> page() returning QWebPage etc.

I agree with you in general, but in this specific case the "Web Inspector" is a name on its own: http://webkit.org/blog/41/introducing-the-web-inspector/

> I also like requested more than triggered.

It's actually triggered, ie it's not a request you can ignore and then nothing will happen. I think that's why we went for triggered.

> For setInspectedElement(), I would just go for inspectElement().

That changes the semantics of the function. It's a property setter, not a function that does something (shows the view, highlights, etc). It would not match the getter inspectedElement() either.

[Jocelyn Turcotte]

(In reply to comment #3)
> (In reply to comment #2)
> > I also like requested more than triggered.
> 
> It's actually triggered, ie it's not a request you can ignore and then nothing
> will happen. I think that's why we went for triggered.
> 

I think that what makes us feel that its triggered is the inspection that happen "magically".
If QWebPage have no direct effect on the inspector beside through this signal's emission, I agree that it feels more like a request than a trigger.


Take for example if we have two signals/connections instead of one which connects to the QWebInspector:
QWPage::webInspectorShowRequested() ==> QWInspector::show()
QWPage::inspectElementRequested(QWE&) ==> QWInspector::setInspectedElement(QWE&)


Having one signal connected to both slots might also make sense, still no magic:
QWebPage::webInspectorRequested(QWE&) ==> QWebInspector::show()
QWebPage::webInspectorRequested(QWE&) ==> QWebInspector::setInspectedElement(QWE&)

[Jocelyn Turcotte]

Created attachment 42119 [details]
Patch: Remove the signal

This patch remove the signal for this release since it is not perfect yet and we think that it's use is not mandatory for now.

The only scenario I could think of is if somebody wants to lazily create the QWebInspector object until the first trigger through the context menu. This scenario can still be achieved in the 4.6 release by subclassing QWebPage and override ::triggerAction for InspectElement actions.

IMPORTANT: ===> 
We can remove the signal from 4.6 only if we all agree that in the future the signal connections between QWebPage and QWebInspector will be done automatically in QWebInspector::setPage(QWPage*).
This would allow us to change the "magic" behavior of 4.6 by an "automatically connected signals" behavior in 4.7.
Right now in trunk, this connection has to be made by the developer, which is not convenient since being able to change it is useless power in 95% of the time.

== Explanation:
// In 4.6
QWebInspector *inspector = new QWebInspector;
inspector->setPage(page) // <-- In there, QWebPage keeps a pointer to the
                         // inspector and in trigger action, calls 
                         // insp->show() and sets its element

// In 4.7
QWebInspector *inspector = new QWebInspector;
inspector->setPage(page) // <-- In there, signals from QWebPage are
                         // connected to the show() and
                         // setInspectedElement(QWE&) slots

[Kenneth Rohde Christiansen]

> == Explanation:
> // In 4.6
> QWebInspector *inspector = new QWebInspector;
> inspector->setPage(page) // <-- In there, QWebPage keeps a pointer to the
>                          // inspector and in trigger action, calls 
>                          // insp->show() and sets its element
> 
> // In 4.7
> QWebInspector *inspector = new QWebInspector;
> inspector->setPage(page) // <-- In there, signals from QWebPage are
>                          // connected to the show() and
>                          // setInspectedElement(QWE&) slots

It is possible to use the same web inspector for more pages?

I don't like the set page so much, as it is more a "watch page" or so. Would it be possible to do it the other way around?

QWebPage::setInspectable(new QWebInspector) ?

[Jocelyn Turcotte]

(In reply to comment #6)
> It is possible to use the same web inspector for more pages?
> 
> I don't like the set page so much, as it is more a "watch page" or so. Would it
> be possible to do it the other way around?
> 
> QWebPage::setInspectable(new QWebInspector) ?

The relation is 1<->1, so if you assign the same inspector to a different page, the previous page is unassociated (explained to the user in the documentation).

A reason I like the "set" is because it suggests that the objects are bound together. We first called it setInspectedPage, but since the web inspector implicitly inspects pages, so we omitted the "inspected". Maybe we should re-add inspected or watched to the method just in case we want the QWebInspector to have different interractions with a QWebPage? (what would that be?)

The method is in the inspector class to make it feel similar to QWebView::setPage, they are both UI layer objects that interact with the web page which feels more like a "content" layer object.

[Benjamin Poulain]

(In reply to comment #6)
> It is possible to use the same web inspector for more pages?

That's a good point, but I think we can handle that later without braking the current behavior.

Later, we can add the signal webInspectorTriggered() and have setInspectedElement() to change the element. In that case, you will be able to use the same inspector for different pages.

We can also add webInspectorTriggered() without the argument in the future.

[Jocelyn Turcotte]

(In reply to comment #8)
> (In reply to comment #6)
> > It is possible to use the same web inspector for more pages?
> 
> That's a good point, but I think we can handle that later without braking the
> current behavior.
> 
> Later, we can add the signal webInspectorTriggered() and have
> setInspectedElement() to change the element. In that case, you will be able to
> use the same inspector for different pages.
> 
> We can also add webInspectorTriggered() without the argument in the future.

The backend don't support this scenario. Moreover, the UI is not designed to display information from more than one page at the same time. The javascript console would also cause problems.

Behind the scene, the web inspector is tightly bound to WebCore's page and is not an independent API.

What we could do is to keep states like the current displayed tab within the QWebInspector and re-set this value when we do a setPage on a different QWebPage.

[Simon Hausmann]

Committed r50656: <http://trac.webkit.org/changeset/50656>

[Kenneth Rohde Christiansen]

Still shouldn't it be setWebInspector then and not setInspectable?

> > 
> > QWebPage::setInspectable(new QWebInspector) ?