Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1619733003:
ui/base/models: rewrite TreeNode without using ScopedVector (Closed)
Descriptionui/base/models: rewrite TreeNode without using ScopedVector
Now that we are using C++11, moveable types inside containers are supported
and ScopedVector will be removed. So we should migrate to a solution
that does not use ScopedVector.
Unfortunately children() accessor complicates the things and does not
allow us to easily use std::vector<scoped_ptr<NodeType>>, so another
solution with STLDeleteElements was choosen.
BUG=554289
TEST=ui_base_unittests --gtest_filter=*Tree*
R=sky@chromium.org
Committed: https://crrev.com/62a03a94d25bfa7407b088120da89a4cbcbb2cb9
Cr-Commit-Position: refs/heads/master@{#370769}
Patch Set 1 #
Total comments: 5
Patch Set 2 : content_settings fix #
Messages
Total messages: 14 (4 generated)
Is this OK? PTAL! https://codereview.chromium.org/1619733003/diff/1/ui/base/models/tree_node_mo... File ui/base/models/tree_node_model.h (right): https://codereview.chromium.org/1619733003/diff/1/ui/base/models/tree_node_mo... ui/base/models/tree_node_model.h:173: std::vector<NodeType*>& children() { return children_; } I had to use std::vector<NodeType*> because of this accessor. I wish we didn't have it all!
LGTM https://codereview.chromium.org/1619733003/diff/1/ui/base/models/tree_node_mo... File ui/base/models/tree_node_model.h (right): https://codereview.chromium.org/1619733003/diff/1/ui/base/models/tree_node_mo... ui/base/models/tree_node_model.h:173: std::vector<NodeType*>& children() { return children_; } On 2016/01/21 14:24:45, tfarina wrote: > I had to use std::vector<NodeType*> because of this accessor. I wish we didn't > have it all! I disagree. It's an implementation detail as to how this class stores the underlying children and this accessor is meant to return the children. It doesn't make sense to return a vector<scoped_ptr<NodeType>>& here.
The CQ bit was checked by tfarina@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/1619733003/#ps20001 (title: "content_settings fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1619733003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1619733003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== ui/base/models: rewrite TreeNode without using ScopedVector Now that we are using C++11, moveable types inside containers are supported and ScopedVector will be removed. So we should migrate to a solution that does not use ScopedVector. Unfortunately children() accessor complicates the things and does not allow us to easily use std::vector<scoped_ptr<NodeType>>, so another solution with STLDeleteElements was choosen. BUG=554289 TEST=ui_base_unittests --gtest_filter=*Tree* R=sky@chromium.org ========== to ========== ui/base/models: rewrite TreeNode without using ScopedVector Now that we are using C++11, moveable types inside containers are supported and ScopedVector will be removed. So we should migrate to a solution that does not use ScopedVector. Unfortunately children() accessor complicates the things and does not allow us to easily use std::vector<scoped_ptr<NodeType>>, so another solution with STLDeleteElements was choosen. BUG=554289 TEST=ui_base_unittests --gtest_filter=*Tree* R=sky@chromium.org Committed: https://crrev.com/62a03a94d25bfa7407b088120da89a4cbcbb2cb9 Cr-Commit-Position: refs/heads/master@{#370769} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/62a03a94d25bfa7407b088120da89a4cbcbb2cb9 Cr-Commit-Position: refs/heads/master@{#370769}
Message was sent while issue was closed.
vabr@chromium.org changed reviewers: + vabr@chromium.org
Message was sent while issue was closed.
Drive-by question for sky@. Cheers, Vaclav https://codereview.chromium.org/1619733003/diff/1/ui/base/models/tree_node_mo... File ui/base/models/tree_node_model.h (right): https://codereview.chromium.org/1619733003/diff/1/ui/base/models/tree_node_mo... ui/base/models/tree_node_model.h:173: std::vector<NodeType*>& children() { return children_; } On 2016/01/21 18:53:54, sky wrote: > On 2016/01/21 14:24:45, tfarina wrote: > > I had to use std::vector<NodeType*> because of this accessor. I wish we didn't > > have it all! > > I disagree. It's an implementation detail as to how this class stores the > underlying children and this accessor is meant to return the children. It > doesn't make sense to return a vector<scoped_ptr<NodeType>>& here. Does that mean that the accessor is meant to abstract the way the children are stored? It gives full access exactly to the storage, which does not seem like an abstraction. It's not clear to me how the caller is supposed to use the accessor. Because it has no const in the return type, and no comment about permitted usage, it seems like it is meant to modify the storage of children. But then I don't understand why it would not make sense to expose the storage as a vector of scoped_ptrs.
Message was sent while issue was closed.
https://codereview.chromium.org/1619733003/diff/1/ui/base/models/tree_node_mo... File ui/base/models/tree_node_model.h (right): https://codereview.chromium.org/1619733003/diff/1/ui/base/models/tree_node_mo... ui/base/models/tree_node_model.h:173: std::vector<NodeType*>& children() { return children_; } On 2016/01/22 13:57:51, vabr (Chromium) wrote: > On 2016/01/21 18:53:54, sky wrote: > > On 2016/01/21 14:24:45, tfarina wrote: > > > I had to use std::vector<NodeType*> because of this accessor. I wish we > didn't > > > have it all! > > > > I disagree. It's an implementation detail as to how this class stores the > > underlying children and this accessor is meant to return the children. It > > doesn't make sense to return a vector<scoped_ptr<NodeType>>& here. > > Does that mean that the accessor is meant to abstract the way the children are > stored? It gives full access exactly to the storage, which does not seem like an > abstraction. > > It's not clear to me how the caller is supposed to use the accessor. Because it > has no const in the return type, and no comment about permitted usage, it seems > like it is meant to modify the storage of children. But then I don't understand > why it would not make sense to expose the storage as a vector of scoped_ptrs. Good point. I missed that it isn't const. Callers are allowed to mutate to their hearts content.
Message was sent while issue was closed.
https://codereview.chromium.org/1619733003/diff/1/ui/base/models/tree_node_mo... File ui/base/models/tree_node_model.h (right): https://codereview.chromium.org/1619733003/diff/1/ui/base/models/tree_node_mo... ui/base/models/tree_node_model.h:173: std::vector<NodeType*>& children() { return children_; } On 2016/01/22 16:42:09, sky wrote: > On 2016/01/22 13:57:51, vabr (Chromium) wrote: > > On 2016/01/21 18:53:54, sky wrote: > > > On 2016/01/21 14:24:45, tfarina wrote: > > > > I had to use std::vector<NodeType*> because of this accessor. I wish we > > didn't > > > > have it all! > > > > > > I disagree. It's an implementation detail as to how this class stores the > > > underlying children and this accessor is meant to return the children. It > > > doesn't make sense to return a vector<scoped_ptr<NodeType>>& here. > > > > Does that mean that the accessor is meant to abstract the way the children are > > stored? It gives full access exactly to the storage, which does not seem like > an > > abstraction. > > > > It's not clear to me how the caller is supposed to use the accessor. Because > it > > has no const in the return type, and no comment about permitted usage, it > seems > > like it is meant to modify the storage of children. But then I don't > understand > > why it would not make sense to expose the storage as a vector of scoped_ptrs. > > Good point. I missed that it isn't const. Callers are allowed to mutate to their > hearts content. Right, in which case it seems like exposing the scoped_ptrs in the vector would make it easier for the callers not to mess up ownership accidentally. Would you agree with that?
Message was sent while issue was closed.
On 2016/01/22 16:44:49, vabr (Chromium) wrote: > https://codereview.chromium.org/1619733003/diff/1/ui/base/models/tree_node_mo... > File ui/base/models/tree_node_model.h (right): > > https://codereview.chromium.org/1619733003/diff/1/ui/base/models/tree_node_mo... > ui/base/models/tree_node_model.h:173: std::vector<NodeType*>& children() { > return children_; } > On 2016/01/22 16:42:09, sky wrote: > > On 2016/01/22 13:57:51, vabr (Chromium) wrote: > > > On 2016/01/21 18:53:54, sky wrote: > > > > On 2016/01/21 14:24:45, tfarina wrote: > > > > > I had to use std::vector<NodeType*> because of this accessor. I wish we > > > didn't > > > > > have it all! > > > > > > > > I disagree. It's an implementation detail as to how this class stores the > > > > underlying children and this accessor is meant to return the children. It > > > > doesn't make sense to return a vector<scoped_ptr<NodeType>>& here. > > > > > > Does that mean that the accessor is meant to abstract the way the children > are > > > stored? It gives full access exactly to the storage, which does not seem > like > > an > > > abstraction. > > > > > > It's not clear to me how the caller is supposed to use the accessor. Because > > it > > > has no const in the return type, and no comment about permitted usage, it > > seems > > > like it is meant to modify the storage of children. But then I don't > > understand > > > why it would not make sense to expose the storage as a vector of > scoped_ptrs. > > > > Good point. I missed that it isn't const. Callers are allowed to mutate to > their > > hearts content. > > Right, in which case it seems like exposing the scoped_ptrs in the vector would > make it easier for the callers not to mess up ownership accidentally. Would you > agree with that? I briefly looked at the usage. It's a mixture of find and sorting. Exposing vector<scoped_ptr> would make that usage more painful.
Message was sent while issue was closed.
On 2016/01/22 16:49:25, sky wrote: > On 2016/01/22 16:44:49, vabr (Chromium) wrote: > > > https://codereview.chromium.org/1619733003/diff/1/ui/base/models/tree_node_mo... > > File ui/base/models/tree_node_model.h (right): > > > > > https://codereview.chromium.org/1619733003/diff/1/ui/base/models/tree_node_mo... > > ui/base/models/tree_node_model.h:173: std::vector<NodeType*>& children() { > > return children_; } > > On 2016/01/22 16:42:09, sky wrote: > > > On 2016/01/22 13:57:51, vabr (Chromium) wrote: > > > > On 2016/01/21 18:53:54, sky wrote: > > > > > On 2016/01/21 14:24:45, tfarina wrote: > > > > > > I had to use std::vector<NodeType*> because of this accessor. I wish > we > > > > didn't > > > > > > have it all! > > > > > > > > > > I disagree. It's an implementation detail as to how this class stores > the > > > > > underlying children and this accessor is meant to return the children. > It > > > > > doesn't make sense to return a vector<scoped_ptr<NodeType>>& here. > > > > > > > > Does that mean that the accessor is meant to abstract the way the children > > are > > > > stored? It gives full access exactly to the storage, which does not seem > > like > > > an > > > > abstraction. > > > > > > > > It's not clear to me how the caller is supposed to use the accessor. > Because > > > it > > > > has no const in the return type, and no comment about permitted usage, it > > > seems > > > > like it is meant to modify the storage of children. But then I don't > > > understand > > > > why it would not make sense to expose the storage as a vector of > > scoped_ptrs. > > > > > > Good point. I missed that it isn't const. Callers are allowed to mutate to > > their > > > hearts content. > > > > Right, in which case it seems like exposing the scoped_ptrs in the vector > would > > make it easier for the callers not to mess up ownership accidentally. Would > you > > agree with that? > > I briefly looked at the usage. It's a mixture of find and sorting. Exposing > vector<scoped_ptr> would make that usage more painful. Fair enough, thanks for checking! Cheers, Vaclav |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
