Sourcemapped locations are not parsed properly for windows

RESOLVED FIXED in Firefox 51

Status

()

Firefox
Developer Tools: Console
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jbhoosreddy, Assigned: jbhoosreddy, Mentored)

Tracking

unspecified
Firefox 51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [source-maps])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

a year ago
Created attachment 8776078 [details]
Screen Shot 2016-07-29 at 11.31.19 AM.png

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)

Updated

a year ago
Assignee: nobody → jaideepb
(Assignee)

Updated

a year ago
Status: NEW → ASSIGNED
(Assignee)

Updated

a year ago
Depends on: 1179813
Whiteboard: [source-maps]
(Assignee)

Comment 1

a year ago
Created attachment 8776084 [details] [diff] [review]
1290527.patch

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]
1290527.patch

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+!
(Assignee)

Comment 4

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2693363a7f44
(Assignee)

Comment 5

a year ago
Created attachment 8776294 [details] [diff] [review]
1290527.patch [1.0]

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+
(Assignee)

Comment 7

a year ago
Created attachment 8776336 [details] [diff] [review]
1290527.patch [2.0]
Attachment #8776294 - Attachment is obsolete: true
Attachment #8776336 - Flags: review+
(Assignee)

Comment 8

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d62e0f7ef0c8
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 9

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/6981e2d0a09f
Fix a parsing issue for sourcemapped locations on Windows. r=jsantell
Keywords: checkin-needed

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6981e2d0a09f
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51

Comment 11

a year ago
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

[testday-20160812]
You need to log in before you can comment on or make changes to this bug.