Paul Irish (Gerrit)

unread,

Jul 3, 2019, 6:40:04 PM7/3/19

to apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Joel Einbinder, Tricium, chromium...@chromium.org, devtools...@chromium.org, Pavel Feldman

Ready for an initial look.

View Change

To view, visit change 1687089. To unsubscribe, or for help writing mail filters, visit settings.

Joel Einbinder (Gerrit)

unread,

Jul 8, 2019, 6:34:46 PM7/8/19

to Paul Irish, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Tricium, chromium...@chromium.org, devtools...@chromium.org, Pavel Feldman

View Change

8 comments:

File third_party/blink/renderer/devtools/front_end/bindings/SASSSourceMapping.js:
- Patch Set #3, Line 86: this._sourceMapAttachedForTest(sourceMap);
  nit: this should be last.
- Patch Set #3, Line 88: const resource = header.cssModel().target().model(SDK.ResourceTreeModel).resourceForURL(header.contentURL());
  In theory, multiple distinct resources can have the same URL. We should at least try to go to the correct frame.
  There's also a race. If we enable the network agent after the css agent, we'd get the stylesheet before the resource. This would fail to find the resource, and we'd fail to get the source map.
File third_party/blink/renderer/devtools/front_end/network/NetworkItemView.js:
- Patch Set #3, Line 89: Network.NetworkItemView.Tabs.SourceMap, Common.UIString('Source Map'), this._sourceMapView,
  nit: prefer ls over Common.UISString.
File third_party/blink/renderer/devtools/front_end/network/requestSourceMapView.css:
- Patch Set #3, Line 6: .request-sourcemap-view {
  nit: extra space
- Patch Set #3, Line 28: /* Calculations assume 1px border. */
  /* keep in sync with FILE */
File third_party/blink/renderer/devtools/front_end/text_utils/TextUtils.js:
- Patch Set #3, Line 113: * Find common string prefix
  drop comment
- Patch Set #3, Line 117: commonPrefix(strings) {
  should we handle strings.length === 0?
File third_party/blink/renderer/devtools/front_end/webtreemap/webtreemap.js:
- Patch Set #3, Line 20: // var kBorderWidth = 1;
  It looks like we modified this file. Should we just adopt it?

To view, visit change 1687089. To unsubscribe, or for help writing mail filters, visit settings.

Jeff Fisher (Gerrit)

unread,

Jul 11, 2019, 3:20:21 PM7/11/19

to Paul Irish, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Kayce Basques, Joel Einbinder, Tricium, chromium...@chromium.org, devtools...@chromium.org, Pavel Feldman

[+Kayce for new functionality]

Seems like a useful visualizer. I would love to be able to use it on our own bundles, though I suppose we'd have to use a standard bundler for that. :)

View Change

1 comment:

Commit Message:
- Patch Set #3, Line 7: DevTools: [Network] Add sourcemap treemap visualization
  details about this feature and a screenshot would be appreciated.

To view, visit change 1687089. To unsubscribe, or for help writing mail filters, visit settings.

Connor Clark (Gerrit)

unread,

Jul 14, 2019, 11:57:45 PM7/14/19

to Paul Irish, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Kayce Basques, Jeff Fisher, Joel Einbinder, Tricium, chromium...@chromium.org, devtools...@chromium.org, Pavel Feldman

View Change

1 comment:

File third_party/blink/renderer/devtools/front_end/sdk/SourceMap.js:
- Patch Set #3, Line 591: const sourceURL = entry ? entry.sourceURL : null;
  there could be a way to advance the column based on the next mapping - the assumption being that mapping are ordered by generated line/col. if this is true, you could do `column = nextMapping.generatedColumn ; line = nextMapping.generatedLine`
  it _feels like_ all these lookups would be slow, but maybe not. also would be hesitant to attempt this w/o some easy to reason about tests.

To view, visit change 1687089. To unsubscribe, or for help writing mail filters, visit settings.

Paul Irish (Gerrit)

unread,

Jul 15, 2019, 5:47:19 PM7/15/19

to apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Connor Clark, Kayce Basques, Jeff Fisher, Joel Einbinder, Tricium, chromium...@chromium.org, devtools...@chromium.org, Pavel Feldman

View Change

2 comments:

File third_party/blink/renderer/devtools/front_end/bindings/CompilerScriptMapping.js:

Patch Set #3, Line 222:

Hard to repro, but I sometimes get an exception here. Is this due to the race you mentioned?

CompilerScriptMapping.js:222 Uncaught (in promise) TypeError: Cannot read property 'resourceForURL' of null
    at Bindings.CompilerScriptMapping._sourceMapAttached (CompilerScriptMapping.js:222)
    at SDK.SourceMapManager.dispatchEventToListeners (Object.js:115)
    at SDK.SourceMapManager.attach (SourceMapManager.js:177)
    at SDK.SourceMapManager.onSourceMap (SourceMapManager.js:166)

File third_party/blink/renderer/devtools/front_end/bindings/SASSSourceMapping.js:

Patch Set #3, Line 90: resource

Also I did manage to get an error for this on occasion. Very hard to repro.

InspectorFrontendHost.js:515 TypeError: Cannot read property 'Symbol(Bindings.NetworkProject._frameAttributionSymbol)' of null TypeError: Cannot read property 'Symbol(Bindings.NetworkProject._frameAttributionSymbol)' of null
    at Function.removeFrameAttribution (NetworkProject.js:108)
    at Bindings.SASSSourceMapping._sourceMapDetached (SASSSourceMapping.js:103)
    at SDK.SourceMapManager.dispatchEventToListeners (Object.js:115)
    at SDK.SourceMapManager.detachSourceMap (SourceMapManager.js:202)
    at SDK.CSSModel._styleSheetRemoved (CSSModel.js:517)
    at SDK.CSSDispatcher.styleSheetRemoved (CSSModel.js:758)
    ...

To view, visit change 1687089. To unsubscribe, or for help writing mail filters, visit settings.

Joel Einbinder (Gerrit)

unread,

Jul 16, 2019, 10:50:45 AM7/16/19

to Paul Irish, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Connor Clark, Kayce Basques, Jeff Fisher, Tricium, chromium...@chromium.org, devtools...@chromium.org, Pavel Feldman

View Change

2 comments:

File third_party/blink/renderer/devtools/front_end/bindings/CompilerScriptMapping.js:

- Patch Set #3, Line 222:

- Hard to repro, but I sometimes get an exception here. Is this due to the race you mentioned? […]
  looks like DebuggerModel before ResourceModel. Different race, same concept.
File third_party/blink/renderer/devtools/front_end/bindings/SASSSourceMapping.js:

- Patch Set #3, Line 90: resource

- Also I did manage to get an error for this on occasion. Very hard to repro. […]
  This one I don't know.

To view, visit change 1687089. To unsubscribe, or for help writing mail filters, visit settings.

Paul Irish (Gerrit)

unread,

Aug 14, 2019, 8:47:32 PM8/14/19

to devtools-re...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Joel Einbinder, Connor Clark, Kayce Basques, Jeff Fisher, Tricium, chromium...@chromium.org, devtools...@chromium.org, Pavel Feldman

View Change

1 comment:

Commit Message:

- Patch Set #3, Line 7: DevTools: [Network] Add sourcemap treemap visualization
  details about this feature and a screenshot would be appreciated.

- Wrote it up and added some screencasts to the bug:
  https://bugs.chromium.org/p/chromium/issues/detail?id=981148

To view, visit change 1687089. To unsubscribe, or for help writing mail filters, visit settings.

Paul Irish (Gerrit)

unread,

Aug 15, 2019, 8:20:17 PM8/15/19

to devtools-re...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Joel Einbinder, Connor Clark, Kayce Basques, Jeff Fisher, Tricium, chromium...@chromium.org, devtools...@chromium.org, Pavel Feldman

Please TAL

View Change

10 comments:

File third_party/blink/renderer/devtools/front_end/bindings/SASSSourceMapping.js:
- Patch Set #3, Line 86: this._sourceMapAttachedForTest(sourceMap);

- nit: this should be last.

- Ack

- Patch Set #3, Line 88: const resource = header.cssModel().target().model(SDK.ResourceTreeModel).resourceForURL(header.contentURL());

- In theory, multiple distinct resources can have the same URL. […]
  Thanks. Dropped this approach and am using a singleton to collect all sourceMap state instead.

- Patch Set #3, Line 90: resource

- This one I don't know.
  approach dropped.
File third_party/blink/renderer/devtools/front_end/network/NetworkItemView.js:

- Patch Set #3, Line 89: Network.NetworkItemView.Tabs.SourceMap, Common.UIString('Source Map'), this._sourceMapView,
  nit: prefer ls over Common.UISString.

- Patch Set #3, Line 6: .request-sourcemap-view {
  nit: extra space

- Done

- Patch Set #3, Line 28: /* Calculations assume 1px border. */
  /* keep in sync with FILE */

- Patch Set #3, Line 591: const sourceURL = entry ? entry.sourceURL : null;

- there could be a way to advance the column based on the next mapping - the assumption being that map […]
  I really like this idea. I tried it out and it'll require adding some new methods to TextUtils/TextCursors... I think we can do it in a followup.
  In the meantime, the perf of this current approach is acceptable.
File third_party/blink/renderer/devtools/front_end/text_utils/TextUtils.js:

- Patch Set #3, Line 113: * Find common string prefix
  drop comment

- Done

- Patch Set #3, Line 117: commonPrefix(strings) {

- should we handle strings. […]
  Done
File third_party/blink/renderer/devtools/front_end/webtreemap/webtreemap.js:

- Patch Set #3, Line 20: // var kBorderWidth = 1;

- It looks like we modified this file. […]
  we can do that. what's involved.. removing it from skip_compilation?

To view, visit change 1687089. To unsubscribe, or for help writing mail filters, visit settings.

Paul Irish (Gerrit)

unread,

Aug 16, 2019, 9:11:00 PM8/16/19

to devtools-re...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Joel Einbinder, Connor Clark, Kayce Basques, Jeff Fisher, Tricium, chromium...@chromium.org, devtools...@chromium.org, Pavel Feldman

feedback from joel:

View Change

2 comments:

File third_party/blink/renderer/devtools/front_end/sdk/SourceMapManager.js:

Patch Set #6, Line 227: SDK
sourcemapmanagerobserver

Patch Set #6, Line 267: sourceMapObserver

follow this pattern instead

SDK.cssMetadata = function() {
  if (!SDK.CSSMetadata._instance)
    SDK.CSSMetadata._instance = new SDK.CSSMetadata(SDK.CSSMetadata._generatedProperties || []);
  return SDK.CSSMetadata._instance;
};

To view, visit change 1687089. To unsubscribe, or for help writing mail filters, visit settings.

Paul Irish (Gerrit)

unread,

Aug 16, 2019, 9:32:23 PM8/16/19

to devtools-re...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Joel Einbinder, Connor Clark, Kayce Basques, Jeff Fisher, Tricium, chromium...@chromium.org, devtools...@chromium.org, Pavel Feldman

webtreemap updated. feedback addressed. ptal

View Change

2 comments:

File third_party/blink/renderer/devtools/front_end/sdk/SourceMapManager.js:

- Patch Set #6, Line 227: SDK
  sourcemapmanagerobserver

- Done

- Patch Set #6, Line 267: sourceMapObserver

- follow this pattern instead […]
  Done

To view, visit change 1687089. To unsubscribe, or for help writing mail filters, visit settings.

Jeff Fisher (Gerrit)

unread,

Aug 20, 2019, 5:03:47 PM8/20/19

to Paul Irish, devtools-re...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Commit Bot, Joel Einbinder, Connor Clark, Kayce Basques, Tricium, chromium...@chromium.org, devtools...@chromium.org, Pavel Feldman

I love this change a lot! It's so nice to see bundle contents visualized without having to use any third party tools (e.g. webpack-bundle-analyzer) and I'm especially excited for the code coverage integration.

Outside of the minor nits in the comments, I think my biggest piece of feedback is it appears that this tab is wholly un-navigable via the keyboard? Tab goes right over the visualization and back to the inspect element button at the top of the window.

I would love to be able to move focus around between the items with the keyboard and use enter/space to invoke. It seems like it wouldn't be too much work to do this; presumably managing focus would be enough to make screenreaders work too?

View Change

4 comments:

File third_party/blink/renderer/devtools/front_end/network/RequestSourceMapView.js:
- Patch Set #9, Line 49: window.webtreemap.sort(treemapNodes);
  nit: it feels kinda weird to use 'window' here. I think we were using 'self' in other places, but is it even necessary? We don't prefix other globals like codemirror.
- Patch Set #9, Line 74: id: id,
  nit: can just put 'id' since the key/value variable are the same
File third_party/blink/renderer/devtools/front_end/webtreemap/webtreemap.js:
- Patch Set #9, Line 1: /**
  do we need a README.chromium for this file?
File third_party/blink/web_tests/http/tests/devtools/components/common-prefix.js:
- Patch Set #9, Line 17: var strings = ['c:/win32/calc.exe', 'c:/win32/notepad.exe', 'c:/win32/winmine.exe'];
  should we test with backslash? or can we guarantee these will always be properly canonicalized to forward slashes?

To view, visit change 1687089. To unsubscribe, or for help writing mail filters, visit settings.

Connor Clark (Gerrit)

unread,

Aug 20, 2019, 10:51:47 PM8/20/19

to Paul Irish, devtools-re...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Commit Bot, Joel Einbinder, Kayce Basques, Jeff Fisher, Tricium, chromium...@chromium.org, devtools...@chromium.org, Pavel Feldman

View Change

2 comments:

File third_party/blink/renderer/devtools/front_end/network/RequestSourceMapView.js:
- Patch Set #9, Line 80: const sourcePathSegments = source.replace(sharedPrefix, '').split('/');
  The `/` slashes here concern me. Could you try running this w/ a source map that uses windows `\` slashes?
File third_party/blink/web_tests/http/tests/devtools/components/common-prefix.js:

- Patch Set #9, Line 17: var strings = ['c:/win32/calc.exe', 'c:/win32/notepad.exe', 'c:/win32/winmine.exe'];

- should we test with backslash? or can we guarantee these will always be properly canonicalized to fo […]
  the unit under test here (common prefix) is slash (or any character) agnostic- but this is a really good point. I left a comment in the relative place (RequestSourceMapView.js)

To view, visit change 1687089. To unsubscribe, or for help writing mail filters, visit settings.

Paul Irish (Gerrit)

unread,

Aug 21, 2019, 8:58:11 PM8/21/19

to devtools-re...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Commit Bot, Joel Einbinder, Connor Clark, Kayce Basques, Jeff Fisher, Tricium, chromium...@chromium.org, devtools...@chromium.org, Pavel Feldman

Outside of the minor nits in the comments, I think my biggest piece of feedback is it appears that this tab is wholly un-navigable via the keyboard? Tab goes right over the visualization and back to the inspect element button at the top of the window.
I would love to be able to move focus around between the items with the keyboard and use enter/space to invoke. It seems like it wouldn't be too much work to do this; presumably managing focus would be enough to make screenreaders work too?

Sure, I gave that a shot. Do you want to provide feedback over on my treemap pull request?
https://github.com/paulirish/webtreemap-cdt/pull/1

View Change

To view, visit change 1687089. To unsubscribe, or for help writing mail filters, visit settings.

Jeff Fisher (Gerrit)

unread,

Aug 22, 2019, 4:27:06 PM8/22/19

to Paul Irish, devtools-re...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Commit Bot, Joel Einbinder, Connor Clark, Kayce Basques, Tricium, chromium...@chromium.org, devtools...@chromium.org, Pavel Feldman

Sure, I gave that a shot. Do you want to provide feedback over on my treemap pull request?
https://github.com/paulirish/webtreemap-cdt/pull/1

Thanks for doing this! (and also thanks for letting me use GitHub to review 😁)

View Change

To view, visit change 1687089. To unsubscribe, or for help writing mail filters, visit settings.

Josip Sokcevic (Gerrit)

unread,

Apr 6, 2021, 10:07:32 PM4/6/21

to Paul Irish, devtools-re...@chromium.org, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Joel Einbinder, Connor Clark, Kayce Basques, Jeff Fisher, Tricium, chromium...@chromium.org, devtools...@chromium.org, Pavel Feldman

Josip Sokcevic abandoned this change.

View Change

Abandoned

stale CL, restore if still relevant

To view, visit change 1687089. To unsubscribe, or for help writing mail filters, visit settings.

Paul Irish (Gerrit)

unread,

Mar 28, 2025, 6:16:44 PM3/28/25

to Tricium, Pavel Feldman, Jeff Fisher, chromium...@chromium.org, Joel Einbinder, devtools...@chromium.org, Kayce Basques, Connor Clark, devtools-re...@chromium.org, apavlo...@chromium.org, caseq...@chromium.org, pfeldma...@chromium.org, blink-...@chromium.org, lushnik...@chromium.org, kozyatins...@chromium.org

Paul Irish has uploaded the change for review

Paul Irish removed Commit Bot, Tricium, Pavel Feldman, Jeff Fisher, chromium...@chromium.org, Joel Einbinder, devtools...@chromium.org, Kayce Basques, Connor Clark, devtools-re...@chromium.org, apavlo...@chromium.org, caseq...@chromium.org, pfeldma...@chromium.org, blink-...@chromium.org, lushnik...@chromium.org and kozyatins...@chromium.org from reviewers of this change.

Commit message

DevTools: [Network] Add sourcemap treemap visualization

Bug: 981148

Change-Id: I9cc409ee74703cc07856c472537ba907b5fa1624

Change diff

Change information

Files:

M third_party/blink/renderer/devtools/.eslintignore
M third_party/blink/renderer/devtools/BUILD.gn
M third_party/blink/renderer/devtools/front_end/externs.js
M third_party/blink/renderer/devtools/front_end/network/NetworkItemView.js
A third_party/blink/renderer/devtools/front_end/network/RequestSourceMapView.js
M third_party/blink/renderer/devtools/front_end/network/module.json
M third_party/blink/renderer/devtools/front_end/network/network_strings.grdp
A third_party/blink/renderer/devtools/front_end/network/requestSourceMapView.css
M third_party/blink/renderer/devtools/front_end/sdk/NetworkRequest.js
M third_party/blink/renderer/devtools/front_end/sdk/SourceMap.js
M third_party/blink/renderer/devtools/front_end/sdk/SourceMapManager.js
M third_party/blink/renderer/devtools/front_end/shell.json
M third_party/blink/renderer/devtools/front_end/text_utils/TextUtils.js
A third_party/blink/renderer/devtools/front_end/webtreemap/module.json
A third_party/blink/renderer/devtools/front_end/webtreemap/webtreemap.js
A third_party/blink/web_tests/http/tests/devtools/components/common-prefix-expected.txt
A third_party/blink/web_tests/http/tests/devtools/components/common-prefix.js
M third_party/blink/web_tests/http/tests/devtools/modules-load-network-expected.txt

Change size: L

Delta: 18 files changed, 851 insertions(+), 3 deletions(-)

Open in Gerrit

Related details

Attention set is empty

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

Reply all

Reply to author

Forward

DevTools: [Network] Add sourcemap treemap visualization [chromium/src : master]

Paul Irish (Gerrit)

Joel Einbinder (Gerrit)

Jeff Fisher (Gerrit)

Connor Clark (Gerrit)

Paul Irish (Gerrit)

Joel Einbinder (Gerrit)

Paul Irish (Gerrit)

Paul Irish (Gerrit)

Paul Irish (Gerrit)

Paul Irish (Gerrit)

Jeff Fisher (Gerrit)

Connor Clark (Gerrit)

Paul Irish (Gerrit)

Jeff Fisher (Gerrit)

Josip Sokcevic (Gerrit)

Paul Irish (Gerrit)

Paul Irish has uploaded the change for review

Commit message

Change diff

Change information

Related details