Bug 73094 - InspectorServer: Enable and connect the WebInspectorServer with WebKit2 pages.
Summary: InspectorServer: Enable and connect the WebInspectorServer with WebKit2 pages.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Jocelyn Turcotte
URL:
Keywords:
Depends on: 73092 73093 73852 73855
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-24 13:34 PST by Jocelyn Turcotte
Modified: 2012-04-03 06:35 PDT (History)
13 users (show)

See Also:


Attachments
Original patch (43.26 KB, patch)
2011-11-24 13:34 PST, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (35.51 KB, patch)
2011-11-24 13:37 PST, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (24.94 KB, patch)
2011-12-05 12:48 PST, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (16.84 KB, patch)
2011-12-08 11:04 PST, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (16.40 KB, patch)
2012-03-29 11:38 PDT, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (16.49 KB, patch)
2012-04-02 07:46 PDT, Jocelyn Turcotte
hausmann: review+
webkit-ews: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jocelyn Turcotte 2011-11-24 13:34:08 PST
Created attachment 116542 [details]
Original patch

Tracks the "New Part 5" patch of bug #51364.
Comment 1 Jocelyn Turcotte 2011-11-24 13:37:01 PST
Created attachment 116543 [details]
Patch

This patch depends on another patch exposing the DeveloperExtras preference that I'll upload once this looks good.
Comment 2 Jocelyn Turcotte 2011-11-24 13:44:03 PST
Sam, Timothy, could you have a look at WebInspectorProxy, WebPageProxy and WebInspector to tell me if the approach looks reasonable?
Comment 3 Jocelyn Turcotte 2011-12-05 12:48:23 PST
Created attachment 117925 [details]
Patch

Changes from last patch:
- The actual server implementation has been moved to bug #73855 and this patch now only contains the bindings.
Comment 4 Joseph Pecoraro 2011-12-08 10:42:14 PST
Comment on attachment 117925 [details]
Patch

This patch looks like it adds InspectorServer to the build and includes "WebInspectorServer.h" but I don't see those files anywhere. Are they in another patch or am I just missing them? The path looks okay from my perspective.
Comment 5 Jocelyn Turcotte 2011-12-08 11:00:02 PST
(In reply to comment #4)
> (From update of attachment 117925 [details])
> This patch looks like it adds InspectorServer to the build and includes "WebInspectorServer.h" but I don't see those files anywhere. Are they in another patch or am I just missing them? The path looks okay from my perspective.

The files are added in bug #73855 but adding the include path to other builds shouldn't be necessary if I guard the include with ENABLE(INSPECTOR_SERVER).
I'll upload an update.
Comment 6 Jocelyn Turcotte 2011-12-08 11:04:26 PST
Created attachment 118425 [details]
Patch

Guard the include instead of adding InspectorServer to platforms without ENABLE(INSPECTOR_SERVER)
Comment 7 Yael 2012-01-14 12:54:30 PST
Comment on attachment 118425 [details]
Patch

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1500
> +#endif

You shouldn't register the page unconditionally each time that preferences change. You will end up with the same page being registered multiple times.
Comment 8 Yael 2012-01-14 12:57:40 PST
Sorry, the context of my comment got messed up. This is about 
>@@ -1489,6 +1494,11 @@ void WebPageProxy::preferencesDidChange()
>     if (!isValid())
>         return;
> 
>+#if ENABLE(INSPECTOR_SERVER)
>+    if (m_pageGroup->preferences()->developerExtrasEnabled())
>+        inspector()->enableRemoteInspection();
>+#endif
Comment 9 Vsevolod Vlasov 2012-02-14 15:15:39 PST
Comment on attachment 118425 [details]
Patch

r- per comment #7
Comment 10 Jocelyn Turcotte 2012-03-29 11:38:12 PDT
Created attachment 134629 [details]
Patch

- Rebased
- Fixed the registration problem
Comment 11 Early Warning System Bot 2012-03-29 12:24:22 PDT
Comment on attachment 134629 [details]
Patch

Attachment 134629 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12202425
Comment 12 Kenneth Rohde Christiansen 2012-04-02 05:00:54 PDT
Comment on attachment 134629 [details]
Patch

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

> Source/WebKit2/ChangeLog:9
> +        Pages are registered and unregistered as they are created
> +        and destroyed if they have developer extras enabled. The server is

So they are destroyed if developer extras is enabled? :) I think this could be written to avoid confusion.

> Source/WebKit2/UIProcess/qt/QtWebContext.cpp:65
> +            LOG_ERROR("Non numeric port for the inspector server \"%s\". Examples of valid input: \"12345\" or \"192.168.2.14:12345\" (with the address of one of this host's interface).", qPrintable(portStr));

LOG_ERROR here

> Source/WebKit2/UIProcess/qt/QtWebContext.cpp:72
> +            qWarning("Inspector server started successfully. Try pointing a WebKit browser to %s", qPrintable(inspectorServerUrl));

Then qWarning here...

> Source/WebKit2/UIProcess/qt/QtWebContext.cpp:78
> +static void initWebKit()

Could we find a better name?
Comment 13 Jocelyn Turcotte 2012-04-02 07:46:48 PDT
Created attachment 135100 [details]
Patch

- Addressed noted issues.
- Changed the Nokia copyrights to 2012 for new files.
Comment 14 Early Warning System Bot 2012-04-02 11:31:28 PDT
Comment on attachment 135100 [details]
Patch

Attachment 135100 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12311617
Comment 15 Simon Hausmann 2012-04-03 03:16:12 PDT
Comment on attachment 135100 [details]
Patch

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

> Source/WebKit2/UIProcess/qt/QtWebContext.cpp:50
> +    QString inspectorEnv = QString::fromLatin1(qgetenv("QTWEBKIT_INSPECTOR_SERVER"));

I suggest to use fromUtf8.
Comment 16 Jocelyn Turcotte 2012-04-03 06:35:54 PDT
Committed r113028: <http://trac.webkit.org/changeset/113028>