There are three patches (for now) to add useful support for visualizing debug rects: (1) this bug, adds webkit-side plumbing through WebSettingsImpl so that the command-line flag will work (2) chromium-side bug, will use the webkit-side plumbing to fully connect --show-paint-rects command switch to webkit. (3) hook the web inspector "paint rects" checkbox to this setting. - before this, it might be necessary to slightly refactor compositor settings, separating them into (a) settings that remain fixed for the lifetime of the LTH, and (b) settings that can change dynamically on the fly, with at most a simple "updateDynamicSettings()" or "setXXX()" call on the LTH.
Created attachment 141506 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 141506 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141506&action=review R=me - one question for you to consider before setting CQ. > Source/WebKit/chromium/public/WebSettings.h:111 > + virtual bool showPaintRects() const = 0; do we need this getter? most other settings don't have one and i have an allergy to unused getters (feels too much like writing a java bean or something) WebViewImpl can access its m_webSettings to talk to a WebSettingsImpl interface
Comment on attachment 141506 [details] Patch Good question, I'll make sure we don't need it before landing the fixed version.
James, I think the same situation applies to showFPSCounter and showPlatformLayerTree. Looking at the WebViewImpl::settings() accessor, I worry that using m_settings directly, may have some null pointer problems. The accessor actually creates the WebSettingsImpl if it did not already exist. One possible solution: when we want to access the showFPSCounter / showPlatformLayerTree / showPaintRect bools, do it like this: dynamic_cast<WebSettingsImpl*>( settings().get())->showFPSCounter That way we can still remove those virtual accessors. What do you think?
(In reply to comment #5) > James, I think the same situation applies to showFPSCounter and showPlatformLayerTree. > > Looking at the WebViewImpl::settings() accessor, I worry that using m_settings directly, may have some null pointer problems. The accessor actually creates the WebSettingsImpl if it did not already exist. > > > One possible solution: when we want to access the showFPSCounter / showPlatformLayerTree / showPaintRect bools, do it like this: > > dynamic_cast<WebSettingsImpl*>( settings().get())->showFPSCounter > > That way we can still remove those virtual accessors. What do you think? Woah, that sounds like a bad idea. You could wrap the lazy creation of m_webSettings in another helper if you want and have code in WebViewImpl use that.
(In reply to comment #6) > (In reply to comment #5) > > James, I think the same situation applies to showFPSCounter and showPlatformLayerTree. > > > > Looking at the WebViewImpl::settings() accessor, I worry that using m_settings directly, may have some null pointer problems. The accessor actually creates the WebSettingsImpl if it did not already exist. > > > > > > One possible solution: when we want to access the showFPSCounter / showPlatformLayerTree / showPaintRect bools, do it like this: > > > > dynamic_cast<WebSettingsImpl*>( settings().get())->showFPSCounter > > > > That way we can still remove those virtual accessors. What do you think? > > Woah, that sounds like a bad idea. You could wrap the lazy creation of m_webSettings in another helper if you want and have code in WebViewImpl use that. yeah, I should've thought of that =)
Created attachment 141530 [details] Needs review again The change was just significant enough that this needs another review. Sorry for the inconvenience =)
Comment on attachment 141530 [details] Needs review again R=me
Comment on attachment 141530 [details] Needs review again Why aren't we connecting the Debug paint rects flag to this? I'm skeptical of this change until I understand the answer to that.
(In reply to comment #10) > (From update of attachment 141530 [details]) > Why aren't we connecting the Debug paint rects flag to this? I'm skeptical of this change until I understand the answer to that. Which flag are you referring to?
I thought there was a page/Settings flag for this, since we have one for eg repaint counter. Guess we dont, on closer inspection.
(In reply to comment #12) > I thought there was a page/Settings flag for this, since we have one for eg repaint counter. Guess we dont, on closer inspection. Just discussed with Nat offline, I'd still like to land this even though it turned out to be not the direction we wanted to go. We thought this would be a good first-step towards hooking up this visualization to the inspector, but it turned out not to be =) Its still useful to have plumbed it through to the command line as well, though, no harm done IMHO.
+1 and sorry for the churn. I read this wrong at first.
Comment on attachment 141530 [details] Needs review again Clearing flags on attachment: 141530 Committed r116835: <http://trac.webkit.org/changeset/116835>
All reviewed patches have been landed. Closing bug.