Closed Bug 877115 Opened 11 years ago Closed 10 years ago

Moz2Dify GLContext and GLTextureImage

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: nical, Assigned: pehrsons)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [mentor=nsilva@mozilla.com][lang=c++][engineering-friend][leave-open])

Attachments

(4 files, 37 obsolete files)

19.16 KB, patch
nical
: review+
bjacob
: feedback+
RyanVM
: checkin+
Details | Diff | Splinter Review
2.91 KB, patch
nical
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
2.63 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
2.36 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
It would be handy that TextureImage and GLContext work with the Moz2D classes. Right now they use thebes for sizes, texture uploads, etc. which makes it very tempting to only use thebes in code that uses TextureImage (like layers), rather than using the new shiny Moz2D.
I am thinking of something like that in the shot term: methods that just wrap the thebes related stuff in GLContext/textureImage's, with a gfx2D signature instead and calls in the thebes implem internally.

At some point we'll have to take the time to actually purge the thebes stuff out of GLContext, but it'll take time to fix all the code that depends on it.
Attachment #755306 - Flags: feedback?(jgilbert)
Comment on attachment 755306 [details] [diff] [review]
Moz2Dify GLContext and GLTextureImage

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

::: gfx/gl/GLContext.h
@@ +885,5 @@
>                                               const nsIntPoint& aSrcPoint = nsIntPoint(0, 0),
>                                               bool aPixelBuffer = false,
>                                               GLenum aTextureUnit = LOCAL_GL_TEXTURE0);
> +    ShaderProgramType UploadSurfaceToTexture(gfx::DataSourceSurface* aSurface,
> +                                             const gfx::IntRect& aDstRegion,

err, actually I meant nsIntRegion here

::: gfx/gl/GLTextureImage.h
@@ +171,5 @@
>       */
>      virtual bool DirectUpdate(gfxASurface *aSurf, const nsIntRegion& aRegion, const nsIntPoint& aFrom = nsIntPoint(0,0)) = 0;
> +    // Moz2D equivalent
> +    bool UpdateFromDataSource(gfx::DataSourceSurface *aSurf,
> +                              const gfx::IntRect& aRect,

nsIntRegion here instead of IntRect
(In reply to Nicolas Silva [:nical] from comment #1)
> Created attachment 755306 [details] [diff] [review]
> Moz2Dify GLContext and GLTextureImage
> 
> I am thinking of something like that in the shot term: methods that just
> wrap the thebes related stuff in GLContext/textureImage's, with a gfx2D
> signature instead and calls in the thebes implem internally.
> 
> At some point we'll have to take the time to actually purge the thebes stuff
> out of GLContext, but it'll take time to fix all the code that depends on it.

Why not do what we can now? I don't see the reason for keeping the functions you touch here in GLContext.
It's mostly a matter of how much I am willing to be side-tracked from the TextureClient/Host stuff. I really want to get the Texture work done asap so that the people pending on it don't wait for too long, but at the same time it needs at least an equivalent to TextureImage::DirectUpdate that uses a DataSourceSurface rather than a gfxASurface.

I can commit to remove the thebes stuff from TextureImage entirely after the TextureClient/Host work.
Comment on attachment 755306 [details] [diff] [review]
Moz2Dify GLContext and GLTextureImage

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

I will hold you to this. :)
Attachment #755306 - Flags: feedback?(jgilbert) → feedback+
Comment on attachment 755306 [details] [diff] [review]
Moz2Dify GLContext and GLTextureImage

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

::: gfx/gl/GLContext.cpp
@@ +1932,5 @@
>    data += aPoint.x * gfxASurface::BytePerPixelFromFormat(aSurf->Format());
>    return data;
>  }
>  
> +ShaderProgramType 

Trailing whitespace

::: gfx/gl/GLTextureImage.h
@@ +20,5 @@
> +class DataSourceSurface;
> +}
> +}
> +
> +namespace mozilla {

Merge the two mozilla blocks
Whiteboard: [mentor=nsilva@mozilla.com][lang=c++]
I'd like to provisionaly grab this and see what I can do. I'd be aiming for a proper conversion, rather than more wrappers (which just piles up work for the future,) so I'd appreciate any hints as to what order to approach that in, i.e., how can I break it up into smaller jobs starting with those where the client code will give the least aggro?
(In reply to Adrian May [:adrianmay] from comment #7)
> I'd like to provisionaly grab this and see what I can do. I'd be aiming for
> a proper conversion, rather than more wrappers (which just piles up work for
> the future,) so I'd appreciate any hints as to what order to approach that
> in, i.e., how can I break it up into smaller jobs starting with those where
> the client code will give the least aggro?

Great!

So basically we want to take all the APIs in GLContext and TextureImage that use Thebes classes (such as gfxImageSurface, gfxIntSize, gfxImageFormat...) and convert them into using Moz2D classes (gfx::DataSourceSurface, gfx::IntSize, gfx::SurfaceFormat...).

Thebes stuff lives here: http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/
Moz2D stuff here: http://mxr.mozilla.org/mozilla-central/source/gfx/2d/

In any case, we will need the Thebes-style APIs to stick around for a while in GLContext and TextureImage, because I suppose they have a lot of users a bit of everywhere and you don't want to convert users of GLContext into using Moz2D all at once.

You could take one of the Thebes-based methods, convert it into using Moz2D, and then wrap this method into an adapter for thebes so that e keep the thebes interface but then it is easier to get rid of it.

like, say, TextureImage::UploadSurfaceToTexture:
Right now it is implemented in term of Thebes and wrapped into a Moz2D adapter method, you could invert this by implementing it in term of Moz2D and wrap it with a Thebes.

Maybe doing this will require that you convert other methods of TextureImage or GLContext. You will find out along the way. If so, try to do them in separate patches to ease reviews and make it not not painful for you to rebase if other people touch surrounding code before you get to land your work.

Don't hesitate to ask a lot of questions on this bug, by email, or on irc.mozilla.org on the #gfx channel.
Assignee: nobody → adrian.alexander.may
Attached patch Moz2DWrappers_1.patch (obsolete) — Splinter Review
OK, here's a start. But there are problems. 

GetUpdateRegion is blocked because I can't find a Moz2D equivalent of nsIntRegion, i.e. something with lots of rectangles in. Maybe I'm just blind.

BeginUpdate and GetBackingSurface have bigger problems. They return gfxASurface. That has a whole zoo of derived classes which can all be returned in these positions, but don't all seem to have equivalents in Moz2D. Even if they did, there'd be a lot of work to clone the old types into the new ones.

Advice?
Attachment #787408 - Flags: review?(nical.bugzilla)
Comment on attachment 787408 [details] [diff] [review]
Moz2DWrappers_1.patch

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

Awesome, I am glad to see you are already getting started!

::: gfx/gl/GLTextureImage.cpp
@@ +644,5 @@
>  
> +
> +// Moz2D equivalents...
> +const gfx::IntSize& TextureImage::GetSizeMoz2D() const { 
> +  return ToIntSize(mSize); 

nit: please remove the trailing white spaces.

::: gfx/gl/GLTextureImage.h
@@ +144,5 @@
>      virtual nsIntRect GetTileRect() {
>          return nsIntRect(nsIntPoint(0,0), mSize);
>      }
> +    // Moz2D equivalent...
> +    gfx::IntRect GetTileRectMoz2D();

See my comment about GetSizeMoz2D

@@ +242,5 @@
>      { return nullptr; }
>  
>      const nsIntSize& GetSize() const { return mSize; }
> +    // Moz2D equivalent...
> +    const gfx::IntSize& GetSizeMoz2D() const;

I hadn't really thought about the case of return types (sorry), but actually I am not sure it is worth adding a new method name for the cases where we are converting the method name. It makes the user go from

gfx::IntSize size = gfx::ToIntSize(image->GetSize());

to

gfx::IntSize size = image->GetSizeMoz2D();

While ultimately we want to just call

gfx::IntSize size = image->GetSize();

I am not sure it is worth adding temporary method names in this case since the callers will have to be rewritten as we advance the Moz2Dification.
What you did for method parameters is definitely good, though.

It would be interesting to see how much foreign code you would need to modify to convert GetSize() to just return a gfx::IntSize (the modification would just be wrapping the caller into gfx::NsIntSize(). That could be worth doing in a separate patch.

I realize I may be contradicting myself (sorry about that) because I said earlier that you don't want to convert all the users of TextureImage to Moz2D at once, but in the case of a simple method like GetSize it is much easier than in the case of stuff that manipulates gfxImageSurfaces for instance.

@@ +283,5 @@
>      virtual nsIntRect GetSrcTileRect() {
>          return nsIntRect(nsIntPoint(0,0), mSize);
>      }
> +    // Moz2D equivalent...
> +    gfx::IntRect GetSrcTileRectMoz2D(); 

nit: please remove the trailing white spaces.
And same comment as for GetSizeMoz2D.
Comment on attachment 787408 [details] [diff] [review]
Moz2DWrappers_1.patch

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

If you can reupload a patch without the XxxMoz2D() methods, i'll r+ it.
Cancelling the review of this patch in the mean time (usually some people use r- in this case but it tends to scare new contributors because of the "negative" notion, there is stuff in your patch that we want to keep).
Attachment #787408 - Flags: review?(nical.bugzilla)
Moz2D doesn't have an equivalent of nsIntRegion yet, so we will keep nsIntRegion arounf for a while.
OK, I'll zap the XxxMoz2D methods and take a look at converting the client code for IntRect and IntSize, It should be a no-brainer, but perhaps big. I guess we won't touch the gfxASurface stuff?
Attached patch Moz2DWrappers_2.patch (obsolete) — Splinter Review
Here it is without the XxxMoz2D calls or trailing spaces.
Attachment #787408 - Attachment is obsolete: true
Attachment #787445 - Flags: review?(nical.bugzilla)
Comment on attachment 787445 [details] [diff] [review]
Moz2DWrappers_2.patch

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

cool, I'll land this today
Attachment #787445 - Flags: review?(nical.bugzilla) → review+
(In reply to Adrian May [:adrianmay] from comment #13)
> OK, I'll zap the XxxMoz2D methods and take a look at converting the client
> code for IntRect and IntSize, It should be a no-brainer, but perhaps big. I
> guess we won't touch the gfxASurface stuff?

Yes, my advice is try to do one patch just for GetSize() first, then another one for, say, GetTileRect(), etc. When we are done with the easier stuff we can look at how we can tackle gfxASurface which will most likely be a bit more involved.

As you convert call sites of some of the methods you will probably see that some users of the methods are already using gfx::IntSize and converting to thebes so in some cases you will just remove the adapter glue code, which is nice.
So are nsIntSize and gfxIntSize the same thing? I got an error for overloading a function between those two.
(In reply to Adrian May [:adrianmay] from comment #17)
> So are nsIntSize and gfxIntSize the same thing? I got an error for
> overloading a function between those two.

Apparently so. http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPoint.h#17
Attached patch Moz2DWrappersGetSize.patch (obsolete) — Splinter Review
Changed TextureImage::GetSize to return gfx::IntSize and fixed the clients by including the glue and wrapping ThebesIntSize around the returned gfx::IntSize.

This sits on top of Moz2DWrappers_2.patch. 

I'll plough through the similar cases now and send individual patches.
Attachment #787954 - Flags: review?(nical.bugzilla)
Attached patch Moz2DWrappersTileRect.patch (obsolete) — Splinter Review
Here's another chunk for the TileRect stuff. Sits on top of Moz2DWrappersGetSize.patch and tidies up some stuff I messed earlier.
Attachment #788024 - Flags: review?(nical.bugzilla)
Attached patch Moz2DWrappersMisc.patch (obsolete) — Splinter Review
I think this just about wraps it up for GLTextureImage. Note the removal of the & in GetSize. Sits on top of Moz2DWrappersTileRect.patch.
Attachment #788042 - Flags: review?(nical.bugzilla)
Comment on attachment 787954 [details] [diff] [review]
Moz2DWrappersGetSize.patch

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

::: gfx/gl/GLTextureImage.cpp
@@ +648,5 @@
>    Resize(NsIntSize(aSize));
>  }
>  
> +const gfx::IntSize& TextureImage::GetSize() const { 
> +  return ToIntSize(mSize); 

Please fix trailing whitespace
Attached patch Moz2DWrappersMisc.patch (obsolete) — Splinter Review
Whitespace fixed.
Attachment #788042 - Attachment is obsolete: true
Attachment #788042 - Flags: review?(nical.bugzilla)
Attachment #788102 - Flags: review?(nical.bugzilla)
Comment on attachment 788024 [details] [diff] [review]
Moz2DWrappersTileRect.patch

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

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +182,1 @@
>        return gfx::IntSize(rect.width, rect.height);

Could be "return mTexImage->GetTileRect().Size();"

@@ +182,3 @@
>        return gfx::IntSize(rect.width, rect.height);
>      }
>      return gfx::IntSize(mTexImage->GetSize().width, mTexImage->GetSize().height);

And you could have made this "return mTexImage->GetSize();" in the previous patch.

@@ +367,3 @@
>        return gfx::IntSize(rect.width, rect.height);
>      }
>      return gfx::IntSize(mTexture->GetSize().width, mTexture->GetSize().height);

Ditto

::: gfx/layers/opengl/ThebesLayerOGL.cpp
@@ +235,5 @@
>  
>      bool usingTiles = (mTexImage->GetTileCount() > 1);
>      do {
>        if (mTexImageOnWhite) {
> +        NS_ASSERTION(ThebesIntRect(mTexImageOnWhite->GetTileRect()) == ThebesIntRect(mTexImage->GetTileRect()), "component alpha textures should be the same size.");

Is this necessary?
Attached patch Moz2DWrappersMisc.patch (obsolete) — Splinter Review
Fixed. I see what you meant about existing wrappers now.

The assert did indeed need massaging. Otherwise it complained about == being private in something, I forget what. Try it and see. It was interesting.
Attachment #788107 - Flags: review?(nical.bugzilla)
Attachment #788102 - Attachment is obsolete: true
Attachment #788102 - Flags: review?(nical.bugzilla)
Comment on attachment 787954 [details] [diff] [review]
Moz2DWrappersGetSize.patch

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

r=me with white spaces fixed
Attachment #787954 - Flags: review?(nical.bugzilla) → review+
Hi Nicolas,

> r=me with white spaces fixed

Sorry, I'm a bit confused. Are you asking me to change something or satisfied with the current patch?
Comment on attachment 788024 [details] [diff] [review]
Moz2DWrappersTileRect.patch

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

I agree with Ms2ger's comments, r=me with his comments addressed (except for the assert change since it is needed).
Attachment #788024 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 787954 [details] [diff] [review]
Moz2DWrappersGetSize.patch

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

And please add the two changes Ms2ger mentioned in the other patch here, about returning mTexImage->GetSize() directly in TextureImageTextureSourceOGL::GetSize()
and TextureImageDeprecatedTextureHostOGL::GetSize()
(In reply to Adrian May [:adrianmay] from comment #27)
> Hi Nicolas,
> 
> > r=me with white spaces fixed
> 
> Sorry, I'm a bit confused. Are you asking me to change something or
> satisfied with the current patch?

The patch doesn't have any outstanding logic problem so I "r+ed" it but I would like you to upload a new version with white spaces fixed and that makes GetSize look nice in TextureImageTextureSourceOGL. (since those are just small changes I wouldn't r- the patch. so r+ in advance if you make small changes).

Since you  will be uploading new versions of the patches bugzilla will put them out of order, please add "Part X -" (or anything with a number of a letter to indicate the order) in the title of the new patches so that we can easily tell the order.

And thanks again for submitting patches so quickly
OK, I'll note the "Part X" thing for the future. In this instance, the 4 patches all follow on from each other in the order that they're posted above. I obsoleted anything else.

I believe I fixed everything Ms2ger asked for by modifying the final patch. I realise that's a bit naff cos you'd have to land dodgy patches and then fix them with the final one, so you can only do them all at once, which is bad. 

But right now I'm not sure how to fix that in my hg q. I could qpop to the original problems and fix them, but then the subsequent patches would be baseless, or would they?

So is it OK right now or do you need more changes from me?
Comment on attachment 788107 [details] [diff] [review]
Moz2DWrappersMisc.patch

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

Oh okay I hadn't seen you'd addressed the previous comments in a followup patch.
Usually it's better to do it in the patches themselves but in this case since all the patch do roughly the same stuff and are small it's okay to do it as a follow up.
If you have already fixed your previous patch we'll take it, otherwise I'll fold them. Sorry for the contradictory information.
For the next patches let's modify the patches themselves when addressing comments rather than doing followups. If you are not using it, mercurial queues is an hg extension that helps a lot with this kind of workflows.

Same here, r=me with comments below addressed.

::: gfx/gl/GLTextureImage.h
@@ +236,5 @@
>       */
>      virtual already_AddRefed<gfxASurface> GetBackingSurface()
>      { return nullptr; }
>  
> +    const gfx::IntSize GetSize() const;

Since you return by value you don't need to make the returned value const.

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +619,5 @@
>      SurfaceStream* surfStream = SurfaceStream::FromHandle(streamDesc.handle());
>      if (surfStream) {
>        mStreamGL = dont_AddRef(surfStream->GLContext());
>      }
> +  }

Let's not fix the white spaces outside of the scope of the patch. It's a bit frustrating but it makes the hg history easier to navigate.
Attachment #788107 - Flags: review?(nical.bugzilla) → review+
Also, when uploading a modified version of a patch that has been r+ed already, just add "(carries r=nical)" in the patch title, non need to ask for another review if the changes aren't big.
Attached patch Moz2DWrappersMisc.patch (obsolete) — Splinter Review
Here then, with the other trailing whitespace back in, the const removed, and the endorsement in the patch title.
Attachment #788107 - Attachment is obsolete: true
BTW, I am using hg queues, but I still don't know how to modify an earlier patch without the later ones losing their base, i.e. trying to patch files that aren't the same anymore. Or is it clever enough to rebase the later patches automatically?
Attached patch Moz2DWrappersMisc.patch (obsolete) — Splinter Review
Took the const out of the cpp as well. Duh!
Attachment #788150 - Attachment is obsolete: true
Whiteboard: [mentor=nsilva@mozilla.com][lang=c++] → [mentor=nsilva@mozilla.com][lang=c++][engineering-friend]
Rebased and folded the patch queue into one patch.
pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=32813127cb6b

I'll land as soon as try is green.
Attachment #755306 - Attachment is obsolete: true
Attachment #787445 - Attachment is obsolete: true
Attachment #787954 - Attachment is obsolete: true
Attachment #788024 - Attachment is obsolete: true
Attachment #788167 - Attachment is obsolete: true
(In reply to Adrian May [:adrianmay] from comment #35)
> BTW, I am using hg queues, but I still don't know how to modify an earlier
> patch without the later ones losing their base, i.e. trying to patch files
> that aren't the same anymore. Or is it clever enough to rebase the later
> patches automatically?

To modify an earlier patch you need to come back to that patch ("hg qpop") and once you are done modifying and you have "hg qrefresh"ed it, "hg qpush" back to the later ones and solve the eventual conflicts.

You have to rebase your patch queue every now and then because mozilla-central changes while you are writing your code, so you at least need to rebase before landing.

The more often you do it the easiest it is to rebase, but it always depend on how complex the patch is. For instance it took me just a few minutes to rebase your patch queue.

I would not say mq is very clever about rebasing, (basically it will not try anything fancy if the code has changed within a few lines of the code you modified) but it does the job and if the patch is small enough it is usally trivial to fix the conflicts.
OK thanks for the tute. I'll experiment with that in some sandbox. In the meantime I'm busy with something for the Android crew but I'll be back over here soon enough, probably finishing off 898436.
fixed the build and landed
https://hg.mozilla.org/integration/mozilla-inbound/rev/32c7fa71f624
Whiteboard: [mentor=nsilva@mozilla.com][lang=c++][engineering-friend] → [mentor=nsilva@mozilla.com][lang=c++][engineering-friend][leave-open]
It'd be great to move the methods in GLContext that we are converting to Moz2D into a separate file and make them not methods of GLContext at all.

Say, GLUtils.h/.cpp

for example:

already_AddRefed<gfxImageSurface> mozilla::gl::GLContext::GetTexImage(GLuint aTexture, bool aYInvert, SurfaceFormat aFormat);

Would become

TemporaryRef<mozilla::gfx::DataSourceSurface> mozilla::gl::ReadBack(GLuint aTexture, bool aYInvert, SurfaceFormat aFormat)

Doing that would also help slim down GLContext that's become bloated.
Since we are already hacking around on GLContext, would it make sense to put the current context into TLS, since the driver portion of it already is anyway? Ref-counting the GLContext and keeping a million references in destructors to it seems to cause a trail of tears and shutdown issues.
(In reply to Andreas Gal :gal from comment #43)
> Since we are already hacking around on GLContext, would it make sense to put
> the current context into TLS, since the driver portion of it already is
> anyway? Ref-counting the GLContext and keeping a million references in
> destructors to it seems to cause a trail of tears and shutdown issues.

Do you mean keeping track of the last context that was made current to avoid the cost of MakeCurrent()? If so my understanding is that we can't do this because of something like plugins that are not out of process may be doing opengl stuff with their own contexts and we can't track that.
Regarding putting the current context in TLS, see bug 749678, where this was proposed in an attempt to optimize away redundant MakeCurrent calls and make GetCurrentContext cheap. This didn't land because on most platforms we couldn't easily ensure that we were the only ones talking to OpenGL on the main thread; but this could definitely land on Firefox OS if it's useful there; and recently in a IRC conversation Vlad suggested that we could actually make this safe everywhere by patching {egl,wgl,etc}MakeCurrent in memory directly with our TLS instrumentation.

What issues are caused by GLContext being reference-counted?

Notice that, like many things around gfx, a GLContext can have to be referenced by various things belonging to different 'world' each with their own lifetimes. For example, a GLContext may have to be referenced by a Layer, whose lifetime is tied to the IPC system, and it could also have to be referenced by a canvas rendering context, whose lifetime is tied to garbage collection. Given that, it is difficult to imagine an alternative to reference-counting GLContexts.
That doesn't make sense to me. A GLContext has a specific lifetime. If I make a canvas, the GLContext should live as long as that canvas lives. Nobody should ever get access to that context, except to render to it. Nobody should render to it once it was destroyed. Similarly for layers.
Well, that was a bad example, sorry. I was thinking of surfaces, not GL contexts. Let me try again.

Suppose that we start multiplexing GLContext's. Multiplexing OpenGL contexts is something that we want to start playing with soon for Skia/GL; doing that multiplexing at the level of GLContext is a reasonable thing to want to do eventually so that more things can share a GLContext (e.g. WebGL contexts). When we get there, we'll have multiple things holding on to a GLContext, and they'll have unrelated lifetimes (e.g. canvas rendering contexts exposed to javascript).

Speaking of Skia/GL, that was another good use case for refcounted GLContext, as we had many pieces of code trying to reference that GLContext (the CanvasRenderingContext2D, the DrawTargetSkia, Skia internals) and being able to rely on refcounting GLContext allowed for simpler, easier-to-review code that doesn't crash.

More generically, GLContexts are just tools to talk to the GPU; I don't see a strong reason to want them to have single owners. They should be accessed from only one thread, but it should be OK for them to be used by many things, as long as these things are on the same thread.
Thats an even stronger reason to not ref-count GLContext. Instead we should have 1 GLContext per thread. All access can be scoped to ensure exclusive access and we can lazily spin up the GLContext when a thread accesses it and spin it down on a timeout or shutdown (again, access is always scoped, so doing so from the event loop is safe).
Maybe, that solution (absolute multiplexing, only 1 GLContext per thread) will be possible; that would actually be pretty cool; I had actually not thought of going that far; I can't tell out of hand whether it's going to be actually possible or not, especially if we are talking about doing it everywhere on all platforms (otherwise, it's not removing the need for refcounting). Things to watch out for will include:

 1. Context loss. We'd now lose all GLContext's as soon as we lose one, e.g. when running a 'nasty' WebGL script. That would be a regression on OSes/drivers where per-context loss works reliably, which is the case of e.g. recent NVIDIA desktop drivers.

 2. Possible performance implications. Part of why I don't know yet if we'll want to multiplex WebGL's GLContexts everywhere (on mobile, we'll probably want to, for the memory savings, but that is less critical on desktop) is that I don't know yet what the performance implications will be. In theory it should not be bad, but I don't know what driver-side performance caveats this not-quite-usual usage pattern of OpenGL will run into. For the more special patterns used by Skia/GL, we already know that Google is using multiplexing there, so it must be working well enough for that, at least on mobile GPUs.

 3. Security implications, which we may or may not care about. In typical recent desktop GPUs (I don't know about mobile), there is an actual MMU, and at least one major GPU vendor said that each OpenGL context gets a separate address space. That means that we get some isolation from using actually separate OpenGL contexts. That might be considered worthwhile e.g. for WebGL on desktop (while security matters less outside of WebGL, and on mobile the memory savings from multiplexing might outweigh these somewhat abstract security considerations).
Also, notice that experimenting with GLContext multiplexing is blocked on bug 901498 which will make it possible to implement GLContext multiplexing without having to write special code paths for all the extraneous code that is currently built into GLContext.
(In reply to Benoit Jacob [:bjacob] from comment #49)
> Maybe, that solution (absolute multiplexing, only 1 GLContext per thread)
> will be possible; that would actually be pretty cool; I had actually not
> thought of going that far; I can't tell out of hand whether it's going to be
> actually possible or not, especially if we are talking about doing it
> everywhere on all platforms (otherwise, it's not removing the need for
> refcounting). Things to watch out for will include:
> 
>  1. Context loss. We'd now lose all GLContext's as soon as we lose one, e.g.
> when running a 'nasty' WebGL script. That would be a regression on
> OSes/drivers where per-context loss works reliably, which is the case of
> e.g. recent NVIDIA desktop drivers.

This is probably tolerable, especially since we are moving to process separation anyway. In case of a context loss we recreate it and layers shouldn't be affected.

> 
>  2. Possible performance implications. Part of why I don't know yet if we'll
> want to multiplex WebGL's GLContexts everywhere (on mobile, we'll probably
> want to, for the memory savings, but that is less critical on desktop) is
> that I don't know yet what the performance implications will be. In theory
> it should not be bad, but I don't know what driver-side performance caveats
> this not-quite-usual usage pattern of OpenGL will run into. For the more
> special patterns used by Skia/GL, we already know that Google is using
> multiplexing there, so it must be working well enough for that, at least on
> mobile GPUs.

I would worry about this once we run into it. The simplification of ripping out GLContext reference is significant. If that ever shows to be bad for perf we can use 1 per type (1 per thread per WebGL use, one per content use, etc). That should fix any performance issues, even if we get them, and still be much easier to reason about.

> 
>  3. Security implications, which we may or may not care about. In typical
> recent desktop GPUs (I don't know about mobile), there is an actual MMU, and
> at least one major GPU vendor said that each OpenGL context gets a separate
> address space. That means that we get some isolation from using actually
> separate OpenGL contexts. That might be considered worthwhile e.g. for WebGL
> on desktop (while security matters less outside of WebGL, and on mobile the
> memory savings from multiplexing might outweigh these somewhat abstract
> security considerations).

Process separation will yield this benefit. As long you have one CPU address space, OpenGL addresses spaces don't matter IMO.

I am currently debugging a GLContext shutdown issue, and thats **** me off enough that I will volunteer for this work once my shader cleanup patch actually has landed.
(Agree on the dependency, this will need a long list of dependencies before we can move on the actually 1-GLContext switch.)
(In reply to Andreas Gal :gal from comment #51)
> >  3. Security implications, which we may or may not care about. In typical
> > recent desktop GPUs (I don't know about mobile), there is an actual MMU, and
> > at least one major GPU vendor said that each OpenGL context gets a separate
> > address space. That means that we get some isolation from using actually
> > separate OpenGL contexts. That might be considered worthwhile e.g. for WebGL
> > on desktop (while security matters less outside of WebGL, and on mobile the
> > memory savings from multiplexing might outweigh these somewhat abstract
> > security considerations).
> 
> Process separation will yield this benefit.

Process separation will only fully yield this benefit if we have iframes of different origins run in different processes. I haven't been following electrolysis closely, so I don't know whether that is part of the current plan.

> As long you have one CPU address
> space, OpenGL addresses spaces don't matter IMO.

On way that GPU-side separate address spaces are potentially interesting even in a single-process browser, is that we arguably trust opaque vendor GPU-side code less than we trust our own CPU-side code. (Of course, there also is the separate problem of bugs in CPU-side driver code).
As long the CPU address space is shared, the GPU address space simply doesn't matter. Just about 0 exploits have been shown attacking via a GPU address space vector. Thousands have been shown attacking via CPU address space. I am comfortable going with the historic numbers here.
I have been looking at Moz2Difying the GLContext a bit now. I'll submit some patches this week.

adrianmay: if you are still on top of this, let me know :)
Attachment #8345314 - Flags: review?(nical.bugzilla)
Attachment #8345316 - Flags: review?(nical.bugzilla)
Attachment #8345317 - Flags: review?(nical.bugzilla)
Attachment #8345318 - Flags: review?(nical.bugzilla)
Attachment #8345319 - Flags: review?(nical.bugzilla)
bzexport didn't set the reviewer properly from the summary apparently..

Changes are mostly to gfxIntSize. Should be straight forward.
Will need a run through try as well. I have only tried compiling for mac and B2G.
Attachment #8345314 - Flags: review?(nical.bugzilla) → review+
Attachment #8345316 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8345317 [details] [diff] [review]
[Moz2Dify] Part 3. Change gfxIntSize to gfx::IntSize in GLContext::OffscreenSize and dependendent classes.

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

r=me with the comment below added

::: gfx/layers/CopyableCanvasLayer.cpp
@@ +130,5 @@
> +      RefPtr<DataSourceSurface> readDSurf = sharedSurf_Basic->GetData();
> +      readSurf = new gfxImageSurface(readDSurf->GetData(),
> +                                     ThebesIntSize(readDSurf->GetSize()),
> +                                     readDSurf->Stride(),
> +                                     SurfaceFormatToImageFormat(readDSurf->GetFormat()));

Here we need to make sure that readSurf never outlives sharedSurf_Basic->mData. Please add a comment here stressing that (in particular we should be careful not to return it), and as a security, please also declare RefPtr<DataSourceSurface> readDSurf next to readSurf's declaration, so that the lifetime constraint is visible and explicit in this code.
Attachment #8345317 - Flags: review?(nical.bugzilla) → review+
Attachment #8345318 - Flags: review?(nical.bugzilla) → review+
Attachment #8345319 - Flags: review?(nical.bugzilla) → review+
Ouch! It looks like this stuff collided badly with the GLContext modularization work :( could you rebase again, please? A lot of stuff has moved away from GLContext.h into other files but the logic should not have changed much, so if the rebase goes smoothly and it builds no need for another round of reviews, I'll push to try right away.
Attachment #8345314 - Attachment is obsolete: true
Fixed according to your comments.
Attachment #8345317 - Attachment is obsolete: true
Attachment #8345912 - Flags: review?(nical.bugzilla)
Attachment #8345319 - Attachment is obsolete: true
Attachment #8345916 - Flags: review?(nical.bugzilla)
Attachment #8345898 - Attachment description: [Moz2Dify] Part 1. Remove ImageFormat typedef from GLContext. ( → [Moz2Dify] Part 1. Remove ImageFormat typedef from GLContext. (carries r=nical)
Attachment #8345898 - Flags: review?(nical.bugzilla)
Attachment #8345910 - Flags: review?(nical.bugzilla)
nical, there you go, freshly rebased and fixed.

I tried the carries r= magic both through bzexport and manually but no go there. Any hints?
bzexport also frequently complains about not being able to set the assignee. Do you mind? =)
Assignee: adrian.alexander.may → pehrsons
(In reply to Andreas Pehrson [:pehrsons] from comment #70)
> nical, there you go, freshly rebased and fixed.
> 
> I tried the carries r= magic both through bzexport and manually but no go
> there. Any hints?

I don't use bzexport so I don't know what problem you are referring to. What's wrong with your patches? carries r= is in the title so it looks good to me.

(In reply to Nicolas Silva [:nical] from comment #64)
> Ouch! It looks like this stuff collided badly with the GLContext
> modularization work :( could you rebase again, please?

Err sorry, I got mixed up and thought I had to apply the first patch too (the one written by Adrian) which has already landed so it collided massively with itself. I could have probably applied your patches without asking you to rebase.
Attachment #8345898 - Flags: review?(nical.bugzilla) → review+
Attachment #8345910 - Flags: review?(nical.bugzilla) → review+
Attachment #8345912 - Flags: review?(nical.bugzilla) → review+
Attachment #8345915 - Flags: review?(nical.bugzilla) → review+
Attachment #8345916 - Flags: review?(nical.bugzilla) → review+
I thought carries would make bugzilla keep the r+ but maybe that's wrong. Can't see it documented anywhere.
(In reply to Andreas Pehrson [:pehrsons] from comment #73)
> I thought carries would make bugzilla keep the r+ but maybe that's wrong.
> Can't see it documented anywhere.

I don't think we have that magic, so we put "carries r=" in the patch title when uploading to bugzilla. It's better if you just put "r=" at the end of the hg commit message though.
Looks like you have a few build targets to fix :)
Ah. Ohwell.

I set up a windows environment to make my future life easier, so fixing this has taken some time. Should be in by tomorrow if most things compile over night!
I'll apply your patches locally and work on top of them, so that I don't bitrot you as I continue with GLContext cleanup.
Attachment #8345912 - Attachment is obsolete: true
Attachment #8347166 - Attachment is obsolete: true
Attachment #8347180 - Attachment is obsolete: true
Comment on attachment 8347182 [details] [diff] [review]
[Moz2Dify] Part 3. Change gfxIntSize to gfx::IntSize in GLContext::OffscreenSize and dependendent classes.

Fixed the windows errors now. Could you please double check that I am using moz2d correctly in CanvasLayerD3D*::UpdateSurface?

Cheers!
Attachment #8347182 - Flags: review?(nical.bugzilla)
Comment on attachment 8345916 [details] [diff] [review]
[Moz2Dify] Part 5. Minimize gfxIntSize usage in GLContext. (carries r=nical)

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

::: gfx/gl/GLContext.cpp
@@ +2507,5 @@
>      return true;
>  }
>  
>  bool
> +GLContext::ResizeScreenBuffer(const IntSize& size)

ResizeOffscreen calls ResizeScreenBuffer(ThebesIntSize(aNewSize)), which creates a new gfxIntSize, only to implicitly create a new IntSize to call this function. Please remove the ThebesIntSize call.
Attachment #8345916 - Attachment is obsolete: true
Attachment #8347198 - Attachment description: [Moz2Dify] Part 5. Minimize gfxIntSize usage in GLContext. (carries → [Moz2Dify] Part 5. Minimize gfxIntSize usage in GLContext. (carries r=nical)
Attachment #8347182 - Flags: review?(nical.bugzilla) → review+
Attachment #8347182 - Attachment is obsolete: true
Comment on attachment 8348133 [details] [diff] [review]
[Moz2Dify] Part 3. Change gfxIntSize to gfx::IntSize in GLContext::OffscreenSize and dependendent classes.

Rebase needed some new code.
Attachment #8348133 - Flags: review?(nical.bugzilla)
Attachment #8348273 - Flags: review?(nical.bugzilla)
Attachment #8348273 - Flags: review?(nical.bugzilla) → review+
Attachment #8348133 - Flags: review?(nical.bugzilla) → review+
I reproduced, in a debug build on linux with GL layers enabled ( layers.acceleration.force-enabled , default outside of linux), while running online WebGL 1.0.1 conformance tests ( https://www.khronos.org/registry/webgl/conformance-suites/1.0.1/webgl-conformance-tests.html ).

Assertion failure: dest->Stride() == dest->Width() * destPixelSize, at /hack/mozilla-central/gfx/gl/GLContext.cpp:2145

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff074da37 in mozilla::gl::GLContext::ReadPixelsIntoImageSurface (this=0x7fffbc47f800, dest=0x7fffbec2fa60) at /hack/mozilla-central/gfx/gl/GLContext.cpp:2145
2145        MOZ_ASSERT(dest->Stride() == dest->Width() * destPixelSize);
(gdb) bt
#0  0x00007ffff074da37 in mozilla::gl::GLContext::ReadPixelsIntoImageSurface (this=0x7fffbc47f800, dest=0x7fffbec2fa60) at /hack/mozilla-central/gfx/gl/GLContext.cpp:2145
#1  0x00007ffff074d72f in mozilla::gl::GLContext::ReadScreenIntoImageSurface (this=0x7fffbc47f800, dest=0x7fffbec2fa60) at /hack/mozilla-central/gfx/gl/GLContext.cpp:2086
#2  0x00007ffff076c00b in mozilla::gl::SharedSurface_Basic::Fence (this=0x7fffbabfe880) at /hack/mozilla-central/gfx/gl/SharedSurfaceGL.cpp:310
#3  0x00007ffff076dc01 in mozilla::gfx::SurfaceStream_TripleBuffer::SwapProducer (this=0x7fffaf5148a0, factory=0x7fffc5cd5980, size=...) at /hack/mozilla-central/gfx/gl/SurfaceStream.cpp:425
#4  0x00007ffff0754faf in mozilla::gl::GLScreenBuffer::Swap (this=0x7fffb6f51ca0, size=...) at /hack/mozilla-central/gfx/gl/GLScreenBuffer.cpp:424
#5  0x00007ffff0755115 in mozilla::gl::GLScreenBuffer::PublishFrame (this=0x7fffb6f51ca0, size=...) at /hack/mozilla-central/gfx/gl/GLScreenBuffer.cpp:445
#6  0x00007ffff074be59 in mozilla::gl::GLContext::PublishFrame (this=0x7fffbc47f800) at /hack/mozilla-central/gfx/gl/GLContext.cpp:1498
#7  0x00007ffff176f08f in mozilla::WebGLContext::PresentScreenBuffer (this=0x7fffb5887800) at /hack/mozilla-central/content/canvas/src/WebGLContext.cpp:1186
#8  0x00007ffff1778b08 in mozilla::WebGLContextUserData::PreTransactionCallback (data=0x7fffb933ced0) at /hack/mozilla-central/content/canvas/src/WebGLContext.cpp:857
#9  0x00007ffff081d4ab in mozilla::layers::CanvasLayer::FirePreTransactionCallback (this=0x7fffc46cf800) at /hack/mozilla-central/gfx/layers/Layers.h:1834
#10 0x00007ffff082a805 in mozilla::layers::ClientCanvasLayer::RenderLayer (this=0x7fffc46cf800) at /hack/mozilla-central/gfx/layers/client/ClientCanvasLayer.cpp:120
(gdb) p dest->Stride()
$2 = 208
(gdb) p dest->Width()
$3 = 50
(gdb) p destPixelSize
$4 = 4

So the bug is that the existing code assumed a fully packed surface (no stride at the end of each row of pixels) and that assumption broken by the switch to moz2d surfaces, which apparently are not packed (above we have a 8 pixel stride at the end of each row (208-50*4))
Attachment #8350089 - Flags: review?(bas)
Attachment #8348133 - Attachment is obsolete: true
Comment on attachment 8350089 [details] [diff] [review]
Add factory method for DataSourceSurfaces with custom stride.

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

::: gfx/2d/Factory.cpp
@@ +655,5 @@
> +Factory::CreateDataSourceSurfaceWithStride(const IntSize &aSize,
> +                                           SurfaceFormat aFormat,
> +                                           int32_t aStride)
> +{
> +  if (aStride < aSize.width * BytesPerPixel(aFormat)) {

nit: Add an assertion here for debug builds.
Attachment #8350089 - Flags: review?(bas) → review+
Now we know that it is part 3 that is causing the reftest failures.
Yep. I finally have a working environment so I hope to have a fix pretty soon. Currently building firefox at AMS =)
I just reproduced it locally. Work can start..
Comment on attachment 8350091 [details] [diff] [review]
[Moz2Dify] Part 3. Change gfxIntSize to gfx::IntSize in GLContext::OffscreenSize and dependendent classes.

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

::: gfx/layers/d3d10/CanvasLayerD3D10.cpp
@@ +168,5 @@
> +                                                                   map.RowPitch,
> +                                                                   FORMAT_B8G8R8A8);
> +          Rect drawRect(0, 0, shareSurf->Size().width, shareSurf->Size().height);
> +          dt->DrawSurface(frameData, drawRect, drawRect);
> +          dt->Flush();

Here is the bad guy causing the failures. I have no idea why yet, but I tested drawing with gfxContext and two gfxImageSurfaces (like before, essentially) and that worked just fine.
Attachment #8350091 - Attachment is obsolete: true
Attachment #8351413 - Flags: review?(nical.bugzilla)
Attachment #8351413 - Flags: feedback?(bjacob)
Made a fix before. This time a rebase.
Attachment #8351413 - Attachment is obsolete: true
Attachment #8351413 - Flags: review?(nical.bugzilla)
Attachment #8351413 - Flags: feedback?(bjacob)
Attachment #8351417 - Flags: review?(nical.bugzilla)
Attachment #8351417 - Flags: feedback?(bjacob)
try push: https://tbpl.mozilla.org/?tree=Try&rev=dd66daddfdea

Let's see some magic! Consider it my christmas gift for you :p
Attachment #8351417 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8351417 [details] [diff] [review]
[Moz2Dify] Part 3. Change gfxIntSize to gfx::IntSize in GLContext::OffscreenSize and dependendent classes.

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

Awesome Xmas present!

Note that there remains the bad enum conversion that we discussed in part 1. I'll fix that too, and land the whole thing.
Attachment #8351417 - Flags: feedback?(bjacob) → feedback+
Apparently due to some more bitrotting, a few more fixes had to be made to get things to build on either desktop Linux or Android. Enough to require another try push:

https://tbpl.mozilla.org/?tree=Try&rev=7c99e2d2750a
Remaining platforms (Win and B2G) also needed similar build fixes:
https://tbpl.mozilla.org/?tree=Try&rev=b6388a1b41fd
Excellent work Benoit! And happy new year =)
Attachment #8357028 - Flags: review?(nical.bugzilla)
Attachment #8357028 - Flags: feedback?(bjacob)
Attachment #8357029 - Flags: review?(nical.bugzilla)
Attachment #8357028 - Flags: review?(nical.bugzilla) → review+
Attachment #8357029 - Flags: review?(nical.bugzilla) → review+
Attachment #8357028 - Flags: feedback?(bjacob) → feedback+
Keywords: checkin-needed
Note on the order: keep it chronological
Attachment #8351417 - Attachment is obsolete: true
Attachment #788921 - Attachment is obsolete: true
Attachment #8345898 - Attachment is obsolete: true
Attachment #8345915 - Attachment is obsolete: true
Attachment #8347198 - Attachment is obsolete: true
Attachment #8350089 - Attachment is obsolete: true
Attachment #8351414 - Attachment is obsolete: true
Attachment #8351415 - Attachment is obsolete: true
Attachment #8357028 - Flags: checkin+
Attachment #8357029 - Flags: checkin+
Comment on attachment 8358486 [details] [diff] [review]
Moz2Dify CanvasLayerD3D9 drawing shared surface into D3DLOCKED_RECT.

This is a continuation of a previous patch on this bug: https://hg.mozilla.org/mozilla-central/rev/4757b9992474
Attachment #8358486 - Flags: review?(nical.bugzilla)
Attachment #8358487 - Flags: review?(nical.bugzilla)
Attachment #8358486 - Flags: review?(nical.bugzilla) → review+
Attachment #8358487 - Flags: review?(nical.bugzilla) → review+
Attachment #8357028 - Attachment is obsolete: true
Attachment #8357029 - Attachment is obsolete: true
Keywords: checkin-needed
Depends on: 959154
Comment on attachment 8357028 [details] [diff] [review]
Remove GetTexImage from GLContext helper;

They're not obsolete just because they landed :)
Attachment #8357028 - Attachment is obsolete: false
Attachment #8357029 - Attachment is obsolete: false
This has some conflicts that I don't want to try to rebase through, sorry :(

Also, should this bug be resolved when these two patches land?
Keywords: checkin-needed
Oh, I just saw you obsolete the older ones and assumed that was how it's done.

No, not resolved yet.
Attachment #8358486 - Attachment is obsolete: true
Attachment #8358487 - Attachment is obsolete: true
Attachment #8359217 - Attachment description: Moz2Dify CanvasLayerD3D9 drawing shared surface into D3DLOCKED_RECT. → Moz2Dify CanvasLayerD3D9 drawing shared surface into D3DLOCKED_RECT. (carries r=nical)
Attachment #8359218 - Attachment description: Moz2Dify CanvasLayerD3D10 drawing shared surface into D3D10_MAPPED_TEXTURE2D. → Moz2Dify CanvasLayerD3D10 drawing shared surface into D3D10_MAPPED_TEXTURE2D. (carries r=nical)
Keywords: checkin-needed
I did it mainly to better keep track of what had landed and what hadn't :). Was hard to make sense of as someone who hasn't been watching this bug all along :). Thanks for the rebase!

https://hg.mozilla.org/integration/mozilla-inbound/rev/dba042cbc381
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfd0864f189d
Keywords: checkin-needed
Attachment #8359217 - Flags: checkin+
Attachment #8359218 - Flags: checkin+
Depends on: 959157
I asked you to do the rebase because of what were obvious context differences that I didn't want to make assumptions about. Looks like those differences were ignored in your rebase. Consequently, these broke Windows builds. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/456104e27a9a

https://tbpl.mozilla.org/php/getParsedLog.php?id=32922046&tree=Mozilla-Inbound
Attachment #8359217 - Attachment is obsolete: true
Attachment #8359217 - Flags: checkin+ → checkin-
Attachment #8359218 - Attachment is obsolete: true
Attachment #8359218 - Flags: checkin+ → checkin-
Attachment #8359338 - Attachment description: Moz2Dify CanvasLayerD3D9 drawing shared surface into D3DLOCKED_RECT. → Moz2Dify CanvasLayerD3D9 drawing shared surface into D3DLOCKED_RECT. (carries r=nical)
Attachment #8359339 - Attachment description: Moz2Dify CanvasLayerD3D10 drawing shared surface into D3D10_MAPPED_TEXTURE2D. → Moz2Dify CanvasLayerD3D10 drawing shared surface into D3D10_MAPPED_TEXTURE2D. (carries r=nical)
Sorry Ryan, apparently not obvious enough for me >_<
Well, lesson learned!

Here goes a try push: https://tbpl.mozilla.org/?tree=Try&rev=4cd01e1836f1
Ah, win64 is not a release target. Then we should be good to go.

Sorry again for the inconvenience.
Keywords: checkin-needed
Attachment #8359338 - Flags: checkin+
Attachment #8359339 - Flags: checkin+
I see these two patches merged in mozilla-central. Should the bug be closed?
No, there is still some work to be done.

I have been occupied by other things lately but hope to get back to it soon.
Cool! Great work Andreas.
Would you mind if we closed this bug and filed a follow-up bug for the remaining work? This stuff landed a while ago and it gets confusing if patches from a single bug are split across releases. Also, this bug shows up as a mentored bug in the bugsahoy dashboard when it's not really appropriate for mentoring it seems like.
Flags: needinfo?(pehrsons)
Please do. I had hoped to get around to finish this one but that never happened, unfortunately. It should not be too much left though.
Flags: needinfo?(pehrsons)
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: