Closed Bug 1290527 Opened 7 years ago Closed 7 years ago

Sourcemapped locations are not parsed properly for windows


(DevTools :: Console, defect)

Not set


(firefox51 fixed)

Firefox 51
Tracking Status
firefox51 --- fixed


(Reporter: jbhoosreddy, Assigned: jbhoosreddy, Mentored)



(Whiteboard: [source-maps])


(2 files, 2 obsolete files)

The sourcemapped locations for Windows host or development are not parsed properly.

Expected Result:
You should only see the file name in the location.

Observed Result:
You see the whole url instead of the file name in the location.
Assignee: nobody → jaideepb
Depends on: 1179813
Whiteboard: [source-maps]
Attached patch 1290527.patch (obsolete) — Splinter Review
In the initial source map work, the location parsing for Windows not implemented. I have tested it in a Windows environment visually. This is a very minor change so a try run might not be needed, but I can do it when I follow up on your review suggestions.
Attachment #8776084 - Flags: review?(jsantell)
Comment on attachment 8776084 [details] [diff] [review]

Review of attachment 8776084 [details] [diff] [review]:

::: devtools/client/shared/components/frame.js
@@ +191,5 @@
>          displaySource :
>          displaySource.slice(displaySource.lastIndexOf("/") + 1);
> +      displaySource = displaySource.lastIndexOf("\\") < 0 ?
> +        displaySource :
> +        displaySource.slice(displaySource.lastIndexOf("\\") + 1);

We should probably add these formatters to a frame utils method or something, also so we can easily test it. Since these are sourcemapped locations, they really can be any sort of representation (doesn't have to be "URL" like or file system like at all), and we're just trying to do our best to display it nicely, so maybe a 'format source map path' utility would be helpful (and for testing)
Attachment #8776084 - Flags: review?(jsantell)
Forgot to mention that this is a good approach, nice find! Other than moving the two formatters here to a frame utils file (maybe there's a better place?), once we add a test to it (there are tests for frame-utils), should be good for a quick r+!
Attached patch 1290527.patch [1.0] (obsolete) — Splinter Review
As per your comments, I moved the two file name formatters to a method in source-utils and added tests for the same.
Attachment #8776084 - Attachment is obsolete: true
Attachment #8776294 - Flags: review?(jsantell)
Comment on attachment 8776294 [details] [diff] [review]
1290527.patch [1.0]

Review of attachment 8776294 [details] [diff] [review]:

A few nits, but looks good! r+

::: devtools/client/shared/source-utils.js
@@ +308,5 @@
> + */
> +function getSourceMappedFile(source) {
> +  // If sourcemapped source is a OSX path, return
> +  // the characters after last "/".
> +  source = source.lastIndexOf("/") < 0 ?

These should probably be if conditionals rather than ternary ops at this point (both unix and windows)

::: devtools/client/shared/test/unit/test_source-utils.js
@@ +153,5 @@
>  });
> +// Test for source mapped file name
> +add_task(function* () {
> +  const { getSourceMappedFile } = sourceUtils;

We should add a normal case of some string returning the same value just as a base case
Attachment #8776294 - Flags: review?(jsantell) → review+
Attachment #8776294 - Attachment is obsolete: true
Attachment #8776336 - Flags: review+
Keywords: checkin-needed
Pushed by
Fix a parsing issue for sourcemapped locations on Windows. r=jsantell
Keywords: checkin-needed
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
I have successfully reproduce this bug on firefox nightly 50.0a1 (2016-07-29)
with windows 7 (32 bit)
Mozilla/5.0 (Windows NT 6.1; rv:50.0) Gecko/20100101 Firefox/50.0

I found this fix on latest nightly 51.0a1 (2016-08-14)

Mozilla/5.0 (Windows NT 6.1; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID : 20160814030203

Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.