Chrome side of changes required to populate appcache resource list....

Chrome side of changes required to populate appcache resource list.

BUG = 2821005
TEST = Manually navigate.


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

[michaeln, 2010-07-16 22:03:06]

http://codereview.chromium.org/3009005/diff/11002/30001
File chrome/browser/appcache/appcache_dispatcher_host.cc (right):

http://codereview.chromium.org/3009005/diff/11002/30001#newcode150
chrome/browser/appcache/appcache_dispatcher_host.cc:150: int host_id,
appcache::AppCacheResourceListMsg_Params* params) {
indent

http://codereview.chromium.org/3009005/diff/11002/30002
File chrome/browser/appcache/appcache_dispatcher_host.h (right):

http://codereview.chromium.org/3009005/diff/11002/30002#newcode62
chrome/browser/appcache/appcache_dispatcher_host.h:62: int host_id,
appcache::AppCacheResourceListMsg_Params* params);
indent

http://codereview.chromium.org/3009005/diff/11002/30006
File chrome/common/appcache/appcache_backend_proxy.h (right):

http://codereview.chromium.org/3009005/diff/11002/30006#newcode39
chrome/common/appcache/appcache_backend_proxy.h:39: int host_id,
appcache::AppCacheResourceListMsg_Params* params);
indent

http://codereview.chromium.org/3009005/diff/11002/30009
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/3009005/diff/11002/30009#newcode3069
chrome/common/render_messages.h:3069: return ReadParam(m, iter, &p->resources);
indent

http://codereview.chromium.org/3009005/diff/11002/30011
File chrome/renderer/renderer_webapplicationcachehost_impl.cc (right):

http://codereview.chromium.org/3009005/diff/11002/30011#newcode48
chrome/renderer/renderer_webapplicationcachehost_impl.cc:48:
appcache::AppCacheMsg_Params params;
ooops... please override the method with the new signature here

http://codereview.chromium.org/3009005/diff/11002/30012
File webkit/appcache/appcache.cc (right):

http://codereview.chromium.org/3009005/diff/11002/30012#newcode91
webkit/appcache/appcache.cc:91: cache_size_ = cache_record.cache_size;
same comment as before...

As coded cache_size_ will be set after we've loaded the cache from the database,
but it will not be valid for a newly created cache. Maybe modify AddEntry() to
update the sum as entries are added? That way it would be valid for newly
created caches too. And then we could DCHECK(cache_record.cache_size ==
cache_size_) down below this for loop.

http://codereview.chromium.org/3009005/diff/11002/30014
File webkit/appcache/appcache_backend_impl.cc (right):

http://codereview.chromium.org/3009005/diff/11002/30014#newcode127
webkit/appcache/appcache_backend_impl.cc:127: int host_id,
appcache::AppCacheResourceListMsg_Params* params) {
indent

http://codereview.chromium.org/3009005/diff/11002/30015
File webkit/appcache/appcache_backend_impl.h (right):

http://codereview.chromium.org/3009005/diff/11002/30015#newcode8
webkit/appcache/appcache_backend_impl.h:8: #include <hash_map>
is this needed?

http://codereview.chromium.org/3009005/diff/11002/30015#newcode38
webkit/appcache/appcache_backend_impl.h:38: int host_id,
AppCacheResourceListMsg_Params* params);
indent

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

http://codereview.chromium.org/3009005/diff/11002/30021#newcode12
webkit/appcache/appcache_host.cc:12: #include
"webkit/appcache/appcache_interfaces.h"
is this new include really needed? i see kNoCacheId being used in the ctor and
that's defined in the interfaces.h file, so i expect this is already in the
include graph

http://codereview.chromium.org/3009005/diff/11002/30021#newcode44
webkit/appcache/appcache_host.cc:44: void
AppCacheHost::GetResourceList(AppCacheResourceListMsg_Params* params) {
if you do move the method definition in the .h file, please move this down
correspondingly

http://codereview.chromium.org/3009005/diff/11002/30021#newcode45
webkit/appcache/appcache_host.cc:45: if (associated_cache_.get()) {
webcore tests .is_complete() and if not returns an empty list, should we match
that behavior?

http://codereview.chromium.org/3009005/diff/11002/30021#newcode441
webkit/appcache/appcache_host.cc:441: // Cached and information available.
i don't understand this comment

http://codereview.chromium.org/3009005/diff/11002/30021#newcode443
webkit/appcache/appcache_host.cc:443: if (cache->is_complete()) {
maybe is complete could be added to the SelectedCacheInfo struct we send to the
other side instead of branching like this? wdyt?

http://codereview.chromium.org/3009005/diff/11002/30021#newcode447
webkit/appcache/appcache_host.cc:447: params.size = cache->cache_size();
indents here are whacky

http://codereview.chromium.org/3009005/diff/11002/30022
File webkit/appcache/appcache_host.h (right):

http://codereview.chromium.org/3009005/diff/11002/30022#newcode56
webkit/appcache/appcache_host.h:56: void
GetResourceList(AppCacheResourceListMsg_Params* params);
Could add a comment here about
// Support for inspecting appcaches.
And move this method definition down further in the .h file, maybe just under
the CreateRequestHandler method?

http://codereview.chromium.org/3009005/diff/11002/30023
File webkit/appcache/appcache_host_unittest.cc (right):

http://codereview.chromium.org/3009005/diff/11002/30023#newcode39
webkit/appcache/appcache_host_unittest.cc:39: appcache::Status status, const
appcache::AppCacheMsg_Params& params) {
indents are off

http://codereview.chromium.org/3009005/diff/11002/30023#newcode70
webkit/appcache/appcache_host_unittest.cc:70: virtual void OnContentBlocked(int
host_id) {
looks like sync/merge trouble here... we should keep that param... right?

http://codereview.chromium.org/3009005/diff/11002/30024
File webkit/appcache/appcache_interfaces.h (right):

http://codereview.chromium.org/3009005/diff/11002/30024#newcode54
webkit/appcache/appcache_interfaces.h:54: struct AppCacheMsg_Params {
Similar comments as before...

This struct name is too generic, also the naming convention is out of place in
this .h file. It looks like chrome IPC message naming, but we're in the appcache
namespace here which is independent of the chrome's IPC'isms. Also it looks like
the existing cache info params delivered to OnCacheSelected could be moved into
this struct as well.


struct AppCacheInfo {
  int64 cache_id;
  Status status;
  GURL manifest_url;
  base::Time creation_time;
  base::Time update_time;
  int64 cache_size;
  bool is_complete;  // <---- wdyt???
};

virtual void OnCacheSelected(int host_id, const AppCacheInfo& info) = 0;

http://codereview.chromium.org/3009005/diff/11002/30024#newcode72
webkit/appcache/appcache_interfaces.h:72: struct AppCacheResourceListMsg_Params
{
same comment as before...

I think we could live without this intermediary structure and w/o a std::vector
typedef. Having std::vector<T> in the method signatures is informative. The
other interface methods that take vectors are defined that way. Is there any
reason for this one to be different?

http://codereview.chromium.org/3009005/diff/11002/30025
File webkit/appcache/appcache_request_handler_unittest.cc (right):

http://codereview.chromium.org/3009005/diff/11002/30025#newcode46
webkit/appcache/appcache_request_handler_unittest.cc:46: virtual void
OnContentBlocked(int host_id) {}
ditto sync/merge

http://codereview.chromium.org/3009005/diff/11002/30026
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/3009005/diff/11002/30026#newcode378
webkit/appcache/appcache_storage_impl.cc:378: base::Time mock_time;
const kNoTime?

http://codereview.chromium.org/3009005/diff/11002/30026#newcode486
webkit/appcache/appcache_storage_impl.cc:486: if (success_) {
same comment as before

For newly created groups, we may need to update the create_time_ here. Also a
unit_test to test this behavior could be good.

http://codereview.chromium.org/3009005/diff/11002/30026#newcode937
webkit/appcache/appcache_storage_impl.cc:937: base::Time mock_time;
ditto

http://codereview.chromium.org/3009005/diff/11002/30026#newcode1047
webkit/appcache/appcache_storage_impl.cc:1047: kNoCacheId, GURL(), true));
sync/merge issue... please retain the manifest_url parameter

http://codereview.chromium.org/3009005/diff/11002/30027
File webkit/appcache/appcache_update_job_unittest.cc (right):

http://codereview.chromium.org/3009005/diff/11002/30027#newcode167
webkit/appcache/appcache_update_job_unittest.cc:167: Status status, const
appcache::AppCacheMsg_Params& params) {
indent

http://codereview.chromium.org/3009005/diff/11002/30027#newcode229
webkit/appcache/appcache_update_job_unittest.cc:229: virtual void
OnContentBlocked(int host_id) {
retain the 'manifest_url' param

http://codereview.chromium.org/3009005/diff/11002/30028
File webkit/appcache/mock_appcache_storage.cc (right):

http://codereview.chromium.org/3009005/diff/11002/30028#newcode179
webkit/appcache/mock_appcache_storage.cc:179: const base::Time time;
const kNoTime?

indents are off in this block

http://codereview.chromium.org/3009005/diff/11002/30029
File webkit/appcache/web_application_cache_host_impl.cc (right):

http://codereview.chromium.org/3009005/diff/11002/30029#newcode81
webkit/appcache/web_application_cache_host_impl.cc:81: const
appcache::AppCacheMsg_Params& params) {
indent the method args above

http://codereview.chromium.org/3009005/diff/11002/30029#newcode84
webkit/appcache/web_application_cache_host_impl.cc:84: if
(params.manifest_url.is_valid()) {
A really good test for whether or not we have a selected cache is to compare
selected_cache_id to kNoCacheId.

This class probably has to represent 'no cache selected' in some way. As coded,
it's notion of which cache is selected is unchanged when switching from
something to nothing selected.

(also please adjust the indent)

http://codereview.chromium.org/3009005/diff/11002/30029#newcode91
webkit/appcache/web_application_cache_host_impl.cc:91:
client_->didChangeCacheAssociation();
Similarly, we probably should to call thru to webkit/webcore when switching from
something to nothing.

http://codereview.chromium.org/3009005/diff/11002/30029#newcode184
webkit/appcache/web_application_cache_host_impl.cc:184:
WebVector<WebApplicationCacheHost::ResourceInfo>
ditto ResourceInfo may be usable without full scoping

http://codereview.chromium.org/3009005/diff/11002/30029#newcode188
webkit/appcache/web_application_cache_host_impl.cc:188: web_resources[i] =
WebApplicationCacheHost::ResourceInfo();
do we need this assignment to an empty struct?

http://codereview.chromium.org/3009005/diff/11002/30029#newcode195
webkit/appcache/web_application_cache_host_impl.cc:195: web_resources[i].url =
WebURL(params.resources[i].url);
i'm pretty sure there's an assignment operator to do the conversion w/o
explicitly using the WebURL ctor here

http://codereview.chromium.org/3009005/diff/11002/30030
File webkit/appcache/web_application_cache_host_impl.h (right):

http://codereview.chromium.org/3009005/diff/11002/30030#newcode40
webkit/appcache/web_application_cache_host_impl.h:40: const
appcache::AppCacheMsg_Params& params);
indent

http://codereview.chromium.org/3009005/diff/11002/30030#newcode60
webkit/appcache/web_application_cache_host_impl.h:60:
WebKit::WebVector<WebKit::WebApplicationCacheHost::ResourceInfo>* resources);
Since this is a WebApplicationCacheHost derviced class, do we need to fully
scope ResourceInfo and CacheInfo in these signatures?

http://codereview.chromium.org/3009005/diff/11002/30030#newcode84
webkit/appcache/web_application_cache_host_impl.h:84: GURL manifest;
manifest_url ?

http://codereview.chromium.org/3009005/diff/11002/30030#newcode87
webkit/appcache/web_application_cache_host_impl.h:87: double update_time;
Can these be of type base::Time?

http://codereview.chromium.org/3009005/diff/11002/30030#newcode88
webkit/appcache/web_application_cache_host_impl.h:88: long long size;
int64 in chrome speak

http://codereview.chromium.org/3009005/diff/11002/30030#newcode90
webkit/appcache/web_application_cache_host_impl.h:90: creation_time(c),
update_time(u), size(s) { }
indent, and no need for whitespace in empty {} blocks

http://codereview.chromium.org/3009005/diff/11002/30030#newcode92
webkit/appcache/web_application_cache_host_impl.h:92: scoped_ptr<AppCacheInfo>
appcache_info_;
We could put an AppCacheInfo struct inline here.

[Paweł Hajdan Jr., 2010-07-16 22:18:54]

Drive-by with minor style nits in test files. No need to wait for another review
by me.

Michael, please make sure to catch style nits in reviews.

http://codereview.chromium.org/3009005/diff/11002/30020
File webkit/appcache/appcache_group_unittest.cc (right):

http://codereview.chromium.org/3009005/diff/11002/30020#newcode5
webkit/appcache/appcache_group_unittest.cc:5: #include <string>
nit: please add empty line after this

http://codereview.chromium.org/3009005/diff/11002/30020#newcode12
webkit/appcache/appcache_group_unittest.cc:12: #include
"webkit/appcache/appcache_interfaces.h"
nit: same here.

[michaeln, 2010-07-17 00:53:51]

> http://codereview.chromium.org/3009005/diff/11002/30030#newcode92
> webkit/appcache/web_application_cache_host_impl.h:92: scoped_ptr<AppCacheInfo>
> appcache_info_;
> We could put an AppCacheInfo struct inline here.

Just to clarify, i meant to say put one of the structs defined in
appcache_interfaces.h inline here.

[michaeln, 2010-07-21 20:09:59]

You'll need to sync/merge, the patch isn't applying.

This isn't a full review of the latest patch. I'm just looking at the three
files that contain comments in them right.

http://codereview.chromium.org/3009005/diff/43002/57012
File chrome/renderer/renderer_webapplicationcachehost_impl.cc (right):

http://codereview.chromium.org/3009005/diff/43002/57012#newcode45
chrome/renderer/renderer_webapplicationcachehost_impl.cc:45: int64
selected_cache_id, appcache::Status status) {
This is intended to be an override of the virtual method defined in the base
class. Since the signature of the base class method is changing, so too should
this method signature.

http://codereview.chromium.org/3009005/diff/43002/57032
File webkit/appcache/web_application_cache_host_impl.cc (right):

http://codereview.chromium.org/3009005/diff/43002/57032#newcode90
webkit/appcache/web_application_cache_host_impl.cc:90:
client_->didChangeCacheAssociation();
Similar comments to last time...

This class probably has to represent 'no cache selected' in some way. As coded,
it's notion of which cache is selected is unchanged when switching from
something to nothing selected.

Similarly we probably should call thru to webkit/webcore when switching from
something (cacheId != kNoCacheId) to nothing (cacheId == kNoCacheId).

(also please adjust the indent)

http://codereview.chromium.org/3009005/diff/43002/57032#newcode184
webkit/appcache/web_application_cache_host_impl.cc:184:
web_resources(resource_infos.size());
looks like this will fit on one line now

http://codereview.chromium.org/3009005/diff/43002/57032#newcode193
webkit/appcache/web_application_cache_host_impl.cc:193: web_resources[i].url =
WebURL(resource_infos[i].url);
i thought there was an assignment operator to do the conversion w/o explicitly
using the WebURL ctor here... is that not the case?

http://codereview.chromium.org/3009005/diff/43002/57032#newcode194
webkit/appcache/web_application_cache_host_impl.cc:194: }
indents are off in the body of the for loop

http://codereview.chromium.org/3009005/diff/43002/57033
File webkit/appcache/web_application_cache_host_impl.h (right):

http://codereview.chromium.org/3009005/diff/43002/57033#newcode39
webkit/appcache/web_application_cache_host_impl.h:39: void OnCacheSelected(const
appcache::AppCacheInfo& info);
This method was made virtual a couple of CLs ago. We need to retain the
'virtual' keyword here.

http://codereview.chromium.org/3009005/diff/43002/57033#newcode80
webkit/appcache/web_application_cache_host_impl.h:80: struct AppCacheInfo {
Seems a little unfortunate to define a new 'cache info' struct here when we have
two to choose from. Have you considered using either a WebKit::WACH::CacheInfo
struct here or an appache::AppCacheInfo struct here? Alternatively we could just
store the values we use as distinct fields here.

http://codereview.chromium.org/3009005/diff/43002/57033#newcode87
webkit/appcache/web_application_cache_host_impl.h:87: creation_time(c),
update_time(u), size(s) {}
This wrapping is odd to my eye. its difficult to see where the constructor
arguments stop and where the initializer list starts. This would be more in
keeping with the style used in the project...

    AppCacheInfo(GURL m, base::Time c, base::Time u, long long s)
        : manifest_url(m), creation_time(c), update_time(u), size(s) {}

Use int64 instead of long long
The GURL arg s/b const GURL&

[Kavita Kanetkar, 2010-07-22 03:24:30]

Latest try succeeded. Please take another look.

Thanks!


On 2010/07/21 20:09:59, michaeln wrote:
> You'll need to sync/merge, the patch isn't applying.
> 
> This isn't a full review of the latest patch. I'm just looking at the three
> files that contain comments in them right.
> 
> http://codereview.chromium.org/3009005/diff/43002/57012
> File chrome/renderer/renderer_webapplicationcachehost_impl.cc (right):
> 
> http://codereview.chromium.org/3009005/diff/43002/57012#newcode45
> chrome/renderer/renderer_webapplicationcachehost_impl.cc:45: int64
> selected_cache_id, appcache::Status status) {
> This is intended to be an override of the virtual method defined in the base
> class. Since the signature of the base class method is changing, so too should
> this method signature.
> 
> http://codereview.chromium.org/3009005/diff/43002/57032
> File webkit/appcache/web_application_cache_host_impl.cc (right):
> 
> http://codereview.chromium.org/3009005/diff/43002/57032#newcode90
> webkit/appcache/web_application_cache_host_impl.cc:90:
> client_->didChangeCacheAssociation();
> Similar comments to last time...
> 
> This class probably has to represent 'no cache selected' in some way. As
coded,
> it's notion of which cache is selected is unchanged when switching from
> something to nothing selected.
> 
> Similarly we probably should call thru to webkit/webcore when switching from
> something (cacheId != kNoCacheId) to nothing (cacheId == kNoCacheId).
> 
> (also please adjust the indent)
> 
> http://codereview.chromium.org/3009005/diff/43002/57032#newcode184
> webkit/appcache/web_application_cache_host_impl.cc:184:
> web_resources(resource_infos.size());
> looks like this will fit on one line now
> 
> http://codereview.chromium.org/3009005/diff/43002/57032#newcode193
> webkit/appcache/web_application_cache_host_impl.cc:193: web_resources[i].url =
> WebURL(resource_infos[i].url);
> i thought there was an assignment operator to do the conversion w/o explicitly
> using the WebURL ctor here... is that not the case?
> 
> http://codereview.chromium.org/3009005/diff/43002/57032#newcode194
> webkit/appcache/web_application_cache_host_impl.cc:194: }
> indents are off in the body of the for loop
> 
> http://codereview.chromium.org/3009005/diff/43002/57033
> File webkit/appcache/web_application_cache_host_impl.h (right):
> 
> http://codereview.chromium.org/3009005/diff/43002/57033#newcode39
> webkit/appcache/web_application_cache_host_impl.h:39: void
OnCacheSelected(const
> appcache::AppCacheInfo& info);
> This method was made virtual a couple of CLs ago. We need to retain the
> 'virtual' keyword here.
> 
> http://codereview.chromium.org/3009005/diff/43002/57033#newcode80
> webkit/appcache/web_application_cache_host_impl.h:80: struct AppCacheInfo {
> Seems a little unfortunate to define a new 'cache info' struct here when we
have
> two to choose from. Have you considered using either a WebKit::WACH::CacheInfo
> struct here or an appache::AppCacheInfo struct here? Alternatively we could
just
> store the values we use as distinct fields here.
> 
> http://codereview.chromium.org/3009005/diff/43002/57033#newcode87
> webkit/appcache/web_application_cache_host_impl.h:87: creation_time(c),
> update_time(u), size(s) {}
> This wrapping is odd to my eye. its difficult to see where the constructor
> arguments stop and where the initializer list starts. This would be more in
> keeping with the style used in the project...
> 
>     AppCacheInfo(GURL m, base::Time c, base::Time u, long long s)
>         : manifest_url(m), creation_time(c), update_time(u), size(s) {}
> 
> Use int64 instead of long long
> The GURL arg s/b const GURL&

[michaeln, 2010-07-23 02:28:10]

glad to see this is getting wrapped up

http://codereview.chromium.org/3009005/diff/5005/2003
File chrome/browser/appcache/appcache_dispatcher_host.cc (right):

http://codereview.chromium.org/3009005/diff/5005/2003#newcode106
chrome/browser/appcache/appcache_dispatcher_host.cc:106: appcache::AppCacheInfo
info;
Can you make a default ctor for this class that initializes data members. It'd
be good to not have uninitialized values in these structures.

http://codereview.chromium.org/3009005/diff/5005/2007
File chrome/browser/browsing_data_appcache_helper.cc (right):

http://codereview.chromium.org/3009005/diff/5005/2007#newcode126
chrome/browser/browsing_data_appcache_helper.cc:126: info.update_time =
base::Time();
a default ctor might make this less busy, just need to set the manifest_url
field

http://codereview.chromium.org/3009005/diff/5005/2012
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/3009005/diff/5005/2012#newcode3176
chrome/common/render_messages.h:3176: WriteParam(m, p.update_time);
Can you also serialize/deserialize last_access_time so no data members are left
out.

http://codereview.chromium.org/3009005/diff/5005/2016
File webkit/appcache/appcache.cc (right):

http://codereview.chromium.org/3009005/diff/5005/2016#newcode25
webkit/appcache/appcache.cc:25: cache_size_(0) {
There's a gcc compile error here because the data members are being initialized
in a different order from how their defined. If you put cache_size_ above
service_ here, should be good.

http://codereview.chromium.org/3009005/diff/5005/2023
File webkit/appcache/appcache_group.h (right):

http://codereview.chromium.org/3009005/diff/5005/2023#newcode57
webkit/appcache/appcache_group.h:57: const base::Time& creation_time() const {
return creation_time_; }
void set_creation_time(const base::Time& time) {
   return creation_time_ = time;
}

http://codereview.chromium.org/3009005/diff/5005/2023#newcode125
webkit/appcache/appcache_group.h:125: const base::Time creation_time_;
maybe this shouldn't be const in light of how initial creation works

http://codereview.chromium.org/3009005/diff/5005/2028
File webkit/appcache/appcache_interfaces.h (right):

http://codereview.chromium.org/3009005/diff/5005/2028#newcode81
webkit/appcache/appcache_interfaces.h:81: };*/
remove the commented out block

http://codereview.chromium.org/3009005/diff/5005/2028#newcode87
webkit/appcache/appcache_interfaces.h:87: int host_id, const
appcache::AppCacheInfo& info) = 0;
indent two more spaces

http://codereview.chromium.org/3009005/diff/5005/2031
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/3009005/diff/5005/2031#newcode229
webkit/appcache/appcache_storage_impl.cc:229: info.update_time =
cache_record.update_time;
any other fields that we should set?

info.is_complete = true otherwise the cache would not be in the database
info.status = CACHED would be a reasonable value for that
info.cache_id = cache_record.cache_id

http://codereview.chromium.org/3009005/diff/5005/2039
File webkit/appcache/web_application_cache_host_impl.cc (right):

http://codereview.chromium.org/3009005/diff/5005/2039#newcode96
webkit/appcache/web_application_cache_host_impl.cc:96: // of cache assocition
change from something to nothing.
I don't understand why we're not updating the internal representation when
cache_id == kNoCacheId. And I think we have a way to tell webcore when the cache
association changes, client_->didChangeCacheAssociation().

Maybe like we talked about...

status_ = info.status;
has_status_ = true;
info_ = info;
client_->didChangeCacheAssociation();

http://codereview.chromium.org/3009005/diff/5005/2039#newcode187
webkit/appcache/web_application_cache_host_impl.cc:187:
WebApplicationCacheHost::CacheInfo* info) {
In webcore the equivalent method is testing for is_complete. Seems like we be
doing the same here.

http://codereview.chromium.org/3009005/diff/5005/2039#newcode198
webkit/appcache/web_application_cache_host_impl.cc:198: WebVector<ResourceInfo>*
resources) {
In webcore the equivalent method is testing for is_complete. Seems like we be
doing the same here.

[michaeln, 2010-07-26 22:41:26]

Some of these comments are minor nits, but there are a few material things to
look at too.

http://codereview.chromium.org/3009005/diff/50004/81001
File chrome/browser/appcache/appcache_dispatcher_host.cc (right):

http://codereview.chromium.org/3009005/diff/50004/81001#newcode107
chrome/browser/appcache/appcache_dispatcher_host.cc:107:
frontend_proxy_.OnCacheSelected(host_id, info);
Food for thought, looks like this could fit on a single line...
frontend_proxy_.OnCacheSelected(host_id, appcache::AppCacheInfo());

http://codereview.chromium.org/3009005/diff/50004/81011
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/3009005/diff/50004/81011#newcode606
chrome/common/render_messages_internal.h:606: appcache::AppCacheInfo /* appcache
info */)
since the type is clear, the /* appcache info* / comment isn't needed

http://codereview.chromium.org/3009005/diff/50004/81014
File webkit/appcache/appcache.cc (left):

http://codereview.chromium.org/3009005/diff/50004/81014#oldcode92
webkit/appcache/appcache.cc:92: 
maybe retain this blank line

http://codereview.chromium.org/3009005/diff/50004/81022
File webkit/appcache/appcache_host.cc (right):

http://codereview.chromium.org/3009005/diff/50004/81022#newcode445
webkit/appcache/appcache_host.cc:445: if (cache->is_complete()) {
We could set all of these fields here instead of branching. It's odd to see
cache_id being set, but not manifest_url. Also eventually i suspect the
inspector will want to represent an incomplete cache in progress.

http://codereview.chromium.org/3009005/diff/50004/81022#newcode457
webkit/appcache/appcache_host.cc:457: frontend_->OnCacheSelected(host_id_,
info);
The ctor above should have initted these fields already, so these assignments
aren't needed. Another option would be to move the 'info' into the if (cache)
block and do the following here...

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

http://codereview.chromium.org/3009005/diff/50004/81023
File webkit/appcache/appcache_host.h (right):

http://codereview.chromium.org/3009005/diff/50004/81023#newcode77
webkit/appcache/appcache_host.h:77: // Supports devtools for inspecting appcache
resources.
nit: the other comments are like "Support for xxx"

http://codereview.chromium.org/3009005/diff/50004/81024
File webkit/appcache/appcache_host_unittest.cc (left):

http://codereview.chromium.org/3009005/diff/50004/81024#oldcode283
webkit/appcache/appcache_host_unittest.cc:283: 
nit: retain the blank line

http://codereview.chromium.org/3009005/diff/50004/81025
File webkit/appcache/appcache_interfaces.h (right):

http://codereview.chromium.org/3009005/diff/50004/81025#newcode65
webkit/appcache/appcache_interfaces.h:65: last_access_time(base::Time()),
cache_id(kNoCacheId),
You shouldn't need to invoke the base::Time ctor for datamembers of that type,
the compiler with take care of that for us.

But please do init is_complete to false.

http://codereview.chromium.org/3009005/diff/50004/81028
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/3009005/diff/50004/81028#newcode231
webkit/appcache/appcache_storage_impl.cc:231: info.is_complete = true;
What do you think about setting status = CACHED here? Leaving it as UNCACHED
isn't right.

http://codereview.chromium.org/3009005/diff/50004/81030
File webkit/appcache/web_application_cache_host_impl.cc (right):

http://codereview.chromium.org/3009005/diff/50004/81030#newcode95
webkit/appcache/web_application_cache_host_impl.cc:95: // Update it.
This comment does seem useful, also, does appcache_info_ = info not work here?

http://codereview.chromium.org/3009005/diff/50004/81030#newcode101
webkit/appcache/web_application_cache_host_impl.cc:101: // Notify client of the
change.
ditto comment given how the line of code below reads

http://codereview.chromium.org/3009005/diff/50004/81030#newcode185
webkit/appcache/web_application_cache_host_impl.cc:185: // Converts to WebKit
API.
Ditto comment (and below too). Most of this file is about coverting to/from
webkit api.

http://codereview.chromium.org/3009005/diff/50004/81031
File webkit/appcache/web_application_cache_host_impl.h (right):

http://codereview.chromium.org/3009005/diff/50004/81031#newcode79
webkit/appcache/web_application_cache_host_impl.h:79: appcache::AppCacheInfo
appcache_info_;
nit: maybe cache_info_ for more brevity or associated_cache_info_ for more
descriptive

[kkanetkar, 2010-07-27 00:07:59]

Thanks for the review. I have addressed the comments. Just had 2
additional comments:

1) webkit/appcache/appcache_storage_impl.cc
I thought it didn't matter because original code relied only on time infos and
manifest. But for correctness yes. Do you think it may be anything else that
CACHED ?


2)
webkit/appcache/appcache_host.cc
Spoke offline - Crash case where owning group is null.
Separate CL for getting the info. Added TODO

On 2010/07/26 22:41:26, michaeln wrote:
> We could set all of these fields here instead of branching. It's odd to see
> cache_id being set, but not manifest_url. Also eventually i suspect the
> inspector will want to represent an incomplete cache in progress.


On 2010/07/26 22:41:26, michaeln wrote:
> What do you think about setting status = CACHED here? Leaving it as UNCACHED
> isn't right.

On Mon, Jul 26, 2010 at 3:41 PM,  <michaeln@chromium.org> wrote:
> Some of these comments are minor nits, but there are a few material things
> to
> look at too.
>
>
> http://codereview.chromium.org/3009005/diff/50004/81001
> File chrome/browser/appcache/appcache_dispatcher_host.cc (right):
>
> http://codereview.chromium.org/3009005/diff/50004/81001#newcode107
> chrome/browser/appcache/appcache_dispatcher_host.cc:107:
> frontend_proxy_.OnCacheSelected(host_id, info);
> Food for thought, looks like this could fit on a single line...
> frontend_proxy_.OnCacheSelected(host_id, appcache::AppCacheInfo());
>
> http://codereview.chromium.org/3009005/diff/50004/81011
> File chrome/common/render_messages_internal.h (right):
>
> http://codereview.chromium.org/3009005/diff/50004/81011#newcode606
> chrome/common/render_messages_internal.h:606: appcache::AppCacheInfo /*
> appcache info */)
> since the type is clear, the /* appcache info* / comment isn't needed
>
> http://codereview.chromium.org/3009005/diff/50004/81014
> File webkit/appcache/appcache.cc (left):
>
> http://codereview.chromium.org/3009005/diff/50004/81014#oldcode92
> webkit/appcache/appcache.cc:92:
> maybe retain this blank line
>
> http://codereview.chromium.org/3009005/diff/50004/81022
> File webkit/appcache/appcache_host.cc (right):
>
> http://codereview.chromium.org/3009005/diff/50004/81022#newcode445
> webkit/appcache/appcache_host.cc:445: if (cache->is_complete()) {
> We could set all of these fields here instead of branching. It's odd to
> see cache_id being set, but not manifest_url. Also eventually i suspect
> the inspector will want to represent an incomplete cache in progress.
>
> http://codereview.chromium.org/3009005/diff/50004/81022#newcode457
> webkit/appcache/appcache_host.cc:457:
> frontend_->OnCacheSelected(host_id_, info);
> The ctor above should have initted these fields already, so these
> assignments aren't needed. Another option would be to move the 'info'
> into the if (cache) block and do the following here...
>
> frontend_->OnCacheSelected(host_id_, AppCacheInfo());
>
> http://codereview.chromium.org/3009005/diff/50004/81023
> File webkit/appcache/appcache_host.h (right):
>
> http://codereview.chromium.org/3009005/diff/50004/81023#newcode77
> webkit/appcache/appcache_host.h:77: // Supports devtools for inspecting
> appcache resources.
> nit: the other comments are like "Support for xxx"
>
> http://codereview.chromium.org/3009005/diff/50004/81024
> File webkit/appcache/appcache_host_unittest.cc (left):
>
> http://codereview.chromium.org/3009005/diff/50004/81024#oldcode283
> webkit/appcache/appcache_host_unittest.cc:283:
> nit: retain the blank line
>
> http://codereview.chromium.org/3009005/diff/50004/81025
> File webkit/appcache/appcache_interfaces.h (right):
>
> http://codereview.chromium.org/3009005/diff/50004/81025#newcode65
> webkit/appcache/appcache_interfaces.h:65:
> last_access_time(base::Time()), cache_id(kNoCacheId),
> You shouldn't need to invoke the base::Time ctor for datamembers of that
> type, the compiler with take care of that for us.
>
> But please do init is_complete to false.
>
> http://codereview.chromium.org/3009005/diff/50004/81028
> File webkit/appcache/appcache_storage_impl.cc (right):
>
> http://codereview.chromium.org/3009005/diff/50004/81028#newcode231
> webkit/appcache/appcache_storage_impl.cc:231: info.is_complete = true;
> What do you think about setting status = CACHED here? Leaving it as
> UNCACHED isn't right.
>
> http://codereview.chromium.org/3009005/diff/50004/81030
> File webkit/appcache/web_application_cache_host_impl.cc (right):
>
> http://codereview.chromium.org/3009005/diff/50004/81030#newcode95
> webkit/appcache/web_application_cache_host_impl.cc:95: // Update it.
> This comment does seem useful, also, does appcache_info_ = info not work
> here?
>
> http://codereview.chromium.org/3009005/diff/50004/81030#newcode101
> webkit/appcache/web_application_cache_host_impl.cc:101: // Notify client
> of the change.
> ditto comment given how the line of code below reads
>
> http://codereview.chromium.org/3009005/diff/50004/81030#newcode185
> webkit/appcache/web_application_cache_host_impl.cc:185: // Converts to
> WebKit API.
> Ditto comment (and below too). Most of this file is about coverting
> to/from webkit api.
>
> http://codereview.chromium.org/3009005/diff/50004/81031
> File webkit/appcache/web_application_cache_host_impl.h (right):
>
> http://codereview.chromium.org/3009005/diff/50004/81031#newcode79
> webkit/appcache/web_application_cache_host_impl.h:79:
> appcache::AppCacheInfo appcache_info_;
> nit: maybe cache_info_ for more brevity or associated_cache_info_ for
> more descriptive
>
> http://codereview.chromium.org/3009005/show
>

[michaeln, 2010-07-27 19:10:50]

Lgtm

http://codereview.chromium.org/3009005/diff/48008/50034
File webkit/appcache/web_application_cache_host_impl.cc (right):

http://codereview.chromium.org/3009005/diff/48008/50034#newcode193
webkit/appcache/web_application_cache_host_impl.cc:193: // Convert
resource_infos to WebKit API.
nit: stating the obvious

http://codereview.chromium.org/3009005/diff/90001/91028
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/3009005/diff/90001/91028#newcode231
webkit/appcache/appcache_storage_impl.cc:231: info.is_complete = true;
did you intend to set status here too?