Implement NavigationControllerWebView.PostURL

Implement NavigationControllerWebView.PostURL

Used for android WebView.postUrl

HasPostData in NavigationEntry is used to indicate to the renderer that the
navigation is a http post request. The usage here is slightly overloaded.
HasPostData is set when from the renderer side for a http post request. This
change also uses it to indicate a browser initiated http post request from
PostURL. is_post is added to ViewMsg_Navigate_Params.

A BrowserInitiatedPostData field is added to NavigationEntry in order to pass
the post data to the renderer. Similar field added to ViewMsg_Navigate_Params.
This field is only used for temporarily storing and passing post data to the
renderer. Since the post data is also saved in ContentState after loading the
it is cleared when ContentState is set.

BUG=
TEST=New tests passed

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

[joth, 2012-07-26 20:41:40]

https://chromiumcodereview.appspot.com/10829044/diff/1/content/browser/web_co...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/10829044/diff/1/content/browser/web_co...
content/browser/web_contents/navigation_controller_impl.cc:762:
active_entry->SetBrowserInitiatedPostData("");
I think we prefer std::string() to ""

https://chromiumcodereview.appspot.com/10829044/diff/1/content/public/browser...
File content/public/browser/navigation_entry.h (right):

https://chromiumcodereview.appspot.com/10829044/diff/1/content/public/browser...
content/public/browser/navigation_entry.h:126: // whether the page had post
data.
should this comment be updated as per the second paragraph of the change
description?

https://chromiumcodereview.appspot.com/10829044/diff/1/content/public/browser...
content/public/browser/navigation_entry.h:155: virtual void
SetBrowserInitiatedPostData(const std::string& data) = 0;
Would it make sense to have this method automatically call SetHasPostData(true)
? If so, document that here :)

Also, these two methods may as well go up on line 133 with HasPostData.

https://chromiumcodereview.appspot.com/10829044/diff/1/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10829044/diff/1/content/renderer/rende...
content/renderer/render_view_impl.cc:1101:
params.browser_initiated_post_data.c_str(),
c_str() -> data()

[boliu, 2012-07-26 21:06:45]

Hi jam, jamesr,

Can I get an initial review of this CL to see if the approach is acceptable?

This CL implements code paths neeeded for android WebView.postUrl:
http://developer.android.com/reference/android/webkit/WebView.html#postUrl(ja...,
byte[])

Patch Set 1 is not for commit yet since I did not add any tests and there is
still a TODO left in the code.

[boliu, 2012-07-27 22:47:40]

Ping! Patch set 4 is ready for review and submission.

https://chromiumcodereview.appspot.com/10829044/diff/1/content/browser/web_co...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/10829044/diff/1/content/browser/web_co...
content/browser/web_contents/navigation_controller_impl.cc:762:
active_entry->SetBrowserInitiatedPostData("");
On 2012/07/26 20:41:40, joth wrote:
> I think we prefer std::string() to ""

Not relevant since moving to vector<char>

https://chromiumcodereview.appspot.com/10829044/diff/1/content/public/browser...
File content/public/browser/navigation_entry.h (right):

https://chromiumcodereview.appspot.com/10829044/diff/1/content/public/browser...
content/public/browser/navigation_entry.h:126: // whether the page had post
data.
On 2012/07/26 20:41:40, joth wrote:
> should this comment be updated as per the second paragraph of the change
> description?

Done.

https://chromiumcodereview.appspot.com/10829044/diff/1/content/public/browser...
content/public/browser/navigation_entry.h:155: virtual void
SetBrowserInitiatedPostData(const std::string& data) = 0;
On 2012/07/26 20:41:40, joth wrote:
> Would it make sense to have this method automatically call
SetHasPostData(true)
> ? If so, document that here :)
> 
> Also, these two methods may as well go up on line 133 with HasPostData.

I don't think it should call SetHasPostData(true) since this is a simple data
container class and should not contain any hidden logic. For example SetPostID
does not call SetHasPostData either.

Moved to below PostID fields

https://chromiumcodereview.appspot.com/10829044/diff/1/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10829044/diff/1/content/renderer/rende...
content/renderer/render_view_impl.cc:1101:
params.browser_initiated_post_data.c_str(),
On 2012/07/26 20:41:40, joth wrote:
> c_str() -> data()

Not relevant anymore after moving to vector<char>

[joth, 2012-07-27 23:12:37]

lg, but IANAO

https://chromiumcodereview.appspot.com/10829044/diff/1/content/public/browser...
File content/public/browser/navigation_entry.h (right):

https://chromiumcodereview.appspot.com/10829044/diff/1/content/public/browser...
content/public/browser/navigation_entry.h:155: virtual void
SetBrowserInitiatedPostData(const std::string& data) = 0;
On 2012/07/27 22:47:41, boliu wrote:
> On 2012/07/26 20:41:40, joth wrote:
> > Would it make sense to have this method automatically call
> SetHasPostData(true)
> > ? If so, document that here :)
> > 
> > Also, these two methods may as well go up on line 133 with HasPostData.
> 
> I don't think it should call SetHasPostData(true) since this is a simple data
> container class and should not contain any hidden logic. For example SetPostID
> does not call SetHasPostData either.
> 
> Moved to below PostID fields

Makes sense.

https://chromiumcodereview.appspot.com/10829044/diff/9001/content/public/brow...
File content/public/browser/navigation_entry.h (right):

https://chromiumcodereview.appspot.com/10829044/diff/9001/content/public/brow...
content/public/browser/navigation_entry.h:145: virtual const std::vector<char>&
GetBrowserInitiatedPostData() const = 0;
Hmmm rather than pull in 'vector' another option would be to use
base::RefCountedMemory, as this allows the client to choose the most convenient
container and easily avoids a couple of the copies along the line.
(it also allows a nice differentiation of empty vs null data)

Up to the API OWNERS whether that's a concept they'd like included here?

[boliu, 2012-07-27 23:17:36]

http://codereview.chromium.org/10829044/diff/9001/content/public/browser/navi...
File content/public/browser/navigation_entry.h (right):

http://codereview.chromium.org/10829044/diff/9001/content/public/browser/navi...
content/public/browser/navigation_entry.h:145: virtual const std::vector<char>&
GetBrowserInitiatedPostData() const = 0;
Does RefCountedMemory work with IPC? This data crosses IPC boundary , and git
grep RefCountedMemory | grep IPC didn't find an instance where it's used for IPC
messages.

[jam, 2012-07-27 23:36:14]

Charlie: can you take a look at this too? Thanks

http://codereview.chromium.org/10829044/diff/9001/content/public/browser/navi...
File content/public/browser/navigation_entry.h (right):

http://codereview.chromium.org/10829044/diff/9001/content/public/browser/navi...
content/public/browser/navigation_entry.h:145: virtual const std::vector<char>&
GetBrowserInitiatedPostData() const = 0;
On 2012/07/27 23:17:36, boliu wrote:
> Does RefCountedMemory work with IPC?

no. you can send shared memory though.

who generates this browser-initiated posts? i.e. how big can it be? if it's in
the MBs, it should be sent over IPC as shared memory.

> This data crosses IPC boundary , and git
> grep RefCountedMemory | grep IPC didn't find an instance where it's used for
IPC
> messages.

http://codereview.chromium.org/10829044/diff/9001/content/renderer/render_vie...
File content/renderer/render_view_browsertest.cc (right):

http://codereview.chromium.org/10829044/diff/9001/content/renderer/render_vie...
content/renderer/render_view_browsertest.cc:317: // Set up post ata
nit; data

http://codereview.chromium.org/10829044/diff/9001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10829044/diff/9001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:1097: // set post data
nit: capitalization and period per the style guide

[joth, 2012-07-28 00:34:49]

On 2012/07/27 23:36:14, John Abd-El-Malek wrote:
> Charlie: can you take a look at this too? Thanks
> 
>
http://codereview.chromium.org/10829044/diff/9001/content/public/browser/navi...
> File content/public/browser/navigation_entry.h (right):
> 
>
http://codereview.chromium.org/10829044/diff/9001/content/public/browser/navi...
> content/public/browser/navigation_entry.h:145: virtual const
std::vector<char>&
> GetBrowserInitiatedPostData() const = 0;
> On 2012/07/27 23:17:36, boliu wrote:
> > Does RefCountedMemory work with IPC?
> 
> no. you can send shared memory though.
> 
> who generates this browser-initiated posts? i.e. how big can it be? if it's in
> the MBs, it should be sent over IPC as shared memory.
> 

The data comes from the app -- there's like a million apps using this, so one or
two no doubt send megabytes.


My suggestion on RefCounted memory was just an easy win for the 2 or 3
in-process copies that occur in the current design (and, adds a bit of API
flexibility too). I think optimizing the IPC is more effort and only worth it if
we find strong need for it.



> > This data crosses IPC boundary , and git
> > grep RefCountedMemory | grep IPC didn't find an instance where it's used for
> IPC
> > messages.
> 
>
http://codereview.chromium.org/10829044/diff/9001/content/renderer/render_vie...
> File content/renderer/render_view_browsertest.cc (right):
> 
>
http://codereview.chromium.org/10829044/diff/9001/content/renderer/render_vie...
> content/renderer/render_view_browsertest.cc:317: // Set up post ata
> nit; data
> 
>
http://codereview.chromium.org/10829044/diff/9001/content/renderer/render_vie...
> File content/renderer/render_view_impl.cc (right):
> 
>
http://codereview.chromium.org/10829044/diff/9001/content/renderer/render_vie...
> content/renderer/render_view_impl.cc:1097: // set post data
> nit: capitalization and period per the style guide

[Charlie Reis, 2012-07-28 00:36:59]

On 2012/07/27 23:36:14, John Abd-El-Malek wrote:
> Charlie: can you take a look at this too? Thanks

Yes, I'll take a look on Monday.  We may actually already have code for doing a
post submission from the browser process, since I think we're able to do it from
session restore.

[boliu, 2012-07-28 01:06:17]

As far as I know, session restore post data is stored in the opaque
NavigationEntry::Get/SetContentState. It is only decoded on the renderer side.
If it were to be used, then the data will need to be encoded first on the
browser side then decoded on the renderer side, which seems a bit hacky.

Here are the copies of the post data for the entire call:
1) From java byte[] array to a c++ vector<char> local variable (code not in this
CL)
2) From vector<char> to NavigationEntry.
3) From NavigationEntry to ipc ViewMsg_NavigateParams.
4) IPC copying ViewMsg_NavigateParams to renderer (I think this is the case by
John's comment?)
5) On Renderer side, from ViewMsg_NavigateParams to WebURLRequest to be handed
off to WebKit.

This does *not* include copying ContentState, which contains the post data, back
to the browser.

I think 1) and 5) are unavoidable since it crosses language/module boundaries.
2) can be avoided by using RefCountedMemory. 3) and 4) can be avoided by also
using shared memory (I think?)

Will switch to RefCountedMemory to avoid 2) since it's easy. Not sure if it's
worthwhile to optimize anything else in that path since the usage in apps is
unclear. Comments?

[boliu, 2012-07-30 18:52:08]

Switched to using RefCountedBytes and saves step 2). Did not use
RefCountedMemory super class in method definitions because I thought step 3)
could be avoided, but see inline comment why this was not done.

If RefCountedMemory is preferred in function/method definitions, I can update
this cl.

[boliu, 2012-07-30 18:52:40]

And the inline comment.

http://codereview.chromium.org/10829044/diff/8012/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

http://codereview.chromium.org/10829044/diff/8012/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:241:
params->browser_initiated_post_data =
A copy can be saved here by using vector::swap instead of assignment. But that
breaks that assumption that WebContentsImpl::NavigateToEntry does not modify
NavigationEntryImpl in any way.

[joth, 2012-07-30 19:10:18]

I'm not overly excited by switching from vector->RefCountedMem just for saving
one copy, but it would give us an easy way to save one more in future e.g.
change IPC params know how to read data-blob input from RefCountedMem  (i.e.
copy #3 in your list)

If we do use it in the public API, it should definitely be the RefCountedMemory
base-class rather than any of the subclasses.

http://codereview.chromium.org/10829044/diff/8012/content/public/browser/andr...
File content/public/browser/android/navigation_controller_webview.h (right):

http://codereview.chromium.org/10829044/diff/8012/content/public/browser/andr...
content/public/browser/android/navigation_controller_webview.h:8: #include
<vector>
not needed

http://codereview.chromium.org/10829044/diff/8012/content/public/browser/andr...
content/public/browser/android/navigation_controller_webview.h:38: const
scoped_refptr<base::RefCountedBytes> http_body,
if we want to use refcountedXxx at all, it should be RefCountedMemory. Otherwise
it gives the client not extra flexibility over just passing a vector.

I think* the preferred style is to pass the naked pointer (RefCountedMemory*
http_body) or if a scoped_ptr<> is specified, it should be a const reference .

(* there was a long debate on the maillist last year, no strong consensus
reached IIRC)

http://codereview.chromium.org/10829044/diff/8012/content/public/browser/navi...
File content/public/browser/navigation_entry.h (right):

http://codereview.chromium.org/10829044/diff/8012/content/public/browser/navi...
content/public/browser/navigation_entry.h:9: #include <vector>
again not needed

http://codereview.chromium.org/10829044/diff/8012/content/public/browser/navi...
content/public/browser/navigation_entry.h:148: GetBrowserInitiatedPostData()
const = 0;
input is raw pointer of const&

return type raw pointer or return the scoped_ptr by value, but non-const.


ah here we are
https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thr...

yes raw pointers seemed generally preferred.

[Charlie Reis, 2012-07-30 20:36:38]

On 2012/07/28 01:06:17, boliu wrote:
> As far as I know, session restore post data is stored in the opaque
> NavigationEntry::Get/SetContentState. It is only decoded on the renderer side.
> If it were to be used, then the data will need to be encoded first on the
> browser side then decoded on the renderer side, which seems a bit hacky.

Ok, navigation-related changes LGTM then.  I'll let you all work out the details
for copying.

http://codereview.chromium.org/10829044/diff/8012/content/browser/web_content...
File content/browser/web_contents/navigation_controller_impl.cc (right):

http://codereview.chromium.org/10829044/diff/8012/content/browser/web_content...
content/browser/web_contents/navigation_controller_impl.cc:661: // Must be http
scheme for a post request
nit: End with a period.

http://codereview.chromium.org/10829044/diff/8012/content/browser/web_content...
File content/browser/web_contents/navigation_controller_impl_unittest.cc
(right):

http://codereview.chromium.org/10829044/diff/8012/content/browser/web_content...
content/browser/web_contents/navigation_controller_impl_unittest.cc:586:
EXPECT_TRUE(post_entry->GetHasPostData());
That's an awkward name.  Should it be HasPostData()?

[boliu, 2012-07-30 21:53:45]

http://codereview.chromium.org/10829044/diff/8012/content/browser/web_content...
File content/browser/web_contents/navigation_controller_impl.cc (right):

http://codereview.chromium.org/10829044/diff/8012/content/browser/web_content...
content/browser/web_contents/navigation_controller_impl.cc:661: // Must be http
scheme for a post request
On 2012/07/30 20:36:38, creis wrote:
> nit: End with a period.

Done.

http://codereview.chromium.org/10829044/diff/8012/content/browser/web_content...
File content/browser/web_contents/navigation_controller_impl_unittest.cc
(right):

http://codereview.chromium.org/10829044/diff/8012/content/browser/web_content...
content/browser/web_contents/navigation_controller_impl_unittest.cc:586:
EXPECT_TRUE(post_entry->GetHasPostData());
This is an existing method in NavigationEntry; I'm only overloading its usage in
this CL. The Set/Get pattern is used throughout NavigationEntry class, so seems
inappropriate for me to change.

http://codereview.chromium.org/10829044/diff/8012/content/public/browser/andr...
File content/public/browser/android/navigation_controller_webview.h (right):

http://codereview.chromium.org/10829044/diff/8012/content/public/browser/andr...
content/public/browser/android/navigation_controller_webview.h:8: #include
<vector>
On 2012/07/30 19:10:18, joth wrote:
> not needed

Done.

http://codereview.chromium.org/10829044/diff/8012/content/public/browser/andr...
content/public/browser/android/navigation_controller_webview.h:38: const
scoped_refptr<base::RefCountedBytes> http_body,
Switch to const& here since it's an input as per the discussion thread.

http://codereview.chromium.org/10829044/diff/8012/content/public/browser/navi...
File content/public/browser/navigation_entry.h (right):

http://codereview.chromium.org/10829044/diff/8012/content/public/browser/navi...
content/public/browser/navigation_entry.h:9: #include <vector>
On 2012/07/30 19:10:18, joth wrote:
> again not needed

Done.

http://codereview.chromium.org/10829044/diff/8012/content/public/browser/navi...
content/public/browser/navigation_entry.h:148: GetBrowserInitiatedPostData()
const = 0;
Using const* instead of const& for Set so it is possible to pass in NULL.

Switch to scoped_refptr<const base::RefCountedMemory> for Get. scoped_refptr is
used because this value is very short lived and returning a raw pointer seems a
bit dangerous. The added const is necessary due to the const in the Set method.
RefCountedMemory only provides const public methods so this is okay.

[boliu, 2012-07-31 00:05:11]

Hi John, could you take another look at this cl? I think it is about ready for
commit.

http://codereview.chromium.org/10829044/diff/11021/content/public/browser/nav...
File content/public/browser/navigation_entry.h (right):

http://codereview.chromium.org/10829044/diff/11021/content/public/browser/nav...
content/public/browser/navigation_entry.h:149: virtual const
base::RefCountedMemory* GetBrowserInitiatedPostData() const = 0;
Per discussion with joth, changed to return a raw pointer since a this is not
thread safe so scoped_refptr does not offer much protection.

[jam, 2012-07-31 01:45:39]

creis is an owner now, so deferring to him

[boliu, 2012-07-31 15:33:29]

Hi Charlie, do you want to take another look or can I try landing with CQ?

[Charlie Reis, 2012-07-31 16:39:42]

Still LGTM.

[commit-bot: I haz the power, 2012-07-31 17:00:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/10829044/11021

[commit-bot: I haz the power, 2012-07-31 18:32:49]

Change committed as 149232