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: |
|
Created attachment 124727 [details]
Patch
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.
It seems to clutter UI to me. 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 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 Created attachment 124757 [details]
Patch
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.
Created attachment 124884 [details]
Patch
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 on attachment 124884 [details]
Patch
r- as per loislo's comment.
Created attachment 124950 [details]
Patch
Comment on attachment 124950 [details]
Patch
looks good to me
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 Created attachment 125115 [details]
Patch
Comment on attachment 125115 [details] Patch Clearing flags on attachment: 125115 Committed r106633: <http://trac.webkit.org/changeset/106633> All reviewed patches have been landed. Closing bug. |
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.