WebInspector: HTML5 Offline Web Applications Support (ApplicationCache)

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[sideshowbarker]

Web Inspector could provide a direct way to view the contents of an application cache and associated manifest file. Might also make sense to provide ability to edit contents of the manifest file through Web Inspector

[David Kilzer (:ddkilzer)]

See also Item 5 on <http://lists.apple.com/archives/Safari-iPhone-Web-Dev/2009/Sep/msg00022.html>:

"5. The cache manifest functionality (which we are using to cache all the MIMES) is very very touchy. One small mistake causes an error and nothing works. The information provided to the developer one 'what went wrong' is meager at best. Not really Apples fault, but these WebKit guys need to get serious on this stuff. Getting the cache manifest stuff right was easily 60%-70% of our issues. You see a lot of demos on 'simple' applications out there but no one ever gets complex with their examples."

The developer would like a way to see which resources are NOT included in the manifest for a given page load.

[Patrick Mueller]

Wondering if we could do something in the Resources panel.  Again, this could be selector based, like All/Documents/Stylesheets selectors at the top, only show the different states of the resources in question.  Semantically this works, as long as every resource you're interested in actually shows up in the resources list on the left - not sure that's the case - specifically, resources that weren't accessed, might you want to know about them as well?

The selector-based scheme would be very useful to identify the resources by "how it was accessed", but we still probably want an indication associated with the each resource indicating how it was accessed.  

Thoughts:

- doesn't seem appropriate to add it to the resource header information available when you click on the resource - it should probably be available right in the resources list itself, or in the timeline.

- showing another icon next to the resource name seems like it's making that part of the UI too busy.  

- perhaps we could augment the file icon with a "decorator" the way Eclipse applies SCM and Java type decorators to icons.  Specifically, these are little "add-on" icons that get displayed directly to the left or right of the icon, augmenting the original.  

- Another thought would be augnmenting the timeline - perhaps put some kind of icon just before the individual timeline (or sometimes blob) indicating how the resource was retrieved.

- Another thought is to display a new type of Graph, in addition to Time and Size.  Could reuse the same values for the time graph, but use different colors indicating how the resource was accessed.

Beyond all that, it would be nice to have a summary of the type of information a developer would actually like to see.  I've played with app-cache myself, and found it unbelievably frustrating to debug - I ended up tailing my apache logs to figure out what was going on - but I don't know all the ins-and-outs of this stuff yet.  Do we want to have some special behaviour for the app-cache events, for instance?  (no clue where that might go)

[Joseph Pecoraro]

Some more ideas floating around today were:

- Show the manifest resource in the Resources Panel
- Add a new "Application Cache" section to the Storage Panel
  - have a table of the cached resources, including any info on them
    - is cached, was it CACHE / NETWORK / FALLBACK
  - [pfeldman] ability to wipe the cache resources [x] button in status bar
  - ability to reload / swap cache from a button in the status bar?
- Display certain statuses somewhere (maybe with icons like Audits)
  - Application Cache => Idle / Checking / Downloading / Error, etc
  - Connectivity => navigator.onLine or offline

[Kavita Kanetkar]

Yes these were the exact places I was thinking of putting the appcache info.
Only additional information we have is the time info such as last updated/created/accssed etc. (And ability to remove caches which you mentioned)

Also there needs to be an API for this so that we can get info from appcache asynchronously.

Should I start on the API part?

Thanks.

[Joseph Pecoraro]

(In reply to comment #4)
> Should I start on the API part?

Absolutely, do as much as you like! =)

[Patrick Mueller]

(In reply to comment #4)
> Yes these were the exact places I was thinking of putting the appcache info.
> Only additional information we have is the time info such as last
> updated/created/accssed etc. (And ability to remove caches which you mentioned)
> 
> Also there needs to be an API for this so that we can get info from appcache
> asynchronously.

Beyond just debugger support, it would be nice to see the app-cache implementation itself, as developer's see it, enhanced.  The reason we need debug support is partially because the spec is pretty thin.

For instance, error events from app-cache come back with absolutely no information.  What if we added an error code and/or message?  For progress events, what if we sent back along with the event, the URL of the resource, and actually progress numbers - which download this out of the total resource downloads, and the total number of resource downloads that WebKit is going to be making.

I don't remember if anyone's looking at enhancing the spec - though I think someone mentioned at the meeting there was some work going on here.

[Michael Nordman]

(In reply to comment #6)
> Beyond just debugger support, it would be nice to see the app-cache
> implementation itself, as developer's see it, enhanced.  The reason we need
> debug support is partially because the spec is pretty thin.
> 
> For instance, error events from app-cache come back with absolutely no
> information.  What if we added an error code and/or message?  For progress
> events, what if we sent back along with the event, the URL of the resource, and
> actually progress numbers - which download this out of the total resource
> downloads, and the total number of resource downloads that WebKit is going to
> be making.
>
> I don't remember if anyone's looking at enhancing the spec - though I think
> someone mentioned at the meeting there was some work going on here.

Adding an error message is a good idea.

The spec does call for some additional info available in the progress events thru these attribute values. These aren't implemented yet in webcore (or chrome).
- lengthComputable, should be true
- total, the number of URLs to process
- loaded, the number of URLs that have been processed (downloaded or skipped)

[Patrick Mueller]

(In reply to comment #7)
> Adding an error message is a good idea.

Except it should probably be a code, and not a message, and thus the spec should probably be updated.
 
> The spec does call for some additional info available in the progress events
> thru these attribute values. These aren't implemented yet in webcore (or
> chrome).
> - lengthComputable, should be true
> - total, the number of URLs to process
> - loaded, the number of URLs that have been processed (downloaded or skipped)

Ah!  I see those now - but I had to search the spec to find them :-)   BTW, the spec is available at the URL field value in this bug.  Why aren't such things in the IDL?  Does the IDL not have a mechanism to describe these, or is the IDL in the spec not matching up to the wording of the spec (I'm an IDL n00b) and so the IDL in the spec needs to be updated.

In any case, we need to open up another bug to get those properties added.  I'll do this if no one else has (quick search didn't find any hits)

[Michael Nordman]

(In reply to comment #8)
> (In reply to comment #7)
> > Adding an error message is a good idea.
> 
> Except it should probably be a code, and not a message, and thus the spec
> should probably be updated.

A piece of gears history, webapp developers had a hard time with diagnosing
things going wrong in the gears.LocalServer module (the moral equivalent to 
html5.AppCache). It wasn't enough to express "some URL failed to download"
in an error code. Developers needed to know which URL was causing the
manifest update to fail. In Gears we baked the URL into an error message and
actually persisted that error messsage and made it available as an attribute
on the (equivalent of the) ApplicationCache scriptable object. Something like
the .lastErrorMessage attribute.

> > The spec does call for some additional info available in the progress events
> > thru these attribute values. These aren't implemented yet in webcore (or
> > chrome).
> > - lengthComputable, should be true
> > - total, the number of URLs to process
> > - loaded, the number of URLs that have been processed (downloaded or skipped)
> 
> Ah!  I see those now - but I had to search the spec to find them :-)   BTW, the
> spec is available at the URL field value in this bug.  Why aren't such things
> in the IDL?  Does the IDL not have a mechanism to describe these, or is the IDL
> in the spec not matching up to the wording of the spec (I'm an IDL n00b) and so
> the IDL in the spec needs to be updated.

The structure of the ProgressEvent is actually defined in a separate spec
http://dev.w3.org/2006/webapi/progress/Progress.html

> In any case, we need to open up another bug to get those properties added. 
> I'll do this if no one else has (quick search didn't find any hits)

I haven't opened a webkit bug for this, but there is a chrome bug for this.
http://code.google.com/p/chromium/issues/detail?id=39370

[Patrick Mueller]

(In reply to comment #9)

> I haven't opened a webkit bug for this, but there is a chrome bug for this.
> http://code.google.com/p/chromium/issues/detail?id=39370

I opened bug 37602 to track this.

[Joseph Pecoraro]

Done with the Easy Part. The UI:
http://grab.by/3PXh

I don't think this will be a good way to show "FALLBACK" information. I'll have to experiment once I start working more with Application Cache and actually get data from the backend!

[Timothy Hatcher]

IDLE?

[Michael Nordman]

(In reply to comment #11)
> Done with the Easy Part. The UI:
> http://grab.by/3PXh
> 
> I don't think this will be a good way to show "FALLBACK" information. I'll have
> to experiment once I start working more with Application Cache and actually get
> data from the backend!

A good start. The 'type' column may need some work? Entries can be of multiple types. Really its more of a bitfield than an enumerated type. We could "dumb it down" in the UI, but not sure we should? Let's say the usual case is what your calling 'manifest' for entries listed in the CACHE section of the file. Some of those may also be 'master' entries or 'fallback' entries. We also have to represent entries listed in the NETWORK section (confusingly also referred to as the online white list in the spec).

There is also the update status value, and date/times the refer to the appcache as whole rather than individual entries. Maybe visually we could use a non scrollable pane above the scrollable list view containing the entries for this type info.

[Joseph Pecoraro]

(In reply to comment #12)
> IDLE?

applicationCache.status (expand `applicationCache` in the Console to find out more).

I think an "online / offline" indicator with just Green / Red would work great here. But seeing as it is a "status bar" showing the status here seems smart, and I think it would be useful.  One thing I've noticed in my implementation is that the status changes very quickly in the common case (Checking -> Idle). But I also haven't tested this with a "fresh" application requiring a lot of downloading which would be best to show the (Updating -> Downloading -> Update Ready) statuses. I should really get around to setting up a full test environment for this, which might be useful for manual tests.


> A good start. The 'type' column may need some work? Entries can be of multiple
> types. Really its more of a bitfield than an enumerated type. We could "dumb it
> down" in the UI, but not sure we should?

Ahh yes, you're absolutely right. I have to learn more about how WebKit's ApplicationCacheResource::Type is used. I don't know yet what happens if resource is in CACHE and is fetched from a FALLBACK source, but I intend to handle that. ApplicationCacheResource::Types are Master, Manifest, Explicit, Foreign, and Fallback. Are things similar in Chromium?


> We also have to
> represent entries listed in the NETWORK section (confusingly also referred to
> as the online white list in the spec).

Did you send an email to the whatwg mailing list? Things won't improve unless you mention it! =)
http://lists.whatwg.org/listinfo.cgi/whatwg-whatwg.org


> There is also the update status value, and date/times the refer to the appcache
> as whole rather than individual entries. Maybe visually we could use a non
> scrollable pane above the scrollable list view containing the entries for this
> type info.

That sounds good. This would be a better place to put bulk of information, instead of the status bar. Also, the date the files were cached is great information. I hope we have that in WebKit.

Do you know offhand the differences in WebKit and Chromium's implementations. Like what information you have available that I might not know about?

[Michael Nordman]

> Ahh yes, you're absolutely right. I have to learn more about how WebKit's
> ApplicationCacheResource::Type is used. I don't know yet what happens if
> resource is in CACHE and is fetched from a FALLBACK source, but I intend to
> handle that. ApplicationCacheResource::Types are Master, Manifest, Explicit,
> Foreign, and Fallback. Are things similar in Chromium?

Those categories are a matter of the HTML5 spec, so yes. Although the Chromium
port does not use the enum defined in ApplicationCacheResource.h. The only files
we share in common are
* DOMApplicationCache.idl .h .cpp
* AppliationCacheHost.h (but not .cpp!!)

When you design the interface between the inspector and the impl, please define
a new class to encapsulate that interface. In the chromium port we can provide a
different .cpp file for that interface (just as we've done with ApplicationCacheHost.cpp
in WebKit\WebKit\chromium\src)

Maybe class InspectorApplicationCacheAgent?

> Did you send an email to the whatwg mailing list? Things won't improve unless
> you mention it! =)
> http://lists.whatwg.org/listinfo.cgi/whatwg-whatwg.org

Of course, some was adopted but most was not.
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2008-August/015948.html

> Do you know offhand the differences in WebKit and Chromium's implementations.
> Like what information you have available that I might not know about?

The only difference that I can think of is that chrome keeps track of some
datetime values that webcore does not?

* creation datetime
* last update datetime
* last access datetime
  (this will be uninteresting in the 'page' inspector because the page being
   inspected will have just accessed it very recently)

[Joseph Pecoraro]

> Of course, some was adopted but most was not.
> http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2008-August/015948.html

August 2008! And no direct responses? It might be worth sending a new email now that there has been two years of experience since then. Just my 2 cents. I'll read your comments when I find some time!

[Joseph Pecoraro]

Created attachment 54232 [details]
[IMAGE] Empty Cache - Patch 1

[Joseph Pecoraro]

Created attachment 54233 [details]
[IMAGE] Items in Cache - Patch 1

[Joseph Pecoraro]

Created attachment 54234 [details]
[PATCH] WebCore Messages -> Frontend (Includes New Files)

This is a good start. Nothing very complex, but I can break this down to 2 patches if desired.

In this patch:
An InspectorApplicationCacheAgent is owned by the InspectorController and is accessible from the InspectorController with a convenience method on the Page (like InspectorTimelineAgent). Right now it receives some messages from WebCore that are sent to the front end. The messages are very simple status updates right now. Such as the applicationCache.status changes and navigator.onLine changes (online / offline events). It also allows for special handling of the manifest file. No special handling as of part 1.

Notes for reviewers:
I've attached pictures of the UI for this part of the patch. There are FIXMEs in JavaScript UI code describing all places that may need to undergo a change / discussion. Also, attachment #1 is the only possible UI after part 1. The front-end does not yet fetch resources to populate the table. However, I did attach an image of what items would look like with the state of this patch. Also, make sure I'm using OwnPtr correctly (no leaks). I have yet to find the "best" place to put initialization of the InspectorApplicationCacheAgent, but it should probably be done lazily, only if the page needs it or not.

[Joseph Pecoraro]

Looking over my own patch, I give a few parameters names, but only use them in INSPECTOR blocks. I need to add UNUSED_PARAMs in an #else block. I have fixed these locally:

>  void ApplicationCacheGroup::didReceiveData(ResourceHandle* handle, const char* data, int length, int lengthReceived)
> void ApplicationCacheGroup::didFail(ResourceHandle* handle, const ResourceError& error)

[Joseph Pecoraro]

1 last thing. I should cache the connectivity and application cache states even if the ApplicationCacheView is not the visible view. That way, when the view does become visible, it can display the correct state, instead of nothing!

[Pavel Feldman]

Comment on attachment 54234 [details]
[PATCH] WebCore Messages -> Frontend (Includes New Files)

WebCore/inspector/InspectorApplicationCacheAgent.cpp:38
 +  : m_inspectorController(inspectorController)
We used to indent these. Did it change?

WebCore/inspector/InspectorApplicationCacheAgent.cpp:54
 +      if (m_frontend)
You should limit the lifetime of this agent to the lifetime of the front-end. You would not need to check for frontend existence then. In fact I don't see where you clear this field, so the check does not work anyways.

WebCore/inspector/InspectorController.cpp:509
 +      m_applicationCacheAgent = 0;
This should instead be in the disconnectFrontend. Lifetime of this agent is similar to the InspectorDOMAgent. You could further generalize releaseDOMAgent to releaseAgents and do the tear down there. That way you can safely reference front-end from within the agent.

WebCore/inspector/InspectorController.h:192
 +      void getCookies(long callId);
(now that you touched it - it should have been called cookies back then (and the other guy rawCookies) with no get prefix.)

WebCore/inspector/front-end/ApplicationCacheItemsView.js:74
 +              this.connectivityIcon, this.connectivityMessage, this.divider,
[Online] [Idle] confuses me on the second screenshot. Should one of the statuses go to the tree view or to the table header?

WebCore/inspector/front-end/StoragePanel.js:558
 +          // as a localized string. Should it be the name of the Manifest file? Domain?
We should distinguish these by domain when we have multiple iframes loaded, right? So it should be just domain name under the Applications Cache section item? (Just as cookies?)

WebCore/loader/appcache/ApplicationCacheGroup.cpp:476
 +      // Because willSendRequest only gets called during redirects, we initialize
Is this appcache loader's specifics?

WebCore/loader/appcache/ApplicationCacheGroup.cpp:493
 +          page->inspectorController()->willSendRequest(m_currentResourceIdentifier, request, redirectResponse);
Pardon my ignorance, but what exactly this loader is loading. Options are:
1) it fetches manifest file
2) it fetches and caches resources in manifest
3) it loads cached resources mentioned in manifest

WebCore/page/Page.cpp:787
 +  InspectorApplicationCacheAgent* Page::inspectorApplicationCacheAgent() const
Given that the use of this guy is limited to the Page and AppCache class, I'd inline access path in Page and get it via controller in AppCache subsystem. In contrast, timeline has a convenience getter here because it is used all over the place in WebCore

[Timothy Hatcher]

Why does it say "manifest or domain" in the sidebar item?

Should the sidebar item even be there if the page doesn't use app cache?

We really need to hide sections/groups in the sidebar that are empty.

[Joseph Pecoraro]

(In reply to comment #22)
> We used to indent these. Did it change?

Fixed.

> You should limit the lifetime of this agent to the lifetime of the front-end.
> You would not need to check for frontend existence then. In fact I don't see
> where you clear this field, so the check does not work anyways.

Fixed but can be improved. Yes this is the lifetime of the front-end, but not necessarily the lifetime of an ApplicationCache which is not always needed.


> This should instead be in the disconnectFrontend. Lifetime of this agent is
> similar to the InspectorDOMAgent. You could further generalize releaseDOMAgent
> to releaseAgents and do the tear down there. That way you can safely reference
> front-end from within the agent.

Fixed. Changed releaseDOMAgent to releaseFrontendLifetimeAgents.


> (now that you touched it - it should have been called cookies back then (and
> the other guy rawCookies) with no get prefix.)

These changes muddy the waters of an already large patch. How about a follow up patch?


> [Online] [Idle] confuses me on the second screenshot. Should one of the
> statuses go to the tree view or to the table header?

That may be possible, I will experiment:

  - Online / Offline is the connectivity indicator. This could probably
    go someplace prominent, as it is not specifically related to the
    applicationCache.
  - IDLE / etc. is the applicationCache status. This could probably
    go into the tree view or table header.


> We should distinguish these by domain when we have multiple iframes loaded,
> right? So it should be just domain name under the Applications Cache section
> item? (Just as cookies?)

Sounds good. Not done yet.


> WebCore/loader/appcache/ApplicationCacheGroup.cpp:476
>  +      // Because willSendRequest only gets called during redirects, we
> initialize
> Is this appcache loader's specifics?

This is actually coded in ResourceHandleMac. It quick breaks, before delegating to the ResourceClient's willSendRequest. It was a check added for performance reasons due to a Leopard change in CFNetwork that started spitting out more notifications.

I think what you're asking is should the Inspector notifications in ResourceLoader do the same thing. Apparently not. I think it triggers the first on its own, separate from the ResourceClient delegates. I had tracked it down at one point, but I've since forgotten. I think it was in ResourceLoader initialization though.


>> page->inspectorController()->willSendRequest(m_currentResourceIdentifier, request, redirectResponse);
>
> Pardon my ignorance, but what exactly this loader is loading. Options are:
> 1) it fetches manifest file
> 2) it fetches and caches resources in manifest
> 3) it loads cached resources mentioned in manifest

Ahh. I actually haven't fully tested this code. My understanding of it is that it is both (1) and (2). Initially it is the manifest, and then for each resource specified to be fetched in the manifest it goes though here (serially?). You' were right to r- this, I actually haven't tested this enough. Specifically I have to test this where I actually download a number of files to see how they show up in the Resources Panel.


> Given that the use of this guy is limited to the Page and AppCache class, I'd
> inline access path in Page and get it via controller in AppCache subsystem. In
> contrast, timeline has a convenience getter here because it is used all over
> the place in WebCore

Fixed. I've removed the changes to Page.h and always go through the inspector controller.

[Joseph Pecoraro]

(In reply to comment #23)
> Why does it say "manifest or domain" in the sidebar item?

That was one of the FIXME's in the code. I think we decided on "domain" like cookies.

> > +        // FIXME: This string has not yet been decided on, and has not yet been added
> > +        // as a localized string. Should it be the name of the Manifest file? Domain?
> > +        return WebInspector.UIString("manifest or domain");


> Should the sidebar item even be there if the page doesn't use app cache?
> We really need to hide sections/groups in the sidebar that are empty.

This is another FIXME =). I will work on adding it lazily.

> > +        // FIXME: For testing purposes, always add the ApplicationCache.
> > +        this.addApplicationCache();

[Joseph Pecoraro]

> I think what you're asking is should the Inspector notifications in
> ResourceLoader do the same thing. Apparently not.

Should have been:

I think what you're asking is should the Inspector notifications in ResourceLoader need the same fix. Apparently not.

[Michael Nordman]

39 class InspectorApplicationCacheAgent : public Noncopyable {
40 public:
41     InspectorApplicationCacheAgent(InspectorController* inspectorController, InspectorFrontend* frontend);
42     ~InspectorApplicationCacheAgent();
43     
44     // Backend to Frontend
45     void didReceiveManifestResponse(unsigned long identifier, const ResourceResponse&);
46     void updateApplicationCacheStatus(int status);
47     void updateNetworkState(bool isNowOnline);
48 
49     // From Frontend
50     // Nothing yet...
51 
52 private:
53     InspectorController* m_inspectorController;
54     InspectorFrontend* m_frontend;
55 };

Thanx for putting a .h file in there for this stuff.

What's the lifetime of the inspector controller/agents compared to the frames/pages/documents that are being inspected? That would help me understand what's going here quite a lot.

I think it may be confusing to co-mingle the display of resources loaded on behalf of an update job running in the background with resources directly loaded by the page. I hope you plan on visually differentiating them in the UI. You've seperated out the call for the manifest resource having been loaded by the update job, but you haven't separated out a call for other resources being loaded by the update job. Having separate entry points into the 'inspector' would make it easier to differentiate them further downstream in the resources tab of the inspector. Have you considered adding another method for...
  void didReceiveResourceResponse(...)
... to this new class.

Also, update jobs actually can be running on behalf of several pages. Looks like the plumbing you've got only routes the response info to one of those pages, so if your looking at the 'right' inspector view, you'll see them and if not you'll be out of luck i think. I'm not sure how to reconcile that with the per-page inspector interface really, except maybe to push the response info to all of the relevant inspectors.

The manifest url should probably show up somewhere in the UI very prominently. Maybe that could be the label on the left-hand-side?

If there is no appcache, I think you can just have an empty APPLICATION CACHE section on the left-hand-side. (I think that's in keeping with how some of the others work when empty).

[Joseph Pecoraro]

(In reply to comment #27)
> Thanx for putting a .h file in there for this stuff.

Sure. It makes the most sense as soon as the frontend starts making requests for resources.


> What's the lifetime of the inspector controller/agents compared to the
> frames/pages/documents that are being inspected? That would help me understand
> what's going here quite a lot.

Pavel could best answer that. I'm still learning it myself and I would also appreciate an in depth answer! =)


> I think it may be confusing to co-mingle the display of resources loaded on
> behalf of an update job running in the background with resources directly
> loaded by the page. I hope you plan on visually differentiating them in the UI.
> You've seperated out the call for the manifest resource having been loaded by
> the update job, but you haven't separated out a call for other resources being
> loaded by the update job. Having separate entry points into the 'inspector'
> would make it easier to differentiate them further downstream in the resources
> tab of the inspector. Have you considered adding another method for...
>   void didReceiveResourceResponse(...)
> ... to this new class.

Correct. This is the area of this patch I bombed (see above comments). I also condensed most of the work I did, but not all, into this "part 1" patch. Many of the points you raised I was hoping to address in a "part 2" or later patch, but since I haven't made clear my plans it may be confusing. I also rushed to get a patch up because I had promised one this weekend and so it could give Kavita an idea of what I had done and how it could be worked with.

Briefly. In Part 1 I wanted at the very least for the Manifest file to show up in the Resources panel, as a typical resource. In a Part 2 I would start dealing with Cached Resources and I was toying with the idea of adding a new InspectorResource type / category "AppCache". That way you could filter them in the Resources Panel, and they would get their own color. At this point a separate didReceiveResourceResponse would be necessary, and the seemingly useless didReceiveManifestResponse would start to do something.


 
> Also, update jobs actually can be running on behalf of several pages. Looks
> like the plumbing you've got only routes the response info to one of those
> pages, so if your looking at the 'right' inspector view, you'll see them and if
> not you'll be out of luck i think. I'm not sure how to reconcile that with the
> per-page inspector interface really, except maybe to push the response info to
> all of the relevant inspectors.

Good point. This is a very strong argument for a "Browser Inspector" entity above a "Web Inspector". Pavel was talking about this the other day. I don't think there is currently an object that knows about multiple InspectorControllers. I could attempt such an object in the next patch.


> The manifest url should probably show up somewhere in the UI very prominently.
> Maybe that could be the label on the left-hand-side?

How about in the screenshot where I say "manifest or domain"? I agree that the manifest name would be very convenient. I will have to experiment.


> If there is no appcache, I think you can just have an empty APPLICATION CACHE
> section on the left-hand-side. (I think that's in keeping with how some of the
> others work when empty).

Yes, this was mentioned above and I plan to do that. There is already a few FIXMEs.

[Pavel Feldman]

- InspectorController's life time matches the Page (it is created in the constructor and is destroyed in the destructor I guess)
- Agents are case-by-case. InspectorDOMAgent (and this new AppCache one as well) have life time matching the front-end existence.

I don't think we need an entity that knows about multiple inspector controllers at this moment. I think we should start with the user scenarios / mocks instead. It is too easy to introduce a confusion via mixing the per-page and per-browser contents.

[Michael Nordman]

(In reply to comment #29)
> - InspectorController's life time matches the Page (it is created in the
> constructor and is destroyed in the destructor I guess)
> - Agents are case-by-case. InspectorDOMAgent (and this new AppCache one as
> well) have life time matching the front-end existence.

I see code in InspectorDOMAgent::setDocument that allows it to be used to inspect multiple documents (in series not in parallel). So I'll assume the InspectorAppCacheAgent will be used similarly to inspect the state of different documents over time. And if that's the case... a method to reset it when the document is switched might be good.

[Joseph Pecoraro]

Its been a while, but I'll outline the "parts" I had in mind for splitting this up:

Part 1: Backend Push (to Frontend)
  - create new files (requires updating all build files)
  - UI boilerplate in JavaScript front-end
  - show manifest file in Resources Panel
  - push basic AppCache events to the frontend (status and connectivity)

Part 2: Frontend Pull
  - display a table of cached resources in the frontend UI
  - ensure handled properly across refresh / page navigation
  - cleanup part 1 to only show ApplicationCache when appropriate

Follow Up Improvements (would be other bugs)
  - add buttons to the view to delete / update / etc
  - indicate / show application cache resources in the Resources Panel
  - improve graphic for ApplicationCache in Storage Panel
  - output better error messages based on what went wrong

The idea behind splitting the patches up like this would be that they
are buildable, usable, reviewable chunks. =)

I think Kavita has gone into part 2. Kavita, would it be possible to make
a couple patches along the lines above? I assume you needed to make
changes to my original patch (if you used it at all).

[Michael Nordman]

(In reply to comment #31)
> Its been a while, but I'll outline the "parts" I had in mind for splitting this up:
> 
> Part 1: Backend Push (to Frontend)
>   - create new files (requires updating all build files)
>   - UI boilerplate in JavaScript front-end
>   - show manifest file in Resources Panel
>   - push basic AppCache events to the frontend (status and connectivity)

A couple of comments/questions on the "show manifest file in Resources Panel" item.

Which instance of the manifest file? The (potentially old) manifest file, fetched from the cache, corresponding to the current version of the cache associated with the page? Or the (potentially different) manifest file, fetched from the network, in the act of updating the cache? I think you intend the latter but just want to be sure.

Fyi, the manifest file associated with a cache is always stored in that appcache as a cached resource, so it will be available for in the table of cached resources mentioned below.


> Part 2: Frontend Pull
>   - display a table of cached resources in the frontend UI
>   - ensure handled properly across refresh / page navigation
>   - cleanup part 1 to only show ApplicationCache when appropriate
> 
> Follow Up Improvements (would be other bugs)
>   - add buttons to the view to delete / update / etc
>   - indicate / show application cache resources in the Resources Panel
>   - improve graphic for ApplicationCache in Storage Panel
>   - output better error messages based on what went wrong
> 
> The idea behind splitting the patches up like this would be that they
> are buildable, usable, reviewable chunks. =)
> 
> I think Kavita has gone into part 2. Kavita, would it be possible to make
> a couple patches along the lines above? I assume you needed to make
> changes to my original patch (if you used it at all).

[Joseph Pecoraro]

> Which instance of the manifest file? The (potentially old) manifest file,
> fetched from the cache, corresponding to the current version of the
> cache associated with the page? Or the (potentially different) manifest
> file, fetched from the network, in the act of updating the cache? I think
> you intend the latter but just want to be sure.

I haven't followed changes in the spec recently, but I most certainly
mean the most up to date. Doesn't the browser, if online always? fetch
the manifest file and check:

  1. if there was an error it could be
    - 404/401 or something like that would show up as obvious
    - a parse error, the resource data would still be useful
  2. if it has changed
    - resource data would still be useful
  3. if it was byte-for-byte identical, or 304, or equivalent
    - resource data would still be useful
  4. if offline, uses the manifest
    - manifest data would be useful

So in all of these cases it looks like the best thing to do is show what
we got from the network! In the case of 3 we may have to show the
contents from a cache (like we already do for typical cached resources).

[Kavita Kanetkar]

Oh Sorry I didn't get emails about updated comments.

Yeah, I did use UI from your patch (ApplicationCacheItemsView.js).
I added the async getAppCacheInfo into it all the way to backend and controller and realized that I will need both push and pull for certain parts of appcache information.

Your data grid for resource name, size and type is good. I am not sure where in the UI should I put the actual manifest file for the page and the time information (last updated/accessed/created etc). Can I just stick this above the data grid that you create for other information?

[Joseph Pecoraro]

Kavita, do you have updated version of these patches, or should
I just pick up from the original patch I attached?

[Kavita Kanetkar]

Created attachment 60170 [details]
Just WebCore side of patch

Hello,

Please look at the WebCore side of things for integrating this. Few things to note:

There are uimplemented ApplicationCacheHost.cpp methods that I have implemented on chrome side under WebKit/chromium/src.
Please let me know if you want any changes to overall structure.

Also, I have not used Joe's ApplicationCacheInspectorAgent (all the logic is in InspectorController). I can change that if you want.

Lastly, the WebKit API side of things should that go in this bug or should I file a separate one?

Thanks!
Kavita

[Kavita Kanetkar]

Created attachment 60172 [details]
Screenshot of resource list grid

This is how the inspector grid looks (We need a new appcache icon image).

[Joseph Pecoraro]

Cool, I'll check this out later!

> Lastly, the WebKit API side of things should that go in this bug or should I file a separate one?

Probably this one. What is it for? Will something not work without it?

[Pavel Feldman]

Joe, are you looking at this?

[Joseph Pecoraro]

(In reply to comment #39)
> Joe, are you looking at this?

Yes, I promised by the end of the week.

[Kavita Kanetkar]

Created attachment 60289 [details]
WebKit API side of things.

Hello,  

I am adding the WebKit API patch to this bug as well. Joe, this was required so that inspector could talk to 
chrome's implementation of appcache.

After first round of comments, I can combine these 2 patches.

Thanks,
Kavita

[Michael Nordman]

What is the plan for handling documents with subframes, where each frame in the hierarchy may be associated with a different appcache? How will that be reflected in the UI, and how does the inspector infrastructure differentiate between happenings in different frames? If I understand the inspector UI properly, the intent is to inspect the entire frame hierarchy in the page and they all share the same InspectorController and InspectorFrontend instance... is that right?

I'd like to understand where we're headed along those lines. Consider this condition...

TopLevelDocument --> associated with appcache1
SubFrameDocument1 --> associated with appcache2
SubFrameDocument2 --> not associated with any appcache
SubFrameDocument3 --> associated with appcache1

What does the left-hand-side of the storage tab look like given that condition? I can think of two plausible outcomes, (maybe there's a third option), I just want to know where we're headed...

1) There are 2 appcache icons, one representing appcache1 and the other appcache2. So it lists distinct appcaches in use in the frame hierarchy being inspected. Clicking on an icon shows the resource list and state of that particular cache in the right-hand-side.

2) There are 3 appcache icons, one for each document in the hierarchy that is associated with an appcache. So it lists distinct appcacheHosts that are doing something with an appcache. Clicking on an icon shows the resource list and state of the cache associated with the document  in the right-hand-side.

Also, the frame/document/cache associations are not static. Subframes can come and go. Subframes can be navigated and get a whole new documents. And a document contained within a frame can call swapCache() and end up associated with a different cache.

The database system has some similarity in that multiple documents in the frame hierarchy can have the same database open (or even a single document can have the same database opened multiple times). How is that represented in the inspector?


> After first round of comments, I can combine these 2 patches.
Actually, you should probably be thinking about carving into more, not less, patches. There are bits and pieces that span areas of expertise, carving it up will help to get the right folks to review the relevant bits and pieces.

[Pavel Feldman]

We need an "App Cache" group that would contain a number of appcaches. Each of them would be identified with the frame's url.

[Michael Nordman]

(In reply to comment #43)
> We need an "App Cache" group that would contain a number of appcaches. Each of them would be identified with the frame's url.

So in the hypothetical frame hierarchy described above, how many items would you have in the left-hand-side? I think you're suggesting 3, one for each frame/doc that is associated with an appcache, and having the frame's url baked into the list item in the left-hand-side.

[Pavel Feldman]

> 
> So in the hypothetical frame hierarchy described above, how many items would you have in the left-hand-side? I think you're suggesting 3, one for each frame/doc that is associated with an appcache, and having the frame's url baked into the list item in the left-hand-side.

Correct.

[Joseph Pecoraro]

Created attachment 60474 [details]
[PATCH] Dump of All Changes - To Be Broken Down

@Reviewers: I intend to break this down into numerous parts. I'm thinking two,
because a lot is boilerplate and easy to follow, but I might make it 3 patches if
I can find a sweetspot. However, feel free to provide comments in the meantime.

@Kavita, I applied the patch locally, got it building, and made some improvements.
In order to get this building I had to do a _lot_ of work. There were a number of
files in my patch which were missing in this patch! InspectorApplicationCacheAgent
for instance. Numerous build files were neglected, which appeared in my patch,
and plenty of other small things (images, localizedStrings, ...).

One more thing. Your patch had some seriously messed up whitespace! Including
in the JavaScript file where whole sections were wildly formatted. Please be aware
of this in the future, and set your editor up to more closely match WebKit's style.

Changes you need to know:
  - moved all the new structs to InspectorApplicationCacheAgent
  - moved the "ApplicationCacheInfo" handling to InspectorApplicationCacheAgent
  - renamed some (still not all) "AppCache" and "Appcache" to "ApplicationCache"

Proposed Followup Fixes in Separate Bugs:
  - WebKit: Show ApplicationCache resource data in Resources Panel
  - WebKit: Add ApplicationCache "Creation Time" and "Update Time"
  - Show ApplicationCache resources differently in Resources Panel"
  - Better handle multiple manifests, such as multiple <iframe>s
  - Hook Up "Referesh" and "Delete" buttons in ApplicationCache DataGrid
  - Only show "APPLICATION CACHE" section in Storage when one exists
  - Refactor: drop "get" from "getCookies" and "getApplicationCache"

[Joseph Pecoraro]

Created attachment 60479 [details]
[PATCH] Part 1: Backend -> Frontend Messages. ApplicationCache Status and Connectivity Status.

I think this part is pretty stable. It suffers from a few issues
which I've noted as follow-up patches.

[Joseph Pecoraro]

Created attachment 60480 [details]
[PATCH] Part 2: Frontend pull. ApplicationCache Resource Information in a DataGrid. (needs cleanup)

This part needs cleanup. I mostly just moved Kavita's ApplicationCache items
into the InspectorApplicationCacheAgent. But I also have to move other parts
and make it more closely match other inspector messages.

[Joseph Pecoraro]

Created attachment 60481 [details]
[IMAGE] WebKit's View in Resources Panel

Pavel a Long Time ago:
> >> page->inspectorController()->willSendRequest(m_currentResourceIdentifier, request, redirectResponse);
> >
> > Pardon my ignorance, but what exactly this loader is loading. Options are:
> > 1) it fetches manifest file
> > 2) it fetches and caches resources in manifest
> > 3) it loads cached resources mentioned in manifest
> 
> Ahh. I actually haven't fully tested this code. My understanding of it is that it is both
> (1) and (2). Initially it is the manifest, and then for each resource specified to be
> fetched in the manifest it goes though here (serially?). You' were right to r- this,
> I actually haven't tested this enough. Specifically I have to test this where I actually
> download a number of files to see how they show up in the Resources Panel.

This is an image showing how this behaves. As resources are requested / downloaded
for application cache, they show up in the ResourcesPanel. This shows a page that is
updating two inner <iframe>s with manifests. This scenario was one where each
<iframe> had cached resources, I just updated both manifests and refreshed.  So all
new resources need to be fetched. I've outlined in:

  blue = normal resources on page load, these are coming from the already cached application cache
  yellow = resources downloaded via the application cache resource downloader

I think this is fine, but as mentioned the actual "Content" isn't hooked up yet. So
clicking on one of these resources only shows you the "Headers" tab.

[Joseph Pecoraro]

Created attachment 60486 [details]
[PATCH] Part 2: Pulling ApplicationCache Resources to Display in the Inspector.

Much better! Still not perfect. I also haven't tested with Fallback
URLs and stuff, or with large application caches. But I think this
is a big improvement over the last.

[Joseph Pecoraro]

Created attachment 60487 [details]
[IMAGE] What It Looks Like At the Moment

Note the sidebar item format.

  - maintitle is always the domain, like cookies
    "example.com"
  - subtitle is the manifest name and size
    "example.manifest (2.34MB)"

There is now a FIXME for Chrome to put the "creation time" and
"update time" somewhere else. I don't think it fits very well
in the sidebar, and I think the manifest file and size are
much more useful.

[Pavel Feldman]

Comment on attachment 60479 [details]
[PATCH] Part 1: Backend -> Frontend Messages. ApplicationCache Status and Connectivity Status.

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!

[Pavel Feldman]

Comment on attachment 60486 [details]
[PATCH] Part 2: Pulling ApplicationCache Resources to Display in the Inspector.

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::ResourceInfoList* 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.

[Joseph Pecoraro]

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

[Joseph Pecoraro]

Created attachment 60499 [details]
[PATCH] Part 1: Backend -> Frontend Messages. ApplicationCache Status and Connectivity Status.

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

Done.


> > 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
> 
> So, keep as much Inspector code in WebCore/inspector as possible?
> That sounds reasonable. I'll give it a shot.

Hmm, this seems like it might be more difficult than its worth. The
document will change as the page is navigated. So I would have to
keep track and keep adding event listeners to the document. I could
do something in NetworkStateNotifier, but I think the current
approach is lightweight. Its also where I would look to find the code.

[Joseph Pecoraro]

Created attachment 60500 [details]
[PATCH] Part 2: Pulling ApplicationCache Resources to Display in the Inspector.

> WebCore/inspector/InspectorApplicationCacheAgent.cpp:72
>  +      ApplicationCacheHost* host = m_inspectorController->inspectedPage()->mainFrame()->loader()->documentLoader()->applicationCacheHost();
> How about you cache documentLoader in a var above?

Done.


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

Hmm, yes I'll add a follow up patch to see if this is desired. Good catch.


> WebCore/loader/appcache/ApplicationCacheHost.cpp:246
>  +  void ApplicationCacheHost::fillResourceList(InspectorApplicationCacheAgent::ResourceInfoList* 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.
> [...] 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.

Done.


> Btw, put the Google copyright in case you are using her code

Done.

Added Kativa to the ChangeLog and added copyright to InspectorApplicationCacheAgent.cpp
for the implementation of the buildObject/Array methods, and other functions. Updated
copyright in DOMAgent.js. All other files had minor changes or were up to date.

[Pavel Feldman]

> Hmm, this seems like it might be more difficult than its worth. The
> document will change as the page is navigated. So I would have to
> keep track and keep adding event listeners to the document. I could
> do something in NetworkStateNotifier, but I think the current
> approach is lightweight. Its also where I would look to find the code.

DOMAgent could be of help here! Leaving for your consideration though.

[Joseph Pecoraro]

Landed Part 1 and 2 in:
http://trac.webkit.org/changeset/62502
http://trac.webkit.org/changeset/62503

I'll be watching the bots.

[WebKit Review Bot]

http://trac.webkit.org/changeset/62502 might have broken Qt Linux ARMv5 Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/62502
http://trac.webkit.org/changeset/62503

[WebKit Review Bot]

http://trac.webkit.org/changeset/62503 might have broken Qt Linux ARMv5 Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/62502
http://trac.webkit.org/changeset/62503

[Joseph Pecoraro]

Build Fix for Chromium: (missing forward declare)
http://trac.webkit.org/changeset/62504

Build Fix for Win & Qt: (pass blank ResourceResponse instead of 0)
http://trac.webkit.org/changeset/62505

Build Fix for GTK: (missed build file)
http://trac.webkit.org/changeset/62506

[Joseph Pecoraro]

Bots look good! There is a missing export for "Chromium Windows Release".
Does anyone know how to fix that?

The comments on this bug are already very large. I'm going to close this bug.
Lets try and move discussion to the individual bugs. If I missed any thoughts,
please file new bugs! Try to mention "Application Cache" in the title.

  > - WebKit: Show ApplicationCache resource data in Resources Panel
  https://bugs.webkit.org/show_bug.cgi?id=41633
  
  > - WebKit: Add ApplicationCache "Creation Time" and "Update Time"
  https://bugs.webkit.org/show_bug.cgi?id=41634
  
  > - Show ApplicationCache resources differently in Resources Panel"
  https://bugs.webkit.org/show_bug.cgi?id=41635
  
  > - Better handle multiple manifests, such as multiple <iframe>s
  https://bugs.webkit.org/show_bug.cgi?id=41636
  
  > - Hook Up "Refresh" and "Delete" buttons in ApplicationCache DataGrid
  https://bugs.webkit.org/show_bug.cgi?id=41637
  
  > - Only show "APPLICATION CACHE" section in Storage when one exists
  https://bugs.webkit.org/show_bug.cgi?id=41638
  
  > Looks like localizable names to me. Todo?
  https://bugs.webkit.org/show_bug.cgi?id=41639
  
  > The developer would like a way to see which resources are NOT included in the manifest for a given page load.
  https://bugs.webkit.org/show_bug.cgi?id=41640
  
  > Show Better Error Messages
  https://bugs.webkit.org/show_bug.cgi?id=41642

Some of these have "// FIXME:" comments in the source already, because
we knew about them and where they should be implemented but would
make an already large patch even larger.

@Kativa. I realize there will need to be some changes on your end
for Chromium. bug 41632 can serve as an umbrella for that. And
you can post comments there.

[Michael Nordman]

Hi guys,

I think these patches were somewhat hastily landed. We (Kavita and I) got back from a couple of days off and were surprised to find breaking changes committed.

        115	        ApplicationCache* applicationCacheForInspector() const { return applicationCache(); } 

The class ApplicationCache does not exist in the chromium port. Please remove the dependency on that class from ApplicationCacheHost public interface and from InspectorApplicationCacheAgent.

        31	#include "ApplicationCache.h" 
 	32	#include "ApplicationCacheResource.h" 

The only loader/appcache files that exist in the chromium port (prior to the introduction of these new inspector classes) are
  DOMApplicationCache .idl .h. cpp
  ApplicationCacheHost.h
Please remove these dependencies is define the interfaces required by the inspector elsewhere. Our plan was to define those interfaces on ApplicationCacheHost. Alternatively we could provide a chromium specific impl of InspectorApplicationCacheAgent in a chromium specific .cpp file (but that wasn't our first choice).


        46	    struct ApplicationCacheInfo { 
 	47	        ApplicationCacheInfo(const KURL& manifest, const String& creationTime, const String& updateTime, long long size) 
 	48	            : m_manifest(manifest) 
 	49	            , m_creationTime(creationTime) 
 	50	            , m_updateTime(updateTime) 
 	51	            , m_size(size) 
 	52	        { 
 	53	        } 
 	54	 
 	55	        KURL m_manifest; 
 	56	        String m_creationTime; 
 	57	        String m_updateTime; 
 	58	        long long m_size; 
 	59	    };

String doesn't seem like the best datatype for the datetime fields.

        1268	WebInspector._addAppCacheDomain = function(domain) 
 	1269	{ 
 	1270	    // Eliminate duplicate domains from the list. 
 	1271	    if (domain in this.applicationCacheDomains) 
 	1272	        return; 
 	1273	    this.applicationCacheDomains[domain] = true; 
 	1274	 
 	1275	    if (!this.panels.storage) 
 	1276	        return; 
 	1277	    this.panels.storage.addApplicationCache(domain); 
 	1278	}

Is the implication that there can only be one appcache for a given domain represented in the list on the right-hand-side?

[Pavel Feldman]

(In reply to comment #63)
> Hi guys,
> 
> I think these patches were somewhat hastily landed. We (Kavita and I) got back from a couple of days off and were surprised to find breaking changes committed.
> 
>         115            ApplicationCache* applicationCacheForInspector() const { return applicationCache(); } 
> 
> The class ApplicationCache does not exist in the chromium port. Please remove the dependency on that class from ApplicationCacheHost public interface and from InspectorApplicationCacheAgent.
> 

Michael, that is not how WebKit works. You can't remove random WebCore files from one of the port's builds and require that WebCore does not use them. At the moment of forking the code / excluding the files you take full responsibility for keeping your fork green.

>         31    #include "ApplicationCache.h" 
>      32    #include "ApplicationCacheResource.h" 
> 
> The only loader/appcache files that exist in the chromium port (prior to the introduction of these new inspector classes) are
>   DOMApplicationCache .idl .h. cpp
>   ApplicationCacheHost.h
> Please remove these dependencies is define the interfaces required by the inspector elsewhere. Our plan was to define those interfaces on ApplicationCacheHost. Alternatively we could provide a chromium specific impl of InspectorApplicationCacheAgent in a chromium specific .cpp file (but that wasn't our first choice).

I think it is clear that your request is not valid. Instead of investing into the diverged inspector facilities on top of diverged app cache facilities, I would fix the underlying code.

 Joe, I've heard that negotiating common app cache interface has been challenging and chromium ended up in this unpleasant forked state. Do you think you can help us fixing the underlying code and setting right abstractions in WebCore? In return we could invest into / provide decent support for the appcache in the inspector. No one wants to deal with nasty forks. See the alternate ApplicationCacheHost implementation under the WebKit/chromium/src.

[Joseph Pecoraro]

(In reply to comment #64)
> > I think these patches were somewhat hastily landed. We (Kavita and I) got back
> > from a couple of days off and were surprised to find breaking changes committed.

Sorry if they were hasty. I was anxious to get this in. The turnaround time for the
last patch was very long (4/25 -> 6/30). Working from two different sides goes
much faster if there is something checked in that can be worked on concurrently
by both. That was my intent.


> > The class ApplicationCache does not exist in the chromium port. Please remove
> > the dependency on that class from ApplicationCacheHost public interface and from
> > InspectorApplicationCacheAgent.

> > Please remove these dependencies is define the interfaces required by the
> > inspector elsewhere. Our plan was to define those interfaces on ApplicationCacheHost.
> > Alternatively we could provide a chromium specific impl of
> > InspectorApplicationCacheAgent in a chromium specific .cpp file (but that wasn't our first choice).

Originally, following Kavita's patch, I had the following in ApplicationCacheHost:
https://bugs.webkit.org/attachment.cgi?id=60486&action=prettypatch

    #if ENABLE(INSPECTOR)
            void fillResourceList(InspectorApplicationCacheAgent::ResourceInfoList*);
            InspectorApplicationCacheAgent::ApplicationCacheInfo applicationCacheInfo();
    #endif

Looking back on this, that would have satisfied both chromium and WebKit. However,
Pavel had a very good comment during review, that we should move those functions into
Inspector classes. The philosophy being keep as much of the inspector code in inspector
classes as possible, so it has the least impact on the original code as possible. I agree
with that approach where it makes sense (I didn't feel that moving the Page.cpp changes
to an Inspector file made sense because of how much it would increase complexity).

My final patch followed that and moved the functions into InspectorApplicationCacheAgent.
Unfortunately, I inlined one of these functions, but it could be turned back into a function
and be made virtual. The only exception now is that interfaces guaranteed in WebCore,
do not seem to be available in Chromium.


> > The only loader/appcache files that exist in the chromium port (prior to the introduction of these new inspector classes) are
> >   DOMApplicationCache .idl .h. cpp
> >   ApplicationCacheHost.h
> 
> I think it is clear that your request is not valid. Instead of investing into the diverged
> inspector facilities on top of diverged app cache facilities, I would fix the underlying code.
>
>  Joe, I've heard that negotiating common app cache interface has been challenging and
> chromium ended up in this unpleasant forked state. Do you think you can help us fixing
> the underlying code and setting right abstractions in WebCore? In return we could invest
> into / provide decent support for the appcache in the inspector. No one wants to deal
> with nasty forks. See the alternate ApplicationCacheHost implementation under the
> WebKit/chromium/src.

I'd be happy to help. I've been getting more familiar with ApplicationCache after doing this
work, some debugging, and some other patches. I think the first thing to do would be to
open a new bug about upstreaming / merging / changing ApplicationCache support, so
that there can be discussion. I can make sure the right people are CC'd.

Michael, I know you have a number of reasons that Chromium forked ApplicationCache,
mentioning those reasons would be important. I don't know the best approach here without
knowing the reasons behind Chromium's fork. But I think there have been other areas
where Chromium has had an implementation and gracefully upstreamed / converted
interfaces resulting in an overall improvement.

[Michael Nordman]

(In reply to comment #65)
> (In reply to comment #64)
> > > I think these patches were somewhat hastily landed. We (Kavita and I) got back
> > > from a couple of days off and were surprised to find breaking changes committed.
> 
> Sorry if they were hasty. I was anxious to get this in. The turnaround time for the
> last patch was very long (4/25 -> 6/30). Working from two different sides goes
> much faster if there is something checked in that can be worked on concurrently
> by both. That was my intent.
> 
> 
> > > The class ApplicationCache does not exist in the chromium port. Please remove
> > > the dependency on that class from ApplicationCacheHost public interface and from
> > > InspectorApplicationCacheAgent.
> 
> > > Please remove these dependencies is define the interfaces required by the
> > > inspector elsewhere. Our plan was to define those interfaces on ApplicationCacheHost.
> > > Alternatively we could provide a chromium specific impl of
> > > InspectorApplicationCacheAgent in a chromium specific .cpp file (but that wasn't our first choice).
> 
> Originally, following Kavita's patch, I had the following in ApplicationCacheHost:
> https://bugs.webkit.org/attachment.cgi?id=60486&action=prettypatch
> 
>     #if ENABLE(INSPECTOR)
>             void fillResourceList(InspectorApplicationCacheAgent::ResourceInfoList*);
>             InspectorApplicationCacheAgent::ApplicationCacheInfo applicationCacheInfo();
>     #endif
> 
> Looking back on this, that would have satisfied both chromium and WebKit. However,
> Pavel had a very good comment during review, that we should move those functions into
> Inspector classes. The philosophy being keep as much of the inspector code in inspector
> classes as possible, so it has the least impact on the original code as possible. I agree
> with that approach where it makes sense (I didn't feel that moving the Page.cpp changes
> to an Inspector file made sense because of how much it would increase complexity).
> 
> My final patch followed that and moved the functions into InspectorApplicationCacheAgent.
> Unfortunately, I inlined one of these functions, but it could be turned back into a function
> and be made virtual. The only exception now is that interfaces guaranteed in WebCore,
> do not seem to be available in Chromium.

As you noted, having the interfaces required by the inspector on ApplicationCacheHost is simple
and satisfying. That was our (Kavita and I) plan of attack because we would not have to fork
InspectorApplicationCacheAgent in any way.

We have mightily forked one class, ApplicationCacheHost. So long as the appcache related interfaces
needed by webcore (outside of the loader/appcache directory) are represented on this class, things
are honky dory.

The changes that have been committed simply break us.


> 
> > > The only loader/appcache files that exist in the chromium port (prior to the introduction of these new inspector classes) are
> > >   DOMApplicationCache .idl .h. cpp
> > >   ApplicationCacheHost.h
> > 
> > I think it is clear that your request is not valid. Instead of investing into the diverged
> > inspector facilities on top of diverged app cache facilities, I would fix the underlying code.
> >
> >  Joe, I've heard that negotiating common app cache interface has been challenging and
> > chromium ended up in this unpleasant forked state. Do you think you can help us fixing
> > the underlying code and setting right abstractions in WebCore? In return we could invest
> > into / provide decent support for the appcache in the inspector. No one wants to deal
> > with nasty forks. See the alternate ApplicationCacheHost implementation under the
> > WebKit/chromium/src.
> 
> I'd be happy to help. I've been getting more familiar with ApplicationCache after doing this
> work, some debugging, and some other patches. I think the first thing to do would be to
> open a new bug about upstreaming / merging / changing ApplicationCache support, so
> that there can be discussion. I can make sure the right people are CC'd.
> 
> Michael, I know you have a number of reasons that Chromium forked ApplicationCache,
> mentioning those reasons would be important. I don't know the best approach here without
> knowing the reasons behind Chromium's fork. But I think there have been other areas
> where Chromium has had an implementation and gracefully upstreamed / converted
> interfaces resulting in an overall improvement.

I think reworking webcore's loader/appcache to suit chrome is a big distraction
from integrating the existing impls with the inspector.

[Joseph Pecoraro]

> > Looking back on this, that would have satisfied both chromium and WebKit. However,
> > Pavel had a very good comment during review, that we should move those functions into
> > Inspector classes.
> 
> As you noted, having the interfaces required by the inspector on ApplicationCacheHost
> is simple and satisfying.

Its simple but its messy. I agree it would be a good short term solution,
but the larger problem is the way that WebKit and Chromium Application
Cache implementations are forked.


> That was our (Kavita and I) plan of attack because we would not have to fork
> InspectorApplicationCacheAgent in any way.

I thought the original plan of attack was to subclass or subvert a new class;
early on you suggested adding a new InspectorApplicationCacheAgent for this:

    > When you design the interface between the inspector and the impl, please define
    > a new class to encapsulate that interface. In the chromium port we can provide a
    > different .cpp file for that interface (just as we've done with ApplicationCacheHost.cpp
    > in WebKit\WebKit\chromium\src)
    > 
    > Maybe class InspectorApplicationCacheAgent?

I liked this solution, so I moved as much into the agent as possible. I did
however overlook the missing "ApplicationCache" class in the interface.
What would be your suggestion to improve the interface?


> > Michael, I know you have a number of reasons that Chromium forked ApplicationCache,
> > mentioning those reasons would be important. I don't know the best approach here without
> > knowing the reasons behind Chromium's fork. But I think there have been other areas
> > where Chromium has had an implementation and gracefully upstreamed / converted
> > interfaces resulting in an overall improvement.
> 
> I think reworking webcore's loader/appcache to suit chrome is a big distraction
> from integrating the existing impls with the inspector.

This sounds like a viable short term solution. But I would like to see a longer term
solution put in place. I'm willing to help there.

[Michael Nordman]

(In reply to comment #67)
> > > Looking back on this, that would have satisfied both chromium and WebKit. However,
> > > Pavel had a very good comment during review, that we should move those functions into
> > > Inspector classes.
> > 
> > As you noted, having the interfaces required by the inspector on ApplicationCacheHost
> > is simple and satisfying.
> 
> Its simple but its messy. I agree it would be a good short term solution,
> but the larger problem is the way that WebKit and Chromium Application
> Cache implementations are forked.

It simply addresses the additional design constraint that you guys neglected to deal with, namely he files in loader/appcache mostly don't exist in chrome. If you actually consider that additional constraint, i think its not messy at all. I would say that the #if CHROMIUMs that you actually have put in place are very messy.


> 
> > That was our (Kavita and I) plan of attack because we would not have to fork
> > InspectorApplicationCacheAgent in any way.
> 
> I thought the original plan of attack was to subclass or subvert a new class;
> early on you suggested adding a new InspectorApplicationCacheAgent for this:
> 
>     > When you design the interface between the inspector and the impl, please define
>     > a new class to encapsulate that interface. In the chromium port we can provide a
>     > different .cpp file for that interface (just as we've done with ApplicationCacheHost.cpp
>     > in WebKit\WebKit\chromium\src)
>     > 
>     > Maybe class InspectorApplicationCacheAgent?
> 
> I liked this solution, so I moved as much into the agent as possible. I did
> however overlook the missing "ApplicationCache" class in the interface.
> What would be your suggestion to improve the interface?

Yes, we had gravitated away from that after seeing how little was required in the ACH class. To fork another file is to fork more, not less. We came to see that InspectorApplicationCacheAgent could be shared code and were pressing down that path.

> 
> > > Michael, I know you have a number of reasons that Chromium forked ApplicationCache,
> > > mentioning those reasons would be important. I don't know the best approach here without
> > > knowing the reasons behind Chromium's fork. But I think there have been other areas
> > > where Chromium has had an implementation and gracefully upstreamed / converted
> > > interfaces resulting in an overall improvement.
> > 
> > I think reworking webcore's loader/appcache to suit chrome is a big distraction
> > from integrating the existing impls with the inspector.
> 
> This sounds like a viable short term solution. But I would like to see a longer term
> solution put in place. I'm willing to help there.

Pavel mentioned something earlier about "how WebKit works". From what I've seen in the past, patches get reverted if there are valid issues raised.

Would you consider reverting these patches. They really were not sufficiently reviewed. We haven't even gotten to the actual appcachi'ness aspects of it. The interfaces you've put in place aren't sufficient to handle the use cases. Thusfar I've just been in damage control mode because what was committed just won't function for chrome from the get go. We were really surprised to see these changes land over the holiday weekend without a chance for either of us to take a look.

I'd be more than happy to help give these changes a proper review if given the chance.

[Joseph Pecoraro]

> Yes, we had gravitated away from that after seeing how little was required in the ACH class.
> To fork another file is to fork more, not less. We came to see that InspectorApplicationCacheAgent
> could be shared code and were pressing down that path.

I was never made aware of this.


> I would say that the #if CHROMIUMs that you actually have put in place are very messy.

These were to prevent the build from breaking. Now that the build is fine, its time to
write the Chromium side of this.


> Pavel mentioned something earlier about "how WebKit works". From what I've seen in the past,
> patches get reverted if there are valid issues raised.
> [...] Would you consider reverting these patches. They really were not sufficiently reviewed.

Reverting the patches leaves me with the same problem from before.  As I understand it,
Chromium is not broken in any way. It builds and AppCache works, it just don't have support
for the inspector. It was expected (from Kativa's patch above) to land as a separate patch
anyways. I can move the functions back to ApplicationCacheHost, but in all cases Chromium
is going to have to land a separate patch.

If something is really broken, than I will consider rolling this out, if that is easier that
patching it.

I can attach a patch moving these functions to ApplicationCacheHost that you can
incorporate into a Chromium patch.

[Kavita Kanetkar]

From what I read so far, there's a disconnect in agreeing about the way to proceed. Joe, needing patches for chromium to integrate inspector and appcache is not the issue. Just that the way you moved things from ACH to agent makes it harder for us to do it (because the way ACH is the only abstraction between chrome and webkit's appcache impl).

For example:
Your agent directly calls loader/appcache code to get information. We would need to rely on ACH anyway for this information since there'e no other way to talk to chrome appcache.

Moving methods to ACH is a better approach than forking the agent. But I am ok with either of the solution.

So breakage is not build/compile breakage but a design breakage. ACH methods would have help us plug chrome side easily.

Thanks,
Kavita

[Joseph Pecoraro]

Created attachment 60825 [details]
[PATCH] Start of a Chromium Patch - Move Code Back to ApplicationCacheHost

(In reply to comment #70)
> From what I read so far, there's a disconnect in agreeing about the way to proceed.
> Joe, needing patches for chromium to integrate inspector and appcache is not the
> issue. Just that the way you moved things from ACH to agent makes it harder for us
> to do it (because the way ACH is the only abstraction between chrome and webkit's
> appcache impl).

The move was recommended by a reviewer. I happen to agree with it. But it seems
there is more to this than meets the eye. I've mentioned this is an okay short term
solution, so putting a patch up to revert this decision can be done by anyone.


> Moving methods to ACH is a better approach than forking the agent.
> But I am ok with either of the solution.

I've attached a starting patch to get the Chromium team started doing this.
But if you take this approach, please file a bug about unforking or better
abstracting the different platform application caches.

[Michael Nordman]

(In reply to comment #69)
> > Yes, we had gravitated away from that after seeing how little was required in the ACH class.
> > To fork another file is to fork more, not less. We came to see that InspectorApplicationCacheAgent
> > could be shared code and were pressing down that path.
> 
> I was never made aware of this.

It was reflected in the Kavita's patch.

> Reverting the patches leaves me with the same problem from before.

What problem is that?

> As I understand it,
> Chromium is not broken in any way. It builds and AppCache works, it just don't have support
> for the inspector. It was expected (from Kativa's patch above) to land as a separate patch
> anyways. I can move the functions back to ApplicationCacheHost, but in all cases Chromium
> is going to have to land a separate patch.

I was asking you to consider reverting not so much because of the chrome issues, that can be patched one way or the other. I'm more concerned about unasked questions regarding other aspects of the patch.

- When a page calls swapCache and gets associated with a new cache, how does the display get updated? What interface does the 'backend' call to prod the 'frontend' into realizing that something has changed? Kavita was calling this interface applicationCacheSelected() and had it as a method on the inspector controller.

- When multiple top-level pages are associated with the same appcache group, it looks like only one (actually could be none) will be notified of update status changes, what about the others? I'm not talking about the nested frames, multiple pages using the same cache. What is your plan for that?

- Can you separate populating the list view on the left-hand-side from the detailed grid view on the right-hand-side and defer fetching the resource list until needed? That could help make the interface more responsive.

- How does InspectorApplicationCacheAgent::didReceiveManifestResponse figure out which appcache/appcacheHost its being called by? Same question for all interfaces related to "pushing" info from the backend to the frontend. What is the plan for that?

- Why is 'domain' used as the label in the left-hand-side? The cookie model doesn't really apply, there can be multiple application caches per domain and in use in the same page at the same time. This real estate could probably be put to better use than having the 'domain' on display.

[Joseph Pecoraro]

> > Reverting the patches leaves me with the same problem from before.
> 
> What problem is that?

  (1) Patches that grow larger and larger in bugzilla, getting outdated, requiring rebaselines.
  (2) Multiple people working on the same code blind. Kavita was using my baseline patch,
      so any changes she made I couldn't be aware of until her patch is done.


> I was asking you to consider reverting not so much because of the chrome issues, that can be
> patched one way or the other. I'm more concerned about unasked questions regarding other
> aspects of the patch.

In the past if a feature is landing that requires a lot of parts (Timeline Panel, Audits Panel,
even the new HTML5 Parser) we have chosen to land but disable the feature, by default.
I agree that this is nowhere near complete. I would rather we turn the feature off than take
it out of the tree. Does that sound good?

It is easier to land incremental patches than to make large patches that often get out of
date when they sit on Bugzilla. Breaking pieces like this down into separate patches also
makes it easier to review.

Your observations and concerns are very good. I'd suggest filing bugs with "Application
Cache" in the title.


> - When a page calls swapCache and gets associated with a new cache. [...]

Currently it does not. This would be a good incremental patch. 


> - Can you separate populating the list view on the left-hand-side from
> the detailed grid view on the right-hand-side and defer fetching the
> resource list until needed? That could help make the interface more responsive.

This is how it works right now. When you select the ApplicationCache from
the left-hand-side it then fetches the data lazily.


> - When multiple top-level pages are associated with the same appcache group, it looks
> like only one (actually could be none) will be notified of update status changes, what
> about the others? I'm not talking about the nested frames, multiple pages using the
> same cache. What is your plan for that?

> - How does InspectorApplicationCacheAgent::didReceiveManifestResponse figure out
> which appcache/appcacheHost its being called by? Same question for all interfaces
> related to "pushing" info from the backend to the frontend. What is the plan for that?
> 
> - Why is 'domain' used as the label in the left-hand-side? The cookie model doesn't
> really apply, there can be multiple application caches per domain and in use in the
> same page at the same time. This real estate could probably be put to better use than
> having the 'domain' on display.

These all deal with multiple application caches, or multiple domains. This initial patch
doesn't deal with this, and admittedly handles it wrong right now. I didn't intend to have
this working in the first patch, and I took what Kavita had in her patch regarding domains
and left it unchanged. I see its behavior is wrong. I filed a bug:
https://bugs.webkit.org/show_bug.cgi?id=41636

[Pavel Feldman]

Reading through the thread, I can say few things:
- It is a shame ApplicationCache.h was used, but there was really no sign or comment preventing it from being used. I know it was a tough decision and there probably is a handful of people involved in that decision that know that the Host is the only touchable thing. There is no one to blame here.
- It is a shame that the patch was compiled from parts of Kavita work - we all like when the credit is shared fairly.
- However, I agree with Joe that we can't land when everything is ready. Implementation is incremental and as long as it does not break the build (is hidden), we are good with it.

So please, lets get back to constructive way of going further.

Here is what I would suggest:
1) For all the items Michael enumerated in #72, please file the bugs
2) Lets hide appcache from the UI unless it is ready and both parities are happy with it
3) Lets fix chromium fast so that we could iterate against working appcache inspection
4) Lets make sure ApplicationCacheHost.h has a nice interface that Inspector could talk to and move ApplicationCache.h (and rest of the implementation classes) to under WebCore/loader/appcache/internal.

[Michael Nordman]

> It is easier to land incremental patches.

And so long as we share a reasonably common idea of where we're going, its easier to have multiple people landing patches incrementally too!

Please understand where I'm coming from. I'm not saying it has to be landed in it final form on day one. What I am suggesting is that there is a reasonably clear idea of both what the feature set is and roughly how the code will be arranged to accomplish that prior to landing significant patches. And that we have a common understanding of those things.

My most recent questions (#72) were intended to ferret out that reasonably clear idea of "how". My earlier questions in response to Kavita's patch (#42) were to ferret out that reasonably clear idea of "what".

(In reply to comment #74)
> Reading through the thread, I can say few things:
> - It is a shame ApplicationCache.h was used, but there was really no sign or comment preventing it from being used.

I have tried to communicate this whenever i see changes involving appcache, in this bug see comment #15.

> Here is what I would suggest:
> 1) For all the items Michael enumerated in #72, please file the bugs
> 2) Lets hide appcache from the UI unless it is ready and both parities are happy with it
> 3) Lets fix chromium fast so that we could iterate against working appcache inspection

Kavita's got some work in progress for the chrome side of things and for the WebKit API layer.
http://codereview.chromium.org/2821005/show
http://codereview.chromium.org/2804026/show
It would be good to get those put to bed, so we have real data to provide to the inspector agent. There's nothing blocking progress on these. Until we have a reasonable agent interface to talk to, we can just drop whatever data comes in on the floor for now.

> 4) Lets make sure ApplicationCacheHost.h has a nice interface that Inspector could talk to and move ApplicationCache.h (and rest of the implementation classes) to under WebCore/loader/appcache/internal.

Oooo... relocating stuff to /loader/appcache/internal is a very good idea!

5) Think thru, agree upon, and land the common interfaces sooner rather than later. Once these interfaces are in place, things can go on in parallel more smoothly. In this case the interfaces of interest are:
- The user interface.
- Those methods of class InspectorApplicationCacheAgent intended to be called by appcache/internal to "push" stuff.
- Those methods of class ApplicationCacheHost intended to be called by the InspectorApplicationCacheAgent to "pull" stuff.

Probably would have been good to have just those interfaces go in first. Without code to "push" stuff and returning null in response to "pull" stuff. (Its not too late to revert and do just that).

The "push" oriented methods of InspectorApplicationCacheAgent may want to take an ApplicationCacheHost parameter to the inspector side of things can distinguish between events and state changes in different subframes. Per the earlier discussion, in the UI we eventually want 1 icon per subframe that's actually associated with an appcache (right?).

6) Give people ample opportunity to review patches.

>> - When multiple top-level pages are associated with the same appcache group, it looks
>> like only one (actually could be none) will be notified of update status changes, what
>> about the others? I'm not talking about the nested frames, multiple pages using the
>> same cache. What is your plan for that?
>
> These all deal with multiple application caches

This one in particular is a little different. It doesn't have anything todo with multiple application cache instances. Rather multiple documents associated to the same cache. They way you're plumbing update status changes from the ApplicationCacheGroup to the InspectorAgent is flawed. Take a look at how m_frame is used. It only gets you to the context that initiated the update. It might be better to siphon off these state changes to the agent where the events are fired into the DOM ... see ApplicationCacheHost::notifyDOMApplicationCache.

>> - Can you separate populating the list view on the left-hand-side from
>> the detailed grid view on the right-hand-side and defer fetching the
>> resource list until needed? That could help make the interface more responsive.
>
>This is how it works right now. When you select the ApplicationCache from
>the left-hand-side it then fetches the data lazily.

This method looks like it returns info used to populate the list on the left as well as info used to populate the grid. So then what is the basis for the list on the left-hand-side prior to lazily fetching? How do you know whether or not to put an icon there and what labels to put on that icon? Or do you just always plot an icon with generic labeling on it?

[Kavita Kanetkar]

Created attachment 60992 [details]
chrome-side changes for resource list

Changes: Fills in resource list for main frame's appcache resources for chrome. Contains WebKit API side too.

Please take a look.

Thanks!

[Joseph Pecoraro]

Comment on attachment 60992 [details]
chrome-side changes for resource list

> +                'public/WebApplicationCacheResourceInfoList.h'

I don't see this file. I saw it in an older patch. Is it being added?


> +++ WebKit/chromium/src/ApplicationCacheHost.cpp	(working copy)
> +#include "InspectorApplicationCacheAgent.h"

Hmm, I had this in my starter patch but I don't think its needed. This
includes ApplicationCacheHost.h which includes that header. You may be
able to remove it.


> +        for (size_t i = 0; i < webResources.size(); ++i) {
> +        // Convert results from webResources to resources.
> +        resources->append(InspectorApplicationCacheAgent::ResourceInfo(
> +            KURL(webResources[i].m_resource), webResources[i].m_isMaster, webResources[i].m_isManifest, webResources[i].m_isFallback,
> +                 webResources[i].m_isForeign, webResources[i].m_isExplicit, webResources[i].m_size));
> +        }

Style: Indent the contents of a for loop. So the comment and resources->append statements.


> +++ WebKit/chromium/ChangeLog	(working copy)
> @@ -1,3 +1,19 @@
> [phone]  Kavita Kanetkar  <[e-mail]>
> +        (WebCore::ApplicationCacheHost::applicationCacheInfo): Unimplimented.

Typo: "Unimplimented" => "Unimplemented"


> +++ WebCore/ChangeLog	(working copy)
> +        No new tests. (OOPS!)

This line can be removed, or expanded to say nothing has changed.
Might be worth adding a comment as well. Like "Moving functions
from InspectorApplicationCacheAgent to ApplicationCacheHost to
make it easier for Chromium to provide implementations."


> +++ WebCore/inspector/front-end/ApplicationCacheItemsView.js	(working copy)
> @@ -139,7 +139,8 @@
>          this._resources = applicationCaches.resources;
>          var lastPathComponent = applicationCaches.lastPathComponent;
>  
> -        if (!this._manifest) {
> +        // FIXME: Change this to check manifest instead of resources.
> +        if (!this._resources.length) {

Yes, this FIXME is important because this makes it so a valid manifest without
resources will show the "No AppCache" message. Any idea what it will take
to include an implementation of applicationCacheInfo()? It was in your older
patch.


> +++ WebCore/loader/appcache/ApplicationCacheHost.cpp	(working copy)
> +InspectorApplicationCacheAgent::ApplicationCacheInfo ApplicationCacheHost::applicationCacheInfo()
> +{
> +    ApplicationCache* cache = applicationCache();
> +        if (!cache || !cache->isComplete())
> +            return InspectorApplicationCacheAgent::ApplicationCacheInfo(KURL(), String(), String(), 0);

Style: If statement and body are double indented.

This is another weird whitespace regression in one of your patches =).
Any idea why the formatting changed?


> +++ WebCore/loader/appcache/ApplicationCacheHost.h	(working copy)
>  #if ENABLE(OFFLINE_WEB_APPLICATIONS)
> -
> +#include "InspectorApplicationCacheAgent.h"

Style: I think we want to leave that blank line in.


>          void stopDeferringEvents(); // Also raises the events that have been queued up.
> +#if ENABLE(INSPECTOR)
> +        void fillResourceList(InspectorApplicationCacheAgent::ResourceInfoList*);
> +        InspectorApplicationCacheAgent::ApplicationCacheInfo applicationCacheInfo();
> +#endif

Style: Same here, I think there should be a blank line before and the #if.

[WebKit Review Bot]

Attachment 60992 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3430046

[Michael Nordman]

Comment on attachment 60992 [details]
chrome-side changes for resource list

WebCore/inspector/InspectorApplicationCacheAgent.h:87
 +      void updateApplicationCacheStatus(int status);
I think the enumerated type here was better.

WebCore/inspector/front-end/ApplicationCacheItemsView.js:143
 +          if (!this._resources.length) {
Sounds like this *should* be testing the _manifest attribute, lets just leave it that way. I think your making this change in case getResourceList() has returned something but getAppCacheInfo() hasn't, is that right? But once these interfaces are working, that isn't a condition that should arise. Maybe there's some other reason?

WebCore/loader/appcache/ApplicationCacheHost.cpp:283
 +          if (!cache || !cache->isComplete())
this indent is off

WebKit/chromium/public/WebApplicationCacheHost.h:85
 +      virtual void getResourceList(WebApplicationCacheResourceInfoList* resources) { };
How about we put the webkit api changes can be in a separate patch so we get the right eyes to look at that one and to cover the cases we can reasonably anticipate.
new incoming interface on WebApplicationCacheHostClient
- onCacheSelected
new outgoing interfaces on WebApplicationCacheHost
- getResources
- deleteCacheGroup  // pretty clear we'll want this


WebKit/chromium/WebKit.gyp:87
 +                  'public/WebApplicationCacheResourceInfoList.h'
seperate patch?

WebKit/chromium/src/ApplicationCacheHost.cpp:239
 +      }
We should just put a FIXME here for now. When we have the webkit api in place and functional enough (on the chrome side) to not break the UI, we can return real data to populate the UI.

[Joseph Pecoraro]

(In reply to comment #79)
> (From update of attachment 60992 [details])
> WebCore/inspector/InspectorApplicationCacheAgent.h:87
>  +      void updateApplicationCacheStatus(int status);
> I think the enumerated type here was better.

The enum would be nice. Is there a way to forward declare either a
class's struct or a class's enum? The reason I changed this back to
int was because:

  ApplicationCacheHost includes InspectorApplicationCacheAgent.h for the
  InspectorApplicationCacheAgent:: ResourceInfoList and ApplicationCacheInfo types.

  If InspectorApplicationCacheAgent were to include ApplicationCacheHost.h for the
  ApplicationCacheHost::Status enum type, there would be a circular dependency
  that would cause problems building these files.

I tried some obvious ideas, but they didn't work for me.

[Joseph Pecoraro]

> I tried some obvious ideas, but they didn't work for me.

I know I could have moved the enum out to a separate file, but I figured that
would be more complex then necessary. After all, the enum becomes an int
eventually.

[Michael Nordman]

(In reply to comment #81)
> > I tried some obvious ideas, but they didn't work for me.
> 
> I know I could have moved the enum out to a separate file, but I figured that
> would be more complex then necessary. After all, the enum becomes an int
> eventually.

We could define the ResourceInfoList and ApplicationCacheInfo types in ApplicationCacheHost.h.

[Kavita Kanetkar]

That's how I had it originally. Should I move it there and use enum instead of an int?

Thanks,
Kavita.

[Michael Nordman]

(In reply to comment #83)
> That's how I had it originally. Should I move it there and use enum instead of an int?

That would be my vote.

[Kavita Kanetkar]

Created attachment 61099 [details]
patch

Joe, can we land your patch ? This bug can be closed then. I will open new WebKit API bug for tracking other changes.

Thanks,
Kavita

[Joseph Pecoraro]

(In reply to comment #83)
> That's how I had it originally. Should I move it there and use enum instead of an int?

I'd rather not put all the code in ApplicationCacheHost, just the interface would be nice.
The enum turns into an int eventually, so I think its fine here. Should we do a
static_cast<int> to make it clearer?


(In reply to comment #85)
> Joe, can we land your patch? This bug can be closed then. I will open new WebKit
> API bug for tracking other changes.

What is wrong with your patch?

It looks like you just put up my patch, with ChangeLogs. You are probably missing
something, like the this.installInspectorControllerDelegate_("getApplicationCaches")
in WebKit/chromium/src/js/InspectorControllerImpl.js.

[Michael Nordman]

Comment on attachment 61099 [details]
patch

removing review flags from this patch, a new bug has been opened for the chromium bits

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