[Webkit-unassigned] [Bug 27821] ApplicationCacheHost refactoring

[bugzilla-daemon at webkit.org]

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


Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #33810|review?                     |review-
               Flag|                            |




--- Comment #6 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2009-07-30 14:02:40 PDT ---
(From update of attachment 33810)
> 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.

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