Bug 86255

Summary: [chromium] Plumb --show-paint-rects to accelerated compositor
Product: WebKit Reporter: Shawn Singh <shawnsingh>
Component: Layout and RenderingAssignee: Shawn Singh <shawnsingh>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, fishd, jamesr, nduca, tkent+wkapi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Needs review again none

Description Shawn Singh 2012-05-11 14:58:28 PDT
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.
Comment 1 Shawn Singh 2012-05-11 15:16:06 PDT
Created attachment 141506 [details]
Patch
Comment 2 WebKit Review Bot 2012-05-11 15:21:08 PDT
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 3 James Robinson 2012-05-11 15:28:39 PDT
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 4 Shawn Singh 2012-05-11 15:31:25 PDT
Comment on attachment 141506 [details]
Patch


Good question, I'll make sure we don't need it before landing the fixed version.
Comment 5 Shawn Singh 2012-05-11 16:22:03 PDT
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?
Comment 6 James Robinson 2012-05-11 16:30:21 PDT
(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.
Comment 7 Shawn Singh 2012-05-11 16:31:37 PDT
(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 =)
Comment 8 Shawn Singh 2012-05-11 17:04:29 PDT
Created attachment 141530 [details]
Needs review again

The change was just significant enough that this needs another review. Sorry for the inconvenience =)
Comment 9 James Robinson 2012-05-11 17:10:38 PDT
Comment on attachment 141530 [details]
Needs review again

R=me
Comment 10 Nat Duca 2012-05-11 17:22:17 PDT
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.
Comment 11 James Robinson 2012-05-11 17:29:59 PDT
(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?
Comment 12 Nat Duca 2012-05-11 17:36:22 PDT
I thought there was a page/Settings flag for this, since we have one for eg repaint counter. Guess we dont, on closer inspection.
Comment 13 Shawn Singh 2012-05-11 17:43:43 PDT
(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.
Comment 14 Nat Duca 2012-05-11 17:44:31 PDT
+1 and sorry for the churn. I read this wrong at first.
Comment 15 WebKit Review Bot 2012-05-11 19:20:06 PDT
Comment on attachment 141530 [details]
Needs review again

Clearing flags on attachment: 141530

Committed r116835: <http://trac.webkit.org/changeset/116835>
Comment 16 WebKit Review Bot 2012-05-11 19:20:11 PDT
All reviewed patches have been landed.  Closing bug.