Remove the HANDLED_EXTERNALLY status code.

Remove the HANDLED_EXTERNALLY status code.

This removes the HANDLED_EXTERNALLY status replacing its use with ResourceRequestInfo::HandledExternally.

BUG=none
TEST=unit_tests, content_unittests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155427

[mkosiba (inactive), 2012-06-22 14:36:58]

Hi Darin,
    Here is one approach we could take to remove the HANDLED_EXTERNALLY status
code from the net stack. I'm not super happy about it since we're replacing the
HANDLED_EXTERNALLY status code with the (CANCELED, ERR_UNKNOWN_URL_SCHEME)
status + error code pair, so that's not buying much in terms of encapsulation.
    IMHO HANDLED_EXTERNALLY could be a private thing between WebURLLoader and
ResourceDispatcher{'', Host, Delegate} but since code outside of that set
doesn't uniformly handle HANDLED_EXTERNALLY as either SUCCESS or CANCELED it
would seem that introducing such an encapsulation would require changes to
behavior.

Cheers!
   Martin

[darin (slow to review), 2012-06-22 19:32:24]

This is a good start.  I think we might want to invent an error code that
is semantically more correct.  Overloading ERR_UNKNOWN_URL_SCHEME is not
correct.

One idea is to piggyback on the notion of HTTP 204 responses.  A 204
response mean "no content", which in the case of a navigation results in
the navigation being aborted.  We could perhaps build on that idea with an
ERR_NO_CONTENT.

However, that error code doesn't really have any meaning to the network
stack.  That problem may suggest another solution.  We could perhaps say
that a range of network error codes are reserved for definition by the
embedder.  The embedder (Chrome) could invent its own error codes and use
one of those to convey this "handled externally" condition.  We could have
an ERR_HANDLED_EXTERNALLY defined outside of the network library.

Hmm... this is something that deserves some thought... any other ideas?

-Darin


On Fri, Jun 22, 2012 at 7:36 AM, <mkosiba@chromium.org> wrote:

> Reviewers: darin,
>
> Message:
> Hi Darin,
>    Here is one approach we could take to remove the HANDLED_EXTERNALLY
> status
> code from the net stack. I'm not super happy about it since we're
> replacing the
> HANDLED_EXTERNALLY status code with the (CANCELED, ERR_UNKNOWN_URL_SCHEME)
> status + error code pair, so that's not buying much in terms of
> encapsulation.
>    IMHO HANDLED_EXTERNALLY could be a private thing between WebURLLoader
> and
> ResourceDispatcher{'', Host, Delegate} but since code outside of that set
> doesn't uniformly handle HANDLED_EXTERNALLY as either SUCCESS or CANCELED
> it
> would seem that introducing such an encapsulation would require changes to
> behavior.
>
> Cheers!
>   Martin
>
> Description:
> Remove the HANDLED_EXTERNALLY status code.
>
> This removes the HANDLED_EXTERNALLY status replacing its use with the
> net::ERR_UNKNOWN_URL_SCHEME error code.
> This change also modifies the InterceptNavigationResourceThr**ottle to
> cancel the request with the net::ERR_UNKNOWN_URL_SCHEME error code.
>
> BUG=none
> TEST=unit_tests, content_unittests
>
>
> Please review this at
https://chromiumcodereview.**appspot.com/10640019/<https://chromiumcodereview...
>
> SVN Base:
http://git.chromium.org/**chromium/src.git@master<http://git.chromium.org/chr...
>
> Affected files:
>  M chrome/browser/extensions/**user_script_listener_unittest.**cc
>  M chrome/browser/net/chrome_**network_delegate.cc
>  M chrome/browser/renderer_host/**intercept_navigation_resource_**
> throttle.cc
>  M chrome/browser/renderer_host/**intercept_navigation_resource_**
> throttle_unittest.cc
>  M content/browser/renderer_host/**buffered_resource_handler.cc
>  M content/browser/renderer_host/**resource_loader.h
>  M content/browser/renderer_host/**resource_loader.cc
>  M content/browser/renderer_host/**throttling_resource_handler.h
>  M content/browser/renderer_host/**throttling_resource_handler.cc
>  M content/public/browser/**resource_controller.h
>  M content/public/common/common_**param_traits.cc
>  M net/url_request/url_request_**status.h
>  M webkit/appcache/appcache_**request_handler.cc
>  M webkit/glue/resource_loader_**bridge.h
>  M webkit/glue/weburlloader_impl.**cc
>
>
> Index: chrome/browser/extensions/**user_script_listener_unittest.**cc
> diff --git a/chrome/browser/extensions/**user_script_listener_unittest.**cc
> b/chrome/browser/extensions/**user_script_listener_unittest.**cc
> index 02cba5ba29066c74b3b5500b6e6472**b8d9507b37..**
> 1a68c9ee64faf506508158aa221cb9**e2457235b1 100644
> --- a/chrome/browser/extensions/**user_script_listener_unittest.**cc
> +++ b/chrome/browser/extensions/**user_script_listener_unittest.**cc
> @@ -47,6 +47,9 @@ class ThrottleController : public
> base::SupportsUserData::Data,
>   virtual void Cancel() {
>     NOTREACHED();
>   }
> +  virtual void CancelWithError(int error) {
> +    NOTREACHED();
> +  }
>
>  private:
>   net::URLRequest* request_;
> Index: chrome/browser/net/chrome_**network_delegate.cc
> diff --git a/chrome/browser/net/chrome_**network_delegate.cc
> b/chrome/browser/net/chrome_**network_delegate.cc
> index cc87909b0cd04a57aea64fb1ad96f0**6d167f1a36..**
> c0d968ca638ad51f2fc897851e3e1d**3ba8172577 100644
> --- a/chrome/browser/net/chrome_**network_delegate.cc
> +++ b/chrome/browser/net/chrome_**network_delegate.cc
> @@ -233,7 +233,8 @@ void ChromeNetworkDelegate::**OnRawBytesRead(const
> net::URLRequest& request,
>  void ChromeNetworkDelegate::**OnCompleted(net::URLRequest* request,
>                                         bool started) {
>   if (request->status().status() == net::URLRequestStatus::SUCCESS ||
> -      request->status().status() ==
net::URLRequestStatus::**HANDLED_EXTERNALLY)
> {
> +      (request->status().status() == net::URLRequestStatus::**CANCELED &&
> +       request->status().error() == net::ERR_UNKNOWN_URL_SCHEME)) {
>     bool is_redirect = request->response_headers() &&
>         net::HttpResponseHeaders::**IsRedirectResponseCode(
>             request->response_headers()->**response_code());
> Index: chrome/browser/renderer_host/**intercept_navigation_resource_**
> throttle.cc
> diff --git
a/chrome/browser/renderer_**host/intercept_navigation_**resource_throttle.cc
> b/chrome/browser/renderer_**host/intercept_navigation_**
> resource_throttle.cc
> index 674aa5502be29a0c2d76d87ee6050b**516a01a263..**
> 35a99257db9648e3cecc3b1264d6cc**112a6ef697 100644
> --- a/chrome/browser/renderer_**host/intercept_navigation_**
> resource_throttle.cc
> +++ b/chrome/browser/renderer_**host/intercept_navigation_**
> resource_throttle.cc
> @@ -10,9 +10,8 @@
>  #include "content/public/browser/**render_process_host.h"
>  #include "content/public/browser/**resource_request_info.h"
>  #include "content/public/browser/**resource_controller.h"
> -#include "content/public/browser/web_**contents.h"
> -#include "content/public/browser/web_**contents_delegate.h"
>  #include "content/public/common/**referrer.h"
> +#include "net/base/net_errors.h"
>  #include "net/url_request/url_request.**h"
>
>  using content::BrowserThread;
> @@ -20,8 +19,6 @@ using content::**ChildProcessSecurityPolicy;
>  using content::Referrer;
>  using content::RenderViewHost;
>  using content::ResourceRequestInfo;
> -using content::WebContents;
> -using content::WebContentsDelegate;
>
>  namespace {
>
> @@ -118,9 +115,7 @@ void InterceptNavigationResourceThr**
> ottle::OnResultObtained(
>   DCHECK(BrowserThread::**CurrentlyOn(BrowserThread::IO)**);
>
>   if (should_ignore_navigation) {
> -    // TODO(mkosiba): Cancel() results in a CANCELLED status but what we
> really
> -    // want is a HANDLED_EXTERNALLY status.
> -    controller()->Cancel();
> +    controller()->CancelWithError(**net::ERR_UNKNOWN_URL_SCHEME);
>   } else {
>     controller()->Resume();
>   }
> Index: chrome/browser/renderer_host/**intercept_navigation_resource_**
> throttle_unittest.cc
> diff --git
a/chrome/browser/renderer_**host/intercept_navigation_**resource_throttle_unittest.cc
> b/chrome/browser/renderer_**host/intercept_navigation_**
> resource_throttle_unittest.cc
> index 53e5d4f6422a0dc9838ebf7c583d81**dd6cbe13b0..**
> 88d641e0a7f455a6b2e2d0fbfa7257**7af83efa36 100644
> --- a/chrome/browser/renderer_**host/intercept_navigation_**
> resource_throttle_unittest.cc
> +++ b/chrome/browser/renderer_**host/intercept_navigation_**
> resource_throttle_unittest.cc
> @@ -68,6 +68,10 @@ class MockResourceController
>
>   // content::ResourceController
>   virtual void Cancel() {
> +    NOTREACHED();
> +  }
> +  virtual void CancelWithError(int error) {
> +    DCHECK(error == net::ERR_UNKNOWN_URL_SCHEME);
>     DCHECK(status_ == UNKNOWN);
>     status_ = CANCELLED;
>     ContinueTestCase();
> Index: content/browser/renderer_host/**buffered_resource_handler.cc
> diff --git a/content/browser/renderer_**host/buffered_resource_**handler.cc
> b/content/browser/renderer_**host/buffered_resource_**handler.cc
> index b57da4a3e8ebe1ce3711f4b1f4c271**b0b8505b1b..**
> a8aea5ef3dab656675e3eb27c53ac1**8dd62d3c12 100644
> --- a/content/browser/renderer_**host/buffered_resource_**handler.cc
> +++ b/content/browser/renderer_**host/buffered_resource_**handler.cc
> @@ -380,7 +380,8 @@ bool BufferedResourceHandler::**
> UseAlternateResourceHandler(
>   bool defer_ignored = false;
>   next_handler_->**OnResponseStarted(request_id, response_,
> &defer_ignored);
>   DCHECK(!defer_ignored);
> -  net::URLRequestStatus status(net::URLRequestStatus::**HANDLED_EXTERNALLY,
> 0);
> +  net::URLRequestStatus status(net::URLRequestStatus::**CANCELED,
> +                               net::ERR_UNKNOWN_URL_SCHEME);
>   next_handler_->**OnResponseCompleted(request_**id, status,
> std::string());
>
>   // Remove the non-owning pointer to the CrossSiteResourceHandler, if any,
> Index: content/browser/renderer_host/**resource_loader.cc
> diff --git a/content/browser/renderer_**host/resource_loader.cc
> b/content/browser/renderer_**host/resource_loader.cc
> index b5cba214cd3e95491e2be720f1de02**09d326519f..**
> 503459fbc7f85345dd5cecad5b900a**05689b352d 100644
> --- a/content/browser/renderer_**host/resource_loader.cc
> +++ b/content/browser/renderer_**host/resource_loader.cc
> @@ -104,6 +104,10 @@ void ResourceLoader::CancelRequest(**bool
> from_renderer) {
>   CancelRequestInternal(net::**ERR_ABORTED, from_renderer);
>  }
>
> +void ResourceLoader::**CancelWithError(int error) {
> +  CancelRequestInternal(error, false);
> +}
> +
>  void ResourceLoader::**ReportUploadProgress() {
>   ResourceRequestInfoImpl* info = GetRequestInfo();
>
> Index: content/browser/renderer_host/**resource_loader.h
> diff --git a/content/browser/renderer_**host/resource_loader.h
> b/content/browser/renderer_**host/resource_loader.h
> index bb37cdd43e9e4109eb715c7ddaaf1d**f9bc220756..**
> 94491136525ff21a12dda330d3d6b7**823f98a210 100644
> --- a/content/browser/renderer_**host/resource_loader.h
> +++ b/content/browser/renderer_**host/resource_loader.h
> @@ -77,6 +77,7 @@ class ResourceLoader : public net::URLRequest::Delegate,
>   // ResourceController implementation:
>   virtual void Resume() OVERRIDE;
>   virtual void Cancel() OVERRIDE;
> +  virtual void CancelWithError(int error) OVERRIDE;
>
>   void StartRequestInternal();
>   void CancelRequestInternal(int error, bool from_renderer);
> Index: content/browser/renderer_host/**throttling_resource_handler.cc
> diff --git a/content/browser/renderer_**host/throttling_resource_**handler.cc
> b/content/browser/renderer_**host/throttling_resource_**handler.cc
> index 412cadfaf0b04601c3badc0594c78d**e34c9690fc..**
> 02ffbd0fc88342f48fc4a5e2b7910b**516703ae21 100644
> --- a/content/browser/renderer_**host/throttling_resource_**handler.cc
> +++ b/content/browser/renderer_**host/throttling_resource_**handler.cc
> @@ -96,6 +96,10 @@ void ThrottlingResourceHandler::**Cancel() {
>   controller()->Cancel();
>  }
>
> +void ThrottlingResourceHandler::**CancelWithError(int error) {
> +  controller()->CancelWithError(**error);
> +}
> +
>  void ThrottlingResourceHandler::**Resume() {
>   switch (deferred_stage_) {
>     case DEFERRED_NONE:
> Index: content/browser/renderer_host/**throttling_resource_handler.h
> diff --git a/content/browser/renderer_**host/throttling_resource_**handler.h
> b/content/browser/renderer_**host/throttling_resource_**handler.h
> index a660f53669b5b651a849b5d10622e5**214816aebc..**
> fac6172b6e8f2c8308ba8aee0ad472**0e11fe4c2a 100644
> --- a/content/browser/renderer_**host/throttling_resource_**handler.h
> +++ b/content/browser/renderer_**host/throttling_resource_**handler.h
> @@ -40,6 +40,7 @@ class ThrottlingResourceHandler : public
> LayeredResourceHandler,
>
>   // ResourceThrottleController implementation:
>   virtual void Cancel() OVERRIDE;
> +  virtual void CancelWithError(int error) OVERRIDE;
>   virtual void Resume() OVERRIDE;
>
>  private:
> Index: content/public/browser/**resource_controller.h
> diff --git a/content/public/browser/**resource_controller.h
> b/content/public/browser/**resource_controller.h
> index 17c0eabafe7ef01aa03c3dad9b920d**92acf04392..**
> bdc5ffb7d3576b279d280ba034f9b3**9697ca82f6 100644
> --- a/content/public/browser/**resource_controller.h
> +++ b/content/public/browser/**resource_controller.h
> @@ -12,6 +12,7 @@ namespace content {
>  class ResourceController {
>  public:
>   virtual void Cancel() = 0;
> +  virtual void CancelWithError(int error) = 0;
>   virtual void Resume() = 0;
>  protected:
>   virtual ~ResourceController() {}
> Index: content/public/common/common_**param_traits.cc
> diff --git a/content/public/common/**common_param_traits.cc
> b/content/public/common/**common_param_traits.cc
> index 723c33bcb748b31d1f46fef8beef42**4f7d0abe55..**
> 74ffa348b6484cd573f744e77ba532**5d93058056 100644
> --- a/content/public/common/**common_param_traits.cc
> +++ b/content/public/common/**common_param_traits.cc
> @@ -167,9 +167,6 @@ void ParamTraits<net::**URLRequestStatus>::Log(const
> param_type& p,
>     case net::URLRequestStatus::IO_**PENDING:
>       status = "IO_PENDING ";
>       break;
> -    case net::URLRequestStatus::**HANDLED_EXTERNALLY:
> -      status = "HANDLED_EXTERNALLY";
> -      break;
>     case net::URLRequestStatus::**CANCELED:
>       status = "CANCELED";
>       break;
> Index: net/url_request/url_request_**status.h
> diff --git a/net/url_request/url_request_**status.h
> b/net/url_request/url_request_**status.h
> index 1e497ab4df6babc5a910ae202d3e5f**4f3d67e4bd..**
> 2d282d149009700fa9a511be90f1af**7feb495769 100644
> --- a/net/url_request/url_request_**status.h
> +++ b/net/url_request/url_request_**status.h
> @@ -23,11 +23,6 @@ class URLRequestStatus {
>     // completed.
>     IO_PENDING,
>
> -    // Request was successful but was handled by an external program, so
> there
> -    // is no response data. This usually means the current page should
> not be
> -    // navigated, but no error should be displayed. |error_| will be 0.
> -    HANDLED_EXTERNALLY,
> -
>     // Request was cancelled programatically.
>     CANCELED,
>
> @@ -45,9 +40,7 @@ class URLRequestStatus {
>   void set_error(int e) { error_ = e; }
>
>   // Returns true if the status is success, which makes some calling code
> more
> -  // convenient because this is the most common test. Note that we do NOT
> treat
> -  // HANDLED_EXTERNALLY as success. For everything except user
> notifications,
> -  // this value should be handled like an error (processing should stop).
> +  // convenient because this is the most common test.
>   bool is_success() const {
>     return status_ == SUCCESS || status_ == IO_PENDING;
>   }
> Index: webkit/appcache/appcache_**request_handler.cc
> diff --git a/webkit/appcache/appcache_**request_handler.cc
> b/webkit/appcache/appcache_**request_handler.cc
> index 0f123a134bb52fe16ad9d8da5966f7**b9e4a89d9d..**
> d8da30f2f2888308d906dc23c6e491**ebe93daf14 100644
> --- a/webkit/appcache/appcache_**request_handler.cc
> +++ b/webkit/appcache/appcache_**request_handler.cc
> @@ -124,8 +124,7 @@ AppCacheURLRequestJob* AppCacheRequestHandler::**
> MaybeLoadFallbackForResponse(
>   if (!found_fallback_entry_.has_**response_id())
>     return NULL;
>
> -  if (request->status().status() == net::URLRequestStatus::**CANCELED ||
> -      request->status().status() ==
net::URLRequestStatus::**HANDLED_EXTERNALLY)
> {
> +  if (request->status().status() == net::URLRequestStatus::**CANCELED) {
>     // 6.9.6, step 4: But not if the user canceled the download.
>     return NULL;
>   }
> Index: webkit/glue/resource_loader_**bridge.h
> diff --git a/webkit/glue/resource_loader_**bridge.h
> b/webkit/glue/resource_loader_**bridge.h
> index 70db933767cda47e9f01be9c883245**708b7aa6b3..**
> 4d067c1a4b4a8814bb9f776273b0fd**335a0c2cad 100644
> --- a/webkit/glue/resource_loader_**bridge.h
> +++ b/webkit/glue/resource_loader_**bridge.h
> @@ -334,7 +334,7 @@ class ResourceLoaderBridge {
>     virtual void OnReceivedCachedMetadata(const char* data, int len) { }
>
>     // Called when the response is complete.  This method signals
> completion of
> -    // the resource load.ff
> +    // the resource load.
>     virtual void OnCompletedRequest(
>         const net::URLRequestStatus& status,
>         const std::string& security_info,
> Index: webkit/glue/weburlloader_impl.**cc
> diff --git a/webkit/glue/weburlloader_**impl.cc
> b/webkit/glue/weburlloader_**impl.cc
> index 104795dd1eed0e554ef5025249e660**2c20cd0646..**
> 143ec67c8b504ce64d97b013e4061a**369a87e510 100644
> --- a/webkit/glue/weburlloader_**impl.cc
> +++ b/webkit/glue/weburlloader_**impl.cc
> @@ -641,23 +641,15 @@ void WebURLLoaderImpl::Context::**
> OnCompletedRequest(
>
>   if (client_) {
>     if (status.status() != net::URLRequestStatus::**SUCCESS) {
> -      int error_code;
> -      if (status.status() == net::URLRequestStatus::**HANDLED_EXTERNALLY)
> {
> -        // By marking this request as aborted we insure that we don't
> navigate
> -        // to an error page.
> -        error_code = net::ERR_ABORTED;
> -      } else {
> -        error_code = status.error();
> -      }
>       WebURLError error;
> -      if (error_code == net::ERR_ABORTED) {
> +      if (status.error() == net::ERR_ABORTED) {
>         error.isCancellation = true;
> -      } else if (error_code == net::ERR_TEMPORARILY_**THROTTLED) {
> +      } else if (status.error() == net::ERR_TEMPORARILY_**THROTTLED) {
>         error.localizedDescription = WebString::fromUTF8(
>             kThrottledErrorDescription);
>       }
>       error.domain = WebString::fromUTF8(net::**kErrorDomain);
> -      error.reason = error_code;
> +      error.reason = status.error();
>       error.unreachableURL = request_.url();
>       client_->didFail(loader_, error);
>     } else {
> @@ -741,7 +733,7 @@ void WebURLLoaderImpl::**loadSynchronously(const
> WebURLRequest& request,
>   const net::URLRequestStatus::Status& status =
>       sync_load_response.status.**status();
>   if (status != net::URLRequestStatus::SUCCESS &&
> -      status != net::URLRequestStatus::**HANDLED_EXTERNALLY) {
> +      sync_load_response.status.**error() !=
> net::ERR_UNKNOWN_URL_SCHEME) {
>     response.setURL(final_url);
>     error.domain = WebString::fromUTF8(net::**kErrorDomain);
>     error.reason = sync_load_response.status.**error();
>
>
>

[mkosiba (inactive), 2012-06-22 20:57:02]

On Fri, Jun 22, 2012 at 8:32 PM, Darin Fisher <darin@chromium.org> wrote:

> This is a good start.  I think we might want to invent an error code that
> is semantically more correct.  Overloading ERR_UNKNOWN_URL_SCHEME is not
> correct.
>

Good point.


> One idea is to piggyback on the notion of HTTP 204 responses.  A 204
> response mean "no content", which in the case of a navigation results in
> the navigation being aborted.  We could perhaps build on that idea with an
> ERR_NO_CONTENT.
>
> However, that error code doesn't really have any meaning to the network
> stack.  That problem may suggest another solution.  We could perhaps say
> that a range of network error codes are reserved for definition by the
> embedder.  The embedder (Chrome) could invent its own error codes and use
> one of those to convey this "handled externally" condition.  We could have
> an ERR_HANDLED_EXTERNALLY defined outside of the network library.
>
> Hmm... this is something that deserves some thought... any other ideas?
>

Well.. the embedder can associate arbitrary objects with the URLRequest via
SupportsUserData, would that be better than reserving the range of network
codes?

[darin (slow to review), 2012-06-22 21:02:01]

On Fri, Jun 22, 2012 at 1:56 PM, Martin Kosiba <mkosiba@chromium.org> wrote:

>
>
> On Fri, Jun 22, 2012 at 8:32 PM, Darin Fisher <darin@chromium.org> wrote:
>
>> This is a good start.  I think we might want to invent an error code that
>> is semantically more correct.  Overloading ERR_UNKNOWN_URL_SCHEME is not
>> correct.
>>
>
> Good point.
>
>
>>  One idea is to piggyback on the notion of HTTP 204 responses.  A 204
>> response mean "no content", which in the case of a navigation results in
>> the navigation being aborted.  We could perhaps build on that idea with an
>> ERR_NO_CONTENT.
>>
>> However, that error code doesn't really have any meaning to the network
>> stack.  That problem may suggest another solution.  We could perhaps say
>> that a range of network error codes are reserved for definition by the
>> embedder.  The embedder (Chrome) could invent its own error codes and use
>> one of those to convey this "handled externally" condition.  We could have
>> an ERR_HANDLED_EXTERNALLY defined outside of the network library.
>>
>> Hmm... this is something that deserves some thought... any other ideas?
>>
>
> Well.. the embedder can associate arbitrary objects with the URLRequest
> via SupportsUserData, would that be better than reserving the range of
> network codes?
>
>


Maybe we just need to add another field to ResourceMsg_RequestComplete, but
it seems like part of the problem is plumbing that up from WebURLLoader to
the RenderView.  We need to pass the field through WebURLError and
WebCore::ResourceError.  Those error structures have a domain (string)
field that can be used to differentiate network errors from other classes
of errors.  That may be useful here.

-Darin

[mkosiba (inactive), 2012-06-25 14:05:28]

Hi Darin,
    So I uploaded the result of a bit of hacking that reflects some of the
improvements that we've been discussing. By no means is this close to a final
checkin but I wanted to get more feedback on whether I'm headed in the right
direction.

Thanks!
    Martin

[darin (slow to review), 2012-06-25 16:15:32]

https://chromiumcodereview.appspot.com/10640019/diff/6001/content/browser/ren...
File content/browser/renderer_host/resource_loader.cc (right):

https://chromiumcodereview.appspot.com/10640019/diff/6001/content/browser/ren...
content/browser/renderer_host/resource_loader.cc:111:
CancelRequestInternal(net::ERR_ABORTED, false);
another idea, which may be too crazy, is to just get rid of URLRequestStatus
entirely.  since net::ERR_ABORTED implies cancellation at the WebURLLoaderImpl
level, we don't really even need to pass the URLRequestStatus::status() member.

Furthermore (and this is maybe where my proposal gets crazy), since net:: error
codes are all negative, we could use a positive value to indicate "handled
externally".  This is not entirely odd as that is not really an error condition.
 We know that network error codes will never be positive since various network
level interfaces use an integer to pass either byte counts or error codes.

It seems like it might simplify the code if we just had a single int error code
field to pass around and inspect.  Thoughts?

[mkosiba (inactive), 2012-06-25 17:11:16]

On 2012/06/25 16:15:32, darin wrote:
>
https://chromiumcodereview.appspot.com/10640019/diff/6001/content/browser/ren...
> File content/browser/renderer_host/resource_loader.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10640019/diff/6001/content/browser/ren...
> content/browser/renderer_host/resource_loader.cc:111:
> CancelRequestInternal(net::ERR_ABORTED, false);
> another idea, which may be too crazy, is to just get rid of URLRequestStatus
> entirely.  since net::ERR_ABORTED implies cancellation at the WebURLLoaderImpl
> level, we don't really even need to pass the URLRequestStatus::status()
member.

Sounds interesting. I'll give it a shot. The only concern I have would be that
replacing URLRequestStatus with an int makes the code less flexible. I'd be
tempted to keep it around as a wrapper around the single error code value, also
I'd prefer to write status.is_success() than SUCCEEDED(error_code).

> Furthermore (and this is maybe where my proposal gets crazy), since net::
error
> codes are all negative, we could use a positive value to indicate "handled
> externally".  This is not entirely odd as that is not really an error
condition.
>  We know that network error codes will never be positive since various network
> level interfaces use an integer to pass either byte counts or error codes.
> 
> It seems like it might simplify the code if we just had a single int error
code
> field to pass around and inspect.  Thoughts?

Not really that different from Windows HRESULTs, we should be fine as long as we
don't introduce the equivalent of S_FALSE :P

[darin (slow to review), 2012-06-25 17:24:31]

On Mon, Jun 25, 2012 at 10:11 AM, <mkosiba@chromium.org> wrote:

> On 2012/06/25 16:15:32, darin wrote:
>
> https://chromiumcodereview.**appspot.com/10640019/diff/**
>
6001/content/browser/renderer_**host/resource_loader.cc<https://chromiumcodereview.appspot.com/10640019/diff/6001/content/browser/renderer_host/resource_loader.cc>
>
>> File content/browser/renderer_host/**resource_loader.cc (right):
>>
>
>
> https://chromiumcodereview.**appspot.com/10640019/diff/**
>
6001/content/browser/renderer_**host/resource_loader.cc#**newcode111<https://chromiumcodereview.appspot.com/10640019/diff/6001/content/browser/renderer_host/resource_loader.cc#newcode111>
>
>> content/browser/renderer_host/**resource_loader.cc:111:
>> CancelRequestInternal(net::**ERR_ABORTED, false);
>> another idea, which may be too crazy, is to just get rid of
>> URLRequestStatus
>> entirely.  since net::ERR_ABORTED implies cancellation at the
>> WebURLLoaderImpl
>> level, we don't really even need to pass the URLRequestStatus::status()
>>
> member.
>
> Sounds interesting. I'll give it a shot. The only concern I have would be
> that
> replacing URLRequestStatus with an int makes the code less flexible. I'd be
> tempted to keep it around as a wrapper around the single error code value,
> also
> I'd prefer to write status.is_success() than SUCCEEDED(error_code).


Internally, the network stack just passes around an int error code.
 Externally, with the exception of the code were focused on, most of the
code just deals with the int error code too.  I've longed wanted to kill
URLRequestStatus.

NOTE:  I'm not proposing that you kill URLRequestStatus entirely in this
CL.  I'm only requesting that we move the resource loading code
(ResourceDispatcher + ResourceLoaderBridge) away from it.



>
>  Furthermore (and this is maybe where my proposal gets crazy), since net::
>>
> error
>
>> codes are all negative, we could use a positive value to indicate "handled
>> externally".  This is not entirely odd as that is not really an error
>>
> condition.
>
>>  We know that network error codes will never be positive since various
>> network
>> level interfaces use an integer to pass either byte counts or error codes.
>>
>
>  It seems like it might simplify the code if we just had a single int error
>>
> code
>
>> field to pass around and inspect.  Thoughts?
>>
>
> Not really that different from Windows HRESULTs, we should be fine as long
> as we
> don't introduce the equivalent of S_FALSE :P
>
>
Right!

-Darin


>
>
https://chromiumcodereview.**appspot.com/10640019/<https://chromiumcodereview...
>

[mkosiba (inactive), 2012-06-26 16:58:59]

Hi Darin,
    Here is a version that doesn't use URLRequestStatus in the
ResourceLoader/weburlloader. What do you think?

[mkosiba (inactive), 2012-06-26 17:01:41]

https://chromiumcodereview.appspot.com/10640019/diff/13001/content/browser/re...
File content/browser/renderer_host/async_resource_handler.cc (right):

https://chromiumcodereview.appspot.com/10640019/diff/13001/content/browser/re...
content/browser/renderer_host/async_resource_handler.cc:300: error_code =
net::ERR_FAILED;
I was going back and forth on whether to do this here or to have some similar
DCHECK in URLRequestStatus and fix up callers. Ultimately settled for this
approach as fixing up callers to pass would introduce noise to the CL. We could
do it as a preliminary to this one.

[mkosiba (inactive), 2012-07-11 15:16:00]

ping

[darin (slow to review), 2012-07-11 18:12:43]

http://codereview.chromium.org/10640019/diff/13001/net/url_request/url_reques...
File net/url_request/url_request.cc (left):

http://codereview.chromium.org/10640019/diff/13001/net/url_request/url_reques...
net/url_request/url_request.cc:516: DCHECK(error < 0);
It feels wrong to allow non-negative (non-error) results here.  If we want to
support cancelling the request with OK_HANDLED_EXTERNALLY, then we should
probably make it a negative value: ERR_HANDLED_EXTERNALLY.

This brings us back to the previous discussion about whether or not this is an
error code or a success code.  We should also think about whether we want the
network stack to know about the concept of "handled externally".

The goal of removing URLRequestStatus::HANDLED_EXTERNALLY is either to remove
the concept of "handled externally" from the network stack or to help us
eliminate URLRequestStatus altogether.  Both seem like reasonable goals.

I am curious about completely eliminating the "handled externally" concept from
the network stack since it isn't something that makes much sense to a network
stack.  A network stack produces data or errors.

"handled externally" is something that only makes sense to web page navigation
requests.

[mkosiba (inactive), 2012-07-30 16:11:19]

Hi Darin,
    In this iteration I got rid of the HANDLED_EXTERNALLY error code at the
expense of ResourceRequestInfo gaining a HandledExternally field and public
getter. This also required reintroducing the handled_externally bit in the
ResourceLoaderBridge::Peer.

[darin (slow to review), 2012-07-30 17:50:47]

OK, thanks for continuing to push on this CL.  Will review ASAP.
-Darin

On Mon, Jul 30, 2012 at 9:11 AM, <mkosiba@chromium.org> wrote:

> Hi Darin,
>     In this iteration I got rid of the HANDLED_EXTERNALLY error code at the
> expense of ResourceRequestInfo gaining a HandledExternally field and public
> getter. This also required reintroducing the handled_externally bit in the
> ResourceLoaderBridge::Peer.
>
>
http://codereview.chromium.**org/10640019/<http://codereview.chromium.org/106...
>

[mkosiba (inactive), 2012-08-08 14:30:43]

Ping? 

On 2012/07/30 17:50:47, darin wrote:
> OK, thanks for continuing to push on this CL.  Will review ASAP.
> -Darin
> 
> On Mon, Jul 30, 2012 at 9:11 AM, <mailto:mkosiba@chromium.org> wrote:
> 
> > Hi Darin,
> >     In this iteration I got rid of the HANDLED_EXTERNALLY error code at the
> > expense of ResourceRequestInfo gaining a HandledExternally field and public
> > getter. This also required reintroducing the handled_externally bit in the
> > ResourceLoaderBridge::Peer.
> >
> >
>
http://codereview.chromium.**org/10640019/%3Chttp://codereview.chromium.org/1...>
> >

[darin (slow to review), 2012-08-10 18:03:02]

Sorry again for the slow response.  It has been a struggle for me to come up
with good advice on this patch.

http://codereview.chromium.org/10640019/diff/20034/content/public/browser/res...
File content/public/browser/resource_controller.h (right):

http://codereview.chromium.org/10640019/diff/20034/content/public/browser/res...
content/public/browser/resource_controller.h:15: virtual void
CancelWithError(int error) = 0;
It looks like we don't need CancelWithError since nothing calls this function in
this CL.

http://codereview.chromium.org/10640019/diff/20034/content/public/browser/res...
content/public/browser/resource_controller.h:16: virtual void
HandledExternally() = 0;
This doesn't seem like the best name for this function.  We want to tell the
ResourceLoader to cancel the request _and_ to also forget that the request was
ever generated.  Maybe CancelAndIgnore or CancelAndSuppressErrorReporting? 
Neither of these suggestions strike me as all that great.

Another idea is to make this into a function that synthesizes a HTTP 204 "No
content" response.  CancelAndReturnNoContent or CancelAndSynthesizeNoContent or
CancelAndSimulateNoContent?

I'm having a hard time coming up with a good solution to this problem.

CancelAndIgnore is probably my preference at this point.  It at least conveys
the fact that the URLRequest will be canceled.  Using the term "ignore" is
consistent with the WebKit API where we have WebNavigationPolicyIgnore, which is
roughly equivalent.  Plus if you read all of ResourceController methods with a
suffix of "Request", this one would be CancelAndIgnore-Request, which is pretty
close to my description of "cancel the request and forget that the request was
ever generated."

I would then use terms like "was_ignored" instead of "was_handled_externally."

http://codereview.chromium.org/10640019/diff/20034/net/base/net_errors.h
File net/base/net_errors.h (right):

http://codereview.chromium.org/10640019/diff/20034/net/base/net_errors.h#newc...
net/base/net_errors.h:42: // Returns true if |error| is a success error code.
I'm not sure we want to add these.  You'll need feedback from net/OWNERS.

http://codereview.chromium.org/10640019/diff/20034/net/url_request/url_reques...
File net/url_request/url_request.cc (left):

http://codereview.chromium.org/10640019/diff/20034/net/url_request/url_reques...
net/url_request/url_request.cc:521: 
nit: gratuitous change

http://codereview.chromium.org/10640019/diff/20034/webkit/glue/resource_loade...
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/10640019/diff/20034/webkit/glue/resource_loade...
webkit/glue/resource_loader_bridge.h:340: bool is_handled_externally,
nit: was_handled_externally

http://codereview.chromium.org/10640019/diff/20034/webkit/glue/weburlloader_i...
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/10640019/diff/20034/webkit/glue/weburlloader_i...
webkit/glue/weburlloader_impl.cc:644: if (!net::IsSuccess(error_code) ||
is_handled_externally) {
I think you should just compare error_code against net::OK here since we don't
support positive error codes in this context.

[mkosiba (inactive), 2012-08-29 15:53:01]

Sorry for taking most of forever to get another iteration out, was busy with the
android_webview stuff.

http://codereview.chromium.org/10640019/diff/20034/content/public/browser/res...
File content/public/browser/resource_controller.h (right):

http://codereview.chromium.org/10640019/diff/20034/content/public/browser/res...
content/public/browser/resource_controller.h:15: virtual void
CancelWithError(int error) = 0;
On 2012/08/10 18:03:02, darin wrote:
> It looks like we don't need CancelWithError since nothing calls this function
in
> this CL.

Done.

http://codereview.chromium.org/10640019/diff/20034/content/public/browser/res...
content/public/browser/resource_controller.h:16: virtual void
HandledExternally() = 0;
On 2012/08/10 18:03:02, darin wrote:
> This doesn't seem like the best name for this function.  We want to tell the
> ResourceLoader to cancel the request _and_ to also forget that the request was
> ever generated.  Maybe CancelAndIgnore or CancelAndSuppressErrorReporting? 
> Neither of these suggestions strike me as all that great.
> 
> Another idea is to make this into a function that synthesizes a HTTP 204 "No
> content" response.  CancelAndReturnNoContent or CancelAndSynthesizeNoContent
or
> CancelAndSimulateNoContent?
> 
> I'm having a hard time coming up with a good solution to this problem.
> 
> CancelAndIgnore is probably my preference at this point.  It at least conveys
> the fact that the URLRequest will be canceled.  Using the term "ignore" is
> consistent with the WebKit API where we have WebNavigationPolicyIgnore, which
is
> roughly equivalent.  Plus if you read all of ResourceController methods with a
> suffix of "Request", this one would be CancelAndIgnore-Request, which is
pretty
> close to my description of "cancel the request and forget that the request was
> ever generated."
> 
> I would then use terms like "was_ignored" instead of "was_handled_externally."

Ok, will go with CancelAndIgnore.

http://codereview.chromium.org/10640019/diff/20034/net/base/net_errors.h
File net/base/net_errors.h (right):

http://codereview.chromium.org/10640019/diff/20034/net/base/net_errors.h#newc...
net/base/net_errors.h:42: // Returns true if |error| is a success error code.
On 2012/08/10 18:03:02, darin wrote:
> I'm not sure we want to add these.  You'll need feedback from net/OWNERS.

I'll pull it out for now. Should I be concerned about folks writing error_code
!= net::OK when they really mean (error_code != net::OK && error_code !=
net::ERR_IO_PENDING)?

http://codereview.chromium.org/10640019/diff/20034/net/url_request/url_reques...
File net/url_request/url_request.cc (left):

http://codereview.chromium.org/10640019/diff/20034/net/url_request/url_reques...
net/url_request/url_request.cc:521: 
On 2012/08/10 18:03:02, darin wrote:
> nit: gratuitous change

Done.

http://codereview.chromium.org/10640019/diff/20034/webkit/glue/resource_loade...
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/10640019/diff/20034/webkit/glue/resource_loade...
webkit/glue/resource_loader_bridge.h:340: bool is_handled_externally,
On 2012/08/10 18:03:02, darin wrote:
> nit: was_handled_externally

changed to 'ignored'.

http://codereview.chromium.org/10640019/diff/20034/webkit/glue/weburlloader_i...
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/10640019/diff/20034/webkit/glue/weburlloader_i...
webkit/glue/weburlloader_impl.cc:644: if (!net::IsSuccess(error_code) ||
is_handled_externally) {
On 2012/08/10 18:03:02, darin wrote:
> I think you should just compare error_code against net::OK here since we don't
> support positive error codes in this context.

Positive error codes support was dropped in one of the previous iterations
(there is no net::OK anymore).

[mkosiba (inactive), 2012-08-29 16:24:42]

New version ready for review.

[darin (slow to review), 2012-08-29 22:01:37]

LGTM w/ these nits fixed.

http://codereview.chromium.org/10640019/diff/30001/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/10640019/diff/30001/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.cc:264: (info && info->Ignored())) {
nit: perhaps it would be worth it to also check for status == CANCELED here? 
that might be a bit easier for the reader to then understand the meaning.

http://codereview.chromium.org/10640019/diff/30001/content/browser/renderer_h...
File content/browser/renderer_host/resource_loader.cc (right):

http://codereview.chromium.org/10640019/diff/30001/content/browser/renderer_h...
content/browser/renderer_host/resource_loader.cc:106: info->set_ignored(true);
nit: Maybe this flag would be more clear if it were named
"set_was_ignored_by_handler".  Then, when you inspect the property on
ResourceRequestInfo, it'll be a function call named "WasIgnoredByHandler."  That
should help people understand that the request is being ignored by the
downstream consumer, etc.

http://codereview.chromium.org/10640019/diff/30001/content/public/browser/res...
File content/public/browser/resource_controller.h (right):

http://codereview.chromium.org/10640019/diff/30001/content/public/browser/res...
content/public/browser/resource_controller.h:15: virtual void CancelAndIgnore()
= 0;
nit: The Cancel and Resume methods are explained by the comment above
ResourceController.  You should also explain what CancelAndIgnore means.

// Used to either resume a deferred resource load or cancel a resource load at
// any time.  CancelAndIgnore is a variation of Cancel that also causes the
// requester of the resource to act like the request was never made.

[mkosiba (inactive), 2012-08-30 15:29:43]

Ok.. this might be the version that's going in. Could you please take a look at
the DCHECK I've added to AsyncResourceHandler? I'm not quite sure if that's the
best place to put it..

http://codereview.chromium.org/10640019/diff/30001/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/10640019/diff/30001/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.cc:264: (info && info->Ignored())) {
On 2012/08/29 22:01:37, darin wrote:
> nit: perhaps it would be worth it to also check for status == CANCELED here? 
> that might be a bit easier for the reader to then understand the meaning.

We already check for status().status() == CANCELED; that check is 9 lines below
in an else if (..) clause.

http://codereview.chromium.org/10640019/diff/30001/content/browser/renderer_h...
File content/browser/renderer_host/resource_loader.cc (right):

http://codereview.chromium.org/10640019/diff/30001/content/browser/renderer_h...
content/browser/renderer_host/resource_loader.cc:106: info->set_ignored(true);
On 2012/08/29 22:01:37, darin wrote:
> nit: Maybe this flag would be more clear if it were named
> "set_was_ignored_by_handler".  Then, when you inspect the property on
> ResourceRequestInfo, it'll be a function call named "WasIgnoredByHandler." 
That
> should help people understand that the request is being ignored by the
> downstream consumer, etc.

Great naming suggestion! Thanks!

http://codereview.chromium.org/10640019/diff/30001/content/public/browser/res...
File content/public/browser/resource_controller.h (right):

http://codereview.chromium.org/10640019/diff/30001/content/public/browser/res...
content/public/browser/resource_controller.h:15: virtual void CancelAndIgnore()
= 0;
On 2012/08/29 22:01:37, darin wrote:
> nit: The Cancel and Resume methods are explained by the comment above
> ResourceController.  You should also explain what CancelAndIgnore means.
> 
> // Used to either resume a deferred resource load or cancel a resource load at
> // any time.  CancelAndIgnore is a variation of Cancel that also causes the
> // requester of the resource to act like the request was never made.

Done.

http://codereview.chromium.org/10640019/diff/44002/content/browser/renderer_h...
File content/browser/renderer_host/async_resource_handler.cc (right):

http://codereview.chromium.org/10640019/diff/44002/content/browser/renderer_h...
content/browser/renderer_host/async_resource_handler.cc:325: // If this check
fails, then we're in an inconsistent state because all
not sure if this is the best place to put the check...

[darin (slow to review), 2012-08-30 16:15:18]

http://codereview.chromium.org/10640019/diff/44002/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/10640019/diff/44002/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.cc:264: (info &&
info->WasIgnoredByHandler())) {
Sorry, my suggestion was to write the code like this:

  if ((request->status().status() == net::URLRequestStatus::SUCCESS) ||
      (request->status().status() == net::URLRequestStatus::CANCELED &&
       info && info->WasIgnoredByHandler())) {
    ...

The check for CANCELED is obviously redundant here, but perhaps it makes the
code a bit clearer.  I am not strongly in favor of doing this, so if you
prefer leaving out the CANCELED check that is OK.

http://codereview.chromium.org/10640019/diff/44002/content/browser/renderer_h...
File content/browser/renderer_host/async_resource_handler.cc (right):

http://codereview.chromium.org/10640019/diff/44002/content/browser/renderer_h...
content/browser/renderer_host/async_resource_handler.cc:314: if (status.status()
== net::URLRequestStatus::IO_PENDING)
It should not be possible for status.status() to be IO_PENDING at this point.

http://codereview.chromium.org/10640019/diff/44002/content/browser/renderer_h...
content/browser/renderer_host/async_resource_handler.cc:321: error_code =
net::ERR_FAILED;
I think you should take error_code from status.error().  You can assert that if
status.status() == CANCELED that the resulting error_code is ERR_ABORTED.

You definitely don't want to map all error codes to ERR_FAILED.

http://codereview.chromium.org/10640019/diff/44002/content/browser/renderer_h...
content/browser/renderer_host/async_resource_handler.cc:328: DCHECK(!info ||
!info->WasIgnoredByHandler() ||
I recommend saving WasIgnoredByHandler to a variable since you use it again
below.

I think you can also assume that info is non-null since it must be at this
point.

I would just write the DCHECK like this:

  // Ignored requests should also be canceled requests.
  DCHECK(!was_ignored_by_handler || error_code == net::ERR_ABORTED);

http://codereview.chromium.org/10640019/diff/44002/content/public/browser/res...
File content/public/browser/resource_request_info.h (right):

http://codereview.chromium.org/10640019/diff/44002/content/public/browser/res...
content/public/browser/resource_request_info.h:91: // True if the request was
canceled due to it being handled by an external
nit: This comment may be a bit too specific.  We don't really know why
CancelAndIgnore was called.  Punting a resource request to an external handler
is certainly the only reason that exists today.  Maybe you could change this
comment to be like so:

  // True if ResourceController::CancelAndIgnore() was called.  For example,
  // the requested URL may be being loaded by an external program.

[mkosiba (inactive), 2012-08-31 14:45:10]

http://codereview.chromium.org/10640019/diff/44002/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/10640019/diff/44002/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.cc:264: (info &&
info->WasIgnoredByHandler())) {
On 2012/08/30 16:15:18, darin wrote:
> Sorry, my suggestion was to write the code like this:
> 
>   if ((request->status().status() == net::URLRequestStatus::SUCCESS) ||
>       (request->status().status() == net::URLRequestStatus::CANCELED &&
>        info && info->WasIgnoredByHandler())) {
>     ...
> 
> The check for CANCELED is obviously redundant here, but perhaps it makes the
> code a bit clearer.  I am not strongly in favor of doing this, so if you
> prefer leaving out the CANCELED check that is OK.

After a bit of browsing the code I think the line should be removed. I don't
think it's currently possible to hit this code path as
ExtensionWebRequestEventRouter::OnCompleted has a DCHECK(...status() ==
URLRequestStatus::SUCCESS) in it that none of the test code encounters.

The only place where we used the HANDLED_EXTERNALLY status code was in the
BufferedResourceHandle::UseAlternateNextHandler but the HANDLED_EXTERNALLY
status code from there ends up being sent to the renderer only (it's just used
as a parameter to a method, it's not actually set on the URLRequest's status
field).

http://codereview.chromium.org/10640019/diff/44002/content/browser/renderer_h...
File content/browser/renderer_host/async_resource_handler.cc (right):

http://codereview.chromium.org/10640019/diff/44002/content/browser/renderer_h...
content/browser/renderer_host/async_resource_handler.cc:314: if (status.status()
== net::URLRequestStatus::IO_PENDING)
On 2012/08/30 16:15:18, darin wrote:
> It should not be possible for status.status() to be IO_PENDING at this point.

Done.

http://codereview.chromium.org/10640019/diff/44002/content/browser/renderer_h...
content/browser/renderer_host/async_resource_handler.cc:321: error_code =
net::ERR_FAILED;
On 2012/08/30 16:15:18, darin wrote:
> I think you should take error_code from status.error().  You can assert that
if
> status.status() == CANCELED that the resulting error_code is ERR_ABORTED.
> 
> You definitely don't want to map all error codes to ERR_FAILED.

This only fixes up cases where code uses URLRequestStatus(CANCELED, 0) or
URLRequestStatus(FAILED, 0). I tried to fix up the callers but there's quite a
few of them (test code mostly), so I thought it might make more sense to fix 'em
up when we get around to removing URLRequestStatus completely.

http://codereview.chromium.org/10640019/diff/44002/content/browser/renderer_h...
content/browser/renderer_host/async_resource_handler.cc:328: DCHECK(!info ||
!info->WasIgnoredByHandler() ||
On 2012/08/30 16:15:18, darin wrote:
> I recommend saving WasIgnoredByHandler to a variable since you use it again
> below.
> 
> I think you can also assume that info is non-null since it must be at this
> point.
> 
> I would just write the DCHECK like this:
> 
>   // Ignored requests should also be canceled requests.
>   DCHECK(!was_ignored_by_handler || error_code == net::ERR_ABORTED);

Done.

http://codereview.chromium.org/10640019/diff/44002/content/public/browser/res...
File content/public/browser/resource_request_info.h (right):

http://codereview.chromium.org/10640019/diff/44002/content/public/browser/res...
content/public/browser/resource_request_info.h:91: // True if the request was
canceled due to it being handled by an external
On 2012/08/30 16:15:18, darin wrote:
> nit: This comment may be a bit too specific.  We don't really know why
> CancelAndIgnore was called.  Punting a resource request to an external handler
> is certainly the only reason that exists today.  Maybe you could change this
> comment to be like so:
> 
>   // True if ResourceController::CancelAndIgnore() was called.  For example,
>   // the requested URL may be being loaded by an external program.

Done.

[commit-bot: I haz the power, 2012-08-31 14:45:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/10640019/40004

[commit-bot: I haz the power, 2012-08-31 14:45:45]

Presubmit check for 10640019-40004 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    net

Presubmit checks took 3.2s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[mkosiba (inactive), 2012-08-31 14:59:14]

Matt,
    Could you please take a look at net/ changes?

[mmenke, 2012-08-31 15:02:59]

On 2012/08/31 14:59:14, Martin Kosiba wrote:
> Matt,
>     Could you please take a look at net/ changes?

net/ LGTM.

[darin (slow to review), 2012-08-31 16:15:51]

LGTM

http://codereview.chromium.org/10640019/diff/40004/content/browser/renderer_h...
File content/browser/renderer_host/async_resource_handler.cc (right):

http://codereview.chromium.org/10640019/diff/40004/content/browser/renderer_h...
content/browser/renderer_host/async_resource_handler.cc:326: error_code ==
net::OK)
nit: indentation... realize you are probably aligning the '==', but that's not
conventional.  just left-align.

[mmenke, 2012-08-31 16:17:23]

http://codereview.chromium.org/10640019/diff/40004/content/browser/renderer_h...
File content/browser/renderer_host/async_resource_handler.cc (right):

http://codereview.chromium.org/10640019/diff/40004/content/browser/renderer_h...
content/browser/renderer_host/async_resource_handler.cc:330: error_code =
net::ERR_FAILED;
Random nit:  Should also use brackets when the predicates in conditionals take
more than one line.

[darin (slow to review), 2012-08-31 16:55:51]

http://codereview.chromium.org/10640019/diff/40004/content/browser/renderer_h...
File content/browser/renderer_host/async_resource_handler.cc (right):

http://codereview.chromium.org/10640019/diff/40004/content/browser/renderer_h...
content/browser/renderer_host/async_resource_handler.cc:330: error_code =
net::ERR_FAILED;
On 2012/08/31 16:17:23, Matt Menke wrote:
> Random nit:  Should also use brackets when the predicates in conditionals take
> more than one line.

Agreed.  I had a similar thought, but didn't realize it was a style rule.

[mkosiba (inactive), 2012-09-06 18:30:22]

http://codereview.chromium.org/10640019/diff/40004/content/browser/renderer_h...
File content/browser/renderer_host/async_resource_handler.cc (right):

http://codereview.chromium.org/10640019/diff/40004/content/browser/renderer_h...
content/browser/renderer_host/async_resource_handler.cc:326: error_code ==
net::OK)
On 2012/08/31 16:15:51, darin wrote:
> nit: indentation... realize you are probably aligning the '==', but that's not
> conventional.  just left-align.

no idea how the indent ended up this way, certainly wasn't intentional.

http://codereview.chromium.org/10640019/diff/40004/content/browser/renderer_h...
content/browser/renderer_host/async_resource_handler.cc:330: error_code =
net::ERR_FAILED;
On 2012/08/31 16:17:23, Matt Menke wrote:
> Random nit:  Should also use brackets when the predicates in conditionals take
> more than one line.

Done.

[commit-bot: I haz the power, 2012-09-06 18:30:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/10640019/44006

[commit-bot: I haz the power, 2012-09-06 21:48:37]

Try job failure for 10640019-44006 (retry) on win_rel for step
"sync_integration_tests".
It's a second try, previously, step "sync_integration_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-09-07 15:57:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/10640019/44006

[commit-bot: I haz the power, 2012-09-07 18:24:32]

Change committed as 155427