Add ability to wire up sharelist in glcontext creation

Add ability to wire up sharelist in glcontext creation

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1604993005

Committed: https://skia.googlesource.com/skia/+/b59d1bc7a8596f346d57a9cfcd461dddc6d75edb

[joshua.litt, 2016-01-19 21:49:35]

Description was changed from

==========
Add ability to wire up sharelist in glcontext creation

BUG=skia:
==========

to

==========
Add ability to wire up sharelist in glcontext creation

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[joshualitt, 2016-01-19 21:50:04]

joshualitt@google.com changed reviewers:
+ bsalomon@google.com, joshualitt@google.com

[bsalomon, 2016-01-19 22:00:57]

https://codereview.chromium.org/1604993005/diff/1/include/gpu/gl/SkGLContext.h
File include/gpu/gl/SkGLContext.h (right):

https://codereview.chromium.org/1604993005/diff/1/include/gpu/gl/SkGLContext....
include/gpu/gl/SkGLContext.h:121: * the library.
Should comment on shareList. Also, name doesn't seem intuitive to me as it's not
a list.

Also, the param must be a context previously returned by
SkCreatePlatformGLContext. Otherwise it may be of the a unexpected subclass
(angle, osmesa, ...) .

https://codereview.chromium.org/1604993005/diff/1/src/gpu/GrContextFactory.h
File src/gpu/GrContextFactory.h (right):

https://codereview.chromium.org/1604993005/diff/1/src/gpu/GrContextFactory.h#...
src/gpu/GrContextFactory.h:124: SkGLContext* shareList = nullptr);
Does this really need to be part of GrContextFactory? I don't think our other
tools need it and the implementation in the cpp doesn't really work correctly.

https://codereview.chromium.org/1604993005/diff/1/src/gpu/gl/mac/SkCreatePlat...
File src/gpu/gl/mac/SkCreatePlatformGLContext_mac.cpp (right):

https://codereview.chromium.org/1604993005/diff/1/src/gpu/gl/mac/SkCreatePlat...
src/gpu/gl/mac/SkCreatePlatformGLContext_mac.cpp:112: SkGLContext*
SkCreatePlatformGLContext(GrGLStandard forcedGpuAPI, SkGLContext*) {
I think these backends where it is unimplemented should fail if the sharedList
is non-null since the result would be surprising.

[joshualitt, 2016-01-20 14:36:56]

https://codereview.chromium.org/1604993005/diff/1/include/gpu/gl/SkGLContext.h
File include/gpu/gl/SkGLContext.h (right):

https://codereview.chromium.org/1604993005/diff/1/include/gpu/gl/SkGLContext....
include/gpu/gl/SkGLContext.h:121: * the library.
On 2016/01/19 22:00:57, bsalomon wrote:
> Should comment on shareList. Also, name doesn't seem intuitive to me as it's
not
> a list.
> 
> Also, the param must be a context previously returned by
> SkCreatePlatformGLContext. Otherwise it may be of the a unexpected subclass
> (angle, osmesa, ...) .

Acknowledged.

https://codereview.chromium.org/1604993005/diff/1/src/gpu/GrContextFactory.h
File src/gpu/GrContextFactory.h (right):

https://codereview.chromium.org/1604993005/diff/1/src/gpu/GrContextFactory.h#...
src/gpu/GrContextFactory.h:124: SkGLContext* shareList = nullptr);
On 2016/01/19 22:00:57, bsalomon wrote:
> Does this really need to be part of GrContextFactory? I don't think our other
> tools need it and the implementation in the cpp doesn't really work correctly.

Acknowledged.

https://codereview.chromium.org/1604993005/diff/1/src/gpu/gl/mac/SkCreatePlat...
File src/gpu/gl/mac/SkCreatePlatformGLContext_mac.cpp (right):

https://codereview.chromium.org/1604993005/diff/1/src/gpu/gl/mac/SkCreatePlat...
src/gpu/gl/mac/SkCreatePlatformGLContext_mac.cpp:112: SkGLContext*
SkCreatePlatformGLContext(GrGLStandard forcedGpuAPI, SkGLContext*) {
On 2016/01/19 22:00:57, bsalomon wrote:
> I think these backends where it is unimplemented should fail if the sharedList
> is non-null since the result would be surprising.

Acknowledged.

[bsalomon, 2016-01-20 15:26:07]

lgtm, suggestions inline

https://codereview.chromium.org/1604993005/diff/20001/include/gpu/gl/SkGLCont...
File include/gpu/gl/SkGLContext.h (right):

https://codereview.chromium.org/1604993005/diff/20001/include/gpu/gl/SkGLCont...
include/gpu/gl/SkGLContext.h:118: * with SkCreatePlatformGLContext.  NULL
indicates that no sharing is to take place.
Can you rewrap the rest of the comment to agree with your addition?

https://codereview.chromium.org/1604993005/diff/20001/src/gpu/gl/egl/SkCreate...
File src/gpu/gl/egl/SkCreatePlatformGLContext_egl.cpp (right):

https://codereview.chromium.org/1604993005/diff/20001/src/gpu/gl/egl/SkCreate...
src/gpu/gl/egl/SkCreatePlatformGLContext_egl.cpp:321: SkASSERT(!shareContext);
Why these asserts rather than just return nullptr?

[joshualitt, 2016-01-20 15:45:20]

https://codereview.chromium.org/1604993005/diff/20001/src/gpu/gl/egl/SkCreate...
File src/gpu/gl/egl/SkCreatePlatformGLContext_egl.cpp (right):

https://codereview.chromium.org/1604993005/diff/20001/src/gpu/gl/egl/SkCreate...
src/gpu/gl/egl/SkCreatePlatformGLContext_egl.cpp:321: SkASSERT(!shareContext);
On 2016/01/20 15:26:07, bsalomon wrote:
> Why these asserts rather than just return nullptr?

I can see it both ways.  It make it easier to spot the failure because the
program will halt at the offending line, instead of just getting a null context
and not knowing why.  Also, I didn't want to clutter up all of these functions
with an additional check.

Of course, this means it won't work in release builds, which is unfortunate.

If you have a preference for returning null, then I can go that route.

[bsalomon, 2016-01-20 15:51:24]

On 2016/01/20 15:45:20, joshualitt wrote:
>
https://codereview.chromium.org/1604993005/diff/20001/src/gpu/gl/egl/SkCreate...
> File src/gpu/gl/egl/SkCreatePlatformGLContext_egl.cpp (right):
> 
>
https://codereview.chromium.org/1604993005/diff/20001/src/gpu/gl/egl/SkCreate...
> src/gpu/gl/egl/SkCreatePlatformGLContext_egl.cpp:321: SkASSERT(!shareContext);
> On 2016/01/20 15:26:07, bsalomon wrote:
> > Why these asserts rather than just return nullptr?
> 
> I can see it both ways.  It make it easier to spot the failure because the
> program will halt at the offending line, instead of just getting a null
context
> and not knowing why.  Also, I didn't want to clutter up all of these functions
> with an additional check.
> 
> Of course, this means it won't work in release builds, which is unfortunate.
> 
> If you have a preference for returning null, then I can go that route.

Release builds would just silently ignore the param.

[joshualitt, 2016-01-20 15:55:18]

On 2016/01/20 15:51:24, bsalomon wrote:
> On 2016/01/20 15:45:20, joshualitt wrote:
> >
>
https://codereview.chromium.org/1604993005/diff/20001/src/gpu/gl/egl/SkCreate...
> > File src/gpu/gl/egl/SkCreatePlatformGLContext_egl.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1604993005/diff/20001/src/gpu/gl/egl/SkCreate...
> > src/gpu/gl/egl/SkCreatePlatformGLContext_egl.cpp:321:
SkASSERT(!shareContext);
> > On 2016/01/20 15:26:07, bsalomon wrote:
> > > Why these asserts rather than just return nullptr?
> > 
> > I can see it both ways.  It make it easier to spot the failure because the
> > program will halt at the offending line, instead of just getting a null
> context
> > and not knowing why.  Also, I didn't want to clutter up all of these
functions
> > with an additional check.
> > 
> > Of course, this means it won't work in release builds, which is unfortunate.
> > 
> > If you have a preference for returning null, then I can go that route.
> 
> Release builds would just silently ignore the param.

Okay, I'd prefer SkAlwaysAssert, but given we don't have it I'll just return
null.

[joshualitt, 2016-01-20 15:55:22]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2016-01-20 15:55:22]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1604993005/#ps40001
(title: "feedback inc")

[commit-bot: I haz the power, 2016-01-20 15:55:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1604993005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1604993005/40001

[commit-bot: I haz the power, 2016-01-20 16:07:04]

Description was changed from

==========
Add ability to wire up sharelist in glcontext creation

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Add ability to wire up sharelist in glcontext creation

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/b59d1bc7a8596f346d57a9cfcd461dddc6d75edb
==========

[commit-bot: I haz the power, 2016-01-20 16:07:04]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/b59d1bc7a8596f346d57a9cfcd461dddc6d75edb