Last Comment Bug 877567 - Add a tool to toggle the display of layer borders
: Add a tool to toggle the display of layer borders
Status: NEW
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
-- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
: J. Ryan Stinnett [:jryans] (use ni?)
Depends on:
  Show dependency treegraph
Reported: 2013-05-30 00:34 PDT by Panos Astithas [:past]
Modified: 2015-06-09 03:10 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

WIP (7.40 KB, patch)
2013-06-17 02:55 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
Expose layer borders and tile borders to devtools (46.80 KB, patch)
2013-06-17 10:33 PDT, Nicolas Silva [:nical]
no flags Details | Diff | Splinter Review
Expose layer diagnostic flags to devtools for layer and tile borders (38.02 KB, patch)
2013-06-21 06:31 PDT, Nicolas Silva [:nical]
b56girard: review-
Details | Diff | Splinter Review

Description User image Panos Astithas [:past] 2013-05-30 00:34:54 PDT
From bug 873972:

"We now have a way to display the border of layers, which is useful to have an idea of how a webpage is split in different layers. The way content is layered has perforance/memory impacts so it is sometimes useful to see how a given web page is layered.

In order to see layer borders one just needs to set the pref "layers.draw-borders" to true. It works on platforms that use OMTC, that is fennec and B2G at the moment."

We should add a tool/command to toggle the display of layer borders, similar to the existing tool that toggles paint flashing. Since OMTC isn't implemented in desktop yet, we should either make it remote-able from the start, or wait for the desktop implementation first.
Comment 1 User image Paul Rouget [:paul] 2013-06-04 05:45:49 PDT
We should probably expose this feature as a DOMWindowUtils method (like we did for bug 847890).
Comment 2 User image Paul Rouget [:paul] 2013-06-10 02:27:26 PDT
I talked to :nical, I think I have a good understanding of how to expose that to DOMWindowUtils.

It is required to start with a remote implementation (I'll probably do the same for paint flashing).
Comment 3 User image Paul Rouget [:paul] 2013-06-17 02:55:03 PDT
Created attachment 763467 [details] [diff] [review]

This patch exposes the methods to enable the layer borders from DOMUtils. It just sets a property in the `Layer` class for now.
Comment 4 User image Nicolas Silva [:nical] 2013-06-17 10:33:14 PDT
Created attachment 763658 [details] [diff] [review]
Expose layer borders and tile borders to devtools

I still have a few adjustments to make for the tile borders to work correctly, but most of the work is there.

the property to change is compositionDiagnosticFlags in nsIDOMWindowUtils which is a bitfield with:
- 0x1: enable layer borders
- 0x2: enable layer tile borders

The flags are forwarded per layers to the compositor threads.
Whenever we need some more debug tools for layers we will just have to add a bit to the bitfield and implement it on the compositor side.
Comment 5 User image Paul Rouget [:paul] 2013-06-17 13:18:25 PDT
gfx/layers/composite/ContainerLayerComposite.cpp needs to be updated too I guess.

See aManager->GetCompositor()->DrawDiagnostics(gfx::Color,...)
Comment 6 User image Paul Rouget [:paul] 2013-06-18 01:34:14 PDT
And you need to delete the declaration of sDrawLayerBorders in gfx/thebes/gfxPlatform.cpp.
Comment 7 User image Nicolas Silva [:nical] 2013-06-21 06:31:38 PDT
Created attachment 765901 [details] [diff] [review]
Expose layer diagnostic flags to devtools for layer and tile borders
Comment 8 User image Paul Rouget [:paul] 2013-06-21 07:53:40 PDT
Thanks a lot Nicolas for working on that! Now the modifications are live, no need to rebuild the layers, right?

Can you document how to use compositionDiagnosticFlags in nsIDOMWindowUtils.idl? (add a reference to `CompositionDiagnosticFlags` or copy the comments you add in CompositorTypes.h).

Also, it would great to have something like `getCompositionDiagnosticCapabilities()`.
From the devtools point of view, we need to know what we can use (layers are only available if OMTC and tiles only on mobile). It would return `3` for fennec and `1` on desktop if OMTC, `0` on desktop if no OMTC. (I can do it myself later if you want).

Also, before landing this code, I'd like to get a chance to try this from the devtools toolbox and see if it works as expected.
Comment 9 User image Paul Rouget [:paul] 2013-06-24 07:52:58 PDT
> gBrowser.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>   .getInterface(Ci.nsIDOMWindowUtils).compositionDiagnosticFlags = 1

I see borders around content and chrome (it should only show borders for content).
Comment 10 User image Benoit Girard (:BenWa) 2013-07-03 10:56:04 PDT
Comment on attachment 765901 [details] [diff] [review]
Expose layer diagnostic flags to devtools for layer and tile borders

Review of attachment 765901 [details] [diff] [review]:

Sorry for the long review. Looks good but I'd like to take a look with these fixed, follow up review will be faster.

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +1389,5 @@
>      */
>     attribute boolean paintFlashing;
>     /**
> +    * Flags to debug the behavior of layers.

Can you document this better. What are the possible values and what do they do exactly.

::: gfx/layers/Compositor.cpp
@@ +25,2 @@
>  {
> +  CompositionDiagnosticFlags flags = aFlags | mDiagnosticFlags;

I don't really like that you're mixing two different concepts here. The diagnostics options and the type of diagnostics that's being drawn. I think they should have their own types of be evaluated and compared separately.

::: gfx/layers/CompositorTypes.h
@@ +41,5 @@
> +typedef uint32_t CompositionDiagnosticFlags;
> +// Whether to render the border of a layer
> +const CompositionDiagnosticFlags EnableLayerBorders = 1 << 0;

Why not use an enum here?

::: gfx/layers/Layers.h
@@ +581,5 @@
>    static void InitLog();
>    static PRLogModuleInfo* sLog;
>    uint64_t mId;
>    bool mInTransaction;
> +  CompositionDiagnosticFlags mDiagnosticFlags;

I think this should be a property of either the compositor or the layer manager but not both.

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +160,5 @@
>      gfx::Rect rect(visibleRect.x, visibleRect.y, visibleRect.width, visibleRect.height);
>      gfx::Rect clipRect(aClipRect.x, aClipRect.y, aClipRect.width, aClipRect.height);
> +    aManager->GetCompositor()->DrawDiagnostics(rect, clipRect, transform,
> +                                               gfx::Point(aOffset.x, aOffset.y),
> +                                               IsContainer);

Can we handle scrollable continayer layer differently? This is meant to highlight an area that has an APZC while may not always have child layers.

::: gfx/layers/composite/ImageHost.cpp
@@ +97,5 @@
> +  IntSize textureSize = mTextureHost->GetSize();
> +  gfx::Rect gfxPictureRect
> +    = mHasPictureRect ? gfx::Rect(0, 0,
> +                                  mPictureRect.width,mPictureRect.height)

space after ','

::: gfx/thebes/gfxPlatform.cpp
@@ +525,5 @@
>  }
> +CompositionDiagnosticFlags gfxPlatform::GetCompositionDiagnosticFlags() const
> +{
> +    CompositionDiagnosticFlags flags = mEnableLayerBorders ? mozilla::layers::EnableLayerBorders : 0;

Do you mind adding a debug check that this is only called on the main thread? Just to be safe it doesn't get called from the compositor thread.

@@ +526,5 @@
> +CompositionDiagnosticFlags gfxPlatform::GetCompositionDiagnosticFlags() const
> +{
> +    CompositionDiagnosticFlags flags = mEnableLayerBorders ? mozilla::layers::EnableLayerBorders : 0;
> +    if (mEnableTileBorders) {

optional: If you wanted a nicer style you could do:
flags =
  (mEnableLayerBorders ? mozilla::layers::EnableLayerBorders : 0) |
  (mEnableTileBorders ? mozilla::layers::EnableTileBorders : 0);
Comment 11 User image David Baron :dbaron: ⌚️UTC-8 2014-06-03 02:34:12 PDT
It looks like a something similar to the patch above landed as .  Could that lead to further progress on exposing this to devtools?

Note You need to log in before you can comment on or make changes to this bug.