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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jul 7 13:43:27 PDT 2010


--- Comment #65 from Joseph Pecoraro <joepeck at webkit.org>  2010-07-07 13:43:27 PST ---
(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:

            void fillResourceList(InspectorApplicationCacheAgent::ResourceInfoList*);
            InspectorApplicationCacheAgent::ApplicationCacheInfo applicationCacheInfo();

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.

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

More information about the webkit-unassigned mailing list