Using ResourceRequestBody as the type of HTTP body outside of //content.

Using ResourceRequestBody as the type of HTTP body outside of //content.

This CL changes representation of HTTP body used above //content layer.
Before this CL, HTTP body was represented by base::RefCountedMemory;
after this CL it is represented by a more generic ResourceRequestBody.
This lays the foundations for passing an arbitrary HTTP body out of and
back into //content via content::OpenURLParams and
NavigationController::LoadURLParams.  This ability will be used in a
later CL to preserve body of HTTP POST for navigations handled via
FrameHostMsg_OpenURL IPC message.

This CL adjusts names of fields and enum and values to reflect that
ResourceRequestBody can now represent not only browser-initiated POST
data, but also renderer-initiated POST data.  Such change should be safe
(since we already track who initiated the navigation in
|is_renderer_initiated| field and should make security decisions based
on this field).

Summary of the changes:

- Renamed fields and enum values:
  - |browser_initiated_post_data| -> |post_data|.
  - |LOAD_TYPE_BROWSER_INITIATED_HTTP_POST| -> |LOAD_TYPE_HTTP_POST|.

- Changed type of |browser_initiated_post_data| aka |post_data|:
  - |scoped_refptr<base::RefCountedMemory>| ->
    |scoped_refptr<content::ResourceRequestBody>|
  - Java byte[] -> ResourceRequestBody Java class that wraps a
    serialized form of the native content::ResourceRequestBody.

- Exposed ability to create ResourceRequestBody out of a byte array
  - in C++ (used by test code and DoSearchByImageInNewTab) and
    Java (needed by a public WebView::postUrl API in Android SDK).

- Follow-up changes
  - Removed no longer needed code:
    NavigationEntryImpl::ConstructBodyFromBrowserInitiatedPostData
  - Tweaked tests to account for new behavior (most imporatantly - reverse
    expectations in SendRendererInitiatedRequestUsingPOST test in
    chrome/browser/ui/browser_navigator_browsertest.cc)
  - Forwarding post body into LoadURLParams without looking at
    |is_renderer_initiated| (in chrome/browser/ui/browser_navigator.cc,
    components/web_contents_delegate_android/web_contents_delegate_android.cc
    and content/shell/browser/shell.cc).

BUG=344348
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/477a5a270598c17e72365572114490ae60e1ef56
Cr-Commit-Position: refs/heads/master@{#400211}

[Łukasz Anforowicz, 2016-06-06 17:10:01]

Description was changed from

==========
Using ResourceRequestBody as the type of browser_initiated_post_data.

This CL uses ResourceRequestBody to represent HTTP body in navigation
requests outside of //content layer.  This lays the foundations for
passing of arbitrary HTTP body out of and back into //content via
content::OpenURLParams and NavigationController::LoadURLParams.
This ability will be used in a later CL to preserve body of HTTP POST
for navigations handled via FrameHostMsg_OpenURL IPC message.

Since this CL replaces std::vector<char> with ResourceRequestBody,
it also adds //content public API for creating ResourceRequestBody
out of a vector of bytes.  At the same time, ability to create a
ResourceRequestBody that wraps files (or refers to filesystem or blob
URLs) remains restricted to internal code inside //content layer.

This CL adjusts names of fields and enum and values to reflect that
ResourceRequestBody can represent not only browser-initiated POST data,
but also renderer-initiated POST data.  Such change should be safe
(since we already track who initiated the navigation in
|is_renderer_initiated| field and should make security decisions based
on this field).  Renamed fields and enum values:
- |browser_initiated_post_data| -> |post_data|.
- |LOAD_TYPE_BROWSER_INITIATED_HTTP_POST| -> |LOAD_TYPE_HTTP_POST|.

BUG=344348
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Using ResourceRequestBody as the type of HTTP body outside of //content.

This CL uses ResourceRequestBody to represent HTTP body in navigation
requests outside of //content layer.  This lays the foundations for
passing of arbitrary HTTP body out of and back into //content via
content::OpenURLParams and NavigationController::LoadURLParams.
This ability will be used in a later CL to preserve body of HTTP POST
for navigations handled via FrameHostMsg_OpenURL IPC message.

Since this CL replaces std::vector<char> with ResourceRequestBody,
it also adds //content public API for creating ResourceRequestBody
out of a vector of bytes.  At the same time, ability to create a
ResourceRequestBody that wraps files (or refers to filesystem or blob
URLs) remains restricted to internal code inside //content layer.

This CL adjusts names of fields and enum and values to reflect that
ResourceRequestBody can represent not only browser-initiated POST data,
but also renderer-initiated POST data.  Such change should be safe
(since we already track who initiated the navigation in
|is_renderer_initiated| field and should make security decisions based
on this field).

Summary of the changes:
- Renamed fields and enum values:
  - |browser_initiated_post_data| -> |post_data|.
  - |LOAD_TYPE_BROWSER_INITIATED_HTTP_POST| -> |LOAD_TYPE_HTTP_POST|.
- Changed type of |browser_initiated_post_data| aka |post_data|:
  - |scoped_refptr<base::RefCountedMemory>| ->
    |scoped_refptr<content::ResourceRequestBody>|
  - Java byte[] -> ResourceRequestBody wrapping a native pointer
    to content::ResourceRequestBody.

BUG=344348
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Łukasz Anforowicz, 2016-06-08 22:11:25]

Description was changed from

==========
Using ResourceRequestBody as the type of HTTP body outside of //content.

This CL uses ResourceRequestBody to represent HTTP body in navigation
requests outside of //content layer.  This lays the foundations for
passing of arbitrary HTTP body out of and back into //content via
content::OpenURLParams and NavigationController::LoadURLParams.
This ability will be used in a later CL to preserve body of HTTP POST
for navigations handled via FrameHostMsg_OpenURL IPC message.

Since this CL replaces std::vector<char> with ResourceRequestBody,
it also adds //content public API for creating ResourceRequestBody
out of a vector of bytes.  At the same time, ability to create a
ResourceRequestBody that wraps files (or refers to filesystem or blob
URLs) remains restricted to internal code inside //content layer.

This CL adjusts names of fields and enum and values to reflect that
ResourceRequestBody can represent not only browser-initiated POST data,
but also renderer-initiated POST data.  Such change should be safe
(since we already track who initiated the navigation in
|is_renderer_initiated| field and should make security decisions based
on this field).

Summary of the changes:
- Renamed fields and enum values:
  - |browser_initiated_post_data| -> |post_data|.
  - |LOAD_TYPE_BROWSER_INITIATED_HTTP_POST| -> |LOAD_TYPE_HTTP_POST|.
- Changed type of |browser_initiated_post_data| aka |post_data|:
  - |scoped_refptr<base::RefCountedMemory>| ->
    |scoped_refptr<content::ResourceRequestBody>|
  - Java byte[] -> ResourceRequestBody wrapping a native pointer
    to content::ResourceRequestBody.

BUG=344348
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Using ResourceRequestBody as the type of HTTP body outside of //content.

This CL uses ResourceRequestBody to represent HTTP body in navigation
requests outside of //content layer.  This lays the foundations for
passing of arbitrary HTTP body out of and back into //content via
content::OpenURLParams and NavigationController::LoadURLParams.
This ability will be used in a later CL to preserve body of HTTP POST
for navigations handled via FrameHostMsg_OpenURL IPC message.

This CL adjusts names of fields and enum and values to reflect that
ResourceRequestBody can represent not only browser-initiated POST data,
but also renderer-initiated POST data.  Such change should be safe
(since we already track who initiated the navigation in
|is_renderer_initiated| field and should make security decisions based
on this field).

Summary of the changes:

- Renamed fields and enum values:
  - |browser_initiated_post_data| -> |post_data|.
  - |LOAD_TYPE_BROWSER_INITIATED_HTTP_POST| -> |LOAD_TYPE_HTTP_POST|.

- Changed type of |browser_initiated_post_data| aka |post_data|:
  - |scoped_refptr<base::RefCountedMemory>| ->
    |scoped_refptr<content::ResourceRequestBody>|
  - Java byte[] -> ResourceRequestBody wrapping a native pointer
    to content::ResourceRequestBody.

- Expose ability to create ResourceRequestBody out of a byte array
  - in C++ (used by test code) and Java (needed by a public
    WebView::postUrl API in Android SDK).

- Follow-up changes
  - Remove no longer needed code:
    NavigationEntryImpl::ConstructBodyFromBrowserInitiatedPostData
  - Tweak tests to account for new behavior (most imporatantly - reverse
    expectations in SendRendererInitiatedRequestUsingPOST test in
    chrome/browser/ui/browser_navigator_browsertest.cc)
  - Forward post body into LoadURLParams without looking at
    |is_renderer_initiated| (in chrome/browser/ui/browser_navigator.cc,
    components/web_contents_delegate_android/web_contents_delegate_android.cc
    and content/shell/browser/shell.cc).

BUG=344348
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Łukasz Anforowicz, 2016-06-08 23:00:32]

Description was changed from

==========
Using ResourceRequestBody as the type of HTTP body outside of //content.

This CL uses ResourceRequestBody to represent HTTP body in navigation
requests outside of //content layer.  This lays the foundations for
passing of arbitrary HTTP body out of and back into //content via
content::OpenURLParams and NavigationController::LoadURLParams.
This ability will be used in a later CL to preserve body of HTTP POST
for navigations handled via FrameHostMsg_OpenURL IPC message.

This CL adjusts names of fields and enum and values to reflect that
ResourceRequestBody can represent not only browser-initiated POST data,
but also renderer-initiated POST data.  Such change should be safe
(since we already track who initiated the navigation in
|is_renderer_initiated| field and should make security decisions based
on this field).

Summary of the changes:

- Renamed fields and enum values:
  - |browser_initiated_post_data| -> |post_data|.
  - |LOAD_TYPE_BROWSER_INITIATED_HTTP_POST| -> |LOAD_TYPE_HTTP_POST|.

- Changed type of |browser_initiated_post_data| aka |post_data|:
  - |scoped_refptr<base::RefCountedMemory>| ->
    |scoped_refptr<content::ResourceRequestBody>|
  - Java byte[] -> ResourceRequestBody wrapping a native pointer
    to content::ResourceRequestBody.

- Expose ability to create ResourceRequestBody out of a byte array
  - in C++ (used by test code) and Java (needed by a public
    WebView::postUrl API in Android SDK).

- Follow-up changes
  - Remove no longer needed code:
    NavigationEntryImpl::ConstructBodyFromBrowserInitiatedPostData
  - Tweak tests to account for new behavior (most imporatantly - reverse
    expectations in SendRendererInitiatedRequestUsingPOST test in
    chrome/browser/ui/browser_navigator_browsertest.cc)
  - Forward post body into LoadURLParams without looking at
    |is_renderer_initiated| (in chrome/browser/ui/browser_navigator.cc,
    components/web_contents_delegate_android/web_contents_delegate_android.cc
    and content/shell/browser/shell.cc).

BUG=344348
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Using ResourceRequestBody as the type of HTTP body outside of //content.

This CL uses ResourceRequestBody to represent HTTP body in navigation
requests outside of //content layer.  This lays the foundations for
passing of arbitrary HTTP body out of and back into //content via
content::OpenURLParams and NavigationController::LoadURLParams.
This ability will be used in a later CL to preserve body of HTTP POST
for navigations handled via FrameHostMsg_OpenURL IPC message.

This CL adjusts names of fields and enum and values to reflect that
ResourceRequestBody can represent not only browser-initiated POST data,
but also renderer-initiated POST data.  Such change should be safe
(since we already track who initiated the navigation in
|is_renderer_initiated| field and should make security decisions based
on this field).

Summary of the changes:

- Renamed fields and enum values:
  - |browser_initiated_post_data| -> |post_data|.
  - |LOAD_TYPE_BROWSER_INITIATED_HTTP_POST| -> |LOAD_TYPE_HTTP_POST|.

- Changed type of |browser_initiated_post_data| aka |post_data|:
  - |scoped_refptr<base::RefCountedMemory>| ->
    |scoped_refptr<content::ResourceRequestBody>|
  - Java byte[] -> ResourceRequestBody wrapping a native pointer
    to content::ResourceRequestBody.

- Expose ability to create ResourceRequestBody out of a byte array
  - in C++ (used by test code) and Java (needed by a public
    WebView::postUrl API in Android SDK).

- Follow-up changes
  - Remove no longer needed code:
    NavigationEntryImpl::ConstructBodyFromBrowserInitiatedPostData
  - Tweak tests to account for new behavior (most imporatantly - reverse
    expectations in SendRendererInitiatedRequestUsingPOST test in
    chrome/browser/ui/browser_navigator_browsertest.cc)
  - Forward post body into LoadURLParams without looking at
    |is_renderer_initiated| (in chrome/browser/ui/browser_navigator.cc,
    components/web_contents_delegate_android/web_contents_delegate_android.cc
    and content/shell/browser/shell.cc).

BUG=344348
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Łukasz Anforowicz, 2016-06-08 23:55:09]

Description was changed from

==========
Using ResourceRequestBody as the type of HTTP body outside of //content.

This CL uses ResourceRequestBody to represent HTTP body in navigation
requests outside of //content layer.  This lays the foundations for
passing of arbitrary HTTP body out of and back into //content via
content::OpenURLParams and NavigationController::LoadURLParams.
This ability will be used in a later CL to preserve body of HTTP POST
for navigations handled via FrameHostMsg_OpenURL IPC message.

This CL adjusts names of fields and enum and values to reflect that
ResourceRequestBody can represent not only browser-initiated POST data,
but also renderer-initiated POST data.  Such change should be safe
(since we already track who initiated the navigation in
|is_renderer_initiated| field and should make security decisions based
on this field).

Summary of the changes:

- Renamed fields and enum values:
  - |browser_initiated_post_data| -> |post_data|.
  - |LOAD_TYPE_BROWSER_INITIATED_HTTP_POST| -> |LOAD_TYPE_HTTP_POST|.

- Changed type of |browser_initiated_post_data| aka |post_data|:
  - |scoped_refptr<base::RefCountedMemory>| ->
    |scoped_refptr<content::ResourceRequestBody>|
  - Java byte[] -> ResourceRequestBody wrapping a native pointer
    to content::ResourceRequestBody.

- Expose ability to create ResourceRequestBody out of a byte array
  - in C++ (used by test code) and Java (needed by a public
    WebView::postUrl API in Android SDK).

- Follow-up changes
  - Remove no longer needed code:
    NavigationEntryImpl::ConstructBodyFromBrowserInitiatedPostData
  - Tweak tests to account for new behavior (most imporatantly - reverse
    expectations in SendRendererInitiatedRequestUsingPOST test in
    chrome/browser/ui/browser_navigator_browsertest.cc)
  - Forward post body into LoadURLParams without looking at
    |is_renderer_initiated| (in chrome/browser/ui/browser_navigator.cc,
    components/web_contents_delegate_android/web_contents_delegate_android.cc
    and content/shell/browser/shell.cc).

BUG=344348
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Using ResourceRequestBody as the type of HTTP body outside of //content.

This CL uses ResourceRequestBody to represent HTTP body in navigation
requests outside of //content layer.  This lays the foundations for
passing of arbitrary HTTP body out of and back into //content via
content::OpenURLParams and NavigationController::LoadURLParams.
This ability will be used in a later CL to preserve body of HTTP POST
for navigations handled via FrameHostMsg_OpenURL IPC message.

This CL adjusts names of fields and enum and values to reflect that
ResourceRequestBody can represent not only browser-initiated POST data,
but also renderer-initiated POST data.  Such change should be safe
(since we already track who initiated the navigation in
|is_renderer_initiated| field and should make security decisions based
on this field).

Summary of the changes:

- Renamed fields and enum values:
  - |browser_initiated_post_data| -> |post_data|.
  - |LOAD_TYPE_BROWSER_INITIATED_HTTP_POST| -> |LOAD_TYPE_HTTP_POST|.

- Changed type of |browser_initiated_post_data| aka |post_data|:
  - |scoped_refptr<base::RefCountedMemory>| ->
    |scoped_refptr<content::ResourceRequestBody>|
  - Java byte[] -> ResourceRequestBody wrapping a native pointer
    to content::ResourceRequestBody.

- Exposed ability to create ResourceRequestBody out of a byte array
  - in C++ (used by test code) and Java (needed by a public
    WebView::postUrl API in Android SDK).

- Follow-up changes
  - Removed no longer needed code:
    NavigationEntryImpl::ConstructBodyFromBrowserInitiatedPostData
  - Tweaked tests to account for new behavior (most imporatantly - reverse
    expectations in SendRendererInitiatedRequestUsingPOST test in
    chrome/browser/ui/browser_navigator_browsertest.cc)
  - Forwarding post body into LoadURLParams without looking at
    |is_renderer_initiated| (in chrome/browser/ui/browser_navigator.cc,
    components/web_contents_delegate_android/web_contents_delegate_android.cc
    and content/shell/browser/shell.cc).

BUG=344348
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Łukasz Anforowicz, 2016-06-09 16:00:21]

Description was changed from

==========
Using ResourceRequestBody as the type of HTTP body outside of //content.

This CL uses ResourceRequestBody to represent HTTP body in navigation
requests outside of //content layer.  This lays the foundations for
passing of arbitrary HTTP body out of and back into //content via
content::OpenURLParams and NavigationController::LoadURLParams.
This ability will be used in a later CL to preserve body of HTTP POST
for navigations handled via FrameHostMsg_OpenURL IPC message.

This CL adjusts names of fields and enum and values to reflect that
ResourceRequestBody can represent not only browser-initiated POST data,
but also renderer-initiated POST data.  Such change should be safe
(since we already track who initiated the navigation in
|is_renderer_initiated| field and should make security decisions based
on this field).

Summary of the changes:

- Renamed fields and enum values:
  - |browser_initiated_post_data| -> |post_data|.
  - |LOAD_TYPE_BROWSER_INITIATED_HTTP_POST| -> |LOAD_TYPE_HTTP_POST|.

- Changed type of |browser_initiated_post_data| aka |post_data|:
  - |scoped_refptr<base::RefCountedMemory>| ->
    |scoped_refptr<content::ResourceRequestBody>|
  - Java byte[] -> ResourceRequestBody wrapping a native pointer
    to content::ResourceRequestBody.

- Exposed ability to create ResourceRequestBody out of a byte array
  - in C++ (used by test code) and Java (needed by a public
    WebView::postUrl API in Android SDK).

- Follow-up changes
  - Removed no longer needed code:
    NavigationEntryImpl::ConstructBodyFromBrowserInitiatedPostData
  - Tweaked tests to account for new behavior (most imporatantly - reverse
    expectations in SendRendererInitiatedRequestUsingPOST test in
    chrome/browser/ui/browser_navigator_browsertest.cc)
  - Forwarding post body into LoadURLParams without looking at
    |is_renderer_initiated| (in chrome/browser/ui/browser_navigator.cc,
    components/web_contents_delegate_android/web_contents_delegate_android.cc
    and content/shell/browser/shell.cc).

BUG=344348
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Using ResourceRequestBody as the type of HTTP body outside of //content.

This CL changes representation of HTTP body used above //content layer.
Before this CL, HTTP body was represented by base::RefCountedMemory;
after this CL it is represented by a more generic ResourceRequestBody.
This lays the foundations for passing an arbitrary HTTP body out of and
back into //content via content::OpenURLParams and
NavigationController::LoadURLParams.  This ability will be used in a
later CL to preserve body of HTTP POST for navigations handled via
FrameHostMsg_OpenURL IPC message.

This CL adjusts names of fields and enum and values to reflect that
ResourceRequestBody can now represent not only browser-initiated POST
data, but also renderer-initiated POST data.  Such change should be safe
(since we already track who initiated the navigation in
|is_renderer_initiated| field and should make security decisions based
on this field).

Summary of the changes:

- Renamed fields and enum values:
  - |browser_initiated_post_data| -> |post_data|.
  - |LOAD_TYPE_BROWSER_INITIATED_HTTP_POST| -> |LOAD_TYPE_HTTP_POST|.

- Changed type of |browser_initiated_post_data| aka |post_data|:
  - |scoped_refptr<base::RefCountedMemory>| ->
    |scoped_refptr<content::ResourceRequestBody>|
  - Java byte[] -> ResourceRequestBody wrapping a native pointer
    to content::ResourceRequestBody.

- Exposed ability to create ResourceRequestBody out of a byte array
  - in C++ (used by test code) and Java (needed by a public
    WebView::postUrl API in Android SDK).

- Follow-up changes
  - Removed no longer needed code:
    NavigationEntryImpl::ConstructBodyFromBrowserInitiatedPostData
  - Tweaked tests to account for new behavior (most imporatantly - reverse
    expectations in SendRendererInitiatedRequestUsingPOST test in
    chrome/browser/ui/browser_navigator_browsertest.cc)
  - Forwarding post body into LoadURLParams without looking at
    |is_renderer_initiated| (in chrome/browser/ui/browser_navigator.cc,
    components/web_contents_delegate_android/web_contents_delegate_android.cc
    and content/shell/browser/shell.cc).

BUG=344348
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Łukasz Anforowicz, 2016-06-09 16:11:52]

lukasza@chromium.org changed reviewers:
+ creis@chromium.org

[Łukasz Anforowicz, 2016-06-09 16:11:53]

Charlie, can you take a look please?

[Łukasz Anforowicz, 2016-06-09 16:26:49]

+site-isolation-reviews@chromium.org

[Charlie Reis, 2016-06-09 22:38:16]

Thanks!  LGTM with some minor comments below.

https://codereview.chromium.org/2038233002/diff/60001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_navigator_browsertest.cc (right):

https://codereview.chromium.org/2038233002/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser_navigator_browsertest.cc:1425:
EXPECT_EQ(expected_title, title);
Sounds good.  We'll need this path for OpenURL.

https://codereview.chromium.org/2038233002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/2038233002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:675: NOTREACHED() <<
"Http post load must use http(s) scheme.";
You determined recently that this check is invalid, right?  We might not need to
change it in this CL, but I bet we'll need to change it to support POSTs to/from
extension URLs, etc.

https://codereview.chromium.org/2038233002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/2038233002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:414: // information is stored
in |content_state_| above. It is also only shallow
|content_state_| above -> PageState.

https://codereview.chromium.org/2038233002/diff/60001/content/content_browser...
File content/content_browser.gypi (right):

https://codereview.chromium.org/2038233002/diff/60001/content/content_browser...
content/content_browser.gypi:227: 'public/browser/navigation_data.h',
I usually applaud fixing things like this as we go, but in this case it looks
like there's no changes to this file needed for this CL.  Maybe we can land the
changes to this file separately, to avoid tying it's fate to the rest of the CL?

Same with the IWYU fixes in network_errors_listing_ui.cc and
web_ui_mojo_browsertest.cc, since this CL is big enough already.  :)

https://codereview.chromium.org/2038233002/diff/60001/content/public/browser/...
File content/public/browser/navigation_entry.h (right):

https://codereview.chromium.org/2038233002/diff/60001/content/public/browser/...
content/public/browser/navigation_entry.h:145: //    WebKit to actually make the
request.
Is this distinction still accurate?  Sounds like we might put the POST data into
|post_data| for OpenURL cases, in the midst of an ongoing navigation.

Also, I think "content_state" was the name for PageState at the time this was
added (r149232 in 2012).  Presumably the POST data is still in the PageState
after commit?

https://codereview.chromium.org/2038233002/diff/60001/content/public/browser/...
content/public/browser/navigation_entry.h:154: // For efficiency, this should be
cleared when content_state is populated
Same question.

https://codereview.chromium.org/2038233002/diff/60001/content/public/browser/...
File content/public/browser/page_navigator.h (left):

https://codereview.chromium.org/2038233002/diff/60001/content/public/browser/...
content/public/browser/page_navigator.h:57: // It is not for things like file
uploads.
Sanity check: Android apps won't be gaining an unexpected ability to do file
uploads via this change, right?  ResourceRequestBody would support that, but I
think there's no way for Java code to pass it in.

https://codereview.chromium.org/2038233002/diff/60001/content/public/common/r...
File content/public/common/resource_request_body.h (right):

https://codereview.chromium.org/2038233002/diff/60001/content/public/common/r...
content/public/common/resource_request_body.h:42: static
scoped_refptr<ResourceRequestBody> CreateFromBytes(const char* bytes,
You mentioned that this should only be used for Android and for tests.  I've
found that unless we make that explicit, people add other call sites that we
didn't intend.

Maybe we can have a CreateFromBytesForTesting on all platforms and a
CreateFromBytes ifdef'd to Android?  Under the hood, they can use the same code,
but that would make people think twice before trying to use this in other ways.

https://codereview.chromium.org/2038233002/diff/60001/content/public/common/r...
content/public/common/resource_request_body.h:45: const std::string& bytes);
Maybe we can get by with just the first version of this?  Looks like there's
only about new 3 places that would have to call data() and size() when passing
it in.

(This is mainly useful to avoid having 4 methods if we go with my ForTesting
suggestion above.)

[Łukasz Anforowicz, 2016-06-10 19:20:50]

Charlie, can you take another look?  I think I've addressed all of your
comments, but I had a really hard time wrapping my head about implications of
storing POST data either in PageState or
NavigationEntryImpl::[browser_initiated_post_data] - I hope that my replies
below make sense, but take them with a grain of salt :-)

https://codereview.chromium.org/2038233002/diff/60001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_navigator_browsertest.cc (right):

https://codereview.chromium.org/2038233002/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser_navigator_browsertest.cc:1425:
EXPECT_EQ(expected_title, title);
On 2016/06/09 22:38:16, Charlie Reis wrote:
> Sounds good.  We'll need this path for OpenURL.

Acknowledged.

https://codereview.chromium.org/2038233002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/2038233002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:675: NOTREACHED() <<
"Http post load must use http(s) scheme.";
On 2016/06/09 22:38:16, Charlie Reis wrote:
> You determined recently that this check is invalid, right?  We might not need
to
> change it in this CL, but I bet we'll need to change it to support POSTs
to/from
> extension URLs, etc.

Thanks for raising this issue.  You're right - after this CL this DCHECK can
probably be trigerred for a renderer-initiated POST.  I think fixing this in a
separate CL should be fine (given the relatively benign fallback - cancelling
the navigation + especially since I also would want to add a test for this
scenario which won't be possible until after the next CL
[https://codereview.chromium.org/2004653002/]).  We'll probably also have to
chat more about a way to trigger this DCHECK from a test - I am not sure if a
web page can have an subframe with extension URI (and post to it).  And when
adding the test described above, we probably should also remove the
short-circuiting in chrome/renderer/chrome_content_renderer_client.cc: 1076:
ChromeContentRendererClient::ShouldFork.

FWIW, I've added an explicit TODO here.

https://codereview.chromium.org/2038233002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/2038233002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:414: // information is stored
in |content_state_| above. It is also only shallow
On 2016/06/09 22:38:16, Charlie Reis wrote:
> |content_state_| above -> PageState.

Done.

https://codereview.chromium.org/2038233002/diff/60001/content/content_browser...
File content/content_browser.gypi (right):

https://codereview.chromium.org/2038233002/diff/60001/content/content_browser...
content/content_browser.gypi:227: 'public/browser/navigation_data.h',
On 2016/06/09 22:38:16, Charlie Reis wrote:
> I usually applaud fixing things like this as we go, but in this case it looks
> like there's no changes to this file needed for this CL.  Maybe we can land
the
> changes to this file separately, to avoid tying it's fate to the rest of the
CL?

Done.  Ooops, I thought that I've reverted changes in this file (it got modified
by mass-rename.sh but afterwards I decided that I don't need to move things here
[I ended up adding things to content_common.gypi instead]).
 
> Same with the IWYU fixes in network_errors_listing_ui.cc and
> web_ui_mojo_browsertest.cc, since this CL is big enough already.  :)

These are actually needed - these files fail to compile otherwise (they were
depending on including base/memory/ref_counted_memory.h via headers where I
changed this to include content/public/common/resource_request_body.h instead).

https://codereview.chromium.org/2038233002/diff/60001/content/public/browser/...
File content/public/browser/navigation_entry.h (right):

https://codereview.chromium.org/2038233002/diff/60001/content/public/browser/...
content/public/browser/navigation_entry.h:145: //    WebKit to actually make the
request.
On 2016/06/09 22:38:16, Charlie Reis wrote:
> Is this distinction still accurate?  Sounds like we might put the POST data
into
> |post_data| for OpenURL cases, in the midst of an ongoing navigation.
> 
> Also, I think "content_state" was the name for PageState at the time this was
> added (r149232 in 2012).  Presumably the POST data is still in the PageState
> after commit?

Hmmm... you're right - NavigationEntry::Set[BrowserInitiated]PostData method is
only called with non-null data from NavigationControllerImpl::LoadURLWithParams.
 And after the CL under review, LoadURLWithParams can be called with
LOAD_TYPE_[BROWSER_INITIATED]_HTTP_POST in more cases - potentially in cases
where the POST data is already present in PageState (which I think is limited to
reloads and history navigations).

I tried to see if browser-side ever looks at ResourceRequestBody embedded inside
PageState.  This can happen, because PageState is only semi-opaque - it shines
through via ExplodedHttpBody (so I looked at references to
ExplodedHttpBody::request_body.  I see that PlzNavigate extracts the body in
NavigationRequest::CreateBrowserInitiated, but this is fine - this is only the
beginning of a request, and afterwards I think we would not look for POST data
inside PageState.  Other than PlzNavigate, the ExplodedHttpBody::request_body
field is only referenced in code dealing with page/history serialization.

So - based on the previous paragraph, uses of PageState vs
NavigationEntryImpl::[browser_initiated]_post_data_ are not overlapping.  This
makes me feel relatively confident that the CL under review is okay to land
(i.e. shouldn't unexpectedly affect behavior related to code inspecting
PageState).

https://codereview.chromium.org/2038233002/diff/60001/content/public/browser/...
content/public/browser/navigation_entry.h:154: // For efficiency, this should be
cleared when content_state is populated
On 2016/06/09 22:38:16, Charlie Reis wrote:
> Same question.

I think the comment above is still true.  PageState will be populated *after*
the HTTP request has already been made, so no need to retain ResourceRequestBody
afterwards (in addition to what is inside PageState).  OTOH, I don't know if
this is also true for PlzNavigate (+ whether we actually clear this at the right
moments).

https://codereview.chromium.org/2038233002/diff/60001/content/public/browser/...
File content/public/browser/page_navigator.h (left):

https://codereview.chromium.org/2038233002/diff/60001/content/public/browser/...
content/public/browser/page_navigator.h:57: // It is not for things like file
uploads.
On 2016/06/09 22:38:16, Charlie Reis wrote:
> Sanity check: Android apps won't be gaining an unexpected ability to do file
> uploads via this change, right?  ResourceRequestBody would support that, but I
> think there's no way for Java code to pass it in.

Correct.  2 reasons why this cannot happen:

1. Native (i.e. C++) content/public/common/resource_request_body.h only exposes
a function to create ResourceRequestBody out of a vector of bytes.

2. Java's
content/public/android/java/src/org/chromium/content_public/common/ResourceRequestBody.java
(added in this CL) only has 1 public method - create (taking an array of bytes).

https://codereview.chromium.org/2038233002/diff/60001/content/public/common/r...
File content/public/common/resource_request_body.h (right):

https://codereview.chromium.org/2038233002/diff/60001/content/public/common/r...
content/public/common/resource_request_body.h:42: static
scoped_refptr<ResourceRequestBody> CreateFromBytes(const char* bytes,
On 2016/06/09 22:38:16, Charlie Reis wrote:
> You mentioned that this should only be used for Android and for tests.  I've
> found that unless we make that explicit, people add other call sites that we
> didn't intend.

I found out that I was wrong about that - there is one piece of non-Android
product code that also depends on content::ResourceRequestBody::CreateFromBytes.
 It is: chrome/browser/ui/tab_contents/core_tab_helper.cc:
CoreTabHelper::DoSearchByImageInNewTab.  I am not sure what this does, but it
seems that it posts image data to a search engine
(IDC_CONTENT_CONTEXT_SEARCHWEBFORIMAGE).

> 
> Maybe we can have a CreateFromBytesForTesting on all platforms and a
> CreateFromBytes ifdef'd to Android?  Under the hood, they can use the same
code,
> but that would make people think twice before trying to use this in other
ways.

Given above, I don't think we can do this.

https://codereview.chromium.org/2038233002/diff/60001/content/public/common/r...
content/public/common/resource_request_body.h:45: const std::string& bytes);
On 2016/06/09 22:38:16, Charlie Reis wrote:
> Maybe we can get by with just the first version of this?  Looks like there's
> only about new 3 places that would have to call data() and size() when passing
> it in.
> 
> (This is mainly useful to avoid having 4 methods if we go with my ForTesting
> suggestion above.)

Done.

[Charlie Reis, 2016-06-10 20:59:28]

Thanks-- still LGTM with a few minor nits.

https://codereview.chromium.org/2038233002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/2038233002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:675: NOTREACHED() <<
"Http post load must use http(s) scheme.";
On 2016/06/10 19:20:50, Łukasz Anforowicz wrote:
> On 2016/06/09 22:38:16, Charlie Reis wrote:
> > You determined recently that this check is invalid, right?  We might not
need
> to
> > change it in this CL, but I bet we'll need to change it to support POSTs
> to/from
> > extension URLs, etc.
> 
> Thanks for raising this issue.  You're right - after this CL this DCHECK can
> probably be trigerred for a renderer-initiated POST.  I think fixing this in a
> separate CL should be fine (given the relatively benign fallback - cancelling
> the navigation + especially since I also would want to add a test for this
> scenario which won't be possible until after the next CL
> [https://codereview.chromium.org/2004653002/]).  We'll probably also have to
> chat more about a way to trigger this DCHECK from a test - I am not sure if a
> web page can have an subframe with extension URI (and post to it).  And when
> adding the test described above, we probably should also remove the
> short-circuiting in chrome/renderer/chrome_content_renderer_client.cc: 1076:
> ChromeContentRendererClient::ShouldFork.
> 
> FWIW, I've added an explicit TODO here.

Acknowledged.

https://codereview.chromium.org/2038233002/diff/60001/content/content_browser...
File content/content_browser.gypi (right):

https://codereview.chromium.org/2038233002/diff/60001/content/content_browser...
content/content_browser.gypi:227: 'public/browser/navigation_data.h',
On 2016/06/10 19:20:50, Łukasz Anforowicz wrote:
> On 2016/06/09 22:38:16, Charlie Reis wrote:
> > I usually applaud fixing things like this as we go, but in this case it
looks
> > like there's no changes to this file needed for this CL.  Maybe we can land
> the
> > changes to this file separately, to avoid tying it's fate to the rest of the
> CL?
> 
> Done.  Ooops, I thought that I've reverted changes in this file (it got
modified
> by mass-rename.sh but afterwards I decided that I don't need to move things
here
> [I ended up adding things to content_common.gypi instead]).
>  
> > Same with the IWYU fixes in network_errors_listing_ui.cc and
> > web_ui_mojo_browsertest.cc, since this CL is big enough already.  :)
> 
> These are actually needed - these files fail to compile otherwise (they were
> depending on including base/memory/ref_counted_memory.h via headers where I
> changed this to include content/public/common/resource_request_body.h
instead).

Acknowledged.

https://codereview.chromium.org/2038233002/diff/60001/content/public/browser/...
File content/public/browser/navigation_entry.h (right):

https://codereview.chromium.org/2038233002/diff/60001/content/public/browser/...
content/public/browser/navigation_entry.h:145: //    WebKit to actually make the
request.
On 2016/06/10 19:20:50, Łukasz Anforowicz wrote:
> On 2016/06/09 22:38:16, Charlie Reis wrote:
> > Is this distinction still accurate?  Sounds like we might put the POST data
> into
> > |post_data| for OpenURL cases, in the midst of an ongoing navigation.
> > 
> > Also, I think "content_state" was the name for PageState at the time this
was
> > added (r149232 in 2012).  Presumably the POST data is still in the PageState
> > after commit?
> 
> Hmmm... you're right - NavigationEntry::Set[BrowserInitiated]PostData method
is
> only called with non-null data from
NavigationControllerImpl::LoadURLWithParams.
>  And after the CL under review, LoadURLWithParams can be called with
> LOAD_TYPE_[BROWSER_INITIATED]_HTTP_POST in more cases - potentially in cases
> where the POST data is already present in PageState (which I think is limited
to
> reloads and history navigations).
> 
> I tried to see if browser-side ever looks at ResourceRequestBody embedded
inside
> PageState.  This can happen, because PageState is only semi-opaque - it shines
> through via ExplodedHttpBody (so I looked at references to
> ExplodedHttpBody::request_body.  I see that PlzNavigate extracts the body in
> NavigationRequest::CreateBrowserInitiated, but this is fine - this is only the
> beginning of a request, and afterwards I think we would not look for POST data
> inside PageState.  Other than PlzNavigate, the ExplodedHttpBody::request_body
> field is only referenced in code dealing with page/history serialization.
> 
> So - based on the previous paragraph, uses of PageState vs
> NavigationEntryImpl::[browser_initiated]_post_data_ are not overlapping.  This
> makes me feel relatively confident that the CL under review is okay to land
> (i.e. shouldn't unexpectedly affect behavior related to code inspecting
> PageState).

Ok.  I'm slightly concerned about the opportunity for future bugs if we have
POST data in two places for in-progress navigations (e.g., reloads and history
navigations), but I think there's a path to avoid that-- I'm hoping to make
PageState less opaque in the near future, in which case we can probably unify
them.  This should be fine for now.

https://codereview.chromium.org/2038233002/diff/60001/content/public/browser/...
content/public/browser/navigation_entry.h:154: // For efficiency, this should be
cleared when content_state is populated
On 2016/06/10 19:20:50, Łukasz Anforowicz wrote:
> On 2016/06/09 22:38:16, Charlie Reis wrote:
> > Same question.
> 
> I think the comment above is still true.  PageState will be populated *after*
> the HTTP request has already been made, so no need to retain
ResourceRequestBody
> afterwards (in addition to what is inside PageState).  OTOH, I don't know if
> this is also true for PlzNavigate (+ whether we actually clear this at the
right
> moments).

Not sure on PlzNavigate either, but this seems reasonable to me.

https://codereview.chromium.org/2038233002/diff/60001/content/public/common/r...
File content/public/common/resource_request_body.h (right):

https://codereview.chromium.org/2038233002/diff/60001/content/public/common/r...
content/public/common/resource_request_body.h:42: static
scoped_refptr<ResourceRequestBody> CreateFromBytes(const char* bytes,
On 2016/06/10 19:20:50, Łukasz Anforowicz wrote:
> On 2016/06/09 22:38:16, Charlie Reis wrote:
> > You mentioned that this should only be used for Android and for tests.  I've
> > found that unless we make that explicit, people add other call sites that we
> > didn't intend.
> 
> I found out that I was wrong about that - there is one piece of non-Android
> product code that also depends on
content::ResourceRequestBody::CreateFromBytes.
>  It is: chrome/browser/ui/tab_contents/core_tab_helper.cc:
> CoreTabHelper::DoSearchByImageInNewTab.  I am not sure what this does, but it
> seems that it posts image data to a search engine
> (IDC_CONTENT_CONTEXT_SEARCHWEBFORIMAGE).

As Daniel points out, this is the "Search Google for image" context menu item
when you right click on images.

> 
> > 
> > Maybe we can have a CreateFromBytesForTesting on all platforms and a
> > CreateFromBytes ifdef'd to Android?  Under the hood, they can use the same
> code,
> > but that would make people think twice before trying to use this in other
> ways.
> 
> Given above, I don't think we can do this.

Ah, they beat me to it!  That's just the kind of thing I was afraid of-- using
an Android-specific API for new non-Android features, likely without a review of
the implications.  Ok, let's hope it's a sane use case, since now we have to
support it.

I suppose we could update the comment above to list that as another example,
though we don't really need to be exhaustive about how it's used.  (That would
just be a breadcrumb that it's used outside of Android and tests, so suggestions
like mine don't work.)

https://codereview.chromium.org/2038233002/diff/80001/content/public/browser/...
File content/public/browser/navigation_entry.h (right):

https://codereview.chromium.org/2038233002/diff/80001/content/public/browser/...
content/public/browser/navigation_entry.h:144: // 2) content_state when a post
request has started and is extracted by
PageState

https://codereview.chromium.org/2038233002/diff/80001/content/public/browser/...
content/public/browser/navigation_entry.h:154: // For efficiency, this should be
cleared when content_state is populated
PageState

[Łukasz Anforowicz, 2016-06-13 18:01:09]

lukasza@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Łukasz Anforowicz, 2016-06-13 18:01:11]

Ted, could you please take a look at the Java / JNI / Android parts?

Some notes to help you get started on the review:

- This is part of a sequence of CLs that adds support for handling arbitrary
POST body by OpenURL path (needed if //chrome layer wants to handle POST
navigation + needed for POST targeting a remote frame [i.e. OOPIF from another
renderer process]).  In particular this CL is dependent on an earlier CL (see
Rietveld for details).

- This CL starts to use ResourceRequestBody to represent an arbitrary POST body
(previous type was limited to representing an array of bytes).  Therefore this
CL needs to plumb ResourceRequestBody through Android/Java (replacing the old
Java byte[] <-> C++ base::RefCountedMemory mapping).

https://codereview.chromium.org/2038233002/diff/60001/content/public/browser/...
File content/public/browser/navigation_entry.h (right):

https://codereview.chromium.org/2038233002/diff/60001/content/public/browser/...
content/public/browser/navigation_entry.h:145: //    WebKit to actually make the
request.
On 2016/06/10 20:59:28, Charlie Reis wrote:
> On 2016/06/10 19:20:50, Łukasz Anforowicz wrote:
> > On 2016/06/09 22:38:16, Charlie Reis wrote:
> > > Is this distinction still accurate?  Sounds like we might put the POST
data
> > into
> > > |post_data| for OpenURL cases, in the midst of an ongoing navigation.
> > > 
> > > Also, I think "content_state" was the name for PageState at the time this
> was
> > > added (r149232 in 2012).  Presumably the POST data is still in the
PageState
> > > after commit?
> > 
> > Hmmm... you're right - NavigationEntry::Set[BrowserInitiated]PostData method
> is
> > only called with non-null data from
> NavigationControllerImpl::LoadURLWithParams.
> >  And after the CL under review, LoadURLWithParams can be called with
> > LOAD_TYPE_[BROWSER_INITIATED]_HTTP_POST in more cases - potentially in cases
> > where the POST data is already present in PageState (which I think is
limited
> to
> > reloads and history navigations).
> > 
> > I tried to see if browser-side ever looks at ResourceRequestBody embedded
> inside
> > PageState.  This can happen, because PageState is only semi-opaque - it
shines
> > through via ExplodedHttpBody (so I looked at references to
> > ExplodedHttpBody::request_body.  I see that PlzNavigate extracts the body in
> > NavigationRequest::CreateBrowserInitiated, but this is fine - this is only
the
> > beginning of a request, and afterwards I think we would not look for POST
data
> > inside PageState.  Other than PlzNavigate, the
ExplodedHttpBody::request_body
> > field is only referenced in code dealing with page/history serialization.
> > 
> > So - based on the previous paragraph, uses of PageState vs
> > NavigationEntryImpl::[browser_initiated]_post_data_ are not overlapping. 
This
> > makes me feel relatively confident that the CL under review is okay to land
> > (i.e. shouldn't unexpectedly affect behavior related to code inspecting
> > PageState).
> 
> Ok.  I'm slightly concerned about the opportunity for future bugs if we have
> POST data in two places for in-progress navigations (e.g., reloads and history
> navigations), but I think there's a path to avoid that-- I'm hoping to make
> PageState less opaque in the near future, in which case we can probably unify
> them.  This should be fine for now.

Acknowledged.

https://codereview.chromium.org/2038233002/diff/60001/content/public/browser/...
content/public/browser/navigation_entry.h:154: // For efficiency, this should be
cleared when content_state is populated
On 2016/06/10 20:59:28, Charlie Reis wrote:
> On 2016/06/10 19:20:50, Łukasz Anforowicz wrote:
> > On 2016/06/09 22:38:16, Charlie Reis wrote:
> > > Same question.
> > 
> > I think the comment above is still true.  PageState will be populated
*after*
> > the HTTP request has already been made, so no need to retain
> ResourceRequestBody
> > afterwards (in addition to what is inside PageState).  OTOH, I don't know if
> > this is also true for PlzNavigate (+ whether we actually clear this at the
> right
> > moments).
> 
> Not sure on PlzNavigate either, but this seems reasonable to me.

Acknowledged.

https://codereview.chromium.org/2038233002/diff/60001/content/public/common/r...
File content/public/common/resource_request_body.h (right):

https://codereview.chromium.org/2038233002/diff/60001/content/public/common/r...
content/public/common/resource_request_body.h:42: static
scoped_refptr<ResourceRequestBody> CreateFromBytes(const char* bytes,
On 2016/06/10 20:59:28, Charlie Reis wrote:
> On 2016/06/10 19:20:50, Łukasz Anforowicz wrote:
> > On 2016/06/09 22:38:16, Charlie Reis wrote:
> > > You mentioned that this should only be used for Android and for tests. 
I've
> > > found that unless we make that explicit, people add other call sites that
we
> > > didn't intend.
> > 
> > I found out that I was wrong about that - there is one piece of non-Android
> > product code that also depends on
> content::ResourceRequestBody::CreateFromBytes.
> >  It is: chrome/browser/ui/tab_contents/core_tab_helper.cc:
> > CoreTabHelper::DoSearchByImageInNewTab.  I am not sure what this does, but
it
> > seems that it posts image data to a search engine
> > (IDC_CONTENT_CONTEXT_SEARCHWEBFORIMAGE).
> 
> As Daniel points out, this is the "Search Google for image" context menu item
> when you right click on images.
> 
Oh, ok.  It seems to continue to work in the private build of Chrome with these
changes :-)
> > 
> > > Maybe we can have a CreateFromBytesForTesting on all platforms and a
> > > CreateFromBytes ifdef'd to Android?  Under the hood, they can use the same
> > code,
> > > but that would make people think twice before trying to use this in other
> > ways.
> > 
> > Given above, I don't think we can do this.
> 
> Ah, they beat me to it!  That's just the kind of thing I was afraid of-- using
> an Android-specific API for new non-Android features, likely without a review
of
> the implications.  Ok, let's hope it's a sane use case, since now we have to
> support it.
> 
> I suppose we could update the comment above to list that as another example,
> though we don't really need to be exhaustive about how it's used.  (That would
> just be a breadcrumb that it's used outside of Android and tests, so
suggestions
> like mine don't work.)

Done.

https://codereview.chromium.org/2038233002/diff/80001/content/public/browser/...
File content/public/browser/navigation_entry.h (right):

https://codereview.chromium.org/2038233002/diff/80001/content/public/browser/...
content/public/browser/navigation_entry.h:144: // 2) content_state when a post
request has started and is extracted by
On 2016/06/10 20:59:28, Charlie Reis wrote:
> PageState

Done.

https://codereview.chromium.org/2038233002/diff/80001/content/public/browser/...
content/public/browser/navigation_entry.h:154: // For efficiency, this should be
cleared when content_state is populated
On 2016/06/10 20:59:28, Charlie Reis wrote:
> PageState

Done.

https://codereview.chromium.org/2038233002/diff/100001/content/public/android...
File
content/public/android/java/src/org/chromium/content_public/common/ResourceRequestBody.java
(right):

https://codereview.chromium.org/2038233002/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content_public/common/ResourceRequestBody.java:15:
public final class ResourceRequestBody {
Quick rationalization of introducing a separate ResourceRequestBody Java class
(instead of using |long|):

I understand that some native pointers are represented simply as |long| in
Java/JNI world.  Using |long| would be wrong in this case, because the native
content::ResourceRequestBody is ref-counted (and we would easily run into
ref-counting trouble when assigning/copying |long| around without tight coupling
of the assigning/copying with corresponding ref-count management).

[Ted C, 2016-06-13 23:49:14]

Adding boliu for his recent thoughts on gc sore spots in Android land

https://codereview.chromium.org/2038233002/diff/100001/content/public/android...
File
content/public/android/java/src/org/chromium/content_public/common/ResourceRequestBody.java
(right):

https://codereview.chromium.org/2038233002/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content_public/common/ResourceRequestBody.java:27:
protected void finalize() {
Mark as @Override

In general, finalizers are a bad thing and we do our absolute best to discourage
them in all possible ways:
https://developer.android.com/reference/java/lang/Object.html#finalize()

Also, need to call super.finalize()

This general approach has led to some of the most annoying bugs for us to have
to track down in terms of subtly introducing memory leaks.  If some other part
of the code transitively holds onto a strong reference to these objects from
native then this will never get triggered/gc'd.  And by its nature it would be
really hard to introduce a GC test.

Example of these tests:
https://cs.chromium.org/chromium/src/android_webview/javatests/src/org/chromi...

Is there any memory benefit we are getting from making this change?  Hard to say
this really frowned upon thing should be allowed without any gains (especially
since post data on android just an opaque blob we pass around...why not expose a
to byte[] since we already have to do the reverse direction -- if there are
gains here, that is where it could be more compelling).

The other approach would be to expose incrementing and decrementing the ref
counting in java, but that is disgusting in its own right and probably no better
off in terms of bugs and predictability.  This is a pattern that we don't want
to make common as it will be a maintenance nightmare for us.

[Łukasz Anforowicz, 2016-06-14 00:37:51]

lukasza@chromium.org changed reviewers:
+ boliu@chromium.org

[Łukasz Anforowicz, 2016-06-14 00:37:52]

+boliu for real :-)

Thanks for the comments.  I have some C# experience (i.e. I am responsible for
C# wrapper for native WMI API - Microsoft.Management.Infrastructure namespace)
and am familiar with similar C# advise to avoid finalizers in every day C# code
(i.e. in C# finalizers also delay collection + run on a single dedicated thread
+ C# also has advice to provide explicit Close/Dispose method for *non-memory*
resources that need deterministic/timely cleanup).  Still - finalizers are
unavoidable on native/managed boundary in C# (sometimes the Close/Dispose method
just doesn't fit + seems really weird on a memory-only resource).  I think in
this CL we have a situation where a finalizer is justified.

https://codereview.chromium.org/2038233002/diff/100001/content/public/android...
File
content/public/android/java/src/org/chromium/content_public/common/ResourceRequestBody.java
(right):

https://codereview.chromium.org/2038233002/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content_public/common/ResourceRequestBody.java:27:
protected void finalize() {
On 2016/06/13 23:49:14, Ted C wrote:
> Mark as @Override

Done.  Thanks for pointing this out.
> 
> In general, finalizers are a bad thing and we do our absolute best to
discourage
> them in all possible ways:
> https://developer.android.com/reference/java/lang/Object.html#finalize()

I think the CL is compliant with the link above:

- No "blocking work" in the finalizers in the CL - only decrementing the
ref-count
  (note that this can NOT cascade into more ref-counts being decremented,
   because ResourceRequestBody does not own ref-counts to any other objects).

- The CL does not "rely on them [finalizers] for [deterministic/timely] cleanup
[of non-memory resources]".  
  I assume that this is the same advice C# developers get, but that it is okay
  (just as it is okay for C#) to use finalizers to release native memory.

- "A finalizer is usually only necessary for a class that has a native peer and
needs to call a native method to destroy that peer."
  This is exactly the situation we are in here.

- I believe that just as in C#, finalizers should be avoided *except* when they
are unavoidable on the native / managed boundary.  Sometimes an explicit Close
method just doesn't make sense (no good place to call it from).

> Also, need to call super.finalize()

Done.

I see the advice from
https://developer.android.com/reference/java/lang/Object.html#finalize() that
says "Unlike constructors, finalizers are not automatically chained. You are
responsible for calling super.finalize() yourself.".  I understand that *if
chaining is needed*, then I need to call the base method myself (unlike in a
constructor).  But I think chaining is not needed here (because
ResourceRequestBody derives directly from the java.lang.Object class.  Still -
I'll defer to your judgement here.  WDYT?

> This general approach has led to some of the most annoying bugs for us to have
> to track down in terms of subtly introducing memory leaks.  If some other part
> of the code transitively holds onto a strong reference to these objects from
> native then this will never get triggered/gc'd.  And by its nature it would be
> really hard to introduce a GC test.
> 
> Example of these tests:
>
https://cs.chromium.org/chromium/src/android_webview/javatests/src/org/chromi...
> 
> Is there any memory benefit we are getting from making this change?  Hard to
say
> this really frowned upon thing should be allowed without any gains (especially
> since post data on android just an opaque blob we pass around...why not expose
a
> to byte[] since we already have to do the reverse direction -- if there are
> gains here, that is where it could be more compelling).

RE: why not expose a byte[]

Because ResourceRequestBody can wrap not only a byte array, but a sequence of
elements, where each element is either
1) a byte array
2) a reference to a file (a range of bytes within this file)
3) a reference to a "filesystem" file (web storage api)
4) a reference to web blob (i.e. blob URI scheme)

Both in native and Java world I need to start supporting a more generic HTTP
body.  For example - in this CL I am also changing from using
base::RefCountedMemory (wrapping a byte array) into using
content::ResourceRequestBody (wrapping arbitrary HTTP body) - see changes in
chrome/browser/ui/browser_navigator_params.h [and cascading into other files]. 
As I explain in the CL description, supporting an arbitrary, generic HTTP body
is needed to fix issue 344348 (which will be done in the very next CL -
https://codereview.chromium.org/2004653002/).

To summarize, I cannot use Java's byte array, and need to:

1) pass an opaque native content::ResourceRequestBody into Java (in this code
path ResourceRequestBody is constructed inside //content layer and can wrap not
only a byte array, but can also refer to files and other kinds of elements used
to build HTTP body payload)

2) construct Java equivalent of native content::ResourceRequestBody out of Java
byte array.

3) extract native content::ResourceRequestBody out of Java and into native code.

> The other approach would be to expose incrementing and decrementing the ref
> counting in java, but that is disgusting in its own right and probably no
better
> off in terms of bugs and predictability.  This is a pattern that we don't want
> to make common as it will be a maintenance nightmare for us.

Well, I think that passing the pointer as |long| would be even more
nightmarish...  There is no way to restrict when and where the |long| will be
copied to (and no way to make sure the ref-count is balanced when such pointer
copies are made).


Do you have an alternative approach in mind?  Or - is there something I can do
to make the finalizer here more palatable?  Maybe add a scary comment here?

[boliu, 2016-06-14 04:07:59]

There are basically 3 designs:

1) gc-based resource clean up, ie finalizer
main issue here is impossible to detect leaks, now or in the future
Scary comment isn't going to help because code that introduces the leak is going
to be far away from the finalizer, essentially anything that decides to hold a
jni strong ref to ResourceRequestBody. And having a test is only useful for the
use cases that the test actually checks.

2) marshall everything into java, then back to native
I don't agree it's as complex as outlined. The post data can be opaque to java
side, so just need to define a serialization format. I'm pretty sure this data
need to be serialized to disk (?), so it's not like it's any extra work. It is
going to be extra copies though, both going to and from java.

3) provide an explicit destroy or release method on java side, and maybe use gc
to ensure destroy is called
this is actually closer to the "expose refcount to java" idea, risk is of course
forgetting to call destroy
looking at existing usage, LoadUrlParams is passed into AsyncTabCreationParams,
and then an Intent, so LoadUrlParams really can be long lived, so this would
really suck a lot


I don't see enough justification to do 1). So I'd still vote for 2).

[Łukasz Anforowicz, 2016-06-14 16:30:09]

Thanks for the quick reply.  I'll try to switch to approach #2
(serialization/deserialization at the native/Java boundary).

On 2016/06/14 04:07:59, boliu wrote:
> There are basically 3 designs:
> 
> 1) gc-based resource clean up, ie finalizer
> main issue here is impossible to detect leaks, now or in the future
> Scary comment isn't going to help because code that introduces the leak is
going
> to be far away from the finalizer, essentially anything that decides to hold a
> jni strong ref to ResourceRequestBody.

If something holds a jni strong reference to ResourceRequestBody, then we have a
memory leak regardless of whether ResourceRequestBody wraps a |long nativePtr|
or a |byte[] serializedFormOfNativeResourceRequestBody|.  I can see how cycles
can sometimes lead to trouble, but ResourceRequestBody cannot introduce a cycle
(the native ResourceRequestBody owns ResourceRequestBody::Elements which don't
own anything [i.e. they only embed data - file paths, byte vectors, guids,
etc.).

Still - I guess, if we can avoid having to "just be careful" with ref-counting,
then approach #2 (serialization/deserialization at the native/Java boundary) is
clearly better.

> And having a test is only useful for the
> use cases that the test actually checks.

Thanks for bringing this up.


I was thinking about unit tests for this code, and couldn't come up with a
simple scheme for testing...  I've verified that Java->native translation has
test coverage (via instrumentation tests of WebView [1]), but it would also be
nice to have a unit test that verifies round-tripping of the translation at the
native/Java boundary.  I assume that such a test would have to

1) be an instrumentation test (i.e. in a gtest-based unit test, there would be
no java environment / no way to grab JNIEnv*).

2) in order to test more than just ResourceRequestBody-wrapping-byte-array (i.e.
to test cases when it wraps file references or web blob uuids) the test has to
have access to internals of //content layer (//content public API exposes only
minimal surface of ResourceRequestBody - in particular it only exposes
CreateFromBytes for construction).

(1) and (2) above are at conflict... :-/


So - currently test coverage looks is as follows: existing tests cover
Java->native path (i.e. ExtractResourceRequestBodyFromJavaObject), OTOH,
native->Java path doesn't have good, continuous test coverage, because:

1. trigerring OpenURL path requires either:

1.1 site isolation (and site isolation bots only cover Linux and Windows;  this
should eventually change as part of the Top Document Isolation project which
will need to test OOPIFs on Android)
or
1.2 forcing OpenUrl path via
web_contents()->GetMutableRendererPrefs()->browser_handles_all_top_level_requests
(which AFAICT doesn't have a Java counter-part / cannot be set from
instrumentation tests).

2. trigerring native->Java conversion requires exercising code above content
layer (so browser tests added in the next CL [2] won't help here


I guess my plan is to

1) rely on test coverage being eventually added for TDI

2) rely on the fact that without TDI, the native->Java code path should not be
exercised today (because 2a) ChromeContentRendererClient::ShouldFork returns
false for non-GET HTTP methods [effectively disabling 1.2 above] and because 2b)
OOPIFs are not present on android (no site isolation, TDI not yet ready)
[effectively disabling 1.1 above]).


WDYT?  Any easy way to add a unit test that I missed?


BTW: I note that before this CL, there is also no test coverage for the native
-> Java path (at least all bots were green even after I tried inserting a
CHECK(false) into this code).


[1] DISPLAY=:0 out/gn_android/bin/run_android_webview_test_apk
--logcat-output-file ~/scratch/logcat.txt --fast-local-dev -f
org.chromium.android_webview.test.AwContentsClientOnFormResubmissionTest*

[2] https://codereview.chromium.org/2004653002/patch/120001/130001


> 2) marshall everything into java, then back to native
> I don't agree it's as complex as outlined. The post data can be opaque to java
> side, so just need to define a serialization format. I'm pretty sure this data
> need to be serialized to disk (?), so it's not like it's any extra work. It is
> going to be extra copies though, both going to and from java.

Good point - let me try that.  There is indeed a
content/common/page_state_serialization.cc
which covers serialization of the native ResourceRequestBody - I can try to
reuse that.

A quick clarification (we are probably on the same page, but wanted to
double-check :-) - serialization of ResourceRequestBody provided by
page_state_serialization.cc serializes shape of the native data structures (as
opposed to serializing contents of HTTP body as a byte array - doing this would
be impractical if HTTP body refers to really big files).

Quick draft of how I plan to follow approach #2:

- Keep separate ResourceRequestBody.java class (alternative - using Java's
byte[] as before - would be IMO too confusing wrt whether byte[] represents
serialization of ResourceRequestBody VS represents contents of HTTP body [e.g.
as passed to android.webkit.WebView.postUrl method]).

- Change field of ResourceRequestBody.java from |long nativePtr| to |byte[]
serializedFormOfNativeResourceRequestBody|.

- I'll either expose Write/ReadHttpBody from
content/common/page_state_serialization.cc as-is, OR create a smaller
Write/ReadResourceRequestBody and expose this (I think I prefer the latter; 
let's see if this can be done in a clean way).  I plan to hide this pair of
functions behind #if defined(OS_ANDROID) (similarily to how
DecodePageStateWithDeviceScaleFactorForTesting is hidden today).

- Have resource_request_body_android.cc depend on
content/common/page_state_serialization.h

> 3) provide an explicit destroy or release method on java side, and maybe use
gc
> to ensure destroy is called
> this is actually closer to the "expose refcount to java" idea, risk is of
course
> forgetting to call destroy
> looking at existing usage, LoadUrlParams is passed into
AsyncTabCreationParams,
> and then an Intent, so LoadUrlParams really can be long lived, so this would
> really suck a lot

Agreed - there is no good place to call Close/Dispose (or rather - there are too
many places where this would have to happen).
> 
> 
> I don't see enough justification to do 1). So I'd still vote for 2).

Ok - let me try approach #2 (I'll ping you when I have a new patchset with the
new approach - hopefully later today or tomorrow at the latest).  Let me know if
you have any concerns with the draft of how I plan to do this (as outlined
above).

[boliu, 2016-06-14 18:37:17]

On 2016/06/14 16:30:09, Łukasz Anforowicz wrote:
> Thanks for the quick reply.  I'll try to switch to approach #2
> (serialization/deserialization at the native/Java boundary).
> 
> On 2016/06/14 04:07:59, boliu wrote:
> > There are basically 3 designs:
> > 
> > 1) gc-based resource clean up, ie finalizer
> > main issue here is impossible to detect leaks, now or in the future
> > Scary comment isn't going to help because code that introduces the leak is
> going
> > to be far away from the finalizer, essentially anything that decides to hold
a
> > jni strong ref to ResourceRequestBody.
> 
> If something holds a jni strong reference to ResourceRequestBody, then we have
a
> memory leak regardless of whether ResourceRequestBody wraps a |long nativePtr|
> or a |byte[] serializedFormOfNativeResourceRequestBody|.  I can see how cycles
> can sometimes lead to trouble, but ResourceRequestBody cannot introduce a
cycle
> (the native ResourceRequestBody owns ResourceRequestBody::Elements which don't
> own anything [i.e. they only embed data - file paths, byte vectors, guids,
> etc.).

And it's hard to predict how code is going to evolve in the future. Maybe one
day some Element needs some java thing that accidentally refers back to the
LoadUrlParams. That kind of thing..

> 
> Still - I guess, if we can avoid having to "just be careful" with
ref-counting,
> then approach #2 (serialization/deserialization at the native/Java boundary)
is
> clearly better.
> 
> > And having a test is only useful for the
> > use cases that the test actually checks.
> 
> Thanks for bringing this up.
> 
> 
> I was thinking about unit tests for this code, and couldn't come up with a
> simple scheme for testing...  I've verified that Java->native translation has
> test coverage (via instrumentation tests of WebView [1]), but it would also be
> nice to have a unit test that verifies round-tripping of the translation at
the
> native/Java boundary.  I assume that such a test would have to
> 
> 1) be an instrumentation test (i.e. in a gtest-based unit test, there would be
> no java environment / no way to grab JNIEnv*).
> 
> 2) in order to test more than just ResourceRequestBody-wrapping-byte-array
(i.e.
> to test cases when it wraps file references or web blob uuids) the test has to
> have access to internals of //content layer (//content public API exposes only
> minimal surface of ResourceRequestBody - in particular it only exposes
> CreateFromBytes for construction).
> 
> (1) and (2) above are at conflict... :-/

There are also instrumentation tests based on content shell (which call into
content-internal code I think) and chrome. But instrumentation test by its
nature is an integration test, not unit tests.

We don't have a good answer for unit tests that involves jni.

> 
> 
> So - currently test coverage looks is as follows: existing tests cover
> Java->native path (i.e. ExtractResourceRequestBodyFromJavaObject), OTOH,
> native->Java path doesn't have good, continuous test coverage, because:
> 
> 1. trigerring OpenURL path requires either:
> 
> 1.1 site isolation (and site isolation bots only cover Linux and Windows; 
this
> should eventually change as part of the Top Document Isolation project which
> will need to test OOPIFs on Android)
> or
> 1.2 forcing OpenUrl path via
>
web_contents()->GetMutableRendererPrefs()->browser_handles_all_top_level_requests
> (which AFAICT doesn't have a Java counter-part / cannot be set from
> instrumentation tests).
> 
> 2. trigerring native->Java conversion requires exercising code above content
> layer (so browser tests added in the next CL [2] won't help here
> 
> 
> I guess my plan is to
> 
> 1) rely on test coverage being eventually added for TDI
> 
> 2) rely on the fact that without TDI, the native->Java code path should not be
> exercised today (because 2a) ChromeContentRendererClient::ShouldFork returns
> false for non-GET HTTP methods [effectively disabling 1.2 above] and because
2b)
> OOPIFs are not present on android (no site isolation, TDI not yet ready)
> [effectively disabling 1.1 above]).
> 
> 
> WDYT?  Any easy way to add a unit test that I missed?
> 
> 
> BTW: I note that before this CL, there is also no test coverage for the native
> -> Java path (at least all bots were green even after I tried inserting a
> CHECK(false) into this code).

I don't know much about top-level domain isolation to say much about this. I
suspect Ted doesn't either.

> 
> 
> [1] DISPLAY=:0 out/gn_android/bin/run_android_webview_test_apk
> --logcat-output-file ~/scratch/logcat.txt --fast-local-dev -f
> org.chromium.android_webview.test.AwContentsClientOnFormResubmissionTest*
> 
> [2] https://codereview.chromium.org/2004653002/patch/120001/130001
> 
> 
> > 2) marshall everything into java, then back to native
> > I don't agree it's as complex as outlined. The post data can be opaque to
java
> > side, so just need to define a serialization format. I'm pretty sure this
data
> > need to be serialized to disk (?), so it's not like it's any extra work. It
is
> > going to be extra copies though, both going to and from java.
> 
> Good point - let me try that.  There is indeed a
> content/common/page_state_serialization.cc
> which covers serialization of the native ResourceRequestBody - I can try to
> reuse that.
> 
> A quick clarification (we are probably on the same page, but wanted to
> double-check :-) - serialization of ResourceRequestBody provided by
> page_state_serialization.cc serializes shape of the native data structures (as
> opposed to serializing contents of HTTP body as a byte array - doing this
would
> be impractical if HTTP body refers to really big files).

Didn't know that. Sounds like should be ok though, right? :)

> 
> Quick draft of how I plan to follow approach #2:
> 
> - Keep separate ResourceRequestBody.java class (alternative - using Java's
> byte[] as before - would be IMO too confusing wrt whether byte[] represents
> serialization of ResourceRequestBody VS represents contents of HTTP body [e.g.
> as passed to android.webkit.WebView.postUrl method]).
> 
> - Change field of ResourceRequestBody.java from |long nativePtr| to |byte[]
> serializedFormOfNativeResourceRequestBody|.

Maybe it's ok to avoid ResourceRequestBody, if you just rename byte[] to
something like "mSerailizedPostData", or maybe "mEncodedPostData"? Will let the
actual owner decide though.

> 
> - I'll either expose Write/ReadHttpBody from
> content/common/page_state_serialization.cc as-is, OR create a smaller
> Write/ReadResourceRequestBody and expose this (I think I prefer the latter; 
> let's see if this can be done in a clean way).  I plan to hide this pair of
> functions behind #if defined(OS_ANDROID) (similarily to how
> DecodePageStateWithDeviceScaleFactorForTesting is hidden today).
> 
> - Have resource_request_body_android.cc depend on
> content/common/page_state_serialization.h
> 
> > 3) provide an explicit destroy or release method on java side, and maybe use
> gc
> > to ensure destroy is called
> > this is actually closer to the "expose refcount to java" idea, risk is of
> course
> > forgetting to call destroy
> > looking at existing usage, LoadUrlParams is passed into
> AsyncTabCreationParams,
> > and then an Intent, so LoadUrlParams really can be long lived, so this would
> > really suck a lot
> 
> Agreed - there is no good place to call Close/Dispose (or rather - there are
too
> many places where this would have to happen).
> > 
> > 
> > I don't see enough justification to do 1). So I'd still vote for 2).
> 
> Ok - let me try approach #2 (I'll ping you when I have a new patchset with the
> new approach - hopefully later today or tomorrow at the latest).  Let me know
if
> you have any concerns with the draft of how I plan to do this (as outlined
> above).

[Łukasz Anforowicz, 2016-06-14 21:49:55]

boliu@, can you take another look please?

On 2016/06/14 18:37:17, boliu wrote:
> On 2016/06/14 16:30:09, Łukasz Anforowicz wrote:
> > I was thinking about unit tests for this code, and couldn't come up with a
> > simple scheme for testing...  I've verified that Java->native translation
has
> > test coverage (via instrumentation tests of WebView [1]), but it would also
be
> > nice to have a unit test that verifies round-tripping of the translation at
> the
> > native/Java boundary.  I assume that such a test would have to
> > 
> > 1) be an instrumentation test (i.e. in a gtest-based unit test, there would
be
> > no java environment / no way to grab JNIEnv*).
> > 
> > 2) in order to test more than just ResourceRequestBody-wrapping-byte-array
> (i.e.
> > to test cases when it wraps file references or web blob uuids) the test has
to
> > have access to internals of //content layer (//content public API exposes
only
> > minimal surface of ResourceRequestBody - in particular it only exposes
> > CreateFromBytes for construction).
> > 
> > (1) and (2) above are at conflict... :-/
> 
> There are also instrumentation tests based on content shell (which call into
> content-internal code I think) and chrome. But instrumentation test by its
> nature is an integration test, not unit tests.
> 
> We don't have a good answer for unit tests that involves jni.

Ok.  I think I'll stick with the existing instrumentation tests in this case.
> > 
> > 
> > So - currently test coverage looks is as follows: existing tests cover
> > Java->native path (i.e. ExtractResourceRequestBodyFromJavaObject), OTOH,
> > native->Java path doesn't have good, continuous test coverage, because:
> > 
> > 1. trigerring OpenURL path requires either:
> > 
> > 1.1 site isolation (and site isolation bots only cover Linux and Windows; 
> this
> > should eventually change as part of the Top Document Isolation project which
> > will need to test OOPIFs on Android)
> > or
> > 1.2 forcing OpenUrl path via
> >
>
web_contents()->GetMutableRendererPrefs()->browser_handles_all_top_level_requests
> > (which AFAICT doesn't have a Java counter-part / cannot be set from
> > instrumentation tests).
> > 
> > 2. trigerring native->Java conversion requires exercising code above content
> > layer (so browser tests added in the next CL [2] won't help here
> > 
> > 
> > I guess my plan is to
> > 
> > 1) rely on test coverage being eventually added for TDI
> > 
> > 2) rely on the fact that without TDI, the native->Java code path should not
be
> > exercised today (because 2a) ChromeContentRendererClient::ShouldFork returns
> > false for non-GET HTTP methods [effectively disabling 1.2 above] and because
> 2b)
> > OOPIFs are not present on android (no site isolation, TDI not yet ready)
> > [effectively disabling 1.1 above]).
> > 
> > 
> > WDYT?  Any easy way to add a unit test that I missed?
> > 
> > 
> > BTW: I note that before this CL, there is also no test coverage for the
native
> > -> Java path (at least all bots were green even after I tried inserting a
> > CHECK(false) into this code).
> 
> I don't know much about top-level domain isolation to say much about this. I
> suspect Ted doesn't either.

No worries, I'll bring this up when nick@ comes back from BlinkOn next week.
> 
> > [1] DISPLAY=:0 out/gn_android/bin/run_android_webview_test_apk
> > --logcat-output-file ~/scratch/logcat.txt --fast-local-dev -f
> > org.chromium.android_webview.test.AwContentsClientOnFormResubmissionTest*
> > 
> > [2] https://codereview.chromium.org/2004653002/patch/120001/130001
> > 
> > 
> > > 2) marshall everything into java, then back to native
> > > I don't agree it's as complex as outlined. The post data can be opaque to
> java
> > > side, so just need to define a serialization format. I'm pretty sure this
> data
> > > need to be serialized to disk (?), so it's not like it's any extra work.
It
> is
> > > going to be extra copies though, both going to and from java.
> > 
> > Good point - let me try that.  There is indeed a
> > content/common/page_state_serialization.cc
> > which covers serialization of the native ResourceRequestBody - I can try to
> > reuse that.
> > 
> > A quick clarification (we are probably on the same page, but wanted to
> > double-check :-) - serialization of ResourceRequestBody provided by
> > page_state_serialization.cc serializes shape of the native data structures
(as
> > opposed to serializing contents of HTTP body as a byte array - doing this
> would
> > be impractical if HTTP body refers to really big files).
> 
> Didn't know that. Sounds like should be ok though, right? :)

Yes - page_state_serialization.cc does a shallow copy (i.e. only serializes
references to files, not the actual file contents).
> > 
> > Quick draft of how I plan to follow approach #2:
> > 
> > - Keep separate ResourceRequestBody.java class (alternative - using Java's
> > byte[] as before - would be IMO too confusing wrt whether byte[] represents
> > serialization of ResourceRequestBody VS represents contents of HTTP body
[e.g.
> > as passed to android.webkit.WebView.postUrl method]).
> > 
> > - Change field of ResourceRequestBody.java from |long nativePtr| to |byte[]
> > serializedFormOfNativeResourceRequestBody|.
> 
> Maybe it's ok to avoid ResourceRequestBody, if you just rename byte[] to
> something like "mSerailizedPostData", or maybe "mEncodedPostData"? Will let
the
> actual owner decide though.

I'd like to keep a separate ResourceRequestBody to avoid a confusion between:

1) byte[] mEncodedNativeForm (serialized form of shallow references to files +
serialized blobs, etc.)

2) byte[] mHttpBody (actual content of files, byte blobs, etc.)

(in case it makes it easier to see the difference, let me point out that the
ResourceRequestBody.nativeCreateResourceRequestBodyFromBytes JNI/Java static
method coverts from #2 into #1).

[boliu, 2016-06-14 23:54:21]

I was only pulled in for the finalizer discussion. Leave it to owners to review
:p

https://codereview.chromium.org/2038233002/diff/140001/content/public/android...
File
content/public/android/java/src/org/chromium/content_public/common/ResourceRequestBody.java
(right):

https://codereview.chromium.org/2038233002/diff/140001/content/public/android...
content/public/android/java/src/org/chromium/content_public/common/ResourceRequestBody.java:55:
public static ResourceRequestBody createFromBytes(byte[] httpBody) {
hmm, the number of copies here is *really* sad

maybe leave a TODO here that this could be optimized away? ie keep the raw
byte[] and do the "encoding" (or maybe that can be skipped too?) when all of
LoadURLParams are passed to native

only webview uses this, so let's see if anyone complains..

[Łukasz Anforowicz, 2016-06-15 16:58:43]

Ted, can you take a look please?

On 2016/06/14 23:54:21, boliu wrote:
> I was only pulled in for the finalizer discussion. Leave it to owners to
review

Oh, ok.  Thanks for your feedback!

https://codereview.chromium.org/2038233002/diff/140001/content/public/android...
File
content/public/android/java/src/org/chromium/content_public/common/ResourceRequestBody.java
(right):

https://codereview.chromium.org/2038233002/diff/140001/content/public/android...
content/public/android/java/src/org/chromium/content_public/common/ResourceRequestBody.java:55:
public static ResourceRequestBody createFromBytes(byte[] httpBody) {
On 2016/06/14 23:54:21, boliu wrote:
> hmm, the number of copies here is *really* sad

Indeed - 4 more copies than before (at least until the bytes reach the renderer
for the first time - the bytes will be sent back to the browser, then sent over
the network, then send back to renderer [potentially different renderer if there
were redirects]):


Copies made by the old code (after, say, WebView.postUrl call):

Contruction of a Java instance of ResourceRequestBody and LoadUrlParams:
- No copies (data kept as byte[]) until the call to
NavigationController::loadUrl

NavigationController.loadUrl:
1. JavaByteArrayToByteVector called by NavigationControllerAndroid::LoadUrl
2. ResourceRequestBody::AppendBytes called by
NavigationEntryImpl::ConstructBodyFromBrowserInitiatedPostData

IPC and Renderer:
3. IPC serialization/deserialization of ResourceRequestBody (not sure if this
counts as a copy :-)
4. GetWebHTTPBodyForRequestBody called by RenderFrameImpl::NavigateInternal
...


Copies made by the new code (after, say, WebView.postUrl call):

Contruction of a Java instance of ResourceRequestBody and LoadUrlParams:
1. JavaByteArrayToByteVector called by CreateResourceRequestBodyFromBytes
2. ResourceRequestBody::CreateFromBytes called by
CreateResourceRequestBodyFromBytes
3. EncodeResourceRequestBody called by CreateResourceRequestBodyFromBytes
4. base::android::ToJavaByteArray called by CreateResourceRequestBodyFromBytes

NavigationController.loadUrl:
5. JavaByteArrayToByteVector called by ExtractResourceRequestBodyFromJavaObject
6. ResourceRequestBody::AppendBytes indirectly by
ExtractResourceRequestBodyFromJavaObject (via AppendDataToRequestBody aka
AppendDataToHttpBody)

IPC and Renderer:
7. IPC serialization/deserialization of ResourceRequestBody (not sure if this
counts as a copy :-)
8. GetWebHTTPBodyForRequestBody called by RenderFrameImpl::NavigateInternal
...

> 
> maybe leave a TODO here that this could be optimized away? ie keep the raw
> byte[] and do the "encoding" (or maybe that can be skipped too?) when all of
> LoadURLParams are passed to native

Maybe.  OTOH, this seems rather yucky (in that it requires maintaining 2
separate code paths - one for encoded form and one specialized for the byte
array scenario).
> 
> only webview uses this, so let's see if anyone complains..

I don't think this should matter much - if there are not that many bytes, then
in-memory copies won't matter.  If there are many bytes, then sending the bytes
over the network will shadow the impact of in-memory copies.


At any rate - I can leave a TODO, but in CreateResourceRequestBodyFromBytes
(which makes 4 copies of the byte array).  Done?  :-)


PS. With ref-counting I think we could avoid all of the extra 4 copies :-)  So -
the extra copies can be seen as the price to pay for lessened maintenance burden
wrt ref-counting...

[boliu, 2016-06-15 17:07:42]

> At any rate - I can leave a TODO, but in CreateResourceRequestBodyFromBytes
(which makes 4 copies of the byte array).  Done?  :-)

The price to pay is an extra copy each time the data cross the jni border. The
webview only needs to go from java to native once, so in theory only need one
extra copy.

> PS. With ref-counting I think we could avoid all of the extra 4 copies :-)  So
-
the extra copies can be seen as the price to pay for lessened maintenance burden
wrt ref-counting...

Not true actually. You need at least one conversion from byte[] to something
that can be owned in native code.


I know chrome makes a lot of copies already internally. No one has ever spent
time optimizing it, but I guess first step would be make everything in native
code refcounted. Which is what you are doing here now, right? (Didn't read the
entire CL)

[Łukasz Anforowicz, 2016-06-15 21:44:02]

On 2016/06/15 17:07:42, boliu wrote:
> > At any rate - I can leave a TODO, but in CreateResourceRequestBodyFromBytes
> (which makes 4 copies of the byte array).  Done?  :-)
> 
> The price to pay is an extra copy each time the data cross the jni border. The
> webview only needs to go from java to native once, so in theory only need one
> extra copy.
> 
> > PS. With ref-counting I think we could avoid all of the extra 4 copies :-) 
So
> -
> the extra copies can be seen as the price to pay for lessened maintenance
burden
> wrt ref-counting...
> 
> Not true actually. You need at least one conversion from byte[] to something
> that can be owned in native code.

I think we are in violent agreement:
1. There needs to be at least one copy when crossing Java/native bounary
2. Ref-counting would have avoided *additional* 4 copies
   2.a. In the current shape of the CL, we have the following copies when
constructing ResourceRequestBody Java object: 1) copy when going from Java to
native, 2) copy when constructing temporary native ResourceRequestBody, 3) copy
when encoding/serializing ResourceRequestBody, 4) copy when pushing byte[] back
into Java.  When extracting contents of ResourceRequestBody into native side,
there are the following copies: 1) encoded bytes from Java to native, 2) copy
when constructing ResourceRequestBody out of encoded bytes.
   2.b. With ref-counting, we would have only 2 copies when constructing
ResourceRequestBody Java object: 1) copy when going from Java to native, 2) copy
when constructing a *long-lived* native ResourceRequestBody.  There would be no
copies during loadUrl (no need to extract ResourceRequestyBody from Java to
native, because it is already kept as a raw, native pointer in Java).

So:
- 6 copies in current CL.
- 2 copies with ref-counting.  
So - 4 extra copies without ref-counting.

> I know chrome makes a lot of copies already internally. No one has ever spent
> time optimizing it, but I guess first step would be make everything in native
> code refcounted. Which is what you are doing here now, right? (Didn't read the
> entire CL)

Not really.  Before this CL, the body was also ref-counted.  This CL makes the
body more generic (capable of not only wrapping an array of bytes passed from
the browser, but wrapping an arbitrary combination of file references, web blob
uuids, byte arrays, etc [in case when the body originates in the renderer]).

Before this CL (i.e. in chrome/browser/ui/browser_navigator_params.h):
-  scoped_refptr<base::RefCountedMemory> browser_initiated_post_data;

After this CL:
-  scoped_refptr<content::ResourceRequestBody> post_data;

All other changes in this CL are a consequence of this type change (including
the Java / Android changes).

[Łukasz Anforowicz, 2016-06-15 22:07:19]

Description was changed from

==========
Using ResourceRequestBody as the type of HTTP body outside of //content.

This CL changes representation of HTTP body used above //content layer.
Before this CL, HTTP body was represented by base::RefCountedMemory;
after this CL it is represented by a more generic ResourceRequestBody.
This lays the foundations for passing an arbitrary HTTP body out of and
back into //content via content::OpenURLParams and
NavigationController::LoadURLParams.  This ability will be used in a
later CL to preserve body of HTTP POST for navigations handled via
FrameHostMsg_OpenURL IPC message.

This CL adjusts names of fields and enum and values to reflect that
ResourceRequestBody can now represent not only browser-initiated POST
data, but also renderer-initiated POST data.  Such change should be safe
(since we already track who initiated the navigation in
|is_renderer_initiated| field and should make security decisions based
on this field).

Summary of the changes:

- Renamed fields and enum values:
  - |browser_initiated_post_data| -> |post_data|.
  - |LOAD_TYPE_BROWSER_INITIATED_HTTP_POST| -> |LOAD_TYPE_HTTP_POST|.

- Changed type of |browser_initiated_post_data| aka |post_data|:
  - |scoped_refptr<base::RefCountedMemory>| ->
    |scoped_refptr<content::ResourceRequestBody>|
  - Java byte[] -> ResourceRequestBody wrapping a native pointer
    to content::ResourceRequestBody.

- Exposed ability to create ResourceRequestBody out of a byte array
  - in C++ (used by test code) and Java (needed by a public
    WebView::postUrl API in Android SDK).

- Follow-up changes
  - Removed no longer needed code:
    NavigationEntryImpl::ConstructBodyFromBrowserInitiatedPostData
  - Tweaked tests to account for new behavior (most imporatantly - reverse
    expectations in SendRendererInitiatedRequestUsingPOST test in
    chrome/browser/ui/browser_navigator_browsertest.cc)
  - Forwarding post body into LoadURLParams without looking at
    |is_renderer_initiated| (in chrome/browser/ui/browser_navigator.cc,
    components/web_contents_delegate_android/web_contents_delegate_android.cc
    and content/shell/browser/shell.cc).

BUG=344348
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Using ResourceRequestBody as the type of HTTP body outside of //content.

This CL changes representation of HTTP body used above //content layer.
Before this CL, HTTP body was represented by base::RefCountedMemory;
after this CL it is represented by a more generic ResourceRequestBody.
This lays the foundations for passing an arbitrary HTTP body out of and
back into //content via content::OpenURLParams and
NavigationController::LoadURLParams.  This ability will be used in a
later CL to preserve body of HTTP POST for navigations handled via
FrameHostMsg_OpenURL IPC message.

This CL adjusts names of fields and enum and values to reflect that
ResourceRequestBody can now represent not only browser-initiated POST
data, but also renderer-initiated POST data.  Such change should be safe
(since we already track who initiated the navigation in
|is_renderer_initiated| field and should make security decisions based
on this field).

Summary of the changes:

- Renamed fields and enum values:
  - |browser_initiated_post_data| -> |post_data|.
  - |LOAD_TYPE_BROWSER_INITIATED_HTTP_POST| -> |LOAD_TYPE_HTTP_POST|.

- Changed type of |browser_initiated_post_data| aka |post_data|:
  - |scoped_refptr<base::RefCountedMemory>| ->
    |scoped_refptr<content::ResourceRequestBody>|
  - Java byte[] -> ResourceRequestBody Java class that wraps a
    serialized form of the native content::ResourceRequestBody.

- Exposed ability to create ResourceRequestBody out of a byte array
  - in C++ (used by test code and DoSearchByImageInNewTab) and
    Java (needed by a public WebView::postUrl API in Android SDK).

- Follow-up changes
  - Removed no longer needed code:
    NavigationEntryImpl::ConstructBodyFromBrowserInitiatedPostData
  - Tweaked tests to account for new behavior (most imporatantly - reverse
    expectations in SendRendererInitiatedRequestUsingPOST test in
    chrome/browser/ui/browser_navigator_browsertest.cc)
  - Forwarding post body into LoadURLParams without looking at
    |is_renderer_initiated| (in chrome/browser/ui/browser_navigator.cc,
    components/web_contents_delegate_android/web_contents_delegate_android.cc
    and content/shell/browser/shell.cc).

BUG=344348
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Ted C, 2016-06-15 23:04:57]

On 2016/06/15 21:44:02, Łukasz Anforowicz wrote:
> On 2016/06/15 17:07:42, boliu wrote:
> > > At any rate - I can leave a TODO, but in
CreateResourceRequestBodyFromBytes
> > (which makes 4 copies of the byte array).  Done?  :-)
> > 
> > The price to pay is an extra copy each time the data cross the jni border.
The
> > webview only needs to go from java to native once, so in theory only need
one
> > extra copy.
> > 
> > > PS. With ref-counting I think we could avoid all of the extra 4 copies :-)

> So
> > -
> > the extra copies can be seen as the price to pay for lessened maintenance
> burden
> > wrt ref-counting...
> > 
> > Not true actually. You need at least one conversion from byte[] to something
> > that can be owned in native code.
> 
> I think we are in violent agreement:
> 1. There needs to be at least one copy when crossing Java/native bounary
> 2. Ref-counting would have avoided *additional* 4 copies
>    2.a. In the current shape of the CL, we have the following copies when
> constructing ResourceRequestBody Java object: 1) copy when going from Java to
> native, 2) copy when constructing temporary native ResourceRequestBody, 3)
copy
> when encoding/serializing ResourceRequestBody, 4) copy when pushing byte[]
back
> into Java.  When extracting contents of ResourceRequestBody into native side,
> there are the following copies: 1) encoded bytes from Java to native, 2) copy
> when constructing ResourceRequestBody out of encoded bytes.
>    2.b. With ref-counting, we would have only 2 copies when constructing
> ResourceRequestBody Java object: 1) copy when going from Java to native, 2)
copy
> when constructing a *long-lived* native ResourceRequestBody.  There would be
no
> copies during loadUrl (no need to extract ResourceRequestyBody from Java to
> native, because it is already kept as a raw, native pointer in Java).
> 
> So:
> - 6 copies in current CL.
> - 2 copies with ref-counting.  
> So - 4 extra copies without ref-counting.
> 
> > I know chrome makes a lot of copies already internally. No one has ever
spent
> > time optimizing it, but I guess first step would be make everything in
native
> > code refcounted. Which is what you are doing here now, right? (Didn't read
the
> > entire CL)
> 
> Not really.  Before this CL, the body was also ref-counted.  This CL makes the
> body more generic (capable of not only wrapping an array of bytes passed from
> the browser, but wrapping an arbitrary combination of file references, web
blob
> uuids, byte arrays, etc [in case when the body originates in the renderer]).
> 
> Before this CL (i.e. in chrome/browser/ui/browser_navigator_params.h):
> -  scoped_refptr<base::RefCountedMemory> browser_initiated_post_data;
> 
> After this CL:
> -  scoped_refptr<content::ResourceRequestBody> post_data;
> 
> All other changes in this CL are a consequence of this type change (including
> the Java / Android changes).

Hmm...I feel I should have something deeper to comment on, but no.

lgtm

[Łukasz Anforowicz, 2016-06-15 23:17:15]

boliu@, could you please do an OWNERS review for
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java?

(and also don't hesitate to let me know if you have any remaining feedback for
Java / JNI / android changes :-).

[boliu, 2016-06-16 00:02:08]

nah, I trust ted, android_webview lgtm

> - 6 copies in current CL.

Ahh... so sad..

[Łukasz Anforowicz, 2016-06-16 00:13:04]

lukasza@chromium.org changed reviewers:
+ sky@chromium.org

[Łukasz Anforowicz, 2016-06-16 00:13:05]

sky@, could you please do an OWNERS review for the remaining files?:
    chrome/browser/prerender/prerender_browsertest.cc
    chrome/browser/ui/browser_navigator.cc
    chrome/browser/ui/browser_navigator_browsertest.cc
    chrome/browser/ui/browser_navigator_params.cc
    chrome/browser/ui/browser_navigator_params.h
    chrome/browser/ui/tab_contents/core_tab_helper.cc
    chrome/test/base/ui_test_utils.cc

[Łukasz Anforowicz, 2016-06-16 15:27:39]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-16 15:28:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2038233002/180001

[commit-bot: I haz the power, 2016-06-16 16:22:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-16 16:22:29]

Dry run: This issue passed the CQ dry run.

[sky, 2016-06-16 16:58:20]

LGTM

[Łukasz Anforowicz, 2016-06-16 18:21:03]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-06-16 18:21:03]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2038233002/#ps180001
(title: "Rebasing...")

[commit-bot: I haz the power, 2016-06-16 18:21:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2038233002/180001

[commit-bot: I haz the power, 2016-06-16 18:28:58]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-06-16 18:29:12]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-16 18:30:58]

Description was changed from

==========
Using ResourceRequestBody as the type of HTTP body outside of //content.

This CL changes representation of HTTP body used above //content layer.
Before this CL, HTTP body was represented by base::RefCountedMemory;
after this CL it is represented by a more generic ResourceRequestBody.
This lays the foundations for passing an arbitrary HTTP body out of and
back into //content via content::OpenURLParams and
NavigationController::LoadURLParams.  This ability will be used in a
later CL to preserve body of HTTP POST for navigations handled via
FrameHostMsg_OpenURL IPC message.

This CL adjusts names of fields and enum and values to reflect that
ResourceRequestBody can now represent not only browser-initiated POST
data, but also renderer-initiated POST data.  Such change should be safe
(since we already track who initiated the navigation in
|is_renderer_initiated| field and should make security decisions based
on this field).

Summary of the changes:

- Renamed fields and enum values:
  - |browser_initiated_post_data| -> |post_data|.
  - |LOAD_TYPE_BROWSER_INITIATED_HTTP_POST| -> |LOAD_TYPE_HTTP_POST|.

- Changed type of |browser_initiated_post_data| aka |post_data|:
  - |scoped_refptr<base::RefCountedMemory>| ->
    |scoped_refptr<content::ResourceRequestBody>|
  - Java byte[] -> ResourceRequestBody Java class that wraps a
    serialized form of the native content::ResourceRequestBody.

- Exposed ability to create ResourceRequestBody out of a byte array
  - in C++ (used by test code and DoSearchByImageInNewTab) and
    Java (needed by a public WebView::postUrl API in Android SDK).

- Follow-up changes
  - Removed no longer needed code:
    NavigationEntryImpl::ConstructBodyFromBrowserInitiatedPostData
  - Tweaked tests to account for new behavior (most imporatantly - reverse
    expectations in SendRendererInitiatedRequestUsingPOST test in
    chrome/browser/ui/browser_navigator_browsertest.cc)
  - Forwarding post body into LoadURLParams without looking at
    |is_renderer_initiated| (in chrome/browser/ui/browser_navigator.cc,
    components/web_contents_delegate_android/web_contents_delegate_android.cc
    and content/shell/browser/shell.cc).

BUG=344348
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Using ResourceRequestBody as the type of HTTP body outside of //content.

This CL changes representation of HTTP body used above //content layer.
Before this CL, HTTP body was represented by base::RefCountedMemory;
after this CL it is represented by a more generic ResourceRequestBody.
This lays the foundations for passing an arbitrary HTTP body out of and
back into //content via content::OpenURLParams and
NavigationController::LoadURLParams.  This ability will be used in a
later CL to preserve body of HTTP POST for navigations handled via
FrameHostMsg_OpenURL IPC message.

This CL adjusts names of fields and enum and values to reflect that
ResourceRequestBody can now represent not only browser-initiated POST
data, but also renderer-initiated POST data.  Such change should be safe
(since we already track who initiated the navigation in
|is_renderer_initiated| field and should make security decisions based
on this field).

Summary of the changes:

- Renamed fields and enum values:
  - |browser_initiated_post_data| -> |post_data|.
  - |LOAD_TYPE_BROWSER_INITIATED_HTTP_POST| -> |LOAD_TYPE_HTTP_POST|.

- Changed type of |browser_initiated_post_data| aka |post_data|:
  - |scoped_refptr<base::RefCountedMemory>| ->
    |scoped_refptr<content::ResourceRequestBody>|
  - Java byte[] -> ResourceRequestBody Java class that wraps a
    serialized form of the native content::ResourceRequestBody.

- Exposed ability to create ResourceRequestBody out of a byte array
  - in C++ (used by test code and DoSearchByImageInNewTab) and
    Java (needed by a public WebView::postUrl API in Android SDK).

- Follow-up changes
  - Removed no longer needed code:
    NavigationEntryImpl::ConstructBodyFromBrowserInitiatedPostData
  - Tweaked tests to account for new behavior (most imporatantly - reverse
    expectations in SendRendererInitiatedRequestUsingPOST test in
    chrome/browser/ui/browser_navigator_browsertest.cc)
  - Forwarding post body into LoadURLParams without looking at
    |is_renderer_initiated| (in chrome/browser/ui/browser_navigator.cc,
    components/web_contents_delegate_android/web_contents_delegate_android.cc
    and content/shell/browser/shell.cc).

BUG=344348
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/477a5a270598c17e72365572114490ae60e1ef56
Cr-Commit-Position: refs/heads/master@{#400211}
==========

[commit-bot: I haz the power, 2016-06-16 18:31:02]

Patchset 10 (id:??) landed as
https://crrev.com/477a5a270598c17e72365572114490ae60e1ef56
Cr-Commit-Position: refs/heads/master@{#400211}