Currently diffs w/ negative values look like this: > ├──-1.61 MB (-10.15%) ++ compartment([System Principal], [anonymous sandbox] (from: chrome://marionette/content/marionette-server.js:848)) This is commonly misconstrued as increase (yes the percent is clearly negative, but people seem to focus on the raw value). I propose adding a space before the value to make the distinction clearer: > ├─ -1.61 MB (-10.15%) ++ compartment([System Principal], [anonymous sandbox] (from: chrome://marionette/content/marionette-server.js:848))

Just adding a space to negative values looks a little odd: │ │ ├────0.48 MB (-13.60%) ── image(407x305, ...)/locked/surface(407x305@0)/decoded-heap │ │ ├── -0.45 MB (12.72%) -- image(http://l...) │ │ │ ├── -0.41 MB (11.82%) ── decoded-heap [-] So we can see that positive values still have a '───' all the way up to the first digit and the alignment of the child bar (├) for child of a negative value lines up with the space which feels a little off to. It *is* clearer though. njn, any thoughts?

The '-' already throws the formatting off; the extra space would make it worse. How often has this problem come up in practice?

(In reply to Nicholas Nethercote [:njn] from comment #2) > The '-' already throws the formatting off; the extra space would make it > worse. How often has this problem come up in practice? It's a problem w/ most diffs. I've been confused myself and had to explain multiple times to others, most recently on 5/1/2015.

Could we make the negative a different color? Or make the measurement and everything to the right of it red when it is negative or something?

(In reply to Andrew McCreight [:mccr8] from comment #4) > Could we make the negative a different color? Or make the measurement and > everything to the right of it red when it is negative or something? That would certainly help in about:memory, but not so much when pasting elsewhere.

Braces can be added around the negative values something like this: ├────4.24 MB (01.02%) ++ workers/workers(chrome) └──{-1,341.71 MB} (-323.60%) ++ (17 tiny) [?!]

Nick, what do you think about varsha's idea? I suppose any kind of braces could work, ie: {-1,234.56} [-1,234.56] (-1,234.56) This corresponds well to the practice of putting parentheses around negative numbers in accounting.

Parentheses would be fine so long as the formatting is fixed to take account of the extra chars. That doesn't happen for the '-' sign, viz: > ├──-6.13 MB (-5.29%) ++ window(...) > ├──6.13 MB (05.29%) ++ window(...) > ├──-5.08 MB (-4.38%) ++ window(...) > ├──5.08 MB (04.38%) -- window(...) This would be even worse: > ├──(-6.13 MB) (-5.29%) ++ window(...) > ├──6.13 MB (05.29%) ++ window(...) > ├──(-5.08 MB) (-4.38%) ++ window(...) > ├──5.08 MB (04.38%) -- window(...) This would be the right way: > ├── -6.13 MB) (-5.29%) ++ window(...) > ├────6.13 MB (05.29%) ++ window(...) > ├── -5.08 MB) (-4.38%) ++ window(...) > ├────5.08 MB (04.38%) -- window(...) I suspect that putting a space before the '-' would be easier than using parentheses, because it wouldn't require any futzing with the spaces after the "MB".

what if we do something like this: ├────9.79 MB (01.59%) ++ add-ons ├────7.93 MB (01.29%) ++ storage └── (-3,376.84 MB) (-547.80%) -- (18 tiny) [?!] Or is the previous idea better?

(In reply to Nicholas Nethercote [:njn] from comment #8) > Parentheses would be fine so long as the formatting is fixed to take account > of > the extra chars. That doesn't happen for the '-' sign, viz: > > > ├──-6.13 MB (-5.29%) ++ window(...) > > ├──6.13 MB (05.29%) ++ window(...) > > ├──-5.08 MB (-4.38%) ++ window(...) > > ├──5.08 MB (04.38%) -- window(...) > > This would be even worse: > > > ├──(-6.13 MB) (-5.29%) ++ window(...) > > ├──6.13 MB (05.29%) ++ window(...) > > ├──(-5.08 MB) (-4.38%) ++ window(...) > > ├──5.08 MB (04.38%) -- window(...) > > This would be the right way: > > > ├── -6.13 MB) (-5.29%) ++ window(...) > > ├────6.13 MB (05.29%) ++ window(...) > > ├── -5.08 MB) (-4.38%) ++ window(...) > > ├────5.08 MB (04.38%) -- window(...) > > I suspect that putting a space before the '-' would be easier than using > parentheses, because it wouldn't require any futzing with the spaces after > the > "MB". Nick, so you're okay with the weird visual alignment noted in comment 1 that comes with just a space?

(In reply to varsha from comment #9) > what if we do something like this: > ├────9.79 MB (01.59%) ++ add-ons > ├────7.93 MB (01.29%) ++ storage > └── (-3,376.84 MB) (-547.80%) -- (18 tiny) [?!] > > Or is the previous idea better? :njn seems to be leaning towards using just a space (I think), lets just double-check.

Ideally the '├' character would still sit underneath the first digit of the number in the previous line, e.g.: │ │ ├──0.48 MB (-13.60%) ── image(407x305, ...)/locked/surface(407x305@0)/decoded-heap │ │ ├ -0.45 MB (12.72%) -- image(http://l...) │ │ │ ├ -0.41 MB (11.82%) ── decoded-heap [-] Though that also looks a bit weird. Maybe parentheses is the best after all: "(-3.4 MB)". I'm fine with that, so long as it's done in a way such that the digits line up vertically.

Comment on attachment 8743958 [details] [diff] [review] Adding parenthesis to the invalid values Review of attachment 8743958 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/aboutmemory/content/aboutMemory.js @@ +1932,5 @@ > } > + > + // The value. > + if (tIsInvalid) { > + valueText = "{" + valueText + "}"; I think what you need to do is modify |formatInt| [1] to add the parenthesis. As-is this only modifies the string in the invalid case, which in the next statement just gets replaced with "invalid". [1] https://dxr.mozilla.org/mozilla-central/rev/4feb4dd910a5a2d3061dbdd376a80975206819c6/toolkit/components/aboutmemory/content/aboutMemory.js#1622-1624

For your next version of the patch, could you please paste some example output? That makes understanding what you've done easier. Also, the NEEDINFO flag is for general questions. If you want feedback or a review done for a specific patch, you can use the feedback/review flags on the patch attachment.

The output is the following: Here both 0.20 MB and -0.10 MB are invalid values. 0.10 MB (100.0%) -- other ├──0.20 MB (200.00%) ── a [?!] └──{-0.10 MB} (-100.00%) ── b [?!]

(In reply to varsha from comment #16) > > 0.10 MB (100.0%) -- other > ├──0.20 MB (200.00%) ── a [?!] > └──{-0.10 MB} (-100.00%) ── b [?!] In comment 12 I said: > Maybe parentheses is the best after all: "(-3.4 MB)". I'm fine with that, so long as it's done in a way such that the digits > line up vertically. You haven't made them line up vertically. It should look something like this: > 0.10 MB (100.0%) -- other > ├────0.20 MB ( 200.00%) ── a [?!] > └──{-0.10 MB} (-100.00%) ── b [?!]

0.10 MB (100.0%) -- other ├─────0.20 MB ( 200.00%) ── a [?!] └──{-0.10 MB} (-100.00%) ── b [?!]

(In reply to Nicholas Nethercote [:njn] from comment #17) > (In reply to varsha from comment #16) > > > > 0.10 MB (100.0%) -- other > > ├──0.20 MB (200.00%) ── a [?!] > > └──{-0.10 MB} (-100.00%) ── b [?!] > > In comment 12 I said: > > > Maybe parentheses is the best after all: "(-3.4 MB)". I'm fine with that, so long as it's done in a way such that the digits > line up vertically. > > You haven't made them line up vertically. It should look something like this: > > > 0.10 MB (100.0%) -- other > > ├────0.20 MB ( 200.00%) ── a [?!] > > └──{-0.10 MB} (-100.00%) ── b [?!] Thanks for reminding,I have uploaded a new patch for it.

Comment on attachment 8750371 [details] [diff] [review] vertical alignment of numbers Review of attachment 8750371 [details] [diff] [review]: ----------------------------------------------------------------- Apologies for the slow response. (In reply to varsha from comment #18) > Created attachment 8750371 [details] [diff] [review] > vertical alignment of numbers > > 0.10 MB (100.0%) -- other > ├─────0.20 MB ( 200.00%) ── a [?!] > └──{-0.10 MB} (-100.00%) ── b [?!] This is better but the first numbers still don't line up. Something like this would be ideal: > 0.10 MB (100.0%) -- other > ├────0.20 MB ( 200.00%) ── a [?!] > └──{-0.10} MB (-100.00%) ── b [?!] As for the code in the patch, I'm afraid to say that it's falling well short of being suitable for inclusion. There are lots of stylistic problems, which I've discussed below, but more generally it just doesn't feel like a good way to address the problem. Sorry. ::: toolkit/components/aboutmemory/content/aboutMemory.js @@ +21,5 @@ > var Cc = Components.classes; > var Ci = Components.interfaces; > var Cu = Components.utils; > var CC = Components.Constructor; > +var num1; It's very ugly that you use a global variable like this to hold state between iterations in the function below. It's also a poor variable name, giving no indication what it's used for. @@ +1647,5 @@ > let mbytes = (aBytes / (1024 * 1024)).toFixed(2); > let a = String(mbytes).split("."); > // If the argument to formatInt() is -0, it will print the negative sign. > s = formatInt(Number(a[0])) + "." + a[1] + unit; > + let st=s.search("{"); Using string search to detect negativeness in the number is clumsy. Using hasNegativeSign() would be better. Also, you're adding the leading '{' in formatInt() but the trailing '}' in this function. Splitting it across two functions is ugly. You should do both in a single function, either this one or formatInt(). @@ +1650,5 @@ > s = formatInt(Number(a[0])) + "." + a[1] + unit; > + let st=s.search("{"); > + if(st==0){ > + s = formatInt(Number(a[0])) + "." + a[1] + unit+"}"; > + } This code needs more whitespace -- there should be spaces around all operators such as '=', '+', '=='. Also after 'if' and before '{'. This comment applies for all the changes in this patch. Please follow existing style as closely as possible. @@ +1895,5 @@ > aTreelineText2b = > appendN(aTreelineText2b, " ", extraTreelineLength); > } > let treelineText = aTreelineText1 + aTreelineText2a; > + //here was append This is not a useful comment. @@ +1910,5 @@ > reportAssertionFailure("Invalid value (" + aT._amount + " / " + > aRoot._amount + ") for " + > flipBackslashes(unsafePath)); > } > + if(aTreelineText2a =="├──" && tIsInvalid == true){ Hardcoding these strings here is not good. Elsewhere the constants like |kHorizontal| are used. @@ +1912,5 @@ > flipBackslashes(unsafePath)); > } > + if(aTreelineText2a =="├──" && tIsInvalid == true){ > + let i=0; > + for(i=0;i<3;i++){ Just use |for (let i = ...)| to scope |i| within the loop. Ditto with |j| in the loop below (though it can also be called |i|). @@ +1917,5 @@ > + treelineText+=kHorizontal; > + } > + num1=valueText.replace(/[^0-9]/g,"").length; > + } > + if(aTreelineText2a =="└──" && tIsInvalid == true) You can just write |&& tIsInvalid| instead of |&& tIsInvalid == true|. @@ +1921,5 @@ > + if(aTreelineText2a =="└──" && tIsInvalid == true) > + { > + let num2=valueText.replace(/[^0-9]/g,"").length; > + let j; > + for(j=0;j<num1-num2;j++){ This whole approach feels very brittle and hard to understand, like things are being patched up afterwards rather than being done properly. @@ +1922,5 @@ > + { > + let num2=valueText.replace(/[^0-9]/g,"").length; > + let j; > + for(j=0;j<num1-num2;j++){ > + treelineText+=kHorizontal; This loop body is not indented properly. @@ +1951,5 @@ > assert(!aT._hideKids, "leaf node with _hideKids set") > sep = kNoKidsSep; > d = aP; > } > + Trailing whitespace. @@ +1970,5 @@ > : (0 <= num && num < 10 ? " (0" : " (") + numText + "%)"; > + if(tIsInvalid == true && cnum>0){ > + percText = numText === " 100.00" > + ? " (100.0%)" > + : (0 <= num && num < 10 ? " ( 0" : " ( ") + numText + "%)"; Here you are overwriting the previously assigned value of |percText|. It would be better to check the condition first, and then assign the appropriate value depending on the condition. @@ +1976,5 @@ > + else if(tIsInvalid == true &&(cnum<0 && cnum<=-100)) > + { > + percText = numText === " 100.00" > + ? " (100.0%)" > + : (0 <= num && num < 10 ? " ( 0" : " (") + numText + "%)"; The code you've added to this function is highly repetitive. Repetitive code should be avoided by using abstractions such as functions. @@ +1977,5 @@ > + { > + percText = numText === " 100.00" > + ? " (100.0%)" > + : (0 <= num && num < 10 ? " ( 0" : " (") + numText + "%)"; > + } Trailing whitespace. @@ +1983,5 @@ > + else if(tIsInvalid == true && (cnum<0 && cnum>-100)){ > + percText = numText === " 100.00" > + ? " (100.0%)" > + : (0 <= num && num < 10 ? " ( 0" : " ( ") + numText + "%)"; > + } Trailing whitespace.

(In reply to Nicholas Nethercote [:njn] from comment #20) > Comment on attachment 8750371 [details] [diff] [review] > vertical alignment of numbers > > Review of attachment 8750371 [details] [diff] [review]: > ----------------------------------------------------------------- > > Apologies for the slow response. It's okay. > > Created attachment 8750371 [details] [diff] [review] > > vertical alignment of numbers > > > > 0.10 MB (100.0%) -- other > > ├─────0.20 MB ( 200.00%) ── a [?!] > > └──{-0.10 MB} (-100.00%) ── b [?!] > > This is better but the first numbers still don't line up. Something like > this would be ideal: > > > 0.10 MB (100.0%) -- other > > ├────0.20 MB ( 200.00%) ── a [?!] > > └──{-0.10} MB (-100.00%) ── b [?!] I will try to make it more properly aligned. > As for the code in the patch, I'm afraid to say that it's falling well short > of being suitable for inclusion. There are lots of stylistic problems, which > I've discussed below, but more generally it just doesn't feel like a good > way to address the problem. Sorry. > Thanks for your inputs and I will make the changes in the code. > ::: toolkit/components/aboutmemory/content/aboutMemory.js > @@ +21,5 @@ > > var Cc = Components.classes; > > var Ci = Components.interfaces; > > var Cu = Components.utils; > > var CC = Components.Constructor; > > +var num1; > > It's very ugly that you use a global variable like this to hold state > between iterations in the function below. It's also a poor variable name, > giving no indication what it's used for. > sorry.I forgot to add it.It is a variable used to store length of the previous value for alignment purpose. > @@ +1647,5 @@ > > let mbytes = (aBytes / (1024 * 1024)).toFixed(2); > > let a = String(mbytes).split("."); > > // If the argument to formatInt() is -0, it will print the negative sign. > > s = formatInt(Number(a[0])) + "." + a[1] + unit; > > + let st=s.search("{"); > > Using string search to detect negativeness in the number is clumsy. Using > hasNegativeSign() would be better. > But NegativeSign() can be used, in if condition when equated to +0 and -0 it will give same result. > Also, you're adding the leading '{' in formatInt() but the trailing '}' in > this function. Splitting it across two functions is ugly. You should do both > in a single function, either this one or formatInt(). > To detect both +0 and -0,I had to split it in different functions.I have appended '{' it with '-'. > @@ +1650,5 @@ > > s = formatInt(Number(a[0])) + "." + a[1] + unit; > > + let st=s.search("{"); > > + if(st==0){ > > + s = formatInt(Number(a[0])) + "." + a[1] + unit+"}"; > > + } > > This code needs more whitespace -- there should be spaces around all > operators such as '=', '+', '=='. Also after 'if' and before '{'. This > comment applies for all the changes in this patch. Please follow existing > style as closely as possible. > I will implement them in the next patch. > @@ +1895,5 @@ > > aTreelineText2b = > > appendN(aTreelineText2b, " ", extraTreelineLength); > > } > > let treelineText = aTreelineText1 + aTreelineText2a; > > + //here was append > > This is not a useful comment. > Yes,I will change it. > @@ +1910,5 @@ > > reportAssertionFailure("Invalid value (" + aT._amount + " / " + > > aRoot._amount + ") for " + > > flipBackslashes(unsafePath)); > > } > > + if(aTreelineText2a =="├──" && tIsInvalid == true){ > > Hardcoding these strings here is not good. Elsewhere the constants like > |kHorizontal| are used. > > @@ +1912,5 @@ > > flipBackslashes(unsafePath)); > > } > > + if(aTreelineText2a =="├──" && tIsInvalid == true){ > > + let i=0; > > + for(i=0;i<3;i++){ > > Just use |for (let i = ...)| to scope |i| within the loop. Ditto with |j| in > the loop below (though it can also be called |i|). > okay,I will modify them. > @@ +1917,5 @@ > > + treelineText+=kHorizontal; > > + } > > + num1=valueText.replace(/[^0-9]/g,"").length; > > + } > > + if(aTreelineText2a =="└──" && tIsInvalid == true) > > You can just write |&& tIsInvalid| instead of |&& tIsInvalid == true|. > I'll change it. > @@ +1921,5 @@ > > + if(aTreelineText2a =="└──" && tIsInvalid == true) > > + { > > + let num2=valueText.replace(/[^0-9]/g,"").length; > > + let j; > > + for(j=0;j<num1-num2;j++){ > > This whole approach feels very brittle and hard to understand, like things > are being patched up afterwards rather than being done properly. > > @@ +1922,5 @@ > > + { > > + let num2=valueText.replace(/[^0-9]/g,"").length; > > + let j; > > + for(j=0;j<num1-num2;j++){ > > + treelineText+=kHorizontal; > > This loop body is not indented properly. > I'll correct it. > @@ +1951,5 @@ > > assert(!aT._hideKids, "leaf node with _hideKids set") > > sep = kNoKidsSep; > > d = aP; > > } > > + > > Trailing whitespace. > > @@ +1970,5 @@ > > : (0 <= num && num < 10 ? " (0" : " (") + numText + "%)"; > > + if(tIsInvalid == true && cnum>0){ > > + percText = numText === " 100.00" > > + ? " (100.0%)" > > + : (0 <= num && num < 10 ? " ( 0" : " ( ") + numText + "%)"; > > Here you are overwriting the previously assigned value of |percText|. It > would be better to check the condition first, and then assign the > appropriate value depending on the condition. > I'll modify it. > @@ +1976,5 @@ > > + else if(tIsInvalid == true &&(cnum<0 && cnum<=-100)) > > + { > > + percText = numText === " 100.00" > > + ? " (100.0%)" > > + : (0 <= num && num < 10 ? " ( 0" : " (") + numText + "%)"; > > The code you've added to this function is highly repetitive. Repetitive code > should be avoided by using abstractions such as functions. > I will add appropriate function. > @@ +1977,5 @@ > > + { > > + percText = numText === " 100.00" > > + ? " (100.0%)" > > + : (0 <= num && num < 10 ? " ( 0" : " (") + numText + "%)"; > > + } > > Trailing whitespace. > > @@ +1983,5 @@ > > + else if(tIsInvalid == true && (cnum<0 && cnum>-100)){ > > + percText = numText === " 100.00" > > + ? " (100.0%)" > > + : (0 <= num && num < 10 ? " ( 0" : " ( ") + numText + "%)"; > > + } > > Trailing whitespace.

Comment on attachment 8747411 [details] [diff] [review] Adding parenthesis to negative values Clearing review request, njn covered it in the follow up patch.