Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
157063002:
Add helpers for CGL types. (Closed)
|
Created:
6 years, 10 months ago by ccameron Modified:
6 years, 10 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, erikwright+watch_chromium.org, jam, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd helpers for CGL types.
Add base::ScopedTypeRef, which is similar to ScopedCFTypeRef,
but allows for parameterized retain and release functions. This is
necessary for types such as CGLContextObj, which have their own
retain and release methods (and for which calling CFRelease will
result in crashes).
Add some common typedefs of ScopedTypeRef to cgl_util in ui,
along with a scoped CGLContextObj make-current class.
BUG=245900
R=thakis@chromium.org,mark@chromium.org
TBR=garykac@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250077
Patch Set 1 : Trim down #Patch Set 2 : No defaults #
Total comments: 8
Patch Set 3 : Incorporate review feedback #
Total comments: 4
Patch Set 4 : Use Traits #Patch Set 5 : & cleanup #
Total comments: 23
Patch Set 6 : Incorporate review feedback #
Total comments: 4
Patch Set 7 : s/cgl_util/scoped_cgl/ #
Total comments: 1
Patch Set 8 : Remove unused include #
Messages
Total messages: 29 (0 generated)
I had made some remarks about doing this, and they where not enthusiastically received. The more I've thought about it, the more I'm finding myself absolutely convinced that this is the best way to deal with CGL (and related) types. I would go even further to say that we should never *ever* be using an raw CGLContextObj or pixel format (et al) -- we should always use these scoped types. It is extremely hard to verify correctness without this type of construct (and trivial with it). For instance, if you look at the use of CGLPixelFormatObject prior to https://chromiumcodereview.appspot.com/12383042, you can find that (especially in error cases), the pixel format object is leaked. Some suggestions were made to use the pattern of adding scoped deleter objects (a la CGLRendererInfoObjDeleter in gl_context_cgl.cc). That involves have 2 variables around (the object, and the scoper), and requires that they be initialized separately and yet kept in sync. That is a tremendous burden, especially if you want to pass these objects between functions. This scheme has one variable -- the refcount and the object are bound together -- and can be passed around with impunity (just like scoped_refptr). Also, the semantics of a "deleter" are somewhat stretched when they are applied to a retain/release type -- there is no delete. Of note is that for some types (like CGLRendererInfoObj, in the example given) there is a delete (which doesn't alias release), but not for most types (no extra credit for API consistency given). I put together a patch that switches content/browser changes for this in https://codereview.chromium.org/147493011/. That patch also has the scoped-set-current, which I feel is related, but I can separate it out if there are issues. Please let me know what you think about this scheme in general. Also, please disambiguate whether remarks fall into this "I like the idea, but I think it belongs in something other than base" (etc) from "I dislike this idea, full stop" (perhaps email might be a better medium than code review -- let's start here and go to email if it proves difficult.
+mark https://codereview.chromium.org/157063002/diff/110001/base/mac/scoped_typeref.h File base/mac/scoped_typeref.h (right): https://codereview.chromium.org/157063002/diff/110001/base/mac/scoped_typeref... base/mac/scoped_typeref.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/157063002/diff/110001/base/mac/scoped_typeref... base/mac/scoped_typeref.h:15: // ScopedTypeRef<> is a generalized form of ScopedCFTypeRef<>, for use with OTOH, you could look at ScopedCFTypeRef as being a specialized form of ScopedTypeRef. If we went this route, I'd like to see some relationship between them, perhaps even just a typedef of ScopedCFTypeRef as being a partial specialization of ScopedTypeRef with CFRetain/Release. Especially since there's massive duplication here. https://codereview.chromium.org/157063002/diff/110001/ui/gl/cgl_util.cc File ui/gl/cgl_util.cc (right): https://codereview.chromium.org/157063002/diff/110001/ui/gl/cgl_util.cc#newcode1 ui/gl/cgl_util.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. No (c). It's 2014. https://codereview.chromium.org/157063002/diff/110001/ui/gl/cgl_util.h File ui/gl/cgl_util.h (right): https://codereview.chromium.org/157063002/diff/110001/ui/gl/cgl_util.h#newcode1 ui/gl/cgl_util.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. no (c)
I think this is a good idea in general and agree with Avi's assessment to refactor ScopedCFTypeRef in terms of this new class, if you're going to do it at all.
Thanks. I've updated this to avoid the duplication and formally make ScopedCFTypeRef be a specialization of ScopedTypeRef. One wrinkle in this is that I had to parameterize the type used by Retain/Release separately. I assume that nobody will use ScopedTypeRef without a typedef, so the verbosity doesn't bother me. https://codereview.chromium.org/157063002/diff/110001/base/mac/scoped_typeref.h File base/mac/scoped_typeref.h (right): https://codereview.chromium.org/157063002/diff/110001/base/mac/scoped_typeref... base/mac/scoped_typeref.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/02/07 01:45:29, Avi wrote: > no (c) Done. https://codereview.chromium.org/157063002/diff/110001/base/mac/scoped_typeref... base/mac/scoped_typeref.h:15: // ScopedTypeRef<> is a generalized form of ScopedCFTypeRef<>, for use with On 2014/02/07 01:45:29, Avi wrote: > OTOH, you could look at ScopedCFTypeRef as being a specialized form of > ScopedTypeRef. If we went this route, I'd like to see some relationship between > them, perhaps even just a typedef of ScopedCFTypeRef as being a partial > specialization of ScopedTypeRef with CFRetain/Release. > > Especially since there's massive duplication here. Good call. Change ScopedCFTypeRef to be a subclass. https://codereview.chromium.org/157063002/diff/110001/ui/gl/cgl_util.cc File ui/gl/cgl_util.cc (right): https://codereview.chromium.org/157063002/diff/110001/ui/gl/cgl_util.cc#newcode1 ui/gl/cgl_util.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/02/07 01:45:29, Avi wrote: > No (c). It's 2014. Done. https://codereview.chromium.org/157063002/diff/110001/ui/gl/cgl_util.h File ui/gl/cgl_util.h (right): https://codereview.chromium.org/157063002/diff/110001/ui/gl/cgl_util.h#newcode1 ui/gl/cgl_util.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/02/07 01:45:29, Avi wrote: > no (c) Done.
The sucky thing about this is that every time you want to use a smart pointer for one of these wacky CF-like-but-not-quite types, you have to know and write not only its type, but also its release and retain functions and the type on which the release and retain functions operate. That’s a lot of typing. Your example in scoped_typeref.h makes the case very well: // base::ScopedTypeRef< // CGLContextObj, CGLContextObj, CGLRetainContext, CGLReleaseContext> // context; That’s way too much writing, way too much boilerplate setup garbage, just to arrange for context to be released properly. So…no. ScopedTypeRef as a general-purpose API is a no-go. Specializations of CFTypeRef, so that a ScopedCFTypeRef<CGLContextObj> knows how to release and retain itself, just like a ScopedCFTypeRef<CFDictionaryRef> knows how to release and retain itself, is an alternative model. ScopedTypeRef as you’ve written it could serve as the foundation for such a ScopedCFTypeRef. An alternative, keeping the design you’ve adopted, would be to get rid of the awful example in scoped_typeref.h’s comments that encourages people to use it directly. Provide an example that encourages using it by typedef (as you’ve done in cgl_util.h) or inheritance in a template class with a simpler interface (as your modified ScopedCFTypeRef does). Say that it’s only intended to be used as the basis for a usable scoper object type, and that it’s not intended to be used in code all by itself. Finally, I think that the three new template parameters should instead be handled in a traits class. And you can even make ScopedTypeRef use the traits this way template<typename T, typename Traits = ScopedTypeRefTraits<T>> class ScopedTypeRef so that people who want to extend it can write a specialization of ScopedTypeRefTraits and simplify their typedef to typedef base::ScopedTypeRef<CGLContextObj> ScopedCGLContextObjRef; https://codereview.chromium.org/157063002/diff/180001/base/mac/scoped_cftyper... File base/mac/scoped_cftyperef.h (right): https://codereview.chromium.org/157063002/diff/180001/base/mac/scoped_cftyper... base/mac/scoped_cftyperef.h:30: : public ScopedTypeRef<CFT, CFTypeRef, CFRetain, CFRelease> { 4 spaces, not 2. https://codereview.chromium.org/157063002/diff/180001/base/mac/scoped_typeref.h File base/mac/scoped_typeref.h (right): https://codereview.chromium.org/157063002/diff/180001/base/mac/scoped_typeref... base/mac/scoped_typeref.h:47: base::scoped_policy::OwnershipPolicy policy) Having a default policy here is kind of nice.
Okay, the idea of using a Traits structure was really the way to go. I've updated the patch with it, and I'm very happy with the results. Thanks!! (& ptal) https://codereview.chromium.org/157063002/diff/180001/base/mac/scoped_cftyper... File base/mac/scoped_cftyperef.h (right): https://codereview.chromium.org/157063002/diff/180001/base/mac/scoped_cftyper... base/mac/scoped_cftyperef.h:30: : public ScopedTypeRef<CFT, CFTypeRef, CFRetain, CFRelease> { On 2014/02/07 17:22:08, Mark Mentovai wrote: > 4 spaces, not 2. Done. https://codereview.chromium.org/157063002/diff/180001/base/mac/scoped_typeref.h File base/mac/scoped_typeref.h (right): https://codereview.chromium.org/157063002/diff/180001/base/mac/scoped_typeref... base/mac/scoped_typeref.h:47: base::scoped_policy::OwnershipPolicy policy) On 2014/02/07 17:22:08, Mark Mentovai wrote: > Having a default policy here is kind of nice. I've added this back ... but I didn't like the default ownership policy (the very first time I used it, I got it wrong).
https://codereview.chromium.org/157063002/diff/310001/base/mac/scoped_typeref.h File base/mac/scoped_typeref.h (right): https://codereview.chromium.org/157063002/diff/310001/base/mac/scoped_typeref... base/mac/scoped_typeref.h:15: // ScopedTypeRef<> is patterend after scoped_ptr<>, but maintains a ownership s/patterend/patterned/
This is pretty good. Much, much better with the traits. https://codereview.chromium.org/157063002/diff/310001/base/mac/scoped_cftyper... File base/mac/scoped_cftyperef.h (right): https://codereview.chromium.org/157063002/diff/310001/base/mac/scoped_cftyper... base/mac/scoped_cftyperef.h:28: template<typename CFT> No need to templatize this at all, since you’re specifying it explicitly when you instantiate ScopedTypeRef below. You can put this struct into a sub-namespace like “internal” to make it obvious that it’s an implementation detail of this file. (And I’m not an auto-complete user, but it might also make auto-completers less likely to suggest ScopedCFTypeRefTraits as an option to people who really just want ScopedCFTypeRef.) https://codereview.chromium.org/157063002/diff/310001/base/mac/scoped_cftyper... base/mac/scoped_cftyperef.h:40: : public ScopedTypeRef<CFT, ScopedCFTypeRefTraits<CFT> > { I think we only ever build with C++11 language support on Mac now, so if you wanted to close your template with >>, that should be fine. Line 47 also. https://codereview.chromium.org/157063002/diff/310001/base/mac/scoped_cftyper... base/mac/scoped_cftyperef.h:52: virtual ~ScopedCFTypeRef() {} Why is this virtual? https://codereview.chromium.org/157063002/diff/310001/base/mac/scoped_typeref.h File base/mac/scoped_typeref.h (right): https://codereview.chromium.org/157063002/diff/310001/base/mac/scoped_typeref... base/mac/scoped_typeref.h:48: template<typename T, typename Traits = ScopedTypeRefTraits<T> > The >> spelling option should be available here too. https://codereview.chromium.org/157063002/diff/310001/base/mac/scoped_typeref... base/mac/scoped_typeref.h:67: virtual ~ScopedTypeRef() { Why is this virtual? https://codereview.chromium.org/157063002/diff/310001/base/mac/scoped_typeref... base/mac/scoped_typeref.h:77: // This is to be used only to take ownership of objects that are created I like this. Less cumbersome than the existing pattern used for this kind of thing. I suggest InitializeInto as a name, along the same lines as WriteInto, used for a similar purpose for strings (strings/string_util.h). https://codereview.chromium.org/157063002/diff/310001/ui/gl/cgl_util.cc File ui/gl/cgl_util.cc (right): https://codereview.chromium.org/157063002/diff/310001/ui/gl/cgl_util.cc#newco... ui/gl/cgl_util.cc:11: : previous_context_(CGLGetCurrentContext(), base::scoped_policy::RETAIN) { 4-space indent. https://codereview.chromium.org/157063002/diff/310001/ui/gl/cgl_util.cc#newco... ui/gl/cgl_util.cc:13: DCHECK_EQ(error, kCGLNoError) << "CGLSetCurrentContext should never fail"; Do you deal with CGLError often? Do you CHECK on them often? If so, it may make sense to have a whole CGLERROR_LOG family (with CGLERROR_DCHECK) along the lines of base/mac/mac_logging.h. This doesn’t need to be a part of this change, and it doesn’t even need to be done at all, it’s just a suggestion based on past experience. https://codereview.chromium.org/157063002/diff/310001/ui/gl/cgl_util.h File ui/gl/cgl_util.h (right): https://codereview.chromium.org/157063002/diff/310001/ui/gl/cgl_util.h#newcode41 ui/gl/cgl_util.h:41: ScopedCGLSetCurrentContext(CGLContextObj context); explicit? https://codereview.chromium.org/157063002/diff/310001/ui/gl/cgl_util.h#newcode49 ui/gl/cgl_util.h:49: base::ScopedTypeRef<CGLContextObj> previous_context_; DISALLOW_COPY_AND_ASSIGN?
https://codereview.chromium.org/157063002/diff/310001/base/mac/scoped_cftyper... File base/mac/scoped_cftyperef.h (right): https://codereview.chromium.org/157063002/diff/310001/base/mac/scoped_cftyper... base/mac/scoped_cftyperef.h:40: : public ScopedTypeRef<CFT, ScopedCFTypeRefTraits<CFT> > { Does the compiler we use for iOS support C++11? This header file, via ScopedCFTypeRef, is definitely used by them.
Thanks! https://codereview.chromium.org/157063002/diff/310001/base/mac/scoped_cftyper... File base/mac/scoped_cftyperef.h (right): https://codereview.chromium.org/157063002/diff/310001/base/mac/scoped_cftyper... base/mac/scoped_cftyperef.h:28: template<typename CFT> On 2014/02/07 19:53:40, Mark Mentovai wrote: > No need to templatize this at all, since you’re specifying it explicitly when > you instantiate ScopedTypeRef below. > > You can put this struct into a sub-namespace like “internal” to make it obvious > that it’s an implementation detail of this file. (And I’m not an auto-complete > user, but it might also make auto-completers less likely to suggest > ScopedCFTypeRefTraits as an option to people who really just want > ScopedCFTypeRef.) Done. https://codereview.chromium.org/157063002/diff/310001/base/mac/scoped_cftyper... base/mac/scoped_cftyperef.h:40: : public ScopedTypeRef<CFT, ScopedCFTypeRefTraits<CFT> > { On 2014/02/07 19:59:23, Avi wrote: > Does the compiler we use for iOS support C++11? This header file, via > ScopedCFTypeRef, is definitely used by them. Changed (a different instance) to >> -- if the iOS trybots are unhappy I'll change it back. https://codereview.chromium.org/157063002/diff/310001/base/mac/scoped_cftyper... base/mac/scoped_cftyperef.h:52: virtual ~ScopedCFTypeRef() {} On 2014/02/07 19:53:40, Mark Mentovai wrote: > Why is this virtual? Removed (was here for style, not correctness). https://codereview.chromium.org/157063002/diff/310001/base/mac/scoped_typeref.h File base/mac/scoped_typeref.h (right): https://codereview.chromium.org/157063002/diff/310001/base/mac/scoped_typeref... base/mac/scoped_typeref.h:15: // ScopedTypeRef<> is patterend after scoped_ptr<>, but maintains a ownership On 2014/02/07 19:29:31, Avi wrote: > s/patterend/patterned/ Done. https://codereview.chromium.org/157063002/diff/310001/base/mac/scoped_typeref... base/mac/scoped_typeref.h:48: template<typename T, typename Traits = ScopedTypeRefTraits<T> > On 2014/02/07 19:53:40, Mark Mentovai wrote: > The >> spelling option should be available here too. Done. https://codereview.chromium.org/157063002/diff/310001/base/mac/scoped_typeref... base/mac/scoped_typeref.h:67: virtual ~ScopedTypeRef() { On 2014/02/07 19:53:40, Mark Mentovai wrote: > Why is this virtual? Not required for correctness (existant subclass has empty dtor), but I thought it was required for style. Removed. https://codereview.chromium.org/157063002/diff/310001/base/mac/scoped_typeref... base/mac/scoped_typeref.h:77: // This is to be used only to take ownership of objects that are created On 2014/02/07 19:53:41, Mark Mentovai wrote: > I like this. Less cumbersome than the existing pattern used for this kind of > thing. > > I suggest InitializeInto as a name, along the same lines as WriteInto, used for > a similar purpose for strings (strings/string_util.h). Thanks -- Done. https://codereview.chromium.org/157063002/diff/310001/ui/gl/cgl_util.cc File ui/gl/cgl_util.cc (right): https://codereview.chromium.org/157063002/diff/310001/ui/gl/cgl_util.cc#newco... ui/gl/cgl_util.cc:11: : previous_context_(CGLGetCurrentContext(), base::scoped_policy::RETAIN) { On 2014/02/07 19:53:41, Mark Mentovai wrote: > 4-space indent. Done. https://codereview.chromium.org/157063002/diff/310001/ui/gl/cgl_util.cc#newco... ui/gl/cgl_util.cc:13: DCHECK_EQ(error, kCGLNoError) << "CGLSetCurrentContext should never fail"; On 2014/02/07 19:53:41, Mark Mentovai wrote: > Do you deal with CGLError often? Do you CHECK on them often? If so, it may make > sense to have a whole CGLERROR_LOG family (with CGLERROR_DCHECK) along the lines > of base/mac/mac_logging.h. > > This doesn’t need to be a part of this change, and it doesn’t even need to be > done at all, it’s just a suggestion based on past experience. They're used pretty often, but not CHECKed too often (though perhaps they should be). I'll keep that in mind. https://codereview.chromium.org/157063002/diff/310001/ui/gl/cgl_util.h File ui/gl/cgl_util.h (right): https://codereview.chromium.org/157063002/diff/310001/ui/gl/cgl_util.h#newcode41 ui/gl/cgl_util.h:41: ScopedCGLSetCurrentContext(CGLContextObj context); On 2014/02/07 19:53:41, Mark Mentovai wrote: > explicit? Done. https://codereview.chromium.org/157063002/diff/310001/ui/gl/cgl_util.h#newcode49 ui/gl/cgl_util.h:49: base::ScopedTypeRef<CGLContextObj> previous_context_; On 2014/02/07 19:53:41, Mark Mentovai wrote: > DISALLOW_COPY_AND_ASSIGN? Done.
Great. LGTMark@chromium.org.
Thanks! kbr/thakis, can you do a quick ui/gl OWNER review? If there aren't any objections, I'll try to get this in this weekend.
FYI https://codereview.chromium.org/157063002/diff/500001/base/mac/scoped_typeref.h File base/mac/scoped_typeref.h (right): https://codereview.chromium.org/157063002/diff/500001/base/mac/scoped_typeref... base/mac/scoped_typeref.h:35: // CGLCreateContext(pixel_format, share_group, context.InitializeInto()); yes yes yes yes yes yes yes yes yes yes thank you
https://codereview.chromium.org/157063002/diff/500001/ui/gl/cgl_util.h File ui/gl/cgl_util.h (right): https://codereview.chromium.org/157063002/diff/500001/ui/gl/cgl_util.h#newcode6 ui/gl/cgl_util.h:6: #define UI_GL_CGL_UTIL_H_ We don't like _util files as they tend to become dumping grounds. Can you come up with a different name?
https://codereview.chromium.org/157063002/diff/500001/ui/gl/cgl_util.h File ui/gl/cgl_util.h (right): https://codereview.chromium.org/157063002/diff/500001/ui/gl/cgl_util.h#newcode6 ui/gl/cgl_util.h:6: #define UI_GL_CGL_UTIL_H_ On 2014/02/08 02:52:27, Nico wrote: > We don't like _util files as they tend to become dumping grounds. Can you come > up with a different name? Hmm ... any suggestions for names? I'm drawing something of a blank. The best scheme I've come up wit so far is - move the ScopedTypeRefTraits to base/mac/scoped_cgltyperef.h - put the scoped makecurrent in ui/gl/scoped_cgl_set_current.h/cc Is that more reasonable? The file is "base types (ScopedTypeRef) and patterns from base (Scoped set-current), applied to the CGL structures and type.
https://codereview.chromium.org/157063002/diff/500001/ui/gl/cgl_util.h File ui/gl/cgl_util.h (right): https://codereview.chromium.org/157063002/diff/500001/ui/gl/cgl_util.h#newcode6 ui/gl/cgl_util.h:6: #define UI_GL_CGL_UTIL_H_ On 2014/02/10 00:10:45, ccameron1 wrote: > On 2014/02/08 02:52:27, Nico wrote: > > We don't like _util files as they tend to become dumping grounds. Can you come > > up with a different name? > > Hmm ... any suggestions for names? I'm drawing something of a blank. The best > scheme I've come up wit so far is > - move the ScopedTypeRefTraits to base/mac/scoped_cgltyperef.h > - put the scoped makecurrent in ui/gl/scoped_cgl_set_current.h/cc > Is that more reasonable? > > The file is "base types (ScopedTypeRef) and patterns from base (Scoped > set-current), applied to the CGL structures and type. Maybe scoped_cgl.h?
Oh, I meant to say "lgtm" in my comment from friday too, looks like I forgot that :-)
Thanks! Changed to scoped_cgl (makes sense). There's also a scoped_make_current (but it only applies to wrapped contexts, which the browser doesn't use ATM, and doesn't do the refcount management that cgl does -- it may be that they can be merged later).
The CQ bit was checked by ccameron@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ccameron@chromium.org/157063002/670001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
https://codereview.chromium.org/157063002/diff/670001/ui/gl/gl.gyp File ui/gl/gl.gyp (right): https://codereview.chromium.org/157063002/diff/670001/ui/gl/gl.gyp#newcode253 ui/gl/gl.gyp:253: 'scoped_cgl.cc', Sort. s > g.
Thanks -- fixed. Btw, it looks like the mac build failure was legit -- including "base/logging.h" causes the error "remoting_uninstaller_app.mm:34:3: error: unexpected '@' in program". In this change, I included base/logging in scoped_typeref.h (to DCHECK for InitializeInto). Fortunately, scoped_cftyperef.h isn't actually used by this file, so I'm deleting the reference and TBRing the change. Why exactly @try doesn't work in these circumstances is referenced in WebCorePrefixMac.h /* When C++ exceptions are disabled, the C++ library defines |try| and |catch| * to allow C++ code that expects exceptions to build. These definitions * interfere with Objective-C++ uses of Objective-C exception handlers, which * use |@try| and |@catch|. As a workaround, undefine these macros. */
The CQ bit was checked by ccameron@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ccameron@chromium.org/157063002/960001
Message was sent while issue was closed.
Change committed as 250077
Message was sent while issue was closed.
LGTM
Message was sent while issue was closed.
Sorry for the late review, but LGTM. Nice generalization. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
