Bug 35568 - Web Inspector: display list of active workers & support debugging with fake workers
Summary: Web Inspector: display list of active workers & support debugging with fake w...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-02 02:21 PST by Andrey Kosyakov
Modified: 2010-03-15 16:01 PDT (History)
8 users (show)

See Also:


Attachments
Display list of active workers in scripts panel, allow workers debugging by injecting fake workers implementation (89.96 KB, patch)
2010-03-02 02:44 PST, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
Display list of active workers in scripts panel, allow workers debugging by injecting fake workers implementation (style fixed) (89.92 KB, patch)
2010-03-02 03:04 PST, Andrey Kosyakov
pfeldman: review-
Details | Formatted Diff | Diff
Display list of active workers in scripts panel, allow workers debugging by injecting fake workers implementation (nits picked, test added) (95.17 KB, patch)
2010-03-02 11:59 PST, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
Screenshot of workers sidebar pane (25.16 KB, image/png)
2010-03-02 12:05 PST, Andrey Kosyakov
no flags Details
49829: Display list of active workers in scripts panel, allow workers debugging by injecting fake workers implementation (96.61 KB, patch)
2010-03-03 07:41 PST, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
Display list of active workers in scripts panel, allow workers debugging by injecting fake workers implementation (fixed importScript sourceURL for Safari compatibility) (96.89 KB, patch)
2010-03-03 10:27 PST, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
Display list of active workers in scripts panel, allow workers debugging by injecting fake workers implementation (rebased to ToT) (93.52 KB, patch)
2010-03-04 03:03 PST, Andrey Kosyakov
eric: review-
Details | Formatted Diff | Diff
Display list of active workers in scripts panel, allow workers debugging by injecting fake workers implementation (fixed memory footprint problem on page cycler) (100.06 KB, patch)
2010-03-09 10:23 PST, Andrey Kosyakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Kosyakov 2010-03-02 02:21:11 PST
- 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.
Comment 1 Andrey Kosyakov 2010-03-02 02:44:34 PST
Created attachment 49792 [details]
Display list of active workers in scripts panel, allow workers debugging by injecting fake workers implementation
Comment 2 WebKit Review Bot 2010-03-02 02:48:06 PST
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.
Comment 3 Andrey Kosyakov 2010-03-02 03:04:18 PST
Created attachment 49795 [details]
Display list of active workers in scripts panel, allow workers debugging by injecting fake workers implementation (style fixed)
Comment 4 WebKit Review Bot 2010-03-02 03:06:43 PST
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.
Comment 5 Pavel Feldman 2010-03-02 04:49:53 PST
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 @@
> +2010-03-02  Andrey Kosyakov  <caseq@chromium.org>
> +
> +        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):
> +
> +2010-03-02  Andrey Kosyakov  <caseq@chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Need a short description and bug URL (OOPS!)
> +
> +        * src/js/InspectorControllerImpl.js:
> +        (devtools.InspectorBackendImpl):
> +
>  2010-03-01  Jakob Petsovits  <jpetsovits@rim.com>

You have 2 entries here.

It would it great to have a test for addScriptToEvaluateOnLoad.
Comment 6 Andrey Kosyakov 2010-03-02 11:59:28 PST
Created attachment 49829 [details]
Display list of active workers in scripts panel, allow workers debugging by injecting fake workers implementation (nits picked, test added)
Comment 7 WebKit Review Bot 2010-03-02 12:04:35 PST
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.
Comment 8 Andrey Kosyakov 2010-03-02 12:05:24 PST
Created attachment 49830 [details]
Screenshot of workers sidebar pane
Comment 9 Timothy Hatcher 2010-03-02 12:19:35 PST
Are the list of workers in the sidebar clickable to view the source in the Resources panel?
Comment 10 Pavel Feldman 2010-03-02 12:21:49 PST
(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.
Comment 11 Andrey Kosyakov 2010-03-03 07:41:33 PST
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
Comment 12 WebKit Review Bot 2010-03-03 07:45:35 PST
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.
Comment 13 Andrey Kosyakov 2010-03-03 10:27:57 PST
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)
Comment 14 Andrey Kosyakov 2010-03-04 03:03:41 PST
Created attachment 50003 [details]
Display list of active workers in scripts panel, allow workers debugging by injecting fake workers implementation (rebased to ToT)
Comment 15 WebKit Review Bot 2010-03-04 03:08:57 PST
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.
Comment 16 Pavel Feldman 2010-03-04 06:02:06 PST
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
Comment 17 Pavel Feldman 2010-03-05 08:16:19 PST
I had to roll this back since it seems to regress memory footprint according to chromium page cyclers.
Comment 18 WebKit Commit Bot 2010-03-05 14:02:35 PST
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.
Comment 19 WebKit Commit Bot 2010-03-05 14:02:39 PST
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.
Comment 20 WebKit Commit Bot 2010-03-05 14:02:43 PST
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.
Comment 21 Eric Seidel (no email) 2010-03-05 16:34:20 PST
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.
Comment 22 Andrey Kosyakov 2010-03-09 10:23:15 PST
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)
Comment 23 WebKit Review Bot 2010-03-09 10:29:49 PST
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.
Comment 24 Pavel Feldman 2010-03-09 11:12:00 PST
r=me
Comment 25 Pavel Feldman 2010-03-10 01:23:16 PST
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
Comment 26 Eric Seidel (no email) 2010-03-15 16:01:55 PDT
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.