Simplify decidePolicyForNavigation + remove POST-only-in-http(s) DCHECK.

content/browser/frame_host/navigation_controller_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 /*
  * Copyright (C) 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved.
  *     (http://www.torchmobile.com/)
  *
@@ skipping 645 matching lines @@
     // If Telemetry is running, allow the URL load to proceed as if it's
     // unhandled, otherwise Telemetry can't tell if Navigation completed.
     if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
             cc::switches::kEnableGpuBenchmarking))
       return;
   }
 
   // Checks based on params.load_type.
   switch (params.load_type) {
     case LOAD_TYPE_DEFAULT:
-      break;
     case LOAD_TYPE_HTTP_POST:
-      // TODO(lukasza): This assertion is false - it is also possible to POST to
-      // an chrome-extension://... URI.  This might be more common when
-      // allowing renderer-initiated POST after fixing https://crbug.com/344348.
-      if (!params.url.SchemeIs(url::kHttpScheme) &&
-          !params.url.SchemeIs(url::kHttpsScheme)) {
-        NOTREACHED() << "Http post load must use http(s) scheme.";
-        return;
-      }

[Łukasz Anforowicz, 2016/07/20 22:23:04]

I think simply removing the DCHECK is the best option - it doesn't seem right to
try to enforce POST-only-in-http(s) condition only in NavigationConteller, when
other parts of the code apparently are already capable of handling POST to
non-http(s) URIs (we already know about tests that exercise this for file: and
chrome-extension://... URIs).

For completeness, below I try to list other (rejected) alternatives:
- Keep the check above, but replace NOTREACHED with code that changes the method
to GET and drops the POST body.  From test behavior it seems that this is what
//net code must be doing for file: URIs already, so not sure if there is a point
in duplicating this logic.
- Keep the assert here (and maybe even add this assert in other places) after
tweaking the callers to guarantee this condition.  This also seems to duplicate
the fallback-to-GET logic that apparently is already present in //net.

Also just for the record - before putting this CL out for review, I tried to
author a test that would trigger POST to chrome-extension://... URI. 
Unfortunately I failed - despite various attempts of setting
browser_handles_all_top_level_requests from
ExtensionWebRequestApiTest.PostData2, I couldn't make this setting stick into
the right renderer process.

[Charlie Reis, 2016/07/20 22:50:38]

Thanks.  I think I agree, if the net stack is just going to convert it anyway. 
(Otherwise it's kind of meaningless to do a POST to a file URL.)

Do you know if we end up storing the POST data in the file:// NavigationEntry? 
Hopefully not.  (Should be easy to check on a file URL that submits to a new
window, or in the
fast/events/popup-allowed-from-gesture-initiated-form-submit.html test.)

[Charlie Reis, 2016/07/21 23:25:21]

Sure.
Sorry, I meant after commit.
Ah, yes, the post data isn't stored in GetPostData after commmit, apparently. 
As you mention below, we store it in PageState.
Now that's interesting.  I suppose it's not a problem, but it means that we'll
probably prompt for form resubmission on reload (etc) despite being on a file://
URL where that's meaningless.

Anyway, I don't see that causing any particularly bad behavior, so I think it
should be safe to proceed.

       break;
     case LOAD_TYPE_DATA:
       if (!params.url.SchemeIs(url::kDataScheme)) {
         NOTREACHED() << "Data load must use data scheme.";
         return;
       }
       break;
     default:
       NOTREACHED();
       break;
@@ skipping 1392 matching lines @@
     }
   }
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
     const base::Callback<base::Time()>& get_timestamp_callback) {
   get_timestamp_callback_ = get_timestamp_callback;
 }
 
 }  // namespace content