811958 - Objective-C exception handling creates parsing errors when including STL container headers

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Kelsey Gilbert [:jgilbert]]

We include GLContext.h in nsChildView.mm, in which we also use objective-c exceptions. However, when we want to add an STL container to GLContext, we get parsing errors during compile, where the compiler refuses to recognize '@try' syntax.

Removing the requirement for GLContext.h from nsChildView.mm fixes this.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Adding a std::set<int> to GLContext:
https://tbpl.mozilla.org/?tree=Try&rev=cc8c3ef2557f

After disentangling GLContext.h from nsChildView.mm:
https://tbpl.mozilla.org/?tree=Try&rev=2c249948b830

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 1: Pull GLContext out of cocoa stuff

Comment 3

[Kelsey Gilbert [:jgilbert]]

Attachment: Patch 2: Remove TextureImage classes from GLContext.h/cpp

There are a lot of people in hg blame, so I'm not sure who to ask for review here.

Comment 4

[Kelsey Gilbert [:jgilbert]]

Here's the reftest Try run with the other platforms:
https://tbpl.mozilla.org/?tree=Try&rev=8eb23b01056e

It looks eerily green.

Comment 5

[Benoit Girard (:BenWa)]

Comment on attachment 681769 [details] [diff] [review]
patch 1: Pull GLContext out of cocoa stuff

Review of attachment 681769 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine with the following fixed and assuming it's all code moves.

::: gfx/gl/GLContextTypes.h
@@ +1,5 @@
> +/*
> + * GLContextStuff.h
> + *
> + *  Created on: Nov 13, 2012
> + *      Author: jgilbert

Use mozilla boilerplate.

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +5,5 @@
>  
> +#include "LayerManagerOGL.h"
> +
> +#ifdef GLCONTEXT_H_
> +#error GLContext.h should not have been included here.

Reminder: you said you would remove this.

Comment 6

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Girard (:BenWa) from comment #5)
> Comment on attachment 681769 [details] [diff] [review]
> patch 1: Pull GLContext out of cocoa stuff
> 
> Review of attachment 681769 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is fine with the following fixed and assuming it's all code moves.
> 
> ::: gfx/gl/GLContextTypes.h
> @@ +1,5 @@
> > +/*
> > + * GLContextStuff.h
> > + *
> > + *  Created on: Nov 13, 2012
> > + *      Author: jgilbert
> 
> Use mozilla boilerplate.
Yep, sorry, IDE boilerplate.
> 
> ::: gfx/layers/opengl/LayerManagerOGL.cpp
> @@ +5,5 @@
> >  
> > +#include "LayerManagerOGL.h"
> > +
> > +#ifdef GLCONTEXT_H_
> > +#error GLContext.h should not have been included here.
> 
> Reminder: you said you would remove this.
Indeed.

Comment 7

[Benoit Girard (:BenWa)]

Comment on attachment 681775 [details] [diff] [review]
Patch 2: Remove TextureImage classes from GLContext.h/cpp

Review of attachment 681775 [details] [diff] [review]:
-----------------------------------------------------------------

Not a fan of the changes that are ridding along with this code move. At the very least they need their own patch.

::: dom/plugins/base/nsNPAPIPluginInstance.cpp
@@ -128,5 @@
>  
>      if (mTextureInfo.mWidth == 0 || mTextureInfo.mHeight == 0)
>        return 0;
>  
> -    SharedTextureHandle handle = sPluginContext->CreateSharedHandle(TextureImage::ThreadShared, (void*)mTextureInfo.mTexture, GLContext::TextureID);

enums are better than bool. If you want to make this change it should go in a separate patch.

::: gfx/gl/GLTextureImage.cpp
@@ +561,5 @@
> +{
> +    return mImages.Length();
> +}
> +
> +TextureImage::ScopedBindTexture::ScopedBindTexture(TextureImage *aTexture, int aTextureUnit)

You're modifying the code in a large move. This also needs to be a separate patch and I think the previous version was better.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 2: Fix and move ShareType type

Pulled this bit out of the next patch.
Moves the SharedTexture ShareType type into GLContext, and improved the enum names.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3: Move TextureImage into its own files.

Comment 10

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3: Move TextureImage into its own files.

Oops, messed up the fix up, here. Better now. It builds and everything!

Comment 11

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec29e09a270e
https://hg.mozilla.org/integration/mozilla-inbound/rev/1637c39b4ed6
https://hg.mozilla.org/integration/mozilla-inbound/rev/807f2c3df974

Comment 12

[Kelsey Gilbert [:jgilbert]]

Backed out: 
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ca16b44a6db

Comment 13

[Kelsey Gilbert [:jgilbert]]

Again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb15602e78b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/916e594faea1
https://hg.mozilla.org/integration/mozilla-inbound/rev/9aea73623c64

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bb15602e78b9
https://hg.mozilla.org/mozilla-central/rev/916e594faea1
https://hg.mozilla.org/mozilla-central/rev/9aea73623c64