Closed Bug 1259227 Opened 4 years ago Closed 3 years ago

Exported profile's name is clipped after the first dot in file name

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect, P3)

defect

Tracking

(firefox48 affected, firefox52 verified)

RESOLVED FIXED
Firefox 52
Tracking Status
firefox48 --- affected
firefox52 --- verified

People

(Reporter: arni2033, Assigned: wizard_A, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: [btpp-backlog][good first bug][lang=js])

Attachments

(1 file, 3 obsolete files)

>>>   My Info:   Win7_64, Nightly 48, 32bit, ID 20160323030400
STR:
1. Open new tab
2. Open Devtools -> Performance
3. Press button "Start Recording Performance". Press it again to end recording.
4. Click "Save" near the recorded profile, save it with name:
  "2016.03.23 21-24-39 - profile recorded on gaming.youtube.com.json"
5. Reopen Profiler, click "Import", import the file from Step 4

AR:  Title says "2016"
ER:  Title should say something more sensible, or at least provide a title with full filename
Mentor: gtatum
Has STR: --- → yes
Priority: -- → P3
Whiteboard: [btpp-backlog][good first bug][lang=js]
Hi Greg,

Would like to work on this... Could you please explain the file i need to look into and a little more info on why this could be happening.

Thanks,
Amit.
Flags: needinfo?(gtatum)
Hello Amit :)


Here's the flow of what's happening:

The performance controller reacts to the import button click, and calls the performance actor:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/performance/performance-controller.js#325-330

The actor then loads the recording from the file:
https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/performance.js#257-274

Here is the actual bug. The filename is turned into the label. As you can see the regex is too greedy finding the last dot: https://dxr.mozilla.org/mozilla-central/source/devtools/client/performance/modules/io.js#107-111
Flags: needinfo?(gtatum)
Hi Greg,

Thanks for pointing me to the right direction. Please find attached the patch for the changes.

Thanks,
Amit.
Attachment #8744360 - Flags: review?(gtatum)
(In reply to Amit Kr. Chandra [:wizard_A] from comment #3)
> Created attachment 8744360 [details] [diff] [review]
The file is empty.
Flags: needinfo?(amitforfriends_dns)
Hi Greg, Arni,

Sorry about that...
Please find the attached patch.

Thanking You,
Amit.
Attachment #8744360 - Attachment is obsolete: true
Attachment #8744360 - Flags: review?(gtatum)
Flags: needinfo?(amitforfriends_dns)
Attachment #8744363 - Flags: review?(gtatum)
Woops, it looks like we missed a place. It's still clipping the name when you first save the snapshot, but it is fixed when you import. It'd probably be good to hit this one as well. After that it should be good and I'll send it off to the try server to run the tests.

https://dxr.mozilla.org/mozilla-central/source/devtools/client/performance/views/recordings.js#214

Thanks for taking this on!
Attachment #8744363 - Flags: review?(gtatum)
Hi Greg,
Hope this resolves the issue.... Thanks..
Attachment #8744363 - Attachment is obsolete: true
Attachment #8747430 - Flags: review?(gtatum)
Comment on attachment 8747430 [details] [diff] [review]
Bug-1259227_Change_regex_to_replace_extension.patch

Review of attachment 8747430 [details] [diff] [review]:
-----------------------------------------------------------------

The code looks good to me, and it has the desired behavior on my local machine. I'm pushing this up to the try servers to run the tests on it. After a clean run I'll mark it to be checked in. Thanks for the work on this!
Attachment #8747430 - Flags: review?(gtatum) → review+
Why is it so necessary to clip something? What if filenames are like "2016-05-04.bak-<timestamp>" ?
Where "<timestamp>" is several digits representing timestamp.
Debugger, style editor and scratchpad don't clip file extension.
Assignee: nobody → amitforfriends_dns
[good first bug] whiteboard -> keyword mass change
Keywords: good-first-bug
Blocks: perf-bug
Flags: needinfo?(gtatum)
I rebased and shortened the commit message a bit.
Attachment #8747430 - Attachment is obsolete: true
Flags: needinfo?(gtatum)
Attachment #8799970 - Flags: review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/955fd8d87783
Update regex for the imported profile name in the performance tool. r=gregtatum
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/955fd8d87783
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Hey Amit, thanks for the contribution! Sorry it took so long to get resolved :)
I have successfully reproduce this bug on firefox nightly 48.0a1 (2016-03-23)
with windows 7 (32 bit)
Mozilla/5.0 (Windows NT 6.1; rv:48.0) Gecko/20100101 Firefox/48.0

I found this fix on latest nightly 52.0a1 (2016-10-16) (32-bit)

Mozilla/5.0 (Windows NT 6.1; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID : 20161016030205
[bugday-20170215]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.