Fix SizeLabelToMinWidth() function such that no unnecessary space

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

+// Copyright 2016 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 "base/strings/utf_string_conversions.h"
+#include "content/public/test/mock_download_item.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/views/controls/label.h"
+#include "ui/views/test/views_test_base.h"
+
+class DownloadItemViewTest: public views::ViewsTestBase{

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

Nit: Space before ':' and '{'

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

Done.

+ public:
+  DownloadItemViewTest() {
+    base::FilePath empty_file_path;
+    EXPECT_CALL(download_, GetTargetFilePath())
+        .WillRepeatedly(testing::ReturnRef(empty_file_path));
+    view_ = new DownloadItemView(&download_, NULL);

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

Nit: nullptr (2 places)

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

Done.

+  }
+
+  ~DownloadItemViewTest() override {
+    delete view_->dangerous_download_label_;

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

This seems extremely dangerous.  |view_| should be responsible for its own
members.

I suggest making DownloadItemView construct this and add it as a child in its
own constructor.  Not only will this eliminate the need to manage it here, it
will eliminate the need to manually create it in
SetDownloadWarningLabelAndResize() below.

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

Changed DownloadItemView constructor and destructor accordingly. 
Also changed SetdownloadWarningLabelAndResize() to SetText instead of creating a
new label.

+    delete view_;

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

Eliminate the need for this by using a std::unique_ptr, or a member by value.

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

Done.

+    view_ = NULL;
+  }
+
+  void SetDownloadWarningLabelAndResize(const base::string16& label_text) {
+    view_->dangerous_download_label_sized_ = false;
+    view_->dangerous_download_label_ = new views::Label(label_text);
+    view_->dangerous_download_label_->SetMultiLine(true);
+    view_->dangerous_download_label_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+    view_->dangerous_download_label_->SetAutoColorReadabilityEnabled(false);
+    view_->SizeLabelToMinWidth();
+  }
+
+  base::string16 GetLabelText() {

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

Nit: Can be const

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

Done.

+    return view_->dangerous_download_label_->text();
+  }
+
+ private:
+  content::MockDownloadItem download_;
+  DownloadItemView* view_;
+};

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

Nit: DISALLOW_COPY_AND_ASSIGN

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

Done.

+
+// For very short label that can fit in a single line, no need to do any
+// adjustment, return it directly.
+TEST_F(DownloadItemViewTest, SizeLabelToMinWidth_VeryShortText) {
+  base::string16 label_text(base::ASCIIToUTF16("very short label"));

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

Nit: Prefer = to () for initializing strings (see
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variab...
)  (several places)

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

Thanks for the pointer!

+  SetDownloadWarningLabelAndResize(label_text);
+  EXPECT_EQ(label_text, GetLabelText());
+}
+
+// when we have multiple linebreaks that result in the same minimum width, we

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

Nit: Initial caps

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

Done.

+// should place as much text on the first line.

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

Nit: as much text -> as much text as possible

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

Done.

+TEST_F(DownloadItemViewTest, SizeLabelToMinWidth_MoreTextOnFirstLine) {
+  base::string16 label_text(
+      base::ASCIIToUTF16(
+          "aaaa aaaa aaaa aaaa aaaa aaaa bb aaaa aaaa aaaa aaaa aaaa aaaa"));
+  SetDownloadWarningLabelAndResize(label_text);
+  base::string16 expected_text(
+      base::ASCIIToUTF16("aaaa aaaa aaaa aaaa aaaa aaaa bb\n"
+                         "aaaa aaaa aaaa aaaa aaaa aaaa"));
+  EXPECT_EQ(expected_text, GetLabelText());
+}
+
+TEST_F(DownloadItemViewTest, SizeLabelToMinWidth_MaliciousDownloadWarnings) {

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

Nit: Comment about what specific part of the algorithm this tests?  (2 places)

(If it doesn't test anything specific, consider removing it, so we have the
minimum spanning set of tests)

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

Removed these two, and add another test to check how SizeLabelToMinWidth work
with extremely long "word"

+  base::string16 label_text(
+      base::ASCIIToUTF16(
+          "bad.exe is malicious, and Chrome has blocked it."));
+  SetDownloadWarningLabelAndResize(label_text);
+  base::string16 expected_text(
+      base::ASCIIToUTF16("bad.exe is malicious, and\n"
+                         "Chrome has blocked it."));
+  EXPECT_EQ(expected_text, GetLabelText());
+}
+
+TEST_F(DownloadItemViewTest, SizeLabelToMinWidth_UncommonDownloadWarnings) {
+  base::string16 label_text(
+      base::ASCIIToUTF16(
+          "bad.exe is not commonly downloaded and could be dangerous."));
+  SetDownloadWarningLabelAndResize(label_text);
+  base::string16 expected_text(
+      base::ASCIIToUTF16("bad.exe is not commonly\n"
+                         "downloaded and could be dangerous."));
+  EXPECT_EQ(expected_text, GetLabelText());
+}
+
+
+
+