Fix SizeLabelToMinWidth() function such that no unnecessary space

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

Index: chrome/browser/ui/views/download/download_item_view.cc
diff --git a/chrome/browser/ui/views/download/download_item_view.cc b/chrome/browser/ui/views/download/download_item_view.cc
index 99b5344d90e8e6a49eaaed4000c68bd01650753a..1f480e4c2e041254e6b12fdd016db57bb6682a7f 100644
--- a/chrome/browser/ui/views/download/download_item_view.cc
+++ b/chrome/browser/ui/views/download/download_item_view.cc
@@ -343,7 +343,8 @@ void DownloadItemView::OnDownloadUpdated(DownloadItem* download_item) {
 }
 
 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) {
@@ -795,12 +796,16 @@ void DownloadItemView::SubmitDownloadWhenFeedbackServiceEnabled(
 }
 
 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.
+  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() {
@@ -1015,6 +1020,14 @@ gfx::Size DownloadItemView::GetButtonSize() const {
 void DownloadItemView::SizeLabelToMinWidth() {
   if (dangerous_download_label_sized_)
     return;
+  gfx::Size size = dangerous_download_label_->GetPreferredSize();
+  dangerous_download_label_->SetSize(size);
+  dangerous_download_label_sized_ = true;
+
+  // 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;
 
   base::string16 label_text = dangerous_download_label_->text();
   base::TrimWhitespace(label_text, base::TRIM_ALL, &label_text);
@@ -1035,16 +1048,11 @@ void DownloadItemView::SizeLabelToMinWidth() {
   bool status = iter.Init();
   DCHECK(status);
 
-  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) {
+  // searching for the optimal line break position. Stop if we find one that
+  // yields minimum label width.
+  base::string16 prev_text = original_text;
+  for (gfx::Size min_width_size = size; iter.Advance(); min_width_size = size) {
     size_t pos = iter.pos();
     if (pos >= original_text.length())
       break;
@@ -1061,17 +1069,13 @@ void DownloadItemView::SizeLabelToMinWidth() {
     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();
+      dangerous_download_label_->SetSize(min_width_size);
+      return;
     }
     prev_text = current_text;
   }

[Peter Kasting, 2016/12/08 01:05:56]

If this loop terminates (either due to the condition failing or due to the
break), we definitely won't have called SetSize() on
|dangerous_download_label_|.  (We might not have called SetText(), but I think
the cases where we didn't are ones where the existing text was fine.)

This seems like a bug (and probably one the tests should catch?).  It also seems
like more motivation to try and return the size from a separate function, so we
can't miss this?

[Jialiu Lin, 2016/12/08 04:51:58]

Oops, you're right. 

-
-  dangerous_download_label_->SetSize(size);
-  dangerous_download_label_sized_ = true;
 }
 
 void DownloadItemView::Reenable() {