Bug 86255 – [chromium] Plumb --show-paint-rects to accelerated compositor

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shawn Singh]

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.

[Shawn Singh]

Created attachment 141506 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

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

[Shawn Singh]

Comment on attachment 141506 [details]
Patch


Good question, I'll make sure we don't need it before landing the fixed version.

[Shawn Singh]

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?

[James Robinson]

(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.

[Shawn Singh]

(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 =)

[Shawn Singh]

Created attachment 141530 [details]
Needs review again

The change was just significant enough that this needs another review. Sorry for the inconvenience =)

[James Robinson]

Comment on attachment 141530 [details]
Needs review again

R=me

[Nat Duca]

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.

[James Robinson]

(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?

[Nat Duca]

I thought there was a page/Settings flag for this, since we have one for eg repaint counter. Guess we dont, on closer inspection.

[Shawn Singh]

(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.

[Nat Duca]

+1 and sorry for the churn. I read this wrong at first.

[WebKit Review Bot]

Comment on attachment 141530 [details]
Needs review again

Clearing flags on attachment: 141530

Committed r116835: <http://trac.webkit.org/changeset/116835>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.