Bug 153455 - Web Inspector: Include SamplingProfiler's expression-level data for stack frames in the protocol
Summary: Web Inspector: Include SamplingProfiler's expression-level data for stack fra...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on: 152629
Blocks:
  Show dependency treegraph
 
Reported: 2016-01-25 15:21 PST by Saam Barati
Modified: 2016-02-05 15:25 PST (History)
13 users (show)

See Also:


Attachments
patch (11.85 KB, patch)
2016-02-04 14:27 PST, Saam Barati
joepeck: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-yosemite (839.56 KB, application/zip)
2016-02-04 15:22 PST, Build Bot
no flags Details
patch for landing. (11.76 KB, patch)
2016-02-05 13:59 PST, Saam Barati
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (901.33 KB, application/zip)
2016-02-05 14:28 PST, Build Bot
no flags Details
patch for landing. (11.74 KB, patch)
2016-02-05 14:42 PST, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2016-01-25 15:21:52 PST
....
Comment 1 Radar WebKit Bug Importer 2016-01-25 15:22:10 PST
<rdar://problem/24335884>
Comment 2 Saam Barati 2016-02-04 14:27:00 PST
Created attachment 270696 [details]
patch
Comment 3 Joseph Pecoraro 2016-02-04 14:52:45 PST
Comment on attachment 270696 [details]
patch

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

r=me, but address renames

> Source/JavaScriptCore/inspector/protocol/ScriptProfiler.json:20
> +            "id": "Location",

Location is getting to be a heavily overloaded term. I think "ExpressionLocation" would be better.

> Source/JavaScriptCore/inspector/protocol/ScriptProfiler.json:24
> +                { "name": "lineNumber", "type": "integer" },
> +                { "name": "columnNumber", "type": "integer" }

Perhaps we should shorten these to "line" and "column" like ScriptProfiler.StackFrame (below). Whatever we choose should be consistent. I think short is fine, especially for objects like this which will show many many times.

> LayoutTests/inspector/sampling-profiler/expression-location-info.html:49
> +                        let lineNumber = parseInt(lineColumnHashedString.split(":")[0]);
> +                        let columnNumber = parseInt(lineColumnHashedString.split(":")[1]);

Avoid doing the split twice. Up to you if you want to do the map or just pull them out individually.

    let [lineNumber, columnNumber] = lineColumnHashedString.split(":").map(Number);
Comment 4 Build Bot 2016-02-04 15:22:34 PST
Comment on attachment 270696 [details]
patch

Attachment 270696 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/783383

New failing tests:
inspector/sampling-profiler/call-frame-with-dom-functions.html
Comment 5 Build Bot 2016-02-04 15:22:38 PST
Created attachment 270698 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 6 Saam Barati 2016-02-05 13:59:04 PST
Created attachment 270767 [details]
patch for landing.

going to let EWS run tests
Comment 7 Build Bot 2016-02-05 14:28:03 PST
Comment on attachment 270767 [details]
patch for landing.

Attachment 270767 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/787840

New failing tests:
inspector/sampling-profiler/expression-location-info.html
Comment 8 Build Bot 2016-02-05 14:28:06 PST
Created attachment 270772 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 9 Saam Barati 2016-02-05 14:42:48 PST
Created attachment 270773 [details]
patch for landing.

lets give EWS another try.
Comment 10 Saam Barati 2016-02-05 15:25:43 PST
landed in:
http://trac.webkit.org/changeset/196193