Closed
Bug 936511
Opened 11 years ago
Closed 11 years ago
Add layers.dump to dump layer tree
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: BenWa, Assigned: BenWa)
Details
(Whiteboard: [qa-])
Attachments
(1 file, 2 obsolete files)
55.83 KB,
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
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)
Updated•11 years ago
|
Attachment #829327 -
Flags: review?(bas) → review+
Comment 1•11 years ago
|
||
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•11 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•11 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•11 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•11 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•11 years ago
|
||
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•11 years ago
|
||
Also note that now I only support the OMTC case.
Comment 8•11 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•11 years ago
|
||
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+
Assignee | ||
Comment 10•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/92f737230338
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/92f737230338
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Updated•10 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•