UI polish for certificate viewer

chrome/browser/chromeos/frame/bubble_frame_view.cc

 // Copyright (c) 2011 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/chromeos/frame/bubble_frame_view.h"
 
 #include <algorithm>
 
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/chromeos/frame/bubble_window.h"
@@ skipping 10 matching lines @@
 #include "views/controls/throbber.h"
 #include "views/widget/widget.h"
 #include "views/widget/widget_delegate.h"
 #include "views/window/hit_test.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "third_party/skia/include/core/SkPaint.h"
 
 namespace {
 
 const int kTitleTopPadding = 10;
+const int kTitleLeftPadding = 10;
+const int kTitleRightPadding = 10;

[flackr, 2011/11/10 16:03:16]

I don't think there's a need for these extra contents, most likely we will
always want the title to be padded the same as if we didn't apply this new style
(i.e. re-use kHorizontalPadding.)

[bshe, 2011/11/21 16:09:37]

Done.

 const int kTitleContentPadding = 10;
 const int kHorizontalPadding = 10;
 
 // Title font size correction.
 #if defined(CROS_FONTS_USING_BCI)
 const int kTitleFontSizeDelta = 0;
 #else
 const int kTitleFontSizeDelta = 1;
 #endif
 
@@ skipping 10 matching lines @@
       close_button_(NULL),
       throbber_(NULL) {
   if (widget_delegate->ShouldShowWindowTitle()) {
     title_ = new views::Label(widget_delegate->GetWindowTitle());
     title_->SetHorizontalAlignment(views::Label::ALIGN_LEFT);
     title_->SetFont(title_->font().DeriveFont(kFontSizeCorrectionDelta,
                                               gfx::Font::BOLD));
     AddChildView(title_);
   }
 
-  if (style_ & STYLE_XBAR) {
+  if (style_ & STYLE_XBAR || style_ & STYLE_CERT) {

[flackr, 2011/11/10 16:03:16]

Your new style should only be for the custom padding.

[bshe, 2011/11/21 16:09:37]

Done.

     ResourceBundle& rb = ResourceBundle::GetSharedInstance();
     close_button_ = new views::ImageButton(this);
     close_button_->SetImage(views::CustomButton::BS_NORMAL,
         rb.GetBitmapNamed(IDR_CLOSE_BAR));
     close_button_->SetImage(views::CustomButton::BS_HOT,
         rb.GetBitmapNamed(IDR_CLOSE_BAR_H));
     close_button_->SetImage(views::CustomButton::BS_PUSHED,
         rb.GetBitmapNamed(IDR_CLOSE_BAR_P));
     AddChildView(close_button_);
   }
@@ skipping 66 matching lines @@
 
 void BubbleFrameView::ResetWindowControls() {
 }
 
 void BubbleFrameView::UpdateWindowIcon() {
 }
 
 gfx::Insets BubbleFrameView::GetInsets() const {
   return (style_ & STYLE_FLUSH) ?
          gfx::Insets() :
-         gfx::Insets(kTitleTopPadding,
-                     kHorizontalPadding,
-                     0,
-                     kHorizontalPadding);
+         ((style_ & STYLE_CERT) ?
+          gfx::Insets(kTitleTopPadding,
+                      0,
+                      0,
+                      0) :
+          gfx::Insets(kTitleTopPadding,
+                      kHorizontalPadding,
+                      0,
+                      kHorizontalPadding));
 }
 
 gfx::Size BubbleFrameView::GetPreferredSize() {
   gfx::Size pref = frame_->client_view()->GetPreferredSize();
   gfx::Rect bounds(0, 0, pref.width(), pref.height());
   return frame_->non_client_view()->GetWindowBoundsForClientBounds(
       bounds).size();
 }
 
 void BubbleFrameView::Layout() {
   gfx::Insets insets = GetInsets();
 
   gfx::Size title_size;
   if (title_)
     title_size = title_->GetPreferredSize();
   gfx::Size close_button_size;
   if (close_button_)
     close_button_size = close_button_->GetPreferredSize();
   gfx::Size throbber_size;
   if (throbber_)
     throbber_size = throbber_->GetPreferredSize();
 
+  // Need to center elements which are shorter.
+  int max_height = std::max(title_size.height(),
+                            std::max(close_button_size.height(),
+                                     throbber_size.height()));

[flackr, 2011/11/10 16:03:16]

The following cases for STYLE_CERT seem to introduce a lot of duplicate code. I
suggest you set up gfx::Insets titleBarInsets here and then you can use it in
the following code and not require two cases.

[bshe, 2011/11/21 16:09:37]

I use titleBarInsets at first. Then I figure out the XBAR style already adds
padding to title. We just need to offset the content area padding in this case.
So instead of using title padding, I use an gfx::Insets offset_insets to flush
content area. 

+
   if (title_) {
-    title_->SetBounds(
-        insets.left(), insets.top(),
-        std::max(0, width() - insets.width() - close_button_size.width()),
-        title_size.height());
+    // Insert left padding to title area.
+    if (style_ & STYLE_CERT)
+      title_->SetBounds(
+          insets.left() + kTitleLeftPadding,
+          insets.top() + ((max_height - title_size.height()) >> 1), // Center

[flackr, 2011/11/10 16:03:16]

Are divisions by 2 of this style (>> 1) used elsewhere? I think any compiler
will have no trouble optimizing an integer divide by constant 2 to this and it
will be more readable to just divide by 2.

[bshe, 2011/11/21 16:09:37]

Done.

+          std::max(0, width() - insets.width() - close_button_size.width()
+                   - kTitleLeftPadding - kTitleRightPadding),
+          title_size.height());
+    else
+      title_->SetBounds(
+          insets.left(),
+          insets.top() + ((max_height - title_size.height()) >> 1),
+          std::max(0, width() - insets.width() - close_button_size.width()),
+          title_size.height());
   }
 
   if (close_button_) {
-    close_button_->SetBounds(
-        width() - insets.right() - close_button_size.width(), insets.top(),
-        close_button_size.width(), close_button_size.height());
+    // Insert right padding to title area.
+    if (style_ & STYLE_CERT)
+      close_button_->SetBounds(
+          width() - insets.right() - close_button_size.width()
+              - kTitleRightPadding,
+          insets.top() + ((max_height - close_button_size.height()) >> 1),
+          close_button_size.width(), close_button_size.height());
+    else
+      close_button_->SetBounds(
+          width() - insets.right() - close_button_size.width(),
+          insets.top() + ((max_height - close_button_size.height()) >> 1),
+          close_button_size.width(), close_button_size.height());
   }
 
   if (throbber_) {
     throbber_->SetBounds(
-        insets.left(), insets.top(),
+        insets.left(),
+        insets.top() + ((max_height - throbber_size.height()) >> 1),
         std::min(throbber_size.width(), width()),
         throbber_size.height());
   }
 
   int top_height = insets.top();
   if (title_size.height() > 0 ||
       close_button_size.height() > 0 ||
       throbber_size.height() > 0) {
     top_height += kTitleContentPadding + std::max(
         std::max(title_size.height(), close_button_size.height()),
@@ skipping 17 matching lines @@
   canvas->GetSkCanvas()->drawPath(path, paint);
 }
 
 void BubbleFrameView::ButtonPressed(views::Button* sender,
                                     const views::Event& event) {
   if (close_button_ != NULL && sender == close_button_)
     frame_->Close();
 }
 
 }  // namespace chromeos