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

Dongseong Hwang
Reported 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.
Attachments
Patch (9.21 KB, patch)
2013-01-17 16:23 PST, Dongseong Hwang 		no flags
DetailsFormatted DiffDiff
Patch (10.25 KB, patch)
2013-01-22 21:39 PST, Dongseong Hwang 		no flags
DetailsFormatted DiffDiff
Patch (9.64 KB, patch)
2013-01-28 14:55 PST, Dongseong Hwang 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Dongseong Hwang
Comment 1 2013-01-17 16:23:57 PST
Created attachment 183308 [details] Patch
WebKit Review Bot
Comment 2 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
Noam Rosenthal
Comment 3 2013-01-18 10:31:06 PST
This is OK with me; Please make sure a WebKit2 owner signs off on this.
Benjamin Poulain
Comment 4 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.
Joseph Pecoraro
Comment 5 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.
Dongseong Hwang
Comment 6 2013-01-21 18:52:06 PST
(In reply to comment #5) > (In reply to comment #4) Thank for detailed explanation!
Dongseong Hwang
Comment 7 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.
Benjamin Poulain
Comment 8 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.
Dongseong Hwang
Comment 9 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?
Dongseong Hwang
Comment 10 2013-01-22 21:39:26 PST
Created attachment 184135 [details] Patch
Dongseong Hwang
Comment 11 2013-01-24 21:32:30 PST
Benjamin: ping
Noam Rosenthal
Comment 12 2013-01-25 01:39:13 PST
Comment on attachment 184135 [details] Patch This looks ok to me.
Dongseong Hwang
Comment 13 2013-01-28 14:55:50 PST
Created attachment 185073 [details] Patch
Benjamin Poulain
Comment 14 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.
WebKit Review Bot
Comment 15 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>
WebKit Review Bot
Comment 16 2013-01-28 20:33:38 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.