Bug 71808

Summary:

Web Inspector: function remote objetct should provide access to function position in the script

Product:

WebKit

Reporter:

Yury Semikhatsky <yurys>

Component:

Web Inspector (Deprecated)

Assignee:

Yury Semikhatsky <yurys>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, apavlov, ap, bweinstein, dglazkov, ggaren, japhet, joepeck, keishi, loislo, peter.rybin, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

72117

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	pfeldman: review+
Patch	none
Patch	none
Patch	pfeldman: review+, webkit.review.bot: commit-queue-

Yury Semikhatsky

Reported 2011-11-08 06:26:17 PST

Web Inspector: function remote objetct should provide access to script id and the function position in the script.

Attachments
Patch (17.73 KB, patch) 2011-11-10 07:12 PST, Yury Semikhatsky	no flags	Details Formatted Diff Diff
Patch (17.72 KB, patch) 2011-11-10 07:16 PST, Yury Semikhatsky	no flags	Details Formatted Diff Diff
Patch (19.63 KB, patch) 2011-11-10 09:14 PST, Yury Semikhatsky	no flags	Details Formatted Diff Diff
Patch (19.64 KB, patch) 2011-11-11 00:38 PST, Yury Semikhatsky	no flags	Details Formatted Diff Diff
Patch (19.04 KB, patch) 2011-11-11 03:51 PST, Yury Semikhatsky	pfeldman: review+	Details Formatted Diff Diff
Patch (19.78 KB, patch) 2011-11-11 04:23 PST, Yury Semikhatsky	no flags	Details Formatted Diff Diff
Patch (19.63 KB, patch) 2011-11-11 04:39 PST, Yury Semikhatsky	no flags	Details Formatted Diff Diff
Patch (21.41 KB, patch) 2011-11-11 05:54 PST, Yury Semikhatsky	pfeldman: review+ webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Yury Semikhatsky

Comment 1 2011-11-10 07:12:00 PST

Created attachment 114495 [details] Patch

WebKit Review Bot

Comment 2 2011-11-10 07:14:20 PST

Attachment 114495 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/insp..." exit_code: 1 Source/WebCore/inspector/InjectedScript.cpp:91: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InspectorDebuggerAgent.cpp:357: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InspectorDebuggerAgent.h:87: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InjectedScript.h:81: The parameter name "errorString" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/inspector/InjectedScript.h:81: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Total errors found: 5 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.

Yury Semikhatsky

Comment 3 2011-11-10 07:14:58 PST

(In reply to comment #1) > Created an attachment (id=114495) [details] > Patch Note that this patch depends on this V8 change which has not been pushed to trunk yet: http://code.google.com/p/v8/source/detail?r=9935

Yury Semikhatsky

Comment 4 2011-11-10 07:16:02 PST

Created attachment 114496 [details] Patch

WebKit Review Bot

Comment 5 2011-11-10 07:19:50 PST

Attachment 114496 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/insp..." exit_code: 1 Source/WebCore/inspector/InjectedScript.cpp:91: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InspectorDebuggerAgent.cpp:357: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InspectorDebuggerAgent.h:87: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InjectedScript.h:81: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Total errors found: 4 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.

WebKit Review Bot

Comment 6 2011-11-10 07:28:31 PST

Comment on attachment 114496 [details] Patch Attachment 114496 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10400334

Pavel Feldman

Comment 7 2011-11-10 08:30:28 PST

Comment on attachment 114496 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=114496&action=review r- for failing JSC bots. > LayoutTests/inspector/debugger/function-location-expected.txt:9 > +columnNumber: -1 You should not provide columnNumber in case it is not known.

Yury Semikhatsky

Comment 8 2011-11-10 09:14:36 PST

Created attachment 114518 [details] Patch

WebKit Review Bot

Comment 9 2011-11-10 09:16:59 PST

Attachment 114518 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/insp..." exit_code: 1 Source/WebCore/inspector/InjectedScript.cpp:91: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InspectorDebuggerAgent.cpp:357: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InspectorDebuggerAgent.h:87: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InjectedScript.h:81: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Total errors found: 4 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Yury Semikhatsky

Comment 10 2011-11-11 00:38:12 PST

Created attachment 114640 [details] Patch

WebKit Review Bot

Comment 11 2011-11-11 00:40:04 PST

Attachment 114640 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/insp..." exit_code: 1 Source/WebCore/inspector/InjectedScript.cpp:91: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InspectorDebuggerAgent.cpp:357: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InspectorDebuggerAgent.h:87: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InjectedScript.h:81: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Total errors found: 4 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Yury Semikhatsky

Comment 12 2011-11-11 03:51:47 PST

Created attachment 114664 [details] Patch

WebKit Review Bot

Comment 13 2011-11-11 03:54:58 PST

Attachment 114664 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/insp..." exit_code: 1 Source/WebCore/inspector/InjectedScript.cpp:91: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InspectorDebuggerAgent.cpp:357: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InspectorDebuggerAgent.h:87: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InjectedScript.h:81: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Total errors found: 4 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Yury Semikhatsky

Comment 14 2011-11-11 04:23:18 PST

Created attachment 114667 [details] Patch

WebKit Review Bot

Comment 15 2011-11-11 04:26:00 PST

Attachment 114667 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/insp..." exit_code: 1 Source/WebCore/inspector/InjectedScript.cpp:91: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InspectorDebuggerAgent.cpp:357: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InspectorDebuggerAgent.h:87: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InjectedScript.h:81: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Total errors found: 4 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

Pavel Feldman

Comment 16 2011-11-11 04:36:21 PST

Comment on attachment 114664 [details] Patch Inspector part looks good, please check with JSC folks whether exposing SourceCode on JSFunction is Ok.

Yury Semikhatsky

Comment 17 2011-11-11 04:39:35 PST

Created attachment 114671 [details] Patch

WebKit Review Bot

Comment 18 2011-11-11 04:41:20 PST

Attachment 114671 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/insp..." exit_code: 1 Source/WebCore/inspector/InjectedScript.cpp:91: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InspectorDebuggerAgent.cpp:357: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InspectorDebuggerAgent.h:87: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InjectedScript.h:81: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Total errors found: 4 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

Yury Semikhatsky

Comment 19 2011-11-11 05:06:29 PST

Committed r99950: <http://trac.webkit.org/changeset/99950>

Yury Semikhatsky

Comment 20 2011-11-11 05:16:32 PST

Landed this patch instead of another one by mistake which is why the bug was closed, reopening.

Yury Semikhatsky

Comment 21 2011-11-11 05:54:21 PST

Created attachment 114686 [details] Patch

WebKit Review Bot

Comment 22 2011-11-11 05:56:49 PST

Attachment 114686 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/insp..." exit_code: 1 Source/WebCore/inspector/InjectedScript.cpp:91: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InspectorDebuggerAgent.cpp:357: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InspectorDebuggerAgent.h:87: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebCore/inspector/InjectedScript.h:81: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Total errors found: 4 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.

Yury Semikhatsky

Comment 23 2011-11-11 07:27:05 PST

Alexei or Geoffrey could you look at the JSC-related changes, in particular 1. If you are fine with adding JSFunction::sourceCode getter which is used to avoid direct access to runtime/Executable.h that is not visible from WebCore. 2. In JSInjectedScriptHost::functionLocation new JSObject is created and kept as a raw pointer. I've seen such code in other bindings but I'm not sure if I have to create a handle to the object to protect it from being GC'ed or can assume that GC won't run while I'm in that method.

Geoffrey Garen

Comment 24 2011-11-11 08:54:27 PST

> 1. If you are fine with adding JSFunction::sourceCode getter which is used to avoid direct access to runtime/Executable.h that is not visible from WebCore. Seems fine. > 2. In JSInjectedScriptHost::functionLocation new JSObject is created and kept as a raw pointer. I've seen such code in other bindings but I'm not sure if I have to create a handle to the object to protect it from being GC'ed or can assume that GC won't run while I'm in that method. This code is also fine. For future reference, the reason this is OK is not that GC won't run. (You're allocating a string, so GC might run.) Rather, it's OK because the collector scans the C++ stack, and you have a reference to your new object on the C++ stack.

Yury Semikhatsky

Comment 25 2011-11-11 09:08:15 PST

(In reply to comment #24) > > 2. In JSInjectedScriptHost::functionLocation new JSObject is created and kept as a raw pointer. I've seen such code in other bindings but I'm not sure if I have to create a handle to the object to protect it from being GC'ed or can assume that GC won't run while I'm in that method. > > This code is also fine. For future reference, the reason this is OK is not that GC won't run. (You're allocating a string, so GC might run.) Rather, it's OK because the collector scans the C++ stack, and you have a reference to your new object on the C++ stack. That is what I suspected, thanks for the explanation!

WebKit Review Bot

Comment 26 2011-11-11 18:46:49 PST

Comment on attachment 114686 [details] Patch Attachment 114686 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10448611

Yury Semikhatsky

Comment 27 2011-11-14 01:27:51 PST

Committed r100113: <http://trac.webkit.org/changeset/100113>

Note You need to log in before you can comment on or make changes to this bug.