Cleanup: Make quota dispatcher code independent from WebKit types

Cleanup: Make quota dispatcher code independent from WebKit types

Preparing for pepper quota support.

Also adding NotifyStorageModified IPC method, which is intended to be used by pepper FileIO code.

BUG=for 86556
TEST=existing tests should pass

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

[michaeln, 2011-07-19 20:27:36]

http://codereview.chromium.org/7438001/diff/13/content/common/web_quota_dispa...
File content/common/web_quota_dispatcher_callback.h (right):

http://codereview.chromium.org/7438001/diff/13/content/common/web_quota_dispa...
content/common/web_quota_dispatcher_callback.h:15: class
WebQuotaDispatcherCallback : public QuotaDispatcher::Callback {
This class name confused me a bit. I've come to expect classes starting with
"Web" are either defined in the WebKit API or derive from a such a class (maybe
not not always true, but generally that's the case).

Also, the layers of indirection when looking at how 'callbacks' work can obscure
what's going on, so i'd look for ways to avoid defining a new class at the
global scope.

If there aren't any other implementations of QuotaDispatcher::Callback on the
horizon? I wonder if code would be easier to follow if QuotaDispatcher::Callback
was a concrete impl instead of an abstract interface. Of course the
QuotaDispatcher would still have a dependency on the WebKit API in that case
which is part of what your trying to accomplish.

Another way to avoid introducing a new 'callback' class into the global
namespace  could be to define the concrete subclass that wraps the WebKit
callback class inside of an anon namespace within RenderView.cc... a hidden
helper class (QuotaCallbacksWrapper) instead of a top-level thing... less
cognitive weight feeling to me.

Wdyt?

At a minimum, please reconsider this class name so that it doesn't start with
"Web".

[kinuko, 2011-07-20 07:55:52]

Thanks for reviewing.
I dropped web_quota_dispatcher_callback.{cc,h}, moved the impl in
quota_dispatcher.cc as a hidden class and added
QuotaDispatcher::CreateWebStorageQuotaCallbacksWrapper.
How does this look now?

> WebQuotaDispatcherCallback : public QuotaDispatcher::Callback {
> This class name confused me a bit. I've come to expect classes starting with
> "Web" are either defined in the WebKit API or derive from a such a class
(maybe
> not not always true, but generally that's the case).

I can understand this concern, but we can see the similar model in
content/common/file_system/webfilesystem_callback_dispatcher.{cc,h}.

> If there aren't any other implementations of QuotaDispatcher::Callback on the
> horizon? I wonder if code would be easier to follow if
QuotaDispatcher::Callback
> was a concrete impl instead of an abstract interface.

The one reason I wanted to do this is I may want to call QuotaDispatcher methods
from pepper impl... similar reason when we introduced the
webfilesystem_callback_dispatcher.{cc,h}.   (Depending on how we implement
pepper we may not need this change; in that case I'll just withdraw this
changeset)

> Another way to avoid introducing a new 'callback' class into the global
> namespace  could be to define the concrete subclass that wraps the WebKit
> callback class inside of an anon namespace within RenderView.cc... a hidden
> helper class (QuotaCallbacksWrapper) instead of a top-level thing... less
> cognitive weight feeling to me.

Yeah I considered this option, but was afraid it may bloat the render_view
further.  (But this design option sounds most natural to me actually)

[michaeln, 2011-07-20 18:04:30]

LGTM

[kinuko, 2011-07-25 17:25:00]

jam, for content/ changes could you review this?

[kinuko, 2011-07-27 09:20:23]

brettw, could you give me a LGTM for content/ changes?
(Basically just a cleanup; necessary for the other pepper quota change)

On 2011/07/25 17:25:00, kinuko wrote:
> jam, for content/ changes could you review this?

[brettw, 2011-07-27 15:32:08]

LGTM, just a few style nits.

http://codereview.chromium.org/7438001/diff/14001/content/common/quota_dispat...
File content/common/quota_dispatcher.cc (right):

http://codereview.chromium.org/7438001/diff/14001/content/common/quota_dispat...
content/common/quota_dispatcher.cc:21: namespace {
Blank line after this.

http://codereview.chromium.org/7438001/diff/14001/content/common/quota_dispat...
content/common/quota_dispatcher.cc:26: : callbacks_(c) {
Two more spaces before the :

http://codereview.chromium.org/7438001/diff/14001/content/common/quota_dispat...
content/common/quota_dispatcher.cc:44: }  // anonymous namespace
Bank line before this, no "anonymous"

[kinuko, 2011-07-28 13:20:56]

Thanks, fixed them. Will be committing...

On 2011/07/27 15:32:08, brettw wrote:
> LGTM, just a few style nits.
> 
>
http://codereview.chromium.org/7438001/diff/14001/content/common/quota_dispat...
> File content/common/quota_dispatcher.cc (right):
> 
>
http://codereview.chromium.org/7438001/diff/14001/content/common/quota_dispat...
> content/common/quota_dispatcher.cc:21: namespace {
> Blank line after this.
> 
>
http://codereview.chromium.org/7438001/diff/14001/content/common/quota_dispat...
> content/common/quota_dispatcher.cc:26: : callbacks_(c) {
> Two more spaces before the :
> 
>
http://codereview.chromium.org/7438001/diff/14001/content/common/quota_dispat...
> content/common/quota_dispatcher.cc:44: }  // anonymous namespace
> Bank line before this, no "anonymous"

[kinuko, 2011-07-28 13:21:32]

http://codereview.chromium.org/7438001/diff/14001/content/common/quota_dispat...
File content/common/quota_dispatcher.cc (right):

http://codereview.chromium.org/7438001/diff/14001/content/common/quota_dispat...
content/common/quota_dispatcher.cc:21: namespace {
On 2011/07/27 15:32:08, brettw wrote:
> Blank line after this.

Done.

http://codereview.chromium.org/7438001/diff/14001/content/common/quota_dispat...
content/common/quota_dispatcher.cc:26: : callbacks_(c) {
On 2011/07/27 15:32:08, brettw wrote:
> Two more spaces before the :

Done.

http://codereview.chromium.org/7438001/diff/14001/content/common/quota_dispat...
content/common/quota_dispatcher.cc:44: }  // anonymous namespace
On 2011/07/27 15:32:08, brettw wrote:
> Bank line before this, no "anonymous"

Done.