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..32a6e0aecb50607b4b660f07d256e4510eb6d8ab 100644
--- a/chrome/browser/ui/views/download/download_item_view.cc
+++ b/chrome/browser/ui/views/download/download_item_view.cc
@@ -1015,6 +1015,15 @@ gfx::Size DownloadItemView::GetButtonSize() const {
 void DownloadItemView::SizeLabelToMinWidth() {
   if (dangerous_download_label_sized_)
     return;
+  gfx::Size size = dangerous_download_label_->GetPreferredSize();
+  // If the size of the label already smaller than kDangerousDownload, it can
+  // fit into a single line. This is to prevent a short string (e.g.: "This file
+  // is malicious") from being broken up unnecessarily.

[Peter Kasting, 2016/12/06 05:48:20]

Nit: Grammar and constant naming problems.  How about just:

If the label is already narrower than kDangerousTextWidth, we don't need to
linebreak it, as it will fit on a single line.

[Jialiu Lin, 2016/12/06 23:42:33]

Ah.. auto complete.. Thanks for catching this. 

+  if (size.width() <= kDangerousTextWidth) {
+    dangerous_download_label_->SetSize(size);
+    dangerous_download_label_sized_ = true;

[Peter Kasting, 2016/12/06 05:48:20]

Nit: Instead of doing this here, set it immediately after the conditional atop
the function, then we don't need to remember to set it on every exit.

Another way: split this into two functions, like:

void DownloadItemView::SizeLabelToMinWidth() {
  if (dangerous_download_label_sized_)
    return;
  dangerous_download_label_->SetSize(GetLabelSize());
  dangerous_download_label_sized_ = true;
}

gfx::Size DownloadItemView::GetLabelSize() {
  ... most of the rest of this function

The advantage of this is that you can just return the desired size immediately
from places like this or the loop below, instead of having to do cleanup code.

[Jialiu Lin, 2016/12/06 23:42:33]

There are actually two things to be set in this function: the label size, and
the text (with "\n" inserted to break the line).  So it might make more sense to
set them in a single function. 

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

Return a tuple?

Return the text to use, and recalculate the preferred size on the caller side
(wastes a bit of effort)?

But maybe the answer is to set the text within said function, but return the
size to be set by the parent, since we don't always need to reset the text.  See
later patchset comments...

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

Done.

+    return;
+  }
 
   base::string16 label_text = dangerous_download_label_->text();
   base::TrimWhitespace(label_text, base::TRIM_ALL, &label_text);
@@ -1036,15 +1045,14 @@ void DownloadItemView::SizeLabelToMinWidth() {
   DCHECK(status);
 
   base::string16 prev_text = original_text;
-  gfx::Size size = dangerous_download_label_->GetPreferredSize();
-  int min_width = size.width();
+

[Peter Kasting, 2016/12/06 05:48:20]

Nit: Use at most one blank line at a time within a function to minimize use of
vertical whitespace. (See
http://google.github.io/styleguide/cppguide.html#Vertical_Whitespace .)

But here I would put zero blank lines as both these variables relate to each
other.  They also relate to the loop below, so I'd actually declare them below
the comment instead of above it (with no blank lines around them).

At that point it becomes clear that the loop could be written like this,
reducing the body of the loop and scoping |min_size_width| as narrowly as
possible (see http://google.github.io/styleguide/cppguide.html#Local_Variables
):

  for (gfx::Size min_width_size = size; iter.Advance(); min_width_size = size) {

[Jialiu Lin, 2016/12/06 23:42:33]

Done.

+
+  gfx::Size min_width_size = size;
 
   // 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.
+  while (iter.Advance()) {
     size_t pos = iter.pos();
     if (pos >= original_text.length())
       break;
@@ -1061,13 +1069,15 @@ 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()) {

[Peter Kasting, 2016/12/06 05:48:20]

Changing this to >= seems worse.  The effect is that when we have multiple
linebreaks that result in the same minimum width, we'll choose to break after
the first rather than the last.  Ideally, we should place as much text on the
first line as possible subject to the algorithmic constraints, so using '>'
would be better at doing that.

[Jialiu Lin, 2016/12/06 23:42:33]

Agree, change to '>' will place as much text on the first line as possible. 

       dangerous_download_label_->SetText(prev_text);
-      break;
+      dangerous_download_label_->SetSize(min_width_size);
+      dangerous_download_label_sized_ = true;
+      return;
     } else {

[Peter Kasting, 2016/12/06 05:48:20]

Nit: No else after return

[Jialiu Lin, 2016/12/06 23:42:33]

Done.

-      min_width = size.width();
+      min_width_size = size;
+      prev_text = current_text;
     }
-    prev_text = current_text;
   }
 
   dangerous_download_label_->SetSize(size);