Closed
Bug 924403
Opened 11 years ago
Closed 11 years ago
Remove gfx/layers/opengl
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: BenWa, Assigned: nical)
References
(Blocks 1 open bug)
Details
(Whiteboard: [qa-])
Attachments
(3 files, 1 obsolete file)
183.42 KB,
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
7.61 KB,
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
993 bytes,
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
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
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!
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?
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.
Comment 6•11 years ago
|
||
You're going to remove the deprecated textures? And that needs this? That's a different story then.
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.
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).
Attachment #827985 -
Flags: review?(ncameron)
Assignee | ||
Comment 9•11 years ago
|
||
Some build fixes
Attachment #827985 -
Attachment is obsolete: true
Attachment #827985 -
Flags: review?(ncameron)
Attachment #828029 -
Flags: review?(ncameron)
Assignee | ||
Comment 10•11 years ago
|
||
try push https://tbpl.mozilla.org/?tree=Try&rev=44f34d0623c1
Comment 11•11 years ago
|
||
With this does it make sense to eliminate the GLManager abstraction?
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.
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
Attachment #828029 -
Flags: review?(ncameron) → review+
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.
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.
Comment 16•11 years ago
|
||
Attachment #828277 -
Flags: review?(matt.woodrow)
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.
Updated•11 years ago
|
Attachment #828277 -
Flags: review?(matt.woodrow) → review+
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).
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).
Comment 20•11 years ago
|
||
Another try push to check we still build with patch 2: https://tbpl.mozilla.org/?tree=Try&rev=75afdf6272a0
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?
Flags: needinfo?(gal)
Comment 22•11 years ago
|
||
No. Land away. I can rebase on top. Thanks for the patch here :)
Flags: needinfo?(gal)
Comment 23•11 years ago
|
||
Attachment #829037 -
Flags: review?(matt.woodrow)
Updated•11 years ago
|
Attachment #829037 -
Flags: review?(matt.woodrow) → review+
Updated•11 years ago
|
Updated•11 years ago
|
Comment 24•11 years ago
|
||
If this sticks I am buying beer for the team.
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
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
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.
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
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Updated•11 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•