Summary: | [TexMap] Enable debug borders and repaint counter via Settings. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dongseong Hwang <dongseong.hwang> | ||||||||
Component: | Layout and Rendering | Assignee: | 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
Dongseong Hwang
2013-01-17 16:19:24 PST
Created attachment 183308 [details]
Patch
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 This is OK with me; Please make sure a WebKit2 owner signs off on this. Joe, is there any plan to drive this by the inspector? We may want to do things differently. (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. (In reply to comment #5) > (In reply to comment #4) Thank for detailed explanation! As Bruno mentioned at https://bugs.webkit.org/show_bug.cgi?id=105787#c50 I'll remove QT_ prefix. 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. (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? Created attachment 184135 [details]
Patch
Benjamin: ping Comment on attachment 184135 [details]
Patch
This looks ok to me.
Created attachment 185073 [details]
Patch
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 on attachment 185073 [details] Patch Clearing flags on attachment: 185073 Committed r141040: <http://trac.webkit.org/changeset/141040> All reviewed patches have been landed. Closing bug. |