Bug 35568 – Web Inspector: display list of active workers & support debugging with fake workers

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Andrey Kosyakov]

- Display list of active workers in a sidebar on the scripts panel.
- Provide an option to inject a "fake" workers implementation that allows debugging workers.

[Andrey Kosyakov]

Created attachment 49792 [details]
Display list of active workers in scripts panel, allow workers debugging by injecting fake workers implementation

[WebKit Review Bot]

Attachment 49792 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/inspector/InspectorController.cpp:1189:  Missing spaces around !=  [whitespace/operators] [3]
WebCore/workers/Worker.cpp:48:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/dom/Document.h:453:  Use 0 instead of NULL.  [readability/null] [4]
WebCore/inspector/InspectorWorkerResource.h:39:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/inspector/InspectorWorkerResource.h:42:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/workers/SharedWorker.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 6 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andrey Kosyakov]

Created attachment 49795 [details]
Display list of active workers in scripts panel, allow workers debugging by injecting fake workers implementation (style fixed)

[WebKit Review Bot]

Attachment 49795 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/dom/Document.h:453:  Use 0 instead of NULL.  [readability/null] [4]
WebCore/workers/SharedWorker.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

Comment on attachment 49795 [details]
Display list of active workers in scripts panel, allow workers debugging by injecting fake workers implementation (style fixed)

Overall looks good, a bunch of nits below.

> + * Copyright (C) 2009, 2010 Google Inc. All rights reserved.

Make it simply 2010.

>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions are
> @@ -213,6 +213,12 @@ JSValue JSInjectedScriptHost::reportDidD
>      return jsUndefined();
>  }
>  
> +pair<long, ScriptObject> InjectedScriptHost::injectScript(const String& source, ScriptState *scriptState)
> +{
> +    long id = m_nextInjectedScriptId++;
> +    return std::make_pair(id, createInjectedScript(source, this, scriptState, id));
> +}
> +

This should not be custom (make createInjectedScript static member if necessary).

> +#if ENABLE(WORKERS)
> +void InjectedScriptHost::didCreateWorker(long id, const String &url, bool isSharedWorker)

const String& url

> +#if ENABLE(WORKERS)
> +    void didCreateWorker(long id, const String &url, bool isSharedWorker);

ditto

>  
> +    long generateId();

generateId is a bad name since it will conflict with injected script id. nextWorkerId.


> +void InspectorController::clearWorkers()
> +{
> +    if (m_frontend) {
> +        WorkersMap::iterator workersEnd = m_workers.end();
> +        for (WorkersMap::iterator it = m_workers.begin(); it != workersEnd; ++it)
> +            m_frontend->willDestroyWorker(*it->second);
> +    }
> +    m_workers.clear();
> +}

I don't think you need this method - panel's reset will do tear down on the front-end side.

> +    template<typename T> void innerDidCreateWorker(HashMap<T, RefPtr<InspectorWorkerResource> >* map, T id, const String& url, bool isSharedWorker);
> +    template<typename T> void innerWillDestroyWorker(HashMap<T, RefPtr<InspectorWorkerResource> >* map, T id);

I think you wanted to get rid of this.
> +    WebInspector.panels.scripts.sidebarPanes.workers.addWorker.apply(WebInspector.panels.scripts.sidebarPanes.workers, arguments);
> +}
> +
> +WebInspector.willDestroyWorker = function()
> +{
> +    WebInspector.panels.scripts.sidebarPanes.workers.removeWorker.apply(WebInspector.panels.scripts.sidebarPanes.workers, arguments);

You should cache WebInspector.panels.scripts.sidebarPanes.workers.

> +
> +.sidebar-pane-subtitle input[type=checkbox] {

We are already slow. Could you use explicit class for this instead?


>          EventTargetData m_eventTargetData;
> +        long m_id;
> +
> +        static long s_lastWorkerId;

Either m_workerId or s_lastId

> @@ -1,3 +1,23 @@
> [phone]  Andrey Kosyakov  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Added addScriptToEvaluateOnLoad() / removeAllScriptsToEvaluateOnLoad()
> +        (part of fake workers injection support)
> +        https://bugs.webkit.org/show_bug.cgi?id=35568
> +
> +        * src/js/InspectorControllerImpl.js:
> +        (devtools.InspectorBackendImpl):
> +
> [phone]  Andrey Kosyakov  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Need a short description and bug URL (OOPS!)
> +
> +        * src/js/InspectorControllerImpl.js:
> +        (devtools.InspectorBackendImpl):
> +
>  2010-03-01  Jakob Petsovits  <[e-mail]>

You have 2 entries here.

It would it great to have a test for addScriptToEvaluateOnLoad.

[Andrey Kosyakov]

Created attachment 49829 [details]
Display list of active workers in scripts panel, allow workers debugging by injecting fake workers implementation (nits picked, test added)

[WebKit Review Bot]

Attachment 49829 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/dom/Document.h:453:  Use 0 instead of NULL.  [readability/null] [4]
Total errors found: 1 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andrey Kosyakov]

Created attachment 49830 [details]
Screenshot of workers sidebar pane

[Timothy Hatcher]

Are the list of workers in the sidebar clickable to view the source in the Resources panel?

[Pavel Feldman]

(In reply to comment #9)
> Are the list of workers in the sidebar clickable to view the source in the
> Resources panel?

Yes. Links will probably work only in case instrumentation is on and resources are loaded using XHRs. The change that would provide inspector resources for worker scripts is to follow.

[Andrey Kosyakov]

Created attachment 49907 [details]
49829: Display list of active workers in scripts panel, allow workers debugging by injecting fake workers implementation

Fixed relative URL resolution bug in fake worker's implementation of importScript()
Fixed display of worker list upon front-end attach in Safari

[WebKit Review Bot]

Attachment 49907 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/dom/Document.h:453:  Use 0 instead of NULL.  [readability/null] [4]
Total errors found: 1 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andrey Kosyakov]

Created attachment 49920 [details]
 Display list of active workers in scripts panel, allow workers debugging by injecting fake workers implementation (fixed importScript sourceURL for Safari compatibility)

[Andrey Kosyakov]

Created attachment 50003 [details]
Display list of active workers in scripts panel, allow workers debugging by injecting fake workers implementation (rebased to ToT)

[WebKit Review Bot]

Attachment 50003 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/dom/Document.h:453:  Use 0 instead of NULL.  [readability/null] [4]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	C	WebCore/workers/SharedWorker.cpp => WebCore/inspector/InspectorWorkerResource.h
	C	WebCore/inspector/front-end/Script.js => WebCore/inspector/front-end/Checkbox.js
	M	WebCore/ChangeLog
	M	WebCore/English.lproj/localizedStrings.js
	M	WebCore/WebCore.gypi
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/bindings/js/JSInjectedScriptHostCustom.cpp
	M	WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp
	M	WebCore/dom/Document.cpp
	M	WebCore/dom/Document.h
	M	WebCore/dom/ScriptExecutionContext.h
	M	WebCore/inspector/InjectedScriptHost.cpp
	M	WebCore/inspector/InjectedScriptHost.h
	M	WebCore/inspector/InjectedScriptHost.idl
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorController.h
	M	WebCore/inspector/InspectorFrontend.cpp
	M	WebCore/inspector/InspectorFrontend.h
	M	WebCore/inspector/front-end/InjectedFakeWorker.js
	M	WebCore/inspector/front-end/Script.js
	M	WebCore/inspector/front-end/ScriptsPanel.js
	M	WebCore/inspector/front-end/WebKit.qrc
	A	WebCore/inspector/front-end/WorkersSidebarPane.js
	M	WebCore/inspector/front-end/inspector.css
	M	WebCore/inspector/front-end/inspector.html
	M	WebCore/workers/AbstractWorker.cpp
	M	WebCore/workers/AbstractWorker.h
	M	WebCore/workers/SharedWorker.cpp
	M	WebCore/workers/Worker.cpp
	M	WebKit/chromium/src/js/InspectorControllerImpl.js
Committed r55522

[Pavel Feldman]

I had to roll this back since it seems to regress memory footprint according to chromium page cyclers.

[WebKit Commit Bot]

Comment on attachment 49829 [details]
Display list of active workers in scripts panel, allow workers debugging by injecting fake workers implementation (nits picked, test added)

Cleared Pavel Feldman's review+ from obsolete attachment 49829 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[WebKit Commit Bot]

Comment on attachment 49907 [details]
49829: Display list of active workers in scripts panel, allow workers debugging by injecting fake workers implementation

Cleared Pavel Feldman's review+ from obsolete attachment 49907 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[WebKit Commit Bot]

Comment on attachment 49920 [details]
 Display list of active workers in scripts panel, allow workers debugging by injecting fake workers implementation (fixed importScript sourceURL for Safari compatibility)

Cleared Pavel Feldman's review+ from obsolete attachment 49920 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 50003 [details]
Display list of active workers in scripts panel, allow workers debugging by injecting fake workers implementation (rebased to ToT)

Marking r- since this was rolled out.

[Andrey Kosyakov]

Created attachment 50324 [details]
Display list of active workers in scripts panel, allow workers debugging by injecting fake workers implementation (fixed memory footprint problem on page cycler)

[WebKit Review Bot]

Attachment 50324 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/dom/Document.h:453:  Use 0 instead of NULL.  [readability/null] [4]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

r=me

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	C	WebCore/workers/SharedWorker.cpp => WebCore/inspector/InspectorWorkerResource.h
	M	WebCore/ChangeLog
	M	WebCore/English.lproj/localizedStrings.js
	M	WebCore/WebCore.gypi
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/bindings/js/JSInjectedScriptHostCustom.cpp
	M	WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp
	M	WebCore/dom/Document.cpp
	M	WebCore/dom/Document.h
	M	WebCore/dom/ScriptExecutionContext.h
	M	WebCore/inspector/InjectedScriptHost.cpp
	M	WebCore/inspector/InjectedScriptHost.h
	M	WebCore/inspector/InjectedScriptHost.idl
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorController.h
	M	WebCore/inspector/InspectorFrontend.cpp
	M	WebCore/inspector/InspectorFrontend.h
	A	WebCore/inspector/front-end/Checkbox.js
	M	WebCore/inspector/front-end/InjectedFakeWorker.js
	M	WebCore/inspector/front-end/ScriptsPanel.js
	M	WebCore/inspector/front-end/StylesSidebarPane.js
	M	WebCore/inspector/front-end/WebKit.qrc
	A	WebCore/inspector/front-end/WorkersSidebarPane.js
	M	WebCore/inspector/front-end/inspector.css
	M	WebCore/inspector/front-end/inspector.html
	M	WebCore/workers/AbstractWorker.cpp
	M	WebCore/workers/AbstractWorker.h
	M	WebCore/workers/SharedWorker.cpp
	M	WebCore/workers/Worker.cpp
	M	WebKit/chromium/ChangeLog
	M	WebKit/chromium/src/js/InspectorControllerImpl.js
Committed r55771

[Eric Seidel (no email)]

Comment on attachment 50324 [details]
Display list of active workers in scripts panel, allow workers debugging by injecting fake workers implementation (fixed memory footprint problem on page cycler) 

Clearing r? since this bug is closed.