[webkit-reviews] review granted: [Bug 107183] [chromium] Add WebDiscardableMemory to platform : [Attachment 183306] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted Nat Duca
<nduca at chromium.org>'s request for review:
Bug 107183: [chromium] Add WebDiscardableMemory to platform
https://bugs.webkit.org/show_bug.cgi?id=107183

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=183306&action=review


I think a few of these APIs aren't needed, but Platform::allocateAndLock...()
and Platform/..../WebDiscardableMemory.h look good except for some typos and
formatting issues.

> Source/Platform/chromium/public/Platform.h:238
> +    // Determine if the underlying platform supports discardable memory
> +    virtual bool discardableMemorySupported() const { return false; }

I don't think this adds much value.  If the platform doesn't support
discardable memory, allocateAndLock...() will return 0 and I can't think of a
case where the caller would want to distinguish between the platform not
supporting discardable memory and the specific call failing.

> Source/Platform/chromium/public/Platform.h:244
> +    // Discardable memory is often based on an underlying kernel paging
system.
> +    // Thus, allocations below a certain size are often counterproductive.
This
> +    // returns the suggested allocation size in bytes for discardable memory

> +    // allocations.
> +    virtual size_t suggestedMinimumDiscardableMemoryAllocationSizeInBytes()
const { return 0; }

I'm not sure how this would be implemented in a way that's intelligent, since
the implementation of this function has no idea what the caller wants to do. 
If we want to set a hard floor the implementation can just return 0 from
allocateAndLock...() for certain sizes.

> Source/Platform/chromium/public/WebDiscardableMemory.h:37
> +    // Locks the memory, prevent it from being discarded. Once locked. you
may

i think conventionally these sort of block comments are placed before the class
declaration

> Source/Platform/chromium/public/WebDiscardableMemory.h:40
> +    // Lock() may return false, indicating that the underlying memory was

s/Lock()/lock()/

> Source/Platform/chromium/public/WebDiscardableMemory.h:53
> +    //	delete mem; // Make sure to destroy it. Its never going to come
back.

s/Its/It's/

> Source/Platform/chromium/public/WebDiscardableMemory.h:67
> +    // Unlock the memory so that it can be purged by the system. Must be
called
> +    // after every successful lock call.

is deleting the WebDiscardableMemory while locked valid?  If so, we should
document as such somewhere.