930205 - --disable-dump-painting doesn't disable dump painting

Status: NEW

(Core :: Layout, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

The layout debug message at http://hg.mozilla.org/mozilla-central/annotate/5ef0ca4d1068/layout/base/nsLayoutDebugger.cpp#l166 causes endless debug spam in the console. It should be guarded behind some layout-specific debug flag so that it will only be printed when requested.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

A sample of the debug spam,

    XULTextBox 15B47D60(TextBox(label)(0)[value=Nightly]) bounds(3540,3630,2280,900) visible(0,0,0,0) componentAlpha(3540,3630,2220,900) clip()  layer=0A385868
    Opacity 09D20AD8(HTMLScroll(div)(-1)) bounds(6420,3570,7588,1020) visible(0,0,0,0) componentAlpha(6313,3510,7695,1140) clip() (opacity 0.540000) layer=09EB30F0
      Text 09D20DA0(Text(0)"Search or enter address") bounds(6313,3510,7695,1140) visible(0,0,0,0) componentAlpha(6313,3510,7695,1140) clip(6420,3570,50460,1020)  layer=0A385A70
    XULImage 09D23D78(ImageBox(image)(1)) bounds(57060,3600,1320,960) visible(0,0,0,0) componentAlpha(0,0,0,0) clip()  layer=0A385868
    XULImage 09D24230(ImageBox(image)(-1)) bounds(58560,3660,660,840) visible(0,0,0,0) componentAlpha(0,0,0,0) clip()  layer=0A385868
    XULImage 09D24B18(ImageBox(image)(-1)) bounds(59520,3660,840,840) visible(0,0,0,0) componentAlpha(0,0,0,0) clip()  layer=0A385868
  Opacity 09D17FE8(ImageBox(image)(-1)) bounds(300,3120,1920,1920) visible(0,0,0,0) componentAlpha(0,0,0,0) clip(0,0,85680,46440) (opacity 0.400000) layer=0A15AEE0
    BoxShadowOuter 09D17FE8(ImageBox(image)(-1)) bounds(300,3120,1920,1920) visible(0,0,0,0) componentAlpha(0,0,0,0) clip()  layer=15CF5478
    Background 09D17FE8(ImageBox(image)(-1)) bounds(360,3180,1800,1800) visible(0,0,0,0) componentAlpha(0,0,0,0) clip()  layer=15CF5478
    XULImage 09D17FE8(ImageBox(image)(-1)) bounds(360,3180,1800,1800) visible(0,0,0,0) componentAlpha(0,0,0,0) clip()  layer=15CF5478
Painting --- retained layer tree:
D3D10LayerManager (0xa41a2d8)
  D3D10ContainerLayer (0x9eb2280) [visible=< (x=0, y=0, w=1428, h=774); >] [metrics={ viewport=(x=0.000000, y=0.000000, w=1428.000000, h=774.000000) viewportScroll=(x=0.000000, y=0.000000) displayport=(x=0.000000, y=0.000000, w=0.000000, h=0.000000) scrollId=0 }]
    D3D10ThebesLayer (0xa15aee0) [visible=< (x=0, y=0, w=1428, h=87); >] [isFixedPosition anchor=0.000000,0.000000] [valid=< (x=0,
y=0, w=1428, h=87); >]
    D3D10ContainerLayer (0x9eb24e8) [clip=(x=0, y=0, w=1428, h=774)] [visible=< (x=0, y=87, w=1, h=686); >] [isFixedPosition anchor=0.000000,0.000000]
      D3D10ThebesLayer (0x9aeec00) [clip=(x=0, y=0, w=0, h=0)] [not visible] [isFixedPosition anchor=0.000000,0.000000]
      D3D10ColorLayer (0x15b05038) [visible=< (x=0, y=87, w=1, h=686); >] [isFixedPosition anchor=0.000000,0.000000] [color=rgba(26, 26, 26, 0.4)]
    D3D10ContainerLayer (0x9f98350) [clip=(x=1, y=87, w=1426, h=686)] [visible=< (x=1, y=87, w=1426, h=686); >] [opaqueContent] [isFixedPosition anchor=0.000000,0.000000]
      D3D10ThebesLayer (0x9b715a8) [transform=[ 1 0; 0 1; 1 87; ]] [visible=< (x=0, y=0, w=1426, h=686); >] [opaqueContent] [isFixedPosition anchor=0.000000,0.000000] [valid=< (x=0, y=0, w=1426, h=686); >]
    D3D10ContainerLayer (0x9eb29b8) [clip=(x=0, y=0, w=1428, h=774)] [visible=< (x=1427, y=87, w=1, h=686); >] [isFixedPosition anchor=0.000000,0.000000]
      D3D10ThebesLayer (0x9aef090) [clip=(x=0, y=0, w=0, h=0)] [not visible] [isFixedPosition anchor=0.000000,0.000000]
      D3D10ColorLayer (0x15b05408) [visible=< (x=1427, y=87, w=1, h=686); >] [isFixedPosition anchor=0.000000,0.000000] [color=rgba(26, 26, 26, 0.4)]
    D3D10ContainerLayer (0x9eb2c20) [clip=(x=0, y=0, w=1428, h=774)] [visible=< (x=0, y=773, w=1428, h=1); >] [isFixedPosition anchor=0.000000,0.000000]
      D3D10ThebesLayer (0x9aef2d8) [visible=< (x=0, y=773, w=1428, h=1); >] [isFixedPosition anchor=0.000000,0.000000] [valid=< (x=0, y=773, w=1428, h=1); >]

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

This only gets printed when -enable-dump-painting is set, but somehow it is getting set even though that is not in my .mozconfig.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

This is because of http://mxr.mozilla.org/mozilla-central/source/configure.in#6742. Trying --disable-dump-painting now, but if that doesn't work I'd like to separate MOZ_DEBUG from enabling MOZ_DUMP_PAINTING.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Kyle, how can configure.in be changed to respect --disable-dump-painting?

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

One option is to change the default value of MOZ_DUMP_PAINTING before calling MOZ_ARG_ENABLE_BOOL, although unless you introduce another variable that will also define MOZ_LAYERS_HAVE_LOG too.  Another option is to set MOZ_DUMP_PAINTING to something other than " " in the disable block of MOZ_ARG_ENABLE_BOOL and test for that explicitly, skipping the ifdef debug block if it is set to that value.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

https://tbpl.mozilla.org/?tree=Try&rev=3da385e7cce0

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 825964 [details] [diff] [review]
Patch

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

I assume you've tested this with --enable, --disable, and nothing in both debug and opt builds and confirmed it works.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

With --disable-dump-painting:
https://tbpl.mozilla.org/?tree=Try&rev=6da629e03572

With --enable-dump-painting:
https://tbpl.mozilla.org/?tree=Try&rev=3766131bc0cc

With nothing specified:
https://tbpl.mozilla.org/?tree=Try&rev=ba14bcf66030

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

Same as previous patch, but with #ifdef guards around the two assertions that referenced a function that is only available when MOZ_DUMP_PAINTING is defined.

Try push with --disable-dump-painting:
https://tbpl.mozilla.org/?tree=Try&rev=c434b2392ef8

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 826882 [details] [diff] [review]
Patch v1.1

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

::: layout/base/FrameLayerBuilder.cpp
@@ +3224,3 @@
>      NS_ASSERTION(AppUnitsPerDevPixel(cdi->mItem) == appUnitsPerDevPixel,
>                   "a thebes layer should contain items only at the same zoom");
> +#endif

This assertion should be on in every debug build. So the function should be defined in debug builds and in MOZ_DUMP_PAINTING builds.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 826882 [details] [diff] [review]
Patch v1.1

Ok, thanks. I'll fix this and reupload.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.2

Switching AppUnitsPerDevPixel to |#ifdef DEBUG| allowed me to remove the #ifdef guards around the NS_ASSERTIONS that I had added in the earlier patch.

Repushed to tryserver.

With --disable-dump-painting:
https://tbpl.mozilla.org/?tree=Try&rev=62957b113201

With --enable-dump-paiting:
https://tbpl.mozilla.org/?tree=Try&rev=4c1f5d5050c1

With no dump-painting option selected:
https://tbpl.mozilla.org/?tree=Try&rev=be811ca984b1