Expose HTTP response headers via AwNavigation [chromium/src : main]
Peter Conn (Gerrit)
Peter Conn added 1 comment![]( "Open in Gerrit")
PTAL - this CL replaces [this older one](https://chromium-review.git.corp.google.com/c/chromium/src/+/7955997)
Related details
- Peter Pakkenberg
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Pakkenberg (Gerrit)
Peter Pakkenberg added 7 comments![]( "Open in Gerrit")
if (mResponseHeaders == null) {Is the Navigation object expected to only be accessed on the UI thread in the public API?
If not, make sure to have a lock around this field initialization.
List<Pair<String, String>> headers = new ArrayList<>();
headers.add(new Pair<>("Custom-Header", "Value1"));
headers.add(new Pair<>("Custom-Header", "Value2"));Use `List.of(...)`
std::vector<std::string> response_header_names;
std::vector<std::string> response_header_values;Single `std::map<std::string, std::string>`.
Even better if you can use `base::flat_map` and reserve the size needed immediately, but I'm not sure if JNIZero supports `base::flat_map`.
If you do need to keep this as double arrays, reserve the capacity of each vector before building it to save a bit of growth overhead.
env, java_navigation_handle_, url::GURLAndroid::FromNativeGURL(env, gurl),While you are here anyway, could you please update the other Object parameters to use `@JniType` annotations? I'm not sure the `Page` object has an automatic conversion so that can be skipped if it doesn't, but `GURL` and `String` do have automatic conversions.
@JniType("std::vector<std::string>") String[] responseHeaderNames,
@JniType("std::vector<std::string>") String[] responseHeaderValues,JNIZero should support `@JniType("std::map<std::string, std::string>")` so I would suggest you use that instead of dealing with 2 arrays.
mResponseHeaderNames = responseHeaderNames;
mResponseHeaderValues = responseHeaderValues;If you cannot pass a map directly through JNI, then please convert the headers to a Map<String, String> here, so we don't leak the double-array workaround any further.
public String @Nullable [] getResponseHeaderNames() {As mentioned elsewhere, this shoud just be one getter that returns a Nullable map
Related details
- Peter Conn
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Conn (Gerrit)
Peter Conn added 8 comments![]( "Open in Gerrit")
+rakina for //content/
Also Rakina, it looks like NavigationHandle is not thread safe, but the WebView API may be used from any thread so I filed http://crbug.com/532865730 to make it so. Do say if you have any concerns.
if (mResponseHeaders == null) {Is the Navigation object expected to only be accessed on the UI thread in the public API?
If not, make sure to have a lock around this field initialization.
That's a very good point - in fact I don't think Navigation or Page are threadsafe at all. I'd rather fix them holistically - filed crbug.com/532865730 to do so.
List<Pair<String, String>> headers = new ArrayList<>();
headers.add(new Pair<>("Custom-Header", "Value1"));
headers.add(new Pair<>("Custom-Header", "Value2"));Peter ConnUse `List.of(...)`
Done
std::vector<std::string> response_header_names;
std::vector<std::string> response_header_values;Single `std::map<std::string, std::string>`.
Even better if you can use `base::flat_map` and reserve the size needed immediately, but I'm not sure if JNIZero supports `base::flat_map`.
If you do need to keep this as double arrays, reserve the capacity of each vector before building it to save a bit of growth overhead.
Done
env, java_navigation_handle_, url::GURLAndroid::FromNativeGURL(env, gurl),While you are here anyway, could you please update the other Object parameters to use `@JniType` annotations? I'm not sure the `Page` object has an automatic conversion so that can be skipped if it doesn't, but `GURL` and `String` do have automatic conversions.
Done
@JniType("std::vector<std::string>") String[] responseHeaderNames,
@JniType("std::vector<std::string>") String[] responseHeaderValues,JNIZero should support `@JniType("std::map<std::string, std::string>")` so I would suggest you use that instead of dealing with 2 arrays.
Done
mResponseHeaderNames = responseHeaderNames;
mResponseHeaderValues = responseHeaderValues;If you cannot pass a map directly through JNI, then please convert the headers to a Map<String, String> here, so we don't leak the double-array workaround any further.
Done
public String @Nullable [] getResponseHeaderNames() {As mentioned elsewhere, this shoud just be one getter that returns a Nullable map
- Peter Pakkenberg
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Pakkenberg (Gerrit)
Peter Pakkenberg added 1 comment![]( "Open in Gerrit")
while (cpp_navigation_handle_->GetResponseHeaders()->EnumerateHeaderLines(Sorry, forget everything I said about using a map here 😞
`Set-Cookie` is ruining our day here. It can be repeated, and is _not_ safe to concatenate with h a comma.
(There is also no guarantee that the server won't send other repeated headers that cannot safely be comma-concatenated).
The whole API needs to either special case `Set-Cookie` or treat response headers like a list of pairs.
Related details
- Peter Conn
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Conn (Gerrit)
Peter Conn added 1 comment![]( "Open in Gerrit")
while (cpp_navigation_handle_->GetResponseHeaders()->EnumerateHeaderLines(Sorry, forget everything I said about using a map here 😞
`Set-Cookie` is ruining our day here. It can be repeated, and is _not_ safe to concatenate with h a comma.
(There is also no guarantee that the server won't send other repeated headers that cannot safely be comma-concatenated).
The whole API needs to either special case `Set-Cookie` or treat response headers like a list of pairs.
I thought that as well (and had implemented it [here](https://chromium-review.git.corp.google.com/c/chromium/src/+/7955997/5)), but it turns out that the `Set-Cookie` header is stripped out by the network service before it gets to us here.
Given that this is consistent with how WebResourceResponse works, I figure we can keep the API simple and let people deal with cookies through the CookieManager.
Related details
- Peter Pakkenberg
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Pakkenberg (Gerrit)
Peter Pakkenberg added 2 comments![]( "Open in Gerrit")
if (mResponseHeaders == null) {Peter ConnIs the Navigation object expected to only be accessed on the UI thread in the public API?
If not, make sure to have a lock around this field initialization.
That's a very good point - in fact I don't think Navigation or Page are threadsafe at all. I'd rather fix them holistically - filed crbug.com/532865730 to do so.
Acknowledged
while (cpp_navigation_handle_->GetResponseHeaders()->EnumerateHeaderLines(Peter ConnSorry, forget everything I said about using a map here 😞
`Set-Cookie` is ruining our day here. It can be repeated, and is _not_ safe to concatenate with h a comma.
(There is also no guarantee that the server won't send other repeated headers that cannot safely be comma-concatenated).
The whole API needs to either special case `Set-Cookie` or treat response headers like a list of pairs.
I thought that as well (and had implemented it [here](https://chromium-review.git.corp.google.com/c/chromium/src/+/7955997/5)), but it turns out that the `Set-Cookie` header is stripped out by the network service before it gets to us here.
Given that this is consistent with how WebResourceResponse works, I figure we can keep the API simple and let people deal with cookies through the CookieManager.
Ok - we should just make sure to document this behavior then.
But depending on custom headers sent by the server, this coalescing might not be safe to do, especially for non-standard headers.
Related details
- Peter Conn
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Conn (Gerrit)
Peter Conn added 1 comment![]( "Open in Gerrit")
while (cpp_navigation_handle_->GetResponseHeaders()->EnumerateHeaderLines(Peter ConnSorry, forget everything I said about using a map here 😞
`Set-Cookie` is ruining our day here. It can be repeated, and is _not_ safe to concatenate with h a comma.
(There is also no guarantee that the server won't send other repeated headers that cannot safely be comma-concatenated).
The whole API needs to either special case `Set-Cookie` or treat response headers like a list of pairs.
Peter PakkenbergI thought that as well (and had implemented it [here](https://chromium-review.git.corp.google.com/c/chromium/src/+/7955997/5)), but it turns out that the `Set-Cookie` header is stripped out by the network service before it gets to us here.
Given that this is consistent with how WebResourceResponse works, I figure we can keep the API simple and let people deal with cookies through the CookieManager.
Ok - we should just make sure to document this behavior then.
But depending on custom headers sent by the server, this coalescing might not be safe to do, especially for non-standard headers.
Sure, I'll document.
Making the object here more complex than a map opens us up to more design complexity - do we return a list of values for every header, which is kinda ugly, or have two different getters and make the developer choose which one whenever they get something - or do we just expose a list of pairs and leave it up to the developer to sort.
According to Torne, the browser ends up concatenating all non-Set-Cookie headers, so I'm tempted to leave the API simple and not support people doing things in WebView with non-standard headers that would be interpreted in a different way from the browser.
Related details
- Peter Pakkenberg
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Pakkenberg (Gerrit)
Peter Pakkenberg added 1 comment![]( "Open in Gerrit")
while (cpp_navigation_handle_->GetResponseHeaders()->EnumerateHeaderLines(Peter ConnSorry, forget everything I said about using a map here 😞
`Set-Cookie` is ruining our day here. It can be repeated, and is _not_ safe to concatenate with h a comma.
(There is also no guarantee that the server won't send other repeated headers that cannot safely be comma-concatenated).
The whole API needs to either special case `Set-Cookie` or treat response headers like a list of pairs.
Peter PakkenbergI thought that as well (and had implemented it [here](https://chromium-review.git.corp.google.com/c/chromium/src/+/7955997/5)), but it turns out that the `Set-Cookie` header is stripped out by the network service before it gets to us here.
Given that this is consistent with how WebResourceResponse works, I figure we can keep the API simple and let people deal with cookies through the CookieManager.
Peter ConnOk - we should just make sure to document this behavior then.
But depending on custom headers sent by the server, this coalescing might not be safe to do, especially for non-standard headers.
Sure, I'll document.
Making the object here more complex than a map opens us up to more design complexity - do we return a list of values for every header, which is kinda ugly, or have two different getters and make the developer choose which one whenever they get something - or do we just expose a list of pairs and leave it up to the developer to sort.
According to Torne, the browser ends up concatenating all non-Set-Cookie headers, so I'm tempted to leave the API simple and not support people doing things in WebView with non-standard headers that would be interpreted in a different way from the browser.
- Peter Conn
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Pakkenberg (Gerrit)
Peter Pakkenberg voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Peter Conn
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Peter Conn
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |