877567 - Add a tool to toggle the display of layer borders

Status: RESOLVED INACTIVE

(DevTools :: General, defect)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

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

[Paul Rouget [:paul]]

We should probably expose this feature as a DOMWindowUtils method (like we did for bug 847890).

Comment 2

[Paul Rouget [:paul]]

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

[Paul Rouget [:paul]]

Attachment: WIP

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

[Nicolas Silva [:nical]]

Attachment: 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

[Paul Rouget [:paul]]

gfx/layers/composite/ContainerLayerComposite.cpp needs to be updated too I guess.

See aManager->GetCompositor()->DrawDiagnostics(gfx::Color,...)

Comment 6

[Paul Rouget [:paul]]

And you need to delete the declaration of sDrawLayerBorders in gfx/thebes/gfxPlatform.cpp.

Comment 7

[Nicolas Silva [:nical]]

Attachment: Expose layer diagnostic flags to devtools for layer and tile borders

Comment 8

[Paul Rouget [:paul]]

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

[Paul Rouget [:paul]]

> 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

[Benoit Girard (:BenWa)]

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

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It looks like a something similar to the patch above landed as https://hg.mozilla.org/mozilla-central/rev/78c0cf01f34f .  Could that lead to further progress on exposing this to devtools?