[webkit-reviews] review denied: [Bug 40054] [indexedDB] It is impossible to create object stores : [Attachment 57762] Patch

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has denied Andrei Popescu
<andreip at google.com>'s request for review:
Bug 40054: [indexedDB] It is impossible to create object stores
https://bugs.webkit.org/show_bug.cgi?id=40054

Attachment 57762: Patch
https://bugs.webkit.org/attachment.cgi?id=57762&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
WebCore/ChangeLog:12
 +	    * Android.mk:
This list will need to be updated before you land.

WebCore/bindings/scripts/CodeGeneratorV8.pm:2537
 +		$result .= $indent . "if (!scriptContext)\n";
In what cases do we expect this to be possible?

WebCore/page/DOMWindow.cpp:475
 +	m_indexedDatabaseRequest = 0;
Is this necessary?  I can't think of why it would be.

WebCore/storage/IDBDatabaseImpl.cpp:56
 +	callbacks->onSuccess(objectStore.release());
You need to keep a list of object stores and add it to that.  And if it's
already there, return an error.  At least add a FIXME now.

WebCore/storage/IDBDatabaseRequest.cpp:52
 +	    // FIXME: make this work with workers.
I think just an assert is fine.  Sure it'll crash and burn badly if we're in
release mode, but it'd be hard for someone to accidentally enable in workers. 
:-)

WebCore/storage/IDBDatabaseRequest.cpp:58
 +	    return 0;
I think you should still return the IDBRequest and just never call
onsuccess/onerror since it's going away anyway.  Let's not create weird bugs
for developers that are impossible to debug.

WebCore/storage/IDBObjectStoreImpl.h:45
 +	String name() const { return m_name; }
const Sring&

WebCore/storage/IDBObjectStoreImpl.h:46
 +	String keyPath() const { return m_keyPath; }
ditto

WebCore/storage/IDBObjectStoreImpl.h:59
 +	typedef HashMap<String, RefPtr<IDBIndex> > IndexMap;
maybe a newline before typedef?

WebCore/storage/IDBRequest.h:71
 +	// FIXME: Have one onSuccess function for each possible result type.
I think we can remove this now...should be obvious.

WebCore/storage/IndexedDatabaseRequest.cpp:56
 +	if (!context->isDocument()) {
same things here as above.

WebKit/chromium/public/WebIDBDatabase.h:62
 +	virtual void createObjectStore(const WebString& name, const WebString&
keyPath, bool autoIncrement, WebIDBCallbacks*, WebFrame*)
This doesn't need the frame.  Only IndexedDatabase::open since we'll need to
check the content settings.

WebKit/chromium/public/WebIDBObjectStore.h:13
 +   * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
delete

WebKit/chromium/src/IDBObjectStoreProxy.cpp:66
 +	return 0;
Assert not implemented

WebKit/chromium/src/IDBObjectStoreProxy.cpp:71
 +	// FIXME: implement.
Why not do this now?  Just add it to the Web interface and redirect...no sense
in doing part of the plumbing but not all of it.


Close...I should probably do one more scan through though.