Closed
Bug 963005
Opened 10 years ago
Closed 9 years ago
Saving a profile should also rename the currently selected one
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect, P3)
DevTools
Performance Tools (Profiler/Timeline)
Tracking
(firefox43 fixed)
RESOLVED
FIXED
Firefox 43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: past, Assigned: anjalymehla51, Mentored)
References
Details
(Whiteboard: [good first bug][lang=js])
Attachments
(4 files, 5 obsolete files)
3.68 KB,
patch
|
jsantell
:
review+
|
Details | Diff | Splinter Review |
5.96 KB,
image/png
|
Details | |
2.17 KB,
patch
|
jsantell
:
review+
|
Details | Diff | Splinter Review |
6.12 KB,
patch
|
jsantell
:
review+
|
Details | Diff | Splinter Review |
When saving a profile, the name of the currently selected profile in the list is not updated to the new name. That means that creating multiple profiles and then saving them results in Profile 1, Profile 2, etc. without the descriptive names of their persisted versions. Moreover importing the stored profile will add a new entry to the list with the exact same profile.
Updated•10 years ago
|
Priority: -- → P3
Updated•9 years ago
|
Hi I would like to take up on this fix. Could you give me pointer and assist me with the same?
Flags: needinfo?(jsantell)
Comment 2•9 years ago
|
||
Hey anjaly -- here's what I think the best way to do this is: In the RecordingsView (browser/devtools/performance/views/recordings.js), listen to the event on the PerformanceController RECORDING_EXPORTED (bound where the other events for PerformanceController is) and call a function that takes a recording from the event, and updates the label element. Currently, RECORDING_EXPORTED only emits the recording model, but we can also emit the file that the recording was just saved as. With that, we can get the file name. A handler could be something like (not tested, but the idea): _onRecordingExported: function (_, recording, file) { // First we should check that the recording is not from the console, in which case, // it already has a label and we shouldn't update it if (recording.isConsole()) { return; } // Get the "recordingItem" for the recording, which is a representation of a list item in this widget let recordingItem = this.getItemForPredicate(e => e.attachment === recording); let titleNode = $(".recording-item-title", recordingItem.target); // Update the element with the file name, stripping out the extension titleNode.setAttribute("value", file.leafName.replace(/\..+$/, ""); } Check out _onRecordingStateChange in the RecordingsView, that takes a recording model, gets the 'recordingItem', and updates the duration property, similar how the label property should be updated. The test could probably be added to the general recording tests too. Let me know if you have any questions and thanks!
Assignee: nobody → anjalymehla51
Flags: needinfo?(jsantell)
So where exactly this event in workflow should fit in? While saving , the file is exported with the default string name "profile" and emitted . The _onRecordingExported function should be called out before _onSaveButtonClick (before the save function emits the recording? And if thats the case then why not rename it beforehand?
Use needinfo? to be sure questions aren't lost.
Flags: needinfo?(jsantell)
The file which RECORDING_EXPORTED event emits is null . The file is accessible after this :- this.emit(EVENTS.UI_EXPORT_RECORDING, recordingItem.attachment, fp.file); However in here _onRecordingExported: function (_, recording, file){ if (recording.isConsole()) { return; } let recordingItem = this.getItemForPredicate(e => e.attachment === recording); let titleNode = $(".recording-item-title", recordingItem.target); titleNode.setAttribute("value", file.leafName.replace(/\..+$/, "")); }, "file" seems to be undefined . Is there something else to be added to explicitly ask the event to emit file as well?
Ok nevermind . The RECORDING_EXPORTED event earlier emitted only the recording not file. I changed it in performance-controller.js so that file is emitted as well. The module is working fine. I will submit the patch in time :)
Ok nevermind . The RECORDING_EXPORTED event earlier emitted only the recording not file. I changed it in performance-controller.js so that file is emitted as well. The module is working fine. I will submit the patch in time :)
Attachment #8651497 -
Flags: review?(jsantell)
Assignee | ||
Comment 10•9 years ago
|
||
This patch contain code for displaying actual file name after an import.
Attachment #8651520 -
Flags: review?(jsantell)
Updated•9 years ago
|
Attachment #8651497 -
Flags: review?(jsantell)
Comment 11•9 years ago
|
||
Comment on attachment 8651520 [details] [diff] [review] display_name_after_import_export.patch Review of attachment 8651520 [details] [diff] [review]: ----------------------------------------------------------------- Overall, looks great! Some big things though, we should remove any of the importing handlers and things like that, this is only for exporting (another bug handles that), and some formatting. Can you combine these two into the same patch? It's a bit hard to review them separately. Thanks! ::: browser/devtools/performance/performance-controller.js @@ +337,5 @@ > * The file to stream the data into. > */ > exportRecording: Task.async(function*(_, recording, file) { > yield recording.exportRecording(file); > + this.emit(EVENTS.RECORDING_EXPORTED, recording,file); Space between the comma and "file": `recording, file` @@ +370,5 @@ > */ > importRecording: Task.async(function*(_, file) { > let recording = yield gFront.importRecording(file); > this._addNewRecording(recording); > + this.emit(EVENTS.RECORDING_IMPORTED, recording,file); There's another bug (which is done I believe) which does this for importing profiles and renaming, so the importing work here should be taken out @@ +375,5 @@ > > // Only emit in tests for legacy purposes for shorthand -- > // other things in UI should handle the generic NEW_RECORDING > // event to handle lazy recordings. > + //if (DevToolsUtils.testing) { This should not be commented out ::: browser/devtools/performance/views/recordings.js @@ +18,5 @@ > this._onNewRecording = this._onNewRecording.bind(this); > this._onSaveButtonClick = this._onSaveButtonClick.bind(this); > this._onRecordingsCleared = this._onRecordingsCleared.bind(this); > + this._onRecordingExported=this._onRecordingExported.bind(this); > + this._onRecordingImported=this._onRecordingImported.bind(this); These should be lined up with the rest of the function bindings, and spaces around the " = " @@ +25,5 @@ > PerformanceController.on(EVENTS.RECORDING_STATE_CHANGE, this._onRecordingStateChange); > PerformanceController.on(EVENTS.NEW_RECORDING, this._onNewRecording); > PerformanceController.on(EVENTS.RECORDINGS_CLEARED, this._onRecordingsCleared); > + PerformanceController.on(EVENTS.RECORDING_EXPORTED, this._onRecordingExported); > + PerformanceController.on(EVENTS.RECORDING_IMPORTED, this._onRecordingImported); Remove the importing listeners @@ +202,5 @@ > this.emit(EVENTS.UI_EXPORT_RECORDING, recordingItem.attachment, fp.file); > }}); > }, > > + _onRecordingExported: function (_, recording,file){ space before "file", and the formatting in this function should be two spaces of indentation @@ +209,5 @@ > + } > + let recordingItem = this.getItemForPredicate(e => e.attachment === recording); > + let titleNode = $(".recording-item-title", recordingItem.target); > + titleNode.setAttribute("value", file.leafName.replace(/\..+$/, "")); > + extra white space @@ +212,5 @@ > + titleNode.setAttribute("value", file.leafName.replace(/\..+$/, "")); > + > + }, > + > + _onRecordingImported:function (_,recording,file){ Remove things related to importing
Attachment #8651520 -
Flags: review?(jsantell)
Updated•9 years ago
|
Flags: needinfo?(jsantell)
Assignee | ||
Comment 12•9 years ago
|
||
Final patch ! The patch earlier added were kind of draft thats why indentation lacked. Sorry for the trouble
Attachment #8651497 -
Attachment is obsolete: true
Attachment #8651520 -
Attachment is obsolete: true
Attachment #8651527 -
Flags: review?(jsantell)
Comment 13•9 years ago
|
||
Comment on attachment 8651527 [details] [diff] [review] final_export_display_name.patch Review of attachment 8651527 [details] [diff] [review]: ----------------------------------------------------------------- This looks like only one part, can you combine all the changes into one patch?
Attachment #8651527 -
Flags: review?(jsantell)
Assignee | ||
Comment 14•9 years ago
|
||
This contain all the changes in performance-controller.js and recording.js (I hope this should work )
Attachment #8651527 -
Attachment is obsolete: true
Attachment #8651531 -
Flags: review?(jsantell)
Comment 15•9 years ago
|
||
Comment on attachment 8651531 [details] [diff] [review] export_name_display.patch Review of attachment 8651531 [details] [diff] [review]: ----------------------------------------------------------------- It looks like you're diffing the patch between what you have currently, and previous patches, so it cannot be applied -- notice the left side of the diff[0] still containing your changes. See if you can squash down all of your commits into one -- I think this might work[1] [0] https://bugzilla.mozilla.org/attachment.cgi?id=8651531&action=diff [1] http://stackoverflow.com/a/1725638
Attachment #8651531 -
Flags: review?(jsantell)
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8651800 -
Flags: review?(jsantell)
Comment 17•9 years ago
|
||
Comment on attachment 8651800 [details] [diff] [review] export_name_display.patch Review of attachment 8651800 [details] [diff] [review]: ----------------------------------------------------------------- Some small comments on formatting, but looks great! I can write tests or help you write tests, just let me know! ::: browser/devtools/performance/views/recordings.js @@ +200,5 @@ > }}); > }, > > + _onRecordingExported: function (_, recording, file){ > + if(recording.isConsole()) { nit: space after `if` @@ +201,5 @@ > }, > > + _onRecordingExported: function (_, recording, file){ > + if(recording.isConsole()) { > + return; nit: indentation, only two spaces @@ +207,5 @@ > + let recordingItem = this.getItemForPredicate(e => e.attachment === recording); > + let titleNode = $(".recording-item-title", recordingItem.target); > + titleNode.setAttribute("value", file.leafName.replace(/\..+$/, "")); > + }, > + nit: remove extra whitespace here
Attachment #8651800 -
Flags: review?(jsantell)
Assignee | ||
Comment 18•9 years ago
|
||
All indentation take cared of! Yeah sure I can try if you could give me some pointers about tests.
Attachment #8651531 -
Attachment is obsolete: true
Attachment #8651800 -
Attachment is obsolete: true
Attachment #8651915 -
Flags: review?(jsantell)
Comment 19•9 years ago
|
||
Great! For a test, you should be able to add a line in browser_perf_recordings-io-04.js, right after exporting, check to see if the label is updated, something like this should work: is($(".recording-item-title").getAttribute("value"), EXPECTED_LABEL, "Exported recording is displayed with the file name as the label");
Updated•9 years ago
|
Attachment #8651915 -
Flags: review?(jsantell) → review+
Assignee | ||
Comment 20•9 years ago
|
||
For tests , using getAttribute gives the file name of exported file , but to verify it there has to be some definition for EXPECTED_LABEL, where is it coming from? I cannot figure out where to take the parameter from in browser_perf_recordings-io-05.js file once exporting is done.!
Comment 21•9 years ago
|
||
I think browser_perf_recordings-io-01.js is a better test to add this to. The file is saved as 'tmpprofile.json'[0], so you'd expect the rendered display to be "tmpprofile", right? [0] https://github.com/mozilla/gecko-dev/blob/2be1d3e641685fff871e1570fbb01bdfdf161ad1/browser/devtools/performance/test/browser_perf_recordings-io-01.js#L39
Assignee | ||
Comment 22•9 years ago
|
||
The test fails and this is the output
Comment 23•9 years ago
|
||
Looks like creating that temporary file, the `createUnique` function[0] will add on various endings so it doesn't have any save conflicts -- we can check here instead that "tmpprofile" is in the displayed name and that "json" is not. Also a comment why this is necessary would be useful let displayedName = $(".recording-item-title").getAttribute("value"); ok(/^tmpprofile/.test(displayedName), "has expected display name after import"); ok(!/\.json$/.test(displayedName), "display name does not have .json in it"); [0] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIFile#createUnique%28%29
Assignee | ||
Comment 24•9 years ago
|
||
Attachment #8654532 -
Flags: review?(jsantell)
Updated•9 years ago
|
Attachment #8654532 -
Flags: review?(jsantell) → review+
Comment 25•9 years ago
|
||
Gonna push this to our try servers once they're up (hg.mozilla.org seems down at the moment for me?), but I think everything looks good :)
Attachment #8654542 -
Flags: review+
Updated•9 years ago
|
Status: NEW → ASSIGNED
Comment 26•9 years ago
|
||
trees up, pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=494b9b63ca66
Comment 28•9 years ago
|
||
Ok test run looks great, now pushed and landed in fx-team -- should hit nightly in a day! Thanks so much for the contribution and patience, anjaly!
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
Comment 29•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4ceab182385b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 43
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•