936511 - Add layers.dump to dump layer tree

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: patch

Currently we have the facility to dump layers tree but they are not built by default. We use this so often for debugging that I think it's time to upgrade this to a pref.

Comment 1

[Nicolas Silva [:nical]]

CCing Nick who was, iirc, opposed to doing this.
As far as I am concerned I hate the #ifdef soup that the dump code adds so I say shipit!

Comment 2

[Benoit Girard (:BenWa)]

Comment on attachment 829327 [details] [diff] [review]
patch

Formally asking nrc to review this so you can veto it if you wish with a good reason. IMO it's worth taking *tiny* code bloats to accelerate developer velocity.

Comment 3

[Nick Cameron [:nrc]]

Comment on attachment 829327 [details] [diff] [review]
patch

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

Sure, why not? I think I still object to removing ifdefs as a goal in itself, but it sounds like the benefits outweigh the costs here, so no objections.

::: gfx/layers/LayersTypes.h
@@ +9,5 @@
>  #include <stdint.h>                     // for uint32_t
>  #include "nsPoint.h"                    // for nsIntPoint
>  
> +// Enable by default for layers.dump
> +#define MOZ_LAYERS_HAVE_LOG

If this is going to be always on, can't we just remove it?

Comment 4

[Benoit Girard (:BenWa)]

(In reply to Nick Cameron [:nrc] from comment #3)
> Comment on attachment 829327 [details] [diff] [review]
> patch
> 
> Review of attachment 829327 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sure, why not? I think I still object to removing ifdefs as a goal in
> itself, but it sounds like the benefits outweigh the costs here, so no
> objections.
> 
> ::: gfx/layers/LayersTypes.h
> @@ +9,5 @@
> >  #include <stdint.h>                     // for uint32_t
> >  #include "nsPoint.h"                    // for nsIntPoint
> >  
> > +// Enable by default for layers.dump
> > +#define MOZ_LAYERS_HAVE_LOG
> 
> If this is going to be always on, can't we just remove it?

Well you want to leave ifdef but you want to get rid of the defines? I think it's better to have it here then in the mozilla-config.h. What do you have in mind?

Comment 5

[Nick Cameron [:nrc]]

(In reply to Benoit Girard (:BenWa) from comment #4)
> (In reply to Nick Cameron [:nrc] from comment #3)
> > Comment on attachment 829327 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 829327 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Sure, why not? I think I still object to removing ifdefs as a goal in
> > itself, but it sounds like the benefits outweigh the costs here, so no
> > objections.
> > 
> > ::: gfx/layers/LayersTypes.h
> > @@ +9,5 @@
> > >  #include <stdint.h>                     // for uint32_t
> > >  #include "nsPoint.h"                    // for nsIntPoint
> > >  
> > > +// Enable by default for layers.dump
> > > +#define MOZ_LAYERS_HAVE_LOG
> > 
> > If this is going to be always on, can't we just remove it?
> 
> Well you want to leave ifdef but you want to get rid of the defines? I think
> it's better to have it here then in the mozilla-config.h. What do you have
> in mind?

I meant that in this case I think we should remove the ifdefs. Therefore, we may as well remove the define as well.

Comment 6

[Benoit Girard (:BenWa)]

Attachment: patch

I removed the MOZ_LAYERS_HAVE_LOG needed for printing the layer dump but I left it in for places that were used for other layers logging.

Comment 7

[Benoit Girard (:BenWa)]

Also note that now I only support the OMTC case.

Comment 8

[Nick Cameron [:nrc]]

Comment on attachment 833379 [details] [diff] [review]
patch

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

Can you remove the AC_DEFINE from configure.in too? I don't fully understand why we need MOZ_LAYERS_HAVE_LOG - what other logging is it used for and why can't we just assume it is defined there too? r=me if there is a good answer :-)

After adding the comments about the includes, I noticed you removed some other ones, and therefore we need the ones you left. In which case, just ignore those comments.

::: gfx/layers/LayersTypes.h
@@ +8,5 @@
>  
>  #include <stdint.h>                     // for uint32_t
>  #include "nsPoint.h"                    // for nsIntPoint
>  
> +#define MOZ_LAYERS_HAVE_LOG

We still need this?

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +36,5 @@
>  #include "mozilla/gfx/Types.h"          // for Color, SurfaceFormat
>  #include "mozilla/layers/Compositor.h"  // for Compositor
>  #include "mozilla/layers/CompositorTypes.h"
>  #include "mozilla/layers/Effects.h"     // for Effect, EffectChain, etc
> +#include "mozilla/layers/LayersTypes.h"  // for etc

include again

::: gfx/layers/opengl/TextureHostOGL.h
@@ +21,5 @@
>  #include "mozilla/gfx/Point.h"          // for IntSize, IntPoint
>  #include "mozilla/gfx/Types.h"          // for SurfaceFormat, etc
>  #include "mozilla/layers/CompositorTypes.h"  // for TextureFlags
>  #include "mozilla/layers/LayersSurfaces.h"  // for SurfaceDescriptor
> +#include "mozilla/layers/LayersTypes.h"

do we still need the include?

Comment 9

[Benoit Girard (:BenWa)]

Attachment: patch

As discussed on IRC we're keeping the configure option + MOZ_LAYERS_HAVE_LOG since its still used for a bit of logging.

https://tbpl.mozilla.org/?tree=Try&rev=49f063e59f0f

Comment 10

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92f737230338

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/92f737230338