Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1757073002:
Use LayerListIterator when hit testing. (Closed)
|
Created:
4 years, 9 months ago by Ian Vollick Modified:
4 years, 9 months ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse LayerListIterator when hit testing.
BUG=557194, 591512
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/87250ec1798ba5c656ec30aedf2f2e35a2d39267
Cr-Commit-Position: refs/heads/master@{#379009}
Patch Set 1 #Patch Set 2 : Rename some stuff to avoid using the word 'recursion' #
Total comments: 3
Patch Set 3 : now with rbegin rend #Messages
Total messages: 28 (12 generated)
Description was changed from ========== Use LayerListIterator when hit testing. BUG=557194 ========== to ========== Use LayerListIterator when hit testing. BUG=557194 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Description was changed from ========== Use LayerListIterator when hit testing. BUG=557194 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Use LayerListIterator when hit testing. BUG=557194 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
vollick@chromium.org changed reviewers: + ajuma@chromium.org, weiliangc@chromium.org
The CQ bit was checked by vollick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1757073002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1757073002/20001
Description was changed from ========== Use LayerListIterator when hit testing. BUG=557194 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Use LayerListIterator when hit testing. BUG=557194,591512 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
lgtm with a question https://codereview.chromium.org/1757073002/diff/20001/cc/trees/layer_tree_imp... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/1757073002/diff/20001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.cc:1615: for (; it != end; ++it) { Do we need the explicit 'end' here, or could we just check that 'it' is true?
On 2016/03/02 22:36:48, ajuma wrote: > lgtm with a question > > https://codereview.chromium.org/1757073002/diff/20001/cc/trees/layer_tree_imp... > File cc/trees/layer_tree_impl.cc (right): > > https://codereview.chromium.org/1757073002/diff/20001/cc/trees/layer_tree_imp... > cc/trees/layer_tree_impl.cc:1615: for (; it != end; ++it) { > Do we need the explicit 'end' here, or could we just check that 'it' is true? I haven't defined LayerListIterator::operator bool(), but *it would work. wdyt?
danakj@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/1757073002/diff/20001/cc/trees/layer_tree_imp... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/1757073002/diff/20001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.cc:1615: for (; it != end; ++it) { On 2016/03/02 22:36:47, ajuma wrote: > Do we need the explicit 'end' here, or could we just check that 'it' is true? jw, is it possible that rbegin()/rend() on LayerImpl() return iterators and then you can use a range-based for loop?
On 2016/03/02 22:41:49, danakj wrote: > https://codereview.chromium.org/1757073002/diff/20001/cc/trees/layer_tree_imp... > File cc/trees/layer_tree_impl.cc (right): > > https://codereview.chromium.org/1757073002/diff/20001/cc/trees/layer_tree_imp... > cc/trees/layer_tree_impl.cc:1615: for (; it != end; ++it) { > On 2016/03/02 22:36:47, ajuma wrote: > > Do we need the explicit 'end' here, or could we just check that 'it' is true? > > jw, is it possible that rbegin()/rend() on LayerImpl() return iterators and then > you can use a range-based for loop? These would ultimately be provided by the layer list. In the fullness of time, layers won't know about the next element in the list. That said, I could add begin/end rbegin/rend to LayerTreeImpl (which will become the layer list). Happy to add that sugar if you'd prefer it.
https://codereview.chromium.org/1757073002/diff/20001/cc/trees/layer_tree_imp... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/1757073002/diff/20001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.cc:1615: for (; it != end; ++it) { On 2016/03/02 22:41:49, danakj wrote: > On 2016/03/02 22:36:47, ajuma wrote: > > Do we need the explicit 'end' here, or could we just check that 'it' is true? > > jw, is it possible that rbegin()/rend() on LayerImpl() return iterators and then > you can use a range-based for loop? I would prefer that personally, it would make it look like STL so you could use STL tools with it then. Also you wouldn't need temporaries like this even if you didn't use range-based loop for some reason. for (auto it = rbegin(); it = rend(); ++it) would work (btw base::Reversed would be of interest to you then. https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/base/adapte...)
On 2016/03/02 22:46:32, danakj wrote: > https://codereview.chromium.org/1757073002/diff/20001/cc/trees/layer_tree_imp... > File cc/trees/layer_tree_impl.cc (right): > > https://codereview.chromium.org/1757073002/diff/20001/cc/trees/layer_tree_imp... > cc/trees/layer_tree_impl.cc:1615: for (; it != end; ++it) { > On 2016/03/02 22:41:49, danakj wrote: > > On 2016/03/02 22:36:47, ajuma wrote: > > > Do we need the explicit 'end' here, or could we just check that 'it' is > true? > > > > jw, is it possible that rbegin()/rend() on LayerImpl() return iterators and > then > > you can use a range-based for loop? > > I would prefer that personally, it would make it look like STL so you could use > STL tools with it then. Also you wouldn't need temporaries like this even if you > didn't use range-based loop for some reason. > > for (auto it = rbegin(); it = rend(); ++it) would work > > (btw base::Reversed would be of interest to you then. > https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/base/adapte...) sgtm. I'll whip up a patch for that now. Would like to do that in advance of this CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/03/02 23:28:55, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. I've now rebased this upon https://codereview.chromium.org/1752263003/ and have made use of the base::Reversed business.
The CQ bit was checked by vollick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1757073002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1757073002/40001
On 2016/03/03 04:52:07, vollick wrote: > On 2016/03/02 23:28:55, commit-bot: I haz the power wrote: > > Dry run: This issue passed the CQ dry run. > > I've now rebased this upon https://codereview.chromium.org/1752263003/ and have > made use of the base::Reversed business. still lgtm
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 vollick@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1757073002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1757073002/40001
Message was sent while issue was closed.
Description was changed from ========== Use LayerListIterator when hit testing. BUG=557194,591512 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Use LayerListIterator when hit testing. BUG=557194,591512 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Use LayerListIterator when hit testing. BUG=557194,591512 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Use LayerListIterator when hit testing. BUG=557194,591512 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/87250ec1798ba5c656ec30aedf2f2e35a2d39267 Cr-Commit-Position: refs/heads/master@{#379009} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/87250ec1798ba5c656ec30aedf2f2e35a2d39267 Cr-Commit-Position: refs/heads/master@{#379009} |
