Delete ash-based code for Windows drag-and-drop graphics.

ui/base/dragdrop/drag_utils_win.cc

Index: ui/base/dragdrop/drag_utils_win.cc
diff --git a/ui/base/dragdrop/drag_utils_win.cc b/ui/base/dragdrop/drag_utils_win.cc
index fa482e2e026284b56e1edc4761cc0e5781c482b5..53c864a62432602a9b652e3ddc9400dfbf3ac2a1 100644
--- a/ui/base/dragdrop/drag_utils_win.cc
+++ b/ui/base/dragdrop/drag_utils_win.cc
@@ -23,65 +23,55 @@
 
 namespace drag_utils {
 
-static void SetDragImageOnDataObject(HBITMAP hbitmap,

[danakj, 2017/04/03 22:34:01]

Since these have one caller, I collapsed them all together. I felt like this
made things more clear and I didn't need to do things like build gfx::Size to
pass here. It didn't seem these were providing much now that we don't have to
handle ash as a fallback.

-                                     const gfx::Size& size_in_pixels,
-                                     const gfx::Vector2d& cursor_offset,
-                                     IDataObject* data_object) {
-  base::win::ScopedComPtr<IDragSourceHelper> helper;
-  HRESULT rv = CoCreateInstance(CLSID_DragDropHelper, 0, CLSCTX_INPROC_SERVER,
-                                IID_IDragSourceHelper, helper.ReceiveVoid());
-  if (SUCCEEDED(rv)) {
-    SHDRAGIMAGE sdi;
-    sdi.sizeDragImage = size_in_pixels.ToSIZE();
-    sdi.crColorKey = 0xFFFFFFFF;
-    sdi.hbmpDragImage = hbitmap;
-    sdi.ptOffset = gfx::PointAtOffsetFromOrigin(cursor_offset).ToPOINT();
-    helper->InitializeFromBitmap(&sdi, data_object);
-  }
-}
-
-// Blit the contents of the canvas to a new HBITMAP. It is the caller's

[danakj, 2017/04/03 22:34:01]

I read MSDN for a while and I think this comment is wrong (the code does not
handle deleting bits which I think is right). I wrote a new comment.

-// responsibility to release the |bits| buffer.
-static HBITMAP CreateHBITMAPFromSkBitmap(const SkBitmap& sk_bitmap) {
-  base::win::ScopedGetDC screen_dc(NULL);
-  BITMAPINFOHEADER header;
-  skia::CreateBitmapHeader(sk_bitmap.width(), sk_bitmap.height(), &header);
-  void* bits;
-  HBITMAP bitmap =
-      CreateDIBSection(screen_dc, reinterpret_cast<BITMAPINFO*>(&header),
-                       DIB_RGB_COLORS, &bits, NULL, 0);
-  if (!bitmap || !bits)
-    return NULL;
-  DCHECK_EQ(sk_bitmap.rowBytes(), static_cast<size_t>(sk_bitmap.width() * 4));
-  SkAutoLockPixels lock(sk_bitmap);
-  memcpy(
-      bits, sk_bitmap.getPixels(), sk_bitmap.height() * sk_bitmap.rowBytes());
-  return bitmap;
-}
-
 void SetDragImageOnDataObject(const gfx::ImageSkia& image_skia,
                               const gfx::Vector2d& cursor_offset,
                               ui::OSExchangeData* data_object) {
   DCHECK(data_object && !image_skia.size().IsEmpty());
+
   // InitializeFromBitmap() doesn't expect an alpha channel and is confused
   // by premultiplied colors, so unpremultiply the bitmap.
   // SetDragImageOnDataObject(HBITMAP) takes ownership of the bitmap.
-  HBITMAP bitmap = CreateHBITMAPFromSkBitmap(
-      SkBitmapOperations::UnPreMultiply(*image_skia.bitmap()));
-  if (bitmap) {
-    // Attach 'bitmap' to the data_object.
-    SetDragImageOnDataObject(
-        bitmap,
-        gfx::Size(image_skia.bitmap()->width(), image_skia.bitmap()->height()),
-        cursor_offset,
-        ui::OSExchangeDataProviderWin::GetIDataObject(*data_object));
+  SkBitmap unpremul_bitmap =
+      SkBitmapOperations::UnPreMultiply(*image_skia.bitmap());
+  int width = unpremul_bitmap.width();
+  int height = unpremul_bitmap.height();
+  size_t rowbytes = unpremul_bitmap.rowBytes();
+  DCHECK_EQ(rowbytes, static_cast<size_t>(width) * 4u);
+
+  void* bits;
+  HBITMAP hbitmap;
+  {
+    BITMAPINFOHEADER header;
+    skia::CreateBitmapHeader(width, height, &header);
+
+    base::win::ScopedGetDC screen_dc(NULL);

[danakj, 2017/04/03 22:34:01]

I scoped this DC to just be around CreateDIBSection. I think this is right but
please confirm.  It used to scope around the memcpy to bits also.

[Peter Kasting, 2017/04/04 00:31:14]

It makes me a little leery.  I suggest just not putting in this scoping; it
won't hurt anything, it might make it safer, it makes the code a little shorter,
and the scoping implies to the reader that it's important for correctness that
the locals be destroyed before the function ends, which isn't true.

[danakj, 2017/04/04 15:25:33]

I like to scope things as tightly as possible, if I extend the scope to the end
of the function, the next time someone changes this they will say "does the DC
need to stay alive for InitializeFromBitmap()"? the same way I am wondering
about the memcpy. Letting the scope run longer than it needs to make the
relationships more ambiguous IMO. CreateDIBSection doesn't say that the hdc
needs to be kept alive.

http://www.undefined-value.info/hDC_lifetime.html says "You should call
ReleaseDC as soon as possible. I assume if it's holding onto the handle, that
it's not releasing the DC. DCs are a very prescious resource."

I found a number of examples of code that deletes the HDC right after making a
bitmap and then using the bitmap.

https://www-user.tu-chemnitz.de/~heha/petzold/ch14e.htm
          hdc = GetDC (hwnd) ;
          hdcMem  = CreateCompatibleDC (hdc) ;
          hBitmap = CreateCompatibleBitmap (hdc, cxBitmap, cyBitmap) ;
          ReleaseDC (hwnd, hdc) ;
          SelectObject (hdcMem, hBitmap) ;

http://stackoverflow.com/questions/12655700/read-pixels-from-1bpp-bitmap-crea...
access the bits after releasing the HDC.

https://social.msdn.microsoft.com/Forums/en-US/2c96d6c5-352d-4afe-9b13-b9f966...
returns the bits pointer after releasing the HDC.

So, I think this will work.

+    // By giving a null hSection, the |bits| will be destroyed when the
+    // |hbitmap| is destroyed.
+    hbitmap =
+        CreateDIBSection(screen_dc, reinterpret_cast<BITMAPINFO*>(&header),
+                         DIB_RGB_COLORS, &bits, NULL, 0);
+  }
+  if (!hbitmap)
+    return;
+
+  {
+    SkAutoLockPixels lock(unpremul_bitmap);
+    memcpy(bits, unpremul_bitmap.getPixels(), height * rowbytes);
   }
 
-  // TODO: the above code is used in non-Ash, while below is used in Ash. If we
-  // could figure this context out then we wouldn't do unnecessary work. However
-  // as it stands getting this information in ui/base would be a layering
-  // violation.
-  data_object->provider().SetDragImage(image_skia, cursor_offset);
+  base::win::ScopedComPtr<IDragSourceHelper> helper;
+  HRESULT rv = CoCreateInstance(CLSID_DragDropHelper, 0, CLSCTX_INPROC_SERVER,
+                                IID_IDragSourceHelper, helper.ReceiveVoid());
+  if (!SUCCEEDED(rv))
+    return;
+
+  SHDRAGIMAGE sdi;
+  sdi.sizeDragImage.cx = width;
+  sdi.sizeDragImage.cy = height;
+  sdi.crColorKey = 0xFFFFFFFF;
+  sdi.hbmpDragImage = hbitmap;
+  sdi.ptOffset = gfx::PointAtOffsetFromOrigin(cursor_offset).ToPOINT();
+  helper->InitializeFromBitmap(&sdi, data_object);
 }
 
 }  // namespace drag_utils