[Webkit-unassigned] [Bug 36770] More IndexedDB work

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=36770


Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #51946|review?                     |review+
               Flag|                            |




--- Comment #3 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2010-03-29 23:13:19 PST ---
(From update of attachment 51946)
> +++ 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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.