[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





--- Comment #54 from Joseph Pecoraro <joepeck at webkit.org>  2010-07-04 23:49:36 PST ---
(In reply to comment #52)
> (From update of attachment 60479 [details])
> 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.

Hmm, I haven't run into a problem. The InspectorApplicationCacheAgent does redirect
resources through InspectorController, so resources do show up. For instance if I
go to a website with a manifest, and after everything settles I open up the inspector
all the appcache resources did show up. The agent is created in "connectFrontend".
I will have to dig a little to see if that will work for all cases.

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

Done.

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

Fixed. Thanks for noticing. This was done previously for Kativa's patch,
which required it to build WebKit.


> 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.

Done.


> 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?

You mean keep the enum all the way through? I'll give that a shot.


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

Done. Thanks!


> 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).

Yes, I think this is going to be handled further. For getting the
actual cached resource data from the application cache which
is different from the usual resource cache. I'd like to leave it.
Would a FIXME be appropriate than?


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

Heh. This will need to change for multiple manifests to
something else.


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

Already removed in part 2. Missed it when I uploaded part 1.


> 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.

This is when the initial request is made. Does it make sense to
create a ResourceResponse at this point? Also, the spec currently
frowns on redirects so I don't know how useful it would be:

>      Redirects are fatal because they are either indicative of a network
>      problem (e.g. a captive portal); or would allow resources to be
>      added to the cache under URLs that differ from any URL that the
>      networking model will allow access to, leaving orphan entries; or
>      would allow resources to be stored under URLs different than their
>      true URLs. All of these situations are bad.


> 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.

So, keep as much Inspector code in WebCore/inspector as possible?
That sounds reasonable. I'll give it a shot.

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