DevTools Heap profiler: implement sorting.

DevTools Heap profiler: implement sorting.

BUG=26319
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30712

[apavlov, 2009-11-02 17:14:46]

LGTM with comments

http://codereview.chromium.org/342076/diff/1/2
File webkit/glue/devtools/js/heap_profiler_panel.js (right):

http://codereview.chromium.org/342076/diff/1/2#newcode169
Line 169: var operations = { LESS: function (a, b) { return a !== null && a < b;
},
can "a" be undefined in these cases?

http://codereview.chromium.org/342076/diff/1/2#newcode177
Line 177: if (query.indexOf(">") === 0) {
just a suggestion: you could use regexps to parse the entire query quickly. I
can help you come up with one if needed.

http://codereview.chromium.org/342076/diff/1/2#newcode196
Line 196: var megaBytesUnits = (query.length > 2 &&
query.toUpperCase().lastIndexOf("MB") === (query.length - 2));
You could convert query to the upper case somewhere in the beginning for these
"units"

http://codereview.chromium.org/342076/diff/1/2#newcode227
Line 227: {
The brace should go on the previous line

http://codereview.chromium.org/342076/diff/1/2#newcode245
Line 245: if (matchesQuery(current)) {
no braces necessary

http://codereview.chromium.org/342076/diff/1/2#newcode285
Line 285: // Then perform the search again the with same query and callback.
"the with" -> "with the"

http://codereview.chromium.org/342076/diff/1/2#newcode342
Line 342: if (this.snapshotDataGridList) {
no braces necessary

http://codereview.chromium.org/342076/diff/1/2#newcode574
Line 574: for (var i = 0, n = this.children.length; i < n; ++i) {
no braces necessary

http://codereview.chromium.org/342076/diff/1/2#newcode584
Line 584: for (var i = 0, n = this.children.length; i < n; ++i) {
no braces necessary

http://codereview.chromium.org/342076/diff/1/2#newcode639
Line 639: } else {
no braces necessary

http://codereview.chromium.org/342076/diff/1/2#newcode653
Line 653: } else {
no braces necessary

http://codereview.chromium.org/342076/diff/1/2#newcode665
Line 665: (columnIdentifier === "count" && this._searchMatchedCountColumn) ||
I believe we should indent the continuation lines 8 spaces instead of 4,
otherwise the body is lost in the condition.

http://codereview.chromium.org/342076/diff/1/2#newcode744
Line 744: var propertyHash = property + '#' + property2;
I believe this method will fail for the property called 'null'... Feel free to
leave as is if it's not an issue.

http://codereview.chromium.org/342076/diff/1/2#newcode749
Line 749: if ((l === null || r === null) && property2 !== null) {
No braces necessary

[mnaganov (inactive), 2009-11-02 17:53:28]

Thanks, Alexander! Your comments are very neat.

http://codereview.chromium.org/342076/diff/1/2
File webkit/glue/devtools/js/heap_profiler_panel.js (right):

http://codereview.chromium.org/342076/diff/1/2#newcode169
Line 169: var operations = { LESS: function (a, b) { return a !== null && a < b;
},
On 2009/11/02 17:14:46, apavlov wrote:
> can "a" be undefined in these cases?

No, the properties retrieved are always exist.

http://codereview.chromium.org/342076/diff/1/2#newcode177
Line 177: if (query.indexOf(">") === 0) {
On 2009/11/02 17:14:46, apavlov wrote:
> just a suggestion: you could use regexps to parse the entire query quickly. I
> can help you come up with one if needed.

Nice idea! Frankly, I copied this code (with modifications) from WebKit. OK,
refactored to use regexps.

http://codereview.chromium.org/342076/diff/1/2#newcode196
Line 196: var megaBytesUnits = (query.length > 2 &&
query.toUpperCase().lastIndexOf("MB") === (query.length - 2));
On 2009/11/02 17:14:46, apavlov wrote:
> You could convert query to the upper case somewhere in the beginning for these
> "units"

With regexps it's no more a problem, just using a case-insensitive regexp.

http://codereview.chromium.org/342076/diff/1/2#newcode227
Line 227: {
On 2009/11/02 17:14:46, apavlov wrote:
> The brace should go on the previous line

Done.

http://codereview.chromium.org/342076/diff/1/2#newcode245
Line 245: if (matchesQuery(current)) {
On 2009/11/02 17:14:46, apavlov wrote:
> no braces necessary

Done.

http://codereview.chromium.org/342076/diff/1/2#newcode285
Line 285: // Then perform the search again the with same query and callback.
On 2009/11/02 17:14:46, apavlov wrote:
> "the with" -> "with the"

Done.

http://codereview.chromium.org/342076/diff/1/2#newcode342
Line 342: if (this.snapshotDataGridList) {
On 2009/11/02 17:14:46, apavlov wrote:
> no braces necessary

Done.

http://codereview.chromium.org/342076/diff/1/2#newcode574
Line 574: for (var i = 0, n = this.children.length; i < n; ++i) {
On 2009/11/02 17:14:46, apavlov wrote:
> no braces necessary

Done.

http://codereview.chromium.org/342076/diff/1/2#newcode584
Line 584: for (var i = 0, n = this.children.length; i < n; ++i) {
On 2009/11/02 17:14:46, apavlov wrote:
> no braces necessary

Done.

http://codereview.chromium.org/342076/diff/1/2#newcode639
Line 639: } else {
On 2009/11/02 17:14:46, apavlov wrote:
> no braces necessary

Done.

http://codereview.chromium.org/342076/diff/1/2#newcode653
Line 653: } else {
On 2009/11/02 17:14:46, apavlov wrote:
> no braces necessary

Done.

http://codereview.chromium.org/342076/diff/1/2#newcode665
Line 665: (columnIdentifier === "count" && this._searchMatchedCountColumn) ||
On 2009/11/02 17:14:46, apavlov wrote:
> I believe we should indent the continuation lines 8 spaces instead of 4,
> otherwise the body is lost in the condition.

Look at the following snippet from style guide:

if (attr->name() == srcAttr
    || attr->name() == lowsrcAttr
    || (attr->name() == usemapAttr && attr->value().domString()[0] != '#'))
    return;

They use the same indentation for both condition and code. The only difference
is that I put '||' on the previous line, but I think this is preferred for JS,
because interpreter can add a semicolon at EOL.

For me, adding a '{' on the same column as 'if' word would make it more
readable, but it is also against the style.

http://codereview.chromium.org/342076/diff/1/2#newcode744
Line 744: var propertyHash = property + '#' + property2;
On 2009/11/02 17:14:46, apavlov wrote:
> I believe this method will fail for the property called 'null'... Feel free to
> leave as is if it's not an issue.

No, this isn't an issue. I'm not writing a library, after all.

http://codereview.chromium.org/342076/diff/1/2#newcode749
Line 749: if ((l === null || r === null) && property2 !== null) {
On 2009/11/02 17:14:46, apavlov wrote:
> No braces necessary

Done.