Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
21378002:
Support POST in browser navaigation. (Closed)
|
Created:
7 years, 4 months ago by Johnny(Jianning) Ding Modified:
7 years, 4 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, kkimlasb_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionSupport POST in browser navaigation.
The NavigationController already supports POST when navigating a URL, But PageNavigator does not.
This change adds some fieds in OpenURLParams and NavigateParams to pass parameters needed by POST to NavigationController::LoadURLParams.
BUG=89945
TEST=BrowserNavigatorTest.SendRequestUsingPOST
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215518
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 11
Patch Set 3 : #
Total comments: 10
Patch Set 4 : preventing a renderer initiated navigation from using POST #
Total comments: 7
Patch Set 5 : address jam's comments #Patch Set 6 : move PageNavigator Destructor body back #
Messages
Total messages: 25 (0 generated)
thakis@, please review chrome/browser/* avi@, please review content/public/*
John likes content API changes. https://codereview.chromium.org/21378002/diff/1/content/public/browser/page_n... File content/public/browser/page_navigator.h (right): https://codereview.chromium.org/21378002/diff/1/content/public/browser/page_n... content/public/browser/page_navigator.h:80: // opening a URL uses POST if sending POSt without data is disallowed. POSt?
https://codereview.chromium.org/21378002/diff/1/content/public/browser/page_n... File content/public/browser/page_navigator.h (right): https://codereview.chromium.org/21378002/diff/1/content/public/browser/page_n... content/public/browser/page_navigator.h:80: // opening a URL uses POST if sending POSt without data is disallowed. On 2013/07/31 19:35:18, Avi wrote: > POSt? typo fixed. The question is that do we allow send POST request without data? if not, I can drop |uses_post| and check empty of post_data| to indicate whether POST is used.
On 2013/07/31 20:42:47, Johnny(Jianning) Ding wrote: > https://codereview.chromium.org/21378002/diff/1/content/public/browser/page_n... > File content/public/browser/page_navigator.h (right): > > https://codereview.chromium.org/21378002/diff/1/content/public/browser/page_n... > content/public/browser/page_navigator.h:80: // opening a URL uses POST if > sending POSt without data is disallowed. > On 2013/07/31 19:35:18, Avi wrote: > > POSt? > > typo fixed. The question is that do we allow send POST request without data? if > not, I can drop |uses_post| and check empty of post_data| to indicate whether > POST is used. I don't actually know, sorry. (BTW, typos in the change description: "But" > "but", "fieds" > "fields")
https://codereview.chromium.org/21378002/diff/1/chrome/browser/ui/browser_nav... File chrome/browser/ui/browser_navigator.h (right): https://codereview.chromium.org/21378002/diff/1/chrome/browser/ui/browser_nav... chrome/browser/ui/browser_navigator.h:216: bool uses_post; Don't we have the concept of "this navigation uses post data" somewhere already, for the "this was a post request, do you really want to reload?" dialog when reloading a POST page (or going back in history to a post page)? Is that history only? How does that work? Does this here interact sanely with history / repost dialogs?
https://codereview.chromium.org/21378002/diff/1/chrome/browser/ui/browser_nav... File chrome/browser/ui/browser_navigator.h (right): https://codereview.chromium.org/21378002/diff/1/chrome/browser/ui/browser_nav... chrome/browser/ui/browser_navigator.h:216: bool uses_post; On 2013/07/31 21:25:43, Nico wrote: > Don't we have the concept of "this navigation uses post data" somewhere already, > for the "this was a post request, do you really want to reload?" dialog when > reloading a POST page (or going back in history to a post page)? Is that history > only? How does that work? > > Does this here interact sanely with history / repost dialogs? The logic is in NavigationController, right? See https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... NavigationController does support POST, but PageNavigator and its subclass WebContents->Browser::OpenURL doesn't, that is why I have this change
+ben since he wrote browser_navigator.h. Adding POST and postdata in this class feels weird to me (for example, one could now create a PAGE_TRANSITION_AUTO_BOOKMARK NavigateParam which does a post request, which I'm not sure is something we want to allow), but Ben probably knows better if this is in scope for this class.
https://codereview.chromium.org/21378002/diff/13001/content/public/browser/pa... File content/public/browser/page_navigator.h (right): https://codereview.chromium.org/21378002/diff/13001/content/public/browser/pa... content/public/browser/page_navigator.h:81: std::string post_data; take a look at LoadURLParams in content/public/browser/navigation_controller.h over there we use scoped_refptr<base::RefCountedMemory> browser_initiated_post_data; which has benefit of avoiding copies, and you can use a null pointer to indicate no data. (question to content API owners: do we really need both LoadURLParams and OpenURLParams ?)
Charlie: can you take a look? You're more familiar with this stuff than me
In general, supporting POST in the browser process is a very tricky thing to do. There are many types of posts like file uploads we wouldn't be able to support this way. You can see our previous attempt at the more general cross-process POST problem at https://codereview.chromium.org/11193051/, which ended up stalling due to complexity. However, it looks like your use case may be limited enough that we can mostly rely on the existing Android Webview POST logic (e.g., LOAD_TYPE_BROWSER_INITIATED_HTTP_POST). We should make it clear that this is limited support, though. You'll need a test for this as well. Saying it's covered by navigation_controller_impl_unittest is misleading-- that has nothing exercising the new parameters you're introducing. https://codereview.chromium.org/21378002/diff/13001/chrome/browser/ui/browser... File chrome/browser/ui/browser_navigator.cc (right): https://codereview.chromium.org/21378002/diff/13001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator.cc:264: // Only allows the user-initiated navigation to use POST? Why is there a question here? Is this unresolved? https://codereview.chromium.org/21378002/diff/13001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator.cc:268: base::RefCountedString::TakeString(¶ms->post_data); I agree with joth. Is it possible to use scoped_refptr<base::RefCountedMemory> in NavigateParams to be consistent with LoadURLParams? https://codereview.chromium.org/21378002/diff/13001/chrome/browser/ui/browser... File chrome/browser/ui/browser_navigator.h (right): https://codereview.chromium.org/21378002/diff/13001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator.h:215: // Indicates whether this navigation will be sent using POST. We should make it clear here that this is limited support for basic POST data and not things like file uploads. Also, these should be listed up near extra_headers. https://codereview.chromium.org/21378002/diff/13001/content/public/browser/pa... File content/public/browser/page_navigator.h (right): https://codereview.chromium.org/21378002/diff/13001/content/public/browser/pa... content/public/browser/page_navigator.h:75: // Indicates whether this navigation will be sent using POST. Again, please make it clear that this only supports basic POST use cases. https://codereview.chromium.org/21378002/diff/13001/content/public/browser/pa... content/public/browser/page_navigator.h:81: std::string post_data; On 2013/08/01 21:16:50, joth wrote: > take a look at LoadURLParams in content/public/browser/navigation_controller.h > > over there we use > > scoped_refptr<base::RefCountedMemory> browser_initiated_post_data; > > which has benefit of avoiding copies, and you can use a null pointer to indicate > no data. I agree, this seems like it would be better. > (question to content API owners: do we really need both LoadURLParams and > OpenURLParams ?) Joth, maybe you can start a separate thread for that? I'm not sure about the answer.
https://codereview.chromium.org/21378002/diff/13001/chrome/browser/ui/browser... File chrome/browser/ui/browser_navigator.cc (right): https://codereview.chromium.org/21378002/diff/13001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator.cc:264: // Only allows the user-initiated navigation to use POST? On 2013/08/02 16:36:41, creis wrote: > Why is there a question here? Is this unresolved? Done. https://codereview.chromium.org/21378002/diff/13001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator.cc:268: base::RefCountedString::TakeString(¶ms->post_data); On 2013/08/02 16:36:41, creis wrote: > I agree with joth. Is it possible to use scoped_refptr<base::RefCountedMemory> > in NavigateParams to be consistent with LoadURLParams? Done. https://codereview.chromium.org/21378002/diff/13001/chrome/browser/ui/browser... File chrome/browser/ui/browser_navigator.h (right): https://codereview.chromium.org/21378002/diff/13001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator.h:215: // Indicates whether this navigation will be sent using POST. On 2013/08/02 16:36:41, creis wrote: > We should make it clear here that this is limited support for basic POST data > and not things like file uploads. > > Also, these should be listed up near extra_headers. Done. https://codereview.chromium.org/21378002/diff/13001/content/public/browser/pa... File content/public/browser/page_navigator.h (right): https://codereview.chromium.org/21378002/diff/13001/content/public/browser/pa... content/public/browser/page_navigator.h:75: // Indicates whether this navigation will be sent using POST. On 2013/08/02 16:36:41, creis wrote: > Again, please make it clear that this only supports basic POST use cases. Done. https://codereview.chromium.org/21378002/diff/13001/content/public/browser/pa... content/public/browser/page_navigator.h:81: std::string post_data; On 2013/08/02 16:36:41, creis wrote: > On 2013/08/01 21:16:50, joth wrote: > > take a look at LoadURLParams in content/public/browser/navigation_controller.h > > > > over there we use > > > > scoped_refptr<base::RefCountedMemory> browser_initiated_post_data; > > > > which has benefit of avoiding copies, and you can use a null pointer to > indicate > > no data. > > I agree, this seems like it would be better. > > > (question to content API owners: do we really need both LoadURLParams and > > OpenURLParams ?) > > Joth, maybe you can start a separate thread for that? I'm not sure about the > answer. Done.
LGTM with nits. Thanks for adding the tests. https://codereview.chromium.org/21378002/diff/24001/chrome/browser/ui/browser... File chrome/browser/ui/browser_navigator.cc (right): https://codereview.chromium.org/21378002/diff/24001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator.cc:263: // Only allows the browser-initiated navigation to use POST. The comment still seems a bit confusing to me, since you're not checking is_renderer_initiated here. There's nothing in this function preventing a renderer initiated navigation from using this. I'm not concerned about the code, so maybe it's best to just remove the comment. It's clear that "uses_post" causes us to treat it as a POST submission just by looking at this code. https://codereview.chromium.org/21378002/diff/24001/chrome/browser/ui/browser... File chrome/browser/ui/browser_navigator_browsertest.cc (right): https://codereview.chromium.org/21378002/diff/24001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator_browsertest.cc:1297: // This test verifies that navigation can send request using POST. nit: s/navigation/browser initiated navigations/ s/request/requests/ https://codereview.chromium.org/21378002/diff/24001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator_browsertest.cc:1305: chrome::NavigateParams p(MakeNavigateParams()); nit: s/p/params/ https://codereview.chromium.org/21378002/diff/24001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator_browsertest.cc:1308: LOG(INFO) << p.url.spec(); Probably best to remove this log statement unless there's a reason to keep it. https://codereview.chromium.org/21378002/diff/24001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator_browsertest.cc:1316: ASSERT_TRUE(p.target_contents != NULL); nit: Don't need != NULL
https://codereview.chromium.org/21378002/diff/24001/chrome/browser/ui/browser... File chrome/browser/ui/browser_navigator.cc (right): https://codereview.chromium.org/21378002/diff/24001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator.cc:263: // Only allows the browser-initiated navigation to use POST. On 2013/08/02 23:53:43, creis wrote: > The comment still seems a bit confusing to me, since you're not checking > is_renderer_initiated here. There's nothing in this function preventing a > renderer initiated navigation from using this. > > I'm not concerned about the code, so maybe it's best to just remove the comment. > It's clear that "uses_post" causes us to treat it as a POST submission just by > looking at this code. Done. https://codereview.chromium.org/21378002/diff/24001/chrome/browser/ui/browser... File chrome/browser/ui/browser_navigator_browsertest.cc (right): https://codereview.chromium.org/21378002/diff/24001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator_browsertest.cc:1297: // This test verifies that navigation can send request using POST. On 2013/08/02 23:53:43, creis wrote: > nit: s/navigation/browser initiated navigations/ > s/request/requests/ Done. https://codereview.chromium.org/21378002/diff/24001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator_browsertest.cc:1305: chrome::NavigateParams p(MakeNavigateParams()); On 2013/08/02 23:53:43, creis wrote: > nit: s/p/params/ Done. https://codereview.chromium.org/21378002/diff/24001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator_browsertest.cc:1308: LOG(INFO) << p.url.spec(); On 2013/08/02 23:53:43, creis wrote: > Probably best to remove this log statement unless there's a reason to keep it. Done. https://codereview.chromium.org/21378002/diff/24001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator_browsertest.cc:1316: ASSERT_TRUE(p.target_contents != NULL); On 2013/08/02 23:53:43, creis wrote: > nit: Don't need != NULL Done.
lgtm re. my comments https://codereview.chromium.org/21378002/diff/34001/chrome/browser/ui/browser... File chrome/browser/ui/browser_navigator.h (right): https://codereview.chromium.org/21378002/diff/34001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator.h:75: scoped_refptr<base::RefCountedMemory> browser_initiated_post_data; note you could fwd declare rather than include ref_counted_memory
https://codereview.chromium.org/21378002/diff/34001/chrome/browser/ui/browser... File chrome/browser/ui/browser_navigator.h (right): https://codereview.chromium.org/21378002/diff/34001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator.h:75: scoped_refptr<base::RefCountedMemory> browser_initiated_post_data; On 2013/08/03 01:21:11, joth wrote: > note you could fwd declare rather than include ref_counted_memory I tried to include ref_counted_memory, but it ends up needing to add #include "base/memory/ref_counted_memory.h" to all files which include "browser_navigator.h". So I gave up.
https://codereview.chromium.org/21378002/diff/34001/chrome/browser/ui/browser... File chrome/browser/ui/browser_navigator.h (right): https://codereview.chromium.org/21378002/diff/34001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator.h:75: scoped_refptr<base::RefCountedMemory> browser_initiated_post_data; On 2013/08/03 01:44:53, Johnny(Jianning) Ding wrote: > On 2013/08/03 01:21:11, joth wrote: > > note you could fwd declare rather than include ref_counted_memory > > I tried to include ref_counted_memory, but it ends up needing to add > #include "base/memory/ref_counted_memory.h" to all files which include > "browser_navigator.h". So I gave up. Ah yes. the "normal" solution would be to give it explicit copy c'tor and assignment operator defined inside the .cc file, but I think your approach is probably just as good. (avoids the need to remember to include assigning every single member into those methods)
On 2013/08/03 01:44:52, Johnny(Jianning) Ding wrote: > https://codereview.chromium.org/21378002/diff/34001/chrome/browser/ui/browser... > File chrome/browser/ui/browser_navigator.h (right): > > https://codereview.chromium.org/21378002/diff/34001/chrome/browser/ui/browser... > chrome/browser/ui/browser_navigator.h:75: scoped_refptr<base::RefCountedMemory> > browser_initiated_post_data; > On 2013/08/03 01:21:11, joth wrote: > > note you could fwd declare rather than include ref_counted_memory > > I tried to include ref_counted_memory, but it ends up needing to add > #include "base/memory/ref_counted_memory.h" to all files which include > "browser_navigator.h". So I gave up. sorry for typo, I mean I tried to fwd declare ref_counted_memory, but that requires all files which include "browser_navigator.h" to use NavigateParams have to include "base/memory/ref_counted_memory.h" to pass compilation. That will cause I have add many other files in this change. I don't want to mess up this change due to feature launch pressure. Maybe add a TODO?
On 2 August 2013 19:01, <jnd@chromium.org> wrote: > On 2013/08/03 01:44:52, Johnny(Jianning) Ding wrote: > > https://codereview.chromium.**org/21378002/diff/34001/** > chrome/browser/ui/browser_**navigator.h<https://codereview.chromium.org/21378002/diff/34001/chrome/browser/ui/browser_navigator.h> > >> File chrome/browser/ui/browser_**navigator.h (right): >> > > > https://codereview.chromium.**org/21378002/diff/34001/** > chrome/browser/ui/browser_**navigator.h#newcode75<https://codereview.chromium.org/21378002/diff/34001/chrome/browser/ui/browser_navigator.h#newcode75> > >> chrome/browser/ui/browser_**navigator.h:75: >> > scoped_refptr<base::**RefCountedMemory> > >> browser_initiated_post_data; >> >> On 2013/08/03 01:21:11, joth wrote: >> > note you could fwd declare rather than include ref_counted_memory >> > > I tried to include ref_counted_memory, but it ends up needing to add >> #include "base/memory/ref_counted_**memory.h" to all files which include >> "browser_navigator.h". So I gave up. >> > > sorry for typo, > I mean I tried to fwd declare ref_counted_memory, but that requires all > files > which include "browser_navigator.h" to use NavigateParams have to include > "base/memory/ref_counted_**memory.h" to pass compilation. That will cause > I have > add many other files in this change. I don't want to mess up this change > due to > feature launch pressure. Maybe add a TODO? > > I don't think it's even worth a TODO, if jam@ is happy with it as is.
chrome lgtm w nits https://codereview.chromium.org/21378002/diff/34001/chrome/browser/ui/browser... File chrome/browser/ui/browser_navigator.h (right): https://codereview.chromium.org/21378002/diff/34001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator.h:27: } // namespace content ditto. in general, please avoid making stylistic nits like this, the old code was fine https://codereview.chromium.org/21378002/diff/34001/chrome/browser/ui/browser... File chrome/browser/ui/browser_navigator_browsertest.h (right): https://codereview.chromium.org/21378002/diff/34001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator_browsertest.h:22: } // namespace content nit: no need to add the extra "namespace foo" here and above..
https://codereview.chromium.org/21378002/diff/34001/chrome/browser/ui/browser... File chrome/browser/ui/browser_navigator.h (right): https://codereview.chromium.org/21378002/diff/34001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator.h:27: } // namespace content On 2013/08/03 05:23:22, jam wrote: > ditto. > > in general, please avoid making stylistic nits like this, the old code was fine Done. https://codereview.chromium.org/21378002/diff/34001/chrome/browser/ui/browser... File chrome/browser/ui/browser_navigator_browsertest.h (right): https://codereview.chromium.org/21378002/diff/34001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator_browsertest.h:22: } // namespace content On 2013/08/03 05:23:22, jam wrote: > nit: no need to add the extra "namespace foo" here and above.. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jnd@chromium.org/21378002/73001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jnd@chromium.org/21378002/73001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jnd@chromium.org/21378002/98001
Message was sent while issue was closed.
Change committed as 215518 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
