Closed Bug 811958 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Widget: Cocoa, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(3 files, 2 obsolete files)

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.
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
Assignee: nobody → jgilbert
Status: NEW → ASSIGNED
Attachment #681769 - Flags: review?(bgirard)
There are a lot of people in hg blame, so I'm not sure who to ask for review here.
Attachment #681775 - Flags: review?(bgirard)
Here's the reftest Try run with the other platforms:
https://tbpl.mozilla.org/?tree=Try&rev=8eb23b01056e

It looks eerily green.
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.
Attachment #681769 - Flags: review?(bgirard) → review+
(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.
Blocks: 812328
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.
Attachment #681775 - Flags: review?(bgirard) → review-
Pulled this bit out of the next patch.
Moves the SharedTexture ShareType type into GLContext, and improved the enum names.
Attachment #681775 - Attachment is obsolete: true
Attachment #683987 - Flags: review?(bgirard)
Attachment #683988 - Flags: review?(bgirard)
Oops, messed up the fix up, here. Better now. It builds and everything!
Attachment #683988 - Attachment is obsolete: true
Attachment #683988 - Flags: review?(bgirard)
Attachment #684086 - Flags: review?(bgirard)
Attachment #683987 - Flags: review?(bgirard) → review+
Attachment #684086 - Flags: review?(bgirard) → review+
You need to log in before you can comment on or make changes to this bug.