1215953 - Import heap snapshots

Status: RESOLVED FIXED

(DevTools :: Memory, defect)

Comment 1

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

Attachment: 1215953-snapshot-import.patch

fitzgen for review, ntim for CSS review

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8686162 [details] [diff] [review]
1215953-snapshot-import.patch

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

Looks great, thanks!

Just nits below.

::: devtools/client/locales/en-US/memory.properties
@@ +65,5 @@
>  take-snapshot=Take snapshot
>  
> +# LOCALIZATION NOTE (import-snapshot): The label describing the button that initiates
> +# importing a snapshot.
> +import-snapshot=Import...

Use an ellipsis here, there are some in this file that you can copy paste.

@@ +99,5 @@
>  
> +# LOCALIZATION NOTE (snapshot.state.importing.full): The label describing the snapshot
> +# state IMPORTING, used in the main heap view. %S represents the basename of the file
> +# imported.
> +snapshot.state.importing.full=Importing %S…

For example, here is an ellipsis to copy ;)

::: devtools/client/memory/test/unit/test_action-import-snapshot-and-census.js
@@ +23,5 @@
> +  let { subscribe, dispatch, getState } = store;
> +
> +  /**
> +   * Export a snapshot first
> +   */

/**
 * Why do we need such a big announcement? These comments are usually
 * reserved for classes, functions, interfaces, modules, etc.
 */

@@ +28,5 @@
> +  let file = FileUtils.getFile("TmpD", ["tmp.fxsnapshot"]);
> +  file.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);
> +  let destPath = file.path;
> +  let stat = yield OS.File.stat(destPath);
> +  ok(stat.size === 0, "new file is 0 bytes at start");

Can you factor this mess out into a shared function in head.js? It is really gross looking, and my delicate sense of aesthetics would prefer it wasn't duplicated across multiple tests.

Comment 3

[Tim Nguyen :ntim]

Comment on attachment 8686162 [details] [diff] [review]
1215953-snapshot-import.patch

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

::: devtools/client/themes/memory.css
@@ +64,5 @@
>    /**
>     * We want this to be exactly at a --sidebar-width distance from the
>     * toolbar's start boundary. A .devtools-toolbar has a 3px start padding
> +   * and the preceeding .take-snapshot button is exactly 32px, and the import
> +   * button 78px.

Are you sure it's 78px for all locales ? It's very likely going to be longer for other locales.

@@ +69,2 @@
>     */
> +  margin-inline-start: calc(var(--sidebar-width) - 3px - 32px - 78px);

I'm not too happy with the margin hack we're using. There are better ways to accomplish our current layout. Can you file a bug to fix that up ?

@@ +99,5 @@
>  
> +/**
> + * Due to toolbar styles of `.devtools-toolbarbutton:not([label])` which overrides
> + * .devtools-toolbarbutton's min-width of 78px, reset the min-width.
> + */

Ideally, I'd use devtools-button since it's for HTML, but unfortunately, devtools-button isn't well tailored for text buttons. Anyway, I'm fine with keeping .devtools-toolbarbutton for now, and optimize devtools-button for text buttons in another bug.

Comment 4

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] from comment #3)
> Comment on attachment 8686162 [details] [diff] [review]
> 1215953-snapshot-import.patch
> 
> Review of attachment 8686162 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/themes/memory.css
> @@ +64,5 @@
> >    /**
> >     * We want this to be exactly at a --sidebar-width distance from the
> >     * toolbar's start boundary. A .devtools-toolbar has a 3px start padding
> > +   * and the preceeding .take-snapshot button is exactly 32px, and the import
> > +   * button 78px.
> 
> Are you sure it's 78px for all locales ? It's very likely going to be longer
> for other locales.
If it's not, it's likely going to break the layout for other locales.

Comment 5

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

Comment on attachment 8686162 [details] [diff] [review]
1215953-snapshot-import.patch

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

::: devtools/client/locales/en-US/memory.properties
@@ +65,5 @@
>  take-snapshot=Take snapshot
>  
> +# LOCALIZATION NOTE (import-snapshot): The label describing the button that initiates
> +# importing a snapshot.
> +import-snapshot=Import...

Sure, perf uses "..." in this instance, though

::: devtools/client/memory/test/unit/test_action-import-snapshot-and-census.js
@@ +23,5 @@
> +  let { subscribe, dispatch, getState } = store;
> +
> +  /**
> +   * Export a snapshot first
> +   */

Mostly to visually separate and can skip over things that aren't being explicitly tested (i.e. exporting)

::: devtools/client/themes/memory.css
@@ +69,2 @@
>     */
> +  margin-inline-start: calc(var(--sidebar-width) - 3px - 32px - 78px);

min-width is 78px, so could be longer, so this would break for smaller locales, you're right, not sure a better way to do this

Comment 6

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

Comment on attachment 8686162 [details] [diff] [review]
1215953-snapshot-import.patch

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

Made the changes other than the positioning of the toolbar not using the calc of the sidebar width, should be a follow up if necessary

Comment 7

[Pulsebot]

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

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e4d573fb1fc1