Bug 164679 - Web Inspector: SourceCodeTextEditor should display execution lines for background threads
Summary: Web Inspector: SourceCodeTextEditor should display execution lines for backgr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-11-11 23:57 PST by Joseph Pecoraro
Modified: 2016-11-15 22:03 PST (History)
7 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (24.38 KB, patch)
2016-11-14 16:27 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[IMAGE] Thread Line Indicator - Multiple Threads in the same Resource (642.33 KB, image/gif)
2016-11-14 16:28 PST, Joseph Pecoraro
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (deleted)
2016-11-14 17:49 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2016-11-11 23:57:02 PST
Summary:
SourceCodeTextEditor should display execution lines for background threads

Currently we only show the execution line for the active call frame. However, users may be looking at a file which contains the current paused execution line for a background thread, or even have two Workers paused in the same "worker.js" script. Whenever targets are paused, we should be showing their execution line.

Notes:
- Xcode has a nice inline message, like inline error messages, indicating the name of the other thread paused at that location.
Comment 1 Radar WebKit Bug Importer 2016-11-11 23:59:40 PST
<rdar://problem/29233026>
Comment 2 Matt Baker 2016-11-12 14:10:24 PST
I like including an inline message similar to our inline errors/warnings for execution lines. Would execution lines belonging to background threads be styled differently than the line associated with the active call frame? It might be nice to use opacity (or some other style) to de-emphasize execution lines for inactive call frames.

What if two threads are paused at the same location in the same script? Maybe we could indicate something in the inline message, like having a disclosure triangle.
Comment 3 Timothy Hatcher 2016-11-13 11:46:21 PST
(In reply to comment #2)
> I like including an inline message similar to our inline errors/warnings for
> execution lines. Would execution lines belonging to background threads be
> styled differently than the line associated with the active call frame? It
> might be nice to use opacity (or some other style) to de-emphasize execution
> lines for inactive call frames.
> 
> What if two threads are paused at the same location in the same script?
> Maybe we could indicate something in the inline message, like having a
> disclosure triangle.

Currently threads are limited to whole worker scripts, so there is never a mix. In the future we might want to consider things that you mention.
Comment 4 Joseph Pecoraro 2016-11-14 11:45:36 PST
> > What if two threads are paused at the same location in the same script?
> > Maybe we could indicate something in the inline message, like having a
> > disclosure triangle.
> 
> Currently threads are limited to whole worker scripts, so there is never a
> mix. In the future we might want to consider things that you mention.

This is possible right now. If two workers are spawned with the same script ("worker.js") then we should show both in the SourceCodeTextEditor for that script.
Comment 5 Joseph Pecoraro 2016-11-14 16:27:26 PST
Created attachment 294774 [details]
[PATCH] Proposed Fix

Probably won't apply
Comment 6 Joseph Pecoraro 2016-11-14 16:28:03 PST
Created attachment 294775 [details]
[IMAGE] Thread Line Indicator - Multiple Threads in the same Resource
Comment 7 Build Bot 2016-11-14 17:49:03 PST
Comment on attachment 294774 [details]
[PATCH] Proposed Fix

Attachment 294774 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2516778

New failing tests:
js/regress-141098.html
Comment 8 Build Bot 2016-11-14 17:49:06 PST
Created attachment 294788 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 9 Joseph Pecoraro 2016-11-14 20:05:24 PST
Comment on attachment 294788 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

Failure appears unrelated.
Comment 10 Timothy Hatcher 2016-11-15 21:39:25 PST
Comment on attachment 294774 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:714
> +        }
> +        threads.push(target);

Nit: newline between.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:730
> +        threads.remove(target);
> +        if (threads.length) {

Nit: newline between.
Comment 11 WebKit Commit Bot 2016-11-15 22:03:50 PST
Comment on attachment 294774 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 294774

Committed r208783: <http://trac.webkit.org/changeset/208783>
Comment 12 WebKit Commit Bot 2016-11-15 22:03:54 PST
All reviewed patches have been landed.  Closing bug.