Implementing timeout support for XHR

Source/core/platform/network/chromium/ResourceHandle.cpp

Index: Source/core/platform/network/chromium/ResourceHandle.cpp
diff --git a/Source/core/platform/network/chromium/ResourceHandle.cpp b/Source/core/platform/network/chromium/ResourceHandle.cpp
index 54175ef216086b78905b6a56f4874d017cb3f4f1..f3340166db9f9894e3c3abca9a4b2ecbd7b29199 100644
--- a/Source/core/platform/network/chromium/ResourceHandle.cpp
+++ b/Source/core/platform/network/chromium/ResourceHandle.cpp
@@ -1,5 +1,6 @@
 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
+ * Copyright (C) 2013 Intel Corporation. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
@@ -58,6 +59,7 @@ ResourceHandleInternal::ResourceHandleInternal(NetworkingContext* context, const
     , m_owner(0)
     , m_client(client)
     , m_state(ConnectionStateNew)
+    , m_timer(this, &ResourceHandleInternal::timerFired)

[Dominik Röttsches, 2013/04/19 13:45:52]

I renamed this to m_timeoutTimer to be hopefully a bit more descriptive.

 {
 }
 
@@ -73,6 +75,9 @@ void ResourceHandleInternal::start()
     WrappedResourceRequest wrappedRequest(m_request);
     wrappedRequest.setAllowStoredCredentials(allowStoredCredentials());
     m_loader->loadAsynchronously(wrappedRequest, this);
+
+    if (m_request.timeoutInterval() > 0)
+        m_timer.startOneShot(m_request.timeoutInterval());
 }
 
 void ResourceHandleInternal::cancel()
@@ -177,6 +182,21 @@ ResourceHandleInternal* ResourceHandleInternal::FromResourceHandle(ResourceHandl
     return handle->d.get();
 }
 
+void ResourceHandleInternal::timerFired(Timer<ResourceHandleInternal>*)
+{

[abarth-chromium, 2013/04/19 06:54:39]

Usually we add an ASSERT_UNUSED that the timer parameter to this function
matches the timer member variable that we expect.

[Dominik Röttsches, 2013/04/19 13:45:52]

Thanks, added.

+    // Use the same value as in NSURLError.h
+    static const int timeoutError = -1001;

[abarth-chromium, 2013/04/19 06:54:39]

Do we have this constant elsewhere in the project?  It seems odd to hard-code
this value.  Is it an artifact of Blink or of the spec?

[Dominik Röttsches, 2013/04/19 13:45:52]

It's not from the spec. It was rather a WebKit artifact, using this value from
CFNetwork. Removed. An empty-initialized ResourceError + setIsTimeout(true) is
enough here.

+    static const char* const errorDomain = "WebKitNetworkError";

[abarth-chromium, 2013/04/19 06:54:39]

Is this constant defined somewhere else?  It seems odd to have it here as a
literal.

[Dominik Röttsches, 2013/04/19 13:45:52]

AFAIR, it was also used by the CFNetwork backend and used here for consistency.
Since WebCore doesn't actually care about these values, I removed it here.

We could use the values that base/net defines as kErrorDomain = "net" and
net::ERR_TIMED_OUT (-7) - but we don't need to emulate that here.

+
+    m_state = ConnectionStateCanceled;
+    m_loader->cancel();

[abarth-chromium, 2013/04/19 06:54:39]

Does this object have a cancel method that does this work?  It seems like we
should call a function to do this work rather than replicating this detailed
work here.

[Dominik Röttsches, 2013/04/19 13:45:52]

It does have a cancel() method. However, if we call it before sending the
timeout callback, it sends a different error, then sets m_client = 0, and we
can't fire the right callback. Calling it afterwards crashes, because it seems
our own instance of ResourceHandleInternal gets immediately deleted after the
timeout callback. So - trying to squeeze in between WebURLLoader and WebCore - I
don't see a good way to avoid this kind of code duplication here.

+    if (m_client) {
+        ResourceError error(errorDomain, timeoutError, m_request.url().string(), String());
+        error.setIsTimeout(true);
+        m_client->didFail(m_owner, error);
+    }
+}
+
 // ResourceHandle -------------------------------------------------------------
 
 ResourceHandle::ResourceHandle(NetworkingContext* context, const ResourceRequest& request, ResourceHandleClient* client, bool defersLoading, bool shouldContentSniff)