~InspectorPostBodyParser should base64 encode before invoking the RequestPostData callback. |
||||
Issue description
,
Nov 19
Yes, I see that there is a mismatch between the comment in browser_protocol.pdl's getRequestPostData and what it actually does. I could base64 encode and then decode again in the frontend where it is expected to not be base64 encoded, but I wonder if that would make the performance concerns worse (http://crbug.com/869840) or if there are any other applications out there that are expecting the getRequestPostData protocol message to not be base64 encoded. Perhaps we could add an optional getRequestPostData boolean request parameter to enable base64 encoding if needed? +dgozman@ what do you think about these concerns?
,
Nov 19
,
Nov 19
If we can pass post data without encoding, we should do that. When the data is binary, we should optionally encode and pass "base64encoded" bit.
,
Nov 19
When there is a lot of data, how likely is it that you won't need the encoding? Or perhaps: When you don't need the encoding, could it be that the data is usually small enough so that encoding it anyway won't make much of a difference? === I'm asking because not having a base64encoded bit is slightly simpler since in that case the 'binary' data type as implemented in the protocol can handle it (it always base64encodes / decodes).
,
Nov 20
After talking with johannes@ and caseq@, it sounds like the getRequestPostData protocol method does not represent multipart post data very well since it just puts everything in a string with unknown encoding and that it would be better to respond with separate parts each with a mime type, which is what a multipart post actually does. eostroukhov@ made a bug a while ago which is now assigned to me to do this very thing: http://crbug.com/810554 Upon further investigation, it turns out that the backend doesn't include files at all when responding to the getRequestPostData method: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/inspector/inspector_network_agent.cc?rcl=523cf50a9d77e4745b52e06d6c8c7ffa44fe3f69&l=249 For now I will just remove the comment about base64 in the protocol definition and say that it does not include any files. If anyone believes there is a need for a new protocol method for multipart post data, I'd be happy to implement it.
,
Nov 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8f4def77830e423c48b55a20f87c93f28f16c141 commit 8f4def77830e423c48b55a20f87c93f28f16c141 Author: Joey Arhar <jarhar@chromium.org> Date: Sat Nov 24 21:22:09 2018 [DevTools] Correct comment about base64 in getRequestPostData method Bug: 900298 Change-Id: I63f1d78010094e0bd1a9c05244a9e1c61cee3970 Reviewed-on: https://chromium-review.googlesource.com/c/1343512 Commit-Queue: Joey Arhar <jarhar@chromium.org> Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Cr-Commit-Position: refs/heads/master@{#610711} [modify] https://crrev.com/8f4def77830e423c48b55a20f87c93f28f16c141/third_party/blink/renderer/core/inspector/browser_protocol.pdl
,
Nov 24
|
||||
►
Sign in to add a comment |
||||
Comment 1 by l...@chromium.org
, Oct 30Status: Assigned (was: Untriaged)