views/bookmarks: Fix memory leak in BookmarkEditorView.

chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/bookmarks/bookmark_editor_view.h"
 
 #include "base/basictypes.h"
 #include "base/logging.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
@@ skipping 13 matching lines @@
 #include "views/background.h"
 #include "views/controls/button/text_button.h"
 #include "views/controls/label.h"
 #include "views/controls/menu/menu_2.h"
 #include "views/controls/textfield/textfield.h"
 #include "views/focus/focus_manager.h"
 #include "views/layout/grid_layout.h"
 #include "views/layout/layout_constants.h"
 #include "views/widget/widget.h"
 
-using views::Button;
-using views::ColumnSet;
 using views::GridLayout;
-using views::Label;
-using views::Textfield;
+
+namespace {
 
 // Background color of text field when URL is invalid.
-static const SkColor kErrorColor = SkColorSetRGB(0xFF, 0xBC, 0xBC);
+const SkColor kErrorColor = SkColorSetRGB(0xFF, 0xBC, 0xBC);
 
 // Preferred width of the tree.
-static const int kTreeWidth = 300;
+const int kTreeWidth = 300;
 
 // ID for various children.
-static const int kNewFolderButtonID = 1002;
+const int kNewFolderButtonID = 1002;
+
+}  // namespace
 
 // static
 void BookmarkEditor::Show(gfx::NativeWindow parent_hwnd,
                           Profile* profile,
                           const BookmarkNode* parent,
                           const EditDetails& details,
                           Configuration configuration) {
   DCHECK(profile);
   BookmarkEditorView* editor =
       new BookmarkEditorView(profile, parent, details, configuration);
   editor->Show(parent_hwnd);
 }
 
 BookmarkEditorView::BookmarkEditorView(
     Profile* profile,
     const BookmarkNode* parent,
     const EditDetails& details,
     BookmarkEditor::Configuration configuration)
     : profile_(profile),
       tree_view_(NULL),
       new_folder_button_(NULL),
       url_label_(NULL),
+      url_tf_(NULL),
       title_label_(NULL),
       parent_(parent),
       details_(details),
       running_menu_for_root_(false),
       show_tree_(configuration == SHOW_TREE) {
   DCHECK(profile);
   Init();
 }
 
 BookmarkEditorView::~BookmarkEditorView() {
@@ skipping 23 matching lines @@
 bool BookmarkEditorView::CanResize() const {
   return true;
 }
 
 std::wstring BookmarkEditorView::GetWindowTitle() const {
   return UTF16ToWide(l10n_util::GetStringUTF16(IDS_BOOMARK_EDITOR_TITLE));
 }
 
 bool BookmarkEditorView::Accept() {
   if (!IsDialogButtonEnabled(MessageBoxFlags::DIALOGBUTTON_OK)) {
-    // The url is invalid, focus the url field.
-    url_tf_.SelectAll();
-    url_tf_.RequestFocus();
+    if (details_.type != EditDetails::NEW_FOLDER) {
+      // The url is invalid, focus the url field.
+      url_tf_->SelectAll();
+      url_tf_->RequestFocus();
+    }
     return false;
   }
   // Otherwise save changes and close the dialog box.
   ApplyEdits();
   return true;
 }
 
 bool BookmarkEditorView::AreAcceleratorsEnabled(
     MessageBoxFlags::DialogButton button) {
   return !show_tree_ || !tree_view_->GetEditingNode();
@@ skipping 46 matching lines @@
     views::TreeView* tree_view) {
 }
 
 bool BookmarkEditorView::CanEdit(views::TreeView* tree_view,
                                  ui::TreeModelNode* node) {
   // Only allow editting of children of the bookmark bar node and other node.
   EditorNode* bb_node = tree_model_->AsNode(node);
   return (bb_node->parent() && bb_node->parent()->parent());
 }
 
-void BookmarkEditorView::ContentsChanged(Textfield* sender,
+void BookmarkEditorView::ContentsChanged(views::Textfield* sender,
                                          const std::wstring& new_contents) {
   UserInputChanged();
 }
 
-void BookmarkEditorView::ButtonPressed(
-    Button* sender, const views::Event& event) {
+void BookmarkEditorView::ButtonPressed(views::Button* sender,
+                                       const views::Event& event) {
   DCHECK(sender);
   switch (sender->id()) {
     case kNewFolderButtonID:
       NewFolder();
       break;
 
     default:
       NOTREACHED();
   }
 }
@@ skipping 82 matching lines @@
     context_menu_.reset(new views::Menu2(context_menu_contents_.get()));
   }
   context_menu_->RunContextMenuAt(p);
 }
 
 void BookmarkEditorView::Init() {
   bb_model_ = profile_->GetBookmarkModel();
   DCHECK(bb_model_);
   bb_model_->AddObserver(this);
 
-  url_tf_.set_parent_owned(false);
   title_tf_.set_parent_owned(false);
 
   std::wstring title;
   GURL url;
   if (details_.type == EditDetails::EXISTING_NODE) {
     title = details_.existing_node->GetTitle();
     url = details_.existing_node->url();
   } else if (details_.type == EditDetails::NEW_FOLDER) {
     title = UTF16ToWide(
         l10n_util::GetStringUTF16(IDS_BOOMARK_EDITOR_NEW_FOLDER_NAME));
   } else if (details_.type == EditDetails::NEW_URL) {
     string16 title16;
     bookmark_utils::GetURLAndTitleToBookmarkFromCurrentTab(profile_,
         &url, &title16);
     title = UTF16ToWide(title16);
   }
   title_tf_.SetText(title);
   title_tf_.SetController(this);
 
   title_label_ = new views::Label(
       UTF16ToWide(l10n_util::GetStringUTF16(IDS_BOOMARK_EDITOR_NAME_LABEL)));
   title_tf_.SetAccessibleName(WideToUTF16Hack(title_label_->GetText()));
 
-  string16 url_text;
-  if (details_.type != EditDetails::NEW_FOLDER) {
-    std::string languages = profile_
-        ? profile_->GetPrefs()->GetString(prefs::kAcceptLanguages)
-        : std::string();
-    // Because this gets parsed by FixupURL(), it's safe to omit the scheme or
-    // trailing slash, and unescape most characters, but we need to not drop any
-    // username/password, or unescape anything that changes the meaning.
-    url_text = net::FormatUrl(url, languages,
-        net::kFormatUrlOmitAll & ~net::kFormatUrlOmitUsernamePassword,
-        UnescapeRule::SPACES, NULL, NULL, NULL);
-  }
-  url_tf_.SetText(UTF16ToWide(url_text));
-  url_tf_.SetController(this);
-
-  url_label_ = new views::Label(
-      UTF16ToWide(l10n_util::GetStringUTF16(IDS_BOOMARK_EDITOR_URL_LABEL)));
-  url_tf_.SetAccessibleName(WideToUTF16Hack(url_label_->GetText()));
-
   if (show_tree_) {
     tree_view_ = new views::TreeView();
     tree_view_->set_lines_at_root(true);
     new_folder_button_.reset(new views::NativeTextButton(
         this,
         UTF16ToWide(l10n_util::GetStringUTF16(
             IDS_BOOMARK_EDITOR_NEW_FOLDER_BUTTON))));
     new_folder_button_->set_parent_owned(false);
     tree_view_->set_context_menu_controller(this);
 
     tree_view_->SetRootShown(false);
     new_folder_button_->SetEnabled(false);
     new_folder_button_->set_id(kNewFolderButtonID);
   }
 
   // Yummy layout code.
   GridLayout* layout = GridLayout::CreatePanel(this);
   SetLayoutManager(layout);
 
   const int labels_column_set_id = 0;
   const int single_column_view_set_id = 1;
   const int buttons_column_set_id = 2;
 
-  ColumnSet* column_set = layout->AddColumnSet(labels_column_set_id);
+  views::ColumnSet* column_set = layout->AddColumnSet(labels_column_set_id);
   column_set->AddColumn(GridLayout::LEADING, GridLayout::CENTER, 0,
                         GridLayout::USE_PREF, 0, 0);
   column_set->AddPaddingColumn(0, views::kRelatedControlHorizontalSpacing);
   column_set->AddColumn(GridLayout::FILL, GridLayout::CENTER, 1,
                         GridLayout::USE_PREF, 0, 0);
 
   column_set = layout->AddColumnSet(single_column_view_set_id);
   column_set->AddColumn(GridLayout::FILL, GridLayout::FILL, 1,
                         GridLayout::FIXED, kTreeWidth, 0);
 
   column_set = layout->AddColumnSet(buttons_column_set_id);
   column_set->AddColumn(GridLayout::FILL, GridLayout::LEADING, 0,
                         GridLayout::USE_PREF, 0, 0);
   column_set->AddPaddingColumn(1, views::kRelatedControlHorizontalSpacing);
   column_set->AddColumn(GridLayout::FILL, GridLayout::LEADING, 0,
                         GridLayout::USE_PREF, 0, 0);
   column_set->AddPaddingColumn(0, views::kRelatedControlHorizontalSpacing);
   column_set->AddColumn(GridLayout::FILL, GridLayout::LEADING, 0,
                         GridLayout::USE_PREF, 0, 0);
   column_set->LinkColumnSizes(0, 2, 4, -1);
 
   layout->StartRow(0, labels_column_set_id);
 
   layout->AddView(title_label_);
   layout->AddView(&title_tf_);
 
   if (details_.type != EditDetails::NEW_FOLDER) {
+    url_label_ = new views::Label(
+      UTF16ToWide(l10n_util::GetStringUTF16(IDS_BOOMARK_EDITOR_URL_LABEL)));
+
+    std::string languages = profile_
+        ? profile_->GetPrefs()->GetString(prefs::kAcceptLanguages)
+        : std::string();
+    // Because this gets parsed by FixupURL(), it's safe to omit the scheme or
+    // trailing slash, and unescape most characters, but we need to not drop any
+    // username/password, or unescape anything that changes the meaning.
+    string16 url_text = net::FormatUrl(url, languages,
+        net::kFormatUrlOmitAll & ~net::kFormatUrlOmitUsernamePassword,
+        UnescapeRule::SPACES, NULL, NULL, NULL);
+
+    url_tf_ = new views::Textfield;
+    url_tf_->SetText(UTF16ToWide(url_text));
+    url_tf_->SetController(this);
+    url_tf_->SetAccessibleName(WideToUTF16Hack(url_label_->GetText()));
+
     layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
 
     layout->StartRow(0, labels_column_set_id);
     layout->AddView(url_label_);
-    layout->AddView(&url_tf_);
+    layout->AddView(url_tf_);
   }
 
   if (show_tree_) {
     layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
     layout->StartRow(1, single_column_view_set_id);
     layout->AddView(tree_view_);
   }
 
   layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
 
@@ skipping 51 matching lines @@
   tree_model_.reset(new EditorTreeModel(root_node));
 
   tree_view_->SetModel(tree_model_.get());
   tree_view_->SetController(this);
 
   context_menu_.reset();
 
   if (parent())
     ExpandAndSelect();
 }
+
 GURL BookmarkEditorView::GetInputURL() const {
-  return URLFixerUpper::FixupURL(UTF16ToUTF8(url_tf_.text()), std::string());
+  return URLFixerUpper::FixupURL(UTF16ToUTF8(url_tf_->text()), std::string());
 }
 
 std::wstring BookmarkEditorView::GetInputTitle() const {
   return title_tf_.text();
 }
 
 void BookmarkEditorView::UserInputChanged() {
   const GURL url(GetInputURL());
   if (!url.is_valid())
-    url_tf_.SetBackgroundColor(kErrorColor);
+    url_tf_->SetBackgroundColor(kErrorColor);

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

This doesn't seem safe.  UserInputChanged() can be called by Show() which surely
can be called in the NEW_FOLDER case.

[tfarina, 2011/08/11 21:10:13]

Done.

   else
-    url_tf_.UseDefaultBackgroundColor();
+    url_tf_->UseDefaultBackgroundColor();
   GetDialogClientView()->UpdateDialogButtons();
 }
 
 void BookmarkEditorView::NewFolder() {
   // Create a new entry parented to the selected item, or the bookmark
   // bar if nothing is selected.
   EditorNode* parent = tree_model_->AsNode(tree_view_->GetSelectedNode());
   if (!parent) {
     NOTREACHED();
     return;
@@ skipping 86 matching lines @@
 }
 
 void BookmarkEditorView::ApplyEdits(EditorNode* parent) {
   DCHECK(!show_tree_ || parent);
 
   // We're going to apply edits to the bookmark bar model, which will call us
   // back. Normally when a structural edit occurs we reset the tree model.
   // We don't want to do that here, so we remove ourselves as an observer.
   bb_model_->RemoveObserver(this);
 
   GURL new_url(GetInputURL());

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

Similarly, ApplyEdits() can seemingly be called in almost any scenario, and
GetInputURL() blindly accesses |url_tf_|.

[tfarina, 2011/08/11 21:10:13]

Done.

   string16 new_title(WideToUTF16Hack(GetInputTitle()));
 
   if (!show_tree_) {
     bookmark_utils::ApplyEditsWithNoFolderChange(
         bb_model_, parent_, details_, new_title, new_url);
     return;
   }
 
   // Create the new folders and update the titles.
   const BookmarkNode* new_parent = NULL;
@@ skipping 49 matching lines @@
     EditorNode* editor_node,
     BookmarkExpandedStateTracker::Nodes* expanded_nodes) {
   if (!tree_view_->IsExpanded(editor_node))
     return;
 
   if (editor_node->value != 0)  // The root is 0
     expanded_nodes->insert(bb_model_->GetNodeByID(editor_node->value));
   for (int i = 0; i < editor_node->child_count(); ++i)
     UpdateExpandedNodes(editor_node->GetChild(i), expanded_nodes);
 }