Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2894673008:
Refactor scrollbar maps from multimap & std::set to simpler flat_{map, set} (Closed)
DescriptionRefactor scrollbar maps from multimap & std::set to simpler flat_{map, set}
LayerTreeImpl used a multimap to track two scrollbar layer ids
(horizontal and vertical, both optional) which is inefficient. This
patch refactors this map to be a flat_map of ElementId to a pair of
layer ids.
Although not necessary for this patch, ScrollbarLayerImplBase has
been refactored to use a flat_set instead of an std::set for similar
reasons.
BUG=723263
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2894673008
Cr-Commit-Position: refs/heads/master@{#473637}
Committed: https://chromium.googlesource.com/chromium/src/+/8cafc0ba2edea517372c716dd76c11164095c1ad
Patch Set 1 #
Total comments: 4
Patch Set 2 : Update per reviewer comments #
Total comments: 4
Patch Set 3 : Address vmpstr comments #
Total comments: 2
Patch Set 4 : Ensure scrollbars are unregistered before registration #
Total comments: 2
Patch Set 5 : DCHECK_EQ #
Messages
Total messages: 30 (18 generated)
Description was changed from
==========
Refactor scrollbar maps from multimap & std::set to simpler flat_{maps, set}
LayerTreeImpl used a multimap to track the two scrollbar layer ids
(horizontal and vertical, both optional) which is inefficient. This
patch refactors this map to be a flat_map of ElementId to a pair of
layer ids.
Although not necessary for this patch, ScrollbarLayerImplBase has
been refactored to use a flat_set instead of an std::set for similar
reasons.
BUG=723263
==========
to
==========
Refactor scrollbar maps from multimap & std::set to simpler flat_{maps, set}
LayerTreeImpl used a multimap to track the two scrollbar layer ids
(horizontal and vertical, both optional) which is inefficient. This
patch refactors this map to be a flat_map of ElementId to a pair of
layer ids.
Although not necessary for this patch, ScrollbarLayerImplBase has
been refactored to use a flat_set instead of an std::set for similar
reasons.
BUG=723263
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========
The CQ bit was checked by pdr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
Refactor scrollbar maps from multimap & std::set to simpler flat_{maps, set}
LayerTreeImpl used a multimap to track the two scrollbar layer ids
(horizontal and vertical, both optional) which is inefficient. This
patch refactors this map to be a flat_map of ElementId to a pair of
layer ids.
Although not necessary for this patch, ScrollbarLayerImplBase has
been refactored to use a flat_set instead of an std::set for similar
reasons.
BUG=723263
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========
to
==========
Refactor scrollbar maps from multimap & std::set to simpler flat_{map, set}
LayerTreeImpl used a multimap to track two scrollbar layer ids
(horizontal and vertical, both optional) which is inefficient. This
patch refactors this map to be a flat_map of ElementId to a pair of
layer ids.
Although not necessary for this patch, ScrollbarLayerImplBase has
been refactored to use a flat_set instead of an std::set for similar
reasons.
BUG=723263
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========
pdr@chromium.org changed reviewers: + enne@chromium.org, wkorman@chromium.org
pdr@chromium.org changed reviewers: + danakj@chromium.org
lgtm https://codereview.chromium.org/2894673008/diff/1/cc/layers/scrollbar_layer_i... File cc/layers/scrollbar_layer_impl_base.h (right): https://codereview.chromium.org/2894673008/diff/1/cc/layers/scrollbar_layer_i... cc/layers/scrollbar_layer_impl_base.h:99: typedef base::flat_set<ScrollbarLayerImplBase*> ScrollbarSet; Switch to 'using' rather than typedef? https://codereview.chromium.org/2894673008/diff/1/cc/trees/layer_tree_impl.h File cc/trees/layer_tree_impl.h (right): https://codereview.chromium.org/2894673008/diff/1/cc/trees/layer_tree_impl.h#... cc/trees/layer_tree_impl.h:586: // horizontal). This mapping is derived from ScrollbarLayerImplBase's Suggest removing "This mapping..." since code/classes prone to change and folks can code search. Or rephrase to prose like "The mapping is maintained as part of scrollbar registration."
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pdr@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
Thanks for the review https://codereview.chromium.org/2894673008/diff/1/cc/layers/scrollbar_layer_i... File cc/layers/scrollbar_layer_impl_base.h (right): https://codereview.chromium.org/2894673008/diff/1/cc/layers/scrollbar_layer_i... cc/layers/scrollbar_layer_impl_base.h:99: typedef base::flat_set<ScrollbarLayerImplBase*> ScrollbarSet; On 2017/05/20 at 01:46:29, wkorman wrote: > Switch to 'using' rather than typedef? Done https://codereview.chromium.org/2894673008/diff/1/cc/trees/layer_tree_impl.h File cc/trees/layer_tree_impl.h (right): https://codereview.chromium.org/2894673008/diff/1/cc/trees/layer_tree_impl.h#... cc/trees/layer_tree_impl.h:586: // horizontal). This mapping is derived from ScrollbarLayerImplBase's On 2017/05/20 at 01:46:29, wkorman wrote: > Suggest removing "This mapping..." since code/classes prone to change and folks can code search. Or rephrase to prose like "The mapping is maintained as part of scrollbar registration." Done
vmpstr@chromium.org changed reviewers: + vmpstr@chromium.org
lgtm with nits https://codereview.chromium.org/2894673008/diff/20001/cc/layers/scrollbar_lay... File cc/layers/scrollbar_layer_impl_base.h (right): https://codereview.chromium.org/2894673008/diff/20001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_impl_base.h:99: using ScrollbarSet = base::flat_set<ScrollbarLayerImplBase*>; nit: Include flat_set plz. https://codereview.chromium.org/2894673008/diff/20001/cc/trees/layer_tree_imp... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/2894673008/diff/20001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.cc:1701: if (scrollbar_ids.horizontal == Layer::INVALID_ID) not: Instead of no_remaining_scrollbars, you can just check both horizontal and vertical on line 1705
https://codereview.chromium.org/2894673008/diff/20001/cc/layers/scrollbar_lay... File cc/layers/scrollbar_layer_impl_base.h (right): https://codereview.chromium.org/2894673008/diff/20001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_impl_base.h:99: using ScrollbarSet = base::flat_set<ScrollbarLayerImplBase*>; On 2017/05/22 at 16:27:22, vmpstr wrote: > nit: Include flat_set plz. Done https://codereview.chromium.org/2894673008/diff/20001/cc/trees/layer_tree_imp... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/2894673008/diff/20001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.cc:1701: if (scrollbar_ids.horizontal == Layer::INVALID_ID) On 2017/05/22 at 16:27:22, vmpstr wrote: > not: Instead of no_remaining_scrollbars, you can just check both horizontal and vertical on line 1705 nit/not done
https://codereview.chromium.org/2894673008/diff/40001/cc/trees/layer_tree_imp... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/2894673008/diff/40001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.cc:1670: element_id_to_scrollbar_layer_ids_[scroll_element_id].horizontal = If there was a previous scrollbar, does that need to be unregistered? I think previously you could have two scrollbars for the same element id and it wouldn't clobber. Maybe this just needs to DCHECK instead?
https://codereview.chromium.org/2894673008/diff/40001/cc/trees/layer_tree_imp... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/2894673008/diff/40001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.cc:1670: element_id_to_scrollbar_layer_ids_[scroll_element_id].horizontal = On 2017/05/22 at 17:05:52, enne wrote: > If there was a previous scrollbar, does that need to be unregistered? I think previously you could have two scrollbars for the same element id and it wouldn't clobber. > > Maybe this just needs to DCHECK instead? Good point, added a DCHECK.
lgtm https://codereview.chromium.org/2894673008/diff/60001/cc/trees/layer_tree_imp... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/2894673008/diff/60001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.cc:1671: DCHECK(scrollbar_ids.horizontal == Layer::INVALID_ID) DCHECK_EQ, if you don't mind?
https://codereview.chromium.org/2894673008/diff/60001/cc/trees/layer_tree_imp... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/2894673008/diff/60001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.cc:1671: DCHECK(scrollbar_ids.horizontal == Layer::INVALID_ID) On 2017/05/22 at 17:35:12, enne wrote: > DCHECK_EQ, if you don't mind? Done We should enforce this with a presubmit check. This isn't just grumbling... DCHECK_EQ is actually superior because it prints out the two values.
pdr@chromium.org changed reviewers: - danakj@chromium.org
The CQ bit was checked by pdr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org, vmpstr@chromium.org, enne@chromium.org Link to the patchset: https://codereview.chromium.org/2894673008/#ps80001 (title: "DCHECK_EQ")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I think you'd have to write a clang plugin, because not every type can be printed. <_<
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495474767627410,
"parent_rev": "8fb01a7d9c7310372afa618e9b4a6799e68a3365", "commit_rev":
"8cafc0ba2edea517372c716dd76c11164095c1ad"}
Message was sent while issue was closed.
Description was changed from
==========
Refactor scrollbar maps from multimap & std::set to simpler flat_{map, set}
LayerTreeImpl used a multimap to track two scrollbar layer ids
(horizontal and vertical, both optional) which is inefficient. This
patch refactors this map to be a flat_map of ElementId to a pair of
layer ids.
Although not necessary for this patch, ScrollbarLayerImplBase has
been refactored to use a flat_set instead of an std::set for similar
reasons.
BUG=723263
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========
to
==========
Refactor scrollbar maps from multimap & std::set to simpler flat_{map, set}
LayerTreeImpl used a multimap to track two scrollbar layer ids
(horizontal and vertical, both optional) which is inefficient. This
patch refactors this map to be a flat_map of ElementId to a pair of
layer ids.
Although not necessary for this patch, ScrollbarLayerImplBase has
been refactored to use a flat_set instead of an std::set for similar
reasons.
BUG=723263
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2894673008
Cr-Commit-Position: refs/heads/master@{#473637}
Committed:
https://chromium.googlesource.com/chromium/src/+/8cafc0ba2edea517372c716dd76c...
==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/8cafc0ba2edea517372c716dd76c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
