Bug 96952

Summary: IndexedDB: Pending call cleanup
Product: WebKit Reporter: Joshua Bell <jsbell>
Component: New BugsAssignee: Joshua Bell <jsbell>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, dgrogan, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 96444    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description Joshua Bell 2012-09-17 13:56:00 PDT
IndexedDB: Pending call cleanup
Comment 1 Joshua Bell 2012-09-17 13:57:26 PDT
Created attachment 164452 [details]
Patch
Comment 2 Joshua Bell 2012-09-17 13:58:39 PDT
dgrogan@ - this may be redundant with changes you have in progress. Is it worth landing now?
Comment 3 David Grogan 2012-09-17 14:43:02 PDT
Comment on attachment 164452 [details]
Patch

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

LGTM

I haven't done this locally yet, no. This looks great. I'd expect that this would make the already-asserting layout test from bug 96947 hit ASSERT_WITH_MESSAGE(!m_pendingSecondHalfOpenWithVersion, "m_pendingSecondHalfOpenWithVersion not null").  Have you run it?

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:489
> +    ASSERT_WITH_MESSAGE(!m_pendingSecondHalfOpenWithVersion, "m_pendingSecondHalfOpenWithVersion not null");

I'd say change this to plain ASSERT.
Comment 4 David Grogan 2012-09-17 14:47:14 PDT
(In reply to comment #3)
> (From update of attachment 164452 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164452&action=review
> 
> LGTM
> 
> I haven't done this locally yet, no. This looks great. I'd expect that this would make the already-asserting layout test from bug 96947 hit ASSERT_WITH_MESSAGE(!m_pendingSecondHalfOpenWithVersion, "m_pendingSecondHalfOpenWithVersion not null").  Have you run it?

After further thought, I do not think that would happen. This patch should be a no-behavior change.

> > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:489
> > +    ASSERT_WITH_MESSAGE(!m_pendingSecondHalfOpenWithVersion, "m_pendingSecondHalfOpenWithVersion not null");
> 
> I'd say change this to plain ASSERT.
Comment 5 Joshua Bell 2012-09-17 14:52:20 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 164452 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=164452&action=review
> > 
> > LGTM
> > 
> > I haven't done this locally yet, no. This looks great. I'd expect that this would make the already-asserting layout test from bug 96947 hit ASSERT_WITH_MESSAGE(!m_pendingSecondHalfOpenWithVersion, "m_pendingSecondHalfOpenWithVersion not null").  Have you run it?
> 
> After further thought, I do not think that would happen. This patch should be a no-behavior change.

I did run it and didn't any behavior changes in webkit or chromium tests.

> > > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:489
> > > +    ASSERT_WITH_MESSAGE(!m_pendingSecondHalfOpenWithVersion, "m_pendingSecondHalfOpenWithVersion not null");
> > 
> > I'd say change this to plain ASSERT.

Will do.
Comment 6 Joshua Bell 2012-09-17 15:04:04 PDT
Created attachment 164458 [details]
Patch
Comment 7 Joshua Bell 2012-09-17 15:04:27 PDT
tony@ - r?
Comment 8 Joshua Bell 2012-09-19 16:35:24 PDT
Created attachment 164801 [details]
Patch for landing
Comment 9 WebKit Review Bot 2012-09-19 17:12:28 PDT
Comment on attachment 164801 [details]
Patch for landing

Clearing flags on attachment: 164801

Committed r129076: <http://trac.webkit.org/changeset/129076>
Comment 10 WebKit Review Bot 2012-09-19 17:12:31 PDT
All reviewed patches have been landed.  Closing bug.