Bug 77434

Summary: Web Inspector: show percent along with absolute counts in heap snapshots
Product: WebKit Reporter: Alexei Filippov <alph>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, dglazkov, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 78411    
Attachments:
Description Flags
Sample screenshot
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Alexei Filippov 2012-01-31 07:05:02 PST
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.
Comment 1 Alexei Filippov 2012-01-31 07:16:03 PST
Created attachment 124727 [details]
Patch
Comment 2 WebKit Review Bot 2012-01-31 07:18:51 PST
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.
Comment 3 Pavel Feldman 2012-01-31 07:27:27 PST
It seems to clutter UI to me.
Comment 4 Alexei Filippov 2012-01-31 07:34:38 PST
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.
Comment 5 WebKit Review Bot 2012-01-31 07:50:18 PST
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
Comment 6 Alexei Filippov 2012-01-31 09:17:01 PST
Created attachment 124757 [details]
Patch
Comment 7 WebKit Review Bot 2012-01-31 09:18:57 PST
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.
Comment 8 Alexei Filippov 2012-01-31 22:10:48 PST
Created attachment 124884 [details]
Patch
Comment 9 Ilya Tikhonovsky 2012-02-01 04:33:23 PST
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.
Comment 10 Pavel Feldman 2012-02-01 04:39:17 PST
Comment on attachment 124884 [details]
Patch

r- as per loislo's comment.
Comment 11 Alexei Filippov 2012-02-01 07:38:17 PST
Created attachment 124950 [details]
Patch
Comment 12 Ilya Tikhonovsky 2012-02-01 12:40:22 PST
Comment on attachment 124950 [details]
Patch

looks good to me
Comment 13 Pavel Feldman 2012-02-02 04:12:01 PST
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
Comment 14 Alexei Filippov 2012-02-02 04:47:37 PST
Created attachment 125115 [details]
Patch
Comment 15 WebKit Review Bot 2012-02-02 23:37:23 PST
Comment on attachment 125115 [details]
Patch

Clearing flags on attachment: 125115

Committed r106633: <http://trac.webkit.org/changeset/106633>
Comment 16 WebKit Review Bot 2012-02-02 23:37:29 PST
All reviewed patches have been landed.  Closing bug.