Bug 28806 – [Qt] Make the WebInspector available as a widget class.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Jocelyn Turcotte]

Allows users of QtWebKit to control the location of the WebInspector and to dock it.

Notes about this modification in InspectorController:
 
 void InspectorController::setWindowVisible(bool visible, bool attached)
 {
-    if (visible == m_windowVisible)
+    if (visible == m_windowVisible || !m_frontend)
         return;
 
     m_windowVisible = visible;
 
-    if (!m_frontend)
-        return;
-
     if (m_windowVisible) {
         setAttachedWindow(attached);
         populateScriptObjects();


It prevented correct initialization of the web inspector when calling setWindowVisible two times rapidly on first calls.
After verification, all code path with the state (m_windowsVisible && !m_frontend) would lead to a crash or not accomplishing any action.

[Jocelyn Turcotte]

Created attachment 38850 [details]
Changes

[Jocelyn Turcotte]

Created attachment 39143 [details]
Changes without InspectorController.cpp

Moved the modification of WebCore/inspector/InspectorController.cpp in a different bug (29006)

This patch now affect WebCore only by making InspectorController::setAttachedWindow(...) public

[Jocelyn Turcotte]

Created attachment 39174 [details]
Changes v0.3

- Updated patch format
- Added missing consts

[Ariya Hidayat]

Comment on attachment 39174 [details]
Changes v0.3

> +        [Qt] Make the WebInspector available as a widget class.

Shall we mention it simple "as QWidget" to avoid confusion?

> +    void setAttachedWindow(bool);


> diff --git a/WebKit/qt/Api/qwebelement.h b/WebKit/qt/Api/qwebelement.h
> index bc6f8a9..0be8c5a 100644
> --- a/WebKit/qt/Api/qwebelement.h
> +++ b/WebKit/qt/Api/qwebelement.h
> @@ -142,6 +142,7 @@ private:
>      explicit QWebElement(WebCore::Node*);
>  
>      friend class QWebFrame;
> +    friend class QWebPage;
>      friend class QWebHitTestResult;
>      friend class QWebHitTestResultPrivate;

Do we need to add the new friend in this patch? Where is it necessary?

> +void QWebInspectorPrivate::setFrontend(QWidget* value)

"value" is a weird argument name.

> +++ b/WebKit/qt/Api/qwebinspector_p.h
> @@ -0,0 +1,48 @@
> +/*
> +    Copyright (C) 2008, 2009 Nokia Corporation and/or its subsidiary(-ies)
> +    Copyright (C) 2008 Holger Hans Peter Freyther

Just to confirm: although this is a new file, part of it was taken from another code where Holger has his copyright?

> +        * Api/qwebinspector.cpp: Added.
> +        (QWebInspector::QWebInspector):
> +        (QWebInspector::~QWebInspector):
> +        (QWebInspector::setPage):
> +        (QWebInspector::page):
> +        (QWebInspector::dock):
> +        (QWebInspector::undock):
> +        (QWebInspector::isDocked):
> +        (QWebInspector::sizeHint):
> +        (QWebInspector::event):
> +        (QWebInspector::resizeEvent):
> +        (QWebInspector::showEvent):
> +        (QWebInspector::hideEvent):
> +        (QWebInspectorPrivate::setFrontend):
> +        (QWebInspectorPrivate::setDocked):
> +        (QWebInspectorPrivate::adjustFrontendSize):
> +        * Api/qwebinspector.h: Added.
> +        * Api/qwebinspector_p.h: Added.
> +        (QWebInspectorPrivate::QWebInspectorPrivate):

Do we need to mention all the new functions? Probably fine with only listing the new source files?

Final concern: don't we need a unit test (or even a manual test) for this?

r- until the issues are addressed.

[Jocelyn Turcotte]

Created attachment 39254 [details]
Changes v0.4

(In reply to comment #4)

> Shall we mention it simple "as QWidget" to avoid confusion?
Yes, guess so

> 
> Do we need to add the new friend in this patch? Where is it necessary?
No, not in this patch, removed it

> 
> "value" is a weird argument name.
Changed them

> 
> Just to confirm: although this is a new file, part of it was taken from another
> code where Holger has his copyright?
Ah, copy/paste error. Except on this copyright line I don't think he needs copyright.

> 
> Do we need to mention all the new functions? Probably fine with only listing
> the new source files?
Removed

> 
> Final concern: don't we need a unit test (or even a manual test) for this?
I agree, the class is at least used in QtLauncher, but if you think of any other way to test this, please let me know.

[Simon Hausmann]

Comment on attachment 39254 [details]
Changes v0.4

Comments from the API discussion in the office here:

> +    void dock();
> +    void undock();
> +    bool isDocked() const;
[...]
> +    void dockRequested();
> +    void undockRequested();

These functions/signals should disappear from the
public API, as the names don't feel right and we'd like
find a better solution in the future. For now it seems
safer to not expose this aspect of the UI.

> +    QWebInspector* inspector() const;

This function should not be part of the public API, too. It doesn't
add any real convenience, as the developer always has a pointer to the
inspector anyway, as owner.

> +    void inspectElementTriggered();

This signal should be renamed to

    void QWebPage::webInspectorTriggered(const QWebElement&);

[Jocelyn Turcotte]

Created attachment 39347 [details]
Changes v0.5

Stuff that may be worth noting about this patch relatively to v0.4:

The inspectedElement argument to the webInspectorTriggered signal is currently, as I saw, always the same as the focused element in the inspector.
However this is mostly luck since we can't create QWebElements out of non-html nodes and the inspector also seems to focus on the enclosing Html element.

For example if we right click on a text node in the page, the hittestresult will contain a WebCore::Text node.
- This patch will walk the parents until a QWebElement can be created from the hittestresult's parents before giving it to the signal.
- The web inspector will select the surrounding <font> element, however I would not expect this behavior since the text node is available for selection and it may change in the future.

I documented in the signal that the inspectedElement argument is not garanteed to be the same.
There is also the possibility of not giving this argument in the signal, since the user may expect the same value and it may feel unreliable.

Please tell me what you think.

[Simon Hausmann]

(In reply to comment #7)
> Created an attachment (id=39347) [details]
> Changes v0.5
> 
> Stuff that may be worth noting about this patch relatively to v0.4:
> 
> The inspectedElement argument to the webInspectorTriggered signal is currently,
> as I saw, always the same as the focused element in the inspector.
> However this is mostly luck since we can't create QWebElements out of non-html
> nodes and the inspector also seems to focus on the enclosing Html element.
> 
> For example if we right click on a text node in the page, the hittestresult
> will contain a WebCore::Text node.
> - This patch will walk the parents until a QWebElement can be created from the
> hittestresult's parents before giving it to the signal.
> - The web inspector will select the surrounding <font> element, however I would
> not expect this behavior since the text node is available for selection and it
> may change in the future.
> 
> I documented in the signal that the inspectedElement argument is not garanteed
> to be the same.
> There is also the possibility of not giving this argument in the signal, since
> the user may expect the same value and it may feel unreliable.
> 
> Please tell me what you think.

The patch looks good to me. I'd like to r+ it, but after trying the patch I just noticed that "inspect element" from the context menu in the QtLauncher doesn't seem to work for me. It comes up in the splitter, but it doesn't show the inspected element or anything else.

Can you reproduce that?

[Simon Hausmann]

(In reply to comment #8)
[...]
> The patch looks good to me. I'd like to r+ it, but after trying the patch I
> just noticed that "inspect element" from the context menu in the QtLauncher
> doesn't seem to work for me. It comes up in the splitter, but it doesn't show
> the inspected element or anything else.
> 
> Can you reproduce that?

Turns out that this was an unrelated problem with an outdated qrc file.

[Simon Hausmann]

Comment on attachment 39347 [details]
Changes v0.5

r=me, great stuff

[Simon Hausmann]

Landed in r48288