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 997 matching lines @@
     size.SetToMax(save_button_->GetPreferredSize());
   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;
+  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);
   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);
 
   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;
     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()) {

[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);
   dangerous_download_label_sized_ = true;
 }
 
 void DownloadItemView::Reenable() {
   disabled_while_opening_ = false;
   SetEnabled(true);  // Triggers a repaint.
 }
@@ skipping 38 matching lines @@
     SchedulePaint();
 }
 
 SkColor DownloadItemView::GetTextColor() const {
   return GetTextColorForThemeProvider(GetThemeProvider());
 }
 
 SkColor DownloadItemView::GetDimmedTextColor() const {
   return SkColorSetA(GetTextColor(), 0xC7);
 }