1248988 - User-facing terminology for memory tool

Status: RESOLVED FIXED

(DevTools :: Memory, defect, P1)

Description

[Greg Tatum [:gregtatum]]

The terminology in the memory tools reflects the internals of the code, and isn't necessarily geared toward the end users. In order to fix this we can do an audit of the terminology used and make it consistent and simple, but still technically accurate. For instance there is the phrase "Saving census" which could be rephrased to "Saving report".

Comment 1

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

Greg, do you think you can take this bug?

Comment 2

[Greg Tatum [:gregtatum]]

Renaming the items in the "Group By" dropdown is probably one of the bigger parts of this, which is being tackled on: https://bugzilla.mozilla.org/show_bug.cgi?id=1249779

Comment 3

[Greg Tatum [:gregtatum]]

Is it ok to mark this as resolved? I believe Bug #1249779 resolved this issue. Any other thoughts on it?

Comment 4

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

(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #3)
> Is it ok to mark this as resolved? I believe Bug #1249779 resolved this
> issue. Any other thoughts on it?

Can you triage all the strings in memory.properties and suggest better wordings where you think they aren't great?

Comment 5

[Greg Tatum [:gregtatum]]

Attachment: User-facing terminology for memory tool

I went ahead and started editing everything in `memory.properties` to see if
I could simplify the words used, so check out the diff on the patch to see what
some of my suggested edits are.

The phrases "heap snapshot" and "snapshot" were both used interchangeably
throughout. Since we're already in the Memory tool I figured it would be simpler to
consolidate these phrases, as the word "heap" does not necessarily give more
information about what type of snapshot we are looking at.

I completed the renaming of "allocation stacks" to "call stacks".

I tried to re-word the status updates for how the snapshots were being processed to
not include jargon, and provide a little bit more context for what was being
displayed.

There was also some graph theory terminology like tree, and nodes. I tried to reduce
the usage of those terms as well.

Thoughts?

Comment 6

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

Comment on attachment 8737375 [details] [diff] [review]
User-facing terminology for memory tool

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

::: devtools/client/locales/en-US/memory.properties
@@ +57,2 @@
>  # stacks.
> +checkbox.recordAllocationStacks.tooltip=Toggle the recording of call stacks. Subsequent snapshots will be able to group and label object by call stacks, but only with those objects created after toggling this option. Recording call stacks has a performance overhead.

How about we add "when objects are allocated." to the end of the first sentence. I think it is important that we mention somewhere *which* call stacks get recorded. Because there are tons and tons of call stacks a program will have over time as it runs, and if we don't specify it then it is totally ambiguous.

@@ +330,5 @@
>  heapview.field.shallowSize.tooltip=The size of the object itself
>  
>  # LOCALIZATION NOTE (dominatortree.field.label): The name of the column in the
>  # dominator tree for an object's label.
>  dominatortree.field.label=Label

While you're here, would you mind changing this from "Label" to "Dominator"? I've been meaning to do this, but since you're already editing this file...

Thanks!

@@ +358,5 @@
>  heapview.field.totalbytes=Total Bytes
>  
>  # LOCALIZATION NOTE (heapview.field.totalbytes.tooltip): The tooltip for the
>  # column header in the heap view for total bytes.
> +heapview.field.totalbytes.tooltip=The number of bytes take up by this group, including subgroups

taken***

@@ +374,1 @@
>  heapview.field.name=Name

Should this be "Group" ?

@@ +378,5 @@
>  heapview.field.name.tooltip=The name of this group
>  
>  # LOCALIZATION NOTE (shortest-paths.header): The header label for the shortest
>  # paths pane.
> +shortest-paths.header=Retaining Paths in Garbage Collector

"Retaining Paths from Garbage Collector Roots" ?

Maybe just "Retaining Paths" ?

Comment 7

[Greg Tatum [:gregtatum]]

Attachment: User-facing terminology for memory tool

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #6)

> > +checkbox.recordAllocationStacks.tooltip=Toggle the recording of call stacks. Subsequent snapshots will be able to group and label object by call stacks, but only with those objects created after toggling this option. Recording call stacks has a performance overhead.
> 
> How about we add "when objects are allocated." to the end of the first
> sentence. I think it is important that we mention somewhere *which* call
> stacks get recorded. Because there are tons and tons of call stacks a
> program will have over time as it runs, and if we don't specify it then it
> is totally ambiguous.

Ok, I tweaked the wording a bit too.

> @@ +374,1 @@
> >  heapview.field.name=Name
> 
> Should this be "Group" ?

Yes, I think that makes a lot more semantic sense when looking at the report.

> @@ +378,5 @@
> >  heapview.field.name.tooltip=The name of this group
> >  
> >  # LOCALIZATION NOTE (shortest-paths.header): The header label for the shortest
> >  # paths pane.
> > +shortest-paths.header=Retaining Paths in Garbage Collector
> 
> "Retaining Paths from Garbage Collector Roots" ?
> 
> Maybe just "Retaining Paths" ?

I changed it to "Retaining Paths (from Garbage Collector Roots)". It's a long
title, but I think it helps convey the meaning of what you're looking at.

Comment 8

[Greg Tatum [:gregtatum]]

Let me know if there is any additional feedback that anyone has, otherwise I will go ahead and merge this in.

Comment 9

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

LGTM, we can always file more follow ups.

Comment 10

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

r=me

Comment 11

[Greg Tatum [:gregtatum]]

Should I always push to try for changes like this?

Comment 12

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

(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #11)
> Should I always push to try for changes like this?

It doesn't ever hurt, if you aren't 100% sure.

For things like this, I like to think about "what are the repercussions if/when things go wrong?" rather than "what is the obviously likely outcome?" and in this case the things that could go wrong are:

* Breaking some test, and needing to be backed out by a sheriff
* The tree is closed while waiting for the back out / investigation of why things went wrong and which commit to blame
* The tree closure is a drain on everyone else's productivity

When weighed against waiting a handful of hours to actually verify the likely outcome that nothing is broken with a try push, I think doing the try push and waiting wins. Waiting for a try push is only affecting one person's productivity (you) and you can likely figure out a way to keep busy while waiting for the try push results.

Finally, I have definitely closed the tree multiple times because "this is such a tiny, inconsequential patch and there is no way it will break anything and I don't need a try push". :P

Comment 13

[Greg Tatum [:gregtatum]]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #12)
> When weighed against waiting a handful of hours to actually verify the
> likely outcome that nothing is broken with a try push, I think doing the try
> push and waiting wins. Waiting for a try push is only affecting one person's
> productivity (you) and you can likely figure out a way to keep busy while
> waiting for the try push results.

Thanks! this is a very helpful way to frame the decision. Pushing to try it is.

Comment 14

[Greg Tatum [:gregtatum]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0f9d8d3ba66

Comment 15

[Greg Tatum [:gregtatum]]

Attachment: User-facing terminology for memory tool

This patch adds the missing review flag to the commit message.

Comment 16

[Carsten Book [:Tomcat]]

seems to have conflicts:

Hunk #1 FAILED at 23
1 out of 9 hunks FAILED -- saving rejects to file devtools/client/locales/en-US/memory.properties.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh Bug-1248988---User-facing-terminology-for-memory-t.patch

could you take a look ? Thanks !

Comment 17

[Greg Tatum [:gregtatum]]

Attachment: User-facing terminology for memory tool

Re-exported patch

Comment 18

[Greg Tatum [:gregtatum]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4148e47e8c2

Comment 19

[Pulsebot]

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

Comment 20

[Pulsebot]

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

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Had to push follow-up https://hg.mozilla.org/integration/fx-team/rev/e5962142ff0a to use apostroph and unicode ellipsis character in strings. That bc7 failure was visible in the Try run.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1aabbe445e73
https://hg.mozilla.org/mozilla-central/rev/e5962142ff0a

Comment 23

[Francesco Lodolo [:flod]]

This is not how you update existing strings
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

I'm going to file a follow-up bug, but at this point this change is going to slip into aurora and will be fixed in next cycle for localization.

Comment 24

[Chris Mills [:cmills]]

Hi!

I've updated the screenshots and descriptive text in the following articles to account for the new UI labels:

https://developer.mozilla.org/en-US/docs/Tools/Memory
https://developer.mozilla.org/en-US/docs/Tools/Memory/Basic_operations
https://developer.mozilla.org/en-US/docs/Tools/Memory/Dominators_view

I hope they look ok. I've not updated the videos, as I don't really have the capabilities to do that at this time.

I've also not added a note to the Firefox 49 release notes, as this is just new text labels, not new functionality or anything. Let me know if anyone disagrees with this decision.

Thanks.