[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 #68 from Michael Nordman <michaeln at google.com>  2010-07-07 17:27:52 PST ---
(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.

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