Issue 1492353002: Consolidate Windows bitmap and DC code

tomhudson

tomhudson@google.com changed reviewers: + fmalita@chromium.org

4 years, 10 months ago (2016-02-10 17:04:09 UTC) #1

tomhudson

Florin, could you take a look at the skia/ext/ part of this before I send ...

4 years, 10 months ago (2016-02-10 17:04:09 UTC) #2

Evan Stade

estade@chromium.org changed reviewers: + estade@chromium.org

4 years, 10 months ago (2016-02-10 21:41:04 UTC) #3

Evan Stade

https://codereview.chromium.org/1492353002/diff/200001/ui/native_theme/native_theme_win.cc File ui/native_theme/native_theme_win.cc (right): https://codereview.chromium.org/1492353002/diff/200001/ui/native_theme/native_theme_win.cc#newcode371 ui/native_theme/native_theme_win.cc:371: SkSurface* surface = nullptr; why is this declared out ...

4 years, 10 months ago (2016-02-10 21:41:04 UTC) #4

Peter Kasting

pkasting@chromium.org changed reviewers: + pkasting@chromium.org

4 years, 10 months ago (2016-02-11 00:19:25 UTC) #5

Peter Kasting

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_win.cc File skia/ext/skia_utils_win.cc (right): https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_win.cc#newcode30 skia/ext/skia_utils_win.cc:30: // CreateDIBSection appears to get unhappy if we create ...

4 years, 10 months ago (2016-02-11 00:19:26 UTC) #6

tomhudson

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_win.cc File skia/ext/skia_utils_win.cc (right): https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_win.cc#newcode30 skia/ext/skia_utils_win.cc:30: // CreateDIBSection appears to get unhappy if we create ...

4 years, 10 months ago (2016-02-16 17:47:54 UTC) #7

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_wi...
File skia/ext/skia_utils_win.cc (right):

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:30: // CreateDIBSection appears to get unhappy if we
create an empty bitmap, so
On 2016/02/11 00:19:25, Peter Kasting wrote:
> What does "appears to get unhappy" mean?  Be specific rather than colloquial.

This comment comes verbatim out of skia/ext/bitmap_platform_device_win.cc, which
we're trying to get rid of (and so the code being copied now will be deleted in
the future). Agree that it's not clear; it dates back to at least 2008, so I
suspect nobody knows specifically what it means.

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:31: // just create a minimal bitmap
On 2016/02/11 00:19:26, Peter Kasting wrote:
> Nit: Trailing period

Done.

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:35: }
On 2016/02/11 00:19:25, Peter Kasting wrote:
> Nit: Do we really want to force e.g. (30, 0) to (1, 1) instead of (30, 1)?

https://codereview.chromium.org/9459 changed it from (30, 1) to (1, 1), reading
the bug history behind it that might have been preventative rather than
addressing an observed bug?

Skia will treat a bitmap with 0 in width *or* height as empty, so I suspect we
want to do the minimal thing to make the OS happy, and a 1 pixel bitmap is
smaller than a 30 pixel bitmap.

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:48: hdr.biClrImportant = 0;
On 2016/02/11 00:19:26, Peter Kasting wrote:
> Don't do all this manually; use gfx::CreateBitmapHeader().

Dependency is in the wrong direction? gfx depends on Skia; I don't want to make
Skia depend on gfx. (One of the questions we want to investigate this year is:
since we only have a single backend for Chrome (Skia), what parts of gfx are now
unnecessary?)

I'd be happy to consider moving that function here; it seems to be used in 3
places:

* printing/emf_win: might go away now that XP support is ended, but there aren't
people dedicated to that yet; is already including this file.
* ui/base/dragdrop: caller "CreateHBITMAPFromSkBitmap()" really ought to be in
skia/ext, if we don't have an equivalent function already.
* ui/gfx/icon_util: is already using skia/ext/image_operations.

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:53: #if !defined(_WIN64)
On 2016/02/11 00:19:25, Peter Kasting wrote:
> Why is this ifdefed?

cpu@ added that to try to isolate reasons for beta crashes two years ago today
(https://codereview.chromium.org/159523003); there's no discussion of why in the
code review or in the bug so far as I can see.

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:54: // If this call fails, we're gonna crash hard.
Try to get some useful
On 2016/02/11 00:19:26, Peter Kasting wrote:
> Which call?  The call above?  And why/where will we crash?
> 
> Nit: Again, avoid colloquialisms like "gonna crash hard".

Acknowledged.

Another legacy comment. Changing to
  // If CreateDIBSection() failed, try to get some useful
  // information out before we crash for post-mortem analysis.

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:138: HBITMAP backing_handle =
(HBITMAP)GetCurrentObject(context, OBJ_BITMAP);
On 2016/02/11 00:19:25, Peter Kasting wrote:
> No C-style casts (2 places)

Done.

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:142: if (byte_count != sizeof(backing))
On 2016/02/11 00:19:25, Peter Kasting wrote:
> Nit: Just inline this test above to remove the temp (2 places)

Done.

* Is there something I'm missing in the style guide, or is this just a personal
preference?

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:147: backing.bmWidthBytes);
On 2016/02/11 00:19:25, Peter Kasting wrote:
> Nit: Shorter:
> 
>   return SkSurface::NewRasterDirect(
>       SkImageInfo::MakeN32Premul(backing.bmWidth, backing.bmHeight),
>       backing.bmBits, backing.bmWidthBytes);

Done.

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:160: bitmap.installPixels(size, backing.bmBits,
size.minRowBytes());
On 2016/02/11 00:19:26, Peter Kasting wrote:
> Everything about this function above this line is identical to the function
> above.  Use a helper function to avoid code duplication.

I'm not convinced that a helper function is right here. When we have three
function calls, an early out, and multiple things that would need to be returned
from the helper function, the interface gets pretty ugly and the code gets ~30%
longer. Or is there a better way to write a helper function than this?

namespace {

bool PrepareAllocation(HDC context, SkImageInfo* size, BITMAP* backing) {
  HBITMAP backing_handle = 
    static_cast<HBITMAP>GetCurrentObject(context, OBJ_BITMAP);
  if (GetObject(backing_handle, sizeof(*backing),
                             backing) != sizeof(*backing))
    return false;
  *size = SkImageInfo::MakeN32Premul(backing->bmWidth,
                                     backing->bmHeight);
  return true;
}

}

SkSurface* MapPlatformSurface(HDC context) {
  SkImageInfo size;
  Bitmap backing;
  if (!PrepareAllocation(HDC, &size, &backing))
    return nullptr;
  return SkSurface::NewRasterDirect(size, backing.bmBits,
                                    backing.bmWidthBytes);
}

SkBitmap MapPlatformBitmap(HDC context) {
  SkImageInfo size;
  Bitmap backing;
  SkBitmap bitmap;
  if (PrepareAllocation(HDC, &size, &backing))
    bitmap.installPixels(size, backing.bmBits, size.minRowBytes());
  return bitmap;
}

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:165: 
On 2016/02/11 00:19:26, Peter Kasting wrote:
> Nit: Remove this blank line

Done.

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:166: HBITMAP bitmap = nullptr;
On 2016/02/11 00:19:26, Peter Kasting wrote:
> Your code seems to mix NULL and nullptr without apparent rationale.  Be
> consistent (and prefer to consistently use nullptr, if possible).

Done.

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:188: // need to clean up by calling DeleteDC(hdc)
else we leak!
On 2016/02/11 00:19:26, Peter Kasting wrote:
> So why aren't we doing that?

Returning HDC is the entire point of this function. More properly, the *caller*
must DeleteDC(hdc); that's mentioned in the comments in the header file.

https://codereview.chromium.org/1492353002/diff/200001/ui/native_theme/native...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/1492353002/diff/200001/ui/native_theme/native...
ui/native_theme/native_theme_win.cc:371: SkSurface* surface = nullptr;
On 2016/02/10 21:41:04, Evan Stade wrote:
> why is this declared out here

Acknowledged.

https://codereview.chromium.org/1492353002/diff/200001/ui/native_theme/native...
ui/native_theme/native_theme_win.cc:433: canvas->drawColor(SK_ColorWHITE,
SkXfermode::kSrc_Mode);
On 2016/02/11 00:19:26, Peter Kasting wrote:
> On 2016/02/10 21:41:04, Evan Stade wrote:
> > use a utility fn?
> 
> Not necessary; just this would solve your other comments:
> 
>     case kScrollbarCorner: {
>       SkSurface* surface = skia::MapPlatformSurface(hdc);
>       SkCanvas* canvas = surface ? surface->getCanvas() : nullptr;
>       if (canvas)
>         canvas->drawColor(SK_ColorWHITE, SkXfermode::kSrc_Mode);
>       return;
>     }

Done.

https://codereview.chromium.org/1492353002/diff/200001/ui/native_theme/native...
ui/native_theme/native_theme_win.cc:434: }
On 2016/02/10 21:41:04, Evan Stade wrote:
> this can't be the right formatting

Ooh, that's completely broken. Good catch.
Reinforces my request for help figuring out *which* tests to run / *how* to run
the right tests to get coverage of this code. Still working with techstop to get
browser tests to work at all on my Windows dev box.

https://codereview.chromium.org/1492353002/diff/200001/ui/native_theme/native...
ui/native_theme/native_theme_win.cc:1289: if (!surface || !canvas)
On 2016/02/11 00:19:26, Peter Kasting wrote:
> Nit: "if (!canvas)" suffices

Done.

https://codereview.chromium.org/1492353002/diff/200001/ui/native_theme/native...
ui/native_theme/native_theme_win.cc:1411: if (!surface || !canvas)
On 2016/02/11 00:19:26, Peter Kasting wrote:
> Nit: "if (!canvas)" suffices

Done.

Peter Kasting

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_win.cc File skia/ext/skia_utils_win.cc (right): https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_win.cc#newcode30 skia/ext/skia_utils_win.cc:30: // CreateDIBSection appears to get unhappy if we create ...

4 years, 10 months ago (2016-02-16 21:12:10 UTC) #8

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_wi...
File skia/ext/skia_utils_win.cc (right):

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:30: // CreateDIBSection appears to get unhappy if we
create an empty bitmap, so
On 2016/02/16 17:47:54, tomhudson wrote:
> On 2016/02/11 00:19:25, Peter Kasting wrote:
> > What does "appears to get unhappy" mean?  Be specific rather than
colloquial.
> 
> This comment comes verbatim out of skia/ext/bitmap_platform_device_win.cc,
which
> we're trying to get rid of (and so the code being copied now will be deleted
in
> the future). Agree that it's not clear; it dates back to at least 2008, so I
> suspect nobody knows specifically what it means.

Can you try a quick runtime test to see what happens when creating an empty
bitmap, so this can be updated?  I'd rather not just preserve comments that no
one understands, but this does deserve a comment, so we should really understand
why it's necessary.

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:48: hdr.biClrImportant = 0;
On 2016/02/16 17:47:54, tomhudson wrote:
> On 2016/02/11 00:19:26, Peter Kasting wrote:
> > Don't do all this manually; use gfx::CreateBitmapHeader().
> 
> Dependency is in the wrong direction? gfx depends on Skia; I don't want to
make
> Skia depend on gfx. (One of the questions we want to investigate this year is:
> since we only have a single backend for Chrome (Skia), what parts of gfx are
now
> unnecessary?)
> 
> I'd be happy to consider moving that function here; it seems to be used in 3
> places:
> 
> * printing/emf_win: might go away now that XP support is ended, but there
aren't
> people dedicated to that yet; is already including this file.
> * ui/base/dragdrop: caller "CreateHBITMAPFromSkBitmap()" really ought to be in
> skia/ext, if we don't have an equivalent function already.
> * ui/gfx/icon_util: is already using skia/ext/image_operations.

Sounds like we should go ahead and move that function here.  I would do that in
this patch; moving CreateHBITMAPFromSkBitmap() can be done separately if you
want to do it.

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:53: #if !defined(_WIN64)
On 2016/02/16 17:47:54, tomhudson wrote:
> On 2016/02/11 00:19:25, Peter Kasting wrote:
> > Why is this ifdefed?
> 
> cpu@ added that to try to isolate reasons for beta crashes two years ago today
> (https://codereview.chromium.org/159523003); there's no discussion of why in
the
> code review or in the bug so far as I can see.

Can you ping Carlos and try to find out why this is ifdefed?

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:160: bitmap.installPixels(size, backing.bmBits,
size.minRowBytes());
On 2016/02/16 17:47:54, tomhudson wrote:
> On 2016/02/11 00:19:26, Peter Kasting wrote:
> > Everything about this function above this line is identical to the function
> > above.  Use a helper function to avoid code duplication.
> 
> I'm not convinced that a helper function is right here. When we have three
> function calls, an early out, and multiple things that would need to be
returned
> from the helper function, the interface gets pretty ugly and the code gets
~30%
> longer. Or is there a better way to write a helper function than this?
> 
> namespace {
> 
> bool PrepareAllocation(HDC context, SkImageInfo* size, BITMAP* backing) {
>   HBITMAP backing_handle = 
>     static_cast<HBITMAP>GetCurrentObject(context, OBJ_BITMAP);
>   if (GetObject(backing_handle, sizeof(*backing),
>                              backing) != sizeof(*backing))
>     return false;
>   *size = SkImageInfo::MakeN32Premul(backing->bmWidth,
>                                      backing->bmHeight);
>   return true;
> }
> 
> }
> 
> SkSurface* MapPlatformSurface(HDC context) {
>   SkImageInfo size;
>   Bitmap backing;
>   if (!PrepareAllocation(HDC, &size, &backing))
>     return nullptr;
>   return SkSurface::NewRasterDirect(size, backing.bmBits,
>                                     backing.bmWidthBytes);
> }
> 
> SkBitmap MapPlatformBitmap(HDC context) {
>   SkImageInfo size;
>   Bitmap backing;
>   SkBitmap bitmap;
>   if (PrepareAllocation(HDC, &size, &backing))
>     bitmap.installPixels(size, backing.bmBits, size.minRowBytes());
>   return bitmap;
> }

Assuming you don't need to be able to allocate empty-sized objects, you can just
return the size, which cleans things up a bit:

SkImageInfo PrepareAllocation(HDC context, BITMAP* backing) {
  HBITMAP backing_handle =
      static_cast<HBITMAP>GetCurrentObject(context, OBJ_BITMAP);
  const size_t backing_size = sizeof *backing;
  return (GetObject(backing_handle, backing_size, backing) == backing_size)
      ? SkImageInfo::MakeN32Premul(backing->bmWidth, backing->bmHeight)
      : SkImageInfo();
}

SkSurface* MapPlatformSurface(HDC context) {
  Bitmap backing;
  const SkImageInfo size(PrepareAllocation(HDC, &backing));
  return size.isEmpty()
      ? nullptr
      : SkSurface::NewRasterDirect(size, backing.bmBits, backing.bmWidthBytes);
}

SkBitmap MapPlatformBitmap(HDC context) {
  Bitmap backing;
  const SkImageInfo size(PrepareAllocation(HDC, &backing));
  SkBitmap bitmap;
  if (!size.isEmpty())
    bitmap.installPixels(size, backing.bmBits, size.minRowBytes());
  return bitmap;
}

This is still a bit longer than the original, but seems a bit clearer/safer.

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:188: // need to clean up by calling DeleteDC(hdc)
else we leak!
On 2016/02/16 17:47:54, tomhudson wrote:
> On 2016/02/11 00:19:26, Peter Kasting wrote:
> > So why aren't we doing that?
> 
> Returning HDC is the entire point of this function. More properly, the
*caller*
> must DeleteDC(hdc); that's mentioned in the comments in the header file.

In that case, I would change this to:

  // The caller must DeleteDC(hdc) on this object once it's done.
  return hdc;

https://codereview.chromium.org/1492353002/diff/260001/skia/ext/skia_utils_wi...
File skia/ext/skia_utils_win.cc (right):

https://codereview.chromium.org/1492353002/diff/260001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:50: HBITMAP hbitmap = CreateDIBSection(nullptr,
reinterpret_cast<BITMAPINFO*>(&hdr),
Nit: 80 columns

https://codereview.chromium.org/1492353002/diff/260001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:54: // If CreateDIBSection() failed, try to get some
useful
Nit: Wrap as late as possible

https://codereview.chromium.org/1492353002/diff/260001/skia/ext/skia_utils_win.h
File skia/ext/skia_utils_win.h (right):

https://codereview.chromium.org/1492353002/diff/260001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.h:55: // Returns NULL on error.
Nit: NULL -> null (since this actually returns nullptr)

https://codereview.chromium.org/1492353002/diff/260001/ui/native_theme/native...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/1492353002/diff/260001/ui/native_theme/native...
ui/native_theme/native_theme_win.cc:668: HDC offscreen_hdc =
skia::CreateOffscreenSurface(rect.width(), rect.height());
Nit: Use ScopedCreateDC here instead of a raw HDC to ensure proper cleanup. 
This avoids the need for the direct DeleteDC() call.

tomhudson

Description was changed from ========== WIP implement Windows native themes without BitmapPlatformDevice BUG=543755 ========== to ...

4 years, 10 months ago (2016-02-18 15:03:54 UTC) #9

tomhudson

Still on my TODO stack, just keeps getting pushed down by other bits of Chrome/Skia ...

4 years, 7 months ago (2016-04-29 14:02:28 UTC) #11

tomhudson

The CQ bit was checked by tomhudson@google.com to run a CQ dry run

4 years, 6 months ago (2016-06-08 19:43:43 UTC) #12

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1492353002/300001

4 years, 6 months ago (2016-06-08 19:44:23 UTC) #13

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 6 months ago (2016-06-08 20:34:31 UTC) #14

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_compile_dbg_ng/builds/203177) win_chromium_x64_rel_ng on ...

4 years, 6 months ago (2016-06-08 20:34:32 UTC) #15

cpu_(ooo_6.6-7.5)

cpu@chromium.org changed reviewers: + cpu@chromium.org

4 years, 6 months ago (2016-06-12 19:13:15 UTC) #16

cpu_(ooo_6.6-7.5)

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_win.cc File skia/ext/skia_utils_win.cc (right): https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_win.cc#newcode53 skia/ext/skia_utils_win.cc:53: #if !defined(_WIN64) On 2016/02/16 21:12:09, Peter Kasting wrote: > ...

4 years, 6 months ago (2016-06-12 19:13:16 UTC) #17

tomhudson

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_win.cc File skia/ext/skia_utils_win.cc (right): https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_win.cc#newcode30 skia/ext/skia_utils_win.cc:30: // CreateDIBSection appears to get unhappy if we create ...

4 years, 6 months ago (2016-06-21 22:20:23 UTC) #18

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_wi...
File skia/ext/skia_utils_win.cc (right):

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:30: // CreateDIBSection appears to get unhappy if we
create an empty bitmap, so
On 2016/02/16 21:12:09, Peter Kasting wrote:
> Can you try a quick runtime test to see what happens when creating an empty
> bitmap, so this can be updated?  I'd rather not just preserve comments that no
> one understands, but this does deserve a comment, so we should really
understand
> why it's necessary.

Under 64b Windows 7, if I write
  HBITMAP hb = CreateHBitmap(0, 0)
that triggers the CHECK(false) in line 59 of gdi_debug_util_win.cc; I suspect
that the CreateDIBSection() call is failing to allocate and our crash-recovery
code is also failing.

I could change the comment to be explicit about the failure mode?

Note also that we do the same thing on Mac (make sure generated bitmaps are at
least 1x1).

So - contrary to Carlos' expectations around the old #if !defined(_WIN64), it
looks like we can trigger GDI failure on 64b?

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:48: hdr.biClrImportant = 0;
On 2016/02/16 21:12:09, Peter Kasting wrote:
> On 2016/02/16 17:47:54, tomhudson wrote:
> > On 2016/02/11 00:19:26, Peter Kasting wrote:
> > > Don't do all this manually; use gfx::CreateBitmapHeader().
> > 
> > Dependency is in the wrong direction? gfx depends on Skia; I don't want to
> make
> > Skia depend on gfx. (One of the questions we want to investigate this year
is:
> > since we only have a single backend for Chrome (Skia), what parts of gfx are
> now
> > unnecessary?)
> > 
> > I'd be happy to consider moving that function here; it seems to be used in 3
> > places:
> > 
> > * printing/emf_win: might go away now that XP support is ended, but there
> aren't
> > people dedicated to that yet; is already including this file.
> > * ui/base/dragdrop: caller "CreateHBITMAPFromSkBitmap()" really ought to be
in
> > skia/ext, if we don't have an equivalent function already.
> > * ui/gfx/icon_util: is already using skia/ext/image_operations.
> 
> Sounds like we should go ahead and move that function here.  I would do that
in
> this patch; moving CreateHBITMAPFromSkBitmap() can be done separately if you
> want to do it.

Done.

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:160: bitmap.installPixels(size, backing.bmBits,
size.minRowBytes());
On 2016/02/16 21:12:09, Peter Kasting wrote:
> On 2016/02/16 17:47:54, tomhudson wrote:
> > 
> > I'm not convinced that a helper function is right here. When we have three
> > function calls, an early out, and multiple things that would need to be
> returned
> > from the helper function, the interface gets pretty ugly and the code gets
> ~30%
> > longer. Or is there a better way to write a helper function than this?
> > 

...

> 
> Assuming you don't need to be able to allocate empty-sized objects, you can
just
> return the size, which cleans things up a bit:
> 
> SkImageInfo PrepareAllocation(HDC context, BITMAP* backing) {
>   HBITMAP backing_handle =
>       static_cast<HBITMAP>GetCurrentObject(context, OBJ_BITMAP);
>   const size_t backing_size = sizeof *backing;
>   return (GetObject(backing_handle, backing_size, backing) == backing_size)
>       ? SkImageInfo::MakeN32Premul(backing->bmWidth, backing->bmHeight)
>       : SkImageInfo();
> }
> 
> SkSurface* MapPlatformSurface(HDC context) {
>   Bitmap backing;
>   const SkImageInfo size(PrepareAllocation(HDC, &backing));
>   return size.isEmpty()
>       ? nullptr
>       : SkSurface::NewRasterDirect(size, backing.bmBits,
backing.bmWidthBytes);
> }
> 
> SkBitmap MapPlatformBitmap(HDC context) {
>   Bitmap backing;
>   const SkImageInfo size(PrepareAllocation(HDC, &backing));
>   SkBitmap bitmap;
>   if (!size.isEmpty())
>     bitmap.installPixels(size, backing.bmBits, size.minRowBytes());
>   return bitmap;
> }
> 
> This is still a bit longer than the original, but seems a bit clearer/safer.

Done. Thanks for writing the improved version.

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:188: // need to clean up by calling DeleteDC(hdc)
else we leak!
On 2016/02/16 21:12:09, Peter Kasting wrote:
> On 2016/02/16 17:47:54, tomhudson wrote:
> > On 2016/02/11 00:19:26, Peter Kasting wrote:
> > > So why aren't we doing that?
> > 
> > Returning HDC is the entire point of this function. More properly, the
> *caller*
> > must DeleteDC(hdc); that's mentioned in the comments in the header file.
> 
> In that case, I would change this to:
> 
>   // The caller must DeleteDC(hdc) on this object once it's done.
>   return hdc;

Done.

https://codereview.chromium.org/1492353002/diff/260001/skia/ext/skia_utils_wi...
File skia/ext/skia_utils_win.cc (right):

https://codereview.chromium.org/1492353002/diff/260001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:50: HBITMAP hbitmap = CreateDIBSection(nullptr,
reinterpret_cast<BITMAPINFO*>(&hdr),
On 2016/02/16 21:12:10, Peter Kasting wrote:
> Nit: 80 columns

Done.

https://codereview.chromium.org/1492353002/diff/260001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:54: // If CreateDIBSection() failed, try to get some
useful
On 2016/02/16 21:12:10, Peter Kasting wrote:
> Nit: Wrap as late as possible

Done.

https://codereview.chromium.org/1492353002/diff/260001/skia/ext/skia_utils_win.h
File skia/ext/skia_utils_win.h (right):

https://codereview.chromium.org/1492353002/diff/260001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.h:55: // Returns NULL on error.
On 2016/02/16 21:12:10, Peter Kasting wrote:
> Nit: NULL -> null (since this actually returns nullptr)

"nullptr", or "null"?

https://codereview.chromium.org/1492353002/diff/260001/ui/native_theme/native...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/1492353002/diff/260001/ui/native_theme/native...
ui/native_theme/native_theme_win.cc:668: HDC offscreen_hdc =
skia::CreateOffscreenSurface(rect.width(), rect.height());
On 2016/02/16 21:12:10, Peter Kasting wrote:
> Nit: Use ScopedCreateDC here instead of a raw HDC to ensure proper cleanup. 
> This avoids the need for the direct DeleteDC() call.

Done.

Peter Kasting

https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_win.cc File skia/ext/skia_utils_win.cc (right): https://codereview.chromium.org/1492353002/diff/200001/skia/ext/skia_utils_win.cc#newcode30 skia/ext/skia_utils_win.cc:30: // CreateDIBSection appears to get unhappy if we create ...

4 years, 6 months ago (2016-06-22 02:23:34 UTC) #19

tomhudson

JSHeapMemory failures on debug bot seem unconnected. https://codereview.chromium.org/1492353002/diff/380001/skia/ext/skia_utils_win.cc File skia/ext/skia_utils_win.cc (right): https://codereview.chromium.org/1492353002/diff/380001/skia/ext/skia_utils_win.cc#newcode33 skia/ext/skia_utils_win.cc:33: // These ...

4 years, 6 months ago (2016-06-22 21:29:58 UTC) #20

JSHeapMemory failures on debug bot seem unconnected.

https://codereview.chromium.org/1492353002/diff/380001/skia/ext/skia_utils_wi...
File skia/ext/skia_utils_win.cc (right):

https://codereview.chromium.org/1492353002/diff/380001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:33: // These values are shared with
gfx::PlatformDevice
On 2016/06/22 02:23:34, Peter Kasting wrote:
> Nit: Trailing periods

Done.

https://codereview.chromium.org/1492353002/diff/380001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:36: hdr->biHeight = -height;  // minus means top-down
bitmap
On 2016/06/22 02:23:34, Peter Kasting wrote:
> Nit: Initial caps

Done.

https://codereview.chromium.org/1492353002/diff/380001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:48: // CreateDIBSection appears to get unhappy if we
create an empty bitmap, so
Changed "appears to get unhappy if we create" to "fails to allocate anything if
we try to create"

https://codereview.chromium.org/1492353002/diff/380001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:77: }
On 2016/06/22 02:23:34, Peter Kasting wrote:
> Nit Function definition order should match .h declaration order.

Done.

https://codereview.chromium.org/1492353002/diff/380001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:155: return
SkImageInfo::MakeN32Premul(backing->bmWidth, backing->bmHeight);
On 2016/06/22 02:23:34, Peter Kasting wrote:
> Nit: Fractionally less verbose; up to you
> 
>   return (GetObject(backing_handle, backing_size, backing) == backing_size)
>              ? SkImageInfo::MakeN32Premul(backing->bmWidth, backing->bmHeight)
>              : SkImageInfo();

Done.

https://codereview.chromium.org/1492353002/diff/380001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:164: backing.bmWidthBytes);
On 2016/06/22 02:23:33, Peter Kasting wrote:
> Nit: Likewise:
> 
>   return size.IsEmpty() ? nullptr
>                         : SkSurface::MakeRasterDirect(size, backing.bmBits,
>                                                       backing.bmWidthBytes);

Done.

https://codereview.chromium.org/1492353002/diff/380001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:189: SkASSERT(hdc);
On 2016/06/22 02:23:34, Peter Kasting wrote:
> Do we really want to assert here instead of just returning null on failure? 
> Same below.

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.

https://codereview.chromium.org/1492353002/diff/380001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:199: // The caller must call DeleteDC(hdc) on this
object once done with it.
On 2016/06/22 02:23:34, Peter Kasting wrote:
> 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.

Filed crbug.com/622442.

https://codereview.chromium.org/1492353002/diff/380001/ui/native_theme/native...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/1492353002/diff/380001/ui/native_theme/native...
ui/native_theme/native_theme_win.cc:444: SkCanvas* canvas = surface ?
surface->getCanvas() : nullptr;
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.

https://codereview.chromium.org/1492353002/diff/380001/ui/native_theme/native...
ui/native_theme/native_theme_win.cc:687: DCHECK(offscreen_canvas);
On 2016/06/22 02:23:34, Peter Kasting wrote:
> 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?

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.

tomhudson

Nope, those failures are possibly connected to my patch, but I may need Windows-team help ...

4 years, 6 months ago (2016-06-22 22:02:18 UTC) #21

Peter Kasting

LGTM https://codereview.chromium.org/1492353002/diff/380001/skia/ext/skia_utils_win.cc File skia/ext/skia_utils_win.cc (right): https://codereview.chromium.org/1492353002/diff/380001/skia/ext/skia_utils_win.cc#newcode189 skia/ext/skia_utils_win.cc:189: SkASSERT(hdc); On 2016/06/22 21:29:58, tomhudson wrote: > On ...

4 years, 6 months ago (2016-06-22 23:43:52 UTC) #22

LGTM

https://codereview.chromium.org/1492353002/diff/380001/skia/ext/skia_utils_wi...
File skia/ext/skia_utils_win.cc (right):

https://codereview.chromium.org/1492353002/diff/380001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:189: SkASSERT(hdc);
On 2016/06/22 21:29:58, tomhudson wrote:
> On 2016/06/22 02:23:34, Peter Kasting wrote:
> > Do we really want to assert here instead of just returning null on failure? 
> > Same below.
> 
> 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.

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

https://codereview.chromium.org/1492353002/diff/380001/ui/native_theme/native...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/1492353002/diff/380001/ui/native_theme/native...
ui/native_theme/native_theme_win.cc:444: SkCanvas* canvas = surface ?
surface->getCanvas() : nullptr;
On 2016/06/22 21:29:58, tomhudson wrote:
> 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.

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.

https://codereview.chromium.org/1492353002/diff/400001/skia/ext/skia_utils_wi...
File skia/ext/skia_utils_win.cc (right):

https://codereview.chromium.org/1492353002/diff/400001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:183: HDC hdc = CreateCompatibleDC(nullptr);
Seems like we could use ScopedCreateDC here to avoid the explicit DeleteObject()
calls on early-exit below (which are verbose and error-prone).

I think the syntax for the final line then is something like "return
scoped_hdc.Take();".

tomhudson

Description was changed from ========== skia::BitmapPlatformDevice is being deprecated; we need to implement Windows native ...

4 years, 6 months ago (2016-06-23 19:39:22 UTC) #23

tomhudson

The CQ bit was checked by tomhudson@google.com to run a CQ dry run

4 years, 6 months ago (2016-06-23 19:39:51 UTC) #24

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1492353002/420001

4 years, 6 months ago (2016-06-23 19:40:20 UTC) #25

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 6 months ago (2016-06-23 20:10:24 UTC) #26

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/244362) win_clang on ...

4 years, 6 months ago (2016-06-23 20:10:25 UTC) #27

tomhudson

Description was changed from ========== Implement Windows native themes without BitmapPlatformDevice skia::BitmapPlatformDevice is being deprecated; ...

4 years, 5 months ago (2016-07-07 17:33:42 UTC) #28

tomhudson

Description was changed from ========== Consolidate Windows bitmap and DC code While removing skia::BitmapPlatformDevice, reviewers ...

4 years, 5 months ago (2016-07-07 18:16:16 UTC) #29

tomhudson

4 years, 4 months ago (2016-08-03 18:28:44 UTC) #30

Most of these changes have been landed piecemeal in other CLs.

Issue 1492353002: Consolidate Windows bitmap and DC code (Closed)

Description

Patch Set 1 #

Patch Set 2 : add forward decl, finish CreateOffscreenSurface #

Patch Set 3 : #

Patch Set 4 : typo fix #

Patch Set 5 : rebase #

Patch Set 6 : #

Patch Set 7 : #

Patch Set 8 : #

Patch Set 9 : #

Patch Set 10 : rebase #

Patch Set 11 : Fix bugs, extend comments, remove unused parameter #

Patch Set 12 : Respond to Chromium reviewers #

Patch Set 13 : Fix Windows compile error #

Patch Set 14 : #

Patch Set 15 : #

Patch Set 16 : #

Patch Set 17 #

Patch Set 18 #

Patch Set 19 #

Patch Set 20 : remove !_WIN64 restriction on error reporting #

Patch Set 21 : Review #

Patch Set 22 : Fix nested creation causing TaskManager crash? #

Patch Set 23 : Add missing #include #

Patch Set 24 : Consolidate Windows bitmap and DC code #

Patch Set 25 : remove more unnecessary functions #

Patch Set 26 : #include cleanup and header fix #

Patch Set 27 : Move some #includes into .cc files #

Patch Set 28 : expose CreateHBitmap() #

Patch Set 29 : properly merge both versions of CreateHBitmap() #

Patch Set 30 : fix another callsite #

Patch Set 31 : rebase #

Patch Set 32 : fix bad merge #

Messages