[webkit-reviews] review denied: [Bug 42426] Implement remaining Inspector support for chrome's appcache : [Attachment 62248] patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied  review:
Bug 42426: Implement remaining Inspector support for chrome's appcache
https://bugs.webkit.org/show_bug.cgi?id=42426

Attachment 62248: patch
https://bugs.webkit.org/attachment.cgi?id=62248&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
> +++ WebKit/chromium/src/ApplicationCacheHost.cpp	(working copy)
> +    for (int i = 0; i < webResources.size(); ++i) {
> +	   resources->append(ResourceInfo(
> +	   KURL(webResources[i].url), webResources[i].isMaster,
webResources[i].isManifest, webResources[i].isFallback,
webResources[i].isForeign, webResources[i].isExplicit, webResources[i].size));
> +	   }

Whitespace issue.


> +++ WebCore/inspector/InspectorApplicationCacheAgent.h	(working copy)
>  #include "ApplicationCacheHost.h"
> -#include "KURL.h"
>  #include "PlatformString.h"
>  #include <wtf/Noncopyable.h>

Can PlatformString be removed here as well? I don't see other String usage.


>      // Backend to Frontend
>      void didReceiveManifestResponse(unsigned long identifier, const
ResourceResponse&);
> +
>      void updateApplicationCacheStatus(ApplicationCacheHost::Status);
> +
>      void updateNetworkState(bool isNowOnline);

Why the extra newlines?


> +++ WebCore/loader/appcache/ApplicationCacheHost.h	(working copy)
> +#include "KURL.h"
>      ...
>      class KURL;

Looks like the forward declaration can be removed.


> +	   struct CacheInfo {
> +	       CacheInfo(const KURL& manifest, double creationTime, double
updateTime, long long size)
> +		   : m_manifest(manifest)
> +		   , m_creationTime(creationTime)
> +		   , m_updateTime(updateTime)
> +		   , m_size(size)
> +	   {
> +	   }
> +
> +	   KURL m_manifest;
> +	   double m_creationTime;
> +	   double m_updateTime;
> +	   long long m_size;
> +    };

Whitespace issues again on the "struct" and constructor lines.

You did change the creation and update times from String to double.
I like that change. I think it would be nice to clarify this in the
ChangeLog. You didn't just "move it" you also changed it.


> +	   struct CacheInfo {
> +    struct ResourceInfo {
> +    typedef Vector<ResourceInfo> ResourceInfoList;

I think these should be moved into the ENABLE(INSPECTOR) guard.

I don't think any of these are r- worthy, but you don't have committer
rights yet. The Chromium side still needs a chromium reviewer, but it looks
straightforward.