Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2954973002:
DevTools -> Memory Tab -> Allowing to edit a title of a heap snapshot (Closed)
|
Created:
3 years, 5 months ago by diana.suvorova Modified:
3 years, 5 months ago Reviewers:
alph CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionHeap profile usability enhancement. Making it easier to navigate through multiple heap profiles.
BUG (rather feature)= 736776
Review-Url: https://codereview.chromium.org/2954973002
Cr-Commit-Position: refs/heads/master@{#485798}
Committed: https://chromium.googlesource.com/chromium/src/+/8130390255328a1d6e922d8cfd281304935f556b
Patch Set 1 #Patch Set 2 : q #Patch Set 3 : removing files that are not related to the PR #
Total comments: 13
Patch Set 4 : formatting and annotation #Patch Set 5 : 'updating heap snapshot view on title change' #
Total comments: 9
Patch Set 6 : preserving selection and upading all profile views #Patch Set 7 : updating toolbar on detail view switch #Patch Set 8 : just removing extra line #
Total comments: 14
Patch Set 9 : editing only title. bringing back objects allocated before #
Total comments: 8
Patch Set 10 : adding listeners to existing profiles #
Total comments: 2
Patch Set 11 : unnecessary code cleanup #Patch Set 12 : adding myself to authors #
Messages
Total messages: 38 (13 generated)
diana.suvorova@gmail.com changed reviewers: + pfeldman@chromium.org
The CQ bit was checked by diana.suvorova@gmail.com
The CQ bit was unchecked by diana.suvorova@gmail.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
This is my first PR for chromium project. Looking forward to your review! Thanks, Diana
alph@chromium.org changed reviewers: + alph@chromium.org - apavlov+blink@chromium.org, blink-reviews@chromium.org, caseq+blink@chromium.org, chromium-reviews@chromium.org, devtools-reviews@chromium.org, kozyatinskiy+blink@chromium.org, lushnikov+blink@chromium.org, pfeldman+blink@chromium.org, pfeldman@chromium.org
Hi Diana, Thank you for your contribution to DevTools codebase and congratulations with your first patch! Sorry about nobody replied on it so far. But please do not use +blink suffix in reviewers emails as they won't be able to see the patch in their review queues. Here are my comments to the patch. Most of them are minor nits though. Thanks again! https://codereview.chromium.org/2954973002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js (right): https://codereview.chromium.org/2954973002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:645: * @override the padding is off https://codereview.chromium.org/2954973002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:648: ondblclick(event) { please annotate the argument https://codereview.chromium.org/2954973002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:651: if (this._startEditing(/** @type {!Element} */ (event.target))) no need for an 'if' as it always returns false. https://codereview.chromium.org/2954973002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:656: remove the extra line https://codereview.chromium.org/2954973002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:657: _startEditing(eventTarget) { please add annotation. https://codereview.chromium.org/2954973002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:659: if (container) { we prefer early return to keep the code indents minimal, i.e.: if (!container) return; https://codereview.chromium.org/2954973002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:663: // this.listItemElement.getComponentSelection().selectAllChildren(textNodeElement); please do not add commented out code. https://codereview.chromium.org/2954973002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:667: _editingCommitted(container, element, newTitle) { annotate please https://codereview.chromium.org/2954973002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:670: this.profile.setTitle(newTitle); the third argument is in fact an oldTitle. https://codereview.chromium.org/2954973002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:674: _editingCancelled(element, context) { annotate please https://codereview.chromium.org/2954973002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:677: // Need to restore attributes structure. no need for a comment. https://codereview.chromium.org/2954973002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:681: updateTitle() { make it private, i.e. _updateTitle https://codereview.chromium.org/2954973002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:683: this._createSaveLink(); I wonder, why do you need to create a save link? It seems to be unrelated.
HI Alph, thanks a lot for your review! I corrected formatting and added annotations. As for your last comment: >>https://codereview.chromium.org/2954973002/diff/40001/third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js#newcode683 >>third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:683: this._createSaveLink(); >>I wonder, why do you need to create a save link? It seems to be unrelated. I believe I need to recreate save link since InplaceEditor overrides whole container which includes title string and the save link. Now I am realizing that one other view in Memory tab needs to be updated to reflect new profile title. It is a dropdown list on a snapshot picker when one choses to compare two snapshots. I am going to be looking into what is standard way to setup listeners on an attribute change. I will update this PR once I figure it out.
Now I think all the parts of dev tools react appropriately to a snapshot title change. Ready for re-review ! :)
Ready for re-review :) Corrected formatting and annotations; Made sure all parts of heap profiler UI react to the title change.
https://codereview.chromium.org/2954973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/profiler/HeapSnapshotView.js (right): https://codereview.chromium.org/2954973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/profiler/HeapSnapshotView.js:612: for (var i = 0; i < list.length; ++i) { nit: for (var item of list) https://codereview.chromium.org/2954973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/profiler/HeapSnapshotView.js:620: this._filterSelect.removeOptions(); This will kill the current selection, so the view content won't match the filter selection. We need to save/restore it. https://codereview.chromium.org/2954973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/profiler/HeapSnapshotView.js:627: title = Common.UIString('Objects allocated before %s', title); nit: You can now move the custom 0-th item out of the loop, and start looping with 1. https://codereview.chromium.org/2954973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/profiler/HeapSnapshotView.js:660: this._profile.profileType().removeEventListener( The listener had been set on the profile instance, not the type. Also you should be removing the listener for any profile header being removed (so match the addEventListener), not just the one corresponding to the current view. https://codereview.chromium.org/2954973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js (right): https://codereview.chromium.org/2954973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:298: nuke the extra line. https://codereview.chromium.org/2954973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:650: ondblclick(event) { annotate event plz https://codereview.chromium.org/2954973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:651: if (this._editing) if (!this._editing) this._startEditing(); return false; https://codereview.chromium.org/2954973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:661: var container = eventTarget.enclosingNodeOrSelfWithClass('title-container'); Can you create the editor on 'title' element, not on 'title-container' to keep save-link unaffected? https://codereview.chromium.org/2954973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:674: _editingCommitted(container, newTitle, oldTitle) { nit: just drop oldTitle if it's not used.
I believe this should address the comments.
Hi Diana, Looks like you missed couple comments. I'll repeat them. Thank you! https://codereview.chromium.org/2954973002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/profiler/HeapSnapshotView.js (right): https://codereview.chromium.org/2954973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/profiler/HeapSnapshotView.js:254: this._updateControls(); I wonder, why do you need this? https://codereview.chromium.org/2954973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/profiler/HeapSnapshotView.js:616: this._baseSelect.createOption(title); nit: Just inline the title: this._baseSelect.createOption(item.title); https://codereview.chromium.org/2954973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/profiler/HeapSnapshotView.js:628: for (var i = 1; i < list.length; ++i) { We lost "Objects allocated before ..." option. https://codereview.chromium.org/2954973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/profiler/HeapSnapshotView.js:629: var title = list[i].title; nit: just inline the title into the line below. https://codereview.chromium.org/2954973002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js (right): https://codereview.chromium.org/2954973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:646: * @override @param {!Event} event https://codereview.chromium.org/2954973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:660: var container = eventTarget.enclosingNodeOrSelfWithClass('title-container'); Can you please attach the editor to the title element, instead of title-container, so it won't affect the save link. https://codereview.chromium.org/2954973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:673: _editingCommitted(container, newTitle, oldTitle) { you can drop oldTitle args as it's not used.
The CQ bit was checked by diana.suvorova@gmail.com
Thanks a lot for repeating the comments I missed.Should be addressed in most recent patch. It takes me some time to get used to new code review ui. -Diana https://codereview.chromium.org/2954973002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/profiler/HeapSnapshotView.js (right): https://codereview.chromium.org/2954973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/profiler/HeapSnapshotView.js:254: this._updateControls(); On 2017/07/10 at 21:16:30, alph wrote: > I wonder, why do you need this? Here is the scenario that I need it for: - create 2 snapshots - rename first snapshot - switch to a second snapshot - see the base and filter options - without this line they do not have a new name for the first snapshot... I would think that the listeners on profile title should work for all profiles (not only currently selected one), but it doesn't seem to be the case. I naturally might be missing something. Let me know if you have better ideas.. https://codereview.chromium.org/2954973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/profiler/HeapSnapshotView.js:616: this._baseSelect.createOption(title); On 2017/07/10 at 21:16:30, alph wrote: > nit: Just inline the title: this._baseSelect.createOption(item.title); updated. https://codereview.chromium.org/2954973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/profiler/HeapSnapshotView.js:628: for (var i = 1; i < list.length; ++i) { On 2017/07/10 at 21:16:30, alph wrote: > We lost "Objects allocated before ..." option. yes, thanks, should be back https://codereview.chromium.org/2954973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/profiler/HeapSnapshotView.js:629: var title = list[i].title; On 2017/07/10 at 21:16:30, alph wrote: > nit: just inline the title into the line below. updated! https://codereview.chromium.org/2954973002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js (right): https://codereview.chromium.org/2954973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:646: * @override On 2017/07/10 at 21:16:30, alph wrote: > @param {!Event} event thanks! https://codereview.chromium.org/2954973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:660: var container = eventTarget.enclosingNodeOrSelfWithClass('title-container'); On 2017/07/10 at 21:16:30, alph wrote: > Can you please attach the editor to the title element, instead of title-container, so it won't affect the save link. Yes, thanks for suggesting this. It is much cleaner this way. https://codereview.chromium.org/2954973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:673: _editingCommitted(container, newTitle, oldTitle) { On 2017/07/10 at 21:16:31, alph wrote: > you can drop oldTitle args as it's not used. gone
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/2954973002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/profiler/HeapSnapshotView.js (right): https://codereview.chromium.org/2954973002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/profiler/HeapSnapshotView.js:254: this._updateControls(); Oh, got it. Thanks! So the title updates for older profiles do not come to the HeapSnapshotView's created later, as they do not subscribe to these (because the newer views do not get _onReceiveSnapshot called for already existing snapshots). This is a bit awkward that a view have listeners for title change for newer profiles, but not for older ones. I see two options to make it consistent: 1. Keep this _updateControls call, but remove the listener for title change on the view for foreign profiles. E.g add a check: if (profile == this._profile) profile.addEventlistener(...); to the _onReceiveSnapshot 2. Remove this _updateControls call, and add something like: for (var existingProfile of this._profiles()) { existingProfile.addEventListener( Profiler.ProfileHeader.Events.ProfileTitleChanged, this._updateControls, this); } to the HeapSnapshotView constructor. To make sure the view subscribes to title change for all the profiles. https://codereview.chromium.org/2954973002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js (right): https://codereview.chromium.org/2954973002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:651: if (this._editing) nit: if (!this._editing) this._startEditing(/** @type {!Element} */ (event.target)); return false; https://codereview.chromium.org/2954973002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:664: container.textContent = this.profile.title; seems to be not needed anymore. https://codereview.chromium.org/2954973002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:684: this._titleElement.textContent = this.profile.title; Now as you editing the title element directly, the editor should take care of updating the textContent. So you don't need to update it.
ok, thanks a bunch for your help. one more patch. https://codereview.chromium.org/2954973002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/profiler/HeapSnapshotView.js (right): https://codereview.chromium.org/2954973002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/profiler/HeapSnapshotView.js:254: this._updateControls(); On 2017/07/11 at 18:44:23, alph wrote: > Oh, got it. Thanks! > > So the title updates for older profiles do not come to the HeapSnapshotView's created later, as they do not subscribe to these (because the newer views do not get _onReceiveSnapshot called for already existing snapshots). This is a bit awkward that a view have listeners for title change for newer profiles, but not for older ones. I see two options to make it consistent: > > 1. > Keep this _updateControls call, but remove the listener for title change on the view for foreign profiles. E.g add a check: if (profile == this._profile) profile.addEventlistener(...); > to the _onReceiveSnapshot > > 2. > Remove this _updateControls call, and add something like: > > for (var existingProfile of this._profiles()) { > existingProfile.addEventListener( > Profiler.ProfileHeader.Events.ProfileTitleChanged, this._updateControls, this); > } > > to the HeapSnapshotView constructor. To make sure the view subscribes to title change for all the profiles. I see. This makes sense. I went with option #2. https://codereview.chromium.org/2954973002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js (right): https://codereview.chromium.org/2954973002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:651: if (this._editing) On 2017/07/11 at 18:44:23, alph wrote: > nit: > if (!this._editing) > this._startEditing(/** @type {!Element} */ (event.target)); > return false; updated https://codereview.chromium.org/2954973002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:664: container.textContent = this.profile.title; On 2017/07/11 at 18:44:23, alph wrote: > seems to be not needed anymore. true, removed https://codereview.chromium.org/2954973002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:684: this._titleElement.textContent = this.profile.title; On 2017/07/11 at 18:44:23, alph wrote: > Now as you editing the title element directly, the editor should take care of updating the textContent. So you don't need to update it. Thanks, gone
Great! One more comment below. Also before we can commit the patch you need to do some legal stuff. This needs to be done just once. 1. Sign CLA here https://cla.developers.google.com/clas 2. Add yourself to src/AUTHORS file https://codereview.chromium.org/2954973002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js (right): https://codereview.chromium.org/2954973002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:681: this._titleElement.textContent = this.profile.title; You still do not need this. The event is coming from setTitle which is called by editingCommited that already has the title updated.
SLA signed; added myself to authors; Removed unnecessary code. https://codereview.chromium.org/2954973002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js (right): https://codereview.chromium.org/2954973002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/profiler/ProfilesPanel.js:681: this._titleElement.textContent = this.profile.title; On 2017/07/11 at 21:24:58, alph wrote: > You still do not need this. The event is coming from setTitle which is called by editingCommited that already has the title updated. This is great! ProfileTitleChanged listener is gone as well.
lgtm Let's land it. Спасибо!
The CQ bit was checked by alph@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by alph@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1499823112913460,
"parent_rev": "b508c72d8fdcc8879989a63d31c2d639009b3d48", "commit_rev":
"8130390255328a1d6e922d8cfd281304935f556b"}
Message was sent while issue was closed.
Description was changed from ========== Heap profile usability enhancement. Making it easier to navigate through multiple heap profiles. BUG (rather feature)= 736776 ========== to ========== Heap profile usability enhancement. Making it easier to navigate through multiple heap profiles. BUG (rather feature)= 736776 Review-Url: https://codereview.chromium.org/2954973002 Cr-Commit-Position: refs/heads/master@{#485798} Committed: https://chromium.googlesource.com/chromium/src/+/8130390255328a1d6e922d8cfd28... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/8130390255328a1d6e922d8cfd28...
Message was sent while issue was closed.
On 2017/07/11 at 22:51:47, alph wrote: > lgtm > Let's land it. > > Спасибо! Спасибо Вам!!! I still can't believe it. I thought only gods can contribute to chromium....
Message was sent while issue was closed.
On 2017/07/12 05:45:03, diana.suvorova wrote: > On 2017/07/11 at 22:51:47, alph wrote: > > lgtm > > Let's land it. > > > > Спасибо! > > Спасибо Вам!!! I still can't believe it. I thought only gods can contribute to > chromium.... That's true. Welcome to the pantheon. ;-) And congratulations with your first patch! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
