Bug 53565

Summary: Refactor IDBRequest and IDBTransaction a bit
Product: WebKit Reporter: Jeremy Orlow <jorlow>
Component: New BugsAssignee: Jeremy Orlow <jorlow>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, andreip, dglazkov, dgrogan, eric, hans, japhet, mihaip, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch japhet: review+

Description Jeremy Orlow 2011-02-01 18:11:40 PST 
Refactor IDBRequest and IDBTransaction a bit
Comment 1 Jeremy Orlow 2011-02-01 18:14:17 PST 
Created attachment 80868 [details]
Patch
Comment 2 Jeremy Orlow 2011-02-01 18:15:26 PST 
Mihai, this modifies EventQueue so I added you.
Comment 3 Andrei Popescu 2011-02-01 18:44:30 PST 
Looks great.

>  if (m_transaction)
>        IDBPendingTransactionMonitor::removePendingTransaction(m_transaction.get());

Not sure I fully understand this. When can m_transaction be 0? Seems like this wasn't possible before?
Comment 4 Jeremy Orlow 2011-02-01 18:49:57 PST 
(In reply to comment #3)
> Looks great.
> 
> >  if (m_transaction)
> >        IDBPendingTransactionMonitor::removePendingTransaction(m_transaction.get());
> 
> Not sure I fully understand this. When can m_transaction be 0? Seems like this wasn't possible before?

You're right.  That was a mistake.
Comment 5 Jeremy Orlow 2011-02-01 19:58:48 PST 
Created attachment 80877 [details]
Patch
Comment 6 Mihai Parparita 2011-02-02 16:22:38 PST 
Comment on attachment 80877 [details]
Patch

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

> Source/WebCore/dom/EventQueue.cpp:84
> +        eventTarget->dispatchEvent(event);

This is the same as the initial toNode() branch. I think you should be able to just remove that and get the same behavior (turning this into if (..toDOMWindow) {..} else {...}).

> Source/WebCore/storage/IDBRequest.cpp:161
> +    ASSERT(scriptExecutionContext()->isDocument());

I thought the IndexedDB API was going to be available from workers too.
Comment 7 Jeremy Orlow 2011-02-02 16:42:09 PST 
Comment on attachment 80877 [details]
Patch

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

>> Source/WebCore/dom/EventQueue.cpp:84
>> +        eventTarget->dispatchEvent(event);
> 
> This is the same as the initial toNode() branch. I think you should be able to just remove that and get the same behavior (turning this into if (..toDOMWindow) {..} else {...}).

Will do.

>> Source/WebCore/storage/IDBRequest.cpp:161
>> +    ASSERT(scriptExecutionContext()->isDocument());
> 
> I thought the IndexedDB API was going to be available from workers too.

It will be.  But I think this is the best way to go for now.  We'll re-evaluate when we add to workers (which is probably at least a month or two away).
Comment 8 Jeremy Orlow 2011-02-03 17:27:49 PST 
Created attachment 81155 [details]
Patch
Comment 9 Jeremy Orlow 2011-02-03 17:29:53 PST 
Created attachment 81156 [details]
Patch
Comment 10 Jeremy Orlow 2011-02-03 17:30:56 PST 
I rebased this patch and fixed the issue Mihai pointed out.

Nate, can you please review?
Comment 11 WebKit Review Bot 2011-02-03 20:56:17 PST 
Attachment 81156 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7692729
Comment 12 WebKit Review Bot 2011-02-03 23:18:32 PST 
Attachment 81156 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7692789
Comment 13 Nate Chapin 2011-02-04 11:32:51 PST 
Comment on attachment 81156 [details]
Patch

I like it. :)
Comment 14 Jeremy Orlow 2011-02-04 11:53:04 PST 
landed
http://trac.webkit.org/changeset/77650
Comment 15 WebKit Review Bot 2011-02-04 12:02:07 PST 
http://trac.webkit.org/changeset/77650 might have broken Chromium Linux Release