[webkit-reviews] review denied: [Bug 27821] ApplicationCacheHost refactoring : [Attachment 33810] class ApplicationCacheHost rev3

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Michael Nordman
<michaeln at google.com>'s request for review:
Bug 27821: ApplicationCacheHost refactoring
https://bugs.webkit.org/show_bug.cgi?id=27821

Attachment 33810: class ApplicationCacheHost rev3
https://bugs.webkit.org/attachment.cgi?id=33810&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> 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.