DevTools: hoist debugger paused reason to top (issue 2389883003 by luoe@chromium.org)

lu...@chromium.org

unread,

Oct 7, 2016, 10:25:31 PM10/7/16

to lush...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

Reviewers: lushnikov
CL: https://codereview.chromium.org/2389883003/

Message:
ptal

Description:
DevTools: hoist debugger paused reason to top

This interface change moves the paused reason (e.g. paused on breakpoint, paused
on subtree modification, etc) message to the top of the sources panel. See bug
for screenshots.

BUG=572068

Affected files (+147, -100 lines):
M third_party/WebKit/LayoutTests/http/tests/inspector/debugger-test.js
M third_party/WebKit/LayoutTests/inspector/sources/debugger/debug-inlined-scripts.html
M third_party/WebKit/LayoutTests/inspector/sources/debugger/debug-inlined-scripts-expected.txt
M third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js
M third_party/WebKit/Source/devtools/front_end/sources/CallStackSidebarPane.js
M third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js
M third_party/WebKit/Source/devtools/front_end/sources/sourcesPanel.css

lush...@chromium.org

unread,

Oct 12, 2016, 5:26:59 PM10/12/16

to lu...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

https://codereview.chromium.org/2389883003/diff/40001/third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js
File
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js
(right):

https://codereview.chromium.org/2389883003/diff/40001/third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js#newcode145
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:145:
createBreakpointHitSubMessage: function(details)
you don't need two functions - let's keep them as one

https://codereview.chromium.org/2389883003/diff/40001/third_party/WebKit/Source/devtools/front_end/sources/CallStackSidebarPane.js
File
third_party/WebKit/Source/devtools/front_end/sources/CallStackSidebarPane.js
(right):

https://codereview.chromium.org/2389883003/diff/40001/third_party/WebKit/Source/devtools/front_end/sources/CallStackSidebarPane.js#newcode105
third_party/WebKit/Source/devtools/front_end/sources/CallStackSidebarPane.js:105:
this.element.appendChild(element);
this seems to be unrelated?

https://codereview.chromium.org/2389883003/diff/40001/third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js
File
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js
(right):

https://codereview.chromium.org/2389883003/diff/40001/third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js#newcode484
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:484:
if (details) {
if (details)
status.appendChild(this._buildPausedMessage(pausedDetails));

https://codereview.chromium.org/2389883003/diff/40001/third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js#newcode487
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:487:
if (details.reason === WebInspector.DebuggerModel.BreakReason.DOM) {
Let's create a widget for this thing!

It will have one method: render(pausedDetails)

We'll be able to isolate a tone of css!

https://codereview.chromium.org/2389883003/

lu...@chromium.org

unread,

Oct 12, 2016, 8:36:59 PM10/12/16

to lush...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

https://codereview.chromium.org/2389883003/diff/40001/third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js
File
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js
(right):

https://codereview.chromium.org/2389883003/diff/40001/third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js#newcode145
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:145:
createBreakpointHitSubMessage: function(details)

On 2016/10/12 21:26:58, lushnikov wrote:
> you don't need two functions - let's keep them as one

Done.

https://codereview.chromium.org/2389883003/diff/40001/third_party/WebKit/Source/devtools/front_end/sources/CallStackSidebarPane.js
File
third_party/WebKit/Source/devtools/front_end/sources/CallStackSidebarPane.js
(right):

https://codereview.chromium.org/2389883003/diff/40001/third_party/WebKit/Source/devtools/front_end/sources/CallStackSidebarPane.js#newcode105
third_party/WebKit/Source/devtools/front_end/sources/CallStackSidebarPane.js:105:
this.element.appendChild(element);

On 2016/10/12 21:26:58, lushnikov wrote:
> this seems to be unrelated?

Done.

https://codereview.chromium.org/2389883003/diff/40001/third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js
File
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js
(right):

https://codereview.chromium.org/2389883003/diff/40001/third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js#newcode484
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:484:
if (details) {

On 2016/10/12 21:26:59, lushnikov wrote:
> if (details)
> status.appendChild(this._buildPausedMessage(pausedDetails));

Done with a widget as this._debuggerPausedMessage.render(details);

https://codereview.chromium.org/2389883003/diff/40001/third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js#newcode487
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:487:
if (details.reason === WebInspector.DebuggerModel.BreakReason.DOM) {

On 2016/10/12 21:26:59, lushnikov wrote:
> Let's create a widget for this thing!
>
> It will have one method: render(pausedDetails)
>
> We'll be able to isolate a tone of css!

Done as DebuggerPausedMessage.js

https://codereview.chromium.org/2389883003/

dgo...@chromium.org

unread,

Oct 12, 2016, 11:39:13 PM10/12/16

to lu...@chromium.org, lush...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

https://codereview.chromium.org/2389883003/diff/60001/third_party/WebKit/Source/devtools/front_end/sources/DebuggerPausedMessage.js
File
third_party/WebKit/Source/devtools/front_end/sources/DebuggerPausedMessage.js
(right):

https://codereview.chromium.org/2389883003/diff/60001/third_party/WebKit/Source/devtools/front_end/sources/DebuggerPausedMessage.js#newcode11
third_party/WebKit/Source/devtools/front_end/sources/DebuggerPausedMessage.js:11:
WebInspector.VBox.call(this);
Please don't make something a widget if you just want to scope css. Use
WI.createShadowRootWithCoreStyles instead.

https://codereview.chromium.org/2389883003/

lu...@chromium.org

unread,

Oct 13, 2016, 8:04:38 PM10/13/16

to lush...@chromium.org, dgo...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

Right, there really isn't any Widget logic we need here. Implemented as custom
element with shadow instead in patch 5.

https://codereview.chromium.org/2389883003/

lush...@chromium.org

unread,

Oct 13, 2016, 8:16:53 PM10/13/16

to lu...@chromium.org, dgo...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

I proposed using the custom element - but now it turns out to be somewhat
controversial. Dmirty, what do you think?

https://codereview.chromium.org/2389883003/diff/80001/third_party/WebKit/Source/devtools/front_end/sources/DebuggerPausedMessage.js
File
third_party/WebKit/Source/devtools/front_end/sources/DebuggerPausedMessage.js
(right):

https://codereview.chromium.org/2389883003/diff/80001/third_party/WebKit/Source/devtools/front_end/sources/DebuggerPausedMessage.js#newcode35
third_party/WebKit/Source/devtools/front_end/sources/DebuggerPausedMessage.js:35:
render: function(details)
let's pass all the singletons we rely on here

https://codereview.chromium.org/2389883003/diff/80001/third_party/WebKit/Source/devtools/front_end/sources/DebuggerPausedMessage.js#newcode45
third_party/WebKit/Source/devtools/front_end/sources/DebuggerPausedMessage.js:45:
messageWrapper =
WebInspector.domBreakpointsSidebarPane.createBreakpointHitMessage(details);
let's use static method?

https://codereview.chromium.org/2389883003/diff/80001/third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js
File
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js
(right):

https://codereview.chromium.org/2389883003/diff/80001/third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js#newcode479
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:479:
var details = currentDebuggerModel &&
currentDebuggerModel.debuggerPausedDetails();
use "?"

https://codereview.chromium.org/2389883003/

lu...@chromium.org

unread,

Oct 17, 2016, 3:27:41 PM10/17/16

to lush...@chromium.org, dgo...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

In a chat with dgozman@, we considered making DebuggerPausedMessage.js similar
to Toolbar.js, a class that creates a shadow root, but isn't a custom element.
We really don't need to inherit from HTMLDivPrototype for this.

Additional changes in the last patches
- Made enum for WI.DOMBreakpointsSidebarPane.BreakpointTypes,
BreakpointTypeLabels, BreakpointTypeNouns

Please take another look.

https://codereview.chromium.org/2389883003/diff/80001/third_party/WebKit/Source/devtools/front_end/sources/DebuggerPausedMessage.js
File
third_party/WebKit/Source/devtools/front_end/sources/DebuggerPausedMessage.js
(right):

https://codereview.chromium.org/2389883003/diff/80001/third_party/WebKit/Source/devtools/front_end/sources/DebuggerPausedMessage.js#newcode35
third_party/WebKit/Source/devtools/front_end/sources/DebuggerPausedMessage.js:35:
render: function(details)

On 2016/10/14 00:16:53, lushnikov wrote:
> let's pass all the singletons we rely on here

Done.

https://codereview.chromium.org/2389883003/diff/80001/third_party/WebKit/Source/devtools/front_end/sources/DebuggerPausedMessage.js#newcode45
third_party/WebKit/Source/devtools/front_end/sources/DebuggerPausedMessage.js:45:
messageWrapper =
WebInspector.domBreakpointsSidebarPane.createBreakpointHitMessage(details);

On 2016/10/14 00:16:53, lushnikov wrote:
> let's use static method?

Done.

https://codereview.chromium.org/2389883003/diff/80001/third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js
File
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js
(right):

https://codereview.chromium.org/2389883003/diff/80001/third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js#newcode479
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:479:
var details = currentDebuggerModel &&
currentDebuggerModel.debuggerPausedDetails();

On 2016/10/14 00:16:53, lushnikov wrote:
> use "?"

Done.

https://codereview.chromium.org/2389883003/

lush...@chromium.org

unread,

Oct 18, 2016, 7:01:29 PM10/18/16

to lu...@chromium.org, dgo...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

lgtm

https://chromiumcodereview.appspot.com/2389883003/diff/120001/third_party/WebKit/Source/devtools/front_end/sources/debuggerPausedMessage.css
File
third_party/WebKit/Source/devtools/front_end/sources/debuggerPausedMessage.css
(right):

https://chromiumcodereview.appspot.com/2389883003/diff/120001/third_party/WebKit/Source/devtools/front_end/sources/debuggerPausedMessage.css#newcode39
third_party/WebKit/Source/devtools/front_end/sources/debuggerPausedMessage.css:39:
font-size: 11px;
with .monospace, you won't need font-size as well!

https://chromiumcodereview.appspot.com/2389883003/diff/120001/third_party/WebKit/Source/devtools/front_end/sources/debuggerPausedMessage.css#newcode40
third_party/WebKit/Source/devtools/front_end/sources/debuggerPausedMessage.css:40:
font-family: dejavu sans mono, monospace;
use .monospace for this!

https://chromiumcodereview.appspot.com/2389883003/

lu...@chromium.org

unread,

Oct 19, 2016, 5:44:13 PM10/19/16

to lush...@chromium.org, dgo...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

Monospace added for the sub-text message.

https://codereview.chromium.org/2389883003/diff/120001/third_party/WebKit/Source/devtools/front_end/sources/debuggerPausedMessage.css
File
third_party/WebKit/Source/devtools/front_end/sources/debuggerPausedMessage.css
(right):

https://codereview.chromium.org/2389883003/diff/120001/third_party/WebKit/Source/devtools/front_end/sources/debuggerPausedMessage.css#newcode39
third_party/WebKit/Source/devtools/front_end/sources/debuggerPausedMessage.css:39:
font-size: 11px;

On 2016/10/18 23:01:29, lushnikov wrote:
> with .monospace, you won't need font-size as well!

Done.

https://codereview.chromium.org/2389883003/diff/120001/third_party/WebKit/Source/devtools/front_end/sources/debuggerPausedMessage.css#newcode40

third_party/WebKit/Source/devtools/front_end/sources/debuggerPausedMessage.css:40:
font-family: dejavu sans mono, monospace;

On 2016/10/18 23:01:29, lushnikov wrote:
> use .monospace for this!

Done.

https://codereview.chromium.org/2389883003/

lush...@chromium.org

unread,

Oct 19, 2016, 8:39:52 PM10/19/16

to lu...@chromium.org, dgo...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

Test doesn't pass! :(
inspector/sources/debugger-frameworks/frameworks-dom-xhr-event-breakpoints.html

https://codereview.chromium.org/2389883003/

lu...@chromium.org

unread,

Oct 19, 2016, 9:05:31 PM10/19/16

to lush...@chromium.org, dgo...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

On 2016/10/20 00:39:51, lushnikov wrote:
> Test doesn't pass! :(
>
inspector/sources/debugger-frameworks/frameworks-dom-xhr-event-breakpoints.html

Strange...I just pulled, merged, and ran everything locally, and they all
passed. Going to do another dry run.

https://codereview.chromium.org/2389883003/

commit-bot@chromium.org via chromiumcodereview-hr.appspot.com

unread,

Oct 21, 2016, 10:19:03 PM10/21/16

to lu...@chromium.org, lush...@chromium.org, dgo...@chromium.org, commi...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2389883003/160001

https://codereview.chromium.org/2389883003/

commit-bot@chromium.org via chromiumcodereview-hr.appspot.com

unread,

Oct 21, 2016, 11:46:54 PM10/21/16

to lu...@chromium.org, lush...@chromium.org, dgo...@chromium.org, commi...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

Try jobs failed on following builders:
win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/317345)

https://codereview.chromium.org/2389883003/

commit-bot@chromium.org via chromiumcodereview-hr.appspot.com

unread,

Oct 24, 2016, 7:37:12 PM10/24/16

to lu...@chromium.org, lush...@chromium.org, dgo...@chromium.org, commi...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

commit-bot@chromium.org via chromiumcodereview-hr.appspot.com

unread,

Oct 24, 2016, 10:37:43 PM10/24/16

to lu...@chromium.org, lush...@chromium.org, dgo...@chromium.org, commi...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

Try jobs failed on following builders:
win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,

http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/318203)

https://codereview.chromium.org/2389883003/

lu...@chromium.org

unread,

Oct 25, 2016, 7:22:31 PM10/25/16

to lush...@chromium.org, dgo...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

Windows test was failing for multiple reasons:
- WI.UIString.capitalize() capitalizes strings on every platform except Windows,
intended to match the styles of native context menus
- Now that XHR Breakpoints show the url in the paused message, one of the XHR
tests was failing because Windows showed "file:///C:/foo" while other platforms
showed "file:///foo"

Patches 11 and 12 should fix these cases respectively.

https://codereview.chromium.org/2389883003/

commit-bot@chromium.org via chromiumcodereview-hr.appspot.com

unread,

Oct 25, 2016, 8:44:25 PM10/25/16

to lu...@chromium.org, lush...@chromium.org, dgo...@chromium.org, commi...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2389883003/220001

https://codereview.chromium.org/2389883003/

commit-bot@chromium.org via chromiumcodereview-hr.appspot.com

unread,

Oct 25, 2016, 8:50:57 PM10/25/16

to lu...@chromium.org, lush...@chromium.org, dgo...@chromium.org, commi...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

Committed patchset #12 (id:220001)

https://codereview.chromium.org/2389883003/

commit-bot@chromium.org via chromiumcodereview-hr.appspot.com

unread,

Oct 25, 2016, 8:53:03 PM10/25/16

to lu...@chromium.org, lush...@chromium.org, dgo...@chromium.org, commi...@chromium.org, chromium...@chromium.org, caseq...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, apavlo...@chromium.org, devtools...@chromium.org, blink-...@chromium.org, pfel...@chromium.org, kozyatins...@chromium.org

Patchset 12 (id:??) landed as
https://crrev.com/5fa5d3b7589a798770e0f5df18d14939cd16f8a9
Cr-Commit-Position: refs/heads/master@{#427553}

https://codereview.chromium.org/2389883003/

Reply all

Reply to author

Forward