|
|
|
Created:
15 years, 2 months ago by tedbo1 Modified:
15 years, 2 months ago Reviewers:
Steve VanDeBogart Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
The '#include "SkGLDevice.h"' from include/core/SkDevice.h requires internal Skia code to be added to the include search paths when using the deprecated API. This change adds back SkGLCanvas.cpp to avoid exposing SkGLDevice.h to the public API.
The change also includes an explicit virtual destructor on SkDeviceFactory to silence a -Wnon-virtual-dtor warning and allow for -Werror.
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address review comments. #Patch Set 3 : Fix style to be more like existing code. #
Total messages: 3
|
Steve VanDeBogart
LGTM with the following nits. As far as public vs. private API, it's not clear ...
|
15 years, 2 months ago (2010年11月10日 00:40:15 UTC) #1 | |||||||||||||||||||||||||||||||||||||||
LGTM with the following nits. As far as public vs. private API, it's not clear to me that Skia has successfully implemented that concept. Thinking about if though, in this particular case, SkGLCanvas.h should probably be moved to the public API because you must include it to use GL in the non-deprecated way. Happy to review that change if you are so inclined. Thanks for cleaning this up. http://codereview.appspot.com/3009041/diff/1/include/core/SkDevice.h File include/core/SkDevice.h (right): http://codereview.appspot.com/3009041/diff/1/include/core/SkDevice.h#newcode39 include/core/SkDevice.h:39: virtual ~SkDeviceFactory() { } Please put the definition in the .cpp file. http://codereview.appspot.com/3009041/diff/1/include/utils/SkGLCanvas.h File include/utils/SkGLCanvas.h (right): http://codereview.appspot.com/3009041/diff/1/include/utils/SkGLCanvas.h#newcode2 include/utils/SkGLCanvas.h:2: * Copyright (C) 2006 The Android Open Source Project I forgot to update the copyright date when I touched this file. Please update it to 2010. http://codereview.appspot.com/3009041/diff/1/src/gl/SkGLCanvas.cpp File src/gl/SkGLCanvas.cpp (right): http://codereview.appspot.com/3009041/diff/1/src/gl/SkGLCanvas.cpp#newcode1 src/gl/SkGLCanvas.cpp:1: #include "SkGLCanvas.h" Please include the copyright. http://codereview.appspot.com/3009041/diff/1/src/gl/SkGLCanvas.cpp#newcode7 src/gl/SkGLCanvas.cpp:7: /* static */ Style seems to be // static
On Tue, Nov 9, 2010 at 4:40 PM, <vandebo@chromium.org> wrote: > LGTM with the following nits. > > As far as public vs. private API, it's not clear to me that Skia has > successfully implemented that concept. Thinking about if though, in > this particular case, SkGLCanvas.h should probably be moved to the > public API because you must include it to use GL in the non-deprecated > way. Happy to review that change if you are so inclined. I assume you mean SkGLDevice.h here? Maybe SkGL.h would have to move as well? I don't mind doing it but I don't think I'm well set up to test. I just noticed the issue that I'm trying to address in a bit of code that uses the deprecated SkGLCanvas API. > > Thanks for cleaning this up. > > > http://codereview.appspot.com/3009041/diff/1/include/core/SkDevice.h > File include/core/SkDevice.h (right): > > http://codereview.appspot.com/3009041/diff/1/include/core/SkDevice.h#newcode39 > include/core/SkDevice.h:39: virtual ~SkDeviceFactory() { } > Please put the definition in the .cpp file. Done. > > http://codereview.appspot.com/3009041/diff/1/include/utils/SkGLCanvas.h > File include/utils/SkGLCanvas.h (right): > > http://codereview.appspot.com/3009041/diff/1/include/utils/SkGLCanvas.h#newcode2 > include/utils/SkGLCanvas.h:2: * Copyright (C) 2006 The Android Open > Source Project > I forgot to update the copyright date when I touched this file. Please > update it to 2010. Done. > > http://codereview.appspot.com/3009041/diff/1/src/gl/SkGLCanvas.cpp > File src/gl/SkGLCanvas.cpp (right): > > http://codereview.appspot.com/3009041/diff/1/src/gl/SkGLCanvas.cpp#newcode1 > src/gl/SkGLCanvas.cpp:1: #include "SkGLCanvas.h" > Please include the copyright. Done. I'm sorry that I missed that, but I checked a bunch of files in src/gl and most of them don't have a copyright notice. I assumed that maybe the standard was to leave it out and rely on a global notice. > > http://codereview.appspot.com/3009041/diff/1/src/gl/SkGLCanvas.cpp#newcode7 > src/gl/SkGLCanvas.cpp:7: /* static */ > Style seems to be // static Fixed. > > http://codereview.appspot.com/3009041/ > Thanks for looking at this change! If you approve the change keep in mind that I don't have commit access, so I think you'll need to commit it for me. ted