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

[bugzilla-daemon at webkit.org]

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





--- Comment #66 from Michael Nordman <michaeln at google.com>  2010-07-07 14:51:25 PST ---
(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.

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