[Webkit-unassigned] [Bug 24529] WebInspector: HTML5 Offline Web Applications Support (ApplicationCache)

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=24529


Pavel Feldman <pfeldman at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #60479|review?                     |
               Flag|                            |




--- Comment #52 from Pavel Feldman <pfeldman at chromium.org>  2010-07-04 22:45:57 PST ---
(From update of attachment 60479)
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!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.