[Webkit-unassigned] [Bug 91214] [EFL] Add WebMemorySampler feature.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=91214





--- Comment #50 from JungJik Lee <jungjik.lee at samsung.com>  2012-08-29 09:53:18 PST ---
(From update of attachment 161218)
View in context: https://bugs.webkit.org/attachment.cgi?id=161218&action=review

>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:82
>> +static inline void appendMemoryStats(WebMemoryStatistics& webKitMemoryStats, const String& key, size_t value)
> 
> Statistics?
> 
> Why webKitMemoryStats and not just statistics... the class name already makes it clear what it is

done.

>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:86
>> +}
> 
> This methods seems a bit useless.

it makes lines in sampleWebKit function be shorten. :)

>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:88
>> +static ApplicationMemoryStats sampleApplicationMalloc()
> 
> is malloc really the best word? Especially given that this is not tied to the malloc call?
> 
> sampleMemoryAllocatedForApplication ?

done.

>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:93
>> +    FILE* fMemoryStatus = fopen(processPath, "r");
> 
> Why not statmFileDescriptor

done.

>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:97
>> +    applicationStats.totalProgramSize = getToken(fMemoryStatus).toInt();
> 
> nextToken seems like a better name

done.

>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:104
>> +    fclose(fMemoryStatus);
> 
> add a newline before fclose to make it easier to spot

done.

>> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:124
>> +WebMemoryStatistics WebMemorySampler::sampleWebKit() const
> 
> WebKit is the project name and the name of the API's. Maybe sampleWebRuntime() makes more sense?

I think so but mac port uses the same name and I could not find where they are using this function. so I could not change it by myself. :)

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:90
>> +        static bool memorySampler = false;
> 
> memory sampler doesn't sounds like a "booload" to me... sampleMemory?

MemorySampler is already using as a file and class name, so how about initializeMemorySampler?

>> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:91
>> +        static const char environMemorySampler[] = "MEMORY_SAMPLER";
> 
> environ? enviromentVariable? or ?

done! thanks your comments.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.