Closed Bug 1248988 Opened 4 years ago Closed 4 years ago

User-facing terminology for memory tool

Categories

(DevTools :: Memory, defect, P1)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: gregtatum, Assigned: gregtatum)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 3 obsolete files)

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".
Blocks: 1247696
Has STR: --- → irrelevant
Priority: -- → P1
Greg, do you think you can take this bug?
Flags: needinfo?(gtatum)
Assignee: nobody → gtatum
Flags: needinfo?(gtatum)
Depends on: 1249779
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
Is it ok to mark this as resolved? I believe Bug #1249779 resolved this issue. Any other thoughts on it?
(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?
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 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" ?
Attachment #8737375 - Flags: feedback+
(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.
Attachment #8737375 - Attachment is obsolete: true
Let me know if there is any additional feedback that anyone has, otherwise I will go ahead and merge this in.
LGTM, we can always file more follow ups.
Should I always push to try for changes like this?
(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
(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.
This patch adds the missing review flag to the commit message.
Attachment #8737855 - Attachment is obsolete: true
Keywords: checkin-needed
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 !
Flags: needinfo?(gtatum)
Keywords: checkin-needed
Re-exported patch
Attachment #8741483 - Attachment is obsolete: true
Flags: needinfo?(gtatum)
Keywords: checkin-needed
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.
https://hg.mozilla.org/mozilla-central/rev/1aabbe445e73
https://hg.mozilla.org/mozilla-central/rev/e5962142ff0a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
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.
Depends on: 1267215
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.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.