Clean up some NativeTheme code.

ui/native_theme/native_theme_aura.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/native_theme/native_theme_aura.h"
 
 #include <limits>
 #include <utility>
 
 #include "base/logging.h"
@@ skipping 24 matching lines @@
 constexpr SkAlpha kOverlayScrollbarAlphaNormal = 0x4D;
 constexpr SkAlpha kOverlayScrollbarAlphaHovered = 0x80;
 constexpr SkAlpha kOverlayScrollbarAlphaPressed = 0x80;
 
 // Indexed by ScrollbarOverlayColorTheme.
 constexpr SkColor kOverlayScrollbarThumbColor[] = {SK_ColorBLACK,
                                                    SK_ColorWHITE};
 constexpr SkColor kOverlayScrollbarStrokeColor[] = {SK_ColorWHITE,
                                                     SK_ColorBLACK};
 
-SkAlpha ThumbAlphaForState(NativeTheme::State state) {
-  bool overlay = IsOverlayScrollbarEnabled();
-  switch (state) {
-    case NativeTheme::kDisabled:
-      return 0x00;
-    case NativeTheme::kHovered:
-      return overlay ? kOverlayScrollbarAlphaHovered : 0x4D;
-    case NativeTheme::kNormal:
-      return overlay ? kOverlayScrollbarAlphaNormal : 0x33;
-    case NativeTheme::kPressed:
-      return overlay ? kOverlayScrollbarAlphaPressed : 0x80;
-    case NativeTheme::kNumStates:
-      break;
-  }
-
-  NOTREACHED();
-  return 0xFF;
-}
-
 const SkColor kTrackColor = SkColorSetRGB(0xF1, 0xF1, 0xF1);
 
 }  // namespace
 
 // static
 NativeTheme* NativeTheme::GetInstanceForWeb() {
+  return NativeThemeAura::web_instance();
+}
+
+#if !defined(OS_WIN)
+// static
+NativeTheme* NativeTheme::GetInstanceForNativeUi() {
   return NativeThemeAura::instance();
 }
+#endif
+
+// static
+NativeThemeAura* NativeThemeAura::instance() {
+  CR_DEFINE_STATIC_LOCAL(NativeThemeAura, s_native_theme, (false));
+  return &s_native_theme;
+}
 
 // static
-NativeThemeAura* NativeThemeAura::instance() {
-  CR_DEFINE_STATIC_LOCAL(NativeThemeAura, s_native_theme, ());
-  return &s_native_theme;
+NativeThemeAura* NativeThemeAura::web_instance() {
+  CR_DEFINE_STATIC_LOCAL(NativeThemeAura, s_native_theme_for_web,
+                         (IsOverlayScrollbarEnabled()));

[bokan, 2016/11/06 23:37:24]

The is_overlay param can't use IsOverlayScrollbarEnabled since that's statically
set. Tests can turn overlays on/off dynamically at runtime using Blink's
RuntimeEnabledFeatures in which case Blink and NativeTheme will mismatch on
whether they're using overlays.

We should pass in the is_overlay param from client code which can query
WebRuntimeFeatures (see my patch).

[Evan Stade, 2016/11/07 00:40:43]

can you point me at some of the tests you're talking about? I don't really want
to pass any parameters from blink, and it seems like the tests should be able to
append command line switches to toggle this setting (if we need to, we can
prepend this fn with:

if (!IsOverlayScrollbarEnabled())
  return instance();

[bokan, 2016/11/07 12:28:11]

Hm, didn't think about adding command line flags. If we can do that then this is
fine. The failing tests are:

content_browsertests:

RenderViewImplTest.ImeComposition
RenderViewImplTest.OnSetTextDirection

webkit_unit_tests (though the right way to fix these will be to make them use a
mock scrollbar theme, I'll do that in my enabling patch):

BaseAudioContextTest.AutoplayMetrics_NodeStartGestureThenSucces
BaseAudioContextTest.AutoplayMetrics_CreateNoGesture
BaseAudioContextTest.AutoplayMetrics_CreateGesture
BaseAudioContextTest.AutoplayMetrics_NodeStartGesture
BaseAudioContextTest.AutoplayMetrics_NodeStartNoGestureThenSuccess
BaseAudioContextTest.AutoplayMetrics_CallResumeGesture
BaseAudioContextTest.AutoplayMetrics_NodeStartNoGesture
BaseAudioContextTest.AutoplayMetrics_CallResumeNoGesture

browser_tests:

ThreatDOMDetailsTest.Everything
FormClassifierTest.SignupFormWithSigninButtonHTML

The following browser_tests also fail but IIRC they're all hitting the DCHECKs
from Views so should be fixed by this patch already:

TrayAccessibilityTestInstance/TrayAccessibilityTest.CheckMarksOnDetailMenu/0
PhishingDOMFeatureExtractorTest.SubFrames
TaskManagerBrowserTest.IdleWakeups
TaskManagerBrowserTest.WebWorkerJSHeapMemory
TaskManagerBrowserTest.JSHeapMemory
ScrollbarTest.LongPromptScrollbar
TaskManagerUtilityProcessBrowserTest.UtilityJSHeapMemory

[Evan Stade, 2016/11/07 15:05:57]

This seems like the right approach:
https://codereview.chromium.org/2480903002/diff/100001/content/browser/site_p...
I can't get either of these to fail with this patch, with or without flipping
the default for overlay scrollbars in native_theme_switches.cc. 
same, not seeing these fail with this patch (even with scrollbars enabled)

+  return &s_native_theme_for_web;
 }
 
-NativeThemeAura::NativeThemeAura() {
+NativeThemeAura::NativeThemeAura(bool use_overlay_scrollbars)
+    : use_overlay_scrollbars_(use_overlay_scrollbars) {
 // We don't draw scrollbar buttons.
 #if defined(OS_CHROMEOS)
   set_scrollbar_button_length(0);
 #endif
 
-  if (IsOverlayScrollbarEnabled()) {
+  if (use_overlay_scrollbars_) {
     scrollbar_width_ =
         kOverlayScrollbarThumbWidthPressed + kOverlayScrollbarStrokeWidth * 2;
   }
 
   // Images and alphas declarations assume the following order.
   static_assert(kDisabled == 0, "states unexpectedly changed");
   static_assert(kHovered == 1, "states unexpectedly changed");
   static_assert(kNormal == 2, "states unexpectedly changed");
   static_assert(kPressed == 3, "states unexpectedly changed");
 }
@@ skipping 71 matching lines @@
   PaintArrow(canvas, rect, direction, arrow_color);
 }
 
 void NativeThemeAura::PaintScrollbarTrack(
     SkCanvas* canvas,
     Part part,
     State state,
     const ScrollbarTrackExtraParams& extra_params,
     const gfx::Rect& rect) const {
   // Overlay Scrollbar should never paint a scrollbar track.
-  DCHECK(!IsOverlayScrollbarEnabled());
+  DCHECK(!use_overlay_scrollbars_);
   SkPaint paint;
   paint.setColor(kTrackColor);
   canvas->drawIRect(gfx::RectToSkIRect(rect), paint);
 }
 
 void NativeThemeAura::PaintScrollbarThumb(
     SkCanvas* canvas,
     Part part,
     State state,
     const gfx::Rect& rect,
     ScrollbarOverlayColorTheme theme) const {
   // Do not paint if state is disabled.
   if (state == kDisabled)
     return;
 
   TRACE_EVENT0("blink", "NativeThemeAura::PaintScrollbarThumb");
 
+  SkAlpha thumb_alpha = SK_AlphaTRANSPARENT;
+  const bool overlay = use_overlay_scrollbars_;
+  switch (state) {
+    case NativeTheme::kDisabled:
+      thumb_alpha = SK_AlphaTRANSPARENT;
+    case NativeTheme::kHovered:
+      thumb_alpha = overlay ? kOverlayScrollbarAlphaHovered : 0x4D;
+    case NativeTheme::kNormal:
+      thumb_alpha = overlay ? kOverlayScrollbarAlphaNormal : 0x33;
+    case NativeTheme::kPressed:
+      thumb_alpha = overlay ? kOverlayScrollbarAlphaPressed : 0x80;
+    case NativeTheme::kNumStates:
+      break;
+  }
+
   gfx::Rect thumb_rect(rect);
   SkColor thumb_color;
-  SkAlpha thumb_alpha = ThumbAlphaForState(state);
-
-  if (IsOverlayScrollbarEnabled()) {
+  if (overlay) {
     thumb_color = kOverlayScrollbarThumbColor[theme];
 
     // In overlay mode, draw a stroke (border).
     constexpr int kStrokeWidth = kOverlayScrollbarStrokeWidth;
     SkPaint paint;
     paint.setColor(
         SkColorSetA(kOverlayScrollbarStrokeColor[theme], thumb_alpha));
     paint.setStyle(SkPaint::kStroke_Style);
     paint.setStrokeWidth(kStrokeWidth);
 
@@ skipping 20 matching lines @@
 
   SkPaint paint;
   paint.setColor(SkColorSetA(thumb_color, thumb_alpha));
   canvas->drawIRect(gfx::RectToSkIRect(thumb_rect), paint);
 }
 
 void NativeThemeAura::PaintScrollbarCorner(SkCanvas* canvas,
                                            State state,
                                            const gfx::Rect& rect) const {
   // Overlay Scrollbar should never paint a scrollbar corner.
-  DCHECK(!IsOverlayScrollbarEnabled());
+  DCHECK(!use_overlay_scrollbars_);
   SkPaint paint;
   paint.setColor(SkColorSetRGB(0xDC, 0xDC, 0xDC));
   canvas->drawIRect(RectToSkIRect(rect), paint);
 }
 
 gfx::Size NativeThemeAura::GetPartSize(Part part,
                                        State state,
                                        const ExtraParams& extra) const {
-  if (IsOverlayScrollbarEnabled()) {
+  if (use_overlay_scrollbars_) {
     constexpr int minimum_length =
         kOverlayScrollbarMinimumLength + 2 * kOverlayScrollbarStrokeWidth;
 
     // Aura overlay scrollbars need a slight tweak from the base sizes.
     switch (part) {
       case kScrollbarHorizontalThumb:
         return gfx::Size(minimum_length, scrollbar_width_);
       case kScrollbarVerticalThumb:
         return gfx::Size(scrollbar_width_, minimum_length);
       default:
-        // TODO(bokan): We should probably make sure code using overlay
-        // scrollbars isn't asking for part sizes that don't exist. This
-        // currently breaks in Views layout code which indicates they aren't
-        // overlay aware yet. The Views code should be fixed and either this
-        // branch return 0 for parts that don't exist or assert NOTREACHED.
-        // crbug.com/657159.
+        NOTREACHED();

[bokan, 2016/11/06 23:37:24]

Will GetPartSize ever get called for non-scrollbar parts like SliderThumbs? In
that case, we should probably NOTREACHED only on scrollbar parts.

[Evan Stade, 2016/11/07 00:40:43]

probably

[Evan Stade, 2016/11/07 15:05:57]

restored this (minus some of the comment about views layout). Other parts are
still hit. Stack trace: http://hastebin.com/raw/acuxamocor

         break;
     }
   }
 
   return NativeThemeBase::GetPartSize(part, state, extra);
 }
 
 }  // namespace ui