Implementing WebDatabaseObserver in the renderer process....

Implementing WebDatabaseObserver in the renderer process.

BUG=none
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=29993

[michaeln, 2009-10-21 05:31:53]

http://codereview.chromium.org/293038/diff/1/5
File chrome/renderer/render_thread.h (right):

http://codereview.chromium.org/293038/diff/1/5#newcode206
Line 206: scoped_ptr<RendererWebDatabaseObserver>
renderer_web_database_observer_;
Should this be moved up one line for grouping with the other data members under
the comment...

// These objects live solely on the render thread.

http://codereview.chromium.org/293038/diff/1/5#newcode207
Line 207: scoped_refptr<DBMessageFilter> db_message_filter_;
Maybe a comment that indicates the filter is...

// Used on the render thread and ipc thread.

http://codereview.chromium.org/293038/diff/1/6
File chrome/renderer/renderer_web_database_observer.cc (right):

http://codereview.chromium.org/293038/diff/1/6#newcode7
Line 7: #include "chrome/common/db_message_filter.h"
Really, you just need a compile time dependency on IPC::Message::Sender. You
could remove the dependency on the "filter" and its static getter from this
class.

explicit RendererWebDatabaseObserver(IPC::Message::Sender* sender)
  : sender_(sender) {
}

For example, AppCacheDispatcher does exactly that and its constructed a few
lines up from this new class in RenderThread. Fyi RenderThread implements the
IPC::Message::Sender interface.

http://codereview.chromium.org/293038/diff/1/6#newcode12
Line 12: WebKit::WebDatabaseObserver::~WebDatabaseObserver();
is there a need to explicitly call the super classes dtor?

http://codereview.chromium.org/293038/diff/1/6#newcode18
Line 18: db_message_filter->Send(
could just be...
  sender_->Send(...)
... here and below

http://codereview.chromium.org/293038/diff/1/3
File chrome/renderer/renderer_web_database_observer.h (right):

http://codereview.chromium.org/293038/diff/1/3#newcode11
Line 11: class WebDatabase;
This must also be forward declared in WebDatabaseObserver.h, so you may not need
to put another forward decl here.

http://codereview.chromium.org/293038/diff/1/3#newcode16
Line 16: ~RendererWebDatabaseObserver();
does a default dtor not work here?

[michaeln, 2009-10-21 19:38:46]

In reply to a real world responses to some of these comments...

static bool foo_was_destructed = false;

class Foo {
 protected:
  ~Foo() {
    foo_was_destructed = true;
  }
};

class Bar : public Foo {
  // Dumi, notice no dtor definition and no manual invoking of Foo's dtor
};

TEST(Dumi, FunAndGamesWithDestructors) {
  // Foo* foo = new Foo;
  // delete foo; // This line won't compile because Foo's dtor is protected

  EXPECT_FALSE(foo_was_destructed);
  Bar* bar = new Bar;
  delete bar;  // Uses the 'default' automagically generated public dtor
  EXPECT_TRUE(foo_was_destructed);  // Ah ha, Foo's dtor is called!
}

[dumi, 2009-10-21 22:38:59]

http://codereview.chromium.org/293038/diff/1/5
File chrome/renderer/render_thread.h (right):

http://codereview.chromium.org/293038/diff/1/5#newcode206
Line 206: scoped_ptr<RendererWebDatabaseObserver>
renderer_web_database_observer_;
On 2009/10/21 05:31:53, michaeln wrote:
> Should this be moved up one line for grouping with the other data members
under
> the comment...
> 
> // These objects live solely on the render thread.

done.

http://codereview.chromium.org/293038/diff/1/5#newcode207
Line 207: scoped_refptr<DBMessageFilter> db_message_filter_;
On 2009/10/21 05:31:53, michaeln wrote:
> Maybe a comment that indicates the filter is...
> 
> // Used on the render thread and ipc thread.

done.

http://codereview.chromium.org/293038/diff/1/6
File chrome/renderer/renderer_web_database_observer.cc (right):

http://codereview.chromium.org/293038/diff/1/6#newcode7
Line 7: #include "chrome/common/db_message_filter.h"
On 2009/10/21 05:31:53, michaeln wrote:
> Really, you just need a compile time dependency on IPC::Message::Sender. You
> could remove the dependency on the "filter" and its static getter from this
> class.
> 
> explicit RendererWebDatabaseObserver(IPC::Message::Sender* sender)
>   : sender_(sender) {
> }
> 
> For example, AppCacheDispatcher does exactly that and its constructed a few
> lines up from this new class in RenderThread. Fyi RenderThread implements the
> IPC::Message::Sender interface.

done.

http://codereview.chromium.org/293038/diff/1/6#newcode12
Line 12: WebKit::WebDatabaseObserver::~WebDatabaseObserver();
On 2009/10/21 05:31:53, michaeln wrote:
> is there a need to explicitly call the super classes dtor?

removed.

http://codereview.chromium.org/293038/diff/1/6#newcode18
Line 18: db_message_filter->Send(
On 2009/10/21 05:31:53, michaeln wrote:
> could just be...
>   sender_->Send(...)
> ... here and below

done.

http://codereview.chromium.org/293038/diff/1/3
File chrome/renderer/renderer_web_database_observer.h (right):

http://codereview.chromium.org/293038/diff/1/3#newcode11
Line 11: class WebDatabase;
On 2009/10/21 05:31:53, michaeln wrote:
> This must also be forward declared in WebDatabaseObserver.h, so you may not
need
> to put another forward decl here.

removed.

http://codereview.chromium.org/293038/diff/1/3#newcode16
Line 16: ~RendererWebDatabaseObserver();
On 2009/10/21 05:31:53, michaeln wrote:
> does a default dtor not work here?

removed.

[michaeln, 2009-10-22 05:54:55]

modulo nits, lgtm

http://codereview.chromium.org/293038/diff/3002/3003
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/293038/diff/3002/3003#newcode763
Line 763: // Returns the max size for this database
// Notifies the child process of the new database size

http://codereview.chromium.org/293038/diff/3002/3003#newcode764
Line 764: IPC_MESSAGE_CONTROL4(ViewMsg_DatabaseUpdateDatabaseSize,
nit: this could probably do without Database in the name twice ...
ViewMsg_DatabaseSizeUpdate

http://codereview.chromium.org/293038/diff/3002/3003#newcode770
Line 770: IPC_MESSAGE_CONTROL5(ViewMsg_DOMStorageEvent,
please restore the comment for this message

http://codereview.chromium.org/293038/diff/3002/3004
File chrome/renderer/renderer_web_database_observer.h (right):

http://codereview.chromium.org/293038/diff/3002/3004#newcode14
Line 14: virtual void databaseOpened(const WebKit::WebDatabase&);
nit: in chrome style, include the param name in addition to its type

[darin (slow to review), 2009-10-22 06:44:01]

http://codereview.chromium.org/293038/diff/3002/3003
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/293038/diff/3002/3003#newcode764
Line 764: IPC_MESSAGE_CONTROL4(ViewMsg_DatabaseUpdateDatabaseSize,
On 2009/10/22 05:54:55, michaeln wrote:
> nit: this could probably do without Database in the name twice ...
> ViewMsg_DatabaseSizeUpdate
> 

+1

http://codereview.chromium.org/293038/diff/3002/3005
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/293038/diff/3002/3005#newcode143
Line 143: renderer_web_database_observer_.reset(new
RendererWebDatabaseObserver(this));
i think this is called in the wrong place.  please move this to within
EnsureWebKitInitialized.

[dumi, 2009-10-22 23:38:22]

http://codereview.chromium.org/293038/diff/3002/3003
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/293038/diff/3002/3003#newcode763
Line 763: // Returns the max size for this database
On 2009/10/22 05:54:55, michaeln wrote:
> // Notifies the child process of the new database size

done.

http://codereview.chromium.org/293038/diff/3002/3003#newcode764
Line 764: IPC_MESSAGE_CONTROL4(ViewMsg_DatabaseUpdateDatabaseSize,
On 2009/10/22 05:54:55, michaeln wrote:
> nit: this could probably do without Database in the name twice ...
> ViewMsg_DatabaseSizeUpdate
> 

done.

http://codereview.chromium.org/293038/diff/3002/3003#newcode770
Line 770: IPC_MESSAGE_CONTROL5(ViewMsg_DOMStorageEvent,
On 2009/10/22 05:54:55, michaeln wrote:
> please restore the comment for this message

oops. done.

http://codereview.chromium.org/293038/diff/3002/3005
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/293038/diff/3002/3005#newcode143
Line 143: renderer_web_database_observer_.reset(new
RendererWebDatabaseObserver(this));
On 2009/10/22 06:44:01, darin wrote:
> i think this is called in the wrong place.  please move this to within
> EnsureWebKitInitialized.

done.

http://codereview.chromium.org/293038/diff/3002/3004
File chrome/renderer/renderer_web_database_observer.h (right):

http://codereview.chromium.org/293038/diff/3002/3004#newcode14
Line 14: virtual void databaseOpened(const WebKit::WebDatabase&);
On 2009/10/22 05:54:55, michaeln wrote:
> nit: in chrome style, include the param name in addition to its type

done.

[michaeln, 2009-10-23 06:47:48]

still lgtm... the webkit api needs to land first obviously

http://codereview.chromium.org/293038/diff/9001/9002
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/293038/diff/9001/9002#newcode768
Line 768: int64 /* the new database size */)
nit: the parameter order here should match the paramter order in the
corresponding webkit api method (and at all hops thru from  browser to renderer
to webkit api)... whatever order that may be... it should match.

[dumi, 2009-10-23 18:52:16]

http://codereview.chromium.org/293038/diff/9001/9002
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/293038/diff/9001/9002#newcode768
Line 768: int64 /* the new database size */)
On 2009/10/23 06:47:48, michaeln wrote:
> nit: the parameter order here should match the paramter order in the
> corresponding webkit api method (and at all hops thru from  browser to
renderer
> to webkit api)... whatever order that may be... it should match.

fixed.

[michaeln, 2009-10-23 21:51:36]

LGTM (once the webkit api CL is committed)

http://codereview.chromium.org/293038/diff/13001/13004
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/293038/diff/13001/13004#newcode141
Line 141: 
nit: no need for the new blank line

[dumi, 2009-10-23 21:59:55]

http://codereview.chromium.org/293038/diff/13001/13004
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/293038/diff/13001/13004#newcode141
Line 141: 
On 2009/10/23 21:51:36, michaeln wrote:
> nit: no need for the new blank line

removed.