Add option to build skia as a dll....

Add code needed to build skia as a windows dll within the chromium project.
- Export/import skia APIs if SKIA_DLL is defined.
- This change has no effect on skia.


Committed: http://code.google.com/p/skia/source/detail?r=944

[brettw, 2011-03-11 18:39:53]

I'll let Mike approve this or not. For background, this is basically what we did
with googleurl which is also a separate repo.

[Chris Guillory, 2011-03-11 18:58:08]

Brett, do you know if I should put SKIA_API on overrides of virtual
functions that are already declared with SKIA_API?

On Fri, Mar 11, 2011 at 10:39 AM, <brettw@chromium.org> wrote:

> I'll let Mike approve this or not. For background, this is basically
> what we did with googleurl which is also a separate repo.
>
>
> http://codereview.appspot.com/4282042/
>

[brettw, 2011-03-11 19:02:15]

On Fri, Mar 11, 2011 at 10:57 AM, Chris Guillory <ctguil@chromium.org> wrote:
> Brett, do you know if I should put SKIA_API on overrides of virtual
> functions that are already declared with SKIA_API?

I don't have a lot of experience with this, but I would imagine so.

Brett

[reed1, 2011-03-11 19:15:41]

adding more eyes...

I do not have much experience with using this for dlls. Off hand, I'm not sure I
want to export everything that you added these to.

1. How do we test this?
2. How do we know when we've added it to too many headers?

[bsalomon, 2011-03-11 19:20:18]

Would it make sense to apply dllexport / dllimport at the class level?

http://msdn.microsoft.com/en-us/library/81h27t8c(v=VS.100).aspx

On Fri, Mar 11, 2011 at 2:15 PM, <reed@google.com> wrote:

> adding more eyes...
>
> I do not have much experience with using this for dlls. Off hand, I'm
> not sure I want to export everything that you added these to.
>
> 1. How do we test this?
> 2. How do we know when we've added it to too many headers?
>
>
> http://codereview.appspot.com/4282042/
>

[Chris Guillory, 2011-03-11 19:22:35]

On 2011/03/11 19:15:41, reed1 wrote:
> adding more eyes...
> 
> I do not have much experience with using this for dlls. Off hand, I'm not sure
I
> want to export everything that you added these to.
> 
> 1. How do we test this?
I was able to build skia.dll in chrome by modifying these files and others from
within a chromium checkout. If the build is not multi-dll there should be no
effect.

> 2. How do we know when we've added it to too many headers?
The guidance on previous changes like this was to add it to all public functions
and methods.
http://codereview.chromium.org/2939004/

[reed1, 2011-03-11 19:29:35]

Hmmm. Not sure I buy the reasoning that we should put it everywhere, in case
we forget to test the build.

Are there other techniques that are any less invasive?
- adding something to the class definition itself?
- listing functions in an external file?

Also, assuming we want this, something like SK_API or SK_PUBLIC would better
match our macro naming style.

On Fri, Mar 11, 2011 at 2:22 PM, <ctguil@chromium.org> wrote:

> On 2011/03/11 19:15:41, reed1 wrote:
>
>> adding more eyes...
>>
>
>  I do not have much experience with using this for dlls. Off hand, I'm
>>
> not sure I
>
>> want to export everything that you added these to.
>>
>
>  1. How do we test this?
>>
> I was able to build skia.dll in chrome by modifying these files and
> others from within a chromium checkout. If the build is not multi-dll
> there should be no effect.
>
>
>  2. How do we know when we've added it to too many headers?
>>
> The guidance on previous changes like this was to add it to all public
> functions and methods.
> http://codereview.chromium.org/2939004/
>
>
>
> http://codereview.appspot.com/4282042/
>

[Chris Guillory, 2011-03-11 19:35:27]

On 2011/03/11 19:20:18, bsalomon wrote:
> Would it make sense to apply dllexport / dllimport at the class level?
When I did that I had a lot of warnings like:
warning C4091: '__declspec(dllexport)' : ignored on left of 'SkCanvas' when no
variable is declared

These warnings would be treated as errors in other projects:
...SkCanvas.h(863) : error C2220: warning treated as error - no 'object' file
generated
...SkCanvas.h(863) : warning C4091: '__declspec(dllimport)' : ignored on left of
'SkCanvas' when no variable is declared

Maybe we can ignore these warnings?

> 
> http://msdn.microsoft.com/en-us/library/81h27t8c%28v=VS.100%29.aspx
> 
> On Fri, Mar 11, 2011 at 2:15 PM, <mailto:reed@google.com> wrote:
> 
> > adding more eyes...
> >
> > I do not have much experience with using this for dlls. Off hand, I'm
> > not sure I want to export everything that you added these to.
> >
> > 1. How do we test this?
> > 2. How do we know when we've added it to too many headers?
> >
> >
> > http://codereview.appspot.com/4282042/
> >

[Chris Guillory, 2011-03-11 19:39:31]

On 2011/03/11 19:29:35, reed1 wrote:
> Hmmm. Not sure I buy the reasoning that we should put it everywhere, in case
> we forget to test the build.
> 
> Are there other techniques that are any less invasive?
> - adding something to the class definition itself?
Possibly, with ignoring the warnings.
> - listing functions in an external file?
It should be possible only decorate the functions that are required to build and
link chrome.
> 
> Also, assuming we want this, something like SK_API or SK_PUBLIC would better
> match our macro naming style.
> 
> On Fri, Mar 11, 2011 at 2:22 PM, <mailto:ctguil@chromium.org> wrote:
> 
> > On 2011/03/11 19:15:41, reed1 wrote:
> >
> >> adding more eyes...
> >>
> >
> >  I do not have much experience with using this for dlls. Off hand, I'm
> >>
> > not sure I
> >
> >> want to export everything that you added these to.
> >>
> >
> >  1. How do we test this?
> >>
> > I was able to build skia.dll in chrome by modifying these files and
> > others from within a chromium checkout. If the build is not multi-dll
> > there should be no effect.
> >
> >
> >  2. How do we know when we've added it to too many headers?
> >>
> > The guidance on previous changes like this was to add it to all public
> > functions and methods.
> > http://codereview.chromium.org/2939004/
> >
> >
> >
> > http://codereview.appspot.com/4282042/
> >

[bsalomon, 2011-03-11 19:48:05]

Are you sure you did

class __declspec(dllexport) SkCanvas : public SkRefCnt {

and not:

__declspec(dllexport) class SkCanvas : public SkRefCnt {

?

On Fri, Mar 11, 2011 at 2:35 PM, <ctguil@chromium.org> wrote:

> On 2011/03/11 19:20:18, bsalomon wrote:
>
>> Would it make sense to apply dllexport / dllimport at the class level?
>>
> When I did that I had a lot of warnings like:
> warning C4091: '__declspec(dllexport)' : ignored on left of 'SkCanvas'
> when no variable is declared
>
> These warnings would be treated as errors in other projects:
> ...SkCanvas.h(863) : error C2220: warning treated as error - no 'object'
> file generated
> ...SkCanvas.h(863) : warning C4091: '__declspec(dllimport)' : ignored on
> left of 'SkCanvas' when no variable is declared
>
> Maybe we can ignore these warnings?
>
>
>  http://msdn.microsoft.com/en-us/library/81h27t8c%28v=VS.100%29.aspx
>>
>
>  On Fri, Mar 11, 2011 at 2:15 PM, <mailto:reed@google.com> wrote:
>>
>
>  > adding more eyes...
>> >
>> > I do not have much experience with using this for dlls. Off hand,
>>
> I'm
>
>> > not sure I want to export everything that you added these to.
>> >
>> > 1. How do we test this?
>> > 2. How do we know when we've added it to too many headers?
>> >
>> >
>> > http://codereview.appspot.com/4282042/
>> >
>>
>
>
>
> http://codereview.appspot.com/4282042/
>

[Chris Guillory, 2011-03-11 20:02:27]

On 2011/03/11 19:48:05, bsalomon wrote:
> Are you sure you did
> 
> class __declspec(dllexport) SkCanvas : public SkRefCnt {
> 
> and not:
> 
> __declspec(dllexport) class SkCanvas : public SkRefCnt {
> 
> ?
I did it the wrong way. I was just able to get a smaller project to link
correctly doing it the right way. The only warnings were inside skia:
warning C4141: 'dllexport' : used more than once

Looks like this can be applied across the classes and be much less intrusive. 

> 
> On Fri, Mar 11, 2011 at 2:35 PM, <mailto:ctguil@chromium.org> wrote:
> 
> > On 2011/03/11 19:20:18, bsalomon wrote:
> >
> >> Would it make sense to apply dllexport / dllimport at the class level?
> >>
> > When I did that I had a lot of warnings like:
> > warning C4091: '__declspec(dllexport)' : ignored on left of 'SkCanvas'
> > when no variable is declared
> >
> > These warnings would be treated as errors in other projects:
> > ...SkCanvas.h(863) : error C2220: warning treated as error - no 'object'
> > file generated
> > ...SkCanvas.h(863) : warning C4091: '__declspec(dllimport)' : ignored on
> > left of 'SkCanvas' when no variable is declared
> >
> > Maybe we can ignore these warnings?
> >
> >
> >  http://msdn.microsoft.com/en-us/library/81h27t8c%2528v=VS.100%2529.aspx
> >>
> >
> >  On Fri, Mar 11, 2011 at 2:15 PM, <mailto:reed@google.com> wrote:
> >>
> >
> >  > adding more eyes...
> >> >
> >> > I do not have much experience with using this for dlls. Off hand,
> >>
> > I'm
> >
> >> > not sure I want to export everything that you added these to.
> >> >
> >> > 1. How do we test this?
> >> > 2. How do we know when we've added it to too many headers?
> >> >
> >> >
> >> > http://codereview.appspot.com/4282042/
> >> >
> >>
> >
> >
> >
> > http://codereview.appspot.com/4282042/
> >

[Chris Guillory, 2011-03-12 05:08:19]

Ready for another look.

On 2011/03/11 20:02:27, Chris Guillory wrote:
> On 2011/03/11 19:48:05, bsalomon wrote:
> > Are you sure you did
> > 
> > class __declspec(dllexport) SkCanvas : public SkRefCnt {
> > 
> > and not:
> > 
> > __declspec(dllexport) class SkCanvas : public SkRefCnt {
> > 
> > ?
> I did it the wrong way. I was just able to get a smaller project to link
> correctly doing it the right way. The only warnings were inside skia:
> warning C4141: 'dllexport' : used more than once
These warnings were unrelated.
> 
> Looks like this can be applied across the classes and be much less intrusive. 
> 
> > 
> > On Fri, Mar 11, 2011 at 2:35 PM, <mailto:ctguil@chromium.org> wrote:
> > 
> > > On 2011/03/11 19:20:18, bsalomon wrote:
> > >
> > >> Would it make sense to apply dllexport / dllimport at the class level?
> > >>
> > > When I did that I had a lot of warnings like:
> > > warning C4091: '__declspec(dllexport)' : ignored on left of 'SkCanvas'
> > > when no variable is declared
> > >
> > > These warnings would be treated as errors in other projects:
> > > ...SkCanvas.h(863) : error C2220: warning treated as error - no 'object'
> > > file generated
> > > ...SkCanvas.h(863) : warning C4091: '__declspec(dllimport)' : ignored on
> > > left of 'SkCanvas' when no variable is declared
> > >
> > > Maybe we can ignore these warnings?
> > >
> > >
> > > 
http://msdn.microsoft.com/en-us/library/81h27t8c%252528v=VS.100%252529.aspx
> > >>
> > >
> > >  On Fri, Mar 11, 2011 at 2:15 PM, <mailto:reed@google.com> wrote:
> > >>
> > >
> > >  > adding more eyes...
> > >> >
> > >> > I do not have much experience with using this for dlls. Off hand,
> > >>
> > > I'm
> > >
> > >> > not sure I want to export everything that you added these to.
> > >> >
> > >> > 1. How do we test this?
> > >> > 2. How do we know when we've added it to too many headers?
> > >> >
> > >> >
> > >> > http://codereview.appspot.com/4282042/
> > >> >
> > >>
> > >
> > >
> > >
> > > http://codereview.appspot.com/4282042/
> > >

[bsalomon, 2011-03-14 12:49:53]

Overall this seems OK to me, though I'm no expert on windows dll stuff. I had a
few minor inline comments.

http://codereview.appspot.com/4282042/diff/7002/include/core/SkColorPriv.h
File include/core/SkColorPriv.h (right):

http://codereview.appspot.com/4282042/diff/7002/include/core/SkColorPriv.h#ne...
include/core/SkColorPriv.h:238: uint32_t mask = 0xFF00FF;
Is this related to building as a DLL?

http://codereview.appspot.com/4282042/diff/7002/include/core/SkPreConfig.h
File include/core/SkPreConfig.h (right):

http://codereview.appspot.com/4282042/diff/7002/include/core/SkPreConfig.h#ne...
include/core/SkPreConfig.h:101: #if !defined(SKIA_IMPLEMENTATION)
I believe it's skia-style to indent the nested #if/#else/#endifs

http://codereview.appspot.com/4282042/diff/7002/include/core/SkTypes.h
File include/core/SkTypes.h (right):

http://codereview.appspot.com/4282042/diff/7002/include/core/SkTypes.h#newcod...
include/core/SkTypes.h:156: SK_API uint8_t     SkToU8(size_t);
It seems like we should do all or none of these, right? I guess the current CL
is the minimum to get Chrome working?

http://codereview.appspot.com/4282042/diff/7002/src/core/SkBitmapProcState_fi...
File src/core/SkBitmapProcState_filter.h (right):

http://codereview.appspot.com/4282042/diff/7002/src/core/SkBitmapProcState_fi...
src/core/SkBitmapProcState_filter.h:114: uint32_t mask = 0xFF00FF;
same Q. about whether this is related. Also, should mask be static const?

http://codereview.appspot.com/4282042/diff/7002/src/core/SkBitmapProcState_fi...
src/core/SkBitmapProcState_filter.h:144: uint32_t mask = 0xFF00FF;
Same Q.

[reed1, 2011-03-14 13:09:08]

I would feel better about the changes (and our ability to maintain/test it) if
we can also allow the Mac and Linux makefile builds to trigger this: so we build
a sharedlib for skia when we test the command-line tools (gm, bench, tests).

Just to reiterate my earlier comments: I'm fine with this idea, but given its
invasive nature, I want to be sure we can test and maintain it. Only testing it
in a multi-dll build of chrome is not broad enough.

Thanks for the name change. SK_API is short enough to fit on the lines :)

[reed1, 2011-03-14 17:38:08]

Do we need to add SK_API to the template classes?

[twiz1, 2011-03-14 17:55:50]

On 2011/03/14 17:38:08, reed1 wrote:
> Do we need to add SK_API to the template classes?

I was going to mention that.  My understanding is that you can dll export
template instances, but adding dll exports for template declarations does not
serve a beneficial purpose.

See KB article:  http://support.microsoft.com/kb/q168958/

[Chris Guillory, 2011-03-14 21:11:25]

http://codereview.appspot.com/4282042/diff/7002/include/core/SkColorPriv.h
File include/core/SkColorPriv.h (right):

http://codereview.appspot.com/4282042/diff/7002/include/core/SkColorPriv.h#ne...
include/core/SkColorPriv.h:238: uint32_t mask = 0xFF00FF;
On 2011/03/14 12:49:53, bsalomon wrote:
> Is this related to building as a DLL?
Yes. Link errors occur:
webcore_remaining.lib(TransparencyWin.obj) : error LNK2001: unresolved external
symbol "unsigned int const gMask_00FF00FF" (?gMask_00FF00FF@@3IB)

Looks like it'l link correctly if I just add SK_API to extern const uint32_t
gMask_00FF00FF. I've made that change.

http://codereview.appspot.com/4282042/diff/7002/include/core/SkPreConfig.h
File include/core/SkPreConfig.h (right):

http://codereview.appspot.com/4282042/diff/7002/include/core/SkPreConfig.h#ne...
include/core/SkPreConfig.h:101: #if !defined(SKIA_IMPLEMENTATION)
On 2011/03/14 12:49:53, bsalomon wrote:
> I believe it's skia-style to indent the nested #if/#else/#endifs

Done.

http://codereview.appspot.com/4282042/diff/7002/include/core/SkTypes.h
File include/core/SkTypes.h (right):

http://codereview.appspot.com/4282042/diff/7002/include/core/SkTypes.h#newcod...
include/core/SkTypes.h:156: SK_API uint8_t     SkToU8(size_t);
On 2011/03/14 12:49:53, bsalomon wrote:
> It seems like we should do all or none of these, right? I guess the current CL
> is the minimum to get Chrome working?
Agree. Done. Yes, this was the minimum needed to link Chrome.

http://codereview.appspot.com/4282042/diff/7002/src/core/SkBitmapProcState_fi...
File src/core/SkBitmapProcState_filter.h (right):

http://codereview.appspot.com/4282042/diff/7002/src/core/SkBitmapProcState_fi...
src/core/SkBitmapProcState_filter.h:114: uint32_t mask = 0xFF00FF;
On 2011/03/14 12:49:53, bsalomon wrote:
> same Q. about whether this is related. Also, should mask be static const?
Using gMask_00FF00FF is possible now. Makes sense to make it static const.

http://codereview.appspot.com/4282042/diff/7002/src/core/SkBitmapProcState_fi...
src/core/SkBitmapProcState_filter.h:144: uint32_t mask = 0xFF00FF;
On 2011/03/14 12:49:53, bsalomon wrote:
> Same Q.

Done.

[Chris Guillory, 2011-03-14 21:12:04]

On 2011/03/14 13:09:08, reed1 wrote:
> I would feel better about the changes (and our ability to maintain/test it) if
> we can also allow the Mac and Linux makefile builds to trigger this: so we
build
> a sharedlib for skia when we test the command-line tools (gm, bench, tests).
I'll look into this.

> 
> Just to reiterate my earlier comments: I'm fine with this idea, but given its
> invasive nature, I want to be sure we can test and maintain it. Only testing
it
> in a multi-dll build of chrome is not broad enough.
> 
> Thanks for the name change. SK_API is short enough to fit on the lines :)

[Chris Guillory, 2011-03-15 01:07:52]

On 2011/03/14 21:12:04, Chris Guillory wrote:
> On 2011/03/14 13:09:08, reed1 wrote:
> > I would feel better about the changes (and our ability to maintain/test it)
if
> > we can also allow the Mac and Linux makefile builds to trigger this: so we
> build
> > a sharedlib for skia when we test the command-line tools (gm, bench, tests).
> I'll look into this.
Hey Mike. So SK_API is specific to creating a windows DLL for skia. Creating a
shared library on Linux won't test the SK_API change (SK_API is only used under
windows).
http://groups.google.com/a/chromium.org/group/chromium-dev/msg/7f02c8f4de707593

This change, other than being invasive, shouldn't have any effect on skia
proper. It would actually make more sense if we could make this a chromium
change somehow. Perhaps that's harder to maintain, hence the reason mmulti-dll
changes have been upstreamed into the projects that currently build as separate
windows dll in chrome?

> 
> > 
> > Just to reiterate my earlier comments: I'm fine with this idea, but given
its
> > invasive nature, I want to be sure we can test and maintain it. Only testing
> it
> > in a multi-dll build of chrome is not broad enough.
> > 
> > Thanks for the name change. SK_API is short enough to fit on the lines :)

[reed1, 2011-03-15 18:50:19]

We may not choose to exercise this feature anywhere (for chrome) except on
Windows, but if we are going to add this, I would like to *be able to* exercise
it on other platforms as well.

If we don't offer that in this CL, that's fine. But I think it should be on our
list to do, as it will be additional confirmation that we've put the SK_API
macro in the right places.

On 2011/03/15 01:07:52, Chris Guillory wrote:
> On 2011/03/14 21:12:04, Chris Guillory wrote:
> > On 2011/03/14 13:09:08, reed1 wrote:
> > > I would feel better about the changes (and our ability to maintain/test
it)
> if
> > > we can also allow the Mac and Linux makefile builds to trigger this: so we
> > build
> > > a sharedlib for skia when we test the command-line tools (gm, bench,
tests).
> > I'll look into this.
> Hey Mike. So SK_API is specific to creating a windows DLL for skia. Creating a
> shared library on Linux won't test the SK_API change (SK_API is only used
under
> windows).
>
http://groups.google.com/a/chromium.org/group/chromium-dev/msg/7f02c8f4de707593
> 
> This change, other than being invasive, shouldn't have any effect on skia
> proper. It would actually make more sense if we could make this a chromium
> change somehow. Perhaps that's harder to maintain, hence the reason mmulti-dll
> changes have been upstreamed into the projects that currently build as
separate
> windows dll in chrome?
> 
> > 
> > > 
> > > Just to reiterate my earlier comments: I'm fine with this idea, but given
> its
> > > invasive nature, I want to be sure we can test and maintain it. Only
testing
> > it
> > > in a multi-dll build of chrome is not broad enough.
> > > 
> > > Thanks for the name change. SK_API is short enough to fit on the lines :)

[Chris Guillory, 2011-03-15 20:56:51]

On 2011/03/15 18:50:19, reed1 wrote:
> We may not choose to exercise this feature anywhere (for chrome) except on
> Windows, but if we are going to add this, I would like to *be able to*
exercise
> it on other platforms as well.
> 
> If we don't offer that in this CL, that's fine. But I think it should be on
our
> list to do, as it will be additional confirmation that we've put the SK_API
> macro in the right places.
I see. Offering this feature in skia for other platforms will mostly likely
involve putting SK_API on a lot more APIs. I've filed a bug for this.
http://code.google.com/p/skia/issues/detail?id=175

> 
> On 2011/03/15 01:07:52, Chris Guillory wrote:
> > On 2011/03/14 21:12:04, Chris Guillory wrote:
> > > On 2011/03/14 13:09:08, reed1 wrote:
> > > > I would feel better about the changes (and our ability to maintain/test
> it)
> > if
> > > > we can also allow the Mac and Linux makefile builds to trigger this: so
we
> > > build
> > > > a sharedlib for skia when we test the command-line tools (gm, bench,
> tests).
> > > I'll look into this.
> > Hey Mike. So SK_API is specific to creating a windows DLL for skia. Creating
a
> > shared library on Linux won't test the SK_API change (SK_API is only used
> under
> > windows).
> >
>
http://groups.google.com/a/chromium.org/group/chromium-dev/msg/7f02c8f4de707593
> > 
> > This change, other than being invasive, shouldn't have any effect on skia
> > proper. It would actually make more sense if we could make this a chromium
> > change somehow. Perhaps that's harder to maintain, hence the reason
mmulti-dll
> > changes have been upstreamed into the projects that currently build as
> separate
> > windows dll in chrome?
> > 
> > > 
> > > > 
> > > > Just to reiterate my earlier comments: I'm fine with this idea, but
given
> > its
> > > > invasive nature, I want to be sure we can test and maintain it. Only
> testing
> > > it
> > > > in a multi-dll build of chrome is not broad enough.
> > > > 
> > > > Thanks for the name change. SK_API is short enough to fit on the lines
:)

[reed1, 2011-03-15 21:05:36]

LGTM