Re-enable various MSVC warnings.

Re-enable various MSVC warnings.

BUG=81439
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285730

[Peter Kasting, 2014-07-24 19:07:45]

Please give some thought to the allocator change in patch set 2.  MSVC complains
about the old code that we're trying to use exception handling when exceptions
are disabled.  It seems to me that all the code there is simply checking "yes,
we compiled with exceptions disabled".  That doesn't seem like a terribly useful
test to have, so rather than #pragma around the warning, I'm just removing the
test.  But maybe there's some reason it's important not to do that?

[Peter Kasting, 2014-07-24 21:22:33]

armansito: OWNERS for device/bluetooth/
piman: OWNERS for gpu/

[armansito, 2014-07-24 21:41:59]

Added rpaquay@ who owns the windows implementation. Renaud, should
|rfcomm_channel_| be a signed int? Or is the 0xFF default value OK?

[piman, 2014-07-24 21:49:06]

lgtm

https://codereview.chromium.org/415773009/diff/40001/gpu/gles2_conform_suppor...
File gpu/gles2_conform_support/egl/config.cc (right):

https://codereview.chromium.org/415773009/diff/40001/gpu/gles2_conform_suppor...
gpu/gles2_conform_support/egl/config.cc:18: bind_to_texture_rgba_(EGL_TRUE),
Make both of these EGL_FALSE. We don't support pbuffer configs, it wouldn't make
sense for these to be EGL_TRUE for window configs.

[Peter Kasting, 2014-07-24 21:52:10]

Not uploading a new patch set yet, so I can wait for the latest Win try results
to see if there are other breakages.

https://codereview.chromium.org/415773009/diff/40001/gpu/gles2_conform_suppor...
File gpu/gles2_conform_support/egl/config.cc (right):

https://codereview.chromium.org/415773009/diff/40001/gpu/gles2_conform_suppor...
gpu/gles2_conform_support/egl/config.cc:18: bind_to_texture_rgba_(EGL_TRUE),
On 2014/07/24 21:49:06, piman (slow to review) wrote:
> Make both of these EGL_FALSE. We don't support pbuffer configs, it wouldn't
make
> sense for these to be EGL_TRUE for window configs.

Done.  I only picked EGL_TRUE because EGL_DONT_CARE is nonzero, and I didn't
want to change behavior.  (That may suggest that these values aren't very
well-tested today?)

[piman, 2014-07-24 22:05:57]

On 2014/07/24 21:52:10, Peter Kasting wrote:
> Not uploading a new patch set yet, so I can wait for the latest Win try
results
> to see if there are other breakages.
> 
>
https://codereview.chromium.org/415773009/diff/40001/gpu/gles2_conform_suppor...
> File gpu/gles2_conform_support/egl/config.cc (right):
> 
>
https://codereview.chromium.org/415773009/diff/40001/gpu/gles2_conform_suppor...
> gpu/gles2_conform_support/egl/config.cc:18: bind_to_texture_rgba_(EGL_TRUE),
> On 2014/07/24 21:49:06, piman (slow to review) wrote:
> > Make both of these EGL_FALSE. We don't support pbuffer configs, it wouldn't
> make
> > sense for these to be EGL_TRUE for window configs.
> 
> Done.  I only picked EGL_TRUE because EGL_DONT_CARE is nonzero, and I didn't
> want to change behavior.  (That may suggest that these values aren't very
> well-tested today?)

This is not used in production. It's only used for hooking up the Khronos GLES2
conformance tests. It's very possible that these conformance tests don't test
this particular part of the spec. The spec is very clear though (see
http://www.khronos.org/registry/egl/specs/eglspec.1.4.20110406.pdf section 3.4):

EGL_BIND_TO_TEXTURE_RGB and EGL_BIND_TO_TEXTURE_RGBA are booleans indicating
whether the color buffers of a pbuffer created with the EGLConfig can be bound
to a OpenGL ES RGB or RGBA texture respectively. Currently only pbuffers can
be bound as textures, so these attributes may only be EGL_TRUE if the value of
the
EGL_SURFACE_TYPE attribute includes EGL_PBUFFER_BIT


As you can see, we set surface_type_ to EGL_WINDOW_BIT, and that's what we
return when we query EGL_SURFACE_TYPE.

[rpaquay, 2014-07-24 22:45:10]

On 2014/07/24 21:41:59, armansito wrote:
> Added rpaquay@ who owns the windows implementation. Renaud, should
> |rfcomm_channel_| be a signed int? Or is the 0xFF default value OK?

device/bluetooth/bluetooth_service_record_win.cc lgtm
Unsigned int is ok here, as valid values are 0 to 30 only according to MSDN doc.
Maybe it would be better to extract a constant "const uint8
kInvalidRfcommChannel = 0xff;" but the change is lgtm either way.

[Peter Kasting, 2014-07-24 22:48:32]

On 2014/07/24 22:45:10, rpaquay wrote:
> Maybe it would be better to extract a constant "const uint8
> kInvalidRfcommChannel = 0xff;" but the change is lgtm either way.

I think that would probably be a good idea, especially if this can be
generalized and used elsewhere (e.g. "kInvalidChannel" or something) -- I feel
like I've made similar fixes other places in the bluetooth code.

However, I'm not going to make the change myself in this CL, since I don't
necessarily know all the places to ideally look for this and similar patterns;
I'll leave that up to you guys.

[rpaquay, 2014-07-24 23:09:20]

On 2014/07/24 22:48:32, Peter Kasting wrote:
> On 2014/07/24 22:45:10, rpaquay wrote:
> > Maybe it would be better to extract a constant "const uint8
> > kInvalidRfcommChannel = 0xff;" but the change is lgtm either way.
> 
> I think that would probably be a good idea, especially if this can be
> generalized and used elsewhere (e.g. "kInvalidChannel" or something) -- I feel
> like I've made similar fixes other places in the bluetooth code.
> 
> However, I'm not going to make the change myself in this CL, since I don't
> necessarily know all the places to ideally look for this and similar patterns;
> I'll leave that up to you guys.

sgtm

[Peter Kasting, 2014-07-24 23:43:23]

+bbudge for components/nacl/renderer/ OWNERS

[bbudge, 2014-07-25 00:40:20]

components/nacl/renderer lgtm

[brettw, 2014-07-25 20:19:21]

https://codereview.chromium.org/415773009/diff/20001/base/allocator/allocator...
File base/allocator/allocator_unittest.cc (left):

https://codereview.chromium.org/415773009/diff/20001/base/allocator/allocator...
base/allocator/allocator_unittest.cc:312: void* rv = (*func)(kTooBig);
I kind of like this test for the behavior that a "too big" allocation returns
null rather than crashes or whatever. I don't see another test that covers this.

It's also nice to check that people didn't accidentally enable exceptions. Is
there a way to check for this case, like a define or something?

[Peter Kasting, 2014-07-25 20:54:42]

https://codereview.chromium.org/415773009/diff/20001/base/allocator/allocator...
File base/allocator/allocator_unittest.cc (left):

https://codereview.chromium.org/415773009/diff/20001/base/allocator/allocator...
base/allocator/allocator_unittest.cc:312: void* rv = (*func)(kTooBig);
On 2014/07/25 20:19:20, brettw wrote:
> It's also nice to check that people didn't accidentally enable exceptions. Is
> there a way to check for this case, like a define or something?

I think there are two options.  One would be to #pragma disable the relevant
warning here.  The other would be to do something like:

#if defined(_HAS_EXCEPTIONS) && _HAS_EXCEPTIONS
#error
#endif

The second way is briefer, but only works under MSVC.

I'm a little reluctant to do either one, because (a) it seems extremely unlikely
that we'll bring up Chrome on another platform and enable exceptions, since our
code is littered with exception-unsafe idioms, and (b) if someone did this
intentionally for some reason -- especially for projects that may depend on
base/ without being full-blown Chromium -- they really wouldn't _want_ this test
to prevent them from doing that.

However, maybe these don't outweigh the benefit of the test.  Changed.

[brettw, 2014-07-25 20:57:12]

lgtm

[Peter Kasting, 2014-07-25 21:01:53]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-07-25 21:05:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkasting@chromium.org/415773009/100001

[commit-bot: I haz the power, 2014-07-26 00:01:58]

Change committed as 285730