[Webkit-unassigned] [Bug 92725] IndexedDB: ObjectStoreMetaDataKey::m_metaDataType should use byte type

[bugzilla-daemon at webkit.org]

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





--- Comment #13 from Joshua Bell <jsbell at chromium.org>  2012-08-01 08:31:21 PST ---
(In reply to comment #11)
> (From update of attachment 155729 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155729&action=review
> 
> LGTM. If jsbell@ says OK, I'm happy to r+ it.

I'm going to build the Chromium tests locally with this patch first. Working on that now.

> > Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:180
> > +    ASSERT(p < limit);
> > +    if (p >= limit)
> 
> This looks strange. We don't need both. (Although some code in IDBLevelDBCoding.cpp already use both.)
> 
> If 'p >= limit' must not happen, please remove 'if (p >= limit)'. If it can happen, please remove ASSERT(p < limit). (I think that removing ASSERT() looks reasonable in this case.)

As an FYI:

Although much of this code predates me, I believe the intent of these redundant tests is to serve two different purposes. The ASSERTs are present to catch errors during development, e.g. when we're refactoring code, modifying constants, etc. The non-debug tests are in there to catch runtime errors due to corrupt data (e.g. actual on-disk corruption, uncaught bugs that write bad data, etc). 

I agree that's not a very good style, and I'm fine with leaving out the ASSERT here, as the error handling in this file needs a cleanup pass. We should probably switch the ASSERTs over to something like a custom macro that does ASSERT in Debug but also does a LOG_ERROR and stats reporting in Release builds, in addition to returning the error code.

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