Bug 67395

Summary:

[Chromium, Skia] Add virtual to WebGraphicsContext3D to create a new GrGLInterface per context. Insert per-skia-GL-function callback to set correct GL context.

Product:

WebKit

Reporter:

Brian Salomon <bsalomon>

Component:

Platform

Assignee:

Brian Salomon <bsalomon>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

fishd, kbr, senorblanco, vangelis, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none

Description Brian Salomon 2011-09-01 06:39:12 PDT

[Chromium, Skia] Add virtual to WebGraphicsContext3D to create a new GrGLInterface per context. Insert per-skia-GL-function callback to set correct GL context.

Comment 1 Brian Salomon 2011-09-01 06:41:09 PDT

Created attachment 105952 [details]
Patch

Comment 2 Brian Salomon 2011-09-01 06:50:04 PDT

The idea behind this change is to create a new GrGLInterface per-WG3D and to use a new callback facility in Ganesh to make sure the correct context is set before each GL call. The motivation is that we keep seeing wrong context bugs.

The sister change to this in Chrome is here:
http://codereview.chromium.org/7756004/

After both changes have landed I can remove PlatformContextSkia::makeGrContextCurrent() and all callsites as well as the WGC3D::grGLInterface virtual.

Comment 3 Kenneth Russell 2011-09-01 12:14:23 PDT

Comment on attachment 105952 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105952&action=review

Sorry, but I have to r- this because the license needs to be updated in the new file. Also CC'ing fishd as he should review all WebKit API changes. It looks OK overall to me though.

> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:166
> +        // remove this block after WGC3D subclasses in Chromium overriding using grGLInterface()

Add FIXME and please make this comment and the one below a complete sentence.

> Source/WebKit/chromium/src/WebGraphicsContext3D.cpp:15
> + *     from this software without specific prior written permission.

Sorry, this is the wrong version of the license file. I know there are a ton of files that have this license but we should avoid adding more that are wrong. Please derive the license text from Source/WebKit/LICENSE, or see e.g. Source/WebKit/chromium/tests/PODIntervalTreeTest.cpp.

Comment 4 Brian Salomon 2011-09-01 13:19:58 PDT

Created attachment 106016 [details]
Patch

Comment 5 Brian Salomon 2011-09-01 13:21:54 PDT

(In reply to comment #3)
> (From update of attachment 105952 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105952&action=review
> 
> Sorry, but I have to r- this because the license needs to be updated in the new file. Also CC'ing fishd as he should review all WebKit API changes. It looks OK overall to me though.
> 
> > Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:166
> > +        // remove this block after WGC3D subclasses in Chromium overriding using grGLInterface()
> 
> Add FIXME and please make this comment and the one below a complete sentence.
> 
> > Source/WebKit/chromium/src/WebGraphicsContext3D.cpp:15
> > + *     from this software without specific prior written permission.
> 
> Sorry, this is the wrong version of the license file. I know there are a ton of files that have this license but we should avoid adding more that are wrong. Please derive the license text from Source/WebKit/LICENSE, or see e.g. Source/WebKit/chromium/tests/PODIntervalTreeTest.cpp.

The latest patch addresses the license and comments.

Comment 6 Kenneth Russell 2011-09-01 14:54:51 PDT

Comment on attachment 106016 [details]
Patch

Thanks. r=me

Comment 7 WebKit Review Bot 2011-09-01 16:24:09 PDT

Comment on attachment 106016 [details]
Patch

Clearing flags on attachment: 106016

Committed r94359: <http://trac.webkit.org/changeset/94359>

Comment 8 WebKit Review Bot 2011-09-01 16:24:14 PDT

All reviewed patches have been landed.  Closing bug.