Sending renderer notification when update job finishes (Used to notify cache ...

Sending renderer notification when update job finishes (Used to notify cache complete).
OnCacheSelected() event caches appcache info in appcache host. This
info is returned for appcache inspection by devtools.

BUG = None
TEST = Manual


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

[michaeln, 2010-07-31 01:44:20]

http://codereview.chromium.org/3074015/diff/4001/5001
File webkit/appcache/appcache_host.cc (right):

http://codereview.chromium.org/3074015/diff/4001/5001#newcode27
webkit/appcache/appcache_host.cc:27: }
Indents are whacky here
- no need to indent the body of the namespace
- need to indent the body of the function

http://codereview.chromium.org/3074015/diff/4001/5001#newcode423
webkit/appcache/appcache_host.cc:423: frontend_->OnCacheSelected(host_id_,
info);
What if this was a GetAssociatedCacheInfo() method that returned an info struct?

frontend_->OnCacheSelected(host_id_, GetAssocatedCacheInfo());

http://codereview.chromium.org/3074015/diff/4001/5001#newcode470
webkit/appcache/appcache_host.cc:470: frontend_->OnCacheSelected(host_id_,
info);
frontend_->OnCacheSelected(host_id_, GetAssocatedCacheInfo());

http://codereview.chromium.org/3074015/diff/4001/5002
File webkit/appcache/appcache_host.h (right):

http://codereview.chromium.org/3074015/diff/4001/5002#newcode207
webkit/appcache/appcache_host.h:207: bool associated_cache_info_pending_;
here's a long winded member var name
  associated_cache_was_complete_when_selected_
wdyt?

[Kavita Kanetkar, 2010-08-03 03:00:31]

Thanks for the review.

http://codereview.chromium.org/3074015/diff/4001/5001
File webkit/appcache/appcache_host.cc (right):

http://codereview.chromium.org/3074015/diff/4001/5001#newcode27
webkit/appcache/appcache_host.cc:27: }
On 2010/07/31 01:44:20, michaeln wrote:
> Indents are whacky here
> - no need to indent the body of the namespace
> - need to indent the body of the function

Done.

http://codereview.chromium.org/3074015/diff/4001/5001#newcode27
webkit/appcache/appcache_host.cc:27: }
On 2010/07/31 01:44:20, michaeln wrote:
> Indents are whacky here
> - no need to indent the body of the namespace
> - need to indent the body of the function

Done.

http://codereview.chromium.org/3074015/diff/4001/5001#newcode470
webkit/appcache/appcache_host.cc:470: frontend_->OnCacheSelected(host_id_,
info);
I actually think it's the same. Hence the name Fill instead of Get. Again,
strong preference? I'll change it if you want me to.

On 2010/07/31 01:44:20, michaeln wrote:
> frontend_->OnCacheSelected(host_id_, GetAssocatedCacheInfo());

http://codereview.chromium.org/3074015/diff/4001/5002
File webkit/appcache/appcache_host.h (right):

http://codereview.chromium.org/3074015/diff/4001/5002#newcode207
webkit/appcache/appcache_host.h:207: bool associated_cache_info_pending_;
I liked the pending_ name better. Do u have strong preference?

On 2010/07/31 01:44:20, michaeln wrote:
> here's a long winded member var name
>   associated_cache_was_complete_when_selected_
> wdyt?

[michaeln, 2010-08-03 19:53:59]

> I actually think it's the same. Hence the name Fill instead of Get. Again,
> strong preference? I'll change it if you want me to.

Functionally they're equivalent, but i'm looking at it from the point of view of
readability. It could make the code a little less busy and easier to follow.
Compare to these blocks. 

void AppCacheHost::OnUpdateComplete(AppCacheGroup* group)
   ...
   if (associated_cache_info_pending_ && associated_cache_.get() &&
       associated_cache_->is_complete()) {
     associated_cache_info_pending_ = false;
     frontend_->OnCacheSelected(host_id_, GetAssociatedCacheInfo());
   }

void AppCacheHost::AssociateCache(AppCache* cache)
   ...
   if (cache)
     cache->AssociateHost(this);
   associated_cache_info_pending_ = cache && !cache->is_complete;
   frontend_->OnCacheSelected(host_id_, GetAssociatedCacheInfo());

I appreciate the concern about returning structs, although i think our compilers
are smart about that. Maybe make FillAssociatedCacheInfo(&info) an instance
method so it doesn't need to be passed the cache and status values?

void AppCacheHost::OnUpdateComplete(AppCacheGroup* group)
   ...
   if (associated_cache_info_pending_ && associated_cache_.get() &&
       associated_cache_->is_complete()) {
     associated_cache_info_pending_ = false;
     AppCacheInfo info;
     FillAssociatedCacheInfo(&info);
     frontend_->OnCacheSelected(host_id_, info);
   }

void AppCacheHost::AssociateCache(AppCache* cache)
   ...
   if (cache)
     cache->AssociateHost(this);
   associated_cache_info_pending_ = cache && !cache->is_complete;
   AppCacheInfo info;
   FillAssociatedCacheInfo(&info);
   frontend_->OnCacheSelected(host_id_, info);


> http://codereview.chromium.org/3074015/diff/4001/5002
> File webkit/appcache/appcache_host.h (right):
> 
> http://codereview.chromium.org/3074015/diff/4001/5002#newcode207
> webkit/appcache/appcache_host.h:207: bool associated_cache_info_pending_;
> I liked the pending_ name better. Do u have strong preference?

The pending_ name is good, certainly less wordy :)

[Kavita Kanetkar, 2010-08-03 22:29:56]

Thanks for the review!

Keeping Fill as is - discussed offline.

Also about instance methods, I like to keep utility-like methods out of class in
anonym namespace. Rest discussed offline.


On 2010/08/03 19:53:59, michaeln wrote:
> > I actually think it's the same. Hence the name Fill instead of Get. Again,
> > strong preference? I'll change it if you want me to.
> 
> Functionally they're equivalent, but i'm looking at it from the point of view
of
> readability. It could make the code a little less busy and easier to follow.
> Compare to these blocks. 
> 
> void AppCacheHost::OnUpdateComplete(AppCacheGroup* group)
>    ...
>    if (associated_cache_info_pending_ && associated_cache_.get() &&
>        associated_cache_->is_complete()) {
>      associated_cache_info_pending_ = false;
>      frontend_->OnCacheSelected(host_id_, GetAssociatedCacheInfo());
>    }
> 
> void AppCacheHost::AssociateCache(AppCache* cache)
>    ...
>    if (cache)
>      cache->AssociateHost(this);
>    associated_cache_info_pending_ = cache && !cache->is_complete;
>    frontend_->OnCacheSelected(host_id_, GetAssociatedCacheInfo());
> 
> I appreciate the concern about returning structs, although i think our
compilers
> are smart about that. Maybe make FillAssociatedCacheInfo(&info) an instance
> method so it doesn't need to be passed the cache and status values?
> 
> void AppCacheHost::OnUpdateComplete(AppCacheGroup* group)
>    ...
>    if (associated_cache_info_pending_ && associated_cache_.get() &&
>        associated_cache_->is_complete()) {
>      associated_cache_info_pending_ = false;
>      AppCacheInfo info;
>      FillAssociatedCacheInfo(&info);
>      frontend_->OnCacheSelected(host_id_, info);
>    }
> 
> void AppCacheHost::AssociateCache(AppCache* cache)
>    ...
>    if (cache)
>      cache->AssociateHost(this);
>    associated_cache_info_pending_ = cache && !cache->is_complete;
>    AppCacheInfo info;
>    FillAssociatedCacheInfo(&info);
>    frontend_->OnCacheSelected(host_id_, info);
> 
> 
> > http://codereview.chromium.org/3074015/diff/4001/5002
> > File webkit/appcache/appcache_host.h (right):
> > 
> > http://codereview.chromium.org/3074015/diff/4001/5002#newcode207
> > webkit/appcache/appcache_host.h:207: bool associated_cache_info_pending_;
> > I liked the pending_ name better. Do u have strong preference?
> 
> The pending_ name is good, certainly less wordy :)

[michaeln, 2010-08-04 00:34:26]

http://codereview.chromium.org/3074015/diff/11002/12004
File webkit/appcache/appcache_host.cc (right):

http://codereview.chromium.org/3074015/diff/11002/12004#newcode17
webkit/appcache/appcache_host.cc:17: appcache::AppCacheInfo* info) {
Can you move this anon namespace into the appcache namespace so we don't need
all the appcache:: prefixing. Or make it an instance method.

I think you can just pass Status by value, its a primitive numeric type.

Passing the refcounted class AppCache by reference feels odd. We generally pass
refcounted objects by ptr. You can DCHECK(cache) as a form of documentation that
it shouldn't be NULL.

http://codereview.chromium.org/3074015/diff/11002/12004#newcode420
webkit/appcache/appcache_host.cc:420: appcache::AppCacheInfo info;
no need for the appcache:: prefix

[Kavita Kanetkar, 2010-08-04 22:16:43]

Thanks for the offline discussion!

http://codereview.chromium.org/3074015/diff/11002/12004
File webkit/appcache/appcache_host.cc (right):

http://codereview.chromium.org/3074015/diff/11002/12004#newcode17
webkit/appcache/appcache_host.cc:17: appcache::AppCacheInfo* info) {
I like the anonym namespace *outside* named ones. I think it better readable :)
But I moved it to appcache namespace as you wanted.

Spoke offline about Status Vs const& Status. Makes sense to have underlying int
be passed by value!

In this method cache is a const ref so it cannot be NULL, right? I changed as
you asked me to and added DCHECK.
I thought it was a style issue.

On 2010/08/04 00:34:26, michaeln wrote:
> Can you move this anon namespace into the appcache namespace so we don't need
> all the appcache:: prefixing. Or make it an instance method.
> 
> I think you can just pass Status by value, its a primitive numeric type.
> 
> Passing the refcounted class AppCache by reference feels odd. We generally
pass
> refcounted objects by ptr. You can DCHECK(cache) as a form of documentation
that
> it shouldn't be NULL.

http://codereview.chromium.org/3074015/diff/11002/12004#newcode420
webkit/appcache/appcache_host.cc:420: appcache::AppCacheInfo info;
On 2010/08/04 00:34:26, michaeln wrote:
> no need for the appcache:: prefix

Done.

[michaeln, 2010-08-04 23:51:11]

LGTM