Bug 107198

Summary: [TexMap] Enable debug borders and repaint counter via Settings.
Product: WebKit Reporter: Dongseong Hwang <dongseong.hwang>
Component: Layout and RenderingAssignee: Dongseong Hwang <dongseong.hwang>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, benjamin, bruno.abinader, cgarcia, cmarcelo, gustavo, gyuyoung.kim, joepeck, laszlo.gombos, menard, mrobinson, noam, ostap73, rakuco, vivek.vg, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 105787, 107910    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Dongseong Hwang 2013-01-17 16:19:24 PST
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.
Comment 1 Dongseong Hwang 2013-01-17 16:23:57 PST
Created attachment 183308 [details]
Patch
Comment 2 WebKit Review Bot 2013-01-17 16:26:41 PST
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
Comment 3 Noam Rosenthal 2013-01-18 10:31:06 PST
This is OK with me; Please make sure a WebKit2 owner signs off on this.
Comment 4 Benjamin Poulain 2013-01-18 18:43:26 PST
Joe, is there any plan to drive this by the inspector? We may want to do things differently.
Comment 5 Joseph Pecoraro 2013-01-21 12:12:25 PST
(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.
Comment 6 Dongseong Hwang 2013-01-21 18:52:06 PST
(In reply to comment #5)
> (In reply to comment #4)
Thank for detailed explanation!
Comment 7 Dongseong Hwang 2013-01-22 19:49:00 PST
As Bruno mentioned at https://bugs.webkit.org/show_bug.cgi?id=105787#c50
I'll remove QT_ prefix.
Comment 8 Benjamin Poulain 2013-01-22 19:52:54 PST
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.
Comment 9 Dongseong Hwang 2013-01-22 21:28:47 PST
(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?
Comment 10 Dongseong Hwang 2013-01-22 21:39:26 PST
Created attachment 184135 [details]
Patch
Comment 11 Dongseong Hwang 2013-01-24 21:32:30 PST
Benjamin: ping
Comment 12 Noam Rosenthal 2013-01-25 01:39:13 PST
Comment on attachment 184135 [details]
Patch

This looks ok to me.
Comment 13 Dongseong Hwang 2013-01-28 14:55:50 PST
Created attachment 185073 [details]
Patch
Comment 14 Benjamin Poulain 2013-01-28 15:13:03 PST
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.
Comment 15 WebKit Review Bot 2013-01-28 20:33:32 PST
Comment on attachment 185073 [details]
Patch

Clearing flags on attachment: 185073

Committed r141040: <http://trac.webkit.org/changeset/141040>
Comment 16 WebKit Review Bot 2013-01-28 20:33:38 PST
All reviewed patches have been landed.  Closing bug.