first draft of runtime config. system

first draft of runtime config. system


BUG=

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

[Humper, 2013-01-12 22:13:47]

https://codereview.appspot.com/7098051/diff/1/include/core/SkTDArray.h
File include/core/SkTDArray.h (right):

https://codereview.appspot.com/7098051/diff/1/include/core/SkTDArray.h#newcod...
include/core/SkTDArray.h:114: }
This was handy because sometimes I have a pointer to one of these arrays and
it's pretty awkward to do (*array)[i]

https://codereview.appspot.com/7098051/diff/1/src/ports/SkOSFile_stdio.cpp
File src/ports/SkOSFile_stdio.cpp (right):

https://codereview.appspot.com/7098051/diff/1/src/ports/SkOSFile_stdio.cpp#ne...
src/ports/SkOSFile_stdio.cpp:44: ssize_t sk_getline(char **lineptr, size_t *n,
SkFILE *f) {
These will need implementations in the other ports/ files; those are not done
yet.

[bsalomon, 2013-01-14 14:27:59]

Rob also ran into the need for a develop vs production build. You guys should
coordinate.

Since we'll be adding a new build configuration, I think we'll need to grep for
'configurations' in gyp(i) files to update various flags and such that are
configuration dependent. i.e.:

http://code.google.com/p/skia/source/browse/trunk/gyp/common_conditions.gypi#43

https://codereview.appspot.com/7098051/diff/1001/gyp/common.gypi
File gyp/common.gypi (right):

https://codereview.appspot.com/7098051/diff/1001/gyp/common.gypi#newcode97
gyp/common.gypi:97: 'SK_PRODUCTION=1',
It might make it easier to land this and not mess up Chrome by instead making
SK_DEVELOP=1 enable the new code rather than a macro that disables it.

https://codereview.appspot.com/7098051/diff/1001/gyp/core.gyp
File gyp/core.gyp (right):

https://codereview.appspot.com/7098051/diff/1001/gyp/core.gyp#newcode20
gyp/core.gyp:20: '../include/utils',
Is it kosher to include utils in core? Maybe totally find, I just don't know.

https://codereview.appspot.com/7098051/diff/1001/include/core/SkOSFile.h
File include/core/SkOSFile.h (right):

https://codereview.appspot.com/7098051/diff/1001/include/core/SkOSFile.h#newc...
include/core/SkOSFile.h:56: ssize_t sk_getline(char **lineptr, size_t *n, SkFILE
*stream);
Is ssize_t always defined? I thought it wasn't standard but supported by some
std libs. Maybe ptrdiff_t?

https://codereview.appspot.com/7098051/diff/1001/include/utils/SkRTConf.h
File include/utils/SkRTConf.h (right):

https://codereview.appspot.com/7098051/diff/1001/include/utils/SkRTConf.h#new...
include/utils/SkRTConf.h:80: template <typename T> bool parse(const char *name,
T& value);
We generally only pass things as ref if they are also const. Pass by ptr for
output params.

https://codereview.appspot.com/7098051/diff/1001/include/utils/SkRTConf.h#new...
include/utils/SkRTConf.h:92: SkRTConf<T>::SkRTConf(const char *name, const T
&defaultValue, const char *description)
We don't need to say "inline"?

https://codereview.appspot.com/7098051/diff/1001/src/core/SkGraphics.cpp
File src/core/SkGraphics.cpp (right):

https://codereview.appspot.com/7098051/diff/1001/src/core/SkGraphics.cpp#newc...
src/core/SkGraphics.cpp:121: #ifndef SK_PRODUCTION
Maybe this should go first (validate before we setup the effects and such).

https://codereview.appspot.com/7098051/diff/1001/src/core/SkStream.cpp
File src/core/SkStream.cpp (right):

https://codereview.appspot.com/7098051/diff/1001/src/core/SkStream.cpp#newcod...
src/core/SkStream.cpp:793: {
style-nit: { on previous line.

https://codereview.appspot.com/7098051/diff/1001/src/core/SkStream.cpp#newcod...
src/core/SkStream.cpp:798: {
ditto

https://codereview.appspot.com/7098051/diff/1001/src/utils/SkRTConf.cpp
File src/utils/SkRTConf.cpp (right):

https://codereview.appspot.com/7098051/diff/1001/src/utils/SkRTConf.cpp#newco...
src/utils/SkRTConf.cpp:35: char *tmp = (char *) malloc(commentptr-line+1);
sk_malloc (and sk_free)

[reed1, 2013-01-14 15:00:27]

https://codereview.appspot.com/7098051/diff/1001/include/core/SkStream.h
File include/core/SkStream.h (right):

https://codereview.appspot.com/7098051/diff/1001/include/core/SkStream.h#newc...
include/core/SkStream.h:379: 
Is this different than SkFILEWStream(stderr) ?

https://codereview.appspot.com/7098051/diff/1001/include/core/SkString.h
File include/core/SkString.h (right):

https://codereview.appspot.com/7098051/diff/1001/include/core/SkString.h#newc...
include/core/SkString.h:42: 
Lets try writing SkStrContains in terms of SkStrFind... or delete SkStrContains
if no one is using it.

https://codereview.appspot.com/7098051/diff/1001/include/core/SkString.h#newc...
include/core/SkString.h:120: }
/** Return the offset into this string for the found substring, or -1 if not
found. */

[Humper, 2013-01-14 15:48:54]

https://codereview.appspot.com/7098051/diff/1001/gyp/common.gypi
File gyp/common.gypi (right):

https://codereview.appspot.com/7098051/diff/1001/gyp/common.gypi#newcode97
gyp/common.gypi:97: 'SK_PRODUCTION=1',
On 2013/01/14 14:27:59, bsalomon wrote:
> It might make it easier to land this and not mess up Chrome by instead making
> SK_DEVELOP=1 enable the new code rather than a macro that disables it.

agree.  I will coordinate with Jim since it sounds like he's making a developer
target.

https://codereview.appspot.com/7098051/diff/1001/include/core/SkOSFile.h
File include/core/SkOSFile.h (right):

https://codereview.appspot.com/7098051/diff/1001/include/core/SkOSFile.h#newc...
include/core/SkOSFile.h:56: ssize_t sk_getline(char **lineptr, size_t *n, SkFILE
*stream);
On 2013/01/14 14:27:59, bsalomon wrote:
> Is ssize_t always defined? I thought it wasn't standard but supported by some
> std libs. Maybe ptrdiff_t?

ptrdiff_t should work okay -- the getline function that this is built on top of
returns ssize_t, although I think these types are the same except on systems
like 16 bit windows with large memory model enabled.  Unlikely that we're going
to port skia to that anytime soon, yes?

https://codereview.appspot.com/7098051/diff/1001/include/core/SkStream.h
File include/core/SkStream.h (right):

https://codereview.appspot.com/7098051/diff/1001/include/core/SkStream.h#newc...
include/core/SkStream.h:379: 
On 2013/01/14 15:00:28, reed1 wrote:
> Is this different than SkFILEWStream(stderr) ?

Well, it has a shorter name, and I didn't know you could do that.

https://codereview.appspot.com/7098051/diff/1001/include/core/SkString.h
File include/core/SkString.h (right):

https://codereview.appspot.com/7098051/diff/1001/include/core/SkString.h#newc...
include/core/SkString.h:42: 
On 2013/01/14 15:00:28, reed1 wrote:
> Lets try writing SkStrContains in terms of SkStrFind... or delete
SkStrContains
> if no one is using it.

Will do.

https://codereview.appspot.com/7098051/diff/1001/include/utils/SkRTConf.h
File include/utils/SkRTConf.h (right):

https://codereview.appspot.com/7098051/diff/1001/include/utils/SkRTConf.h#new...
include/utils/SkRTConf.h:92: SkRTConf<T>::SkRTConf(const char *name, const T
&defaultValue, const char *description)
On 2013/01/14 14:27:59, bsalomon wrote:
> We don't need to say "inline"?

No, not on clang anyway :)  I'll be amused to see how many other builds this CL
breaks.  At least I have a couple of other development environments I can test
it on before I submit it though.

https://codereview.appspot.com/7098051/diff/1001/src/core/SkGraphics.cpp
File src/core/SkGraphics.cpp (right):

https://codereview.appspot.com/7098051/diff/1001/src/core/SkGraphics.cpp#newc...
src/core/SkGraphics.cpp:121: #ifndef SK_PRODUCTION
On 2013/01/14 14:27:59, bsalomon wrote:
> Maybe this should go first (validate before we setup the effects and such).

yes, definitely -- one might want to use a configuration knob in things that
happen before this.

https://codereview.appspot.com/7098051/diff/1001/src/core/SkStream.cpp
File src/core/SkStream.cpp (right):

https://codereview.appspot.com/7098051/diff/1001/src/core/SkStream.cpp#newcod...
src/core/SkStream.cpp:793: {
On 2013/01/14 14:27:59, bsalomon wrote:
> style-nit: { on previous line.

I will address this by deleting the whole class.  Also noting that the functions
immediately above these suffer from the same style problems :)

One day I'm going to make an entire pass through the code and fix all
whitespace.  If I'm going to have to retrain my fingers, I will NOT look at
other code that has foo( bar );

https://codereview.appspot.com/7098051/diff/1001/src/utils/SkRTConf.cpp
File src/utils/SkRTConf.cpp (right):

https://codereview.appspot.com/7098051/diff/1001/src/utils/SkRTConf.cpp#newco...
src/utils/SkRTConf.cpp:35: char *tmp = (char *) malloc(commentptr-line+1);
On 2013/01/14 14:27:59, bsalomon wrote:
> sk_malloc (and sk_free)

will sk_free work with things that are allocated with regular malloc? 
sk_getline calls the clib getline, which can allocate memory for you and
obviously isn't using the sk_ versions of things.

[bsalomon, 2013-01-14 16:02:57]

https://codereview.appspot.com/7098051/diff/1001/include/core/SkOSFile.h
File include/core/SkOSFile.h (right):

https://codereview.appspot.com/7098051/diff/1001/include/core/SkOSFile.h#newc...
include/core/SkOSFile.h:56: ssize_t sk_getline(char **lineptr, size_t *n, SkFILE
*stream);
On 2013/01/14 15:48:54, Humper wrote:
> On 2013/01/14 14:27:59, bsalomon wrote:
> > Is ssize_t always defined? I thought it wasn't standard but supported by
some
> > std libs. Maybe ptrdiff_t?
> 
> ptrdiff_t should work okay -- the getline function that this is built on top
of
> returns ssize_t, although I think these types are the same except on systems
> like 16 bit windows with large memory model enabled.  Unlikely that we're
going
> to port skia to that anytime soon, yes?

I should think not.

https://codereview.appspot.com/7098051/diff/1001/src/utils/SkRTConf.cpp
File src/utils/SkRTConf.cpp (right):

https://codereview.appspot.com/7098051/diff/1001/src/utils/SkRTConf.cpp#newco...
src/utils/SkRTConf.cpp:35: char *tmp = (char *) malloc(commentptr-line+1);
On 2013/01/14 15:48:54, Humper wrote:
> On 2013/01/14 14:27:59, bsalomon wrote:
> > sk_malloc (and sk_free)
> 
> will sk_free work with things that are allocated with regular malloc? 
> sk_getline calls the clib getline, which can allocate memory for you and
> obviously isn't using the sk_ versions of things.

Hmm... we probably shouldn't mix sk_malloc and free. Maybe this is an exception
where it is OK to call malloc directly.

[Humper, 2013-01-14 17:05:42]

On 2013/01/14 16:02:57, bsalomon wrote:
> https://codereview.appspot.com/7098051/diff/1001/include/core/SkOSFile.h
> File include/core/SkOSFile.h (right):
> 
>
https://codereview.appspot.com/7098051/diff/1001/include/core/SkOSFile.h#newc...
> include/core/SkOSFile.h:56: ssize_t sk_getline(char **lineptr, size_t *n,
SkFILE
> *stream);
> On 2013/01/14 15:48:54, Humper wrote:
> > On 2013/01/14 14:27:59, bsalomon wrote:
> > > Is ssize_t always defined? I thought it wasn't standard but supported by
> some
> > > std libs. Maybe ptrdiff_t?
> > 
> > ptrdiff_t should work okay -- the getline function that this is built on top
> of
> > returns ssize_t, although I think these types are the same except on systems
> > like 16 bit windows with large memory model enabled.  Unlikely that we're
> going
> > to port skia to that anytime soon, yes?
> 
> I should think not.
> 
> https://codereview.appspot.com/7098051/diff/1001/src/utils/SkRTConf.cpp
> File src/utils/SkRTConf.cpp (right):
> 
>
https://codereview.appspot.com/7098051/diff/1001/src/utils/SkRTConf.cpp#newco...
> src/utils/SkRTConf.cpp:35: char *tmp = (char *) malloc(commentptr-line+1);
> On 2013/01/14 15:48:54, Humper wrote:
> > On 2013/01/14 14:27:59, bsalomon wrote:
> > > sk_malloc (and sk_free)
> > 
> > will sk_free work with things that are allocated with regular malloc? 
> > sk_getline calls the clib getline, which can allocate memory for you and
> > obviously isn't using the sk_ versions of things.
> 
> Hmm... we probably shouldn't mix sk_malloc and free. Maybe this is an
exception
> where it is OK to call malloc directly.

I rewrote as much as I could to wrap things in sk_malloc, even if it meant doing
an extra copy, since this isn't performance critical and I hate the idea that
calls like sk_getline() might return something that you can't free with sk_free.

Should have another version for you to look at shortly.

[Humper, 2013-01-14 18:12:35]

On 2013/01/14 17:05:42, Humper wrote:
> On 2013/01/14 16:02:57, bsalomon wrote:
> > https://codereview.appspot.com/7098051/diff/1001/include/core/SkOSFile.h
> > File include/core/SkOSFile.h (right):
> > 
> >
>
https://codereview.appspot.com/7098051/diff/1001/include/core/SkOSFile.h#newc...
> > include/core/SkOSFile.h:56: ssize_t sk_getline(char **lineptr, size_t *n,
> SkFILE
> > *stream);
> > On 2013/01/14 15:48:54, Humper wrote:
> > > On 2013/01/14 14:27:59, bsalomon wrote:
> > > > Is ssize_t always defined? I thought it wasn't standard but supported by
> > some
> > > > std libs. Maybe ptrdiff_t?
> > > 
> > > ptrdiff_t should work okay -- the getline function that this is built on
top
> > of
> > > returns ssize_t, although I think these types are the same except on
systems
> > > like 16 bit windows with large memory model enabled.  Unlikely that we're
> > going
> > > to port skia to that anytime soon, yes?
> > 
> > I should think not.
> > 
> > https://codereview.appspot.com/7098051/diff/1001/src/utils/SkRTConf.cpp
> > File src/utils/SkRTConf.cpp (right):
> > 
> >
>
https://codereview.appspot.com/7098051/diff/1001/src/utils/SkRTConf.cpp#newco...
> > src/utils/SkRTConf.cpp:35: char *tmp = (char *) malloc(commentptr-line+1);
> > On 2013/01/14 15:48:54, Humper wrote:
> > > On 2013/01/14 14:27:59, bsalomon wrote:
> > > > sk_malloc (and sk_free)
> > > 
> > > will sk_free work with things that are allocated with regular malloc? 
> > > sk_getline calls the clib getline, which can allocate memory for you and
> > > obviously isn't using the sk_ versions of things.
> > 
> > Hmm... we probably shouldn't mix sk_malloc and free. Maybe this is an
> exception
> > where it is OK to call malloc directly.
> 
> I rewrote as much as I could to wrap things in sk_malloc, even if it meant
doing
> an extra copy, since this isn't performance critical and I hate the idea that
> calls like sk_getline() might return something that you can't free with
sk_free.
> 
> Should have another version for you to look at shortly.

Actually further investigation into the getline mystery reveals that only
SampleApp on iOS even attempts to compile / link the special SkOSFile_iOS port,
and SampleApp doesn't build on iOS for other reasons (it brings in SkOSFile_iOS
and SkOSFile_stdio, which creates duplicate symbols), so I'm going to ignore the
iOS build issue for now.  

I was able to build gm on iOS with my CL without any issues.

[bsalomon, 2013-01-14 18:17:05]

On 2013/01/14 18:12:35, Humper wrote:
> On 2013/01/14 17:05:42, Humper wrote:
> > On 2013/01/14 16:02:57, bsalomon wrote:
> > > https://codereview.appspot.com/7098051/diff/1001/include/core/SkOSFile.h
> > > File include/core/SkOSFile.h (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/7098051/diff/1001/include/core/SkOSFile.h#newc...
> > > include/core/SkOSFile.h:56: ssize_t sk_getline(char **lineptr, size_t *n,
> > SkFILE
> > > *stream);
> > > On 2013/01/14 15:48:54, Humper wrote:
> > > > On 2013/01/14 14:27:59, bsalomon wrote:
> > > > > Is ssize_t always defined? I thought it wasn't standard but supported
by
> > > some
> > > > > std libs. Maybe ptrdiff_t?
> > > > 
> > > > ptrdiff_t should work okay -- the getline function that this is built on
> top
> > > of
> > > > returns ssize_t, although I think these types are the same except on
> systems
> > > > like 16 bit windows with large memory model enabled.  Unlikely that
we're
> > > going
> > > > to port skia to that anytime soon, yes?
> > > 
> > > I should think not.
> > > 
> > > https://codereview.appspot.com/7098051/diff/1001/src/utils/SkRTConf.cpp
> > > File src/utils/SkRTConf.cpp (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/7098051/diff/1001/src/utils/SkRTConf.cpp#newco...
> > > src/utils/SkRTConf.cpp:35: char *tmp = (char *) malloc(commentptr-line+1);
> > > On 2013/01/14 15:48:54, Humper wrote:
> > > > On 2013/01/14 14:27:59, bsalomon wrote:
> > > > > sk_malloc (and sk_free)
> > > > 
> > > > will sk_free work with things that are allocated with regular malloc? 
> > > > sk_getline calls the clib getline, which can allocate memory for you and
> > > > obviously isn't using the sk_ versions of things.
> > > 
> > > Hmm... we probably shouldn't mix sk_malloc and free. Maybe this is an
> > exception
> > > where it is OK to call malloc directly.
> > 
> > I rewrote as much as I could to wrap things in sk_malloc, even if it meant
> doing
> > an extra copy, since this isn't performance critical and I hate the idea
that
> > calls like sk_getline() might return something that you can't free with
> sk_free.
> > 
> > Should have another version for you to look at shortly.
> 
> Actually further investigation into the getline mystery reveals that only
> SampleApp on iOS even attempts to compile / link the special SkOSFile_iOS
port,
> and SampleApp doesn't build on iOS for other reasons (it brings in
SkOSFile_iOS
> and SkOSFile_stdio, which creates duplicate symbols), so I'm going to ignore
the
> iOS build issue for now.  
> 
> I was able to build gm on iOS with my CL without any issues.

lgtm