Bug fix: Translate: leak of a ComboboxModel

Bug fix: Translate: leak of a ComboboxModel

A ui::Combobox (|denial_combobox_|) doesn't take an owner of a
ui::ComboboxModel, so we should have clarify who is the owner of
the combobox model.
This CL fixed the leak to make TranslateBubbleView owner of the
ui::ComboboxModel (|denial_combobox_model_|).

BUG=309468
R=sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278347

[hajimehoshi, 2014-06-17 09:08:59]

PTAL

[sky, 2014-06-17 16:30:38]

https://codereview.chromium.org/330443004/diff/20001/chrome/browser/ui/transl...
File chrome/browser/ui/translate/translate_denial_combobox_model.h (right):

https://codereview.chromium.org/330443004/diff/20001/chrome/browser/ui/transl...
chrome/browser/ui/translate/translate_denial_combobox_model.h:13: class
TranslateDenialComboboxModel : public ui::ComboboxModel {
How about a SimpleComboboxModel in ui/base/models that takes a
std::vector<base::string16> ?

https://codereview.chromium.org/330443004/diff/20001/chrome/browser/ui/transl...
chrome/browser/ui/translate/translate_denial_combobox_model.h:15: enum {
Generally we use C++ style for enums.

https://codereview.chromium.org/330443004/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/translate/translate_bubble_view.cc (right):

https://codereview.chromium.org/330443004/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.cc:410: denial_combobox_
= new views::Combobox(denial_combobox_model_.get());
I believe you need to explicitly destroy the combobox before the model,
otherwise ~combobox will try to remove itself as an observer from the model that
has now been destroyed.

[hajimehoshi, 2014-06-18 08:33:37]

Thank you!

https://codereview.chromium.org/330443004/diff/20001/chrome/browser/ui/transl...
File chrome/browser/ui/translate/translate_denial_combobox_model.h (right):

https://codereview.chromium.org/330443004/diff/20001/chrome/browser/ui/transl...
chrome/browser/ui/translate/translate_denial_combobox_model.h:13: class
TranslateDenialComboboxModel : public ui::ComboboxModel {
On 2014/06/17 16:30:38, sky wrote:
> How about a SimpleComboboxModel in ui/base/models that takes a
> std::vector<base::string16> ?

Done.

https://codereview.chromium.org/330443004/diff/20001/chrome/browser/ui/transl...
chrome/browser/ui/translate/translate_denial_combobox_model.h:15: enum {
On 2014/06/17 16:30:38, sky wrote:
> Generally we use C++ style for enums.

Done.

https://codereview.chromium.org/330443004/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/translate/translate_bubble_view.cc (right):

https://codereview.chromium.org/330443004/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.cc:410: denial_combobox_
= new views::Combobox(denial_combobox_model_.get());
On 2014/06/17 16:30:38, sky wrote:
> I believe you need to explicitly destroy the combobox before the model,
> otherwise ~combobox will try to remove itself as an observer from the model
that
> has now been destroyed.

|denial_combobox_model_| is initialized only once and destroyed only when the
TranslateBubbleView is destroyed. TranslateBubbleView::CreateView* is called
only at TranslateBubbleView::Init.

In |~TranslateBubbleView|, all children including the combobox is removed and
destructed, so I'm sure that the model is destructed only after the combobox is
destructed.

[sky, 2014-06-18 16:57:19]

LGTM with the following changes.

https://codereview.chromium.org/330443004/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/translate/translate_bubble_view.cc (right):

https://codereview.chromium.org/330443004/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.cc:410: denial_combobox_
= new views::Combobox(denial_combobox_model_.get());
On 2014/06/18 08:33:37, hajimehoshi wrote:
> On 2014/06/17 16:30:38, sky wrote:
> > I believe you need to explicitly destroy the combobox before the model,
> > otherwise ~combobox will try to remove itself as an observer from the model
> that
> > has now been destroyed.
> 
> |denial_combobox_model_| is initialized only once and destroyed only when the
> TranslateBubbleView is destroyed. TranslateBubbleView::CreateView* is called
> only at TranslateBubbleView::Init.
> 
> In |~TranslateBubbleView|, all children including the combobox is removed and
> destructed, so I'm sure that the model is destructed only after the combobox
is
> destructed.

I missed the explicit RemoveAlloChildViews(true). nice!

https://codereview.chromium.org/330443004/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/translate/translate_bubble_view.cc (right):

https://codereview.chromium.org/330443004/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.cc:44: enum
DenialComboboxIndexes {
Chrome style for enums is ALL_CAPS.

https://codereview.chromium.org/330443004/diff/40001/ui/base/models/simple_co...
File ui/base/models/simple_combobox_model.h (right):

https://codereview.chromium.org/330443004/diff/40001/ui/base/models/simple_co...
ui/base/models/simple_combobox_model.h:19: virtual ~SimpleComboboxModel() {}
Don't inline this.

https://codereview.chromium.org/330443004/diff/40001/ui/base/models/simple_co...
ui/base/models/simple_combobox_model.h:28: std::vector<base::string16> items_;
const

https://codereview.chromium.org/330443004/diff/40001/ui/base/models/simple_co...
ui/base/models/simple_combobox_model.h:29: };
DISALLOW_...

[hajimehoshi, 2014-06-19 03:51:23]

Thank you!

https://codereview.chromium.org/330443004/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/translate/translate_bubble_view.cc (right):

https://codereview.chromium.org/330443004/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.cc:44: enum
DenialComboboxIndexes {
On 2014/06/18 16:57:19, sky wrote:
> Chrome style for enums is ALL_CAPS.

Done.

https://codereview.chromium.org/330443004/diff/40001/ui/base/models/simple_co...
File ui/base/models/simple_combobox_model.h (right):

https://codereview.chromium.org/330443004/diff/40001/ui/base/models/simple_co...
ui/base/models/simple_combobox_model.h:19: virtual ~SimpleComboboxModel() {}
On 2014/06/18 16:57:19, sky wrote:
> Don't inline this.

Done.

https://codereview.chromium.org/330443004/diff/40001/ui/base/models/simple_co...
ui/base/models/simple_combobox_model.h:28: std::vector<base::string16> items_;
On 2014/06/18 16:57:19, sky wrote:
> const

Done.

https://codereview.chromium.org/330443004/diff/40001/ui/base/models/simple_co...
ui/base/models/simple_combobox_model.h:29: };
On 2014/06/18 16:57:19, sky wrote:
> DISALLOW_...

Done.

[hajimehoshi, 2014-06-19 03:51:37]

The CQ bit was checked by hajimehoshi@chromium.org

[commit-bot: I haz the power, 2014-06-19 03:54:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/330443004/60001

[commit-bot: I haz the power, 2014-06-19 12:53:17]

Change committed as 278347