[webkit-reviews] review granted: [Bug 181401] ASSERTION FAILED: m_ptr under WebKit::CacheStorage::Caches::writeRecord : [Attachment 331253] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 181401: ASSERTION FAILED: m_ptr under
WebKit::CacheStorage::Caches::writeRecord
https://bugs.webkit.org/show_bug.cgi?id=181401

Attachment 331253: Patch

https://bugs.webkit.org/attachment.cgi?id=331253&action=review




--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 331253
  --> https://bugs.webkit.org/attachment.cgi?id=331253
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=331253&action=review

> Source/WebKit/ChangeLog:16
> +	   clearMemoryRepresentation is about cleaning the in-memory
information of the caches, and
> +	   nullifying m_storage is a memory consumption optimization.

The comment doesn’t make this clear and I thin it needs to. I think there are
three points we want to make clear in the code, not just the change log,
although I hope we can still do it with a fairly short comment:

    1) this "m_storage" part of cleaning is optional and is about memory use
optimization, not required for correct behavior

    2) this "m_storage" part of cleaning must not be done during initialization
-- this is what the comment currently tries to say, but saying "let's keep it
around" is not right; we need to say "we must keep it around"

    3) this "m_storage" part of cleaning is safe to do any time *other* than
initialization

So I suggest you improve the comment -- ideally we cleverly come up with a
short comment that makes all three of these things clear. Also possibly might
want to add a blank line to make this code a separate paragraph so it’s not
grouped as closely with code implementing things that are required because of
point (1).