Remove BitmapPlatformDevice from NativeThemeWin

Remove BitmapPlatformDevice from NativeThemeWin

Replace deprecated APIs in NativeThemeWin::PaintIndirect().

R=pkasting@chromium.org,fmalita@chromium.org
BUG=543755
Committed: https://crrev.com/9601746a20dd756215cffdf2bd35c7cec3bd050e
Cr-Commit-Position: refs/heads/master@{#409509}

[tomhudson, 2016-07-27 14:55:21]

Description was changed from

==========
Remove BitmapPlatformDevice from NativeThemeWin

Replace deprecated APIs in NativeThemeWin::PaintIndirect().

R=pkasting@chromium.org
BUG=543755
==========

to

==========
Remove BitmapPlatformDevice from NativeThemeWin

Replace deprecated APIs in NativeThemeWin::PaintIndirect().

R=pkasting@chromium.org,fmalita@chromium.org
BUG=543755
==========

[tomhudson, 2016-07-27 14:55:21]

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

[tomhudson, 2016-08-01 21:50:21]

Peter, Florin, PTAL.

This doesn't get rid of PaintIndirect(), but at least it gets rid of the bad
Skia APIs, and I've verified against ToT that it doesn't have the same bugs my
previously-landed approach did.

[f(malita), 2016-08-02 16:35:50]

On 2016/08/01 21:50:21, tomhudson wrote:
> Peter, Florin, PTAL.
> 
> This doesn't get rid of PaintIndirect(), but at least it gets rid of the bad
> Skia APIs, and I've verified against ToT that it doesn't have the same bugs my
> previously-landed approach did.

Thanks for continuing to push on this.

The high/Skia-level changes look good to me, but I'm utterly unfamiliar with GDI
so I'll have to defer to pkasting.

[f(malita), 2016-08-02 16:36:13]

https://codereview.chromium.org/2182083002/diff/100001/skia/ext/skia_utils_wi...
File skia/ext/skia_utils_win.cc (left):

https://codereview.chromium.org/2182083002/diff/100001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:187: #if !defined(_WIN64)
This change seems unrelated.

[tomhudson, 2016-08-02 16:53:00]

On 2016/08/02 16:36:13, f(malita) wrote:
>
https://codereview.chromium.org/2182083002/diff/100001/skia/ext/skia_utils_wi...
> File skia/ext/skia_utils_win.cc (left):
> 
>
https://codereview.chromium.org/2182083002/diff/100001/skia/ext/skia_utils_wi...
> skia/ext/skia_utils_win.cc:187: #if !defined(_WIN64)
> This change seems unrelated.

That was discussed in reviews for the previous CL when Peter asked *why* it was
there; I think the conclusion suggested by cpu@
(https://codereview.chromium.org/1492353002/#msg17) was to remove it.

[Peter Kasting, 2016-08-03 01:40:26]

LGTM

Do you know what portion of the remaining original changes actually caused the
bugs?  I'm wondering if there's more of that refactoring we can still salvage.

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

https://codereview.chromium.org/2182083002/diff/100001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:175: const size_t backing_size = sizeof(*backing);
Tiny nit: () not actually necessary here ("sizeof *backing" should work)

https://codereview.chromium.org/2182083002/diff/100001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:215: if (SelectClipRgn(scoped_hdc.Get(), clip) ==
ERROR)
Nit: Could be combined with the next conditional

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

https://codereview.chromium.org/2182083002/diff/100001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.h:66: // Creates a surface writing to the pixels backing
the HDC's bitmap.
Tiny nit: I would say "|context|'s" rather than "the HDC's" here and below, just
to be clear you don't mean some other global HDC or something.

https://codereview.chromium.org/2182083002/diff/100001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.h:67: // Returns nullptr on error.
Nit: I think we typically just say "null" in comments rather than "nullptr",
though I don't know that that holds globally or is documented anywhere.

https://codereview.chromium.org/2182083002/diff/100001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.h:77: SK_API HDC CreateOffscreenSurface(int width, int
height);
Nit: Might want a "TODO(tomhudson): Return a ScopedCreateDC instead, once that
type is returnable.  See https://crbug.com/622442 .

Nit: Can this take a gfx::Size instead of width and height?

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

https://codereview.chromium.org/2182083002/diff/100001/ui/native_theme/native...
ui/native_theme/native_theme_win.cc:689:
(skia::CreateOffscreenSurface(rect.width(), rect.height()));
Is this wrapping what clang-format did?  It seems really weird to wrap before
the paren.

Does using assignment-init rather than constructor-init work here?  If so prefer
that (and it will get wrapped more nicely to boot).

[tomhudson, 2016-08-03 12:55:03]

> Do you know what portion of the remaining original changes actually caused the
> bugs?  I'm wondering if there's more of that refactoring we can still salvage.

What caused the bug was the total rewrite of that CL to just delete
PaintIndirect() and send everything through the PaintDirect() cause. I cc'd you
on an email back on July 26th asking chrome-windows and some graphics oldtimers
who might know the code well enough to know why we're going through
PaintIndirect() and if there's some other way to get rid of it.

I *do* at least want to follow up on our experiences by correcting the comment
block and getting rid of any unnecessary cases in the conditional that trigger
PaintIndirect(); I'll see if I can get a CL up for that before I leave for
vacation. I could probably do a deeper round of git blame spelunking than I have
so far.

Otherwise I think we've captured most or all of the other refactorings.

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

https://codereview.chromium.org/2182083002/diff/100001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:175: const size_t backing_size = sizeof(*backing);
On 2016/08/03 01:40:25, Peter Kasting wrote:
> Tiny nit: () not actually necessary here ("sizeof *backing" should work)

Done.

https://codereview.chromium.org/2182083002/diff/100001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.cc:215: if (SelectClipRgn(scoped_hdc.Get(), clip) ==
ERROR)
On 2016/08/03 01:40:25, Peter Kasting wrote:
> Nit: Could be combined with the next conditional

Done.

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

https://codereview.chromium.org/2182083002/diff/100001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.h:66: // Creates a surface writing to the pixels backing
the HDC's bitmap.
On 2016/08/03 01:40:25, Peter Kasting wrote:
> Tiny nit: I would say "|context|'s" rather than "the HDC's" here and below,
just
> to be clear you don't mean some other global HDC or something.

Done.

https://codereview.chromium.org/2182083002/diff/100001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.h:67: // Returns nullptr on error.
On 2016/08/03 01:40:25, Peter Kasting wrote:
> Nit: I think we typically just say "null" in comments rather than "nullptr",
> though I don't know that that holds globally or is documented anywhere.

Done.

https://codereview.chromium.org/2182083002/diff/100001/skia/ext/skia_utils_wi...
skia/ext/skia_utils_win.h:77: SK_API HDC CreateOffscreenSurface(int width, int
height);
On 2016/08/03 01:40:25, Peter Kasting wrote:
> Nit: Might want a "TODO(tomhudson): Return a ScopedCreateDC instead, once that
> type is returnable.  See https://crbug.com/622442 .

TODO(fmalita) for now.

> Nit: Can this take a gfx::Size instead of width and height?

That inverts the direction of dependency. I know crbug.com/13714 still argues we
should fold all of skia/ext into gfx, but until we do, this code isn't supposed
to depend on anything in gfx, and as far as I can tell only does so within
unittests.

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

https://codereview.chromium.org/2182083002/diff/100001/ui/native_theme/native...
ui/native_theme/native_theme_win.cc:689:
(skia::CreateOffscreenSurface(rect.width(), rect.height()));
On 2016/08/03 01:40:26, Peter Kasting wrote:
> Is this wrapping what clang-format did?  It seems really weird to wrap before
> the paren.
> 
> Does using assignment-init rather than constructor-init work here?  If so
prefer
> that (and it will get wrapped more nicely to boot).

Done.

[tomhudson, 2016-08-03 12:55:07]

The CQ bit was checked by tomhudson@google.com

[tomhudson, 2016-08-03 12:55:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2182083002/#ps120001
(title: "respond to review")

[commit-bot: I haz the power, 2016-08-03 12:55:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 13:26:40]

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

[commit-bot: I haz the power, 2016-08-03 13:26:41]

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

[tomhudson, 2016-08-03 13:31:53]

The CQ bit was checked by tomhudson@google.com

[tomhudson, 2016-08-03 13:31:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2182083002/#ps140001
(title: "Switch back to initialization-via-construction once")

[commit-bot: I haz the power, 2016-08-03 13:32:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tomhudson, 2016-08-03 13:32:42]

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

https://codereview.chromium.org/2182083002/diff/100001/ui/native_theme/native...
ui/native_theme/native_theme_win.cc:689:
(skia::CreateOffscreenSurface(rect.width(), rect.height()));
On 2016/08/03 01:40:26, Peter Kasting wrote:
> Is this wrapping what clang-format did?  It seems really weird to wrap before
> the paren.
> 
> Does using assignment-init rather than constructor-init work here?  If so
prefer
> that (and it will get wrapped more nicely to boot).

Unfortunately, looks like we need constructor-init, but I gave it the wrapping
you prefer in patch set 8.

[commit-bot: I haz the power, 2016-08-03 14:29:29]

Description was changed from

==========
Remove BitmapPlatformDevice from NativeThemeWin

Replace deprecated APIs in NativeThemeWin::PaintIndirect().

R=pkasting@chromium.org,fmalita@chromium.org
BUG=543755
==========

to

==========
Remove BitmapPlatformDevice from NativeThemeWin

Replace deprecated APIs in NativeThemeWin::PaintIndirect().

R=pkasting@chromium.org,fmalita@chromium.org
BUG=543755
==========

[commit-bot: I haz the power, 2016-08-03 14:29:30]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-03 14:30:55]

Description was changed from

==========
Remove BitmapPlatformDevice from NativeThemeWin

Replace deprecated APIs in NativeThemeWin::PaintIndirect().

R=pkasting@chromium.org,fmalita@chromium.org
BUG=543755
==========

to

==========
Remove BitmapPlatformDevice from NativeThemeWin

Replace deprecated APIs in NativeThemeWin::PaintIndirect().

R=pkasting@chromium.org,fmalita@chromium.org
BUG=543755

Committed: https://crrev.com/9601746a20dd756215cffdf2bd35c7cec3bd050e
Cr-Commit-Position: refs/heads/master@{#409509}
==========

[commit-bot: I haz the power, 2016-08-03 14:30:56]

Patchset 8 (id:??) landed as
https://crrev.com/9601746a20dd756215cffdf2bd35c7cec3bd050e
Cr-Commit-Position: refs/heads/master@{#409509}