UI polish for certificate viewer

chrome/browser/chromeos/frame/bubble_frame_view.cc

Index: chrome/browser/chromeos/frame/bubble_frame_view.cc
diff --git a/chrome/browser/chromeos/frame/bubble_frame_view.cc b/chrome/browser/chromeos/frame/bubble_frame_view.cc
index e30bf316f2999033f860b5a18ab621e33560853d..0eb63171a02be00e37406612d10fa91269e5e026 100644
--- a/chrome/browser/chromeos/frame/bubble_frame_view.cc
+++ b/chrome/browser/chromeos/frame/bubble_frame_view.cc
@@ -28,6 +28,8 @@
 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;
 
@@ -58,7 +60,7 @@ BubbleFrameView::BubbleFrameView(views::Widget* frame,
     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,
@@ -145,10 +147,15 @@ 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() {
@@ -171,22 +178,47 @@ void BubbleFrameView::Layout() {
   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());
   }