Bug 80988 – [Chromium] GL_EXT_occlusion_query_boolean and GL_CHROMIUM_command_buffer_query support.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Reveman]

Expose EXT_occlusion_query extensions to WebKit compositor.

[David Reveman]

Created attachment 131623 [details]
Patch

[David Reveman]

Created attachment 131625 [details]
Patch

[David Reveman]

Created attachment 131630 [details]
Patch

Fix typo

[Nat Duca]

Enne, are you comfortable reviewing WGC3D extensions? @kbr is out. Happy to bounce to James but I know his queue is deep.

[Nat Duca]

Comment on attachment 131630 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/Extensions3DChromium.h:49
> +    //   GL_CHROMIUM_fence

Is this what you want to call it? GL_CHROMIM_commands_issued_occlusion_query?

> Source/WebKit/chromium/tests/FakeWebGraphicsContext3D.h:258
> +    virtual WebGLId createQueryEXT() { return 1; }

Is 1 a safe return value?

[Adrienne Walker]

Comment on attachment 131630 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/Extensions3DChromium.h:90
> +        // GL_CHROMIUM_fence
> +        COMMANDS_ISSUED_CHROMIUM = 0x84F2

Where does this get used? Are there not fence setting or checking functions?

>> Source/WebKit/chromium/tests/FakeWebGraphicsContext3D.h:258
>> +    virtual WebGLId createQueryEXT() { return 1; }
> 
> Is 1 a safe return value?

For this fake WGC3D, yes.  It's what we do above for other object-creating functions.  If some test needs to create real IDs, it can override and do that itself.

[David Reveman]

(In reply to comment #5)
> (From update of attachment 131630 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131630&action=review
> 
> > Source/WebCore/platform/graphics/chromium/Extensions3DChromium.h:49
> > +    //   GL_CHROMIUM_fence
> 
> Is this what you want to call it? GL_CHROMIM_commands_issued_occlusion_query?

Maybe GL_CHROMIM_occlusion_query_commands_issued in that case but in this patch I'm just being consistent with the extension name in Gregg's CL  http://codereview.chromium.org/9694025/ so lets move the name discussion over there instead.

> 
> > Source/WebKit/chromium/tests/FakeWebGraphicsContext3D.h:258
> > +    virtual WebGLId createQueryEXT() { return 1; }
> 
> Is 1 a safe return value?

not used at the moment and returning 1 is consistent with the other fake createX() functions.

[David Reveman]

(In reply to comment #6)
> (From update of attachment 131630 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131630&action=review
> 
> > Source/WebCore/platform/graphics/chromium/Extensions3DChromium.h:90
> > +        // GL_CHROMIUM_fence
> > +        COMMANDS_ISSUED_CHROMIUM = 0x84F2
> 
> Where does this get used? Are there not fence setting or checking functions?

The name doesn't currently reflect this but it's an extension on top of the GL_EXT_occlusion_query extension. You can find a usage example in this patch: https://bugs.webkit.org/show_bug.cgi?id=81004 and maybe also as part of the unit test for this CL: http://codereview.chromium.org/9694025/

[David Reveman]

Created attachment 131763 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail] or [e-mail] before submitting because this patch contains changes to the Chromium platform API.

[WebKit Review Bot]

Attachment 131763 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Reveman]

Created attachment 132504 [details]
Patch

[David Reveman]

Please review the patch. The following chromium change need to land before we can put it in the commit queue.

https://chromiumcodereview.appspot.com/9719030/

[Adrienne Walker]

Comment on attachment 132504 [details]
Patch

Sure! Looks good to me.

[David Reveman]

Comment on attachment 132504 [details]
Patch

All required chromium side changes have landed.

[Adrienne Walker]

jamesr: Can you approve the WebGraphicsContext3D API changes?

[James Robinson]

Comment on attachment 132504 [details]
Patch

Looks OK

[WebKit Review Bot]

Comment on attachment 132504 [details]
Patch

Clearing flags on attachment: 132504

Committed r111207: <http://trac.webkit.org/changeset/111207>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

Reopening to attach new patch.

[James Robinson]

Created attachment 132625 [details]
Patch

[James Robinson]

Whoops, wrong bug

[Adrienne Walker]

Rolled out in bug 81561.

[David Reveman]

Created attachment 132862 [details]
Patch

Add base implementations of EXT_occlusion_query functions to be consistent with other extensions.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

API changes ok. Didn't look at impl.

[Adrienne Walker]

How were missing implementations causing test errors? Or, am I misunderstanding what the problem was?

[David Reveman]

Spent a significant amount of time looking at what could possible cause the runtime problem that was the reason for reverting this patch. The only difference I've found compared to other extensions is the lack of base implementations for functions in WebGraphicsContext3D.h. I changed this in the latest patch to be consistent with other extension functions, although I'm not sure how this change can affect a runtime error.

[Adrienne Walker]

(In reply to comment #28)
> Spent a significant amount of time looking at what could possible cause the runtime problem that was the reason for reverting this patch. The only difference I've found compared to other extensions is the lack of base implementations for functions in WebGraphicsContext3D.h. I changed this in the latest patch to be consistent with other extension functions, although I'm not sure how this change can affect a runtime error.

Yeah, it certainly doesn't sound like that would result in runtime errors.

If you haven't already, could you run a try job with this patch to run whatever tests were failing just as a sanity check? If that passes, I'm happy to re-R+/CQ+ this.

[Adrienne Walker]

Comment on attachment 132862 [details]
Patch

http://build.chromium.org/p/tryserver.chromium/builders/mac_gpu/builds/92 looks clean.  Let's try this again.

[WebKit Review Bot]

Comment on attachment 132862 [details]
Patch

Clearing flags on attachment: 132862

Committed r111597: <http://trac.webkit.org/changeset/111597>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.