[webkit-reviews] review denied: [Bug 55794] Web Inspector: Need new graphic icon for garbage collect button : [Attachment 84802] patch adding gc button with temporary graphic icon

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied  review:
Bug 55794: Web Inspector: Need new graphic icon for garbage collect button
https://bugs.webkit.org/show_bug.cgi?id=55794

Attachment 84802: patch adding gc button with temporary graphic icon
https://bugs.webkit.org/attachment.cgi?id=84802&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=84802&action=review

This looks good except for a bunch of nits and a lack of test. You could add
one into LayoutTests/inspector/timeline/(timeline-gc.html)? You can't use
timeline runner harness as is since your action is triggered by the front-end,
but you should do a manual timeline test. For that:

1. copy timeline-script-tab-1.html
2. nuke performActions
3. Test would look like:

function test() 
{
    InspectorTest.startTimeline(step1);

    function step1()
    {
	ProfilerAgent.collectGarbage(step2);
    }

    function step2()
    {
	InspectorTest.stopTimeline(step3);
	InspectorTest.printTimelineRecords();
    }

    function step3()
    {
	InspectorTest.printTimelineRecords();
	InspectorTest.completeTest();
    }
}

You will most likely end up with flaky parameters / entries in the timeline
dump, you can use InspectorTest.printTimelineRecords's filter / formatter as in
timeline-script-tag-1.html to resolve that.

> Source/WebCore/bindings/js/ScriptProfiler.cpp:34
> +#include "GCController.h"

Nit: mind alphabetic order.

> Source/WebCore/inspector/InspectorProfilerAgent.cpp:47
> +#include "ScriptController.h"

Alphabetic order please.

> Source/WebCore/inspector/InspectorProfilerAgent.cpp:48
> +#include "Frame.h"

Is this import needed?

> Source/WebCore/inspector/front-end/Images/garbageCollectButtonGlyph.png:1
> +‰PNG

What generated this patch? Usually webkit-patch, svn-patch or git diff --binary
results in image previews available in bugzilla.

> Source/WebCore/inspector/front-end/TimelinePanel.js:212
> +	   this.garbageCollectButton = new
WebInspector.StatusBarButton(WebInspector.UIString("Collect Garbage"),
"garbage-collect-status-bar-item");

You need to add this string to WebCore/English.lproj/localizedStrings.js (watch
out, utf-16 binary file).

> Source/WebCore/inspector/front-end/TimelinePanel.js:293
> +	ProfilerAgent.collectGarbage();

Weird indentation.