Closed Bug 1161698 Opened 5 years ago Closed 4 years ago

Imported profiles should use their filename as the display label

Categories

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

37 Branch
defect
Not set

Tracking

(firefox44 fixed)

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: jsantell, Assigned: henrik, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 5 obsolete files)

Or, when saving, save the RecordingModel's label as the filename (s/\.json//) before exporting. I think I like the latter option better, too.
Mentor: jsantell
Whiteboard: [good first bug][lang=js]
See Also: → 963005
I implemented the behaviour you descripted in your comment. Should I write tests ? If yes, where are they located ?
Attachment #8646215 - Flags: review?(jsantell)
Comment on attachment 8646215 [details] [diff] [review]
bug-1161698-save-label-as-filename.patch

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

Thanks! I think instead of saving the label in the file, I think going with changing it upon importing is the better way to go -- this lets the user change the file name, and when importing, uses the latest filename, rather than whatever it was originally exported as.

So, same idea, just in loadRecordingFromFile, check if there is a label property, and if not, use the current file name (stripping the extension). Testing should be pretty straight forward, I think checking in `browser_perf_recordings-io-01.js` that the label is "tmpprofile" should do the trick, rather than comparing the label properties of the imported file (because they will no longer be equal, and is just checking undefined, I think anyway). Please send another review with those changes, and I think this'll be great :)

::: browser/devtools/performance/modules/logic/io.js
@@ +54,5 @@
>      let deferred = promise.defer();
>  
>      recordingData.fileType = PERF_TOOL_SERIALIZER_IDENTIFIER;
>      recordingData.version = PERF_TOOL_SERIALIZER_CURRENT_VERSION;
> +    recordingData.label = file.leafName.replace(/\..+$/, ''); // set the label to the filename without its extension

nit: Move the comment so it's above this line, rather than at the end of it
Attachment #8646215 - Flags: review?(jsantell)
Assignee: nobody → schtroumps31
Attachment #8646215 - Attachment is obsolete: true
Attachment #8647560 - Flags: review?(jsantell)
This was actually the good patch :/ . I'm sorry I messed up the patches, now it's causing a lot of trouble.
Flags: needinfo?(jsantell)
So there was some large changes over the last few days -- I think this'll need to be rebased, the io.js file having moved to toolkit/devtools/performance/io.js -- Wilhem, can you check this out to see if everything is still good on this patch?
Flags: needinfo?(jsantell)
Comment on attachment 8647560 [details] [diff] [review]
bug1161698_set_label_to_filename_when_importing.patch

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

Removing this review for now -- let me know if there are any questions you have or how I can help!
Attachment #8647560 - Flags: review?(jsantell)
Here is the revised patch :)
Attachment #8647560 - Attachment is obsolete: true
Flags: needinfo?(jsantell)
Attachment #8651654 - Flags: review?(jsantell)
Comment on attachment 8651654 [details] [diff] [review]
bug1161698_set_label_to_filename_when_importing.patch

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

Looks great! Just some minor formatting changes

::: toolkit/devtools/performance/io.js
@@ +104,4 @@
>      if (recordingData.profile.meta.version === 2) {
>        RecordingUtils.deflateProfile(recordingData.profile);
>      }
> +    if(!recordingData.label)

nit: space after `if` and opening this curly bracket on this line, so:

`if (!recordingData.label) {`

@@ +106,5 @@
>      }
> +    if(!recordingData.label)
> +    {
> +      // set it to the filename without its extension
> +      recordingData.label = file.leafName.replace(/\..+$/, '');

Use double quotes
recordingData.label = file.leafName.replace(/\..+$/, "");
Attachment #8651654 - Flags: review?(jsantell)
Flags: needinfo?(jsantell)
Ping Wilhem -- any updates on this? I can help with a small test for this as well if needed. Thanks!
Flags: needinfo?(nounoursheureux)
No updates in many months, unassigning.
Assignee: nounoursheureux → nobody
Flags: needinfo?(nounoursheureux)
Packaged Wilhelm's code along with the suggested fixes into the new file locations.

The label to the left in performance profiler now contains the filename when importing, rather than just an incremented number.
Attachment #8671298 - Flags: review?(jsantell)
Comment on attachment 8671298 [details] [diff] [review]
bug_1161698_set_label_to_filename_when_importing.patch

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

Looks good, Henrik -- thanks! Pushing to try to see how it does: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6498cd8ad3b8
Attachment #8671298 - Flags: review?(jsantell) → review+
ni? myself so i can check back on the tests later
Flags: needinfo?(jsantell)
Looks like there's a small error in the test[0] -- the tmpprofile created appends additional characters to ensure uniqueness -- the test finds them as `tmpprofile-3` -- we can just check to see if the label starts with `tmpprofile` in this case.

[0] https://treeherder.mozilla.org/#/jobs?repo=try&revision=6498cd8ad3b8
Flags: needinfo?(jsantell) → needinfo?(henrik)
Assignee: nobody → henrik
Errors from Comment#17 refers to "devtools/client/performance/test/browser_perf-recordings-io-04.js", line 146. It would seem we also need to "ignore" the '-3' portion of the output. I am not sure if this is sufficient or the nicest solution, but here goes:

Based on comment #16 and error output from test run in comment #17.


Additionally, could someone please hint how to locally run these tests, there's such an abundance of different kinds accessible. Tried ./mach mochitest "filename" but that tests something else.
Attachment #8671298 - Attachment is obsolete: true
Flags: needinfo?(henrik)
Attachment #8673113 - Flags: review?(jsantell)
Attached patch 1161698.patchSplinter Review
Pushing to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=228a18d6a645
Attachment #8651654 - Attachment is obsolete: true
Attachment #8673113 - Attachment is obsolete: true
Attachment #8673113 - Flags: review?(jsantell)
Attachment #8673209 - Flags: review+
For running tests, `./mach mochitest path/to/test` will work, and testing the whole tool can be run via `./mach mochitest devtools/client/performance` in this case. These are browser mochitests and more info is here: https://developer.mozilla.org/en-US/docs/Browser_chrome_tests
Just landed this -- thanks for your contributions, Henrik!
https://hg.mozilla.org/mozilla-central/rev/7234f26597b1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Depends on: 1252100
Depends on: 1259227
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.