[webkit-reviews] review requested: [Bug 22214] Keep dead resources in memory cache in purgable memory. : [Attachment 25410] updated with Geoff's comments plus some other fixes

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has asked  for review:
Bug 22214: Keep dead resources in memory cache in purgable memory.
https://bugs.webkit.org/show_bug.cgi?id=22214

Attachment 25410: updated with Geoff's comments plus some other fixes
https://bugs.webkit.org/attachment.cgi?id=25410&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
- Implemented naming changes as discussed with Geoff in IRC. VolatileBuffer is
renamed to PurgableBuffer and word purgable generally replaces word volatile in
the API.
- Kept the buffer ownership scheme (as discussed with Geoff), clarified the API
with some comments and by removing purgableBuffer() accessor from SharedBuffer.

- Changed CachedImage to delete m_image (if it is safe) rather than just
deleting its decoded data when turning buffer volatile. The Image object would
still hold a reference of the old non-purgable data buffer.
- Changed CachedScript to turn the buffer purgable asynchronously instead of
synchronously in allClientsRemoved(). This is safer.
- Added more asserts to guarantee consistent state.

> +bool CachedResource::tryMakeVolatile() 
> Looks like the bool result here is unused. Let's return void instead, to
avoid
> bitrot.

I kept the return values as a way of documenting which exits will actually
results in volatile buffer. I think it is helpful even though current call
sited don't check the return value.

>+    const size_t volatileThreshold = 4 * 4096;
>
>Since this is cross-platform code, it might be worth commenting that 4096 is
>the page size on Mac OS X.

Added a comment. It is really more like the "page size of the CPUs most likely
to run WebKit", not just Mac.

>+SharedBuffer::~SharedBuffer()
>+{
>+}
>Can this be removed?

It was needed so I can forward declare PugrableBuffer in SharedBuffer header.
SharedBuffer is included by half of the WebCore.

>+bool CachedResource::tryMakeVolatile() 
>+{
>Would be helpful to ASSERT(!hasClients()) at the top of this function.

Done.

Requesting re-review since there were some non-trivial changes.