Fixed memory leak. Untangled confusing code with menu initialization and ownership.

Fixed memory leak. Untangled confusing code with menu initialization and ownership.

BUG=141037
TEST=Make sure memory bots passed.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150469

[gene, 2012-08-07 19:07:52]

Hi Peter,
Since you did previous CL, could you please take a look at quick fix for memory
leak related to that.

[Peter Kasting, 2012-08-07 19:43:03]

This does fix the bug, but I think it would be better to instead change
ButtonDropDown's |model_| member to be a scoped_ptr, leave the ReloadButton code
the way you had it, and eliminate the ToolbarView |XXX_menu_model_| members,
instead simply passing "new BackForwardMenuModel(...)" directly to the
ButtonDropDown constructor calls in ToolbarView::Init().

This eliminates the need for set_menu_model_ and eliminates the menu model
members on other classes, and also makes the class that owns the menu model
(namely ButtonDropdown) be the one that uses it, which makes the most inherent
sense.

https://chromiumcodereview.appspot.com/10832184/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/reload_button.cc (right):

https://chromiumcodereview.appspot.com/10832184/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/reload_button.cc:62:
ButtonDropDown::set_menu_model(NULL);
This doesn't seem necessary.

[gene, 2012-08-07 20:58:34]

OK, sounds good.
Let me try going this way and have DropDownButton manage menu model lifetime.

https://chromiumcodereview.appspot.com/10832184/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/reload_button.cc (right):

https://chromiumcodereview.appspot.com/10832184/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/reload_button.cc:62:
ButtonDropDown::set_menu_model(NULL);
You are right, it does not now.
However ButtonDropDown menu destructor is called after ReloadButton destructor.
Here I wanted to make sure ~ButtonDropDown does not have invalid pointer.

[gene, 2012-08-07 21:27:15]

OK, here is a new implementation, please take a look.

On 2012/08/07 20:58:34, gene wrote:
> OK, sounds good.
> Let me try going this way and have DropDownButton manage menu model lifetime.
> 
>
https://chromiumcodereview.appspot.com/10832184/diff/1/chrome/browser/ui/view...
> File chrome/browser/ui/views/reload_button.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10832184/diff/1/chrome/browser/ui/view...
> chrome/browser/ui/views/reload_button.cc:62:
> ButtonDropDown::set_menu_model(NULL);
> You are right, it does not now.
> However ButtonDropDown menu destructor is called after ReloadButton
destructor.
> Here I wanted to make sure ~ButtonDropDown does not have invalid pointer.

[Peter Kasting, 2012-08-07 21:32:26]

LGTM

https://chromiumcodereview.appspot.com/10832184/diff/7002/ui/views/controls/b...
File ui/views/controls/button/button_dropdown.h (right):

https://chromiumcodereview.appspot.com/10832184/diff/7002/ui/views/controls/b...
ui/views/controls/button/button_dropdown.h:32: // Constructor takes ownership of
the |model| and destroys
Nit: Allocated with new as opposed to... malloc?

Maybe this should just say "Takes ownership of |model|."

[gene, 2012-08-07 21:42:37]

ben, sky - could you please take a look as OWNERS of ui/views?

https://chromiumcodereview.appspot.com/10832184/diff/7002/ui/views/controls/b...
File ui/views/controls/button/button_dropdown.h (right):

https://chromiumcodereview.appspot.com/10832184/diff/7002/ui/views/controls/b...
ui/views/controls/button/button_dropdown.h:32: // Constructor takes ownership of
the |model| and destroys
On 2012/08/07 21:32:26, Peter Kasting wrote:
> Nit: Allocated with new as opposed to... malloc?
> 
> Maybe this should just say "Takes ownership of |model|."

Done.

[sky, 2012-08-07 21:46:38]

LGTM

[commit-bot: I haz the power, 2012-08-07 22:59:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gene@chromium.org/10832184/4008

[commit-bot: I haz the power, 2012-08-08 00:33:42]

Change committed as 150469