1161698 - Imported profiles should use their filename as the display label

Status: RESOLVED FIXED

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

Description

[Jordan Santell [:jsantell] [@jsantell]]

Or, when saving, save the RecordingModel's label as the filename (s/\.json//) before exporting. I think I like the latter option better, too.

Comment 1

[Wilhem Barbier [:nounoursheureux]]

Attachment: bug-1161698-save-label-as-filename.patch

I implemented the behaviour you descripted in your comment. Should I write tests ? If yes, where are they located ?

Comment 2

[Jordan Santell [:jsantell] [@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

Comment 3

[Wilhem Barbier [:nounoursheureux]]

Attachment: bug1161698_set_label_to_filename_when_importing.patch

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/692b2fea4ef6

Comment 5

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/b7cb976e52a6

Comment 6

[Wilhem Barbier [:nounoursheureux]]

This was actually the good patch :/ . I'm sorry I messed up the patches, now it's causing a lot of trouble.

Comment 7

[Jordan Santell [:jsantell] [@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?

Comment 8

[Jordan Santell [:jsantell] [@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!

Comment 9

[Wilhem Barbier [:nounoursheureux]]

Attachment: bug1161698_set_label_to_filename_when_importing.patch

Here is the revised patch :)

Comment 10

[Jordan Santell [:jsantell] [@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(/\..+$/, "");

Comment 11

[Jordan Santell [:jsantell] [@jsantell]]

Ping Wilhem -- any updates on this? I can help with a small test for this as well if needed. Thanks!

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

No updates in many months, unassigning.

Comment 13

[Henrik Tjäder]

Attachment: bug_1161698_set_label_to_filename_when_importing.patch

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.

Comment 14

[Jordan Santell [:jsantell] [@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

Comment 15

[Jordan Santell [:jsantell] [@jsantell]]

ni? myself so i can check back on the tests later

Comment 16

[Jordan Santell [:jsantell] [@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

Comment 17

[Jordan Santell [:jsantell] [@jsantell]]

Retrying with the change in comment#16 https://treeherder.mozilla.org/#/jobs?repo=try&revision=39db259ab00e

Comment 18

[Henrik Tjäder]

Attachment: bug_1161698_set_label_to_filename_when_importing_ver2.patch

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.

Comment 19

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1161698.patch

Pushing to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=228a18d6a645

Comment 20

[Jordan Santell [:jsantell] [@jsantell]]

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

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7234f26597b1

Comment 22

[Jordan Santell [:jsantell] [@jsantell]]

Just landed this -- thanks for your contributions, Henrik!

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7234f26597b1