[webkit-reviews] review granted: [Bug 36770] More IndexedDB work : [Attachment 51946] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has granted Jeremy Orlow
<jorlow at chromium.org>'s request for review:
Bug 36770: More IndexedDB work
https://bugs.webkit.org/show_bug.cgi?id=36770

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> +++ b/WebCore/page/PageGroup.cpp
...
> +#if ENABLE(INDEXED_DATABASE)
> +IndexedDatabase* PageGroup::indexedDatabase()
> +{
> +    // Do NOT add page settings based access controls here (like in
LocalStorage above).
> +    // ChromeClient might be a good place.  Doing a check in
DOMWindow::indexedDB (so this
> +    // only gets called once we've passed a check) might also be good.  But
the way it
> +    // was done originally was a mistake.

This comment could use some tidying.  It might help to explain
why adding settings based access controls here would be bad.
I think you could probably leave it at that.  The last sentence
is confusing since it seems to refer to some older implementation
of IndexedDB.  Comments like that don't stand the test of time
very well.


> +++ b/WebCore/storage/IndexedDatabaseRequest.h
...
>  class IndexedDatabaseRequest : public RefCounted<IndexedDatabaseRequest> {
>  public:
> +    static PassRefPtr<IndexedDatabaseRequest> create(Page* page)
>      {
> +	   return adoptRef(new IndexedDatabaseRequest(page));
>      }
>      ~IndexedDatabaseRequest();
>  
>      void open(const String& name, const String& description, bool
modifyDatabase, ExceptionCode&);
>  
>  private:
> +    IndexedDatabaseRequest(Page* page);

nit: do not name the param


>  
> +    PassRefPtr<IndexedDatabase> m_indexedDatabase;
> +    Page* m_page;
>  };

Hmm... you've got a reference counted class holding a non-owning pointer
to Page.  That sounds like a recipe for trouble.  I would expect to at
least see a public method that ~Page() should call to clear m_page.


> +++ b/WebKit/chromium/src/WebIndexedDatabaseImpl.cpp
...
> +#include "config.h"
> +#include "WebIndexedDatabaseImpl.h"
> +
> +
> +#include "WebIDBDatabaseError.h"

nit: only 1 new line between those includes.


r=me w/ these issues addressed.