Bug 53565

Summary:

Refactor IDBRequest and IDBTransaction a bit

Product:

WebKit

Reporter:

Jeremy Orlow <jorlow>

Component:

New Bugs

Assignee:

Jeremy Orlow <jorlow>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, andreip, dglazkov, dgrogan, eric, hans, japhet, mihaip, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Other

OS:

OS X 10.5

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	japhet: review+

Jeremy Orlow

Reported 2011-02-01 18:11:40 PST

Refactor IDBRequest and IDBTransaction a bit

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Jeremy Orlow

Comment 1 2011-02-01 18:14:17 PST

Created attachment 80868 [details] Patch

Jeremy Orlow

Comment 2 2011-02-01 18:15:26 PST

Mihai, this modifies EventQueue so I added you.

Andrei Popescu

Comment 3 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?

Jeremy Orlow

Comment 4 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.

Jeremy Orlow

Comment 5 2011-02-01 19:58:48 PST

Created attachment 80877 [details] Patch

Mihai Parparita

Comment 6 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.

Jeremy Orlow

Comment 7 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).

Jeremy Orlow

Comment 8 2011-02-03 17:27:49 PST

Created attachment 81155 [details] Patch

Jeremy Orlow

Comment 9 2011-02-03 17:29:53 PST

Created attachment 81156 [details] Patch

Jeremy Orlow

Comment 10 2011-02-03 17:30:56 PST

I rebased this patch and fixed the issue Mihai pointed out. Nate, can you please review?

WebKit Review Bot

Comment 11 2011-02-03 20:56:17 PST

Attachment 81156 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7692729