Benoit Girard (:BenWa) Reporter
	Description • 11 years ago

With the layers refactoring we're almost ready to remove the old opengl layers code. We still have OS X 10.6 using it and it's useful to have until we certain we're not going to back out OMTC on mac.

We should also completely remove the Copy2D stuff:
https://bugzilla.mozilla.org/show_bug.cgi?id=923409#c2

	Benoit Girard (:BenWa) Reporter
	Comment 1 • 11 years ago

Nical can have this honor :)

	Nick Cameron [:nrc]
	Comment 3 • 11 years ago

nical: do you have patches ready to go for this? I hope it will be unblocked very soon and it is blocking my ThebesLayerBuffer patches (which block Azure content), so it would be great if we could land this as soon as it is unblocked. Thanks!

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 4 • 11 years ago

We haven't identified this as a priority right now - do we need nical to do this?  In my mind a more important thing is to complete the removal of the deprecated textures that nical is on the hook to do, rather than this.  Nick, if you're blocked on this, can you take this on instead?

	Nick Cameron [:nrc]
	Comment 5 • 11 years ago

(In reply to Milan Sreckovic [:milan] from comment #4)
> We haven't identified this as a priority right now - do we need nical to do
> this?  In my mind a more important thing is to complete the removal of the
> deprecated textures that nical is on the hook to do, rather than this. 
> Nick, if you're blocked on this, can you take this on instead?

It is only a priority because it transitively blocks my new work on new textures/removing deprecated textures.

Anyway, I am happy to do this, but I don't want to steal it from nical unless he is keen - it should be fun to do and very quick.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 6 • 11 years ago

You're going to remove the deprecated textures?  And that needs this?  That's a different story then.

	Nick Cameron [:nrc]
	Comment 7 • 11 years ago

This blocks removing Thebes paths from TLB and that soft-blocks Azure content - because adding _another_ path to TLB before we remove some is madness.

	Nicolas Silva [:nical] Assignee
	Comment 8 • 11 years ago

There you go :)
try push: https://tbpl.mozilla.org/?tree=Try&rev=c0374ad1b0e9

I am not sure whether we can land this (there was something about TART performance tests still using LayerManagerOGL).

	Nicolas Silva [:nical] Assignee
	Comment 9 • 11 years ago

Some build fixes

	Nicolas Silva [:nical] Assignee
	Comment 10 • 11 years ago

try push https://tbpl.mozilla.org/?tree=Try&rev=44f34d0623c1

	Andreas Gal :gal
	Comment 11 • 11 years ago

With this does it make sense to eliminate the GLManager abstraction?

	Nick Cameron [:nrc]
	Comment 12 • 11 years ago

(In reply to Andreas Gal :gal from comment #11)
> With this does it make sense to eliminate the GLManager abstraction?

Yes, that was a stop gap until we got to this stage. I will remove it.

	Nick Cameron [:nrc]
	Comment 13 • 11 years ago

Comment on attachment 828029 [details] [diff] [review]
remove non-OMTC GL layers

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

r=me and \o/

As agreed with Nical and Milan, I'll land this and handle the review comments (in a separate patch), also manage the Talos dependency and deal with any fallout.

::: gfx/layers/opengl/GLManager.h
@@ -20,5 @@
>  
>  /**
>   * Minimal interface to allow widgets to draw using OpenGL. Abstracts
> - * LayerManagerOGL and CompositorOGL. Call CreateGLManager with either a
> - * LayerManagerOGL or a LayerManagerComposite backed by a CompositorOGL.

We should be able to get rid of GLManager completely

::: gfx/layers/opengl/LayerManagerOGLProgram.h
@@ -260,5 @@
> -   * a 2D transform.
> -   */
> -  bool LoadMask(Layer* aLayer);
> -
> -  /**

We should rename these files

::: widget/xpwidgets/nsBaseWidget.cpp
@@ +1404,5 @@
>  
>  uint32_t
>  nsBaseWidget::GetGLFrameBufferFormat()
>  {
> +  // XXX - We may want to do something here or just remove that code.

We should use return RGBA here, I think

	Markus Stange [:mstange]
	Comment 14 • 11 years ago

There's another GLManager subclass at http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsChildView.mm#328 (which I added in bug 882523) - looks like I forgot to add it to the comment in GLManager.h. So removing GLManager will require some refactoring in nsChildView.mm.

	Nick Cameron [:nrc]
	Comment 15 • 11 years ago

(In reply to Markus Stange [:mstange] from comment #14)
> There's another GLManager subclass at
> http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsChildView.
> mm#328 (which I added in bug 882523) - looks like I forgot to add it to the
> comment in GLManager.h. So removing GLManager will require some refactoring
> in nsChildView.mm.

ah, ok, thanks. I'll take a look and see if it is still worth removing or not.

	Nick Cameron [:nrc]
	Comment 17 • 11 years ago

I don't think we can remove GLManager, but I think it can be simplified a lot. Seeing as that doesn't block anything, I'm not going to do that now. I filed bug 935734 for that tidy up work.

	Andreas Gal :gal
	Comment 18 • 11 years ago

Don't waste time cleaning up LayerManagerOGLProgram, I have several patches to do that (once my current set stuck). nsChildView.mm definitely needs some refactoring. It duplicates a bunch of layers code (VBO handling, draw etc).

	Nick Cameron [:nrc]
	Comment 19 • 11 years ago

(In reply to Andreas Gal :gal from comment #18)
> Don't waste time cleaning up LayerManagerOGLProgram, I have several patches

I don't intend to :-) (other than the patch here which just does renaming). Do you mean that you would prefer me not to land that patch?

> to do that (once my current set stuck). nsChildView.mm definitely needs some
> refactoring. It duplicates a bunch of layers code (VBO handling, draw etc).

	Nick Cameron [:nrc]
	Comment 20 • 11 years ago

Another try push to check we still build with patch 2: https://tbpl.mozilla.org/?tree=Try&rev=75afdf6272a0

	Nick Cameron [:nrc]
	Comment 21 • 11 years ago

(In reply to Nick Cameron [:nrc] from comment #19)
> (In reply to Andreas Gal :gal from comment #18)
> > Don't waste time cleaning up LayerManagerOGLProgram, I have several patches
> 
> I don't intend to :-) (other than the patch here which just does renaming).
> Do you mean that you would prefer me not to land that patch?

	Andreas Gal :gal
	Comment 22 • 11 years ago

No. Land away. I can rebase on top. Thanks for the patch here :)

	Andreas Gal :gal
	Comment 24 • 11 years ago

If this sticks I am buying beer for the team.

	Nick Cameron [:nrc]
	Comment 25 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/caa48441db53
https://hg.mozilla.org/integration/mozilla-inbound/rev/d04193bc1627
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa0066b3865c

	Hannes Verschore [:h4writer]
	Comment 26 • 11 years ago

Can I assume the Tab Animation Regression on MaxOSX of 50% - 90% is from this landing:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d822990ba9ee&tochange=39bfcadd6492

http://mzl.la/1csWov5
http://mzl.la/18mYqzJ

	Nick Cameron [:nrc]
	Comment 27 • 11 years ago

(In reply to Hannes Verschore [:h4writer] from comment #26)
> Can I assume the Tab Animation Regression on MaxOSX of 50% - 90% is from
> this landing:
> http://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=d822990ba9ee&tochange=39bfcadd6492
> 
> http://mzl.la/1csWov5
> http://mzl.la/18mYqzJ

Yes, totally expected and acceptable - see dev.planning for more details.

	Carsten Book [:Tomcat]
	Comment 28 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/caa48441db53
https://hg.mozilla.org/mozilla-central/rev/d04193bc1627
https://hg.mozilla.org/mozilla-central/rev/aa0066b3865c