Expose details for appcache error events [Chromium]

webkit/browser/appcache/appcache_update_job.cc

Index: webkit/browser/appcache/appcache_update_job.cc
diff --git a/webkit/browser/appcache/appcache_update_job.cc b/webkit/browser/appcache/appcache_update_job.cc
index 03bb909cf09e64bca4e879a469299410c2597c53..f7f0881cca0e7a4d7668bb537be0cf8ff93523a2 100644
--- a/webkit/browser/appcache/appcache_update_job.cc
+++ b/webkit/browser/appcache/appcache_update_job.cc
@@ -76,12 +76,16 @@ class HostNotifier {
     }
   }
 
-  void SendErrorNotifications(const std::string& error_message) {
+  void SendErrorNotifications(const std::string& error_message,
+                              appcache::ErrorReason reason,
+                              const GURL& url,
+                              int status) {
     DCHECK(!error_message.empty());
     for (NotifyHostMap::iterator it = hosts_to_notify.begin();
          it != hosts_to_notify.end(); ++it) {
       AppCacheFrontend* frontend = it->first;
-      frontend->OnErrorEventRaised(it->second, error_message);
+      frontend->OnErrorEventRaised(
+          it->second, error_message, reason, url, status);
     }
   }
 
@@ -420,17 +424,21 @@ AppCacheResponseWriter* AppCacheUpdateJob::CreateResponseWriter() {
   return writer;
 }
 
-void AppCacheUpdateJob::HandleCacheFailure(
-    const std::string& error_message,
-    ResultType result) {
+void AppCacheUpdateJob::HandleCacheFailure(const std::string& error_message,
+                                           ErrorReason reason,
+                                           const GURL& resource_url,
+                                           int resource_status,
+                                           ResultType result) {
   // 6.9.4 cache failure steps 2-8.
   DCHECK(internal_state_ != CACHE_FAILURE);
   DCHECK(!error_message.empty());
   DCHECK(result != UPDATE_OK);
   internal_state_ = CACHE_FAILURE;
   CancelAllUrlFetches();
-  CancelAllMasterEntryFetches(error_message);
-  NotifyAllError(error_message);
+  // TODO: Should both of these errors be generated?

[michaeln, 2014/02/27 23:16:06]

This should be ok, CancelAllMasterEntryFetches is delivered to one set of
clients and NotifyAllError is sent to another. No client sees two errors, and
the same error is seen by both sets.

+  CancelAllMasterEntryFetches(
+      error_message, reason, resource_url, resource_status);
+  NotifyAllError(error_message, reason, resource_url, resource_status);
   DiscardInprogressCache();
   internal_state_ = COMPLETED;
   AppCacheHistograms::CountUpdateJobResult(
@@ -498,15 +506,24 @@ void AppCacheUpdateJob::HandleManifestFetchCompleted(
     const char* kFormatString = "Manifest fetch failed (%d) %s";
     std::string message = FormatUrlErrorMessage(
         kFormatString, manifest_url_, fetcher->result(), response_code);
-    HandleCacheFailure(message, fetcher->result());
+    HandleCacheFailure(message,
+                       ERROR_MANIFEST_FETCH,
+                       manifest_url_,
+                       response_code,
+                       fetcher->result());
   }
 }
 
 void AppCacheUpdateJob::OnGroupMadeObsolete(AppCacheGroup* group,
                                             bool success) {
   DCHECK(master_entry_fetches_.empty());
-  CancelAllMasterEntryFetches("The cache has been made obsolete, "
-                              "the manifest file returned 404 or 410");
+  CancelAllMasterEntryFetches(
+      "The cache has been made obsolete, "
+      "the manifest file returned 404 or 410",
+      ERROR_MANIFEST_FETCH,

[michaeln, 2014/02/27 23:16:06]

s/b ERROR_MANIFEST_OBSOLETE?

This is another of those differences in behavior
- new master entry pages see and error event
- existing cached pages see an obsolete event

[jsbell, 2014/02/28 22:46:00]

Whoops, yes.
Ah, thanks.

+      // TODO: Can we populate these correctly?
+      GURL(),

[michaeln, 2014/02/27 23:16:06]

Maybe we shouldn't populate the url attribute for all reasons? If the reason is
particular to the manifesturl thats should be a known enough value to the
caller.

manifest_url_ should be available here, but the result code is gone at this
point

see const GURL manifest_url_;  // here for easier access

[jsbell, 2014/02/28 22:46:00]

Agreed. Given that the message has "404 or 410" I was wondering if we could pipe
it through, but since we can't then we don't need to send the URL along either.

+      0);
   if (success) {
     DCHECK(group->is_obsolete());
     NotifyAllAssociatedHosts(OBSOLETE_EVENT);
@@ -514,7 +531,11 @@ void AppCacheUpdateJob::OnGroupMadeObsolete(AppCacheGroup* group,
     MaybeCompleteUpdate();
   } else {
     // Treat failure to mark group obsolete as a cache failure.
-    HandleCacheFailure("Failed to mark the cache as obsolete", DB_ERROR);
+    HandleCacheFailure("Failed to mark the cache as obsolete",
+                       ERROR_INTERNAL,
+                       GURL(),
+                       0,
+                       DB_ERROR);
   }
 }
 
@@ -537,7 +558,8 @@ void AppCacheUpdateJob::ContinueHandleManifestFetchCompleted(bool changed) {
     const char* kFormatString = "Failed to parse manifest %s";
     const std::string message = base::StringPrintf(kFormatString,
         manifest_url_.spec().c_str());
-    HandleCacheFailure(message, MANIFEST_ERROR);
+    HandleCacheFailure(
+        message, ERROR_MANIFEST_INVALID, manifest_url_, 0, MANIFEST_ERROR);
     VLOG(1) << message;
     return;
   }
@@ -611,7 +633,11 @@ void AppCacheUpdateJob::HandleUrlFetchCompleted(URLFetcher* fetcher) {
         const char* kFormatString = "Resource fetch failed (%d) %s";
         std::string message = FormatUrlErrorMessage(
             kFormatString, url, fetcher->result(), response_code);
-        HandleCacheFailure(message, fetcher->result());
+        HandleCacheFailure(message,

[michaeln, 2014/02/27 23:16:06]

There are a handful of reasons a fetcher could have failed, fetcher->result()
can contain these kinds of errors

REDIRECT_ERROR (got a 30x which is verbotten, response_code = ?)
SERVER_ERROR (got non-200 response, response_code > 0)
NETWORK_ERROR (got no response whatsoever, response_code = -1)
DISKCACHE_ERROR (failed to write what we got to disk, response_code = ?)

What do we want to say about 'reason' to in the resulting error event?

ERROR_RESOURCE_FETCH definitely describes the operation or step that went wrong,
but thats a little different than the reason for failure. The consumer might be
able to discern the 'reason' based on the value in response_code.

I'm not sure what we want to do here, just bringing it up. If 'message' is also
exposed finer grained distinctions could be made there.

[jsbell, 2014/02/28 22:46:00]

^^^ combined with response_code, I think a single type for those is sufficient.
(i.e. if it's a 5xx they know the server is busted, 4xx they know the config is
wrong, 3xx they should read the spec.)

Then again: if redirect is explicitly called out in the spec, having a distinct
error type for it makes sense. Thoughts?
^^^ Makes sense to distinguish in some way. Or we could say ERROR_RESOURCE_FETCH
and the response_code is 0 ? i.e. this is the same basic category of error.
And this seems like it's the ERROR_INTERNAL bucket, i.e. you as a developer
can't fix this. 
That's what I was leaning towards, apart from the cache failure.

+                           ERROR_RESOURCE_FETCH,
+                           url,
+                           response_code,
+                           fetcher->result());
         return;
       }
     } else if (response_code == 404 || response_code == 410) {
@@ -696,7 +722,8 @@ void AppCacheUpdateJob::HandleMasterEntryFetchCompleted(
     const char* kFormatString = "Manifest fetch failed (%d) %s";
     std::string message = FormatUrlErrorMessage(
         kFormatString, request->url(), fetcher->result(), response_code);
-    host_notifier.SendErrorNotifications(message);
+    host_notifier.SendErrorNotifications(
+        message, ERROR_MANIFEST_FETCH, request->url(), response_code);
 
     // In downloading case, update result is different if all master entries
     // failed vs. only some failing.
@@ -707,7 +734,12 @@ void AppCacheUpdateJob::HandleMasterEntryFetchCompleted(
 
       // Section 6.9.4, step 22.3.
       if (update_type_ == CACHE_ATTEMPT && pending_master_entries_.empty()) {
-        HandleCacheFailure(message, fetcher->result());
+        // TODO: Is this a duplicate error?
+        HandleCacheFailure(message,
+                           ERROR_MANIFEST_FETCH,
+                           request->url(),
+                           response_code,
+                           fetcher->result());
         return;
       }
     }
@@ -749,12 +781,20 @@ void AppCacheUpdateJob::HandleManifestRefetchCompleted(
             << " response code: " << response_code;
     ScheduleUpdateRetry(kRerunDelayMs);
     if (response_code == 200) {
-      HandleCacheFailure("Manifest changed during update", MANIFEST_ERROR);
+      HandleCacheFailure("Manifest changed during update",
+                         ERROR_MANIFEST_CHANGED,
+                         manifest_url_,
+                         response_code,
+                         MANIFEST_ERROR);
     } else {
       const char* kFormatString = "Manifest re-fetch failed (%d) %s";
       std::string message = FormatUrlErrorMessage(
           kFormatString, manifest_url_, fetcher->result(), response_code);
-      HandleCacheFailure(message, fetcher->result());
+      HandleCacheFailure(message,
+                         ERROR_MANIFEST_FETCH,
+                         manifest_url_,
+                         response_code,
+                         fetcher->result());
     }
   }
 }
@@ -770,6 +810,9 @@ void AppCacheUpdateJob::OnManifestInfoWriteComplete(int result) {
                    base::Unretained(this)));
   } else {
     HandleCacheFailure("Failed to write the manifest headers to storage",
+                       ERROR_INTERNAL,
+                       GURL(),
+                       0,
                        DISKCACHE_ERROR);
   }
 }
@@ -784,6 +827,9 @@ void AppCacheUpdateJob::OnManifestDataWriteComplete(int result) {
     StoreGroupAndCache();
   } else {
     HandleCacheFailure("Failed to write the manifest data to storage",
+                       ERROR_INTERNAL,
+                       GURL(),
+                       0,
                        DISKCACHE_ERROR);
   }
 }
@@ -825,7 +871,12 @@ void AppCacheUpdateJob::OnGroupAndNewestCacheStored(AppCacheGroup* group,
       message.append(", would exceed quota");
       result = QUOTA_ERROR;
     }
-    HandleCacheFailure(message, result);
+    HandleCacheFailure(
+        message,
+        would_exceed_quota ? ERROR_QUOTA_EXCEEDED : ERROR_INTERNAL,
+        GURL(),
+        0,
+        result);
   }
 }
 
@@ -853,10 +904,14 @@ void AppCacheUpdateJob::NotifyAllFinalProgress() {
   NotifyAllProgress(GURL());
 }
 
-void AppCacheUpdateJob::NotifyAllError(const std::string& error_message) {
+void AppCacheUpdateJob::NotifyAllError(const std::string& error_message,
+                                       ErrorReason reason,
+                                       const GURL& resource_url,
+                                       int resource_status) {
   HostNotifier host_notifier;
   AddAllAssociatedHostsToNotifier(&host_notifier);
-  host_notifier.SendErrorNotifications(error_message);
+  host_notifier.SendErrorNotifications(
+      error_message, reason, resource_url, resource_status);
 }
 
 void AppCacheUpdateJob::AddAllAssociatedHostsToNotifier(
@@ -911,6 +966,9 @@ void AppCacheUpdateJob::CheckIfManifestChanged() {
       // Use a local variable because service_ is reset in HandleCacheFailure.
       AppCacheService* service = service_;
       HandleCacheFailure("Manifest entry not found in existing cache",
+                         ERROR_INTERNAL,
+                         GURL(),
+                         0,
                          DB_ERROR);
       AppCacheHistograms::AddMissingManifestEntrySample();
       service->DeleteAppCacheGroup(manifest_url_, net::CompletionCallback());
@@ -1152,7 +1210,10 @@ void AppCacheUpdateJob::FetchMasterEntries() {
 }
 
 void AppCacheUpdateJob::CancelAllMasterEntryFetches(
-    const std::string& error_message) {
+    const std::string& error_message,
+    ErrorReason reason,
+    const GURL& resource_url,
+    int resource_status) {
   // For now, cancel all in-progress fetches for master entries and pretend
   // all master entries fetches have completed.
   // TODO(jennb): Delete this when update no longer fetches master entries
@@ -1188,7 +1249,8 @@ void AppCacheUpdateJob::CancelAllMasterEntryFetches(
 
     master_entries_to_fetch_.erase(master_entries_to_fetch_.begin());
   }
-  host_notifier.SendErrorNotifications(error_message);
+  host_notifier.SendErrorNotifications(
+      error_message, reason, resource_url, resource_status);
 }
 
 bool AppCacheUpdateJob::MaybeLoadFromNewestCache(const GURL& url,