allow createScalerContext to return null, and then have the GDI backend trigger that if we fail to …

allow createScalerContext to return null, and then have the GDI backend trigger that if we fail to create the necessary GDI resources

BUG=
R=bungeman@google.com

Committed: https://code.google.com/p/skia/source/detail?r=9957

[bungeman-skia, 2013-07-09 22:45:37]

The interface looks ok, but we can still run out of handles. It would be nice if
there were an easy way to know if there was handle pressure or not (simply for
the sake of the rest of the process, which may also want some handles). It's
easy to get the number of handles used in a process (GetGuiResources), but to
get the process limit means reading from the registry, as far as I can see.

I still think we need (in addition to this change) for the cache to be aware of
other factors (like a handle budget) in order to avoid starving the rest of the
process, which doesn't magically know to try to purge the cache.

https://codereview.chromium.org/18484005/diff/1/src/ports/SkFontHost_win.cpp
File src/ports/SkFontHost_win.cpp (right):

https://codereview.chromium.org/18484005/diff/1/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:433: fDC = CreateCompatibleDC(0);
We do check creation of this guy, and just return an empty glyph if we can't
create it. I think the idea of delay crating this was to avoid creating it until
we actually need the glyph masks. It may just be better to set this guy up in
the constructor and have just the one DC, since they both have the same font in
them.

https://codereview.chromium.org/18484005/diff/1/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:476: fBM = CreateDIBSection(fDC, &info,
DIB_RGB_COLORS, &fBits, 0, 0);
This is an HBITMAP, so this could fail too. This one is harder in that it is
actually something we resize at runtime. We could be squirrely and just TODO
this one.

https://codereview.chromium.org/18484005/diff/1/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:611: fHiResFont = CreateFontIndirect(&lf);
fHiResFont? I suppose things should still work without it, but that seems rather
fishy to have such a subtle change when we could probably just purge the cache
and give the user what they wanted.

https://codereview.chromium.org/18484005/diff/1/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:685: HFONT bitmapFont = CreateFontIndirect(&lf);
here (though this replaces fFont, so we could remove it from the fDDC, delete it
first to free up a handle, but this could fail anyway due to some other thread
snatching it up first)

https://codereview.chromium.org/18484005/diff/1/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:1683: SkScalerContext_Windows* ctx =
SkNEW_ARGS(SkScalerContext_Windows,
It may be easier to understand the intent with something like:

SkAutoTDelete<SkScalerContext_Windows> ctx(SkNEW_ARGS(SkScalerContext_Windows,
(const_cast<LogFontTypeface*>(this), desc)));

return ctx->isValid() ? ctx.detach() : NULL;

[reed1, 2013-07-10 13:59:04]

https://codereview.chromium.org/18484005/diff/1/src/ports/SkFontHost_win.cpp
File src/ports/SkFontHost_win.cpp (right):

https://codereview.chromium.org/18484005/diff/1/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:433: fDC = CreateCompatibleDC(0);
On 2013/07/09 22:45:37, bungeman1 wrote:
> We do check creation of this guy, and just return an empty glyph if we can't
> create it. I think the idea of delay crating this was to avoid creating it
until
> we actually need the glyph masks. It may just be better to set this guy up in
> the constructor and have just the one DC, since they both have the same font
in
> them.

Good optimization -- may do in a 2nd CL

https://codereview.chromium.org/18484005/diff/1/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:611: fHiResFont = CreateFontIndirect(&lf);
On 2013/07/09 22:45:37, bungeman1 wrote:
> fHiResFont? I suppose things should still work without it, but that seems
rather
> fishy to have such a subtle change when we could probably just purge the cache
> and give the user what they wanted.

Done.

https://codereview.chromium.org/18484005/diff/1/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:685: HFONT bitmapFont = CreateFontIndirect(&lf);
On 2013/07/09 22:45:37, bungeman1 wrote:
> here (though this replaces fFont, so we could remove it from the fDDC, delete
it
> first to free up a handle, but this could fail anyway due to some other thread
> snatching it up first)

I think this guy just works. If bitmapFont is 0, then we ultimately assign it to
fFont, and we will be marked as invalid.

https://codereview.chromium.org/18484005/diff/1/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:1683: SkScalerContext_Windows* ctx =
SkNEW_ARGS(SkScalerContext_Windows,
On 2013/07/09 22:45:37, bungeman1 wrote:
> It may be easier to understand the intent with something like:
> 
> SkAutoTDelete<SkScalerContext_Windows> ctx(SkNEW_ARGS(SkScalerContext_Windows,
> (const_cast<LogFontTypeface*>(this), desc)));
> 
> return ctx->isValid() ? ctx.detach() : NULL;

Probably a matter of taste, as I find the current way clearer, but I'll make the
change.

[bungeman-skia, 2013-07-10 14:58:25]

This is better than what we had before (in that we're mostly protecting
ourselves from handle exhaustion), so lgtm on the following provisos.

1) Open a bug stating that HDCOffscreen::draw contains calls to
CreateCompatibleDC and CreateDIBSection which are unprotected in the sense that
they do not attempt a cache gc and then try again.

2) Open a bug about how the cache of SkScalarContexts on Windows must have a
handle budget which can be set at compile time (to mirror the memory budget) or
better at runtime (since the number of available handles may vary). The program
in which Skia is embedded might not stick Skia in it's own process, and may need
it's own GDI handles. It should be able to reserve some for itself.

https://codereview.chromium.org/18484005/diff/1/src/ports/SkFontHost_win.cpp
File src/ports/SkFontHost_win.cpp (right):

https://codereview.chromium.org/18484005/diff/1/src/ports/SkFontHost_win.cpp#...
src/ports/SkFontHost_win.cpp:433: fDC = CreateCompatibleDC(0);
On 2013/07/10 13:59:04, reed1 wrote:
> On 2013/07/09 22:45:37, bungeman1 wrote:
> > We do check creation of this guy, and just return an empty glyph if we can't
> > create it. I think the idea of delay crating this was to avoid creating it
> until
> > we actually need the glyph masks. It may just be better to set this guy up
in
> > the constructor and have just the one DC, since they both have the same font
> in
> > them.
> 
> Good optimization -- may do in a 2nd CL

Changing this so this guy is part of initialization failure (where failure
invokes a cache gc) isn't really an optimization (in some sense its a
de-optimization); without it we will sometimes just not draw (when we could have
drawn, if only we had done a cache gc). Same issue for the HBITMAP below.

[reed1, 2013-07-10 15:39:00]

Committed patchset #2 manually as r9957 (presubmit successful).