Consolidate Windows bitmap and DC code

skia/ext/skia_utils_win.cc

Index: skia/ext/skia_utils_win.cc
diff --git a/skia/ext/skia_utils_win.cc b/skia/ext/skia_utils_win.cc
index 9873373725705fe6f8e4491a54ebd56916829276..a284a4db4e0aee27e5a7702edd2d1ce56d21c238 100644
--- a/skia/ext/skia_utils_win.cc
+++ b/skia/ext/skia_utils_win.cc
@@ -7,7 +7,10 @@
 #include <stddef.h>
 #include <windows.h>
 
+#include "base/debug/gdi_debug_util_win.h"
+#include "base/win/win_util.h"
 #include "third_party/skia/include/core/SkRect.h"
+#include "third_party/skia/include/core/SkSurface.h"
 #include "third_party/skia/include/core/SkTypes.h"
 #include "third_party/skia/include/effects/SkGradientShader.h"
 
@@ -23,10 +26,56 @@ static_assert(sizeof(RECT().right) == sizeof(SkIRect().fRight), "o7");
 static_assert(sizeof(RECT().bottom) == sizeof(SkIRect().fBottom), "o8");
 static_assert(sizeof(RECT) == sizeof(SkIRect), "o9");
 
+void CreateBitmapHeaderWithColorDepth(LONG width,
+                                      LONG height,
+                                      WORD color_depth,
+                                      BITMAPINFOHEADER* hdr) {
+  // These values are shared with gfx::PlatformDevice

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

Nit: Trailing periods

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

Done.

+  hdr->biSize = sizeof(BITMAPINFOHEADER);
+  hdr->biWidth = width;
+  hdr->biHeight = -height;  // minus means top-down bitmap

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

Nit: Initial caps

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

Done.

+  hdr->biPlanes = 1;
+  hdr->biBitCount = color_depth;
+  hdr->biCompression = BI_RGB;  // no compression
+  hdr->biSizeImage = 0;
+  hdr->biXPelsPerMeter = 1;
+  hdr->biYPelsPerMeter = 1;
+  hdr->biClrUsed = 0;
+  hdr->biClrImportant = 0;
+}
+
+HBITMAP CreateHBitmap(int width, int height) {
+  // CreateDIBSection appears to get unhappy if we create an empty bitmap, so

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

Changed "appears to get unhappy if we create" to "fails to allocate anything if
we try to create"

+  // just create a minimal bitmap.
+  if ((width == 0) || (height == 0)) {
+    width = 1;
+    height = 1;
+  }
+
+  BITMAPINFOHEADER hdr = {0};
+  CreateBitmapHeaderWithColorDepth(width, height, 32, &hdr);
+
+  HBITMAP hbitmap = CreateDIBSection(nullptr,
+                                     reinterpret_cast<BITMAPINFO*>(&hdr),
+                                     0, nullptr, nullptr, 0);
+
+  // If CreateDIBSection() failed, try to get some useful information out
+  // before we crash for post-mortem analysis.
+  if (!hbitmap)
+    base::debug::GDIBitmapAllocFailure(&hdr, nullptr);
+
+  return hbitmap;
+}
+
+
 }  // namespace
 
 namespace skia {
 
+void CreateBitmapHeader(int width, int height, BITMAPINFOHEADER* hdr) {
+  CreateBitmapHeaderWithColorDepth(width, height, 32, hdr);
+}

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

Nit Function definition order should match .h declaration order.

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

Done.

+
 POINT SkPointToPOINT(const SkPoint& point) {
   POINT win_point = {
       SkScalarRoundToInt(point.fX), SkScalarRoundToInt(point.fY)
@@ -73,7 +122,7 @@ void InitializeDC(HDC context) {
   SkASSERT(res != 0);
   // As per SetStretchBltMode() documentation, SetBrushOrgEx() must be called
   // right after.
-  res = SetBrushOrgEx(context, 0, 0, NULL);
+  res = SetBrushOrgEx(context, 0, 0, nullptr);
   SkASSERT(res != 0);
 
   // Sets up default orientation.
@@ -97,5 +146,59 @@ void InitializeDC(HDC context) {
   SkASSERT(res != 0);
 }
 
+SkImageInfo PrepareAllocation(HDC context, BITMAP* backing) {
+  HBITMAP backing_handle =
+      static_cast<HBITMAP>(GetCurrentObject(context, OBJ_BITMAP));
+  const size_t backing_size = sizeof(*backing);
+  if (GetObject(backing_handle, backing_size, backing) != backing_size)
+    return SkImageInfo();
+  return SkImageInfo::MakeN32Premul(backing->bmWidth, backing->bmHeight);

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

Nit: Fractionally less verbose; up to you

  return (GetObject(backing_handle, backing_size, backing) == backing_size)
             ? SkImageInfo::MakeN32Premul(backing->bmWidth, backing->bmHeight)
             : SkImageInfo();

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

Done.

+}
+
+sk_sp<SkSurface> MapPlatformSurface(HDC context) {
+  BITMAP backing;
+  const SkImageInfo size(PrepareAllocation(context, &backing));
+  if (size.isEmpty())
+    return nullptr;
+  return SkSurface::MakeRasterDirect(size, backing.bmBits,
+                                     backing.bmWidthBytes);

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

Nit: Likewise:

  return size.IsEmpty() ? nullptr
                        : SkSurface::MakeRasterDirect(size, backing.bmBits,
                                                      backing.bmWidthBytes);

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

Done.

+}
+
+SkBitmap MapPlatformBitmap(HDC context) {
+  BITMAP backing;
+  const SkImageInfo size(PrepareAllocation(context, &backing));
+  SkBitmap bitmap;
+  if (!size.isEmpty())
+    bitmap.installPixels(size, backing.bmBits, size.minRowBytes());
+  return bitmap;
+}
+
+HDC CreateOffscreenSurface(int width, int height) {
+  HBITMAP bitmap = nullptr;
+
+  // If this process doesn't have access to GDI, we'll have to use a shared
+  // memory segment instead.
+  if (!base::win::IsUser32AndGdi32Available())
+    return nullptr;
+
+  bitmap = CreateHBitmap(width, height);
+  if (!bitmap)
+    return nullptr;
+
+  HDC hdc = CreateCompatibleDC(nullptr);
+  SkASSERT(hdc);

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

Do we really want to assert here instead of just returning null on failure? 
Same below.

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

This code was taken from code that asserted, but since we return null if bitmap
creation fails we should be able to do the same here, although we need to at
least clean up the HDC...

Done, with header file comment update.

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

I don't see any header comment update for this function?

+  InitializeDC(hdc);
+  HRGN clip = CreateRectRgn(0, 0, width, height);
+  int result = SelectClipRgn(hdc, clip);
+  SkASSERT(result != ERROR);
+  result = DeleteObject(clip);
+  SkASSERT(result);
+
+  SelectObject(hdc, bitmap);
+
+  // The caller must call DeleteDC(hdc) on this object once done with it.

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

It's too bad we can't return a ScopedCreateDC here to enforce this.  I wonder if
it would be worth modifying that type to support moves like a real unique_ptr
does.

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

Filed crbug.com/622442.

+  return hdc;
+}
+
 }  // namespace skia