Checkbox alignment & bigger height for Uma opt-in view.

chrome/browser/ui/views/session_crashed_bubble_view.cc

 // Copyright 2014 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/session_crashed_bubble_view.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 37 matching lines @@
 using views::GridLayout;
 
 namespace {
 
 // Fixed width of the column holding the description label of the bubble.
 const int kWidthOfDescriptionText = 320;
 
 // Distance between checkbox and the text to the right of it.
 const int kCheckboxTextDistance = 4;
 
+// Bottom inset for UMA opt-in view in pixels.

[msw, 2014/09/19 18:01:48]

nit: be consistent about "optin" vs. "opt-in" in comments.

[yao, 2014/10/09 20:38:40]

Done.

+const int kUMAOptinViewBottomInset = 10;

[msw, 2014/09/19 18:01:48]

nit: This could be moved into the scope of the one function where it's used,
immediately preceding its use.

[yao, 2014/10/09 20:38:40]

Done.

+
 // The color of the text and background of the sub panel to offer UMA optin.
 // These values match the BookmarkSyncPromoView colors.
 const SkColor kBackgroundColor = SkColorSetRGB(245, 245, 245);
 const SkColor kTextColor = SkColorSetRGB(102, 102, 102);
 
 // The Finch study name and group name that enables session crashed bubble UI.
 const char kEnableBubbleUIFinchName[] = "EnableSessionCrashedBubbleUI";
 const char kEnableBubbleUIGroupEnabled[] = "Enabled";
 
 enum SessionCrashedBubbleHistogramValue {
@@ skipping 261 matching lines @@
   uma_label->AddStyleRange(gfx::Range(offset, offset + link_text.length()),
                            link_style);
   views::StyledLabel::RangeStyleInfo uma_style;
   uma_style.color = kTextColor;
   gfx::Range before_link_range(0, offset);
   if (!before_link_range.is_empty())
     uma_label->AddStyleRange(before_link_range, uma_style);
   gfx::Range after_link_range(offset + link_text.length(), uma_text.length());
   if (!after_link_range.is_empty())
     uma_label->AddStyleRange(after_link_range, uma_style);
+  // Since the checkbox has a bigger height than text, moves it down by 1px to

[msw, 2014/09/19 18:01:48]

nit: consider "Shift the text down by 1px to align with the checkbox."

[yao, 2014/10/09 20:38:40]

Done.

+  // make them look aligned.
+  uma_label->SetBorder(views::Border::CreateEmptyBorder(1, 0, 0, 0));

[sky, 2014/09/22 15:06:34]

This only works with the default font on windows.
If we're we want the checkbox image to go from the font baseline to the font
ascent, then we'll need to draw the image in code.
I don't think you should worry about futzing with the checkbox image here. It
should be done in the checkbox code.

 
   // Create a view to hold the checkbox and the text.
   views::View* uma_view = new views::View();
   GridLayout* uma_layout = new GridLayout(uma_view);
   uma_view->SetLayoutManager(uma_layout);
 
   uma_view->set_background(
       views::Background::CreateSolidBackground(kBackgroundColor));
   int inset_left = GetBubbleFrameView()->GetTitleInsets().left();
   uma_layout->SetInsets(views::kRelatedControlVerticalSpacing, inset_left,
-                        views::kRelatedControlVerticalSpacing, inset_left);
+                        kUMAOptinViewBottomInset, inset_left);
 
   const int kReportColumnSetId = 0;
   views::ColumnSet* cs = uma_layout->AddColumnSet(kReportColumnSetId);
   cs->AddColumn(GridLayout::CENTER, GridLayout::LEADING, 0,
                 GridLayout::USE_PREF, 0, 0);
   cs->AddPaddingColumn(0, kCheckboxTextDistance);
   cs->AddColumn(GridLayout::FILL, GridLayout::FILL, 0,
                 GridLayout::FIXED, kWidthOfDescriptionText, 0);
 
   uma_layout->StartRow(0, kReportColumnSetId);
@@ skipping 73 matching lines @@
   GetWidget()->Close();
 }
 
 bool ShowSessionCrashedBubble(Browser* browser) {
   if (IsBubbleUIEnabled()) {
     SessionCrashedBubbleView::Show(browser);
     return true;
   }
   return false;
 }