Issue 6263046: Serialize support for GDI. (Closed)
|
Created:
12 years, 6 months ago by bungeman Modified:
12 years, 6 months ago CC:
skia-review_googlegroups.com Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
Patch Set 1 #Patch Set 2 : Use SkNEW instead of new. #
Total comments: 14
Patch Set 3 : Address comments. #
Total comments: 1
MessagesTotal messages: 8
The change to SkRefCnt to add SkBlockRef was because I changed the SkStreams here to use SkAutoTUnref and I forgot to remove one of the obj->unref() calls. Since the SkAutoTUnref is the one which owns the ref it should block easy access to the ref and unref to mark such mistakes as compile time errors.
Sign in to reply to this message.
Since I was harping on it the other day, might as well use SkNEW instead of new while I'm thinking about it.
Sign in to reply to this message.
http://codereview.appspot.com/6263046/diff/5/include/core/SkRefCnt.h File include/core/SkRefCnt.h (right): http://codereview.appspot.com/6263046/diff/5/include/core/SkRefCnt.h#newcode116 include/core/SkRefCnt.h:116: If we're going to add this to such a core file - we should really want to use it (in many places) - we need to document it - nit: don't need "template" on its own line http://codereview.appspot.com/6263046/diff/5/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (left): http://codereview.appspot.com/6263046/diff/5/src/ports/SkFontHost_win.cpp#old... src/ports/SkFontHost_win.cpp:1164: SkDEBUGFAIL("SkFontHost::Serialize unimplemented"); non-const parameters are typically pointers instead of refs http://codereview.appspot.com/6263046/diff/5/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/6263046/diff/5/src/ports/SkFontHost_win.cpp#new... src/ports/SkFontHost_win.cpp:1174: // accounted for). SkAutoSTMalloc will avoid the dynamic new/delete most of the time Really, we want a utf8->ucs2 with a length limiter built-in, so we don't have to (a) sometimes allocate/convert more than we need, and (b) can avoid the copy all together. http://codereview.appspot.com/6263046/diff/5/src/ports/SkFontHost_win.cpp#new... src/ports/SkFontHost_win.cpp:1178: wideFamilyName, str_len); LF_FACESIZE - 1 ? http://codereview.appspot.com/6263046/diff/5/src/ports/SkFontHost_win.cpp#new... src/ports/SkFontHost_win.cpp:1180: delete [] wideFamilyName; Do we really need L'\0' instead of just 0 ? http://codereview.appspot.com/6263046/diff/5/src/ports/SkFontHost_win.cpp#new... src/ports/SkFontHost_win.cpp:1193: // Allocate a buffer (str_len already has terminating null accounted for). Just use s->setLength(str_len) to allocate space in the string, and s->writable_str() to get a ptr you can write into. No need for the temp allocation. http://codereview.appspot.com/6263046/diff/5/src/ports/SkFontHost_win.cpp#new... src/ports/SkFontHost_win.cpp:1206: const LogFontTypeface* face = static_cast<const LogFontTypeface*>(rawFace); option to pass face->style() to the descriptor's constructor. http://codereview.appspot.com/6263046/diff/5/src/ports/SkFontHost_win.cpp#new... src/ports/SkFontHost_win.cpp:1230: SkFontDescriptor descriptor(stream); These two local vars are ok, but sort of not-needed, since the accessors can be use directly in the call to CreateTypeface(...)
Sign in to reply to this message.
http://codereview.appspot.com/6263046/diff/5/src/ports/SkFontHost_win.cpp File src/ports/SkFontHost_win.cpp (right): http://codereview.appspot.com/6263046/diff/5/src/ports/SkFontHost_win.cpp#new... src/ports/SkFontHost_win.cpp:1174: // accounted for). On 2012/06/01 16:32:20, reed1 wrote: > SkAutoSTMalloc will avoid the dynamic new/delete most of the time > > Really, we want a utf8->ucs2 with a length limiter built-in, so we don't have to > (a) sometimes allocate/convert more than we need, and (b) can avoid the copy all > together. For later clean up, both here and above. http://codereview.appspot.com/6263046/diff/5/src/ports/SkFontHost_win.cpp#new... src/ports/SkFontHost_win.cpp:1178: wideFamilyName, str_len); On 2012/06/01 16:32:20, reed1 wrote: > LF_FACESIZE - 1 ? Why not? http://codereview.appspot.com/6263046/diff/5/src/ports/SkFontHost_win.cpp#new... src/ports/SkFontHost_win.cpp:1180: delete [] wideFamilyName; On 2012/06/01 16:32:20, reed1 wrote: > Do we really need L'\0' instead of just 0 ? Using 0 would work, but I believe this makes it more obvious what is happening. No mental conversions or anything are required this way -- it just obviously works. http://codereview.appspot.com/6263046/diff/5/src/ports/SkFontHost_win.cpp#new... src/ports/SkFontHost_win.cpp:1193: // Allocate a buffer (str_len already has terminating null accounted for). On 2012/06/01 16:32:20, reed1 wrote: > Just use > > s->setLength(str_len) > > to allocate space in the string, and > > s->writable_str() > > to get a ptr you can write into. No need for the temp allocation. Done. http://codereview.appspot.com/6263046/diff/5/src/ports/SkFontHost_win.cpp#new... src/ports/SkFontHost_win.cpp:1206: const LogFontTypeface* face = static_cast<const LogFontTypeface*>(rawFace); On 2012/06/01 16:32:20, reed1 wrote: > option to pass face->style() to the descriptor's constructor. Done. http://codereview.appspot.com/6263046/diff/5/src/ports/SkFontHost_win.cpp#new... src/ports/SkFontHost_win.cpp:1230: SkFontDescriptor descriptor(stream); On 2012/06/01 16:32:20, reed1 wrote: > These two local vars are ok, but sort of not-needed, since the accessors can be > use directly in the call to CreateTypeface(...) Done.
Sign in to reply to this message.
On 2012/06/01 14:17:16, bungeman wrote: > The change to SkRefCnt to add SkBlockRef was because I changed the SkStreams > here to use SkAutoTUnref and I forgot to remove one of the obj->unref() calls. > Since the SkAutoTUnref is the one which owns the ref it should block easy access > to the ref and unref to mark such mistakes as compile time errors. I disagree with the premise here. SkAUtoTUnref doesn't hold *the* reference, it holds a reference and there may be others. It doesn't seem like the errors that this change could catch are worth the extra machinery. Those errors will be caught further down the line anyway with Chrome's memory bots and as I understand it there are plans to bring memory bots to the Skia waterfall.
Sign in to reply to this message.
lgtm w/ request for comment http://codereview.appspot.com/6263046/diff/7001/include/core/SkRefCnt.h File include/core/SkRefCnt.h (left): http://codereview.appspot.com/6263046/diff/7001/include/core/SkRefCnt.h#oldco... include/core/SkRefCnt.h:144: T* operator->() { return fObj; } Please add a 1-line comment at least, documenting why this is helpful.
Sign in to reply to this message.
On 2012/06/01 18:24:49, Steve VanDeBogart wrote: > On 2012/06/01 14:17:16, bungeman wrote: > > The change to SkRefCnt to add SkBlockRef was because I changed the SkStreams > > here to use SkAutoTUnref and I forgot to remove one of the obj->unref() calls. > > Since the SkAutoTUnref is the one which owns the ref it should block easy > access > > to the ref and unref to mark such mistakes as compile time errors. > > I disagree with the premise here. SkAUtoTUnref doesn't hold *the* reference, it > holds a reference and there may be others. It doesn't seem like the errors that > this change could catch are worth the extra machinery. Those errors will be > caught further down the line anyway with Chrome's memory bots and as I > understand it there are plans to bring memory bots to the Skia waterfall. Of course it holds *the* reference, it holds *the* reference which it currently controls because someone passed it in. There exist no other references in the context. All scoped pointers which wrap intrusive reference counted things do this as a matter of course, see SkTScopedComPtr and Chromium's base/win/scoped_comptr.h. There's no point in having a heavy memory checker find these sorts of problems later when it's possible for your IDE/compiler to tell you immediately. There is no extra generated code, it's all just types.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||
