Bug 27821 – ApplicationCacheHost refactoring

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Michael Nordman]

This was split off of https://bugs.webkit.org/show_bug.cgi?id=25436, which was about refactoring webcore's appcache such that it could be reused in chrome. That proved to be a dead end. Went nowhere for several months and just died.

This bug is about putting in place an interface that webcore's impl and an external impl provided by chrome can live behind.

[Michael Nordman]

Created attachment 33755 [details]
class ApplicationCacheHost

This patch started out in rietveld...
http://codereview.chromium.org/160115

There is some history there (several patch sets) that may be useful for reviewers.

[Mark Rowe (bdash)]

Comment on attachment 33755 [details]
class ApplicationCacheHost

> Index: WebCore/loader/appcache/ApplicationCacheHost.cpp
> ===================================================================
> --- WebCore/loader/appcache/ApplicationCacheHost.cpp	(revision 0)
> +++ WebCore/loader/appcache/ApplicationCacheHost.cpp	(revision 0)
> @@ -0,0 +1,374 @@
> +/*
> + * Copyright (c) 2009, Google Inc.  All rights reserved.
> + * 
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + * 
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + * 
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */

This is not the right license to be using given that the file primarily contains code moved from an existing file.  You should use the same license as the file you're moving the code from.

[Michael Nordman]

(In reply to comment #2)
> This is not the right license to be using given that the file primarily
> contains code moved from an existing file.  You should use the same license as
> the file you're moving the code from.

Will do, I'll leave the Google copyright on the .h file and put the Apple copyright on the .cpp file. I have a couple other edits to make too...

* typo  in the ChangeLog to repair too.
* get rid of #include "ThreadableLoader.h" from the appcachehost.h file
* updated and merged .xcodeproj file

[Michael Nordman]

Created attachment 33759 [details]
class ApplicationCacheHost rev2

* fixed a changelog typo
* put the Apple copyright on the new .cpp file
* massaged the #includes in the new .h file
* updated and merged the .xcodeproj file

[Michael Nordman]

Created attachment 33810 [details]
class ApplicationCacheHost rev3

For whatever reason...

private:
    friend class RefCounted<DOMApplicationCache>;
    ~DOMApplicationCache() {}

... doesn't compile with msvc (the RefCounted base class does not have access to the dtor).  Removing the 'class' keyword does compile with msvc, but then gcc is unhappy about that. So... bailing on hiding the dtor, moved it to the public section (which is generally how webcore refcounted classes are).

no other changes

[Darin Fisher (:fishd, Google)]

Comment on attachment 33810 [details]
class ApplicationCacheHost rev3

> Index: WebCore/ChangeLog
...
> +        * loader/appcache/ApplicationCacheHost.cpp: Added.
> +        (WebCore::ApplicationCacheHost::ApplicationCacheHost):
> +        (WebCore::ApplicationCacheHost::~ApplicationCacheHost):
> +        (WebCore::ApplicationCacheHost::selectCacheWithoutManifest):

nit: It is encouraged to delete these lines that list functions in new
files being added.  The point, I gather, is that the function list is
most interesting when existing files are being modified.


> Index: WebCore/html/HTMLHtmlElement.cpp
...
> +    if (manifest.isNull() || manifest.isEmpty())

nit: isEmpty() is all you need since it returns true if the string is
either null or zero-length.


> Index: WebCore/loader/DocumentLoader.h
...
> +#if ENABLE(OFFLINE_WEB_APPLICATIONS)
> +        friend class ApplicationCacheHost;  // for substitute resource delivery
> +        OwnPtr<ApplicationCacheHost> m_applicationCacheHost;
>  #endif

nit: Can this just be a non-pointer member variable?  It isn't lazily allocated.


> Index: WebCore/loader/MainResourceLoader.cpp
...
> @@ -495,27 +478,15 @@ bool MainResourceLoader::load(const Reso
>  
>      m_substituteData = substituteData;
>  
> +    ResourceRequest request(r);
> +
>  #if ENABLE(OFFLINE_WEB_APPLICATIONS)
> -    // Check if this request should be loaded from the application cache
> -    if (!m_substituteData.isValid() && frameLoader()->frame()->settings() && frameLoader()->frame()->settings()->offlineWebApplicationCacheEnabled()) {
> -        ASSERT(!m_applicationCache);
> -
> -        m_applicationCache = ApplicationCacheGroup::cacheForMainRequest(r, m_documentLoader.get());
> -
> -        if (m_applicationCache) {
> -            // Get the resource from the application cache. By definition, cacheForMainRequest() returns a cache that contains the resource.
> -            ApplicationCacheResource* resource = m_applicationCache->resourceForRequest(r);
> -            m_substituteData = SubstituteData(resource->data(), 
> -                                              resource->response().mimeType(),
> -                                              resource->response().textEncodingName(), KURL());
> -        }
> -    }
> +    documentLoader()->applicationCacheHost()->maybeLoadMainResource(request, m_substituteData);
>  #endif
>  
> -    ResourceRequest request(r);
>      bool defer = defersLoading();
>      if (defer) {
> -        bool shouldLoadEmpty = shouldLoadAsEmptyDocument(r.url());
> +        bool shouldLoadEmpty = shouldLoadAsEmptyDocument(request.url());
>          if (shouldLoadEmpty)
>              defer = false;
>      }


> Index: WebCore/loader/appcache/ApplicationCacheHost.h
...
> +#include "DOMApplicationCache.h"
> +#include "KURL.h"

nit: you can probably get away with just forward declaring KURL, right?


> Index: WebCore/loader/appcache/DOMApplicationCache.cpp

> +    switch (eventType) {
> +        case CHECKING_EVENT:
> +            return eventNames().checkingEvent;
> +        case ERROR_EVENT:
> +            return eventNames().errorEvent;
> +        case NOUPDATE_EVENT:
> +            return eventNames().noupdateEvent;
> +        case DOWNLOADING_EVENT:
> +            return eventNames().downloadingEvent;
> +        case PROGRESS_EVENT:
> +            return eventNames().progressEvent;
> +        case UPDATEREADY_EVENT:
> +            return eventNames().updatereadyEvent;
> +        case CACHED_EVENT:
> +            return eventNames().cachedEvent;
> +        case OBSOLETE_EVENT:            
> +            return eventNames().obsoleteEvent;
> +    }
> +    ASSERT_NOT_REACHED();
> +    return eventNames().errorEvent;
>  }
>  
> +// static
> +DOMApplicationCache::EventType DOMApplicationCache::toEventType(const AtomicString& eventName)
>  {
> +    if (eventName == eventNames().checkingEvent)
> +        return CHECKING_EVENT;
> +    if (eventName == eventNames().errorEvent)
> +        return ERROR_EVENT;
> +    if (eventName == eventNames().noupdateEvent)
> +        return NOUPDATE_EVENT;
> +    if (eventName == eventNames().downloadingEvent)
> +        return DOWNLOADING_EVENT;
> +    if (eventName == eventNames().progressEvent)
> +        return PROGRESS_EVENT;
> +    if (eventName == eventNames().updatereadyEvent)
> +        return UPDATEREADY_EVENT;
> +    if (eventName == eventNames().cachedEvent)
> +        return CACHED_EVENT;
> +    if (eventName == eventNames().obsoleteEvent)
> +        return OBSOLETE_EVENT;
> +  
> +    ASSERT_NOT_REACHED();
> +    return ERROR_EVENT;
>  }

nit: Instead of creating a new enum type, perhaps it would have been better
to just pass an AtomicString for the event name?  What does the enum buy us
over just using the event name directly?


> Index: WebCore/page/chromium/ChromeClientChromium.h

> +#if ENABLE(OFFLINE_WEB_APPLICATIONS)
> +        // Callback invoked when the application cache fails to save a cache object
> +        // because storing it would grow the database file past its defined maximum
> +        // size or past the amount of free space on the device. 
> +        // The chrome client would need to take some action such as evicting some
> +        // old caches.
> +        virtual void reachedMaxAppCacheSize(int64_t spaceNeeded) { }
> +#endif

I don't think this change is correct or necessary.  See page/ChromeClient.h.

[Michael Nordman]

Created attachment 33843 [details]
class ApplicationCacheHost rev4

Addresses darin's review comments. About the new enum EventType, its used an a c array index (see m_attributeEventListeners), so leaving that as is.

[Michael Nordman]

> nit: It is encouraged to delete these lines that list functions in new
> files being added.  The point, I gather, is that the function list is
> most interesting when existing files are being modified.

done

> nit: isEmpty() is all you need since it returns true if the string is
> either null or zero-length.

done, thnx

> nit: Can this just be a non-pointer member variable?  It isn't lazily
> allocated.

done, involved 'forwarding' ApplicationCacheHost.hand DOMApplicationCache.h) to the webkit layer, so the xcodeproject file changed too

> nit: you can probably get away with just forward declaring KURL, right?

done, thnx

> nit: Instead of creating a new enum type, perhaps it would have been better
> to just pass an AtomicString for the event name?  What does the enum buy us
> over just using the event name directly?

leaving as is, its used as a 'c' array index too 

> I don't think this change is correct or necessary.  See page/ChromeClient.h.

done

[Darin Fisher (:fishd, Google)]

Comment on attachment 33843 [details]
class ApplicationCacheHost rev4

> Index: WebCore/loader/appcache/DOMApplicationCache.cpp
...
> +    switch (eventType) {
> +        case CHECKING_EVENT:
> +            return eventNames().checkingEvent;
> +        case ERROR_EVENT:

nit: I forgot to mention that the style guide says to not indent
the cases.  I'll take care of this before committing the patch.

R=me

[Michael Nordman]

> nit: I forgot to mention that the style guide says to not indent
> the cases.  I'll take care of this before committing the patch.

doh... and thank you!

[Darin Fisher (:fishd, Google)]

Landed as:  http://trac.webkit.org/changeset/46609

[Michael Nordman]

(4:25:14 PM) fishd: ap: can you repeat your question for michaeln's sake?
(4:25:34 PM) ap: michaeln: why was it ok to remove code from DocumentLoader::setPrimaryLoadComplete()?
(4:26:09 PM) ap: michaeln: probably not too difficult a question, but that's something that should be in ChangeLog
(4:27:28 PM) michaeln: what that code did was transfer ownership of an ApplicationCache reference from MainResourceLoader to DocumentLoader... now there is only one reference (the data member is in ApplicationCacheHOst) so no need transfer from here to there
(4:29:06 PM) ap: michaeln: it would help a lot if such things were explained in ChangeLog
(4:29:51 PM) ap: michaeln: by the way, how does ApplicationCacheHost reflect the old difference between caches owned by MainResourceLoader and DocumentLoader?
(4:30:06 PM) fishd: ap: sorry, i'll be better about enforcing that in future code reviews.
(4:31:37 PM) michaeln: ap: sorry, i'm not steeped in the culture and don't have a sense for how much detail goes in the changelog... to me this was a detail of encapsulating the inline appcache logic into the new class
(4:32:02 PM) ap: michaeln: it's really something a reviewer should notice
(4:32:14 PM) ap: michaeln: by getting confused during a deep review :)
(4:32:40 PM) ap: michaeln: I know it's hard to tell what parts of your own work may be confusing
(4:33:11 PM) michaeln: ap: there was really no difference... just a place to store the reference... when the main resource was done loading, it got transfered
(4:34:33 PM) ap: michaeln: there were some incredibly subtle dependencies on where the reference was at the moment in our code. since you didn't break any tests, you probably got that right though
(4:34:47 PM) michaeln: ap: this exchange would be good in the review log... i'll copy it over when we're done here
(4:35:25 PM) ap: michaeln: sure. I' writing up some post-commit comments in the bug
(4:35:42 PM) ap: michaeln: not trying to deeply review the logic at the moment
(4:35:48 PM) fishd: ap: this review wasn't confusing ... since it started on reitveld ;-)
(4:36:02 PM) ap: fishd: oh, niiice!
(4:36:14 PM) fishd: ap: there is a link to the reitveld history in the bug
(4:36:23 PM) michaeln: you can see the 'history' much more easily thru the patch set progression on reitveld
(4:36:35 PM) fishd: s/reitveld/rietveld/
(4:37:19 PM) fishd: ap: michael and i also sat down and poured over the code together.
(4:37:27 PM) michaeln: ap: i'll respond to comments you have there as well
(4:37:45 PM) ap: fishd: I find jumping between two review systems rather difficult. anyway, confusion and questions from review should be taken as hints that something needs to be put into ChangeLog
(4:38:51 PM) fishd: ok
(4:39:36 PM) michaeln: ap: this pretty much amounted to moving methods and method fragments out of <foo>Loader.cpp and movig them unmodified  into ApplicationCacheHost.cpp
(4:42:26 PM) ap: michaeln: by the way, the difference was:
(4:42:43 PM) ap: michaeln: m_applicationCache in MainResourceLoader was "application cache that the main resource was loaded from (if any)"
(4:43:00 PM) ap: michaeln: and the other was "appcache associated with the document"
(4:43:33 PM) ap: michaeln: when populating the cache for the first time, the main resource is loaded normally, and not from appcache
(4:44:32 PM) ap: michaeln: on the other hand, you can load a main resource that was previously cached to find that it has a different manifest in its html element's attribute
(4:44:40 PM) michaeln: ap: yes, the new ApplicatoinCacheHost has two fields... one for each you mentioned... MainResourceLoader's data member temporarily provided storage for the former
(4:45:16 PM) ap: michaeln: sounds good - I haven't read up to that file yet
(4:46:01 PM) michaeln: ap: i did not intend modify any behavior... and i dont think i have... just shuffled code around a bit
(4:46:50 PM) ap: michaeln, fishd: what I should have started with was that I really like the direction this patch goes in (not he part about separate implementations, of course)
(4:49:24 PM) michaeln: ap: i'm happy to have any path forward at this point... in hindsight, mistake on my part to pursue a common impl , shouldn't have entertained that at all... oh well... live and learn
(4:50:19 PM) ***ap respectfully disagrees that this was a mistake
(4:51:12 PM) michaeln: ap: or rather shouldn't have pursued it for months... a couple of weeks maybe... but when fruit was not being born, should have cut it off
(5:01:16 PM) michaeln: ap: i'll paste our discussion here into the bug if you don't have anymore questions/comments right now?
(5:01:27 PM) ap: ok

[Alexey Proskuryakov]

Comment on attachment 33843 [details]
class ApplicationCacheHost rev4

I'm very happy to see this refactoring - it's really the way to handle the problem at hand. Thanks!

> -#include "ApplicationCacheGroup.h"
> +#if ENABLE(OFFLINE_WEB_APPLICATIONS)
> +#include "ApplicationCacheHost.h"
> +#endif

Why does this have an #if guard? This file is not auto-generated, and t has the same guard inside. The normal style is to avoid polluting all cpp files for non-generated files. It's true that some existing files have this issue already.

> -    if (manifest.isNull())
> -        ApplicationCacheGroup::selectCacheWithoutManifestURL(document()->frame());
> +    if (manifest.isEmpty())
> +        documentLoader->applicationCacheHost()->selectCacheWithoutManifest();

Why this change? If you're changing the behavior for manifest="", please mention that in ChangeLog and add a test.

> Index: WebCore/loader/DocumentLoader.h
> ===================================================================
> --- WebCore/loader/DocumentLoader.h	(revision 46568)
> +++ WebCore/loader/DocumentLoader.h	(working copy)
> @@ -29,6 +29,10 @@
>  #ifndef DocumentLoader_h
>  #define DocumentLoader_h
>  
> +#if ENABLE(OFFLINE_WEB_APPLICATIONS)
> +#include "ApplicationCacheHost.h"
> +#endif

There is no real reason to include ApplicationCacheHost.h here - you can always make m_applicationCacheHost an OwnPtr. That would make rebuilds faster when working on appcache code, and at the same time avoid the need to make the header private.

> +        if (!documentLoader()->applicationCacheHost()->maybeLoadSynchronously(newRequest, error, response, data)) {

I'd prefer a more concrete word than "maybe". If we don't have anything concrete to say, we usually say "IfNeeded" - at least that conveys the idea that the decision is not random!

> Index: WebCore/loader/appcache/ApplicationCacheGroup.h
> ===================================================================
> --- WebCore/loader/appcache/ApplicationCacheGroup.h	(revision 46568)
> +++ WebCore/loader/appcache/ApplicationCacheGroup.h	(working copy)
> @@ -32,6 +32,7 @@
>  #include <wtf/HashMap.h>
>  #include <wtf/HashSet.h>
>  
> +#include "DOMApplicationCache.h"

It's not really appropriate to make ApplicationCacheGroup.h depend on DOM interface. It was bad enough to have a forward declaration for ListenerFunction, but bringing in the whole DOMApplicationCache.h file just invites future layering violations. We need a cleaner interface for these notifications.

>      typedef void (DOMApplicationCache::*ListenerFunction)();
> -    static void postListenerTask(ListenerFunction, const HashSet<DocumentLoader*>&);
> -    static void postListenerTask(ListenerFunction, const Vector<RefPtr<DocumentLoader> >& loaders);
> -    static void postListenerTask(ListenerFunction, DocumentLoader*);
> +    static void postListenerTask(DOMApplicationCache::EventType, const HashSet<DocumentLoader*>&);
> +    static void postListenerTask(DOMApplicationCache::EventType, DocumentLoader*);

The typedef is no longer needed.

> +    : m_DOMApplicationCache(0)

I think that m_domApplicationCache would be more common (or maybe even prescribed) style.


> +    if (!ApplicationCache::requestIsHTTPOrHTTPSGet(request))
> +        return false;

It is quite weird to call ApplicationCache to ask questions like this. Having the method as a local helper was ok, but exposing it as something for others to call really isn't.

> +bool ApplicationCacheHost::scheduleLoadFallbackResourceFromApplicationCache(ResourceLoader* loader, ApplicationCache* cache)
<...>
> +    m_documentLoader->m_pendingSubstituteResources.set(loader, resource);
> +    m_documentLoader->deliverSubstituteResourcesAfterDelay();

Do you have a plan for how to get rid of this? Having ApplicationCacheHost know about such intricate details of DocumentLoader will complicate maintenance quite a bit. Same question for other friend relationships introduced by this patch.

> Property changes on: WebCore/loader/appcache/ApplicationCacheHost.cpp
> ___________________________________________________________________
> Name: svn:executable
>    + *

Please don't.

> +bool ApplicationCacheStorage::transferApplicationCache(const String& cacheDirectory, ApplicationCacheHost* cacheHost)
> +{
> +    ApplicationCache* cache = cacheHost->applicationCache();
> +    if (!cache)
> +        return true;
> +    return ApplicationCacheStorage::storeCopyOfCache(cacheDirectory, cache);
> +}

Why a different name? Storing a copy and transferring are not the same.

> -void DOMApplicationCache::removeEventListener(const AtomicString& eventType, EventListener* eventListener, bool)
> +void DOMApplicationCache::removeEventListener(const AtomicString& eventName, EventListener* eventListener, bool)

The argument is called "type" in DOM 2 Core Events spec. It's not good to reuse the word for something else.

Having to map between enum values and event names is particularly not cool.

> +// static
> +const AtomicString& DOMApplicationCache::toEventName(EventType eventType)

We do not usually add such comments. Do you think this is useful, and should be done elsewhere?

[Michael Nordman]

> I'm very happy to see this refactoring - it's really the way to handle the
> problem at hand. Thanks!

I think this is a reasonable way to handle the problem too. Really, this is much closer to what I was shooting to do when i sent the email to the webkit list entitled "AppCache functionality provided by the embedder of webkit" on april 7. Of course that approach was summarily rejected by the webkit gatekeepers effectively blocking development of this feature for chrome for some time. I'm not clear on what the motivations truely were to have put up those roadblocks, are you? Today, here we are going down that very path.

Anyway... I'm happy enough with the conclusion... but really, the process thru which the conclusion was reached is particularly not cool. 

> > -#include "ApplicationCacheGroup.h"
> > +#if ENABLE(OFFLINE_WEB_APPLICATIONS)
> > +#include "ApplicationCacheHost.h"
> > +#endif
> 
> Why does this have an #if guard? This file is not auto-generated, and t has the
> same guard inside. The normal style is to avoid polluting all cpp files for
> non-generated files. It's true that some existing files have this issue
> already.

I saw a mix of styles. Some includes having #if guards and not others. For example, look you at the previous revisions of
* MainResourceLoader.cpp .h
* DocumentLoader.cpp
* FrameLoader.cpp

I'll be happy to remove them in a subsequent patch.

> 
> > -    if (manifest.isNull())
> > -        ApplicationCacheGroup::selectCacheWithoutManifestURL(document()->frame());
> > +    if (manifest.isEmpty())
> > +        documentLoader->applicationCacheHost()->selectCacheWithoutManifest();
> 
> Why this change? If you're changing the behavior for manifest="", please
> mention that in ChangeLog and add a test.

Good catch! I did make a material change here! When resolving "" against the document url, it will resolve to the document url (right). Its not really worthwhile to pass the documentURL in as the manifestURL.

We should get this clarified in the spec. If there should be a difference in behavior between and empty and a non-existent manifest attribute, it should be spelled out more clearly. Passing in the docURL as the manifestURL is just plain sloppy. I'll ping hixie.

WDYT?

> There is no real reason to include ApplicationCacheHost.h here - you can always
> make m_applicationCacheHost an OwnPtr. That would make rebuilds faster when
> working on appcache code, and at the same time avoid the need to make the
> header private.

Funny, I had it as an OwnPtr. Darin explicitly asked me to put the data member inline to avoid the heap alloc. It really doesn't matter to me one way or the other.
 
> I'd prefer a more concrete word than "maybe". If we don't have anything
> concrete to say, we usually say "IfNeeded" - at least that conveys the idea
> that the decision is not random!

We usually say "maybe" in chrome, seemed like it fit so i used it here. I've been using the terminology in other patches that i've been posting here for some time now too. Why didn't you say something sooner?

> It's not really appropriate to make ApplicationCacheGroup.h depend on DOM
> interface. It was bad enough to have a forward declaration for
> ListenerFunction, but bringing in the whole DOMApplicationCache.h file just
> invites future layering violations. We need a cleaner interface for these
> notifications.

I didn't introduce the dependency on DOMApplicationCache, it was already there.

ApplicationCacheHost.notifyEventListner(EventType) is a decent enough of an interface for the notifications. All we really need is a place to put the Status and EventType enumerated types such that both DOMApplicationCache and ApplicationCacheGroup can see them.

Maybe we should put these enums in ApplicationCacheHost.h

WDYT?
 
> >      typedef void (DOMApplicationCache::*ListenerFunction)();
> The typedef is no longer needed.

Thnx, i'll yank that in a subsequent change.

> > +    : m_DOMApplicationCache(0)
> I think that m_domApplicationCache would be more common (or maybe even
> prescribed) style.

k... I'll fix this in a subsequent change.

> > +    if (!ApplicationCache::requestIsHTTPOrHTTPSGet(request))
> > +        return false;
> It is quite weird to call ApplicationCache to ask questions like this. Having
> the method as a local helper was ok, but exposing it as something for others to
> call really isn't.

That ApplicationCache method was already being abused by other classes (see previous revisions of DocumentLoader and ApplicationCacheGroup). This patch didn't expose anything new.

> > +bool ApplicationCacheHost::scheduleLoadFallbackResourceFromApplicationCache(ResourceLoader* loader, ApplicationCache* cache)
> > +    m_documentLoader->m_pendingSubstituteResources.set(loader, resource);
> > +    m_documentLoader->deliverSubstituteResourcesAfterDelay();
> Do you have a plan for how to get rid of this? Having ApplicationCacheHost know
> about such intricate details of DocumentLoader will complicate maintenance
> quite a bit. Same question for other friend relationships introduced by this
> patch.

Shouldn't be a surprise that I agree the intricate dependencies are an issue.

My role here is to put in place a common interface behind which the chrome team can put an entirely different implementation of this feature set. I think the 'private' section of the newly added class is an accurate reflection of the interdepencies within the existing impl. I'm not introducing them. The authors/maintainers of the existing impl should clean that up as desired or not.

Our plan is to put #if PLATFORM(CHROMIUM) flags in the newly added ApplicationCacheHost.h file to clearly designate those parts of the new interface that actually are common and those parts that aren't. That should help with maintaining the interface that chrome depends on. The inter-dependencies in webcore's impl really isn't my gig.


> > Name: svn:executable
> >    + *
> Please don't.

oops, not sure how that got there?

> > +bool ApplicationCacheStorage::transferApplicationCache(const String& cacheDirectory, ApplicationCacheHost* cacheHost)
> Why a different name? Storing a copy and transferring are not the same.

Its a new method. Used a different name to distinquish it from the existing method is all. See the callsite in WebKit/WebDataSource.mm

> > +void DOMApplicationCache::removeEventListener(const AtomicString& eventName, EventListener* eventListener, bool)
> The argument is called "type" in DOM 2 Core Events spec. It's not good to reuse
> the word for something else.

I did so to distinquish between 'EventType' and 'AtomicString' params.

> Having to map between enum values and event names is particularly not cool.

Why is that? The numeric data type is handy. Its being used an array index for example.

toEventName(type)
There are common code use cases for this method.

toEventType(string)
We have a use case for the .toEventType(string) method in our v8 bindings, but there is no use case in common code.

I could keep the toEventName(type) method here, but move the .toEventType(str) method into a helper within our v8 bindings layer. I think its best to keep them together though.

> > +// static
> > +const AtomicString& DOMApplicationCache::toEventName(EventType eventType)
> We do not usually add such comments. Do you think this is useful, and should be
> done elsewhere?

This is an old habit. I do think its useful, there's a big difference between instance and class methods, and it bugs me that you can't tell the difference by looking at a c++ method definition :)

I'll remove the comments in a sequent patch.

[Michael Nordman]

(In reply to comment #14)
> > > -    if (manifest.isNull())
> > > -        ApplicationCacheGroup::selectCacheWithoutManifestURL(document()->frame());
> > > +    if (manifest.isEmpty())
> > > +        documentLoader->applicationCacheHost()->selectCacheWithoutManifest();
> > 
> > Why this change? If you're changing the behavior for manifest="", please
> > mention that in ChangeLog and add a test.
> 
> Good catch! I did make a material change here! When resolving "" against the
> document url, it will resolve to the document url (right). Its not really
> worthwhile to pass the documentURL in as the manifestURL.
> 
> We should get this clarified in the spec. If there should be a difference in
> behavior between and empty and a non-existent manifest attribute, it should be
> spelled out more clearly. Passing in the docURL as the manifestURL is just
> plain sloppy. I'll ping hixie.
> 
> WDYT?

Presuming we want to distinguish between 'empty' and 'non-existent' for a moment (i'm not sure we really do)...

This may be a better way to handle 'empty' manifest attribute values at this level, don't resolve it.

    // Check the manifest attribute
    AtomicString manifest = getAttribute(manifestAttr);
    if (manifest.isNull())
        documentLoader->applicationCacheHost()->selectCacheWithoutManifest();
    else
        documentLoader->applicationCacheHost()->selectCacheWithManifest(
                !manifest.isEmpty() ? document()->completeURL(manifest) : KURL());


The foriegn entry detection in ApplicationCacheGroup::selectCache(frame, manifestUrl) would have to be modified to also detect 'empty' urls.

The benefit is that the system doesn't download <html> pages as manifest files and fire requisite error onchecking and onerror events into the page.

[Alexey Proskuryakov]

(In reply to comment #14)
> Of course that approach was summarily rejected by the webkit
> gatekeepers effectively blocking development of this feature for chrome for
> some time.

I remember objections to having multiple implementations, and these still stand. Incremental refactoring for compartmentalization like we have here is good.

> I'm not clear on what the motivations truely were to have put up
> those roadblocks, are you?

Please keep Bugzilla discussions technical.

> I saw a mix of styles. Some includes having #if guards and not others. For
> example, look you at the previous revisions of
> * MainResourceLoader.cpp .h
> * DocumentLoader.cpp
> * FrameLoader.cpp
> 
> I'll be happy to remove them in a subsequent patch.

Please do - having different styles adds to confusion. I think this started with some auto-generated files not having been guarded with ifdefs, which of course broke builds where they were not generated.

> We should get this clarified in the spec. If there should be a difference in
> behavior between and empty and a non-existent manifest attribute, it should be
> spelled out more clearly. Passing in the docURL as the manifestURL is just
> plain sloppy. I'll ping hixie.
> 
> WDYT?

Clarifying this in the spec sounds good. We need to add a test for this behavior when it's settled.

> Funny, I had it as an OwnPtr. Darin explicitly asked me to put the data member
> inline to avoid the heap alloc. It really doesn't matter to me one way or the
> other.

I think that better encapsulation is more important in this case than saving one allocation (the tradeoffs would be different for a class that gets lots of instances during normal execution, obviously).

> ApplicationCacheHost.notifyEventListner(EventType) is a decent enough of an
> interface for the notifications. All we really need is a place to put the
> Status and EventType enumerated types such that both DOMApplicationCache and
> ApplicationCacheGroup can see them.
> 
> Maybe we should put these enums in ApplicationCacheHost.h
> 
> WDYT?

Sure, sounds reasonable.

> > > +    if (!ApplicationCache::requestIsHTTPOrHTTPSGet(request))
> That ApplicationCache method was already being abused by other classes (see
> previous revisions of DocumentLoader and ApplicationCacheGroup). This patch
> didn't expose anything new.

OK, still something to fix in the future.

> The authors/maintainers of the existing impl should clean that up as 
> desired or not.

I like to think of all contributors to WebKit as "maintainers" of the complete code base.

> > > +bool ApplicationCacheStorage::transferApplicationCache(const String& cacheDirectory, ApplicationCacheHost* cacheHost)
> > Why a different name? Storing a copy and transferring are not the same.
> 
> Its a new method. Used a different name to distinquish it from the existing
> method is all. See the callsite in WebKit/WebDataSource.mm

I don't understand this. At the call site, you've just replaced a call to storeCopyOfCache() with a call to transferApplicationCache(). This makes no sense.

> I did so to distinquish between 'EventType' and 'AtomicString' params.

So, this code now uses different terminology than the spec and other similar code in WebKit. Please fix that.

> This is an old habit. I do think its useful, there's a big difference between
> instance and class methods, and it bugs me that you can't tell the difference
> by looking at a c++ method definition :)

I agree that it can be helpful at times. Could be something to bring up on webkit-dev mailing list.

> I'll remove the comments in a sequent patch.

Thanks!

[Michael Nordman]

> I don't understand this. At the call site, you've just replaced a call to
> storeCopyOfCache() with a call to transferApplicationCache(). This makes no
> sense.

I can't tell what you're asking for, you seem to have something
specific in mind. If you spell it out, I'll be happy to oblige.

>> I did so to distinquish between 'EventType' and 'AtomicString' params.
> So, this code now uses different terminology than the spec and other similar
> code in WebKit. Please fix that.

k... i'll whack at the terminology.

[Darin Fisher (:fishd, Google)]

>> Funny, I had it as an OwnPtr. Darin explicitly asked me to put the data member
>> inline to avoid the heap alloc. It really doesn't matter to me one way or the
>> other.
>
>I think that better encapsulation is more important in this case than saving
>one allocation (the tradeoffs would be different for a class that gets lots of
>instances during normal execution, obviously).

It wasn't just about avoiding a new allocation as a side-effect of this refactoring, it was about simpler code.  A simple instance variable is simpler than an OwnPtr + new combo.  Reading the header suggests that the member variable's lifetime might not be the same as the ApplicationCacheHost, but that's not the case.  I didn't think the cost of a few more 'private' headers or slightly slower pre-processing time for files including ApplicationCacheHost.h would be more important than cleaner, simpler code.

[Alexey Proskuryakov]

> I didn't think the cost of a few more 'private' headers
> or slightly slower pre-processing time for files including
> ApplicationCacheHost.h would be more important than cleaner, simpler code.

This is definitely something that needs to be considered on a case by case basis, but in the past, we worked to reduce the number of headers included in Document.h, for example. DocumentLoader probably falls into the same category.

[Michael Nordman]

Created attachment 34026 [details]
Patch to address ap's comments

[Michael Nordman]

Re-opening to address some late review comments.

[Adam Barth]

Comment on attachment 33843 [details]
class ApplicationCacheHost rev4

Clearing r+ from obsolete patch to remove from pending-commit.

[Alexey Proskuryakov]

Comment on attachment 34026 [details]
Patch to address ap's comments

> +        ApplicationCacheHost is held in an OwnPtr<> by DocumentLoader.

It would be good to briefly explain why this was done, not just mention what was done. This would both provide useful information to folks skimming over commit logs, and make it easier to discover reasons for changes after svn blame. Of course, there is always a choice between having the explanations in Bugzilla and having them in ChangeLogs and having tham in code comments - I think that this case is definitely a candidate for ChangeLog.

> +        Cleanup ussage of ENABLE(xxx) around includes.

Ditto. The rest of ChangeLog comments look fine to me - we don't usually add blank lines, but it's not a big deal.

> -    class ResourceRequest;
> +    struct ResourceRequest;

There is a mismatch between platforms - in platform/mac/network/ResourceRequest.h, it's a class. I don't know why it's a struct on some other platforms, looks like a bug to me. This should be fixed, but in a separate patch.

This is a great set of improvements, r=me. Please undo the struct/class change though.

[Michael Nordman]

(In reply to comment #23)
> (From update of attachment 34026 [details])
> > +        ApplicationCacheHost is held in an OwnPtr<> by DocumentLoader.
> 
> It would be good to briefly explain why this was done, not just mention what
> was done. This would both provide useful information to folks skimming over
> commit logs, and make it easier to discover reasons for changes after svn
> blame. Of course, there is always a choice between having the explanations in
> Bugzilla and having them in ChangeLogs and having tham in code comments - I
> think that this case is definitely a candidate for ChangeLog.
> 
> > +        Cleanup ussage of ENABLE(xxx) around includes.
> 
> Ditto. The rest of ChangeLog comments look fine to me - we don't usually add
> blank lines, but it's not a big deal.

Ok... I'll poke at the changelog comments and upload a revised patch.

> 
> > -    class ResourceRequest;
> > +    struct ResourceRequest;
> 
> There is a mismatch between platforms - in
> platform/mac/network/ResourceRequest.h, it's a class. I don't know why it's a
> struct on some other platforms, looks like a bug to me. This should be fixed,
> but in a separate patch.
> 
> This is a great set of improvements, r=me. Please undo the struct/class change
> though.

Please see ResourceRequestBase, ResourceRequest is forward declared as a struct there. Every other forward declaration to ResourceRequest in the sources uses the 'struct' keyword. It was a mistake for me to have used the 'class' keyword in the new .h file.  This doesn't compile on some platforms (chromium in particular) due to the mismatch. 

I think the fix is to use 'struct' in platform/mac/network/ResourceRequest.h, and I agree that should be in a seperate patch.

[Alexey Proskuryakov]

> I think the fix is to use 'struct' in platform/mac/network/ResourceRequest.h,
> and I agree that should be in a seperate patch.

That would be most simple change sufficient to clean this up. But ResourceRequest inherits from ResourceRequestBase, which is a class, so it's totally not a struct to me.

[Michael Nordman]

Created attachment 34095 [details]
Patch to address ap's comments (rev2)

Couple of changes in this revision..

1) Restore DOMApplicationCache.h and ApplicationCacheHost.h to 'project' headers instead of 'private' headers since they're no longer in the DocumentLoader.h include graph. I had forgotten to do this in the previous revision.

2) ChangeLog wordsmithing

Also, I've opened... https://bugs.webkit.org/show_bug.cgi?id=28003... to track struct -> class cleanup around ResourceResponse.

[Alexey Proskuryakov]

Comment on attachment 34095 [details]
Patch to address ap's comments (rev2)

r=me

[Darin Fisher (:fishd, Google)]

Landed as:  http://trac.webkit.org/changeset/46786