[webkit-reviews] review canceled: [Bug 24529] WebInspector: HTML5 Offline Web Applications Support (ApplicationCache) : [Attachment 60479] [PATCH] Part 1: Backend -> Frontend Messages. ApplicationCache Status and Connectivity Status.

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has canceled Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 24529: WebInspector: HTML5 Offline Web Applications Support
(ApplicationCache)
https://bugs.webkit.org/show_bug.cgi?id=24529

Attachment 60479: [PATCH] Part 1: Backend -> Frontend Messages.
ApplicationCache Status and Connectivity Status.
https://bugs.webkit.org/attachment.cgi?id=60479&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
Overall looks good, a handful of nits below and a single tiny concern here:
This new agent is working as a timeline (i.e. exists only when there is
front-end instance). It is different from what we use for resources. My
question is whether this agent should exist at all times and track the status
using push methods instead. That way it would be more consistent with handling
resources and such. I can imagine that current arrangement could be better for
appcache domain, just drawing this to your attention.

WebCore/WebCore.vcproj/WebCore.vcproj:50140
 +			       
RelativePath="..\inspector\InspectorApplicationCacheAgent.cpp"
We used to add js files to vcproj too.

WebCore/WebCore.xcodeproj/project.pbxproj:607
 +		24F54EAD101FE914000AE741 /* ApplicationCacheHost.h in Headers
*/ = {isa = PBXBuildFile; fileRef = 24F54EAB101FE914000AE741 /*
ApplicationCacheHost.h */; settings = {ATTRIBUTES = (Private, ); }; };
Why exporting ApplicationCacheHost?

WebCore/inspector/InspectorController.cpp:486
 +	m_frontend = new InspectorFrontend(webInspector, m_client);
I think we should swap these lines so that new front-end was created after the
frontend-lifetime agents are disposed.

WebCore/loader/appcache/ApplicationCacheGroup.cpp:386
 +	       
applicationCacheAgent->updateApplicationCacheStatus(static_cast<int>(status));
How about we include the status from the new agent instead of losing the
semantics here?

WebCore/inspector/InspectorController.h:34
 +  #include "InspectorApplicationCacheAgent.h"
You don't need to include it, forward declaration should be enough.

WebCore/loader/appcache/ApplicationCacheGroup.cpp:481
 +	    inspectorController->willSendRequest(m_currentResourceIdentifier,
handle->request(), 0);
For the redirects, we are usually expecting last parameter to present. That's
in fact how we distinguish redirects from simple requests.

WebCore/loader/appcache/ApplicationCacheGroup.cpp:502
 +		   
applicationCacheAgent->didReceiveManifestResponse(m_currentResourceIdentifier,
response);
This guy will call into didReceiveResponse, so you could try simplifying it via
calling didReceiveResponse in all cases + calling didReceiveManifestResponse
for the special ones (As I understand it is going to be handle further).

WebCore/page/Page.cpp:103
 +  #if ENABLE(INSPECTOR) && ENABLE(OFFLINE_WEB_APPLICATIONS)
How do we further encapsulate inspector complexity into the controller? I don't
like this new code in the Page. How about we add a eventNames().onlineEvent /
eventNames().offlineEvent listeners to the document instead in the new agent's
constructor and remove them in destructor? No r+ for this.

WebCore/inspector/front-end/StoragePanel.js:107
 +	    delete this._cachedApplicationCacheViewStatus;
The number of 'cache' tokens in the name is scary.

WebCore/inspector/front-end/StoragePanel.js:590
 +	    console.log("on select of appcache");
logging!