Bug 99660 - checkbox to toggle FPS counter in the inspector's settings
Summary: checkbox to toggle FPS counter in the inspector's settings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-17 16:54 PDT by egraether
Modified: 2012-11-13 21:53 PST (History)
22 users (show)

See Also:


Attachments
Patch (14.54 KB, patch)
2012-10-17 17:27 PDT, egraether
no flags Details | Formatted Diff | Diff
Patch (16.81 KB, patch)
2012-10-19 10:51 PDT, egraether
no flags Details | Formatted Diff | Diff
Patch (21.37 KB, patch)
2012-10-19 18:24 PDT, egraether
no flags Details | Formatted Diff | Diff
Patch (21.22 KB, patch)
2012-10-23 15:42 PDT, egraether
no flags Details | Formatted Diff | Diff
Patch (20.89 KB, patch)
2012-10-30 12:27 PDT, egraether
no flags Details | Formatted Diff | Diff
Patch (18.33 KB, patch)
2012-11-06 12:05 PST, egraether
no flags Details | Formatted Diff | Diff
Patch (18.62 KB, patch)
2012-11-08 17:31 PST, egraether
no flags Details | Formatted Diff | Diff
Patch (18.69 KB, patch)
2012-11-09 09:11 PST, egraether
no flags Details | Formatted Diff | Diff
Patch (20.30 KB, patch)
2012-11-09 16:02 PST, egraether
no flags Details | Formatted Diff | Diff
Patch (21.15 KB, patch)
2012-11-12 12:04 PST, egraether
no flags Details | Formatted Diff | Diff
Patch (21.14 KB, patch)
2012-11-13 17:59 PST, egraether
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description egraether 2012-10-17 16:54:49 PDT
toggle FPS counter option
Comment 1 egraether 2012-10-17 17:27:58 PDT
Created attachment 169310 [details]
Patch
Comment 2 WebKit Review Bot 2012-10-17 17:29:52 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 WebKit Review Bot 2012-10-17 17:43:52 PDT
Comment on attachment 169310 [details]
Patch

Attachment 169310 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14424298
Comment 4 egraether 2012-10-17 18:16:17 PDT
test does not pass, because of dependency on the associated chromium patch: https://codereview.chromium.org/11189037/
Comment 5 Build Bot 2012-10-17 19:06:36 PDT
Comment on attachment 169310 [details]
Patch

Attachment 169310 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14400643
Comment 6 Peter Beverloo (cr-android ews) 2012-10-17 19:33:38 PDT
Comment on attachment 169310 [details]
Patch

Attachment 169310 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14400649
Comment 7 Andrey Kosyakov 2012-10-19 01:27:03 PDT
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.
Comment 8 egraether 2012-10-19 10:51:00 PDT
Created attachment 169648 [details]
Patch
Comment 9 egraether 2012-10-19 10:59:58 PDT
(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.
Comment 10 WebKit Review Bot 2012-10-19 11:13:09 PDT
Comment on attachment 169648 [details]
Patch

Attachment 169648 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14465529
Comment 11 Peter Beverloo (cr-android ews) 2012-10-19 11:31:53 PDT
Comment on attachment 169648 [details]
Patch

Attachment 169648 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14455620
Comment 12 Peter Beverloo (cr-android ews) 2012-10-19 13:14:52 PDT
Comment on attachment 169648 [details]
Patch

Attachment 169648 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14460579
Comment 13 egraether 2012-10-19 18:24:17 PDT
Created attachment 169736 [details]
Patch
Comment 14 Peter Beverloo (cr-android ews) 2012-10-19 18:41:13 PDT
Comment on attachment 169736 [details]
Patch

Attachment 169736 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14456738
Comment 15 WebKit Review Bot 2012-10-19 19:30:54 PDT
Comment on attachment 169736 [details]
Patch

Attachment 169736 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14459739
Comment 16 Andrey Adaikin 2012-10-20 06:57:03 PDT
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);
Comment 17 Andrey Kosyakov 2012-10-22 01:48:28 PDT
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?
Comment 18 Dana Jansens 2012-10-23 08:19:57 PDT
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))?
Comment 19 egraether 2012-10-23 15:42:31 PDT
Created attachment 170253 [details]
Patch
Comment 20 egraether 2012-10-23 15:50:14 PDT
(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 21 egraether 2012-10-23 16:19:02 PDT
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.
Comment 22 egraether 2012-10-23 16:26:18 PDT
(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.
Comment 23 egraether 2012-10-23 16:29:48 PDT
(In reply to comment #18)
All these suggestions were also addressed in Patch #4 and explained in Comment #21. Thank you.
Comment 24 Peter Beverloo (cr-android ews) 2012-10-23 17:20:56 PDT
Comment on attachment 170253 [details]
Patch

Attachment 170253 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14500746
Comment 25 WebKit Review Bot 2012-10-23 17:25:48 PDT
Comment on attachment 170253 [details]
Patch

Attachment 170253 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14539060
Comment 26 egraether 2012-10-30 12:27:51 PDT
Created attachment 171500 [details]
Patch
Comment 27 egraether 2012-11-05 10:55:54 PST
@caseq, mind taking a look?

ews failures should resolve when http://codereview.chromium.org/11189037/ lands
Comment 28 Andrey Kosyakov 2012-11-06 06:34:43 PST
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;
Comment 29 egraether 2012-11-06 12:05:15 PST
Created attachment 172629 [details]
Patch
Comment 30 egraether 2012-11-06 12:11:37 PST
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.
Comment 31 egraether 2012-11-06 12:18:12 PST
(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
Comment 32 Andrey Kosyakov 2012-11-08 07:08:29 PST
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*
Comment 33 Nat Duca 2012-11-08 11:43:08 PST
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.
Comment 34 Andrey Kosyakov 2012-11-08 12:01:59 PST
(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?
Comment 35 egraether 2012-11-08 12:14:19 PST
(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?
Comment 36 Andrey Kosyakov 2012-11-08 12:26:24 PST
(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.
Comment 37 Nat Duca 2012-11-08 12:54:31 PST
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.
Comment 38 Nat Duca 2012-11-08 12:54:53 PST
Oh, derp derp derp. I can't keep up with y'all. :)
Comment 39 egraether 2012-11-08 17:31:02 PST
Created attachment 173158 [details]
Patch
Comment 40 egraether 2012-11-08 17:35:01 PST
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;
Comment 41 Andrey Kosyakov 2012-11-09 05:49:56 PST
Comment on attachment 173158 [details]
Patch

LGTM. Pavel?
Comment 42 Pavel Feldman 2012-11-09 06:12:19 PST
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.
Comment 43 egraether 2012-11-09 09:11:26 PST
Created attachment 173319 [details]
Patch
Comment 44 egraether 2012-11-09 09:21:54 PST
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.
Comment 45 WebKit Review Bot 2012-11-09 11:22:09 PST
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
Comment 46 Pavel Feldman 2012-11-09 13:30:12 PST
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.
Comment 47 egraether 2012-11-09 16:02:44 PST
Created attachment 173387 [details]
Patch
Comment 48 egraether 2012-11-09 16:08:20 PST
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
Comment 49 Pavel Feldman 2012-11-10 11:28:53 PST
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
Comment 50 egraether 2012-11-12 12:04:13 PST
Created attachment 173683 [details]
Patch
Comment 51 WebKit Review Bot 2012-11-13 02:25:55 PST
Comment on attachment 173683 [details]
Patch

Clearing flags on attachment: 173683

Committed r134391: <http://trac.webkit.org/changeset/134391>
Comment 52 WebKit Review Bot 2012-11-13 02:26:02 PST
All reviewed patches have been landed.  Closing bug.
Comment 53 Dimitri Glazkov (Google) 2012-11-13 09:08:59 PST
Reverted r134391 for reason:

Speculative rollout, trying to fix browser_tests failure.

Committed r134427: <http://trac.webkit.org/changeset/134427>
Comment 54 egraether 2012-11-13 17:59:00 PST
Created attachment 174041 [details]
Patch
Comment 55 egraether 2012-11-13 18:09:52 PST
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.
Comment 56 WebKit Review Bot 2012-11-13 21:53:28 PST
Comment on attachment 174041 [details]
Patch

Clearing flags on attachment: 174041

Committed r134542: <http://trac.webkit.org/changeset/134542>
Comment 57 WebKit Review Bot 2012-11-13 21:53:35 PST
All reviewed patches have been landed.  Closing bug.