Bug 99660 – checkbox to toggle FPS counter in the inspector's settings

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

toggle FPS counter option

Created attachment 169310 [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.

[WebKit Review Bot]

Comment on attachment 169310 [details]
Patch

Attachment 169310 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14424298

test does not pass, because of dependency on the associated chromium patch: https://codereview.chromium.org/11189037/

[Build Bot]

Comment on attachment 169310 [details]
Patch

Attachment 169310 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14400643

[Peter Beverloo (cr-android ews)]

Comment on attachment 169310 [details]
Patch

Attachment 169310 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14400649

[Andrey Kosyakov]

Comment on attachment 169310 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169310&action=review

> Source/WebCore/page/ChromeClient.h:367
> +        virtual void setShowFPSCounter(bool show) { }

Given this is only called by inspector, I think it would be better to plumb it through InspectorClient.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3939
> +            // NOTE: disabled for 'show FPS counter' option in WebInspector
> +            // if (layerTreeViewSettings.showFPSCounter || layerTreeViewSettings.showPlatformLayerTree) {

Please remove the code instead of commenting it out.

Created attachment 169648 [details]
Patch

(In reply to comment #7)
> (From update of attachment 169310 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169310&action=review
> 
> > Source/WebCore/page/ChromeClient.h:367
> > +        virtual void setShowFPSCounter(bool show) { }
> 
> Given this is only called by inspector, I think it would be better to plumb it through InspectorClient.

I will have a look at this.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:3939
> > +            // NOTE: disabled for 'show FPS counter' option in WebInspector
> > +            // if (layerTreeViewSettings.showFPSCounter || layerTreeViewSettings.showPlatformLayerTree) {
> 
> Please remove the code instead of commenting it out.

I agree, this was just temporary. It is solved in a different way in the new Patch, assuring that the fontAtlas is only loaded when used.

Thank you for your remarks.

[WebKit Review Bot]

Comment on attachment 169648 [details]
Patch

Attachment 169648 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14465529

[Peter Beverloo (cr-android ews)]

Comment on attachment 169648 [details]
Patch

Attachment 169648 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14455620

[Peter Beverloo (cr-android ews)]

Comment on attachment 169648 [details]
Patch

Attachment 169648 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14460579

Created attachment 169736 [details]
Patch

[Peter Beverloo (cr-android ews)]

Comment on attachment 169736 [details]
Patch

Attachment 169736 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14456738

[WebKit Review Bot]

Comment on attachment 169736 [details]
Patch

Attachment 169736 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14459739

[Andrey Adaikin]

Comment on attachment 169736 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169736&action=review

> Source/WebCore/inspector/InspectorPageAgent.cpp:724
> +    m_state->setBoolean(PageAgentState::showFPSCounter, show);

should we restore this value in InspectorPageAgent::restore() ?

> Source/WebKit/chromium/src/WebViewImpl.cpp:854
> +#endif

#else
    UNUSED_PARAM(show);

[Andrey Kosyakov]

Comment on attachment 169736 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169736&action=review

> Source/WebCore/inspector/front-end/Settings.js:61
> +    isAcceleratedCompositingActive: false

is that a setting?

> Source/WebCore/inspector/front-end/inspector.js:370
> +    PageAgent.isAcceleratedCompositingActive(WebInspector._initializeCapability.bind(WebInspector, "isAcceleratedCompositingActive", null));

I don't think it's appropriate use for capabilities -- a capability is seen a constant property of particular combination of embedder, platform and enviroment, not something that may vary from page to page or even within a page lifetime. You probably want to check the platform supports accelerated compositing and can display FPS overlay.

> Source/WebCore/inspector/front-end/inspector.js:481
> +    else if (Capabilities.showFPSCounter)
> +        WebInspector.settings.showFPSCounter.set(true);

Could you explain the intent behind these lines?

> Source/WebKit/chromium/src/InspectorClientImpl.cpp:200
> +bool InspectorClientImpl::isAcceleratedCompositingActive()
> +{
> +    return m_inspectedWebView->isAcceleratedCompositingActive(); 
> +}

So we know whether the accelerated compositing is active at some point in time. What if it changes later?

[Dana Jansens]

Comment on attachment 169736 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169736&action=review

> Source/WebKit/chromium/src/WebViewImpl.cpp:839
> +bool WebViewImpl::getShowFPSCounter()

name this "showFPSCounter()"

> Source/WebKit/chromium/src/WebViewImpl.cpp:846
> +#if USE(ACCELERATED_COMPOSITING)

chromium code always has ACCELERATED_COMPOSITING enabled, so this is not needed here.

> Source/WebKit/chromium/src/WebViewImpl.cpp:852
> +        m_layerTreeView->setShowFPSCounter(show);
> +        if (show)
> +            loadFontAtlas();

Can we loadFontAtlas() first so we can DCHECK that the atlas exists when turning on the FPS counter?

> Source/WebKit/chromium/src/WebViewImpl.cpp:3953
> +            if (settingsImpl()->showFPSCounter())
> +                setShowFPSCounter(true);

Why is this here? If settingsImpl()->showFPSCounter() is true, doesn't that mean that setShowFPSCounter(true) was already called (since it is calling settingsImpl()->setShowFPSCounter(show))?

Created attachment 170253 [details]
Patch

(In reply to comment #16)
> (From update of attachment 169736 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169736&action=review
> 
> > Source/WebCore/inspector/InspectorPageAgent.cpp:724
> > +    m_state->setBoolean(PageAgentState::showFPSCounter, show);
> 
> should we restore this value in InspectorPageAgent::restore() ?

I removed this line again. It was there because I copied the implementation of the 'showPaintRects' setting. I could not track down in which cases InspectorPageAgent::restore() is called. Is there a reason to save and restore 'showFPSCounter'?

> > Source/WebKit/chromium/src/WebViewImpl.cpp:854
> > +#endif
> 
> #else
>     UNUSED_PARAM(show);

This change was not necessary. This condition now contains only the call to 'loadFontAtlas()', because this method was declared under the same condition in WebViewImpl.h

thank you

Comment on attachment 170253 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170253&action=review

> Source/WebCore/inspector/InspectorPageAgent.cpp:716
> +void InspectorPageAgent::syncShowFPSCounter(ErrorString* errorString, bool inParam, bool* outParam)

This method synchronizes the 'showFPSCounter' setting between inspector and compositor. It is called during the setup of the inspector. The 'inParam' is true if 'showFPSCounter' was activated with the inspector in the last session and got saved to localStorage with value 'true'.

> Source/WebCore/inspector/InspectorPageAgent.cpp:718
> +    *outParam = inParam || m_client->isShowFPSCounterFlagSet();

Setting the outParam assures that the checkbox in the inspector shows 'checked' in case 'showFPSCounter' was activated with the command-line flag.

> Source/WebCore/inspector/InspectorPageAgent.cpp:719
> +    if (*outParam)

This condition assures that fontAtlas and HUDLayer are only created when needed.

> Source/WebCore/inspector/front-end/Settings.js:60
> +    isForceCompositingModeEnabled: false

Due to remarks of reviewers the checkbox for 'showFPSCounter' now only shows up if 'force compositing mode' is enabled. This value does not change during a session and as it is loaded asynchronous I found it is the cleanest way to put it here. Please let know if there are better places.

> Source/WebKit/chromium/src/WebViewImpl.cpp:844
> +    return settingsImpl()->showFPSCounter();

At the moment I read the command-line flag from the settings of the 'WebViewImpl'. I could not find an easy way to read it from the command-line flags directly.

> Source/WebKit/chromium/src/WebViewImpl.cpp:851
> +#if USE(ACCELERATED_COMPOSITING)

This condition is here, because 'loadFontAtlas()' was declared within the same condition in 'WebViewImpl.h', so this avoids possible build breaks.

(In reply to comment #17)
I addressed your concerns in Patch #4 and explained my revised implementation in Comment #21. Thank you for your remarks.

(In reply to comment #18)
All these suggestions were also addressed in Patch #4 and explained in Comment #21. Thank you.

[Peter Beverloo (cr-android ews)]

Comment on attachment 170253 [details]
Patch

Attachment 170253 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14500746

[WebKit Review Bot]

Comment on attachment 170253 [details]
Patch

Attachment 170253 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14539060

Created attachment 171500 [details]
Patch

@caseq, mind taking a look?

ews failures should resolve when http://codereview.chromium.org/11189037/ lands

[Andrey Kosyakov]

Comment on attachment 171500 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171500&action=review

> Source/WebCore/inspector/Inspector.json:342
> +                "name": "syncShowFPSCounter",
> +                "description": "Synchronizes the showFPSCounter setting with the backend",

I think we're going to get rid of the about:flags flag for the FPS counter eventually, so let's not expose the complexity of syncing different values for the option into the inspector protocol. How about just ORing the values of the command line flag with the value of inspector option?

> Source/WebCore/inspector/Inspector.json:444
> +                "name": "isForceCompositingModeEnabled",

I think the conditions of whether we can show FPS counter may vary per embedder, so hard-coding the limitations of chromium implementation into front-end and adding protocol methods to support this does not seem fair. Can we instead expose the capability, i.e. canShowFPSCounter() and ignore the fact that chromium can only do this while accelerated compositing is on, perhaps just add a warning text to the option in the UI?

> Source/WebCore/inspector/InspectorClient.h:82
> +    virtual bool isShowFPSCounterFlagSet() { return false; }

I suggest we drop this.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3988
> +void WebViewImpl::loadFontAtlas()

nit: loadFontAtlasIfNecessary()

> Source/WebKit/chromium/src/WebViewImpl.cpp:3991
> +    if (!m_isFontAtlasLoaded) {

nit: we prefer early bail-outs, i.e.
if (m_isFontAtlasLoaded)
    return;

Created attachment 172629 [details]
Patch

Comment on attachment 172629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172629&action=review

> Source/WebCore/inspector/InspectorPageAgent.cpp:722
> +    *outParam = m_page->settings()->forceCompositingMode();

I renamed the capability check to canShowFPSCounter, but I'm not yet sure where to put the check for compositing mode best, so that it fits chromium and works for other embedders.

(In reply to comment #28)
> (From update of attachment 171500 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171500&action=review
> 
> > Source/WebCore/inspector/Inspector.json:342
> > +                "name": "syncShowFPSCounter",
> > +                "description": "Synchronizes the showFPSCounter setting with the backend",
> 
> I think we're going to get rid of the about:flags flag for the FPS counter eventually, so let's not expose the complexity of syncing different values for the option into the inspector protocol. How about just ORing the values of the command line flag with the value of inspector option?

I removed the syncing.

> > Source/WebCore/inspector/Inspector.json:444
> > +                "name": "isForceCompositingModeEnabled",
> 
> I think the conditions of whether we can show FPS counter may vary per embedder, so hard-coding the limitations of chromium implementation into front-end and adding protocol methods to support this does not seem fair. Can we instead expose the capability, i.e. canShowFPSCounter() and ignore the fact that chromium can only do this while accelerated compositing is on, perhaps just add a warning text to the option in the UI?

I did the renaming and put a comment about this in the new patch.

> > Source/WebCore/inspector/InspectorClient.h:82
> > +    virtual bool isShowFPSCounterFlagSet() { return false; }
> 
> I suggest we drop this.

dropped, it was used in the syncing.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:3988
> > +void WebViewImpl::loadFontAtlas()
> 
> nit: loadFontAtlasIfNecessary()
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:3991
> > +    if (!m_isFontAtlasLoaded) {
> 
> nit: we prefer early bail-outs, i.e.
> if (m_isFontAtlasLoaded)
>     return;

both nits implemented

please have a look at the new patch, thanks

[Andrey Kosyakov]

Comment on attachment 172629 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172629&action=review

>> Source/WebCore/inspector/InspectorPageAgent.cpp:722
>> +    *outParam = m_page->settings()->forceCompositingMode();
> 
> I renamed the capability check to canShowFPSCounter, but I'm not yet sure where to put the check for compositing mode best, so that it fits chromium and works for other embedders.

Do you think we should only support FPS counters if compositing is forced? I thought that would be almost never for the majority of the users. So my ideas was that we perhaps should always show the checkbos for chrome, but only honor it when in accelerated compositing (whether forced or not).

> Source/WebCore/inspector/InspectorPageAgent.h:113
> +    virtual void canShowFPSCounter(ErrorString*, bool* outParam);

drop outParam in declaration, just bool*

[Nat Duca]

I think making the fps counter chrome only makes sense.

But, I think that its wasted effort to make the fps counter work when we are is software mode. We would have to make a page overlay version of it, and do custom plumbing for fps for the software mode. And, it is our plan to end-of-life software mode EVERYWHERE. As in, chrome will only EVER use force compositing mode. This is already the case on windows canary, android and cros. Linux and MacOS will follow suit in 6 months, I expect.

[Andrey Kosyakov]

(In reply to comment #33)
> I think making the fps counter chrome only makes sense.

I'm not suggesting we implement it on any other platform, but given that part of it is in WebCore, it would be fair to assume other platforms may (eventually) implement it. Hence we just check the capability, and the platform returns whether it can do it or not.

> But, I think that its wasted effort to make the fps counter work when we are is software mode.

I'm not suggesting this either, in my view it's just a matter of whether we disable the checkbox if compositing is not forced or just ignore its value if compositing is not used at the moment.

> And, it is our plan to end-of-life software mode EVERYWHERE. As in, chrome will only EVER use force compositing mode.

Just out of curiosity, what about those with black-listed GPUs?

(In reply to comment #34)
> (In reply to comment #33)
> > I think making the fps counter chrome only makes sense.
> 
> I'm not suggesting we implement it on any other platform, but given that part of it is in WebCore, it would be fair to assume other platforms may (eventually) implement it. Hence we just check the capability, and the platform returns whether it can do it or not.

I agree. So I just plumb the canShowFPSCounter request to InspectorClient and InspectorClientImpl returns the force compositing mode flag then?

[Andrey Kosyakov]

(In reply to comment #35)
> (In reply to comment #34)
> > (In reply to comment #33)
> > > I think making the fps counter chrome only makes sense.
> > 
> > I'm not suggesting we implement it on any other platform, but given that part of it is in WebCore, it would be fair to assume other platforms may (eventually) implement it. Hence we just check the capability, and the platform returns whether it can do it or not.
> 
> I agree. So I just plumb the canShowFPSCounter request to InspectorClient and InspectorClientImpl returns the force compositing mode flag then?

That's certainly an option, but I was rather thinking of always returning true for chrome, so that we still let those that have compositing on some pages but don't have it forced use it whenever compositing is active. There probably wob't be any difference once, as Nat says, compositing would be forced everywhere.

[Nat Duca]

Have you considered plumbing for the frontend to say to the agent "do you support showing an fps counter"?


> I'm not suggesting this either, in my view it's just a matter of whether we disable the checkbox if compositing is not forced or just ignore its value if compositing is not used at the moment.
I'm totally fine ignoring its value. That probably wont upset people too much...


> Just out of curiosity, what about those with black-listed GPUs?
We're going to have a software fallback to force compositing mode, 124671. Force compositing mode will be on but we render the compositing tree with software.

[Nat Duca]

Oh, derp derp derp. I can't keep up with y'all. :)

Created attachment 173158 [details]
Patch

Comment on attachment 173158 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173158&action=review

> Source/WebCore/inspector/InspectorClient.h:83
> +    virtual bool canShowFPSCounter() { return false; }

Made false the default.

> Source/WebCore/inspector/InspectorPageAgent.h:113
> +    virtual void canShowFPSCounter(ErrorString*, bool*);

Dropped the outParam.

> Source/WebKit/chromium/src/InspectorClientImpl.cpp:205
> +    return true;

Chromium returns true;

[Andrey Kosyakov]

Comment on attachment 173158 [details]
Patch

LGTM. Pavel?

[Pavel Feldman]

Comment on attachment 173158 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173158&action=review

Please fix nits prior to landing.

> Source/WebCore/inspector/Inspector.json:351
> +                    { "name": "result", "type": "boolean", "description": "True for showing the FPS counter" }

I do understand that you copied the one above, but we should not call input parameter "result", it should be "enabled" or "show".

>> Source/WebCore/inspector/InspectorClient.h:83
>> +    virtual bool canShowFPSCounter() { return false; }
> 
> Made false the default.

I would follow the pattern above and reverse the order (canShow, then show). You should also move these above to after overrideDeviceMetrics

>> Source/WebCore/inspector/InspectorPageAgent.h:113
>> +    virtual void canShowFPSCounter(ErrorString*, bool*);
> 
> Dropped the outParam.

Also reverse order.

> Source/WebKit/chromium/src/InspectorClientImpl.h:86
> +    virtual bool canShowFPSCounter();

reverse

> Source/WebKit/chromium/src/WebViewImpl.cpp:4030
> +void WebViewImpl::loadFontAtlasIfNecessary()

The order of declaration should match the order of definition.

Created attachment 173319 [details]
Patch

Comment on attachment 173319 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173319&action=review

resolved the nits:

> Source/WebCore/inspector/Inspector.json:351
> +                    { "name": "show", "type": "boolean", "description": "True if the FPS count can be shown" }

reordered and renamed return values

> Source/WebCore/inspector/InspectorClient.h:75
> +    virtual bool canShowFPSCounter() { return false; }

reordered and moved up

> Source/WebCore/inspector/InspectorPageAgent.cpp:714
> +void InspectorPageAgent::canShowFPSCounter(ErrorString*, bool* outParam)

reordered

> Source/WebCore/inspector/InspectorPageAgent.h:112
> +    virtual void canShowFPSCounter(ErrorString*, bool*);

reordered

> Source/WebKit/chromium/src/InspectorClientImpl.cpp:166
> +bool InspectorClientImpl::canShowFPSCounter()

reordered and moved up

> Source/WebKit/chromium/src/InspectorClientImpl.h:80
> +    virtual bool canShowFPSCounter();

reordered and moved up

> Source/WebKit/chromium/src/WebViewImpl.h:654
> +    void loadFontAtlasIfNecessary();

The declaration and definition order is a bit chaotic in WebViewImpl. I moved the declaration right after setIsAcceleratedCompositingActive() to match the definition.

[WebKit Review Bot]

Comment on attachment 173319 [details]
Patch

Rejecting attachment 173319 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/Platform/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/14770709

[Pavel Feldman]

Comment on attachment 173319 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173319&action=review

> Source/WebCore/inspector/InspectorPageAgent.cpp:721
> +    m_client->setShowFPSCounter(show);

Ok, now that I look at it, I can see that there are two important pieces missing. Sorry for not spotting them earlier:

1) You need to m_client->setShowFPSCounter(false) from disable(ErrorString).
It is necessary to remove the FPS counter when inspector is closed. No inspector settings should affect running page when inspector is closed.

2) You need to store enabled state in the m_state via m_state->setBoolean(PageAgentState::showFPSCounter, show) here. 
Then, in enable(ErrorString) you should check it via m_state->getBoolean and in case it is true, you should m_client->setShowFPSCounter(true);

This code path will be used in case when navigation changes renderer process. You basically store your settings in the m_state cookie that is then passed into the restore of the other renderer renderer. 

You can follow the setScriptExecutionDisabled and mimic its behavior.

Created attachment 173387 [details]
Patch

Comment on attachment 173387 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173387&action=review

I thought it was nice that the FPS counter stays, but I understand why it should get disabled.
Thank you very much for the instructions.

> Source/WebCore/inspector/InspectorPageAgent.cpp:370
> +    bool showFPSCounter = m_state->getBoolean(PageAgentState::pageAgentShowFPSCounter);

restoring the setting

> Source/WebCore/inspector/InspectorPageAgent.cpp:381
> +    setShowFPSCounter(0, false);

disabling the setting

> Source/WebCore/inspector/InspectorPageAgent.cpp:725
> +    m_state->setBoolean(PageAgentState::pageAgentShowFPSCounter, show);

saving the setting

[Pavel Feldman]

Comment on attachment 173387 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173387&action=review

> Source/WebCore/inspector/front-end/SettingsScreen.js:277
> +        p.appendChild(this._createCheckboxSetting(WebInspector.UIString("Show FPS meter"), WebInspector.settings.showFPSCounter));

One last nit prior to landing: please add this new string into WebCore/English.lproj/localizedStrings.js

Created attachment 173683 [details]
Patch

[WebKit Review Bot]

Comment on attachment 173683 [details]
Patch

Clearing flags on attachment: 173683

Committed r134391: <http://trac.webkit.org/changeset/134391>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dimitri Glazkov (Google)]

Reverted r134391 for reason:

Speculative rollout, trying to fix browser_tests failure.

Committed r134427: <http://trac.webkit.org/changeset/134427>

Created attachment 174041 [details]
Patch

Comment on attachment 174041 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174041&action=review

I'm sorry for the inconvenience.

> Source/WebCore/inspector/InspectorPageAgent.cpp:728
> +    if (mainFrame() && mainFrame()->view())

This condition fixes the failing Chromium tests.

[WebKit Review Bot]

Comment on attachment 174041 [details]
Patch

Clearing flags on attachment: 174041

Committed r134542: <http://trac.webkit.org/changeset/134542>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.