Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2023283002:
[polymer] Rewrite the analysis tab view (Closed)
|
Created:
4 years, 6 months ago by charliea (OOO until 10-5) Modified:
4 years, 6 months ago CC:
catapult-reviews_chromium.org, tracing-review_chromium.org Base URL:
git@github.com:zeptonaut/catapult.git@polymer10_rewrite_tab_view2 Target Ref:
refs/heads/polymer10-migration Project:
catapult Visibility:
Public. |
Description[polymer] Rewrite the analysis tab view
The analysis tab view has been creating all sorts of problems for the
polymer migration due to some fairly complex update logic that it uses.
This rewrite aims to simplify that logic with a simpler interface and
implementation.
The changed interface required several changes to the analysis view
itself. Most of these changes should be no-ops, with two notable
exceptions:
1) We now create new subviews each time that we need them instead of
caching subviews when they aren't in use. This came mostly from the
fact that I couldn't think of any real advantages of caching them,
as they aren't expensive to create in the first place.
2) We no longer save the tab scroll position. This seems like
something that, if we find we need, we can add back in without too
much hassle.
BUG=catapult:#2285
NOPRESUBMIT=true
NOTRY=true
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/f4e0af7e64128c86c5576b9534d8052f7e054dca
Patch Set 1 : #
Total comments: 27
Patch Set 2 : Code review #
Total comments: 3
Created: 4 years, 6 months ago
(Patch set is too large to download)
Messages
Total messages: 21 (12 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
charliea@chromium.org changed reviewers: + petrcermak@chromium.org
https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/anal... File tracing/tracing/ui/analysis/alert_sub_view.html (right): https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/anal... tracing/tracing/ui/analysis/alert_sub_view.html:9: <link rel="import" href="/tracing/ui/analysis/analysis_link.html"> found that this import line was missing when I went to use the alert sub view in the tab_view_test
LGTM with a couple of comments once Annie and/or Kari are happy. I suspect we might need to add code for remembering the current scroll position later though. Thanks, Petr https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/anal... File tracing/tracing/ui/analysis/analysis_view.html (right): https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/anal... tracing/tracing/ui/analysis/analysis_view.html:83: if (!numEvents) I'd prefer checking numEvents === 0. Alternatively, you could use a switch statement: switch (numEvents) { case 0: ... ... } https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/anal... tracing/tracing/ui/analysis/analysis_view.html:91: * Returns the tab label for the analysis sub view associated with the nit: I think it should be "sub-view" everywhere (hyphen) https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/anal... tracing/tracing/ui/analysis/analysis_view.html:122: // in them) that are instantiated inherit from HTMLElement. Unregistered s/instantiated inherit from/instantiated from/ ? https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/anal... tracing/tracing/ui/analysis/analysis_view.html:175: // last, so we have to refresh the subview. nit: I think it should be "the last ONE" https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/anal... tracing/tracing/ui/analysis/analysis_view.html:194: for (var eventTypeName in eventsByBaseTypeName) { maybe tr.b.iterItems? https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/anal... tracing/tracing/ui/analysis/analysis_view.html:202: var brushingStateController = this.brushingStateController_; It seems like you're not using this ;-) https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/base... File tracing/tracing/ui/base/tab_view.html (right): https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/tab_view.html:3: Copyright 2016 The Chromium Authors. All rights reserved. I don't think you should change this. https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/tab_view.html:9: @fileoverview A series of tabs for the analysis view that control which analysis I think it should be "controlS" because it's "A series" (I know it's kind of weird since "serieS" is plural). https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/tab_view.html:10: sub view is being display. s/display/displayed/ https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/tab_view.html:138: isSelected_: function(subView, selectedSubView) { Do you need this method? https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/base... File tracing/tracing/ui/base/tab_view_test.html (right): https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/tab_view_test.html:3: Copyright 2016 The Chromium Authors. All rights reserved. ditto: I don't think you should change this. https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/tab_view_test.html:33: var model = new tr.Model(); I suggest you create a model via tr.c.TestUtils.newModel instead since it takes care of finalization etc.
aiolos@chromium.org changed reviewers: + aiolos@chromium.org
https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/base... File tracing/tracing/ui/base/tab_view.html (right): https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/tab_view.html:109: this.$.subView.removeChild(this.selectedSubView_); Is there a reason you aren't using Polymer.dom in this section?
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:70001) has been deleted
Patchset #2 (id:80001) has been deleted
https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/anal... File tracing/tracing/ui/analysis/analysis_view.html (right): https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/anal... tracing/tracing/ui/analysis/analysis_view.html:83: if (!numEvents) On 2016/06/01 16:47:09, petrcermak wrote: > I'd prefer checking numEvents === 0. Alternatively, you could use a switch > statement: > > switch (numEvents) { > case 0: > ... > ... > } Done. https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/anal... tracing/tracing/ui/analysis/analysis_view.html:91: * Returns the tab label for the analysis sub view associated with the On 2016/06/01 16:47:09, petrcermak wrote: > nit: I think it should be "sub-view" everywhere (hyphen) Done. https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/anal... tracing/tracing/ui/analysis/analysis_view.html:122: // in them) that are instantiated inherit from HTMLElement. Unregistered On 2016/06/01 16:47:09, petrcermak wrote: > s/instantiated inherit from/instantiated from/ ? Done. https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/anal... tracing/tracing/ui/analysis/analysis_view.html:175: // last, so we have to refresh the subview. On 2016/06/01 16:47:09, petrcermak wrote: > nit: I think it should be "the last ONE" Done. https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/anal... tracing/tracing/ui/analysis/analysis_view.html:194: for (var eventTypeName in eventsByBaseTypeName) { On 2016/06/01 16:47:09, petrcermak wrote: > maybe tr.b.iterItems? Done. https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/anal... tracing/tracing/ui/analysis/analysis_view.html:202: var brushingStateController = this.brushingStateController_; On 2016/06/01 16:47:09, petrcermak wrote: > It seems like you're not using this ;-) Ah, thanks :-) Refactored maybeChangeRelatedEvents into its own method and never removed this afterwards https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/base... File tracing/tracing/ui/base/tab_view.html (right): https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/tab_view.html:3: Copyright 2016 The Chromium Authors. All rights reserved. On 2016/06/01 16:47:09, petrcermak wrote: > I don't think you should change this. Ah, sorry. I originally wrote this as tab_view2 with a new filename and header, then copied over this one, so I messed up the header. https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/tab_view.html:9: @fileoverview A series of tabs for the analysis view that control which analysis On 2016/06/01 16:47:09, petrcermak wrote: > I think it should be "controlS" because it's "A series" (I know it's kind of > weird since "serieS" is plural). Doh. You're right. https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/tab_view.html:10: sub view is being display. On 2016/06/01 16:47:09, petrcermak wrote: > s/display/displayed/ :-( sloppy https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/tab_view.html:109: this.$.subView.removeChild(this.selectedSubView_); On 2016/06/01 16:47:37, aiolos wrote: > Is there a reason you aren't using Polymer.dom in this section? Ack, no. For some reason, I was thinking it wasn't necessary with this.$..., but the docs seem to indicate that it is. https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/tab_view.html:138: isSelected_: function(subView, selectedSubView) { On 2016/06/01 16:47:09, petrcermak wrote: > Do you need this method? Doh. No. It's a duplicate of isChecked_ https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/base... File tracing/tracing/ui/base/tab_view_test.html (right): https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/tab_view_test.html:3: Copyright 2016 The Chromium Authors. All rights reserved. On 2016/06/01 16:47:10, petrcermak wrote: > ditto: I don't think you should change this. Done. https://codereview.chromium.org/2023283002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/tab_view_test.html:33: var model = new tr.Model(); On 2016/06/01 16:47:10, petrcermak wrote: > I suggest you create a model via tr.c.TestUtils.newModel instead since it takes > care of finalization etc. Done.
lgtm https://codereview.chromium.org/2023283002/diff/90001/tracing/tracing/ui/base... File tracing/tracing/ui/base/tab_view.html (right): https://codereview.chromium.org/2023283002/diff/90001/tracing/tracing/ui/base... tracing/tracing/ui/base/tab_view.html:115: You *might* need to flush the dom here, depending on how quickly this.$.subView is accessed afterward.
https://codereview.chromium.org/2023283002/diff/90001/tracing/tracing/ui/base... File tracing/tracing/ui/base/tab_view.html (right): https://codereview.chromium.org/2023283002/diff/90001/tracing/tracing/ui/base... tracing/tracing/ui/base/tab_view.html:115: On 2016/06/01 17:40:46, aiolos wrote: > You *might* need to flush the dom here, depending on how quickly this.$.subView > is accessed afterward. Acknowledged. I think my preference here is to only flush it if I find that for some reason it's necessary.
Description was changed from
==========
[polymer] Rewrite the analysis tab view
The analysis tab view has been creating all sorts of problems for the
polymer migration due to some fairly complex update logic that it uses.
This rewrite aims to simplify that logic with a simpler interface and
implementation.
The changed interface required several changes to the analysis view
itself. Most of these changes should be no-ops, with two notable
exceptions:
1) We now create new subviews each time that we need them instead of
caching subviews when they aren't in use. This came mostly from the
fact that I couldn't think of any real advantages of caching them,
as they aren't expensive to create in the first place.
2) We no longer save the tab scroll position. This seems like
something that, if we find we need, we can add back in without too
much hassle.
BUG=catapult:#2287
==========
to
==========
[polymer] Rewrite the analysis tab view
The analysis tab view has been creating all sorts of problems for the
polymer migration due to some fairly complex update logic that it uses.
This rewrite aims to simplify that logic with a simpler interface and
implementation.
The changed interface required several changes to the analysis view
itself. Most of these changes should be no-ops, with two notable
exceptions:
1) We now create new subviews each time that we need them instead of
caching subviews when they aren't in use. This came mostly from the
fact that I couldn't think of any real advantages of caching them,
as they aren't expensive to create in the first place.
2) We no longer save the tab scroll position. This seems like
something that, if we find we need, we can add back in without too
much hassle.
BUG=catapult:#2287
NOPRESUBMIT=true
NOTRY=true
==========
The CQ bit was checked by charliea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/2023283002/#ps90001 (title: "Code review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2023283002/90001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2023283002/90001
Message was sent while issue was closed.
Description was changed from
==========
[polymer] Rewrite the analysis tab view
The analysis tab view has been creating all sorts of problems for the
polymer migration due to some fairly complex update logic that it uses.
This rewrite aims to simplify that logic with a simpler interface and
implementation.
The changed interface required several changes to the analysis view
itself. Most of these changes should be no-ops, with two notable
exceptions:
1) We now create new subviews each time that we need them instead of
caching subviews when they aren't in use. This came mostly from the
fact that I couldn't think of any real advantages of caching them,
as they aren't expensive to create in the first place.
2) We no longer save the tab scroll position. This seems like
something that, if we find we need, we can add back in without too
much hassle.
BUG=catapult:#2287
NOPRESUBMIT=true
NOTRY=true
==========
to
==========
[polymer] Rewrite the analysis tab view
The analysis tab view has been creating all sorts of problems for the
polymer migration due to some fairly complex update logic that it uses.
This rewrite aims to simplify that logic with a simpler interface and
implementation.
The changed interface required several changes to the analysis view
itself. Most of these changes should be no-ops, with two notable
exceptions:
1) We now create new subviews each time that we need them instead of
caching subviews when they aren't in use. This came mostly from the
fact that I couldn't think of any real advantages of caching them,
as they aren't expensive to create in the first place.
2) We no longer save the tab scroll position. This seems like
something that, if we find we need, we can add back in without too
much hassle.
BUG=catapult:#2287
NOPRESUBMIT=true
NOTRY=true
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========
Message was sent while issue was closed.
Committed patchset #2 (id:90001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Message was sent while issue was closed.
https://codereview.chromium.org/2023283002/diff/90001/tracing/tracing/ui/base... File tracing/tracing/ui/base/tab_view.html (right): https://codereview.chromium.org/2023283002/diff/90001/tracing/tracing/ui/base... tracing/tracing/ui/base/tab_view.html:115: On 2016/06/01 18:08:47, charliea wrote: > On 2016/06/01 17:40:46, aiolos wrote: > > You *might* need to flush the dom here, depending on how quickly > this.$.subView > > is accessed afterward. > > Acknowledged. I think my preference here is to only flush it if I find that for > some reason it's necessary. sgtm.
Message was sent while issue was closed.
Description was changed from
==========
[polymer] Rewrite the analysis tab view
The analysis tab view has been creating all sorts of problems for the
polymer migration due to some fairly complex update logic that it uses.
This rewrite aims to simplify that logic with a simpler interface and
implementation.
The changed interface required several changes to the analysis view
itself. Most of these changes should be no-ops, with two notable
exceptions:
1) We now create new subviews each time that we need them instead of
caching subviews when they aren't in use. This came mostly from the
fact that I couldn't think of any real advantages of caching them,
as they aren't expensive to create in the first place.
2) We no longer save the tab scroll position. This seems like
something that, if we find we need, we can add back in without too
much hassle.
BUG=catapult:#2287
NOPRESUBMIT=true
NOTRY=true
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========
to
==========
[polymer] Rewrite the analysis tab view
The analysis tab view has been creating all sorts of problems for the
polymer migration due to some fairly complex update logic that it uses.
This rewrite aims to simplify that logic with a simpler interface and
implementation.
The changed interface required several changes to the analysis view
itself. Most of these changes should be no-ops, with two notable
exceptions:
1) We now create new subviews each time that we need them instead of
caching subviews when they aren't in use. This came mostly from the
fact that I couldn't think of any real advantages of caching them,
as they aren't expensive to create in the first place.
2) We no longer save the tab scroll position. This seems like
something that, if we find we need, we can add back in without too
much hassle.
BUG=catapult:#2285
NOPRESUBMIT=true
NOTRY=true
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
