Consolidate Windows bitmap and DC code

ui/native_theme/native_theme_win.cc

Index: ui/native_theme/native_theme_win.cc
diff --git a/ui/native_theme/native_theme_win.cc b/ui/native_theme/native_theme_win.cc
index eb7b82f30d33390534331b82697136375146ed37..2ac94c23180d8d4d9fc9194cd9b55c32332a01a4 100644
--- a/ui/native_theme/native_theme_win.cc
+++ b/ui/native_theme/native_theme_win.cc
@@ -16,7 +16,6 @@
 #include "base/win/scoped_hdc.h"
 #include "base/win/scoped_select_object.h"
 #include "base/win/windows_version.h"
-#include "skia/ext/bitmap_platform_device.h"
 #include "skia/ext/platform_canvas.h"
 #include "skia/ext/skia_utils_win.h"
 #include "third_party/skia/include/core/SkCanvas.h"
@@ -24,6 +23,7 @@
 #include "third_party/skia/include/core/SkColorPriv.h"
 #include "third_party/skia/include/core/SkRefCnt.h"
 #include "third_party/skia/include/core/SkShader.h"
+#include "third_party/skia/include/core/SkSurface.h"
 #include "ui/base/material_design/material_design_controller.h"
 #include "ui/display/win/dpi.h"
 #include "ui/gfx/color_palette.h"
@@ -284,10 +284,12 @@ void NativeThemeWin::Paint(SkCanvas* canvas,
     }
   }
 
+  skia::ScopedPlatformPaint paint(canvas);
+  HDC surface = paint.GetPlatformSurface();
   if (needs_paint_indirect)
-    PaintIndirect(canvas, part, state, rect, extra);
+    PaintIndirect(surface, part, state, rect, extra);
   else
-    PaintDirect(canvas, part, state, rect, extra);
+    PaintDirect(surface, part, state, rect, extra);
 }
 
 NativeThemeWin::NativeThemeWin()
@@ -376,14 +378,11 @@ void NativeThemeWin::PaintMenuBackground(SkCanvas* canvas,
   canvas->drawRect(gfx::RectToSkRect(rect), paint);
 }
 
-void NativeThemeWin::PaintDirect(SkCanvas* canvas,
+void NativeThemeWin::PaintDirect(HDC hdc,
                                  Part part,
                                  State state,
                                  const gfx::Rect& rect,
                                  const ExtraParams& extra) const {
-  skia::ScopedPlatformPaint scoped_platform_paint(canvas);
-  HDC hdc = scoped_platform_paint.GetPlatformSurface();
-
   switch (part) {
     case kCheckbox:
       PaintCheckbox(hdc, part, state, rect, extra.button);
@@ -438,12 +437,15 @@ void NativeThemeWin::PaintDirect(SkCanvas* canvas,
       return;
     case kScrollbarHorizontalTrack:
     case kScrollbarVerticalTrack:
-      PaintScrollbarTrack(canvas, hdc, part, state, rect,
-                          extra.scrollbar_track);
+      PaintScrollbarTrack(hdc, part, state, rect, extra.scrollbar_track);
       return;
-    case kScrollbarCorner:
-      canvas->drawColor(SK_ColorWHITE, SkXfermode::kSrc_Mode);
+    case kScrollbarCorner: {
+      sk_sp<SkSurface> surface(skia::MapPlatformSurface(hdc));
+      SkCanvas* canvas = surface ? surface->getCanvas() : nullptr;

[tomhudson, 2016/06/22 21:29:58]

This really *ought* to succeed, given that skia::SupportsPlatformPaint() was
true on the way in here; the null-checking is being conservative. In theory we
could just call surface->getCanvas()->drawColor(...) here.

[Peter Kasting, 2016/06/22 23:43:51]

If we're sure that has to work, I'd rather do that; extra null checks imply that
the calls in question can actually fail and can mislead later readers.

+      if (canvas)
+        canvas->drawColor(SK_ColorWHITE, SkXfermode::kSrc_Mode);
       return;
+    }
     case kTabPanelBackground:
       PaintTabPanelBackground(hdc, rect);
       return;
@@ -452,7 +454,7 @@ void NativeThemeWin::PaintDirect(SkCanvas* canvas,
       return;
     case kTrackbarThumb:
     case kTrackbarTrack:
-      PaintTrackbar(canvas, hdc, part, state, rect, extra.trackbar);
+      PaintTrackbar(hdc, part, state, rect, extra.trackbar);
       return;
     case kWindowResizeGripper:
       PaintWindowResizeGripper(hdc, rect);
@@ -665,7 +667,7 @@ SkColor NativeThemeWin::GetSystemColor(ColorId color_id) const {
   return GetAuraColor(color_id, this);
 }
 
-void NativeThemeWin::PaintIndirect(SkCanvas* canvas,
+void NativeThemeWin::PaintIndirect(HDC hdc,
                                    Part part,
                                    State state,
                                    const gfx::Rect& rect,
@@ -676,12 +678,13 @@ void NativeThemeWin::PaintIndirect(SkCanvas* canvas,
   //                  keeping a cache of the resulting bitmaps.
 
   // Create an offscreen canvas that is backed by an HDC.
-  sk_sp<skia::BitmapPlatformDevice> device(
-      skia::BitmapPlatformDevice::Create(
-          rect.width(), rect.height(), false, NULL));
-  DCHECK(device);
-  SkCanvas offscreen_canvas(device.get());
-  DCHECK(skia::SupportsPlatformPaint(&offscreen_canvas));
+  base::win::ScopedCreateDC offscreen_hdc
+      (skia::CreateOffscreenSurface(rect.width(), rect.height()));
+  sk_sp<SkSurface> offscreen_surface
+      (skia::MapPlatformSurface(offscreen_hdc.Get()));
+  DCHECK(offscreen_surface);
+  SkCanvas* offscreen_canvas = offscreen_surface->getCanvas();
+  DCHECK(offscreen_canvas);

[Peter Kasting, 2016/06/22 02:23:34]

It's really unclear to me how we know when to null-check MapPlatformSurface()
and getCanvas() and handle failure, versus when we can DCHECK the result.  We do
both in this file.  Should we always be doing one or the other?  If not, do
these sites deserve comments about why they can fail in some places and not the
others?

[tomhudson, 2016/06/22 21:29:58]

We don't have any control over the canvas passed into Paint(), and it could be a
PlatformCanvas or a raw canvas (one without OS-specific backing). We want to do
different things depending on which it is.

However, the canvas created here through CreateOffscreenSurface() and subsequent
calls is being created to be a PlatformCanvas; if those return NULL, we have an
allocation failure or some other critical falut we aren't prepared to recover
from.

See caveat in line 444 above.

 
   // Some of the Windows theme drawing operations do not write correct alpha
   // values for fully-opaque pixels; instead the pixels get alpha 0. This is
@@ -691,7 +694,7 @@ void NativeThemeWin::PaintIndirect(SkCanvas* canvas,
   // which pixels get touched by the paint operation. After paint, set any
   // pixels that have alpha 0 to opaque and placeholders to fully-transparent.
   const SkColor placeholder = SkColorSetARGB(1, 0, 0, 0);
-  offscreen_canvas.clear(placeholder);
+  offscreen_canvas->clear(placeholder);
 
   // Offset destination rects to have origin (0,0).
   gfx::Rect adjusted_rect(rect.size());
@@ -709,15 +712,16 @@ void NativeThemeWin::PaintIndirect(SkCanvas* canvas,
     default:
       break;
   }
-  // Draw the theme controls using existing HDC-drawing code.
-  PaintDirect(&offscreen_canvas, part, state, adjusted_rect, adjusted_extra);
 
-  SkBitmap bitmap = skia::ReadPixels(&offscreen_canvas);
+  SkBitmap bitmap = skia::ReadPixels(offscreen_canvas);
+  // Draw the theme controls using existing HDC-drawing code.
+  PaintDirect(offscreen_hdc.Get(), part, state, adjusted_rect, adjusted_extra);
 
   // Post-process the pixels to fix up the alpha values (see big comment above).
+  SkBitmap offscreen_bitmap = skia::MapPlatformBitmap(offscreen_hdc.Get());
   const SkPMColor placeholder_value = SkPreMultiplyColor(placeholder);
   const int pixel_count = rect.width() * rect.height();
-  SkPMColor* pixels = bitmap.getAddr32(0, 0);
+  SkPMColor* pixels = offscreen_bitmap.getAddr32(0, 0);
   for (int i = 0; i < pixel_count; i++) {
     if (pixels[i] == placeholder_value) {
       // Pixel wasn't touched - make it fully transparent.
@@ -731,8 +735,11 @@ void NativeThemeWin::PaintIndirect(SkCanvas* canvas,
     }
   }
 
-  // Draw the offscreen bitmap to the destination canvas.
-  canvas->drawBitmap(bitmap, rect.x(), rect.y());
+  sk_sp<SkSurface> destination_surface(skia::MapPlatformSurface(hdc));
+  DCHECK(destination_surface);
+  SkCanvas* destination_canvas = destination_surface->getCanvas();
+  DCHECK(destination_canvas);
+  destination_canvas->drawBitmap(offscreen_bitmap, rect.x(), rect.y());
 }
 
 HRESULT NativeThemeWin::GetThemePartSize(ThemeName theme_name,
@@ -1249,7 +1256,6 @@ HRESULT NativeThemeWin::PaintScrollbarThumb(
 }
 
 HRESULT NativeThemeWin::PaintScrollbarTrack(
-    SkCanvas* canvas,
     HDC hdc,
     Part part,
     State state,
@@ -1290,6 +1296,10 @@ HRESULT NativeThemeWin::PaintScrollbarTrack(
       (system_colors_[COLOR_SCROLLBAR] != system_colors_[COLOR_WINDOW])) {
     FillRect(hdc, &rect_win, reinterpret_cast<HBRUSH>(COLOR_SCROLLBAR + 1));
   } else {
+    sk_sp<SkSurface> surface(skia::MapPlatformSurface(hdc));
+    SkCanvas* canvas = surface ? surface->getCanvas() : nullptr;
+    if (!canvas)
+      return E_FAIL;
     SkPaint paint;
     RECT align_rect = gfx::Rect(extra.track_x, extra.track_y, extra.track_width,
                                 extra.track_height).ToRECT();
@@ -1335,7 +1345,6 @@ HRESULT NativeThemeWin::PaintSpinButton(
 }
 
 HRESULT NativeThemeWin::PaintTrackbar(
-    SkCanvas* canvas,
     HDC hdc,
     Part part,
     State state,
@@ -1409,6 +1418,10 @@ HRESULT NativeThemeWin::PaintTrackbar(
 
     // If the button is pressed, draw hatching.
     if (extra.classic_state & DFCS_PUSHED) {
+      sk_sp<SkSurface> surface(skia::MapPlatformSurface(hdc));
+      SkCanvas* canvas = surface ? surface->getCanvas() : nullptr;
+      if (!canvas)
+        return E_FAIL;
       SkPaint paint;
       SetCheckerboardShader(&paint, rect_win);