views/bookmarks: Fix memory leak in BookmarkEditorView.

views/bookmarks: Fix memory leak in BookmarkEditorView.

Allocate |url_label_| on the stack instead of in the free store(heap), and set
set_parent_owned to false.

Thus, we manage its memory when we create a unittest with EditDetails::NEW_FOLDER.

BUG=92159
TEST=None

R=pkasting@chromium.org

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

[tfarina, 2011-08-11 13:14:06]

Hi Peter, could you review this to me?

[Peter Kasting, 2011-08-11 18:59:42]

http://codereview.chromium.org/7617006/diff/4/chrome/browser/ui/views/bookmar...
File chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc (right):

http://codereview.chromium.org/7617006/diff/4/chrome/browser/ui/views/bookmar...
chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc:328:
url_label_.set_parent_owned(false);
This whole system concerns me.

We have |url_tf_| and |url_label_| always existing, but only sometimes in the
view hierarchy.  Then various functions access |url_tf_|.

It seems like these objects ought to be created only when they're actually going
to be added to the tree.  In order to make this safe, we need to either verify
that the accessing functions will only be called in this case (and add some
DCHECKs to make that clear), or we should understand why they're accessing
invisible objects, which seems very wrong.

[tfarina, 2011-08-11 20:18:34]

On 2011/08/11 18:59:42, Peter Kasting wrote:
>
http://codereview.chromium.org/7617006/diff/4/chrome/browser/ui/views/bookmar...
> File chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc (right):
> 
>
http://codereview.chromium.org/7617006/diff/4/chrome/browser/ui/views/bookmar...
> chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc:328:
> url_label_.set_parent_owned(false);
> This whole system concerns me.
> 
> We have |url_tf_| and |url_label_| always existing, but only sometimes in the
> view hierarchy.  Then various functions access |url_tf_|.
> 
> It seems like these objects ought to be created only when they're actually
going
> to be added to the tree.  In order to make this safe, we need to either verify
> that the accessing functions will only be called in this case (and add some
> DCHECKs to make that clear), or we should understand why they're accessing
> invisible objects, which seems very wrong.

Right. Please, could you take another look at patch set 2?

[Peter Kasting, 2011-08-11 20:53:31]

My comments below suggest that this class uses |url_tf_| in at least some cases
where it's invisible.  I think that says this class needs to be
well-thought-through to the point where the modifier thoroughly understands it.

http://codereview.chromium.org/7617006/diff/5001/chrome/browser/ui/views/book...
File chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc (right):

http://codereview.chromium.org/7617006/diff/5001/chrome/browser/ui/views/book...
chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc:474:
url_tf_->SetBackgroundColor(kErrorColor);
This doesn't seem safe.  UserInputChanged() can be called by Show() which surely
can be called in the NEW_FOLDER case.

http://codereview.chromium.org/7617006/diff/5001/chrome/browser/ui/views/book...
chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc:583: GURL
new_url(GetInputURL());
Similarly, ApplyEdits() can seemingly be called in almost any scenario, and
GetInputURL() blindly accesses |url_tf_|.

[tfarina, 2011-08-11 21:10:09]

http://codereview.chromium.org/7617006/diff/5001/chrome/browser/ui/views/book...
File chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc (right):

http://codereview.chromium.org/7617006/diff/5001/chrome/browser/ui/views/book...
chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc:474:
url_tf_->SetBackgroundColor(kErrorColor);
On 2011/08/11 20:53:31, Peter Kasting wrote:
> This doesn't seem safe.  UserInputChanged() can be called by Show() which
surely
> can be called in the NEW_FOLDER case.

Done.

http://codereview.chromium.org/7617006/diff/5001/chrome/browser/ui/views/book...
chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc:583: GURL
new_url(GetInputURL());
On 2011/08/11 20:53:31, Peter Kasting wrote:
> Similarly, ApplyEdits() can seemingly be called in almost any scenario, and
> GetInputURL() blindly accesses |url_tf_|.

Done.

[Peter Kasting, 2011-08-11 21:34:58]

LGTM.

There's a lot of sloppiness in this set of classes.  For example, NO_TREE is
only ever the case in tests -- which suggests it shouldn't be an option at all. 
A lot of bookmark_utils functions that take an EditDetails and then do
completely different things with different types should probably be split into
pieces.  This would ultimately allow us to e.g. avoid calling GetInputURL() in
this code when we're in the NEW_FOLDER case and don't actually care about it
anyway.  If you feel ambitious, feel free to start cleaning this stuff up.

http://codereview.chromium.org/7617006/diff/6003/chrome/browser/ui/views/book...
File chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc (right):

http://codereview.chromium.org/7617006/diff/6003/chrome/browser/ui/views/book...
chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc:372: ?
profile_->GetPrefs()->GetString(prefs::kAcceptLanguages)
Nit: Operators go on the ends of the previous lines

[tfarina, 2011-08-11 22:12:23]

http://codereview.chromium.org/7617006/diff/6003/chrome/browser/ui/views/book...
File chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc (right):

http://codereview.chromium.org/7617006/diff/6003/chrome/browser/ui/views/book...
chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc:372: ?
profile_->GetPrefs()->GetString(prefs::kAcceptLanguages)
On 2011/08/11 21:34:58, Peter Kasting wrote:
> Nit: Operators go on the ends of the previous lines

Done.