Fix GL extension filtering so that extension bits are set correctly.

Fix GL extension filtering so that extension bits are set correctly.

Also add unittests for GL extension bits.

BUG=502353, 504758
Committed: https://crrev.com/5e13825bdb728727f4fc78a45b3a2ddd4ebc984c
Cr-Commit-Position: refs/heads/master@{#340330}

[Tobias Sargeant, 2015-07-22 14:44:21]

tobiasjs@chromium.org changed reviewers:
+ kbr@chromium.org, sievers@chromium.org

[Tobias Sargeant, 2015-07-22 14:44:21]

As sievers@ pointed out, the previous work didn't actually fix GL extension
bits. This corrects that oversight, and adds the unittests that would have
caught it the first time around.

[Tobias Sargeant, 2015-07-22 17:08:01]

Not really ready for review yet, it appears. I think the change itself is
correct, but it breaks mock-based unittests pretty badly. Not sure what to do
with this yet.

[Tobias Sargeant, 2015-07-23 11:49:10]

On 2015/07/22 17:08:01, Tobias Sargeant wrote:

Ok. This looks better. Apparently we don't get to change the preceding behaviour
that only caches a filtered list of extensions if there are disabled extensions,
because that's basically baked into the mocking expectations of a huge number of
tests.

Anyhow. PTAL.

[no sievers, 2015-07-23 23:45:57]

The test setup is becoming a bit of hackery. We should probably clean this up as
a follow-up, can you file a bug?
Maybe we can somehow make this work more like the gpu unittests with mocked GL
and some common setup codepath for all tests.

https://codereview.chromium.org/1253433002/diff/80001/ui/gl/gl_api_unittest.cc
File ui/gl/gl_api_unittest.cc (right):

https://codereview.chromium.org/1253433002/diff/80001/ui/gl/gl_api_unittest.c...
ui/gl/gl_api_unittest.cc:78: api_->InitializeFilteredExtensions();
Doesn't InitializeWithContext() call this?

https://codereview.chromium.org/1253433002/diff/80001/ui/gl/gl_gl_api_impleme...
File ui/gl/gl_gl_api_implementation.cc (right):

https://codereview.chromium.org/1253433002/diff/80001/ui/gl/gl_gl_api_impleme...
ui/gl/gl_gl_api_implementation.cc:434: void RealGLApi::InitializeWithContext() {
So this method can go?

https://codereview.chromium.org/1253433002/diff/80001/ui/gl/gl_gl_api_impleme...
ui/gl/gl_gl_api_implementation.cc:439: #if DCHECK_IS_ON()
nit: #if DCHECK_IS_ON is redundant here and below

https://codereview.chromium.org/1253433002/diff/80001/ui/gl/gl_gl_api_impleme...
ui/gl/gl_gl_api_implementation.cc:485:
FilterGLExtensionList(reinterpret_cast<const char*>(
Hmm a bit back and forth going on in here (vector to string, then back to vector
below).

[Ken Russell (switch to Gerrit), 2015-07-23 23:57:44]

I'd like to defer to sievers@' review.

[Tobias Sargeant, 2015-07-24 09:29:15]

Re: test cleanup, I think this bug is already filed for that:

https://code.google.com/p/chromium/issues/detail?id=498455

https://codereview.chromium.org/1253433002/diff/80001/ui/gl/gl_api_unittest.cc
File ui/gl/gl_api_unittest.cc (right):

https://codereview.chromium.org/1253433002/diff/80001/ui/gl/gl_api_unittest.c...
ui/gl/gl_api_unittest.cc:78: api_->InitializeFilteredExtensions();
On 2015/07/23 23:45:57, sievers wrote:
> Doesn't InitializeWithContext() call this?

Not any more. It's safe to remove InitializeWithContext() completely.

https://codereview.chromium.org/1253433002/diff/80001/ui/gl/gl_gl_api_impleme...
File ui/gl/gl_gl_api_implementation.cc (right):

https://codereview.chromium.org/1253433002/diff/80001/ui/gl/gl_gl_api_impleme...
ui/gl/gl_gl_api_implementation.cc:434: void RealGLApi::InitializeWithContext() {
On 2015/07/23 23:45:57, sievers wrote:
> So this method can go?

Yes.

https://codereview.chromium.org/1253433002/diff/80001/ui/gl/gl_gl_api_impleme...
ui/gl/gl_gl_api_implementation.cc:439: #if DCHECK_IS_ON()
On 2015/07/23 23:45:57, sievers wrote:
> nit: #if DCHECK_IS_ON is redundant here and below

I thought so too, but it turns out that because it DCHECKS a bool that's not
compiled in when DCHECK_IS_ON() == 0, it fails to compile on windows. See:

http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...

https://codereview.chromium.org/1253433002/diff/80001/ui/gl/gl_gl_api_impleme...
ui/gl/gl_gl_api_implementation.cc:485:
FilterGLExtensionList(reinterpret_cast<const char*>(
On 2015/07/23 23:45:57, sievers wrote:
> Hmm a bit back and forth going on in here (vector to string, then back to
vector
> below).

I agree, but I felt that the smaller code offset the inefficiency - especially
given that this is meant to happen only at startup.

[no sievers, 2015-07-24 19:21:56]

lgtm, thanks for fixing this!

[Tobias Sargeant, 2015-07-24 19:25:03]

The CQ bit was checked by tobiasjs@chromium.org

[commit-bot: I haz the power, 2015-07-24 19:26:35]

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

[commit-bot: I haz the power, 2015-07-24 20:54:09]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-07-24 20:55:00]

Patchset 6 (id:??) landed as
https://crrev.com/5e13825bdb728727f4fc78a45b3a2ddd4ebc984c
Cr-Commit-Position: refs/heads/master@{#340330}