Alt Fix(?) For Issue 4282059 [DLL export Gr for Chrome Multi-DLL build]

[bsalomon, 2011-03-19 20:10:39]

Is this a viable alternate fix?

[twiz1, 2011-03-19 21:11:16]

On 2011/03/19 20:10:39, bsalomon wrote:
> Is this a viable alternate fix?

I think this is a viable alternative, in that it also makes GrGLInterface.h safe
for inclusion as an API header outside of chrome.  However, it is also a little
fraglie because any headers that make use of GrTypes.h will fail when used
outside of skia.

On a related note - should any of the GR functions other than the GL
configuration entries be exported as part of the 'API'?  My understanding is
that Gr is a layer hidden behind Skia.

Jeff

[twiz1, 2011-03-19 21:11:34]

LGTM

http://codereview.appspot.com/4301044/diff/4001/gpu/include/GrTypes.h
File gpu/include/GrTypes.h (right):

http://codereview.appspot.com/4301044/diff/4001/gpu/include/GrTypes.h#newcode25
gpu/include/GrTypes.h:25: #include <stdint.h>
Nit:  Include ordering.

[twiz1, 2011-03-19 21:23:33]

Also, I pulled this change into Chrome, along with Chrome CL
http://codereview.chromium.org/6677105/, and the shared build is sucessful.

Jeff

On 2011/03/19 21:11:34, twiz1 wrote:
> LGTM
> 
> http://codereview.appspot.com/4301044/diff/4001/gpu/include/GrTypes.h
> File gpu/include/GrTypes.h (right):
> 
>
http://codereview.appspot.com/4301044/diff/4001/gpu/include/GrTypes.h#newcode25
> gpu/include/GrTypes.h:25: #include <stdint.h>
> Nit:  Include ordering.

[reed1, 2011-03-21 11:40:53]

I like the move of stdint.h into GrTypes.h, thanks.

What is the massive change to skia.gyp. Can that be reverted? It appears that
every line changed.

http://codereview.appspot.com/4301044/diff/4001/gpu/include/GrConfig.h
File gpu/include/GrConfig.h (right):

http://codereview.appspot.com/4301044/diff/4001/gpu/include/GrConfig.h#newcod...
gpu/include/GrConfig.h:267: #define GrCrash(MSG) do { GrPrintf(MSG);
GrAlwaysAssert(false); } while (false)
Why was this changed from an inline to a macro?

[bsalomon, 2011-03-21 13:40:34]

Will update soon, going to do a test multi-dll build locally with the patch to
make sure it works. 

Jeff, I think there are a couple other places where Chrome will require GR_API.
GrContext is created explicitly inside Chrome (actually WebKit) and then plugged
into an SkCanvas.

http://codereview.appspot.com/4301044/diff/4001/gpu/include/GrConfig.h
File gpu/include/GrConfig.h (right):

http://codereview.appspot.com/4301044/diff/4001/gpu/include/GrConfig.h#newcod...
gpu/include/GrConfig.h:267: #define GrCrash(MSG) do { GrPrintf(MSG);
GrAlwaysAssert(false); } while (false)
GrAlwaysAssert(false) resolves to an expression that uses int64_t.

On 2011/03/21 11:40:53, reed1 wrote:
> Why was this changed from an inline to a macro?

[bsalomon, 2011-03-21 13:59:34]

I just noticed that Jeff already tested the multi-DLL build! If it works then I
guess we have all the GR_APIs we need (for now). As I mentioned in a comment
we'll want to actually make the GR_DLL build real so that everything that should
be public in Gr is exported.

Jeff, I made the includes alphabetical. Is that what your comment was about?

I agree it is a bit brittle because if app_base were to pull a different Gr
header it may reintroduce stdint.h. IMO it's a Chrome problem to not have a
standard header. More robust fixes are to either modify app_bases gyp to have a
path to a stdint or make a Gr user config file for chrome that defines the
types/macros required by Gr and blocks stdint.h inclusion.



On 2011/03/21 13:40:34, bsalomon wrote:
> Will update soon, going to do a test multi-dll build locally with the patch to
> make sure it works. 
> 
> Jeff, I think there are a couple other places where Chrome will require
GR_API.
> GrContext is created explicitly inside Chrome (actually WebKit) and then
plugged
> into an SkCanvas.
> 
> http://codereview.appspot.com/4301044/diff/4001/gpu/include/GrConfig.h
> File gpu/include/GrConfig.h (right):
> 
>
http://codereview.appspot.com/4301044/diff/4001/gpu/include/GrConfig.h#newcod...
> gpu/include/GrConfig.h:267: #define GrCrash(MSG) do { GrPrintf(MSG);
> GrAlwaysAssert(false); } while (false)
> GrAlwaysAssert(false) resolves to an expression that uses int64_t.
> 
> On 2011/03/21 11:40:53, reed1 wrote:
> > Why was this changed from an inline to a macro?

[reed1, 2011-03-21 14:04:50]

Can we have the sampleapp.sln build via shared libraries?

On Mon, Mar 21, 2011 at 9:59 AM,  <bsalomon@google.com> wrote:
> I just noticed that Jeff already tested the multi-DLL build! If it works
> then I guess we have all the GR_APIs we need (for now). As I mentioned
> in a comment we'll want to actually make the GR_DLL build real so that
> everything that should be public in Gr is exported.
>
> Jeff, I made the includes alphabetical. Is that what your comment was
> about?
>
> I agree it is a bit brittle because if app_base were to pull a different
> Gr header it may reintroduce stdint.h. IMO it's a Chrome problem to not
> have a standard header. More robust fixes are to either modify app_bases
> gyp to have a path to a stdint or make a Gr user config file for chrome
> that defines the types/macros required by Gr and blocks stdint.h
> inclusion.
>
>
>
> On 2011/03/21 13:40:34, bsalomon wrote:
>>
>> Will update soon, going to do a test multi-dll build locally with the
>
> patch to
>>
>> make sure it works.
>
>> Jeff, I think there are a couple other places where Chrome will
>
> require GR_API.
>>
>> GrContext is created explicitly inside Chrome (actually WebKit) and
>
> then plugged
>>
>> into an SkCanvas.
>
>> http://codereview.appspot.com/4301044/diff/4001/gpu/include/GrConfig.h
>> File gpu/include/GrConfig.h (right):
>
>
>
http://codereview.appspot.com/4301044/diff/4001/gpu/include/GrConfig.h#newcod...
>>
>> gpu/include/GrConfig.h:267: #define GrCrash(MSG) do { GrPrintf(MSG);
>> GrAlwaysAssert(false); } while (false)
>> GrAlwaysAssert(false) resolves to an expression that uses int64_t.
>
>> On 2011/03/21 11:40:53, reed1 wrote:
>> > Why was this changed from an inline to a macro?
>
>
>
> http://codereview.appspot.com/4301044/
>

[bsalomon, 2011-03-21 14:09:21]

I'll file a bug to do so. It'll require expanding what is exported via
SK_API and GR_API. Right now both are minimally inserted for Chrome.
Building all the libs (views, core, gr, animator, etc) of skia as separate
DLLs will be a good test for the robustness of SK_API/GR_API.

On Mon, Mar 21, 2011 at 10:03 AM, Mike Reed <reed@google.com> wrote:

> Can we have the sampleapp.sln build via shared libraries?
>
> On Mon, Mar 21, 2011 at 9:59 AM,  <bsalomon@google.com> wrote:
> > I just noticed that Jeff already tested the multi-DLL build! If it works
> > then I guess we have all the GR_APIs we need (for now). As I mentioned
> > in a comment we'll want to actually make the GR_DLL build real so that
> > everything that should be public in Gr is exported.
> >
> > Jeff, I made the includes alphabetical. Is that what your comment was
> > about?
> >
> > I agree it is a bit brittle because if app_base were to pull a different
> > Gr header it may reintroduce stdint.h. IMO it's a Chrome problem to not
> > have a standard header. More robust fixes are to either modify app_bases
> > gyp to have a path to a stdint or make a Gr user config file for chrome
> > that defines the types/macros required by Gr and blocks stdint.h
> > inclusion.
> >
> >
> >
> > On 2011/03/21 13:40:34, bsalomon wrote:
> >>
> >> Will update soon, going to do a test multi-dll build locally with the
> >
> > patch to
> >>
> >> make sure it works.
> >
> >> Jeff, I think there are a couple other places where Chrome will
> >
> > require GR_API.
> >>
> >> GrContext is created explicitly inside Chrome (actually WebKit) and
> >
> > then plugged
> >>
> >> into an SkCanvas.
> >
> >> http://codereview.appspot.com/4301044/diff/4001/gpu/include/GrConfig.h
> >> File gpu/include/GrConfig.h (right):
> >
> >
> >
>
http://codereview.appspot.com/4301044/diff/4001/gpu/include/GrConfig.h#newcod...
> >>
> >> gpu/include/GrConfig.h:267: #define GrCrash(MSG) do { GrPrintf(MSG);
> >> GrAlwaysAssert(false); } while (false)
> >> GrAlwaysAssert(false) resolves to an expression that uses int64_t.
> >
> >> On 2011/03/21 11:40:53, reed1 wrote:
> >> > Why was this changed from an inline to a macro?
> >
> >
> >
> > http://codereview.appspot.com/4301044/
> >
>

[twiz1, 2011-03-21 14:11:47]

On 2011/03/21 13:59:34, bsalomon wrote:
> I just noticed that Jeff already tested the multi-DLL build! If it works then
I
> guess we have all the GR_APIs we need (for now). As I mentioned in a comment
> we'll want to actually make the GR_DLL build real so that everything that
should
> be public in Gr is exported.
> 
> Jeff, I made the includes alphabetical. Is that what your comment was about?

Yes, that's what my nit was about.  = )

> 
> I agree it is a bit brittle because if app_base were to pull a different Gr
> header it may reintroduce stdint.h. IMO it's a Chrome problem to not have a
> standard header. More robust fixes are to either modify app_bases gyp to have
a
> path to a stdint or make a Gr user config file for chrome that defines the
> types/macros required by Gr and blocks stdint.h inclusion.

According to the comments in skia\include\config\sk_stdint.h, it seems that it's
a Windows issue - this header is not exported by the visual studio platform sdk.

Jeff

> 
> 
> 
> On 2011/03/21 13:40:34, bsalomon wrote:
> > Will update soon, going to do a test multi-dll build locally with the patch
to
> > make sure it works. 
> > 
> > Jeff, I think there are a couple other places where Chrome will require
> GR_API.
> > GrContext is created explicitly inside Chrome (actually WebKit) and then
> plugged
> > into an SkCanvas.
> > 
> > http://codereview.appspot.com/4301044/diff/4001/gpu/include/GrConfig.h
> > File gpu/include/GrConfig.h (right):
> > 
> >
>
http://codereview.appspot.com/4301044/diff/4001/gpu/include/GrConfig.h#newcod...
> > gpu/include/GrConfig.h:267: #define GrCrash(MSG) do { GrPrintf(MSG);
> > GrAlwaysAssert(false); } while (false)
> > GrAlwaysAssert(false) resolves to an expression that uses int64_t.
> > 
> > On 2011/03/21 11:40:53, reed1 wrote:
> > > Why was this changed from an inline to a macro?

[bsalomon, 2011-03-21 14:40:58]

Yeah I see that my local VS2010 includes stdint.h but VS2008 does not :(. If
it is just limited to Windows we could do the "make GrUserConfig able to
provide definitions and opt-out of stdint.h" thing and make a Chrome custom
user-config that includes stdint.h everywhere but windows. On Windows it
would define the necessary stdint types and macros directly (to be
equivalent to what's in VS2010's stdint.h).


On Mon, Mar 21, 2011 at 10:11 AM, <twiz@google.com> wrote:

> On 2011/03/21 13:59:34, bsalomon wrote:
>
>> I just noticed that Jeff already tested the multi-DLL build! If it
>>
> works then I
>
>> guess we have all the GR_APIs we need (for now). As I mentioned in a
>>
> comment
>
>> we'll want to actually make the GR_DLL build real so that everything
>>
> that should
>
>> be public in Gr is exported.
>>
>
>  Jeff, I made the includes alphabetical. Is that what your comment was
>>
> about?
>
> Yes, that's what my nit was about.  = )
>
>
>
>  I agree it is a bit brittle because if app_base were to pull a
>>
> different Gr
>
>> header it may reintroduce stdint.h. IMO it's a Chrome problem to not
>>
> have a
>
>> standard header. More robust fixes are to either modify app_bases gyp
>>
> to have a
>
>> path to a stdint or make a Gr user config file for chrome that defines
>>
> the
>
>> types/macros required by Gr and blocks stdint.h inclusion.
>>
>
> According to the comments in skia\include\config\sk_stdint.h, it seems
> that it's a Windows issue - this header is not exported by the visual
> studio platform sdk.
>
> Jeff
>
>
>
>
>
>  On 2011/03/21 13:40:34, bsalomon wrote:
>> > Will update soon, going to do a test multi-dll build locally with
>>
> the patch to
>
>> > make sure it works.
>> >
>> > Jeff, I think there are a couple other places where Chrome will
>>
> require
>
>> GR_API.
>> > GrContext is created explicitly inside Chrome (actually WebKit) and
>>
> then
>
>> plugged
>> > into an SkCanvas.
>> >
>> >
>>
> http://codereview.appspot.com/4301044/diff/4001/gpu/include/GrConfig.h
>
>> > File gpu/include/GrConfig.h (right):
>> >
>> >
>>
>
>
>
http://codereview.appspot.com/4301044/diff/4001/gpu/include/GrConfig.h#newcod...
>
>> > gpu/include/GrConfig.h:267: #define GrCrash(MSG) do { GrPrintf(MSG);
>> > GrAlwaysAssert(false); } while (false)
>> > GrAlwaysAssert(false) resolves to an expression that uses int64_t.
>> >
>> > On 2011/03/21 11:40:53, reed1 wrote:
>> > > Why was this changed from an inline to a macro?
>>
>
>
>
> http://codereview.appspot.com/4301044/
>

[bsalomon, 2011-03-21 15:00:31]

It looks like this problem was already solved for building skia in chrome.
There is ./src/skia/config/win/stdint.h. Can we just point app_base at that?

On Mon, Mar 21, 2011 at 10:40 AM, Brian Salomon <bsalomon@google.com> wrote:

> Yeah I see that my local VS2010 includes stdint.h but VS2008 does not :(.
> If it is just limited to Windows we could do the "make GrUserConfig able to
> provide definitions and opt-out of stdint.h" thing and make a Chrome custom
> user-config that includes stdint.h everywhere but windows. On Windows it
> would define the necessary stdint types and macros directly (to be
> equivalent to what's in VS2010's stdint.h).
>
>
> On Mon, Mar 21, 2011 at 10:11 AM, <twiz@google.com> wrote:
>
>> On 2011/03/21 13:59:34, bsalomon wrote:
>>
>>> I just noticed that Jeff already tested the multi-DLL build! If it
>>>
>> works then I
>>
>>> guess we have all the GR_APIs we need (for now). As I mentioned in a
>>>
>> comment
>>
>>> we'll want to actually make the GR_DLL build real so that everything
>>>
>> that should
>>
>>> be public in Gr is exported.
>>>
>>
>>  Jeff, I made the includes alphabetical. Is that what your comment was
>>>
>> about?
>>
>> Yes, that's what my nit was about.  = )
>>
>>
>>
>>  I agree it is a bit brittle because if app_base were to pull a
>>>
>> different Gr
>>
>>> header it may reintroduce stdint.h. IMO it's a Chrome problem to not
>>>
>> have a
>>
>>> standard header. More robust fixes are to either modify app_bases gyp
>>>
>> to have a
>>
>>> path to a stdint or make a Gr user config file for chrome that defines
>>>
>> the
>>
>>> types/macros required by Gr and blocks stdint.h inclusion.
>>>
>>
>> According to the comments in skia\include\config\sk_stdint.h, it seems
>> that it's a Windows issue - this header is not exported by the visual
>> studio platform sdk.
>>
>> Jeff
>>
>>
>>
>>
>>
>>  On 2011/03/21 13:40:34, bsalomon wrote:
>>> > Will update soon, going to do a test multi-dll build locally with
>>>
>> the patch to
>>
>>> > make sure it works.
>>> >
>>> > Jeff, I think there are a couple other places where Chrome will
>>>
>> require
>>
>>> GR_API.
>>> > GrContext is created explicitly inside Chrome (actually WebKit) and
>>>
>> then
>>
>>> plugged
>>> > into an SkCanvas.
>>> >
>>> >
>>>
>> http://codereview.appspot.com/4301044/diff/4001/gpu/include/GrConfig.h
>>
>>> > File gpu/include/GrConfig.h (right):
>>> >
>>> >
>>>
>>
>>
>>
http://codereview.appspot.com/4301044/diff/4001/gpu/include/GrConfig.h#newcod...
>>
>>> > gpu/include/GrConfig.h:267: #define GrCrash(MSG) do { GrPrintf(MSG);
>>> > GrAlwaysAssert(false); } while (false)
>>> > GrAlwaysAssert(false) resolves to an expression that uses int64_t.
>>> >
>>> > On 2011/03/21 11:40:53, reed1 wrote:
>>> > > Why was this changed from an inline to a macro?
>>>
>>
>>
>>
>> http://codereview.appspot.com/4301044/
>>
>
>

[bsalomon, 2011-03-21 18:46:15]

I updated the fix to *not* remove stdint.h from GrConfig since there is one
checked into Chrome already. I also added GR_API and SK_API to classes
required to compile the multi-dll build with use_skia_gpu=1.


On Mon, Mar 21, 2011 at 10:58 AM, Brian Salomon <bsalomon@google.com> wrote:

> It looks like this problem was already solved for building skia in chrome.
> There is ./src/skia/config/win/stdint.h. Can we just point app_base at that?
>
>
> On Mon, Mar 21, 2011 at 10:40 AM, Brian Salomon <bsalomon@google.com>wrote:
>
>> Yeah I see that my local VS2010 includes stdint.h but VS2008 does not :(.
>> If it is just limited to Windows we could do the "make GrUserConfig able to
>> provide definitions and opt-out of stdint.h" thing and make a Chrome custom
>> user-config that includes stdint.h everywhere but windows. On Windows it
>> would define the necessary stdint types and macros directly (to be
>> equivalent to what's in VS2010's stdint.h).
>>
>>
>> On Mon, Mar 21, 2011 at 10:11 AM, <twiz@google.com> wrote:
>>
>>> On 2011/03/21 13:59:34, bsalomon wrote:
>>>
>>>> I just noticed that Jeff already tested the multi-DLL build! If it
>>>>
>>> works then I
>>>
>>>> guess we have all the GR_APIs we need (for now). As I mentioned in a
>>>>
>>> comment
>>>
>>>> we'll want to actually make the GR_DLL build real so that everything
>>>>
>>> that should
>>>
>>>> be public in Gr is exported.
>>>>
>>>
>>>  Jeff, I made the includes alphabetical. Is that what your comment was
>>>>
>>> about?
>>>
>>> Yes, that's what my nit was about.  = )
>>>
>>>
>>>
>>>  I agree it is a bit brittle because if app_base were to pull a
>>>>
>>> different Gr
>>>
>>>> header it may reintroduce stdint.h. IMO it's a Chrome problem to not
>>>>
>>> have a
>>>
>>>> standard header. More robust fixes are to either modify app_bases gyp
>>>>
>>> to have a
>>>
>>>> path to a stdint or make a Gr user config file for chrome that defines
>>>>
>>> the
>>>
>>>> types/macros required by Gr and blocks stdint.h inclusion.
>>>>
>>>
>>> According to the comments in skia\include\config\sk_stdint.h, it seems
>>> that it's a Windows issue - this header is not exported by the visual
>>> studio platform sdk.
>>>
>>> Jeff
>>>
>>>
>>>
>>>
>>>
>>>  On 2011/03/21 13:40:34, bsalomon wrote:
>>>> > Will update soon, going to do a test multi-dll build locally with
>>>>
>>> the patch to
>>>
>>>> > make sure it works.
>>>> >
>>>> > Jeff, I think there are a couple other places where Chrome will
>>>>
>>> require
>>>
>>>> GR_API.
>>>> > GrContext is created explicitly inside Chrome (actually WebKit) and
>>>>
>>> then
>>>
>>>> plugged
>>>> > into an SkCanvas.
>>>> >
>>>> >
>>>>
>>> http://codereview.appspot.com/4301044/diff/4001/gpu/include/GrConfig.h
>>>
>>>> > File gpu/include/GrConfig.h (right):
>>>> >
>>>> >
>>>>
>>>
>>>
>>>
http://codereview.appspot.com/4301044/diff/4001/gpu/include/GrConfig.h#newcod...
>>>
>>>> > gpu/include/GrConfig.h:267: #define GrCrash(MSG) do { GrPrintf(MSG);
>>>> > GrAlwaysAssert(false); } while (false)
>>>> > GrAlwaysAssert(false) resolves to an expression that uses int64_t.
>>>> >
>>>> > On 2011/03/21 11:40:53, reed1 wrote:
>>>> > > Why was this changed from an inline to a macro?
>>>>
>>>
>>>
>>>
>>> http://codereview.appspot.com/4301044/
>>>
>>
>>
>

[twiz1, 2011-03-21 19:27:22]

LGTM.

Eventually, we should drop the .xproj and .sln files from the repro, and force
users to invoke gyp, correct?

To confirm:  You built Chrome with these changes without failure?  (Esp in the
shared-library config?)

[bsalomon, 2011-03-21 19:40:32]

Yeah, once we can build all the tools (gm, etc), I make sure we can build
everything from the command line on the Mac, etc. then we will remove the
IDE project files.

I was able to build after I modified the skia.gyp in Chrome. I had to:

Add GrGpuGLFixed.cpp
-and-
Add these lines
        ['OS=="win" and component=="shared_library"', {
          'defines': [
            'SKIA_DLL',
            'SKIA_IMPLEMENTATION=1',
>>          'GR_DLL=1',
>>          'GR_IMPLEMENTATION=1',
          ],
          'dependencies': [
            '../base/base.gyp:base',
          ],
          'direct_dependent_settings': {
            'defines': [
              'SKIA_DLL',
>>            'GR_DLL=1',
            ],
          },
        },],

Also, I didn't modify the app_base.gyp to find src/skia/config/win/stdint.h.
Instead in my skia tree inside chrome I reapplied the change to move
stdint.h from GrConfig.h to GrTypes.h and made GrGLInterface.h include
config rather than types.



On Mon, Mar 21, 2011 at 3:27 PM, <twiz@google.com> wrote:

> LGTM.
>
> Eventually, we should drop the .xproj and .sln files from the repro, and
> force users to invoke gyp, correct?
>
> To confirm:  You built Chrome with these changes without failure?  (Esp
> in the shared-library config?)
>
>
> http://codereview.appspot.com/4301044/
>

[twiz1, 2011-03-21 19:48:41]

On 2011/03/21 19:40:32, bsalomon wrote:
> Yeah, once we can build all the tools (gm, etc), I make sure we can build
> everything from the command line on the Mac, etc. then we will remove the
> IDE project files.
> 
> I was able to build after I modified the skia.gyp in Chrome. I had to:
> 
> Add GrGpuGLFixed.cpp
> -and-
> Add these lines
>         ['OS=="win" and component=="shared_library"', {
>           'defines': [
>             'SKIA_DLL',
>             'SKIA_IMPLEMENTATION=1',
> >>          'GR_DLL=1',
> >>          'GR_IMPLEMENTATION=1',
>           ],
>           'dependencies': [
>             '../base/base.gyp:base',
>           ],
>           'direct_dependent_settings': {
>             'defines': [
>               'SKIA_DLL',
> >>            'GR_DLL=1',
>             ],
>           },
>         },],
> 
> Also, I didn't modify the app_base.gyp to find src/skia/config/win/stdint.h.
> Instead in my skia tree inside chrome I reapplied the change to move
> stdint.h from GrConfig.h to GrTypes.h and made GrGLInterface.h include
> config rather than types.

LGTM!

I'm confident that this should allow the submit of my chrome CL: 6677105.

> 
> 
> 
> On Mon, Mar 21, 2011 at 3:27 PM, <mailto:twiz@google.com> wrote:
> 
> > LGTM.
> >
> > Eventually, we should drop the .xproj and .sln files from the repro, and
> > force users to invoke gyp, correct?
> >
> > To confirm:  You built Chrome with these changes without failure?  (Esp
> > in the shared-library config?)
> >
> >
> > http://codereview.appspot.com/4301044/
> >