[webkit-reviews] review granted: [Bug 99660] checkbox to toggle FPS counter in the inspector's settings : [Attachment 173158] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has granted egraether at chromium.org's
request for review:
Bug 99660: checkbox to toggle FPS counter in the inspector's settings
https://bugs.webkit.org/show_bug.cgi?id=99660

Attachment 173158: Patch
https://bugs.webkit.org/attachment.cgi?id=173158&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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.