Bug 53565 – Refactor IDBRequest and IDBTransaction a bit

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeremy Orlow]

Refactor IDBRequest and IDBTransaction a bit

[Jeremy Orlow]

Created attachment 80868 [details]
Patch

[Jeremy Orlow]

Mihai, this modifies EventQueue so I added you.

[Andrei Popescu]

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]

(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]

Created attachment 80877 [details]
Patch

[Mihai Parparita]

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 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]

Created attachment 81155 [details]
Patch

[Jeremy Orlow]

Created attachment 81156 [details]
Patch

[Jeremy Orlow]

I rebased this patch and fixed the issue Mihai pointed out.

Nate, can you please review?

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Nate Chapin]

Comment on attachment 81156 [details]
Patch

I like it. :)

[Jeremy Orlow]

landed
http://trac.webkit.org/changeset/77650

[WebKit Review Bot]

http://trac.webkit.org/changeset/77650 might have broken Chromium Linux Release