Bug 107198 – [TexMap] Enable debug borders and repaint counter via Settings.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dongseong Hwang]

Currently, if the environment variable [QT_]WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS
is set to 1, only CoordinatedBacking shows debug borders and repaint counter.

This patch makes the environment variable change Settings.

After Bug 105787, all backing stores and platform layers in TextureMapper can
show debug borders and repaint counter.

[Dongseong Hwang]

Created attachment 183308 [details]
Patch

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Noam Rosenthal]

This is OK with me; Please make sure a WebKit2 owner signs off on this.

[Benjamin Poulain]

Joe, is there any plan to drive this by the inspector? We may want to do things differently.

[Joseph Pecoraro]

(In reply to comment #4)
> Joe, is there any plan to drive this by the inspector? We may want to do things differently.

The Inspector protocol includes PageAgent.getCompositingBordersVisible and PageAgent.setCompositingBordersVisible to show / hide compositing borders and repaint counters on the page. The Inspector functions are checking / setting WebCore::Settings values. This patch sets those settings values for efl/qt/gtk ports, so they should work just fine alongside the inspector.

The Safari frontend makes use of this part of the protocol.

The OpenSource frontend currently does not include this functionality. Someone could add a button to the OpenSource frontend that makes use of these inspector protocol APIs to toggle showing compositing borders at run time through the inspector.

[Dongseong Hwang]

(In reply to comment #5)
> (In reply to comment #4)
Thank for detailed explanation!

[Dongseong Hwang]

As Bruno mentioned at https://bugs.webkit.org/show_bug.cgi?id=105787#c50
I'll remove QT_ prefix.

[Benjamin Poulain]

Comment on attachment 183308 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183308&action=review

> Source/WebKit/efl/ewk/ewk_view.cpp:842
> +    bool showDebugVisuals = String(getenv("WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS")) == "1";

This is quite overkill. If you care about start up time, you want to avoid creating a new String here.

Just use StringImpl's equal() functions here?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:3438
> +    bool showDebugVisuals = String(getenv("WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS")) == "1";

Ditto.

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:154
> +    bool showDebugVisuals = String(getenv("WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS")) == "1";

ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:261
> +        if (g_value_get_boolean(value))
> +            webkit_settings_set_draw_compositing_indicators(settings, g_value_get_boolean(value));

It would looks nicer to keep the value of g_value_get_boolean(value) instead of calling it twice.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:263
> +            bool showDebugVisuals = String(getenv("WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS")) == "1";

Ditto regarding mem allocations.

[Dongseong Hwang]

(In reply to comment #8)
> (From update of attachment 183308 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183308&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:842
> > +    bool showDebugVisuals = String(getenv("WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS")) == "1";
> 
> This is quite overkill. If you care about start up time, you want to avoid creating a new String here.
> 
> Just use StringImpl's equal() functions here?
> 

Thank you for review.

I'm not familiar to String in WebKit.
Could you check if I go right direction.

bool showDebugVisuals = equal(getenv("WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS"), AtomicString("1", AtomicString::ConstructFromLiteral).impl());

if above code is also inefficient, how about it.

const char* a = getenv("WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS");
bool showDebugVisuals = a && a[0] == '1';

if two options are not acceptable, could you advice me the right code?

[Dongseong Hwang]

Created attachment 184135 [details]
Patch

[Dongseong Hwang]

Benjamin: ping

[Noam Rosenthal]

Comment on attachment 184135 [details]
Patch

This looks ok to me.

[Dongseong Hwang]

Created attachment 185073 [details]
Patch

[Benjamin Poulain]

Comment on attachment 185073 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185073&action=review

In which patch are the getenv() removed from CoordinatedBackingStore?

Some comments, no need to fix those:

> Source/WebKit/efl/ewk/ewk_view.cpp:842
> +    char* debugVisualsEnvironment = getenv("WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS");

const char*

> Source/WebKit/gtk/webkit/webkitwebview.cpp:3439
> +    char* debugVisualsEnvironment = getenv("WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS");

Ditto.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:152
> +    char* debugVisualsEnvironment = getenv("WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS");

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:263
> +            char* debugVisualsEnvironment = getenv("WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS");

Ditto.

[WebKit Review Bot]

Comment on attachment 185073 [details]
Patch

Clearing flags on attachment: 185073

Committed r141040: <http://trac.webkit.org/changeset/141040>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.