[webkit-reviews] review granted: [Bug 199302] ASSERT that a sessionID is valid when encoding it : [Attachment 373074] 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 199302: ASSERT that a sessionID is valid when encoding it
https://bugs.webkit.org/show_bug.cgi?id=199302

Attachment 373074: Patch

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




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

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

> Source/WebCore/Modules/indexeddb/IDBValue.h:57
> -    const PAL::SessionID& sessionID() const { return m_sessionID; }
> +    const PAL::SessionID& sessionID() const
> +    {
> +	   // FIXME: We should assert m_sessionID is valid or remove
m_sessionID.
> +	   return m_sessionID;
> +    }

When changing from a trivial to a multi-line inline function, I think it’s
better style to put the function at the bottom of the file, outside the class
definition, with other inline function and function template definitions. This
helps keep class definitions smaller and easier to read.

> Source/WebCore/Modules/indexeddb/IDBValue.h:78
> -    encoder << m_sessionID;
> +    encoder << m_sessionID.isValid();
> +    if (m_sessionID.isValid())
> +	   encoder << m_sessionID;

Kind of inelegant to have the "invalid" concept built into SessionID but not
work for encode/decode. Maybe change to not having "invalid" session IDs and
use Optional<> instead as a cleanup later. That would make this code nicer like
the m_databaseIdentifier example.

> Source/WebCore/Modules/indexeddb/shared/IDBRequestData.h:74
> -    const IDBDatabaseIdentifier& databaseIdentifier() const { return
m_databaseIdentifier; }
> +    const IDBDatabaseIdentifier& databaseIdentifier() const
> +    {
> +	   ASSERT(m_databaseIdentifier);
> +	   if (!m_databaseIdentifier)
> +	       m_databaseIdentifier = IDBDatabaseIdentifier { };
> +	   return *m_databaseIdentifier;
> +    }

Same comment about moving definitions when making them non-trivial.

> Source/WebCore/PAL/pal/SessionID.h:108
> +    // FIXME: We should fail to decode an invalid sessionID.
> +    ASSERT(SessionID { *sessionID }.isValid());

When will we make that change? I understand that perhaps it’s too risky now,
but when will it be possible?