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.
+const int kUMAOptinViewBottomInset = 10;

[msw, 2014/09/18 23:14:48]

Move this down to where it's used and change the comment to explain why it's
necessary, the identifier already describes what it is.

[yao, 2014/09/19 15:12:27]

There is no particular reason, originally value 8 (from
views::kRelatedControlVerticalSpacing) was used, but the UI designer wants it to
be 10px now. 

+
 // 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 127 matching lines @@
     views::View* anchor_view,
     Browser* browser,
     content::WebContents* web_contents,
     bool offer_uma_optin)
     : BubbleDelegateView(anchor_view, views::BubbleBorder::TOP_RIGHT),
       content::WebContentsObserver(web_contents),
       browser_(browser),
       web_contents_(web_contents),
       restore_button_(NULL),
       uma_option_(NULL),
-      offer_uma_optin_(offer_uma_optin),
+      offer_uma_optin_(true),

[msw, 2014/09/18 23:14:48]

Why are you changing this? Was this intentional?

[yao, 2014/09/19 15:12:27]

Done.

       started_navigation_(false),
       restored_(false) {
   set_close_on_deactivate(false);
   registrar_.Add(
       this,
       chrome::NOTIFICATION_TAB_CLOSING,
       content::Source<content::NavigationController>(&(
           web_contents->GetController())));
   browser->tab_strip_model()->AddObserver(this);
 }
@@ skipping 113 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
+  // make them look aligned.
+  uma_label->SetBorder(views::Border::CreateEmptyBorder(1, 0, 0, 0));

[msw, 2014/09/18 23:14:48]

Why is this and the uma_layout inset change needed? Shouldn't one or the other
be sufficient?

[yao, 2014/09/19 15:12:26]

They are separate things. Changed the issue title to make this clearer.

This is for the checkbox alignment.

The inset change is to make the empty region at bottom of the UMA  opt-in view
higher (2 more pixels). 

 
   // 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;
 }