MacViews: draw Mac combobox arrows

ui/views/controls/combobox/combobox.cc

 // Copyright (c) 2012 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 "ui/views/controls/combobox/combobox.h"
 
 #include <stddef.h>
 
 #include <utility>
 
@@ skipping 734 matching lines @@
   AdjustBoundsForRTLUI(&text_bounds);
   canvas->DrawStringRect(text, font_list, text_color, text_bounds);
 
   int arrow_x = disclosure_arrow_offset + GetDisclosureArrowLeftPadding();
   gfx::Rect arrow_bounds(arrow_x,
                          height() / 2 - arrow_size.height() / 2,
                          arrow_size.width(),
                          arrow_size.height());
   AdjustBoundsForRTLUI(&arrow_bounds);
 
-  // TODO(estade): hack alert! Remove this direct call into CommonTheme. For now
-  // STYLE_ACTION isn't properly themed so we have to override the NativeTheme
-  // behavior. See crbug.com/384071
-  if (style_ == STYLE_ACTION) {
-    ui::CommonThemePaintComboboxArrow(canvas->sk_canvas(), arrow_bounds);
-  } else {
-    ui::NativeTheme::ExtraParams ignored;
-    GetNativeTheme()->Paint(canvas->sk_canvas(),
-                            ui::NativeTheme::kComboboxArrow,
-                            ui::NativeTheme::kNormal,
-                            arrow_bounds,
-                            ignored);
-  }
+  gfx::ImageSkia arrow_image = PlatformStyle::CreateComboboxArrow(
+      enabled(), style_);
+  gfx::Rect image_bounds(arrow_bounds);
+  image_bounds.ClampToCenteredSize(arrow_bounds.size());
+  canvas->DrawImageInt(arrow_image, image_bounds.x(), image_bounds.y());

[tapted, 2016/04/05 08:45:02]

I have a theory, that you could write something like

DCHECK_EQ(image_bounds.x(), arrow_bounds.x());
DCHECK_EQ(image_bounds.y(), arrow_bounds.y());

and it would always be true on all platforms. In which case, we can just pass
arrow_bounds.x/y() and remove lines 757-8.  (don't keep the DCHECK though -
that's just to test the theory :).

And, actually, I wanted to take some screenshots with this patched in, so I
decided just to test that hypothesis in
https://codereview.chromium.org/1863503002/#ps40001 :).  (So, if bots there are
green we can remove lines 758-9 and just use arrow_bounds.{x,y}() here.)

[Elly Fong-Jones, 2016/04/05 20:04:24]

It looks like it exploded on the Windows bots.

[tapted, 2016/04/06 00:07:08]

https://codereview.chromium.org/1863503002/#ps60001 has the green runs, and the
DCHECK

The theory is based on a deskcheck of the code here too - 

line 735 does

gfx::Size arrow_size = ArrowSize();

i.e.

gfx::Size arrow_size = CreateComboboxArrow(..).size();

Then its value is unchanged by the time it gets to

gfx::Rect arrow_bounds(.., .., arrow_size.width(), arrow_size.height);

AdjustBoundsForRTLUI shouldn't affect the size, so we're left with

arrow_bounds.size() == CreateComboboxArrow(..).size() == image_bounds.size().

And if the sizes are equal, then ClampToCenteredSize(..) should be a no-op.



If I've missed something, we should put a comment why we need to call
ClampToCenteredSize, since it's not obvious to me why it's needed (and I'm
pretty sure it isn't :).

[Elly Fong-Jones, 2016/04/06 16:20:21]

Ya, sorry - I should have realized that these two lines:

  gfx::Rect image_bounds(arrow_bounds);
  image_bounds.ClampToCenteredSize(arrow_bounds.size());

are an obvious noop, since if image_bounds == arrow_bounds, it's already
centered inside arrow_bounds. Removed them.

 }
 
 void Combobox::PaintButtons(gfx::Canvas* canvas) {
   DCHECK(style_ == STYLE_ACTION);
 
   gfx::ScopedCanvas scoped_canvas(canvas);
   if (base::i18n::IsRTL()) {
     canvas->Translate(gfx::Vector2d(width(), 0));
     canvas->Scale(-1, 1);
   }
@@ skipping 120 matching lines @@
     case STYLE_NORMAL:
       return kDisclosureArrowRightPadding;
     case STYLE_ACTION:
       return kDisclosureArrowButtonRightPadding;
   }
   NOTREACHED();
   return 0;
 }
 
 gfx::Size Combobox::ArrowSize() const {
-#if defined(OS_LINUX) && !defined(OS_CHROMEOS)
-  // TODO(estade): hack alert! This should always use GetNativeTheme(). For now
-  // STYLE_ACTION isn't properly themed so we have to override the NativeTheme
-  // behavior. See crbug.com/384071
-  const ui::NativeTheme* native_theme_for_arrow =
-      style_ == STYLE_ACTION ? ui::NativeThemeAura::instance()
-                             : GetNativeTheme();
-#else
-  const ui::NativeTheme* native_theme_for_arrow = GetNativeTheme();
-#endif
-
-  ui::NativeTheme::ExtraParams ignored;
-  return native_theme_for_arrow->GetPartSize(ui::NativeTheme::kComboboxArrow,
-                                             ui::NativeTheme::kNormal,
-                                             ignored);
+  return PlatformStyle::CreateComboboxArrow(enabled(), style_).size();
 }
 
 gfx::Size Combobox::GetContentSize() const {
   const gfx::FontList& font_list = GetFontList();
 
   int width = 0;
   for (int i = 0; i < model()->GetItemCount(); ++i) {
     if (model_->IsItemSeparatorAt(i))
       continue;
 
     if (style_ != STYLE_ACTION || i == selected_index_) {
       width = std::max(
           width,
           gfx::GetStringWidth(menu_model_adapter_->GetLabelAt(i), font_list));
     }
   }
   return gfx::Size(width, font_list.GetHeight());
 }
 
 PrefixSelector* Combobox::GetPrefixSelector() {
   if (!selector_)
     selector_.reset(new PrefixSelector(this));
   return selector_.get();
 }
 
 }  // namespace views