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"
 #include "chrome/browser/bookmarks/bookmark_model.h"
 #include "chrome/browser/bookmarks/bookmark_utils.h"
 #include "chrome/browser/history/history.h"
 #include "chrome/browser/net/url_fixer_upper.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
 #include "googleurl/src/gurl.h"
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
 #include "grit/locale_settings.h"
 #include "net/base/net_util.h"
 #include "ui/base/l10n/l10n_util.h"
 #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),
       title_label_(NULL),
       parent_(parent),
       details_(details),
       running_menu_for_root_(false),
       show_tree_(configuration == SHOW_TREE) {
   DCHECK(profile);
   Init();
 }
 
 BookmarkEditorView::~BookmarkEditorView() {
@@ skipping 92 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 121 matching lines @@
     // 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(
+  url_label_.set_parent_owned(false);

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

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.

+  url_label_.SetText(
       UTF16ToWide(l10n_util::GetStringUTF16(IDS_BOOMARK_EDITOR_URL_LABEL)));
-  url_tf_.SetAccessibleName(WideToUTF16Hack(url_label_->GetText()));
+  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);
 
@@ skipping 10 matching lines @@
 
   layout->StartRow(0, labels_column_set_id);
 
   layout->AddView(title_label_);
   layout->AddView(&title_tf_);
 
   if (details_.type != EditDetails::NEW_FOLDER) {
     layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
 
     layout->StartRow(0, labels_column_set_id);
-    layout->AddView(url_label_);
+    layout->AddView(&url_label_);
     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 242 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);
 }