[webkit-reviews] review denied: [Bug 90277] Web Inspector: added DecodeImage and ResizeImage events to TimelineAgent : [Attachment 150916] 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 150916: Patch
https://bugs.webkit.org/attachment.cgi?id=150916&action=review

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


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

Please explain what has changed and why.

> Source/WebCore/inspector/InspectorInstrumentation.h:261
> +    static InspectorTimelineAgent* timelineAgentForOrphanEvents();

I thought this has already landed. Do you need to rebaseline?

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:43
> +#include "InspectorInstrumentation.h"

ditto

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:250
> +    pushCurrentRecord(TimelineRecordFactory::createPaintData(rect),
TimelineRecordType::Paint, true, frame, true);

ditto

> Source/WebCore/platform/graphics/GraphicsContext.cpp:494
> +   
InspectorInstrumentation::willPaintImage(FractionalLayoutRect(dest.location(),
FloatSize(tw, th)));

You should do no extra work in case there are no front-ends. I.e. you should
pass dest, tw and th into the method.

> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:87
>	       TRACE_EVENT("nonCachedResize",
const_cast<NativeImageSkia*>(this), 0);

I would either move this TRACE_EVENT into the instrumentation or make your
instrumentation be called from the macro.

> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:89
> +	       InspectorInstrumentation::willResizeImage();	    

See how TRACE_EVENT above differentiates between the cached and non-cached
resizes. You might want to do the same.