[webkit-reviews] review denied: [Bug 90277] Web Inspector: added DecodeImage and ResizeImage events to TimelineAgent : [Attachment 150164] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Sergey Rogulenko
<rogulenko at google.com>'s request for review:
Bug 90277: Web Inspector: added DecodeImage and ResizeImage events to
TimelineAgent
https://bugs.webkit.org/show_bug.cgi?id=90277

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

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


> Source/WebCore/ChangeLog:8
> +	   * inspector/InspectorInstrumentation.h:

Please explain what happens and why above this line.

> Source/WebCore/inspector/InspectorInstrumentation.h:843
> +    if (!hasFrontends())

Please use FAST_RETURN_IF_NO_FRONTENDS macro

> Source/WebCore/inspector/InspectorInstrumentation.h:845
> +    if (InspectorTimelineAgent::instance())

You can not include InspectorTimelineAgent.h in instrumentation, so this would
not work.

>> Source/WebCore/inspector/InspectorInstrumentation.h:846
>> +	    InspectorTimelineAgent::instance()->willDecodeImage(rect);
> 
> Can we make these instrumentation methods static? This will also let us keep
instance() private.

You need to call willDecodeImagImpl on the instrumentation instance here.

> Source/WebCore/inspector/TimelineRecordFactory.cpp:184
> +    data->setNumber("x", rect.x());

Please rename to createImageData and use in both cases.

> Source/WebCore/inspector/front-end/TimelineModel.js:53
> +    DecodeImage: "DecodeImage",

This will make the tests fail, you need to update expectations.

> Source/WebCore/inspector/front-end/TimelinePanel.js:133
> +    this._presentationModel.addFilter(new
WebInspector.TimelineCategoryFilter());

You should not mix style changes with semantics changes.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:76
> +    recordStyles[recordTypes.DecodeImage] = { title:
WebInspector.UIString("Decode Image"), category: categories["painting"] };

New strings should go to the English.lproj/localizedStrings.js as well.