Fix object ownership in ui/base/models.

Fix object ownership in ui/base/models.

This establishes an API in ui/base/models that strictly manages object ownership, and then fixes all code that interacts with it to also properly handle ownership.

BUG=555865, 554289
Committed: https://crrev.com/cee78e4e2ca3b3f4f7e2806d24789c73da9a7638
Cr-Commit-Position: refs/heads/master@{#422136}

[Avi (use Gerrit), 2016-09-29 02:06:52]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 02:07:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 02:20:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 02:20:33]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[Avi (use Gerrit), 2016-09-29 17:23:23]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 17:23:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 17:40:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 17:40:43]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Avi (use Gerrit), 2016-09-29 18:18:18]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 18:18:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 18:33:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 18:33:24]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Avi (use Gerrit), 2016-09-29 19:13:38]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 19:14:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 19:33:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 19:33:54]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Avi (use Gerrit), 2016-09-29 19:42:36]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 19:43:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 20:42:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 20:42:55]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-09-29 20:54:20]

avi@chromium.org changed reviewers:
+ bauerb@chromium.org, benwells@chromium.org, sky@chromium.org

[Avi (use Gerrit), 2016-09-29 20:54:21]

bauerb: chrome/browser/android, chrome/browser/browsing_data
benwells: ui/app_list
sky: ash/shell, chrome/browser/ui/views, components/bookmarks, ui/base, ui/views

The main ownership changes are in ui/base/models, and they're mostly knock-on
from there.

[sky, 2016-09-29 22:06:49]

Ugh!

https://codereview.chromium.org/2379863002/diff/80001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/2379863002/diff/80001/components/bookmarks/br...
components/bookmarks/browser/bookmark_model.cc:693: for (size_t i = 0; i <
ordered_nodes.size() - 1; ++i) {
if ordered_nodes is empty this code is problematic.

https://codereview.chromium.org/2379863002/diff/80001/components/bookmarks/br...
components/bookmarks/browser/bookmark_model.cc:695:
mutable_parent->Remove(AsMutable(ordered_nodes[i]));
This implementation is way more expensive than the old.

https://codereview.chromium.org/2379863002/diff/80001/components/bookmarks/br...
components/bookmarks/browser/bookmark_model.cc:844: bookmark_bar_node_ =
owned_bb_node.get();
Can you set these members when you add them below?

https://codereview.chromium.org/2379863002/diff/80001/ui/base/models/tree_nod...
File ui/base/models/tree_node_model.h (right):

https://codereview.chromium.org/2379863002/diff/80001/ui/base/models/tree_nod...
ui/base/models/tree_node_model.h:91: auto i = std::find_if(children_.begin(),
children_.end(),
Same code exists on 141-144. Move to a common function?

[Avi (use Gerrit), 2016-09-29 22:56:44]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 22:57:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Avi (use Gerrit), 2016-09-29 22:59:59]

Scott, re your comment "ugh", can you clarify if that's an actionable comment
for me?

https://codereview.chromium.org/2379863002/diff/80001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/2379863002/diff/80001/components/bookmarks/br...
components/bookmarks/browser/bookmark_model.cc:693: for (size_t i = 0; i <
ordered_nodes.size() - 1; ++i) {
On 2016/09/29 22:06:48, sky wrote:
> if ordered_nodes is empty this code is problematic.

Done.

https://codereview.chromium.org/2379863002/diff/80001/components/bookmarks/br...
components/bookmarks/browser/bookmark_model.cc:695:
mutable_parent->Remove(AsMutable(ordered_nodes[i]));
On 2016/09/29 22:06:48, sky wrote:
> This implementation is way more expensive than the old.

Yes. The only thought I have on how to do better is to pass the vector into the
TreeNode and have it rearrange the children. WDYT?

https://codereview.chromium.org/2379863002/diff/80001/components/bookmarks/br...
components/bookmarks/browser/bookmark_model.cc:844: bookmark_bar_node_ =
owned_bb_node.get();
On 2016/09/29 22:06:48, sky wrote:
> Can you set these members when you add them below?

Not sure what you mean. Have something like

  root_children.push_back(std::move(owned_bb_node));
  bookmark_bar_node_ = root_children.back().get();
  root_children.push_back(std::move(owned_other_folder_node));
  other_node_ = root_children.back().get();
  root_children.push_back(std::move(owned_mobile_folder_node));
  mobile_node_ = root_children.back().get();

? That doesn't read as much cleaner to me, but if you prefer...

https://codereview.chromium.org/2379863002/diff/80001/ui/base/models/tree_nod...
File ui/base/models/tree_node_model.h (right):

https://codereview.chromium.org/2379863002/diff/80001/ui/base/models/tree_nod...
ui/base/models/tree_node_model.h:91: auto i = std::find_if(children_.begin(),
children_.end(),
On 2016/09/29 22:06:48, sky wrote:
> Same code exists on 141-144. Move to a common function?

FYI the two callers are of different const-ness. Either we implement the
function twice, or we const_cast. (Unless I'm missing something.)

I'm const_casting; let me know what you think.

[commit-bot: I haz the power, 2016-09-29 23:12:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 23:12:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Avi (use Gerrit), 2016-09-30 00:13:36]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 00:14:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Avi (use Gerrit), 2016-09-30 00:16:48]

https://codereview.chromium.org/2379863002/diff/80001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/2379863002/diff/80001/components/bookmarks/br...
components/bookmarks/browser/bookmark_model.cc:695:
mutable_parent->Remove(AsMutable(ordered_nodes[i]));
On 2016/09/29 22:59:58, Avi wrote:
> On 2016/09/29 22:06:48, sky wrote:
> > This implementation is way more expensive than the old.
> 
> Yes. The only thought I have on how to do better is to pass the vector into
the
> TreeNode and have it rearrange the children. WDYT?

Actually I have an idea.

https://codereview.chromium.org/2379863002/diff/80001/ui/base/models/tree_nod...
File ui/base/models/tree_node_model.h (right):

https://codereview.chromium.org/2379863002/diff/80001/ui/base/models/tree_nod...
ui/base/models/tree_node_model.h:91: auto i = std::find_if(children_.begin(),
children_.end(),
On 2016/09/29 22:59:58, Avi wrote:
> On 2016/09/29 22:06:48, sky wrote:
> > Same code exists on 141-144. Move to a common function?
> 
> FYI the two callers are of different const-ness. Either we implement the
> function twice, or we const_cast. (Unless I'm missing something.)
> 
> I'm const_casting; let me know what you think.

aaaaaand I can't const_cast. I'm stuck with a const iterator to use on
vector::erase, and linux won't let me erase with a const iterator!
https://groups.google.com/a/chromium.org/d/msg/cxx/FJvc7odo2Us/o5fiZBr4CwAJ

I have to put this back; sorry.

[Avi (use Gerrit), 2016-09-30 00:20:37]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 00:21:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 01:12:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 01:12:39]

Dry run: This issue passed the CQ dry run.

[sky, 2016-09-30 03:07:05]

Ugh! was more of the size of the review and the scope of the changes and not
something bad about the change. This definitely clarifies ownership and I like
that!

LGTM

https://codereview.chromium.org/2379863002/diff/80001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/2379863002/diff/80001/components/bookmarks/br...
components/bookmarks/browser/bookmark_model.cc:844: bookmark_bar_node_ =
owned_bb_node.get();
On 2016/09/29 22:59:58, Avi wrote:
> On 2016/09/29 22:06:48, sky wrote:
> > Can you set these members when you add them below?
> 
> Not sure what you mean. Have something like
> 
>   root_children.push_back(std::move(owned_bb_node));
>   bookmark_bar_node_ = root_children.back().get();
>   root_children.push_back(std::move(owned_other_folder_node));
>   other_node_ = root_children.back().get();
>   root_children.push_back(std::move(owned_mobile_folder_node));
>   mobile_node_ = root_children.back().get();
> 
> ? That doesn't read as much cleaner to me, but if you prefer...

My mistake. What you have is fine.

https://codereview.chromium.org/2379863002/diff/80001/ui/base/models/tree_nod...
File ui/base/models/tree_node_model.h (right):

https://codereview.chromium.org/2379863002/diff/80001/ui/base/models/tree_nod...
ui/base/models/tree_node_model.h:91: auto i = std::find_if(children_.begin(),
children_.end(),
On 2016/09/30 00:16:48, Avi wrote:
> On 2016/09/29 22:59:58, Avi wrote:
> > On 2016/09/29 22:06:48, sky wrote:
> > > Same code exists on 141-144. Move to a common function?
> > 
> > FYI the two callers are of different const-ness. Either we implement the
> > function twice, or we const_cast. (Unless I'm missing something.)
> > 
> > I'm const_casting; let me know what you think.
> 
> aaaaaand I can't const_cast. I'm stuck with a const iterator to use on
> vector::erase, and linux won't let me erase with a const iterator!
> https://groups.google.com/a/chromium.org/d/msg/cxx/FJvc7odo2Us/o5fiZBr4CwAJ
> 
> I have to put this back; sorry.

<rant>
I like the thought of containers with std::unique_ptr, but IMO unique_ptr makes
for a level of awkwardness that ScopedVector doesn't have. find is a good
example of the extra level of awkwardness.
</rant>

https://codereview.chromium.org/2379863002/diff/140001/components/bookmarks/b...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/2379863002/diff/140001/components/bookmarks/b...
components/bookmarks/browser/bookmark_model.cc:692: if (ordered_nodes.size() >
1) {
!empty

https://codereview.chromium.org/2379863002/diff/140001/components/bookmarks/b...
components/bookmarks/browser/bookmark_model.cc:693: std::map<const
BookmarkNode*, int> order;
Nice!

[Avi (use Gerrit), 2016-09-30 03:42:24]

https://codereview.chromium.org/2379863002/diff/140001/components/bookmarks/b...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/2379863002/diff/140001/components/bookmarks/b...
components/bookmarks/browser/bookmark_model.cc:692: if (ordered_nodes.size() >
1) {
On 2016/09/30 03:07:05, sky wrote:
> !empty

Why? It doesn't make sense to sort exactly one child.

https://codereview.chromium.org/2379863002/diff/140001/components/bookmarks/b...
components/bookmarks/browser/bookmark_model.cc:693: std::map<const
BookmarkNode*, int> order;
On 2016/09/30 03:07:05, sky wrote:
> Nice!

Thanks!

[benwells, 2016-09-30 04:07:50]

ui/app_list lgtm

[Bernhard Bauer, 2016-09-30 08:58:44]

LGTM, but there are some more ownership clarifications we could do with partner
bookmarks. Raw pointers are starting to feel more and more icky to me...

https://codereview.chromium.org/2379863002/diff/140001/chrome/browser/android...
File chrome/browser/android/bookmarks/partner_bookmarks_reader.cc (right):

https://codereview.chromium.org/2379863002/diff/140001/chrome/browser/android...
chrome/browser/android/bookmarks/partner_bookmarks_reader.cc:157: BookmarkNode*
node = NULL;
Can we make this a unique_ptr<> from the start?

https://codereview.chromium.org/2379863002/diff/140001/chrome/browser/android...
File chrome/browser/android/bookmarks/partner_bookmarks_shim_unittest.cc
(right):

https://codereview.chromium.org/2379863002/diff/140001/chrome/browser/android...
chrome/browser/android/bookmarks/partner_bookmarks_shim_unittest.cc:93:
BookmarkNode* partner_folder1 = new BookmarkNode(1, GURL());
Can we make this a unique_ptr<> as well?

https://codereview.chromium.org/2379863002/diff/140001/chrome/browser/android...
chrome/browser/android/bookmarks/partner_bookmarks_shim_unittest.cc:117:
shim->SetPartnerBookmarksRoot(root_partner_node);
And make this method also take a unique_ptr<>?

[Avi (use Gerrit), 2016-09-30 14:53:44]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2016-09-30 14:53:44]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
benwells@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2379863002/#ps160001
(title: "more android work")

[commit-bot: I haz the power, 2016-09-30 14:54:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 15:14:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 15:14:24]

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_...)

[sky, 2016-09-30 15:57:53]

LGTM

https://codereview.chromium.org/2379863002/diff/140001/components/bookmarks/b...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/2379863002/diff/140001/components/bookmarks/b...
components/bookmarks/browser/bookmark_model.cc:692: if (ordered_nodes.size() >
1) {
On 2016/09/30 03:42:23, Avi wrote:
> On 2016/09/30 03:07:05, sky wrote:
> > !empty
> 
> Why? It doesn't make sense to sort exactly one child.

Your are right. Sorry for the noise.

[Avi (use Gerrit), 2016-09-30 16:09:23]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2016-09-30 16:09:23]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
benwells@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2379863002/#ps180001
(title: "fix")

[commit-bot: I haz the power, 2016-09-30 16:09:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Avi (use Gerrit), 2016-09-30 16:10:12]

https://codereview.chromium.org/2379863002/diff/140001/chrome/browser/android...
File chrome/browser/android/bookmarks/partner_bookmarks_reader.cc (right):

https://codereview.chromium.org/2379863002/diff/140001/chrome/browser/android...
chrome/browser/android/bookmarks/partner_bookmarks_reader.cc:157: BookmarkNode*
node = NULL;
On 2016/09/30 08:58:43, Bernhard Bauer wrote:
> Can we make this a unique_ptr<> from the start?

Done.

https://codereview.chromium.org/2379863002/diff/140001/chrome/browser/android...
File chrome/browser/android/bookmarks/partner_bookmarks_shim_unittest.cc
(right):

https://codereview.chromium.org/2379863002/diff/140001/chrome/browser/android...
chrome/browser/android/bookmarks/partner_bookmarks_shim_unittest.cc:93:
BookmarkNode* partner_folder1 = new BookmarkNode(1, GURL());
On 2016/09/30 08:58:44, Bernhard Bauer wrote:
> Can we make this a unique_ptr<> as well?

Done.

https://codereview.chromium.org/2379863002/diff/140001/chrome/browser/android...
chrome/browser/android/bookmarks/partner_bookmarks_shim_unittest.cc:117:
shim->SetPartnerBookmarksRoot(root_partner_node);
On 2016/09/30 08:58:43, Bernhard Bauer wrote:
> And make this method also take a unique_ptr<>?

Done.

[commit-bot: I haz the power, 2016-09-30 17:08:34]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-30 17:11:24]

Description was changed from

==========
Fix object ownership in ui/base/models.

This establishes an API in ui/base/models that strictly manages object
ownership, and then fixes all code that interacts with it to also properly
handle ownership.

BUG=555865,554289
==========

to

==========
Fix object ownership in ui/base/models.

This establishes an API in ui/base/models that strictly manages object
ownership, and then fixes all code that interacts with it to also properly
handle ownership.

BUG=555865,554289

Committed: https://crrev.com/cee78e4e2ca3b3f4f7e2806d24789c73da9a7638
Cr-Commit-Position: refs/heads/master@{#422136}
==========

[commit-bot: I haz the power, 2016-09-30 17:11:25]

Patchset 10 (id:??) landed as
https://crrev.com/cee78e4e2ca3b3f4f7e2806d24789c73da9a7638
Cr-Commit-Position: refs/heads/master@{#422136}