Bug 76723 - Switch indexeddb to use supplemental IDL for DOMWindow
Summary: Switch indexeddb to use supplemental IDL for DOMWindow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Pilgrim (Google)
URL:
Keywords:
Depends on:
Blocks: 79327
  Show dependency tree / graph
 
Reported: 2012-01-20 11:59 PST by Mark Pilgrim (Google)
Modified: 2012-02-23 20:56 PST (History)
4 users (show)

See Also:

Attachments
Patch (13.33 KB, patch)
2012-01-20 17:27 PST, Mark Pilgrim (Google) 		no flags Details | Formatted Diff | Diff
Patch (13.25 KB, patch)
2012-01-20 18:54 PST, Mark Pilgrim (Google) 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Pilgrim (Google) 2012-01-20 11:59:11 PST 
Switch indexeddb to use supplemental IDL for DOMWindow
Comment 1 Mark Pilgrim (Google) 2012-01-20 17:27:51 PST 
Created attachment 123418 [details]
Patch
Comment 2 Adam Barth 2012-01-20 17:32:03 PST 
Comment on attachment 123418 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/DOMWindowIndexedDatabase.cpp:62
> +       window->setIDBFactory(IDBFactory::create(page->group().idbFactory()));

I think this indent is off by one.

> Source/WebCore/page/DOMWindow.cpp:753
> +PassRefPtr<IDBFactory> DOMWindow::getIDBFactory()

Rather than returning a PassRefPtr<IDBFactory> here, we should just return a raw pointer.  We use PassRefPtr when we're passing ownership of an object.
Comment 3 Adam Barth 2012-01-20 17:59:08 PST 
Comment on attachment 123418 [details]
Patch

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

> Source/WebCore/page/DOMWindow.h:416
> +        PassRefPtr<IDBFactory> getIDBFactory();

Another nit: this function should be called idbFactory().  WebKit doesn't use the "get" prefix for getters.
Comment 4 Mark Pilgrim (Google) 2012-01-20 18:04:17 PST 
(In reply to comment #2)
> > Source/WebCore/page/DOMWindow.cpp:753
> > +PassRefPtr<IDBFactory> DOMWindow::getIDBFactory()
> 
> Rather than returning a PassRefPtr<IDBFactory> here, we should just return a raw pointer.  We use PassRefPtr when we're passing ownership of an object.

But IDBFactory::create(page->group().idbFactory()) is a PassRefPtr.
Comment 5 Adam Barth 2012-01-20 18:06:57 PST 
> But IDBFactory::create(page->group().idbFactory()) is a PassRefPtr.

Sure, but that gets given to the setter, which should take a PassRefPtr (since it grabs a reference).
Comment 6 Adam Barth 2012-01-20 18:10:16 PST 
http://www.webkit.org/coding/RefPtr.html might be worth a read if you haven't seen it before.
Comment 7 Mark Pilgrim (Google) 2012-01-20 18:54:36 PST 
Created attachment 123426 [details]
Patch
Comment 8 Adam Barth 2012-01-20 18:57:40 PST 
Comment on attachment 123426 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/DOMWindowIndexedDatabase.cpp:52
> +    Document* document = window->document();
> +    if (!document)
> +        return 0;

We can probably remove this check, but that's something for a later patch.
Comment 9 WebKit Review Bot 2012-01-20 22:07:04 PST 
Comment on attachment 123426 [details]
Patch

Clearing flags on attachment: 123426

Committed r105569: <http://trac.webkit.org/changeset/105569>
Comment 10 WebKit Review Bot 2012-01-20 22:07:09 PST 
All reviewed patches have been landed.  Closing bug.