Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: update cdt SourceMap to latest devtools frontend #13095

Merged
merged 4 commits into from
Oct 6, 2021

Conversation

connorjclark
Copy link
Collaborator

Should be no behavior changes. But possibly a bug fix or two. Test didn't change. see history: https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/front_end/core/sdk/SourceMap.ts;bpv=1

Recent-ish refactors to frontend allows us to delete the Array.prototype hacking from our SDK.js.

@connorjclark connorjclark requested a review from a team as a code owner September 21, 2021 21:09
@connorjclark connorjclark requested review from adamraine and removed request for a team September 21, 2021 21:09
@google-cla google-cla bot added the cla: yes label Sep 21, 2021
...expressionsToRemove,
...Object.values(rawCodeToReplace),
...Object.keys(rawCodeToReplace),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops :)

// https://github.com/ChromeDevTools/devtools-frontend/blob/master/front_end/core/platform/array-utilities.ts#L125

/**
* @param {any[]} array
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not worth effort / possibly impossible to denote in jsdoc

;
;
;
;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah there's some junk in here but i don't really mind.

@@ -127,9 +127,9 @@ class UnusedJavascriptSummary {
let offset = lineOffsets[mapping.lineNumber];

offset += mapping.columnNumber;
const lastColumnOfMapping =
// @ts-expect-error: We will upstream lastColumnNumber to CDT eventually.
(mapping.lastColumnNumber - 1) || lineLengths[mapping.lineNumber];
Copy link
Collaborator Author

@connorjclark connorjclark Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mapping.lastColumnNumber is sometimes undefined (for the last mapping of each line), and in that case this value falls back to the line length (acknowledgement: this is mixing data defined in the map and data from the script content).

arguably this should be calculated in computeLastGeneratedColumns, but 1) TextSourceMap doesn't know about the script content and 2) each mapping doesn't know how long of a range it covers, so there is no way to know what to set the lastColumnNumber of the last mapping on a line. All of this may mean upstreaming this property will never be feasible.

@connorjclark connorjclark merged commit 6c3d609 into master Oct 6, 2021
@connorjclark connorjclark deleted the update-sourcemap-cdt branch October 6, 2021 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants