Bug 77434 – Web Inspector: show percent along with absolute counts in heap snapshots

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Alexei Filippov]

Created attachment 124725 [details]
Sample screenshot

Make inspector show percents along with counts/sizes instead of switching between two.
The '%' button on the status bar should switch percents on/off.

[Alexei Filippov]

Created attachment 124727 [details]
Patch

[WebKit Review Bot]

Attachment 124727 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

It seems to clutter UI to me.

[Alexei Filippov]

1. There's quite a large unused space in inspector. So it makes sense to show the percents there without the need to switch back and forth.
2. The percents are shown in gray, so the main figures are easily distinguishable.
3. You can turn it off with the '%' button.

[WebKit Review Bot]

Comment on attachment 124727 [details]
Patch

Attachment 124727 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11369955

New failing tests:
inspector/profiler/detailed-heapshots-comparison-show-next.html
inspector/profiler/detailed-heapshots-summary-show-all.html
inspector/profiler/detailed-heapshots-comparison-show-all.html

[Alexei Filippov]

Created attachment 124757 [details]
Patch

[WebKit Review Bot]

Attachment 124757 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexei Filippov]

Created attachment 124884 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 124884 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124884&action=review

Looks good to me with nits.

> Source/WebCore/inspector/front-end/DetailedHeapshotView.js:942
> +        return this.showShallowSizeAsPercent && this.showRetainedSizeAsPercent;

Is it possible to have a single flag for percent?

> LayoutTests/inspector/profiler/detailed-heapshots-comparison-show-all.html:42
> +                countA = parseInt(row.data["addedCount"]);

I'd rather fix the expectations.

[Pavel Feldman]

Comment on attachment 124884 [details]
Patch

r- as per loislo's comment.

[Alexei Filippov]

Created attachment 124950 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 124950 [details]
Patch

looks good to me

[Pavel Feldman]

Comment on attachment 124950 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124950&action=review

> Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:297
> +        if (view.showPercents) {

nit: view._showPercentage

> Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:796
> +        data["sizeDelta"] = this._signForDelta(this._sizeDelta) + Number.withThousandsSeparator(Math.abs(this._sizeDelta));

Why did this change? You should prefer message format to the concatenation.

> Source/WebCore/inspector/front-end/DetailedHeapshotView.js:203
> +        count: { title: WebInspector.UIString("Objects Count"), width: "90px", sortable: true },

you should add this new string to the English.lproj/localizedStrings.js

> Source/WebCore/inspector/front-end/DetailedHeapshotView.js:299
> +        sizeDelta: { title: "Size Delta", width: "72px", sortable: true }

ditto

> Source/WebCore/inspector/front-end/DetailedHeapshotView.js:1094
> +            this.percentButton.title = WebInspector.UIString("Hide percentages of counts and sizes.");

ditto

> Source/WebCore/inspector/front-end/DetailedHeapshotView.js:1097
> +            this.percentButton.title = WebInspector.UIString("Show percentages of counts and sizes.");

ditto

[Alexei Filippov]

Created attachment 125115 [details]
Patch

[WebKit Review Bot]

Comment on attachment 125115 [details]
Patch

Clearing flags on attachment: 125115

Committed r106633: <http://trac.webkit.org/changeset/106633>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.