[webkit-reviews] review denied: [Bug 91214] [EFL] Add WebMemorySampler feature. : [Attachment 158812] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Aug 16 08:51:55 PDT 2012
Gyuyoung Kim <gyuyoung.kim at samsung.com> has denied JungJik Lee
<jungjik.lee at samsung.com>'s request for review:
Bug 91214: [EFL] Add WebMemorySampler feature.
https://bugs.webkit.org/show_bug.cgi?id=91214
Attachment 158812: Patch
https://bugs.webkit.org/attachment.cgi?id=158812&action=review
------- Additional Comments from Gyuyoung Kim <gyuyoung.kim at samsung.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=158812&action=review
First of all, I'm not sure if we can support memory usage functionality as
APIs. Can't we support this functionality when we set environment variable ? In
addition, I think this patch needs some loves. So. I set r-. In addition,
> Source/JavaScriptCore/PlatformEfl.cmake:20
> +IF (ENABLE_MEMORY_SAMPLER)
ENABLE_XXX macros were removed to avoid duplicated #ifdef guard. However,
though MemoryStatics.cpp file is not guarded by #if ENABLE(XXX), I would like
to recommend to add the MemoryStatistics.cpp to existing
JavaScriptCore_SOURCES.
> Source/WTF/wtf/Platform.h:1152
> +#if PLATFORM(EFL)
We don't need to add this macro to here.
> Source/WebKit2/PlatformEfl.cmake:26
> + Shared/efl/WebMemorySamplerEfl.cpp
Please move this line to below block.
> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:24
> + *
Remove unneeded line.
> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:55
> +} ApplicationMemoryStats;
Duplicated struct name declaration ?
> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:63
> + return String();
Please add a new line after return.
> Source/WebKit2/Shared/efl/WebMemorySamplerEfl.cpp:66
> + fscanf(file, "%s", buffer);
Please add a new line.
> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:238
> + EINA_SAFETY_ON_NULL_RETURN(ewkContext);
EFL port has add an empty line after EINA_SAFETY_ON_NULL_RETURN macro.
> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:245
> + EINA_SAFETY_ON_NULL_RETURN(ewkContext);
ditto.
> Source/WebKit2/UIProcess/API/efl/ewk_context.h:139
> +* Start memory sampler.
Add a space before *.
> Source/WebKit2/UIProcess/API/efl/ewk_context.h:147
> +EAPI void ewk_context_memory_sampler_start(Ewk_Context* context, double
timer_interval);
Nit : Wrong '*' operator place.
> Source/WebKit2/UIProcess/API/efl/ewk_context.h:150
> +* Stop memory sampler.
ditto.
> Source/WebKit2/UIProcess/API/efl/ewk_context.h:154
> +EAPI void ewk_context_memory_sampler_stop(Ewk_Context* context);
Nit : Wrong '*' operator place.
> Source/cmake/OptionsEfl.cmake:55
> +SET(ENABLE_MEMORY_SAMPLER ON)
Please use WEBKIT_OPTION_DEFAULT_PORT_VALUE macro.
More information about the webkit-reviews
mailing list