Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1907443006:
PlzNavigate: store POST data in the FrameNavigationEntry (Closed)
|
Created:
4 years, 8 months ago by clamy Modified:
4 years, 7 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, blink-reviews, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, carlosk, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: store POST data in the FrameNavigationEntry
This allows to properly resend it during hitsory navigations when
browser-side-navigation is enabled.
A rational for the design can be found in the following doc: https://docs.google.com/a/chromium.org/document/d/1BtrZjP-lbeza2OdRwr-68poSgGPE6YAfUftP5s1ugY8/edit?usp=sharing
BUG=575210, 608372
TBR=nasko@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/34754b494e8f9706aa48463dcd1f17026ad658a2
Cr-Commit-Position: refs/heads/master@{#395131}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 10
Patch Set 5 : Rebase #Patch Set 6 : Not storing ResourceRequestBody in FrameNavigationEntry anymore #Patch Set 7 : Cleanup #Patch Set 8 : Rebase #Patch Set 9 : Not sending POST data when cross-site redirect #
Total comments: 8
Patch Set 10 : Addressed comments #
Total comments: 45
Patch Set 11 : Addressed comments #
Total comments: 2
Patch Set 12 : Rebase #Patch Set 13 : Addressed comments #
Total comments: 4
Patch Set 14 : Rebase + addressed comments #Messages
Total messages: 52 (21 generated)
Description was changed from ========== PlzNavigate: store POST data in the FrameNavigationEntry This allows to properly resend it during hitsory navigations when browser-side-navigation is enabled. BUG=575210 ========== to ========== PlzNavigate: store POST data in the FrameNavigationEntry This allows to properly resend it during hitsory navigations when browser-side-navigation is enabled. BUG=575210 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== PlzNavigate: store POST data in the FrameNavigationEntry This allows to properly resend it during hitsory navigations when browser-side-navigation is enabled. BUG=575210 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: store POST data in the FrameNavigationEntry This allows to properly resend it during hitsory navigations when browser-side-navigation is enabled. BUG=575210 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
clamy@chromium.org changed reviewers: + creis@chromium.org, nasko@chromium.org
@nasko, creis: PTAL. This is a followup on the patch storing is_post on FrameNavigationEntries, for the POST data. This enables making proper POST history navigations with PlzNavigate. It's a bit cumbersome, so ideas to improve it are very welcomed :).
Hmm, I'm having trouble following the design here. Can you share some notes in a doc about what approach you're taking? (We should also provide more details in the CL description once we settle on a design.) My general impression is that the POST data is stored inside the PageState, so maybe we should be getting it out of there when we need it rather than plumbing a second copy? (The eventual goal is to make PageState into a FrameState class with more transparent access to its members, which might make that easier.) https://codereview.chromium.org/1907443006/diff/60001/content/browser/frame_h... File content/browser/frame_host/frame_navigation_entry.h (right): https://codereview.chromium.org/1907443006/diff/60001/content/browser/frame_h... content/browser/frame_host/frame_navigation_entry.h:154: scoped_refptr<ResourceRequestBody> post_data_; This wasn't stored on NavigationEntry before, was it? How did it used to be handled? If it used to be in PageState, then I'm concerned that adding a second copy here will be expensive. (POST data can be large, right?) Can we just get the data out of the FNE's PageState instead? https://codereview.chromium.org/1907443006/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.h (right): https://codereview.chromium.org/1907443006/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.h:153: scoped_refptr<ResourceRequestBody> post_data); We shouldn't be adding more parameters here. Can it go in |params| instead? https://codereview.chromium.org/1907443006/diff/60001/content/common/page_sta... File content/common/page_state_serialization.cc (right): https://codereview.chromium.org/1907443006/diff/60001/content/common/page_sta... content/common/page_state_serialization.cc:753: bool GeneratePostData(const ExplodedHttpBody exploded, I'm not following why we need this. Was this code somewhere else before? https://codereview.chromium.org/1907443006/diff/60001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1907443006/diff/60001/content/renderer/render... content/renderer/render_frame_impl.cc:546: void AddHTTPBodyToRequest(WebURLRequest* request, I'm not sure I understand this either. Where is this code coming from? It's also not obvious what the relationship between this and POST data is, so it would need a comment. https://codereview.chromium.org/1907443006/diff/60001/content/renderer/render... content/renderer/render_frame_impl.cc:4610: if (IsBrowserSideNavigationEnabled()) { I'm nervous about making this PlzNavigate-only. Is there a way to do something that's consistent with all FrameNavigationEntry modes?
Thanks! To clarify some of the conversions happening in the patch, currently we have: In the renderer ExplodedHTTPBody -> WebHTTPBody -> ResourceRequestBody (sent to the net stack) and WebHTTPBody -> ExplodedHTTPBody to generate the PageState. With the latest version of this patch and PlzNavigate, we have: In the browser: ExplodedHTTPBody -> ResourceRequestBody (sent to the net stack and the renderer on commit) In the renderer: ResourceRequestBody -> WebHTTPBody -> ExplodedHTTPBody to generate the PageState. https://codereview.chromium.org/1907443006/diff/60001/content/browser/frame_h... File content/browser/frame_host/frame_navigation_entry.h (right): https://codereview.chromium.org/1907443006/diff/60001/content/browser/frame_h... content/browser/frame_host/frame_navigation_entry.h:154: scoped_refptr<ResourceRequestBody> post_data_; On 2016/04/27 23:00:55, Charlie Reis wrote: > This wasn't stored on NavigationEntry before, was it? How did it used to be > handled? > > If it used to be in PageState, then I'm concerned that adding a second copy here > will be expensive. (POST data can be large, right?) > > Can we just get the data out of the FNE's PageState instead? Done. https://codereview.chromium.org/1907443006/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.h (right): https://codereview.chromium.org/1907443006/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.h:153: scoped_refptr<ResourceRequestBody> post_data); On 2016/04/27 23:00:55, Charlie Reis wrote: > We shouldn't be adding more parameters here. Can it go in |params| instead? We no longer send the ResourceRequestBody back to the browser (since the info is in the PageState). https://codereview.chromium.org/1907443006/diff/60001/content/common/page_sta... File content/common/page_state_serialization.cc (right): https://codereview.chromium.org/1907443006/diff/60001/content/common/page_sta... content/common/page_state_serialization.cc:753: bool GeneratePostData(const ExplodedHttpBody exploded, On 2016/04/27 23:00:55, Charlie Reis wrote: > I'm not following why we need this. Was this code somewhere else before? We need it because we have code to generate a WebHTTPBody (which is a renderer object), but we don't have code to generate the ResourceRequestBody (which is the common object that can be used in the browser). https://codereview.chromium.org/1907443006/diff/60001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1907443006/diff/60001/content/renderer/render... content/renderer/render_frame_impl.cc:546: void AddHTTPBodyToRequest(WebURLRequest* request, On 2016/04/27 23:00:56, Charlie Reis wrote: > I'm not sure I understand this either. Where is this code coming from? It's > also not obvious what the relationship between this and POST data is, so it > would need a comment. This is new code. We have a way to convert a WebHTTPBody into a ResourceRequestBody but not the reverse. And in PlzNavigate, I have a ResourceRequestBody from the start, and I need to convert it to a WebHTTPPBody so that it is put in the WebURLRequest, which will allow it to end up in the PageState eventually. https://codereview.chromium.org/1907443006/diff/60001/content/renderer/render... content/renderer/render_frame_impl.cc:4610: if (IsBrowserSideNavigationEnabled()) { On 2016/04/27 23:00:56, Charlie Reis wrote: > I'm nervous about making this PlzNavigate-only. Is there a way to do something > that's consistent with all FrameNavigationEntry modes? We no longer send the POST data to the browser.
On 2016/04/29 16:07:17, clamy wrote: > Thanks! To clarify some of the conversions happening in the patch, currently we > have: > In the renderer > ExplodedHTTPBody -> WebHTTPBody -> ResourceRequestBody (sent to the net stack) > and WebHTTPBody -> ExplodedHTTPBody to generate the PageState. > > With the latest version of this patch and PlzNavigate, we have: > In the browser: > ExplodedHTTPBody -> ResourceRequestBody (sent to the net stack and the renderer > on commit) > In the renderer: > ResourceRequestBody -> WebHTTPBody -> ExplodedHTTPBody to generate the > PageState. Thanks-- this helps, and it's looking better. I still have several questions I'm wondering about, and I'll try to turn those into comments on the CL. One first question: why does the renderer need to be given the POST data at commit time? That's off limits in Site Isolation in at least some cases: imagine a page that uses a third party login page like Google or Facebook, where the login form posts your password and then redirects to a random site. We can't give your password to the random site's process. (See https://crbug.com/582211#c15 for context, where this came up for the XssAuditor. We're considering only providing the POST data if no redirect is involved, in which case your CL may actually help fix that bug.) Also, please do write a design doc for how the POST logic is changing. This is an area where there are lots of unknowns and current bugs. Documentation about how it works and what's changing will be immensely helpful in reasoning about it.
Description was changed from ========== PlzNavigate: store POST data in the FrameNavigationEntry This allows to properly resend it during hitsory navigations when browser-side-navigation is enabled. BUG=575210 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: store POST data in the FrameNavigationEntry This allows to properly resend it during hitsory navigations when browser-side-navigation is enabled. BUG=575210, 608372 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
So following your comment I've updated the CL so that we don't send the POST data when we encounter a cross-site redirect. The reason I'm sending the POST data to the renderer is so that it ends up in the PageState and is properly serialized and kept in the FrameNavigationEntry. With PlzNavigate, it would be possible for the POST data to never be sent to the renderer, but that requires significantly more changes to the PageState serialization and/or the FrameNavigationEntry serialization. We would have to keep the POST data in the FrameNavigationEntry and patch it inside the PageState each time this one is updated (since we would never have it set in the renderer). Alternatively, we could not use the http body from the PageState (with the idea of removing it when PlzNavigate launches), and have it always be part of the FrameNavigationEntry (and be serialized alongside the data of the FrameNavigationEntry). But that raises the issue of backwards compatibility (though maybe if we only use it for PlzNavigate this is less of an issue ?). While I agree that it would be better to not send the data to the renderer, I was worried about the maintenance of a very different codepath from the actual navigations (so my idea was to change that after PlzNavigate ships). Now if it turns out that we also need the ability to store the POST data for SiteIsolation without sending it to the renderer for Site Isolation as well, I can look at how to serialize/deserialize everything in the browser.
On 2016/05/03 15:21:11, clamy wrote: > So following your comment I've updated the CL so that we don't send the POST > data when we encounter a cross-site redirect. > > The reason I'm sending the POST data to the renderer is so that it ends up in > the PageState and is properly serialized and kept in the FrameNavigationEntry. > > With PlzNavigate, it would be possible for the POST data to never be sent to the > renderer, but that requires significantly more changes to the PageState > serialization and/or the FrameNavigationEntry serialization. We would have to > keep the POST data in the FrameNavigationEntry and patch it inside the PageState > each time this one is updated (since we would never have it set in the > renderer). Alternatively, we could not use the http body from the PageState > (with the idea of removing it when PlzNavigate launches), and have it always be > part of the FrameNavigationEntry (and be serialized alongside the data of the > FrameNavigationEntry). But that raises the issue of backwards compatibility > (though maybe if we only use it for PlzNavigate this is less of an issue ?). > > While I agree that it would be better to not send the data to the renderer, I > was worried about the maintenance of a very different codepath from the actual > navigations (so my idea was to change that after PlzNavigate ships). Now if it > turns out that we also need the ability to store the POST data for SiteIsolation > without sending it to the renderer for Site Isolation as well, I can look at how > to serialize/deserialize everything in the browser. Ok, there may be reasons to keep sending the data the renderer in the cases we know it's safe, then. As I mentioned before, please share a design doc for this change. There's too much here (especially given the implications for lukasza@ on https://crbug.com/582211) to reason about it without having a writeup of the design decisions involved.
creis@chromium.org changed reviewers: + lukasza@chromium.org
[+lukasza] Thanks-- some thoughts below. Can you add a link to the design doc (https://docs.google.com/a/chromium.org/document/d/1BtrZjP-lbeza2OdRwr-68poSgG...) in the CL description and bug 608372? Thanks for putting it together-- it's very helpful for understanding what changes are needed. I think we'll need more comments on the various POST data conversion helper methods to indicate why they're needed, since it requires a lot of background knowledge to understand what each one is for. I'm also adding Lukasz to the review, since he's looking at what's needed to fix the transfer case in OOPIFs. I suspect there will be a lot of overlap. He's also found some interesting/surprising info on POST submissions and redirects, which I mention below. I'll look more deeply after we resolve some of these high level questions. https://codereview.chromium.org/1907443006/diff/160001/content/browser/frame_... File content/browser/frame_host/frame_navigation_entry.cc (right): https://codereview.chromium.org/1907443006/diff/160001/content/browser/frame_... content/browser/frame_host/frame_navigation_entry.cc:111: if (!RecursivelyFillBody(exploded_state.top, frame_unique_name_, body.get())) Why is this recursive? The PageState on a FrameNavigationEntry is only for this single frame if we're in UseSubframeNavigationEntries mode, which is the one we'd be looking for anyway. Will this ever be called in default Chrome? Maybe we should remove the recursive call and put a DCHECK that we're in UseSubframeNavigationEntries mode. https://codereview.chromium.org/1907443006/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_entry_impl.cc (right): https://codereview.chromium.org/1907443006/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_entry_impl.cc:594: if (IsBrowserSideNavigationEnabled()) At some point, I think we'll need this in all UseSubframeNavigationEntries modes, not just PlzNavigate. (It's necessary for fixing transfers in --isolate-extensions, as we noted on the bug.) If you want this CL to stay focused on PlzNavigate, please put a TODO to consult the frame_entry in OOPIF modes as well. https://codereview.chromium.org/1907443006/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_request.h (right): https://codereview.chromium.org/1907443006/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.h:164: void ResetPostData(); Lukasz found some useful info on this in the spec (which is quite surprising to me). Apparently POST data is preserved on 307 and 308 redirects (which stay POST), regardless of origin. 302, 303, and 304 redirects are converted to GET and lose POST data. While I think it's crazy that sites might pass on their POST data cross-site without realizing it, we should probably preserve Chrome's current behavior here (once we verify that this is true in practice). Not sure what implication that has for this method. https://codereview.chromium.org/1907443006/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1907443006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:909: // navigation encountered a cross-site redirect. See my other comment on ResetPostData. Based on what Lukasz found, we might not need to do this at all. (And hopefully not, because the is_commit parameter feels awkward.)
Description was changed from ========== PlzNavigate: store POST data in the FrameNavigationEntry This allows to properly resend it during hitsory navigations when browser-side-navigation is enabled. BUG=575210, 608372 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: store POST data in the FrameNavigationEntry This allows to properly resend it during hitsory navigations when browser-side-navigation is enabled. A rational for the design can be found in the following doc: https://docs.google.com/a/chromium.org/document/d/1BtrZjP-lbeza2OdRwr-68poSgG... BUG=575210, 608372 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Thanks! Updated the CL description with the link to the design doc. https://codereview.chromium.org/1907443006/diff/160001/content/browser/frame_... File content/browser/frame_host/frame_navigation_entry.cc (right): https://codereview.chromium.org/1907443006/diff/160001/content/browser/frame_... content/browser/frame_host/frame_navigation_entry.cc:111: if (!RecursivelyFillBody(exploded_state.top, frame_unique_name_, body.get())) On 2016/05/11 00:00:23, Charlie Reis wrote: > Why is this recursive? The PageState on a FrameNavigationEntry is only for this > single frame if we're in UseSubframeNavigationEntries mode, which is the one > we'd be looking for anyway. > > Will this ever be called in default Chrome? Maybe we should remove the > recursive call and put a DCHECK that we're in UseSubframeNavigationEntries mode. Ah I didn't know there was only one frame in the PageState. Changed the function accordingly. https://codereview.chromium.org/1907443006/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_entry_impl.cc (right): https://codereview.chromium.org/1907443006/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_entry_impl.cc:594: if (IsBrowserSideNavigationEnabled()) On 2016/05/11 00:00:23, Charlie Reis wrote: > At some point, I think we'll need this in all UseSubframeNavigationEntries > modes, not just PlzNavigate. (It's necessary for fixing transfers in > --isolate-extensions, as we noted on the bug.) > > If you want this CL to stay focused on PlzNavigate, please put a TODO to consult > the frame_entry in OOPIF modes as well. Added a TODO. I prefer this CL to stay focused on PlzNavigate (it's already bulky enough like this). https://codereview.chromium.org/1907443006/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_request.h (right): https://codereview.chromium.org/1907443006/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.h:164: void ResetPostData(); On 2016/05/11 00:00:23, Charlie Reis wrote: > Lukasz found some useful info on this in the spec (which is quite surprising to > me). Apparently POST data is preserved on 307 and 308 redirects (which stay > POST), regardless of origin. 302, 303, and 304 redirects are converted to GET > and lose POST data. > > While I think it's crazy that sites might pass on their POST data cross-site > without realizing it, we should probably preserve Chrome's current behavior here > (once we verify that this is true in practice). Not sure what implication that > has for this method. Removed the function, since the current behavior can be matched in NavigationReques internally. We may reintroduce it if we decide to have POST data be reset cross-site when --site-per-process is enabled. https://codereview.chromium.org/1907443006/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1907443006/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:909: // navigation encountered a cross-site redirect. On 2016/05/11 00:00:23, Charlie Reis wrote: > See my other comment on ResetPostData. Based on what Lukasz found, we might not > need to do this at all. (And hopefully not, because the is_commit parameter > feels awkward.) Done. We indeed don't need to do this.
Thanks, this is looking much better. I haven't had time to review ResourceRequestBody::AppendExplodedHTTPBodyElement or RenderFrameImpl's AddHTTPBodyToRequest in depth yet, but I think we're on the right track. Lukasz, does this look like a good start towards the OOPIF fix as well? Would you be able to review those conversion functions up close, after the investigation you've done so far? I feel bad for holding this up, given the firehose of reviews and requests I'm getting. If not, I'll find time to look closely at them tomorrow. https://codereview.chromium.org/1907443006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1907443006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:461: manager->GetFrameHostForNavigation(navigation_request.get(), true)); Stale param, here and below. https://codereview.chromium.org/1907443006/diff/180001/content/common/page_st... File content/common/page_state_serialization.h (right): https://codereview.chromium.org/1907443006/diff/180001/content/common/page_st... content/common/page_state_serialization.h:93: // This method converts the ExplodedHttpBody in a ResourceRequestBody format. nit: s/in/into/
> Lukasz, does this look like a good start towards the OOPIF fix as well? Looks good to me. I don't want to hold this CL hostage, because I have not been able to figure out yet how to plumb the post data in the non-PlzNavigate mode :-) > Would you be able to review those conversion functions up close, after the > investigation you've done so far? I feel bad for holding this up, given the > firehose of reviews and requests I'm getting. I tried to take a look and left some hopefully helpful comments. At the same time, note that I am not an OWNER of anything and I am pretty new to this stuff, so take them with a grain of salt :-) https://codereview.chromium.org/1907443006/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_entry_impl.cc (right): https://codereview.chromium.org/1907443006/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_entry_impl.cc:593: method = frame_entry.method(); Just trying to understand: what is preventing consulting frame_entry.method() in all the cases? Is it just hardcoded method in 2 places in NavigatorImpl::RequestTransferURL? Or are there more places that don't correctly populate frame_entry.method()? https://codereview.chromium.org/1907443006/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1907443006/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:204: post_data_ = body->MakeCopy(); Can you help me understand why we need to make a deep copy here (rather than just take another ref-count)? I thought that ResourceRequestBody would never be changed after initial construction - is this not the case / is there a need to mutate ResourceRequestBody in some cases? https://codereview.chromium.org/1907443006/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:266: if (common_params_.method == "POST" && redirect_info.new_method != "POST") Shouldn't the second part of the condition be sufficient? (i.e. we don't really need to consult |common_params_.method|, right?) https://codereview.chromium.org/1907443006/diff/180001/content/common/resourc... File content/common/resource_request_body.cc (right): https://codereview.chromium.org/1907443006/diff/180001/content/common/resourc... content/common/resource_request_body.cc:26: AppendBytes(element.data.data(), static_cast<int>(element.data.size())); It is not immediately clear why size_t -> int cast won't lead to trouble. Is there something here or elsewhere that guarantees that the size will fit into an int? If yes, could you please add a DCHECK + a comment explaining this? https://codereview.chromium.org/1907443006/diff/180001/content/common/resourc... content/common/resource_request_body.cc:33: std::numeric_limits<uint64_t>::max(), base::Time()); I see that UploadFileElementReader::OnGetFileInfoCompleted should deal with max_uint64_t fine, but I wonder 1) if other consumers of ResourceRequestBody are aware of this convention and 2) if this convention should be documented in a comment somewhere (maybe in declarations of ResourceRequestBody::AppendFileRange and ResourceRequestBody::AppendFileSystemFileRange; maybe next to fields or accessors of storage::DataElement?). https://codereview.chromium.org/1907443006/diff/180001/content/common/resourc... content/common/resource_request_body.cc:39: base::Time::FromDoubleT(element.file_modification_time)); Just wanted to double-check - is it okay to use base::Time() above, but need to use base::Time::FromDoubleT(element.file_modification_time) here? i.e. the expected file_modification_time is only applicable to file ranges, but never applicable to whole files? If so, then it would be great if this was explained in a comment somewhere in WebHTTPBody. (although it seems that Blink's historical style is to have as little comments as possible... :-( ). https://codereview.chromium.org/1907443006/diff/180001/content/common/resourc... content/common/resource_request_body.cc:44: DCHECK(file_system_url.SchemeIsFileSystem()); I guess a DCHECK is sufficient, because UploadFileSystemFileElementReader::Init will sanitize the url? https://codereview.chromium.org/1907443006/diff/180001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1907443006/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:547: scoped_refptr<ResourceRequestBody> body) { :-) I've started creating my own version of this code (OTOH, I see that your version is better - my version for example I didn't copy the identifier :-/). https://codereview.chromium.org/1956383003/patch/20001/30017 https://codereview.chromium.org/1907443006/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:551: for (auto element : *(body->elements())) { nit: Could you replace auto with: const ResourceRequestBody::Element& ? https://codereview.chromium.org/1907443006/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:555: reinterpret_cast<const char*>(element.bytes()), element.length())); Why is reinterpret_cast needed here? reinterpret_casts are evil, right? FWIW, similar code compiled fine without the cast at https://codereview.chromium.org/1956383003/patch/20001/30017? https://codereview.chromium.org/1907443006/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:559: element.path().AsUTF16Unsafe(), element.offset(), element.length(), Should we translate back std::numeric_limits<uint64_t>::max() into -1, as done in the other translation direction at https://codereview.chromium.org/1907443006/patch/180001/190014 ? Or will Blink understand / gracefully deal with this? I guess this might already work today because in 2s-complement there is (-1) == static_cast<long long>(std::numeric_limits<uint64_t>::max()), but it feels better to be explicit. https://codereview.chromium.org/1907443006/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:570: default: nit: I wonder if we should explicitly spell out types that we don't expect to encounter - spelling them out will show future maintainers that we've thought about these types (rather than having future maintainers wonder if some types were added after this switch statement). FWIW, this is what is done in UploadDataStreamBuilder::Build - https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo...
Thanks! For review speed, note that I'll be traveling all of tomorrow, and Monday is a public holiday in Paris, so I won't have a look at it before Tuesday. https://codereview.chromium.org/1907443006/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_entry_impl.cc (right): https://codereview.chromium.org/1907443006/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_entry_impl.cc:593: method = frame_entry.method(); On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > Just trying to understand: what is preventing consulting frame_entry.method() in > all the cases? Is it just hardcoded method in 2 places in > NavigatorImpl::RequestTransferURL? Or are there more places that don't > correctly populate frame_entry.method()? The issue is that the current architecture expects the method to be set to POST only for main frames, not subframes. So it's probably not populated properly if you're not using subframe navigation entries. https://codereview.chromium.org/1907443006/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1907443006/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:204: post_data_ = body->MakeCopy(); On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > Can you help me understand why we need to make a deep copy here (rather than > just take another ref-count)? I thought that ResourceRequestBody would never be > changed after initial construction - is this not the case / is there a need to > mutate ResourceRequestBody in some cases? I wasn't sure about this so I made a copy to stay safe. If someone from the net stack team explained that it would not be changed, then it's probably safe not to make a copy. https://codereview.chromium.org/1907443006/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:266: if (common_params_.method == "POST" && redirect_info.new_method != "POST") On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > Shouldn't the second part of the condition be sufficient? (i.e. we don't really > need to consult |common_params_.method|, right?) No but I thought it made a bit clearer the case in which we actually reset the request. Can remove the second part if needed. https://codereview.chromium.org/1907443006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1907443006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:461: manager->GetFrameHostForNavigation(navigation_request.get(), true)); On 2016/05/11 23:58:53, Charlie Reis wrote: > Stale param, here and below. Done. https://codereview.chromium.org/1907443006/diff/180001/content/common/page_st... File content/common/page_state_serialization.h (right): https://codereview.chromium.org/1907443006/diff/180001/content/common/page_st... content/common/page_state_serialization.h:93: // This method converts the ExplodedHttpBody in a ResourceRequestBody format. On 2016/05/11 23:58:53, Charlie Reis wrote: > nit: s/in/into/ Done. https://codereview.chromium.org/1907443006/diff/180001/content/common/resourc... File content/common/resource_request_body.cc (right): https://codereview.chromium.org/1907443006/diff/180001/content/common/resourc... content/common/resource_request_body.cc:26: AppendBytes(element.data.data(), static_cast<int>(element.data.size())); On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > It is not immediately clear why size_t -> int cast won't lead to trouble. > > Is there something here or elsewhere that guarantees that the size will fit into > an int? If yes, could you please add a DCHECK + a comment explaining this? This whole method is a copy of what we do to convert a WebHTTPBody to a ResourceRequestBody. It appears all the underlying code (ie ResourceRequestBody and DataElement) expect an int there, hence the cast. In case it ends up being negative, nothing will be added to the body. https://codereview.chromium.org/1907443006/diff/180001/content/common/resourc... content/common/resource_request_body.cc:33: std::numeric_limits<uint64_t>::max(), base::Time()); On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > I see that UploadFileElementReader::OnGetFileInfoCompleted should deal with > max_uint64_t fine, but I wonder 1) if other consumers of ResourceRequestBody are > aware of this convention and 2) if this convention should be documented in a > comment somewhere (maybe in declarations of ResourceRequestBody::AppendFileRange > and ResourceRequestBody::AppendFileSystemFileRange; maybe next to fields or > accessors of storage::DataElement?). Possibly? As explained above, I'm just copy pasting the code for GetRequestBodyForWebURLRequest from web_url_request_util.cc (it used to be in weburlloaderimpl before being moved). So since we instantiate it that way there, I expect consumers of ResourceRequestBody to be aware of it. https://codereview.chromium.org/1907443006/diff/180001/content/common/resourc... content/common/resource_request_body.cc:39: base::Time::FromDoubleT(element.file_modification_time)); On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > Just wanted to double-check - is it okay to use base::Time() above, but need to > use base::Time::FromDoubleT(element.file_modification_time) here? i.e. the > expected file_modification_time is only applicable to file ranges, but never > applicable to whole files? If so, then it would be great if this was explained > in a comment somewhere in WebHTTPBody. (although it seems that Blink's > historical style is to have as little comments as possible... :-( ). Again I imagine it's ok since we've been doing it. Yes WebKit style guide on the comment part is questionnable, though this is based on code coming from content/ so it could have come with a little more comments... https://codereview.chromium.org/1907443006/diff/180001/content/common/resourc... content/common/resource_request_body.cc:44: DCHECK(file_system_url.SchemeIsFileSystem()); On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > I guess a DCHECK is sufficient, because UploadFileSystemFileElementReader::Init > will sanitize the url? I imagine. I don't know if we want to crash the renderer over this (and honestly if it has to be, I'd rather we use a bad message when receiving the message). https://codereview.chromium.org/1907443006/diff/180001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1907443006/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:547: scoped_refptr<ResourceRequestBody> body) { On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > :-) I've started creating my own version of this code (OTOH, I see that your > version is better - my version for example I didn't copy the identifier :-/). > https://codereview.chromium.org/1956383003/patch/20001/30017 This is based on the code from content/renderer/history_serialization.cc. https://codereview.chromium.org/1907443006/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:551: for (auto element : *(body->elements())) { On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > nit: Could you replace auto with: const ResourceRequestBody::Element& ? Done. https://codereview.chromium.org/1907443006/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:555: reinterpret_cast<const char*>(element.bytes()), element.length())); On 2016/05/12 02:54:00, Łukasz Anforowicz wrote: > Why is reinterpret_cast needed here? reinterpret_casts are evil, right? FWIW, > similar code compiled fine without the cast at > https://codereview.chromium.org/1956383003/patch/20001/30017 Done. https://codereview.chromium.org/1907443006/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:559: element.path().AsUTF16Unsafe(), element.offset(), element.length(), On 2016/05/12 02:54:00, Łukasz Anforowicz wrote: > Should we translate back std::numeric_limits<uint64_t>::max() into -1, as done > in the other translation direction at > https://codereview.chromium.org/1907443006/patch/180001/190014 ? Or will Blink > understand / gracefully deal with this? I guess this might already work today > because in 2s-complement there is (-1) == static_cast<long > long>(std::numeric_limits<uint64_t>::max()), but it feels better to be explicit. Done. https://codereview.chromium.org/1907443006/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:570: default: On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > nit: I wonder if we should explicitly spell out types that we don't expect to > encounter - spelling them out will show future maintainers that we've thought > about these types (rather than having future maintainers wonder if some types > were added after this switch statement). FWIW, this is what is done in > UploadDataStreamBuilder::Build - > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo... Done.
https://codereview.chromium.org/1907443006/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_entry_impl.cc (right): https://codereview.chromium.org/1907443006/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_entry_impl.cc:593: method = frame_entry.method(); On 2016/05/12 08:53:13, clamy wrote: > On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > > Just trying to understand: what is preventing consulting frame_entry.method() > in > > all the cases? Is it just hardcoded method in 2 places in > > NavigatorImpl::RequestTransferURL? Or are there more places that don't > > correctly populate frame_entry.method()? > > The issue is that the current architecture expects the method to be set to POST > only for main frames, not subframes. So it's probably not populated properly if > you're not using subframe navigation entries. Acknowledged. https://codereview.chromium.org/1907443006/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1907443006/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:204: post_data_ = body->MakeCopy(); On 2016/05/12 08:53:13, clamy wrote: > On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > > Can you help me understand why we need to make a deep copy here (rather than > > just take another ref-count)? I thought that ResourceRequestBody would never > be > > changed after initial construction - is this not the case / is there a need to > > mutate ResourceRequestBody in some cases? > > I wasn't sure about this so I made a copy to stay safe. If someone from the net > stack team explained that it would not be changed, then it's probably safe not > to make a copy. I think it should be safe not to make a copy - I've tried to look at the callers of ResourceRequestBody's non-const methods (AppendBytes, AppendFileRange, AppendBlob, AppendFileSystemFileRange, elements_mutable, swap_elements, set_identifier) and they appear to only come from code that constructs a fresh ResourceRequestBody (or from test code). Interestingly Cr Code Search was not able to find users of elements_mutable. Seems like a job for Scythe :-) So - I would vote from removing the deep copy, but you are right, that it might be a good idea to double-check this with a //net person. https://codereview.chromium.org/1907443006/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:266: if (common_params_.method == "POST" && redirect_info.new_method != "POST") On 2016/05/12 08:53:13, clamy wrote: > On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > > Shouldn't the second part of the condition be sufficient? (i.e. we don't > really > > need to consult |common_params_.method|, right?) > > No but I thought it made a bit clearer the case in which we actually reset the > request. Can remove the second part if needed. Acknowledged. I don't strongly feel one way or another. https://codereview.chromium.org/1907443006/diff/180001/content/common/resourc... File content/common/resource_request_body.cc (right): https://codereview.chromium.org/1907443006/diff/180001/content/common/resourc... content/common/resource_request_body.cc:26: AppendBytes(element.data.data(), static_cast<int>(element.data.size())); On 2016/05/12 08:53:13, clamy wrote: > On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > > It is not immediately clear why size_t -> int cast won't lead to trouble. > > > > Is there something here or elsewhere that guarantees that the size will fit > into > > an int? If yes, could you please add a DCHECK + a comment explaining this? > > This whole method is a copy of what we do to convert a WebHTTPBody to a > ResourceRequestBody. It is pretty sad that we have to repeat almost the same code, just because ExplodedHttpBodyElement and WebHTTPBody::Element are different structs (they have almost the same fields - they both have blink::WebHTTPBody::Element::Type type field and the other fields just replace blink-types with base or std types - i.e. WebString with std::string). That said, I guess your CL hasn't introduced these separate structs, so there isn't really anything to be done in the current CL... https://codereview.chromium.org/1907443006/diff/180001/content/common/resourc... content/common/resource_request_body.cc:33: std::numeric_limits<uint64_t>::max(), base::Time()); On 2016/05/12 08:53:13, clamy wrote: > On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > > I see that UploadFileElementReader::OnGetFileInfoCompleted should deal with > > max_uint64_t fine, but I wonder 1) if other consumers of ResourceRequestBody > are > > aware of this convention and 2) if this convention should be documented in a > > comment somewhere (maybe in declarations of > ResourceRequestBody::AppendFileRange > > and ResourceRequestBody::AppendFileSystemFileRange; maybe next to fields or > > accessors of storage::DataElement?). > > Possibly? As explained above, I'm just copy pasting the code for > GetRequestBodyForWebURLRequest from web_url_request_util.cc (it used to be in > weburlloaderimpl before being moved). So since we instantiate it that way there, > I expect consumers of ResourceRequestBody to be aware of it. Acknowledged. https://codereview.chromium.org/1907443006/diff/180001/content/common/resourc... content/common/resource_request_body.cc:39: base::Time::FromDoubleT(element.file_modification_time)); On 2016/05/12 08:53:13, clamy wrote: > On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > > Just wanted to double-check - is it okay to use base::Time() above, but need > to > > use base::Time::FromDoubleT(element.file_modification_time) here? i.e. the > > expected file_modification_time is only applicable to file ranges, but never > > applicable to whole files? If so, then it would be great if this was > explained > > in a comment somewhere in WebHTTPBody. (although it seems that Blink's > > historical style is to have as little comments as possible... :-( ). > > Again I imagine it's ok since we've been doing it. Yes WebKit style guide on the > comment part is questionnable, though this is based on code coming from content/ > so it could have come with a little more comments... Acknowledged. https://codereview.chromium.org/1907443006/diff/180001/content/common/resourc... content/common/resource_request_body.cc:44: DCHECK(file_system_url.SchemeIsFileSystem()); On 2016/05/12 08:53:13, clamy wrote: > On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > > I guess a DCHECK is sufficient, because > UploadFileSystemFileElementReader::Init > > will sanitize the url? > > I imagine. I don't know if we want to crash the renderer over this (and honestly > if it has to be, I'd rather we use a bad message when receiving the message). Acknowledged. https://codereview.chromium.org/1907443006/diff/180001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1907443006/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:547: scoped_refptr<ResourceRequestBody> body) { On 2016/05/12 08:53:13, clamy wrote: > On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > > :-) I've started creating my own version of this code (OTOH, I see that your > > version is better - my version for example I didn't copy the identifier :-/). > > https://codereview.chromium.org/1956383003/patch/20001/30017 > > This is based on the code from content/renderer/history_serialization.cc. I see. Fascinating that we have 3 separate structs for essentially the same thing - ExplodedHttpBodyElement, WebHTTPBody::Element and ResourceRequestBody::Element. If those structs truly need to stay the same then I guess there is no way around having to have separate convertion code... :-(
lgtm from my side, assuming that the deep copy of ResourceRequestBody will get looked into and possibly removed.
Ok, I'm happy with this overall, though it'd be good to avoid the deep copy, and I'd like to make sure we're not copy/pasting bugs or unreadable code. :) Once we're happy with the conversion functions (or at least have TODOs for where potential landmines are), this should be ready to go. https://codereview.chromium.org/1907443006/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1907443006/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:204: post_data_ = body->MakeCopy(); On 2016/05/12 19:44:12, Łukasz Anforowicz wrote: > On 2016/05/12 08:53:13, clamy wrote: > > On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > > > Can you help me understand why we need to make a deep copy here (rather than > > > just take another ref-count)? I thought that ResourceRequestBody would > never > > be > > > changed after initial construction - is this not the case / is there a need > to > > > mutate ResourceRequestBody in some cases? > > > > I wasn't sure about this so I made a copy to stay safe. If someone from the > net > > stack team explained that it would not be changed, then it's probably safe not > > to make a copy. > > I think it should be safe not to make a copy - I've tried to look at the callers > of ResourceRequestBody's non-const methods (AppendBytes, AppendFileRange, > AppendBlob, AppendFileSystemFileRange, elements_mutable, swap_elements, > set_identifier) and they appear to only come from code that constructs a fresh > ResourceRequestBody (or from test code). > > Interestingly Cr Code Search was not able to find users of elements_mutable. > Seems like a job for Scythe :-) > > So - I would vote from removing the deep copy, but you are right, that it might > be a good idea to double-check this with a //net person. +1 to removing the deep copy if we can. https://codereview.chromium.org/1907443006/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:266: if (common_params_.method == "POST" && redirect_info.new_method != "POST") On 2016/05/12 19:44:12, Łukasz Anforowicz wrote: > On 2016/05/12 08:53:13, clamy wrote: > > On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > > > Shouldn't the second part of the condition be sufficient? (i.e. we don't > > really > > > need to consult |common_params_.method|, right?) > > > > No but I thought it made a bit clearer the case in which we actually reset the > > request. Can remove the second part if needed. > > Acknowledged. I don't strongly feel one way or another. I think we should remove the first part (common_params_.method == "POST"). It's not likely that post_data_ will exist when common_params_.method != "POST", but if it did, it would be bad to preserve it after a redirect to a GET. https://codereview.chromium.org/1907443006/diff/180001/content/common/resourc... File content/common/resource_request_body.cc (right): https://codereview.chromium.org/1907443006/diff/180001/content/common/resourc... content/common/resource_request_body.cc:26: AppendBytes(element.data.data(), static_cast<int>(element.data.size())); On 2016/05/12 19:44:12, Łukasz Anforowicz wrote: > On 2016/05/12 08:53:13, clamy wrote: > > On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > > > It is not immediately clear why size_t -> int cast won't lead to trouble. > > > > > > Is there something here or elsewhere that guarantees that the size will fit > > into > > > an int? If yes, could you please add a DCHECK + a comment explaining this? > > > > This whole method is a copy of what we do to convert a WebHTTPBody to a > > ResourceRequestBody. > > It is pretty sad that we have to repeat almost the same code, just because > ExplodedHttpBodyElement and WebHTTPBody::Element are different structs (they > have almost the same fields - they both have blink::WebHTTPBody::Element::Type > type field and the other fields just replace blink-types with base or std types > - i.e. WebString with std::string). > > That said, I guess your CL hasn't introduced these separate structs, so there > isn't really anything to be done in the current CL... Please add a comment above this method indicating where it's copied from. Even with all this context, I had a hard time finding it and thought you might have been copying from AppendHTTPBodyElement in history_serialization.cc instead of GetRequestBodyForWebURLRequest in web_url_request_util.cc. It's useful info in case we find a way to refactor them to share code in the future. https://codereview.chromium.org/1907443006/diff/180001/content/common/resourc... content/common/resource_request_body.cc:39: base::Time::FromDoubleT(element.file_modification_time)); On 2016/05/12 19:44:12, Łukasz Anforowicz wrote: > On 2016/05/12 08:53:13, clamy wrote: > > On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > > > Just wanted to double-check - is it okay to use base::Time() above, but need > > to > > > use base::Time::FromDoubleT(element.file_modification_time) here? i.e. the > > > expected file_modification_time is only applicable to file ranges, but never > > > applicable to whole files? If so, then it would be great if this was > > explained > > > in a comment somewhere in WebHTTPBody. (although it seems that Blink's > > > historical style is to have as little comments as possible... :-( ). > > > > Again I imagine it's ok since we've been doing it. Yes WebKit style guide on > the > > comment part is questionnable, though this is based on code coming from > content/ > > so it could have come with a little more comments... > > Acknowledged. In general, copy/pasting code isn't an excuse for ignoring issues with it (in terms of bugs or missing comments). :) It's even more important in those cases, since a bug fix in the original location likely won't get applied here as well. Given how these various conversion functions are scattered around the code without references to each other, I think it's worth understanding whether there are issues that need to be fixed and leaving comments where possible. https://codereview.chromium.org/1907443006/diff/200001/content/browser/frame_... File content/browser/frame_host/frame_navigation_entry.cc (right): https://codereview.chromium.org/1907443006/diff/200001/content/browser/frame_... content/browser/frame_host/frame_navigation_entry.cc:9: #include "base/strings/utf_string_conversions.h" Is this stale?
https://codereview.chromium.org/1907443006/diff/180001/content/common/resourc... File content/common/resource_request_body.cc (right): https://codereview.chromium.org/1907443006/diff/180001/content/common/resourc... content/common/resource_request_body.cc:39: base::Time::FromDoubleT(element.file_modification_time)); On 2016/05/16 21:16:41, Charlie Reis (slow to reply) wrote: > On 2016/05/12 19:44:12, Łukasz Anforowicz wrote: > > On 2016/05/12 08:53:13, clamy wrote: > > > On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > > > > Just wanted to double-check - is it okay to use base::Time() above, but > need > > > to > > > > use base::Time::FromDoubleT(element.file_modification_time) here? i.e. > the > > > > expected file_modification_time is only applicable to file ranges, but > never > > > > applicable to whole files? If so, then it would be great if this was > > > explained > > > > in a comment somewhere in WebHTTPBody. (although it seems that Blink's > > > > historical style is to have as little comments as possible... :-( ). > > > > > > Again I imagine it's ok since we've been doing it. Yes WebKit style guide on > > the > > > comment part is questionnable, though this is based on code coming from > > content/ > > > so it could have come with a little more comments... > > > > Acknowledged. > > In general, copy/pasting code isn't an excuse for ignoring issues with it (in > terms of bugs or missing comments). :) It's even more important in those > cases, since a bug fix in the original location likely won't get applied here as > well. > > Given how these various conversion functions are scattered around the code > without references to each other, I think it's worth understanding whether there > are issues that need to be fixed and leaving comments where possible. I wonder if we could avoid copy&pasted conversion code (here and also in render_frame_impl.cc) by getting rid of ExplodedHttpBodyElement as a separate type (and making it a type alias for ResourceRequestBody::Element instead). I've tried putting together a quick (and very dirty for now - some things still don't compile) CL that does this at https://crrev.com/1987053002.
Thanks! https://codereview.chromium.org/1907443006/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1907443006/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:204: post_data_ = body->MakeCopy(); On 2016/05/16 21:16:41, Charlie Reis (slow to reply) wrote: > On 2016/05/12 19:44:12, Łukasz Anforowicz wrote: > > On 2016/05/12 08:53:13, clamy wrote: > > > On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > > > > Can you help me understand why we need to make a deep copy here (rather > than > > > > just take another ref-count)? I thought that ResourceRequestBody would > > never > > > be > > > > changed after initial construction - is this not the case / is there a > need > > to > > > > mutate ResourceRequestBody in some cases? > > > > > > I wasn't sure about this so I made a copy to stay safe. If someone from the > > net > > > stack team explained that it would not be changed, then it's probably safe > not > > > to make a copy. > > > > I think it should be safe not to make a copy - I've tried to look at the > callers > > of ResourceRequestBody's non-const methods (AppendBytes, AppendFileRange, > > AppendBlob, AppendFileSystemFileRange, elements_mutable, swap_elements, > > set_identifier) and they appear to only come from code that constructs a fresh > > ResourceRequestBody (or from test code). > > > > Interestingly Cr Code Search was not able to find users of elements_mutable. > > Seems like a job for Scythe :-) > > > > So - I would vote from removing the deep copy, but you are right, that it > might > > be a good idea to double-check this with a //net person. > > +1 to removing the deep copy if we can. Done. https://codereview.chromium.org/1907443006/diff/180001/content/common/resourc... File content/common/resource_request_body.cc (right): https://codereview.chromium.org/1907443006/diff/180001/content/common/resourc... content/common/resource_request_body.cc:26: AppendBytes(element.data.data(), static_cast<int>(element.data.size())); On 2016/05/16 21:16:42, Charlie Reis (slow to reply) wrote: > On 2016/05/12 19:44:12, Łukasz Anforowicz wrote: > > On 2016/05/12 08:53:13, clamy wrote: > > > On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > > > > It is not immediately clear why size_t -> int cast won't lead to trouble. > > > > > > > > Is there something here or elsewhere that guarantees that the size will > fit > > > into > > > > an int? If yes, could you please add a DCHECK + a comment explaining > this? > > > > > > This whole method is a copy of what we do to convert a WebHTTPBody to a > > > ResourceRequestBody. > > > > It is pretty sad that we have to repeat almost the same code, just because > > ExplodedHttpBodyElement and WebHTTPBody::Element are different structs (they > > have almost the same fields - they both have blink::WebHTTPBody::Element::Type > > type field and the other fields just replace blink-types with base or std > types > > - i.e. WebString with std::string). > > > > That said, I guess your CL hasn't introduced these separate structs, so there > > isn't really anything to be done in the current CL... > > Please add a comment above this method indicating where it's copied from. Even > with all this context, I had a hard time finding it and thought you might have > been copying from AppendHTTPBodyElement in history_serialization.cc instead of > GetRequestBodyForWebURLRequest in web_url_request_util.cc. It's useful info in > case we find a way to refactor them to share code in the future. Done. https://codereview.chromium.org/1907443006/diff/180001/content/common/resourc... content/common/resource_request_body.cc:39: base::Time::FromDoubleT(element.file_modification_time)); On 2016/05/18 00:22:27, Łukasz Anforowicz wrote: > On 2016/05/16 21:16:41, Charlie Reis (slow to reply) wrote: > > On 2016/05/12 19:44:12, Łukasz Anforowicz wrote: > > > On 2016/05/12 08:53:13, clamy wrote: > > > > On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > > > > > Just wanted to double-check - is it okay to use base::Time() above, but > > need > > > > to > > > > > use base::Time::FromDoubleT(element.file_modification_time) here? i.e. > > the > > > > > expected file_modification_time is only applicable to file ranges, but > > never > > > > > applicable to whole files? If so, then it would be great if this was > > > > explained > > > > > in a comment somewhere in WebHTTPBody. (although it seems that Blink's > > > > > historical style is to have as little comments as possible... :-( ). > > > > > > > > Again I imagine it's ok since we've been doing it. Yes WebKit style guide > on > > > the > > > > comment part is questionnable, though this is based on code coming from > > > content/ > > > > so it could have come with a little more comments... > > > > > > Acknowledged. > > > > In general, copy/pasting code isn't an excuse for ignoring issues with it (in > > terms of bugs or missing comments). :) It's even more important in those > > cases, since a bug fix in the original location likely won't get applied here > as > > well. > > > > Given how these various conversion functions are scattered around the code > > without references to each other, I think it's worth understanding whether > there > > are issues that need to be fixed and leaving comments where possible. > > I wonder if we could avoid copy&pasted conversion code (here and also in > render_frame_impl.cc) by getting rid of ExplodedHttpBodyElement as a separate > type (and making it a type alias for ResourceRequestBody::Element instead). > I've tried putting together a quick (and very dirty for now - some things still > don't compile) CL that does this at https://crrev.com/1987053002. So I did a bit of code archeology, and the time parameter there was introduced in https://codereview.chromium.org/594036/ in 2010. Based on the comments in the code review, it seems it was meant as a validator when they introduced file ranges, and the time value for whole files was set to base::Time() there. I'd like to avoid making modifications to the HTTP post data code used outside of PlzNavigate, so if we want to modify this, I'd prefer to do it in a separate CL (and I'd like to keep this part matching the code in web_url_request_utils.cc). https://codereview.chromium.org/1907443006/diff/200001/content/browser/frame_... File content/browser/frame_host/frame_navigation_entry.cc (right): https://codereview.chromium.org/1907443006/diff/200001/content/browser/frame_... content/browser/frame_host/frame_navigation_entry.cc:9: #include "base/strings/utf_string_conversions.h" On 2016/05/16 21:16:42, Charlie Reis (slow to reply) wrote: > Is this stale? Yes. Removed it.
Patchset #13 (id:240001) has been deleted
Thanks! LGTM with one change in navigation_request.cc. https://codereview.chromium.org/1907443006/diff/180001/content/common/resourc... File content/common/resource_request_body.cc (right): https://codereview.chromium.org/1907443006/diff/180001/content/common/resourc... content/common/resource_request_body.cc:39: base::Time::FromDoubleT(element.file_modification_time)); On 2016/05/19 13:11:57, clamy wrote: > On 2016/05/18 00:22:27, Łukasz Anforowicz wrote: > > On 2016/05/16 21:16:41, Charlie Reis (slow to reply) wrote: > > > On 2016/05/12 19:44:12, Łukasz Anforowicz wrote: > > > > On 2016/05/12 08:53:13, clamy wrote: > > > > > On 2016/05/12 02:53:59, Łukasz Anforowicz wrote: > > > > > > Just wanted to double-check - is it okay to use base::Time() above, > but > > > need > > > > > to > > > > > > use base::Time::FromDoubleT(element.file_modification_time) here? > i.e. > > > the > > > > > > expected file_modification_time is only applicable to file ranges, but > > > never > > > > > > applicable to whole files? If so, then it would be great if this was > > > > > explained > > > > > > in a comment somewhere in WebHTTPBody. (although it seems that Blink's > > > > > > historical style is to have as little comments as possible... :-( ). > > > > > > > > > > Again I imagine it's ok since we've been doing it. Yes WebKit style > guide > > on > > > > the > > > > > comment part is questionnable, though this is based on code coming from > > > > content/ > > > > > so it could have come with a little more comments... > > > > > > > > Acknowledged. > > > > > > In general, copy/pasting code isn't an excuse for ignoring issues with it > (in > > > terms of bugs or missing comments). :) It's even more important in those > > > cases, since a bug fix in the original location likely won't get applied > here > > as > > > well. > > > > > > Given how these various conversion functions are scattered around the code > > > without references to each other, I think it's worth understanding whether > > there > > > are issues that need to be fixed and leaving comments where possible. > > > > I wonder if we could avoid copy&pasted conversion code (here and also in > > render_frame_impl.cc) by getting rid of ExplodedHttpBodyElement as a separate > > type (and making it a type alias for ResourceRequestBody::Element instead). > > I've tried putting together a quick (and very dirty for now - some things > still > > don't compile) CL that does this at https://crrev.com/1987053002. > > So I did a bit of code archeology, and the time parameter there was introduced > in https://codereview.chromium.org/594036/ in 2010. Based on the comments in the > code review, it seems it was meant as a validator when they introduced file > ranges, and the time value for whole files was set to base::Time() there. > > I'd like to avoid making modifications to the HTTP post data code used outside > of PlzNavigate, so if we want to modify this, I'd prefer to do it in a separate > CL (and I'd like to keep this part matching the code in > web_url_request_utils.cc). Agreed. Since we aren't certain there's a bug here, let's leave any further investigation until later. Hopefully Lukasz will be able to eliminate ExplodedHttpBodyElement in his CL. https://codereview.chromium.org/1907443006/diff/260001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1907443006/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:266: if (common_params_.method == "POST" && redirect_info.new_method != "POST") Looks like you might have missed my earlier comment here: I think we should remove the first part (common_params_.method == "POST"). It's not likely that post_data_ will exist when common_params_.method != "POST", but if it did, it would be bad to preserve it after a redirect to a GET.
carlosk@chromium.org changed reviewers: + carlosk@chromium.org
Thanks for working on this. I had a few drafts here from a while ago that were already covered. Only a nit remains. https://codereview.chromium.org/1907443006/diff/260001/content/browser/frame_... File content/browser/frame_host/frame_navigation_entry.cc (right): https://codereview.chromium.org/1907443006/diff/260001/content/browser/frame_... content/browser/frame_host/frame_navigation_entry.cc:90: return body; nit: it seems clearer to return nullptr here and below and only create the scoped_refptr in line 97.
Description was changed from ========== PlzNavigate: store POST data in the FrameNavigationEntry This allows to properly resend it during hitsory navigations when browser-side-navigation is enabled. A rational for the design can be found in the following doc: https://docs.google.com/a/chromium.org/document/d/1BtrZjP-lbeza2OdRwr-68poSgG... BUG=575210, 608372 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: store POST data in the FrameNavigationEntry This allows to properly resend it during hitsory navigations when browser-side-navigation is enabled. A rational for the design can be found in the following doc: https://docs.google.com/a/chromium.org/document/d/1BtrZjP-lbeza2OdRwr-68poSgG... BUG=575210, 608372 TBR=nasko@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Thanks! I'm TBRing nasko for frame_messages since he is OOO this week. https://codereview.chromium.org/1907443006/diff/260001/content/browser/frame_... File content/browser/frame_host/frame_navigation_entry.cc (right): https://codereview.chromium.org/1907443006/diff/260001/content/browser/frame_... content/browser/frame_host/frame_navigation_entry.cc:90: return body; On 2016/05/20 08:59:47, carlosk wrote: > nit: it seems clearer to return nullptr here and below and only create the > scoped_refptr in line 97. Done. https://codereview.chromium.org/1907443006/diff/260001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1907443006/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:266: if (common_params_.method == "POST" && redirect_info.new_method != "POST") On 2016/05/19 17:43:48, Charlie Reis (slow to reply) wrote: > Looks like you might have missed my earlier comment here: > > I think we should remove the first part (common_params_.method == "POST"). It's > not likely that post_data_ will exist when common_params_.method != "POST", but > if it did, it would be bad to preserve it after a redirect to a GET. Done.
The CQ bit was checked by clamy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lukasza@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/1907443006/#ps280001 (title: "Rebase + addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1907443006/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1907443006/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by clamy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1907443006/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1907443006/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by clamy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1907443006/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1907443006/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lukasza@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1907443006/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1907443006/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lukasza@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1907443006/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1907443006/280001
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: store POST data in the FrameNavigationEntry This allows to properly resend it during hitsory navigations when browser-side-navigation is enabled. A rational for the design can be found in the following doc: https://docs.google.com/a/chromium.org/document/d/1BtrZjP-lbeza2OdRwr-68poSgG... BUG=575210, 608372 TBR=nasko@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: store POST data in the FrameNavigationEntry This allows to properly resend it during hitsory navigations when browser-side-navigation is enabled. A rational for the design can be found in the following doc: https://docs.google.com/a/chromium.org/document/d/1BtrZjP-lbeza2OdRwr-68poSgG... BUG=575210, 608372 TBR=nasko@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #14 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: store POST data in the FrameNavigationEntry This allows to properly resend it during hitsory navigations when browser-side-navigation is enabled. A rational for the design can be found in the following doc: https://docs.google.com/a/chromium.org/document/d/1BtrZjP-lbeza2OdRwr-68poSgG... BUG=575210, 608372 TBR=nasko@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: store POST data in the FrameNavigationEntry This allows to properly resend it during hitsory navigations when browser-side-navigation is enabled. A rational for the design can be found in the following doc: https://docs.google.com/a/chromium.org/document/d/1BtrZjP-lbeza2OdRwr-68poSgG... BUG=575210, 608372 TBR=nasko@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/34754b494e8f9706aa48463dcd1f17026ad658a2 Cr-Commit-Position: refs/heads/master@{#395131} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/34754b494e8f9706aa48463dcd1f17026ad658a2 Cr-Commit-Position: refs/heads/master@{#395131}
Message was sent while issue was closed.
On 2016/05/20 at 11:00:12, clamy wrote: > Thanks! I'm TBRing nasko for frame_messages since he is OOO this week. I just saw this while going through my backlog, but please don't TBR IPC changes. > > https://codereview.chromium.org/1907443006/diff/260001/content/browser/frame_... > File content/browser/frame_host/frame_navigation_entry.cc (right): > > https://codereview.chromium.org/1907443006/diff/260001/content/browser/frame_... > content/browser/frame_host/frame_navigation_entry.cc:90: return body; > On 2016/05/20 08:59:47, carlosk wrote: > > nit: it seems clearer to return nullptr here and below and only create the > > scoped_refptr in line 97. > > Done. > > https://codereview.chromium.org/1907443006/diff/260001/content/browser/frame_... > File content/browser/frame_host/navigation_request.cc (right): > > https://codereview.chromium.org/1907443006/diff/260001/content/browser/frame_... > content/browser/frame_host/navigation_request.cc:266: if (common_params_.method == "POST" && redirect_info.new_method != "POST") > On 2016/05/19 17:43:48, Charlie Reis (slow to reply) wrote: > > Looks like you might have missed my earlier comment here: > > > > I think we should remove the first part (common_params_.method == "POST"). It's > > not likely that post_data_ will exist when common_params_.method != "POST", but > > if it did, it would be bad to preserve it after a redirect to a GET. > > Done. |
