Bug 53565 - Refactor IDBRequest and IDBTransaction a bit
Summary: Refactor IDBRequest and IDBTransaction a bit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Jeremy Orlow
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-01 18:11 PST by Jeremy Orlow
Modified: 2011-02-04 12:02 PST (History)
9 users (show)

See Also:


Attachments
Patch (21.86 KB, patch)
2011-02-01 18:14 PST, Jeremy Orlow
no flags Details | Formatted Diff | Diff
Patch (21.96 KB, patch)
2011-02-01 19:58 PST, Jeremy Orlow
no flags Details | Formatted Diff | Diff
Patch (21.77 KB, patch)
2011-02-03 17:27 PST, Jeremy Orlow
no flags Details | Formatted Diff | Diff
Patch (21.98 KB, patch)
2011-02-03 17:29 PST, Jeremy Orlow
japhet: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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