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

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Kavita Kanetkar
<kkanetkar at chromium.org>'s request for review:
Bug 42426: Implement remaining Inspector support for chrome's appcache
https://bugs.webkit.org/show_bug.cgi?id=42426

Attachment 61760: correct patch3
https://bugs.webkit.org/attachment.cgi?id=61760&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
Looks promising. r- for build failure.

WebKit/chromium/src/ApplicationCacheHost.cpp:223
 +	if (m_internal) {
Nit: I prefer guards to conditions in such cases:
if (!m_internal)
    return;
...

WebKit/chromium/src/ApplicationCacheHost.cpp:229
 +		KURL(webResources[i].url), webResources[i].isMaster,
webResources[i].isManifest, webResources[i].isFallback,
webResources[i].isForeign,
poor indentation.

WebCore/ChangeLog:5
 +	    Implement remaining Inspector support for chrome's appcache
You might want to add a note relavant to WebCore here (such as "Move
fillResourceList into the ApplicationCacheHost")

WebCore/inspector/InspectorApplicationCacheAgent.h:87
 +	void updateApplicationCacheStatus(int status);
Somewhere we need to require that this int is in sync with the frontend.

WebCore/loader/appcache/ApplicationCacheHost.cpp:257
 +	if (!cache || !cache->isComplete())
ASSERT was replaced with the guard. Which is right? Or does it depend on the
host browser?