[webkit-reviews] review canceled: [Bug 24529] WebInspector: HTML5 Offline Web Applications Support (ApplicationCache) : [Attachment 60486] [PATCH] Part 2: Pulling ApplicationCache Resources to Display in the Inspector.

[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 60486: [PATCH] Part 2: Pulling ApplicationCache Resources to Display
in the Inspector.
https://bugs.webkit.org/attachment.cgi?id=60486&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
WebCore/inspector/InspectorApplicationCacheAgent.cpp:72
 +	ApplicationCacheHost* host =
m_inspectorController->inspectedPage()->mainFrame()->loader()->documentLoader()
->applicationCacheHost();
How about you cache documentLoader in a var above?

WebCore/inspector/InspectorApplicationCacheAgent.cpp:117
 +	    types.append("Master ");
Looks like localizable names to me. Todo?

WebCore/loader/appcache/ApplicationCacheHost.cpp:246
 +  void
ApplicationCacheHost::fillResourceList(InspectorApplicationCacheAgent::Resource
InfoList* resources)
This I don't like at all. I believe inspector controller functionality should
be encapsulated. We should not force instrumented code to be too involved with
the inspector controller. I'd rather make inspector controller this hosts'
friend and let it read the applicationCache by itself. Or even better, add
public applicationCacheForInspector method that would call applicationCache().
And use it from the agent. In a sense, that's what you are doing in this
method.