Fix SizeLabelToMinWidth() function such that no unnecessary space

chrome/browser/ui/views/download/download_item_view.cc

 // Copyright (c) 2012 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/download/download_item_view.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <vector>
@@ skipping 165 matching lines @@
     : shelf_(parent),
       status_text_(l10n_util::GetStringUTF16(IDS_DOWNLOAD_STATUS_STARTING)),
       dropdown_state_(NORMAL),
       mode_(NORMAL_MODE),
       dragging_(false),
       starting_drag_(false),
       model_(download_item),
       save_button_(nullptr),
       discard_button_(nullptr),
       dropdown_button_(new DropDownButton(this)),
-      dangerous_download_label_(nullptr),
+      dangerous_download_label_(new views::Label()),
       dangerous_download_label_sized_(false),
       disabled_while_opening_(false),
       creation_time_(base::Time::Now()),
       time_download_warning_shown_(base::Time()),
       weak_ptr_factory_(this) {
   SetInkDropMode(InkDropMode::ON);
   DCHECK(download());
   download()->AddObserver(this);
   set_context_menu_controller(this);
 
@@ skipping 19 matching lines @@
 
 DownloadItemView::~DownloadItemView() {
   StopDownloadProgress();
   download()->RemoveObserver(this);
 
   // ExperienceSampling: If the user took no action to remove the warning
   // before it disappeared, then the user effectively dismissed the download
   // without keeping it.
   if (sampling_event_)
     sampling_event_->CreateUserDecisionEvent(ExperienceSamplingEvent::kIgnore);
+
+  delete dangerous_download_label_;
+  dangerous_download_label_ = nullptr;

[Peter Kasting, 2016/12/08 05:10:23]

This potentially double-frees.  Views is expecting to own the pointer you pass
to AddChildView().  You could get around this by marking it as "owned by parent"
(and changing the class member to a unique_ptr), but the best way, I think, is
to move _all_ the code for |dangerous_download_label_| to the constructor,
including the AddChildView() call, and then either moving the other calls that
add children there as well, or else modifying them so they insert their children
in a specific place in the child list instead of at the end using
AddChildViewAt().

[Jialiu Lin, 2016/12/08 07:47:40]

Ack.

 }
 
 // Progress animation handlers.
 
 void DownloadItemView::StartDownloadProgress() {
   if (progress_timer_.IsRunning())
     return;
   progress_start_time_ = base::TimeTicks::Now();
   progress_timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(
                                        DownloadShelf::kProgressRateMs),
@@ skipping 100 matching lines @@
   base::string16 new_tip = model_.GetTooltipText(font_list_, kTooltipMaxWidth);
   if (new_tip != tooltip_text_) {
     tooltip_text_ = new_tip;
     TooltipTextChanged();
   }
 
   UpdateAccessibleName();
 }
 
 void DownloadItemView::OnDownloadDestroyed(DownloadItem* download) {
-  shelf_->RemoveDownloadView(this);  // This will delete us!
+  if (shelf_)
+    shelf_->RemoveDownloadView(this);  // This will delete us!
 }
 
 void DownloadItemView::OnDownloadOpened(DownloadItem* download) {
   disabled_while_opening_ = true;
   SetEnabled(false);
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE,
       base::Bind(&DownloadItemView::Reenable, weak_ptr_factory_.GetWeakPtr()),
       base::TimeDelta::FromMilliseconds(kDisabledOnOpenDuration));
 
@@ skipping 431 matching lines @@
     DownloadCommands::Command download_command,
     bool feedback_enabled) {
   if (feedback_enabled && SubmitDownloadToFeedbackService(download_command))
     return;
 
   DownloadCommands(download()).ExecuteCommand(download_command);
   // WARNING: 'this' is deleted at this point. Don't access 'this'.
 }
 
 void DownloadItemView::LoadIcon() {
-  IconManager* im = g_browser_process->icon_manager();
-  last_download_item_path_ = download()->GetTargetFilePath();
-  im->LoadIcon(last_download_item_path_, IconLoader::SMALL,
-               base::Bind(&DownloadItemView::OnExtractIconComplete,
-                          base::Unretained(this)),
-               &cancelable_task_tracker_);
+  // If there is no download shelf assigned, this is called in a test, then no
+  // need to load the icon.

[Peter Kasting, 2016/12/08 05:10:23]

Does it actually hurt to load this icon in a test?

If not, I would just do that.

If so, maybe we should not key off |shelf_|, which seems like an indirect way to
be checking that this is in a test?  Should there be some kind of variable that
directly says if we're in a test?

[Jialiu Lin, 2016/12/08 07:47:40]

Unfortunately LoadIcon function involves shelf_. So in the new constructor, this
function will not be called. 

+  if (shelf_) {
+    IconManager* im = g_browser_process->icon_manager();
+    last_download_item_path_ = download()->GetTargetFilePath();
+    im->LoadIcon(last_download_item_path_, IconLoader::SMALL,
+                 base::Bind(&DownloadItemView::OnExtractIconComplete,
+                            base::Unretained(this)),
+                 &cancelable_task_tracker_);
+  }
 }
 
 void DownloadItemView::LoadIconIfItemPathChanged() {
   base::FilePath current_download_path = download()->GetTargetFilePath();
   if (last_download_item_path_ == current_download_path)
     return;
 
   LoadIcon();
 }
 
@@ skipping 149 matching lines @@
     save_button_ =
         views::MdTextButton::Create(this, model_.GetWarningConfirmButtonText());
     AddChildView(save_button_);
   }
   discard_button_ = views::MdTextButton::Create(
       this, l10n_util::GetStringUTF16(IDS_DISCARD_DOWNLOAD));
   AddChildView(discard_button_);
 
   base::string16 dangerous_label =
       model_.GetWarningText(font_list_, kTextWidth);
-  dangerous_download_label_ = new views::Label(dangerous_label);
+  dangerous_download_label_->SetText(dangerous_label);
   dangerous_download_label_->SetMultiLine(true);
   dangerous_download_label_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   dangerous_download_label_->SetAutoColorReadabilityEnabled(false);
   AddChildView(dangerous_download_label_);
   SizeLabelToMinWidth();
 
   dropdown_button_->SetVisible(mode_ == MALICIOUS_MODE);
 }
 
 gfx::ImageSkia DownloadItemView::GetWarningIcon() {
@@ skipping 25 matching lines @@
   return size;
 }
 
 // This method computes the minimum width of the label for displaying its text
 // on 2 lines.  It just breaks the string in 2 lines on the spaces and keeps the
 // configuration with minimum width.
 void DownloadItemView::SizeLabelToMinWidth() {
   if (dangerous_download_label_sized_)
     return;
 
+  dangerous_download_label_->SetSize(GetLabelSize());
+  dangerous_download_label_sized_ = true;
+}
+
+gfx::Size DownloadItemView::GetLabelSize() {
+  gfx::Size size = dangerous_download_label_->GetPreferredSize();
+
+  // If the label is already narrower than kDangerousTextWidth, we don't need to
+  // linebreak it, as it will fit on a single line.
+  if (size.width() <= kDangerousTextWidth)
+    return size;
+
   base::string16 label_text = dangerous_download_label_->text();
   base::TrimWhitespace(label_text, base::TRIM_ALL, &label_text);
   DCHECK_EQ(base::string16::npos, label_text.find('\n'));
 
   // Make the label big so that GetPreferredSize() is not constrained by the
   // current width.
   dangerous_download_label_->SetBounds(0, 0, 1000, 1000);
 
   // Use a const string from here. BreakIterator requies that text.data() not
   // change during its lifetime.
   const base::string16 original_text(label_text);
   // Using BREAK_WORD can work in most cases, but it can also break
   // lines where it should not. Using BREAK_LINE is safer although
   // slower for Chinese/Japanese. This is not perf-critical at all, though.
   base::i18n::BreakIterator iter(original_text,
                                  base::i18n::BreakIterator::BREAK_LINE);
   bool status = iter.Init();
   DCHECK(status);
 
+  // Go through the string and try each line break (starting with no line break)
+  // searching for the optimal line break position. Stop if we find one that
+  // yields minimum label width.
   base::string16 prev_text = original_text;
-  gfx::Size size = dangerous_download_label_->GetPreferredSize();
-  int min_width = size.width();
-
-  // Go through the string and try each line break (starting with no line break)
-  // searching for the optimal line break position.  Stop if we find one that
-  // yields one that is less than kDangerousTextWidth wide.  This is to prevent
-  // a short string (e.g.: "This file is malicious") from being broken up
-  // unnecessarily.
-  while (iter.Advance() && min_width > kDangerousTextWidth) {
+  for (gfx::Size min_width_size = size; iter.Advance(); min_width_size = size) {
     size_t pos = iter.pos();
     if (pos >= original_text.length())
       break;
     base::string16 current_text = original_text;
     // This can be a low surrogate codepoint, but u_isUWhiteSpace will
     // return false and inserting a new line after a surrogate pair
     // is perfectly ok.
     base::char16 line_end_char = current_text[pos - 1];
     if (u_isUWhiteSpace(line_end_char))
       current_text.replace(pos - 1, 1, 1, base::char16('\n'));
     else
       current_text.insert(pos, 1, base::char16('\n'));
     dangerous_download_label_->SetText(current_text);
     size = dangerous_download_label_->GetPreferredSize();
 
     // If the width is growing again, it means we passed the optimal width spot.
-    if (size.width() > min_width) {
+    if (size.width() > min_width_size.width()) {
       dangerous_download_label_->SetText(prev_text);
-      break;
-    } else {
-      min_width = size.width();
+      return min_width_size;
     }
     prev_text = current_text;
   }
-
-  dangerous_download_label_->SetSize(size);
-  dangerous_download_label_sized_ = true;
+  return size;
 }
 
 void DownloadItemView::Reenable() {
   disabled_while_opening_ = false;
   SetEnabled(true);  // Triggers a repaint.
 }
 
 void DownloadItemView::ReleaseDropdown() {
   SetDropdownState(NORMAL);
 }
@@ skipping 34 matching lines @@
     SchedulePaint();
 }
 
 SkColor DownloadItemView::GetTextColor() const {
   return GetTextColorForThemeProvider(GetThemeProvider());
 }
 
 SkColor DownloadItemView::GetDimmedTextColor() const {
   return SkColorSetA(GetTextColor(), 0xC7);
 }