Add layers.dump to dump layer tree

RESOLVED FIXED in mozilla28

Status

()

Core
Graphics: Layers
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

unspecified
mozilla28
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 829327 [details] [diff] [review]
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.
Attachment #829327 - Flags: review?(bas)
Attachment #829327 - Flags: review?(bas) → review+
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!
(Assignee)

Comment 2

4 years ago
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.
Attachment #829327 - Flags: review?(ncameron)

Comment 3

4 years ago
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?
Attachment #829327 - Flags: review?(ncameron) → review+
(Assignee)

Comment 4

4 years ago
(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?
Flags: needinfo?(ncameron)

Comment 5

4 years ago
(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.
Flags: needinfo?(ncameron)
(Assignee)

Comment 6

4 years ago
Created attachment 833379 [details] [diff] [review]
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.
Assignee: nobody → bgirard
Attachment #829327 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #833379 - Flags: review?(ncameron)
(Assignee)

Comment 7

4 years ago
Also note that now I only support the OMTC case.

Comment 8

4 years ago
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?
Attachment #833379 - Flags: review?(ncameron) → review+
(Assignee)

Comment 9

4 years ago
Created attachment 8333566 [details] [diff] [review]
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
Attachment #833379 - Attachment is obsolete: true
Attachment #8333566 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/92f737230338
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.