Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1693183002:
Move multipart resource handling to core/fetch (1/2) (Closed)
|
Created:
4 years, 10 months ago by yhirano Modified:
4 years, 9 months ago CC:
blink-reviews, chromium-reviews, gavinp+loader_chromium.org, Nate Chapin, loading-reviews+fetch_chromium.org, tyoshino+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@multipart-cleanup-preliminary Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove multipart resource handling to core/fetch (1/2)
Currently a multipart/x-mixed-replace response is parsed in the
content layer and dispatched to clients in blink. It is problematic
because the parser calls callbacks in a strange way and special
handling code scatters from core/html to content/child.
This change adds MultipartImageResourceParser in core/fetch. It
is basically copied from multipart_response_delegate.
BUG=570608
Committed: https://crrev.com/79aba5a54291da8765270317b5e79d643d96cec6
Cr-Commit-Position: refs/heads/master@{#380431}
Patch Set 1 : #Patch Set 2 : #
Total comments: 15
Patch Set 3 : #
Total comments: 23
Patch Set 4 : rebase #Patch Set 5 : #
Total comments: 2
Patch Set 6 : #Patch Set 7 : rebase #
Total comments: 10
Patch Set 8 : #
Total comments: 25
Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #Patch Set 14 : #Dependent Patchsets: Messages
Total messages: 56 (22 generated)
Description was changed from ========== wip BUG= ========== to ========== Move multipart resource handling to core/fetch (1/2) Currently a multipart/x-mixed-replace response is parsed in the content layer and dispatched to clients in blink. It is problematic because the parser calls callbacks in a strange way and special handling code scatters from core/html to content/child. This change adds MultipartImageResourceParser in core/fetch. It is basically copied from multipart_response_delegate. BUG=570608 ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #2 (id:140001) has been deleted
Patchset #1 (id:120001) has been deleted
yhirano@chromium.org changed reviewers: + hiroshige@chromium.org, japhet@chromium.org
Hi, can you take a look? Though the latter part (https://codereview.chromium.org/1710733002/) is still WIP, I think this CL is ready to review.
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1693183002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1693183002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1693183002/diff/180001/content/child/web_url_... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1693183002/diff/180001/content/child/web_url_... content/child/web_url_loader_impl.cc:1174: bool WebURLLoaderImpl::ParseAdditionalHeaders( What is the reason to place ParseAdditionalHeaders() here under content//, while moving the other parts to Blink? https://codereview.chromium.org/1693183002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp (right): https://codereview.chromium.org/1693183002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:28: size_t find(const char* s, size_t sSize, const char* pattern, size_t patternSize) I think it's better to use std::search() rather than re-implementing here. https://codereview.chromium.org/1693183002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:78: const auto& boundary = m_boundary; nit: Why don't we do |m_data.prepend(m_boundary);|? https://codereview.chromium.org/1693183002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:99: if (isCancelled()) Should we place isCancelled() check here? In the case of isCancelled() in Line 117, it seems to check whether this is cancelled inside didReceiveData() in Line 116. In the case of here Line 99, there doesn't seem such things that lead isCancelled() to be true. If isCancelled() is true here, is it also true from the beginning of this method? https://codereview.chromium.org/1693183002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:156: // it to WebCore. nit: s/send it to WebCore/send it to Client/ (or something)? https://codereview.chromium.org/1693183002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.h (right): https://codereview.chromium.org/1693183002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.h:76: static size_t findBoundary(const Vector<char>& data, Vector<char>* boundary); nit: it might be better to add a comment that mentions this function updates |*boundary|, because the function name |findBoundary| doesn't looks like destructive.
https://codereview.chromium.org/1693183002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp (right): https://codereview.chromium.org/1693183002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:99: if (isCancelled()) On 2016/02/25 18:10:24, hiroshige wrote: > Should we place isCancelled() check here? > In the case of isCancelled() in Line 117, it seems to check whether this is > cancelled inside didReceiveData() in Line 116. > > In the case of here Line 99, there doesn't seem such things that lead > isCancelled() to be true. If isCancelled() is true here, is it also true from > the beginning of this method? Er, I understood, parseHeaders() calls didReceiveResponse(). Then, I think we should rename parseHeaders(), e.g. to parseHeadersAndNotifyClient(), because parseHeaders() looks like it just parse headers.
https://codereview.chromium.org/1693183002/diff/180001/content/child/web_url_... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1693183002/diff/180001/content/child/web_url_... content/child/web_url_loader_impl.cc:1174: bool WebURLLoaderImpl::ParseAdditionalHeaders( On 2016/02/25 18:10:24, hiroshige wrote: > What is the reason to place ParseAdditionalHeaders() here under content//, while > moving the other parts to Blink? Because I want to use net:: functions. https://codereview.chromium.org/1693183002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp (right): https://codereview.chromium.org/1693183002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:28: size_t find(const char* s, size_t sSize, const char* pattern, size_t patternSize) On 2016/02/25 18:10:24, hiroshige wrote: > I think it's better to use std::search() rather than re-implementing here. Oh, I didn't know the function. Thank you! https://codereview.chromium.org/1693183002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:78: const auto& boundary = m_boundary; On 2016/02/25 18:10:24, hiroshige wrote: > nit: Why don't we do |m_data.prepend(m_boundary);|? It causes a strange compile error. https://codereview.chromium.org/1693183002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:99: if (isCancelled()) On 2016/02/25 18:31:05, hiroshige wrote: > On 2016/02/25 18:10:24, hiroshige wrote: > > Should we place isCancelled() check here? > > In the case of isCancelled() in Line 117, it seems to check whether this is > > cancelled inside didReceiveData() in Line 116. > > > > In the case of here Line 99, there doesn't seem such things that lead > > isCancelled() to be true. If isCancelled() is true here, is it also true from > > the beginning of this method? > > Er, I understood, parseHeaders() calls didReceiveResponse(). > Then, I think we should rename parseHeaders(), e.g. to > parseHeadersAndNotifyClient(), because parseHeaders() looks like it just parse > headers. This "parser" class parses incoming data and notifies the client, so I think it is consistent that a parsing function notifies the client. https://codereview.chromium.org/1693183002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:156: // it to WebCore. On 2016/02/25 18:10:24, hiroshige wrote: > nit: s/send it to WebCore/send it to Client/ (or something)? Done. https://codereview.chromium.org/1693183002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.h (right): https://codereview.chromium.org/1693183002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.h:76: static size_t findBoundary(const Vector<char>& data, Vector<char>* boundary); On 2016/02/25 18:10:24, hiroshige wrote: > nit: it might be better to add a comment that mentions this function updates > |*boundary|, because the function name |findBoundary| doesn't looks like > destructive. Done.
https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.h (right): https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.h:58: virtual void didReceiveData(const char* bytes, size_t) = 0; How about renaming these to: responseReceivedMultipart() appendDataMultipart() because ThreadableLoaderClient has the same names. ThreadableLoaderClient::didReceiveResponse() is for all response, while Resource::didReceiveResponse() here is only for multipart responses and doesn't correspond to ThreadableLoaderClient::didReceiveResponse(), which is confusing for me. Also, these methods are more related to Resource::appendData() and Resource::responseReceived(), not ThreadableLoaderClient::didReceiveData() and ThreadableLoaderClient::didReceiveResponse(). https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.h:63: void addData(const char* bytes, size_t); How about also renaming addData() to appendData(), because this is related to Resource::appendData().
This CL looks basically good. Reviewed tests (mostly nits). https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/MultipartImageResourceParserTest.cpp (right): https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParserTest.cpp:44: Vector<Vector<char>> m_data; nit: These tests do not test how many times didReceiveData() is called, while the original tests do check by |client.received_data_|. Particularly, these tests do not test that didReceiveData() is not called when the body is empty. Is this OK? (I feel this might be OK, just want to double check whether this change is OK/intentional) https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParserTest.cpp:48: // access to private members. nit: We can remove this comment as we no longer have |friend| declaration for tests. https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParserTest.cpp:186: boundary.append("bound", 5); Perhaps this should be "--bound"? (Line 289 of content/child/multipart_response_delegate_unittest.cc) https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParserTest.cpp:214: void VariousChunkSizesTest(const TestChunk chunks[], int chunks_size, nit: |variousChunkSizesTest()|, |chunksSize|, |receivedData| according to Blink name style? https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParserTest.cpp:236: ASSERT_TRUE(chunks[i].startPosition < chunks[i].endPosition); nit: ASSERT_LE() can be used. https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParserTest.cpp:248: void VariousChunkSizesTest(const TestChunk (&chunks)[N], size_t responses, int received_data, const char* completedData) ditto > Line 214.
https://codereview.chromium.org/1693183002/diff/180001/content/child/web_url_... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1693183002/diff/180001/content/child/web_url_... content/child/web_url_loader_impl.cc:1174: bool WebURLLoaderImpl::ParseAdditionalHeaders( On 2016/02/25 18:35:13, yhirano wrote: > On 2016/02/25 18:10:24, hiroshige wrote: > > What is the reason to place ParseAdditionalHeaders() here under content//, > while > > moving the other parts to Blink? > > Because I want to use net:: functions. Hmm. The code around this has some issues: |kReplaceHeaders| is duplicated in content// and Blink, ResourceResponse is converted into WebURLResponse and then back to ResourceResponse. But currently I haven't come up with better implementation, and I don't want to this issue to block this CL (having MultipartImageResourceParser in Blink will result in greater benefits than issues here). How about leaving TODO comments that mentions why MultipartImageResourceParser::parseHeaders() WebURLLoaderImpl::ParseAdditionalHeaders() are separated and that they should be clean up? https://codereview.chromium.org/1693183002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp (right): https://codereview.chromium.org/1693183002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:78: const auto& boundary = m_boundary; On 2016/02/25 18:35:13, yhirano wrote: > On 2016/02/25 18:10:24, hiroshige wrote: > > nit: Why don't we do |m_data.prepend(m_boundary);|? > > It causes a strange compile error. I see. Probably this is WTF::Vector's problem.
Looks pretty good! Just a couple of nitpicks. https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp (right): https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:21: const char* kReplaceHeaders[] = { It seems bad to have this array both here and web_url_loader_impl.cc. Can we avoid that? https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:51: if (m_isParsingTop) { This was call first_received_data_ before. That name seems more clear to me than m_isParsingTop. Am I missing something? https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/Platform.h:323: virtual bool parseAdditionalHeaders(const char* bytes, size_t /* size */, WebURLResponse*, size_t* end) const { return false; } Perhaps this should be named a bit more specifically? parseMultipartHeadersFromBody?
Patchset #5 (id:240001) has been deleted
https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp (right): https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:21: const char* kReplaceHeaders[] = { On 2016/02/25 21:48:29, Nate Chapin wrote: > It seems bad to have this array both here and web_url_loader_impl.cc. Can we > avoid that? Done. https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:51: if (m_isParsingTop) { On 2016/02/25 21:48:29, Nate Chapin wrote: > This was call first_received_data_ before. That name seems more clear to me than > m_isParsingTop. Am I missing something? I wondered what "first" means: it may be true in the second addData call. Is it good to rename it to isParsingFirstBoundary? https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.h (right): https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.h:58: virtual void didReceiveData(const char* bytes, size_t) = 0; On 2016/02/25 18:39:08, hiroshige wrote: > How about renaming these to: > responseReceivedMultipart() > appendDataMultipart() > because ThreadableLoaderClient has the same names. > > ThreadableLoaderClient::didReceiveResponse() is for all response, while > Resource::didReceiveResponse() here is only for multipart responses and doesn't > correspond to ThreadableLoaderClient::didReceiveResponse(), which is confusing > for me. > > Also, these methods are more related to Resource::appendData() and > Resource::responseReceived(), not ThreadableLoaderClient::didReceiveData() and > ThreadableLoaderClient::didReceiveResponse(). I think appendData is not appropriate for a "client" class. How about onePartInMultipartReceived / multipartDataReceived? https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.h:63: void addData(const char* bytes, size_t); On 2016/02/25 18:39:08, hiroshige wrote: > How about also renaming addData() to appendData(), because this is related to > Resource::appendData(). Done. https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/MultipartImageResourceParserTest.cpp (right): https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParserTest.cpp:44: Vector<Vector<char>> m_data; On 2016/02/25 21:36:27, hiroshige wrote: > nit: These tests do not test how many times didReceiveData() is called, while > the original tests do check by |client.received_data_|. > Particularly, these tests do not test that didReceiveData() is not called when > the body is empty. > Is this OK? (I feel this might be OK, just want to double check whether this > change is OK/intentional) I think it's OK because ImageResource doesn't update the image while receiving data. https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParserTest.cpp:48: // access to private members. On 2016/02/25 21:36:27, hiroshige wrote: > nit: We can remove this comment as we no longer have |friend| declaration for > tests. Done. https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParserTest.cpp:186: boundary.append("bound", 5); On 2016/02/25 21:36:27, hiroshige wrote: > Perhaps this should be "--bound"? (Line 289 of > content/child/multipart_response_delegate_unittest.cc) Done. https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParserTest.cpp:214: void VariousChunkSizesTest(const TestChunk chunks[], int chunks_size, On 2016/02/25 21:36:27, hiroshige wrote: > nit: |variousChunkSizesTest()|, |chunksSize|, |receivedData| according to Blink > name style? Done. https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParserTest.cpp:236: ASSERT_TRUE(chunks[i].startPosition < chunks[i].endPosition); On 2016/02/25 21:36:27, hiroshige wrote: > nit: ASSERT_LE() can be used. Done. https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParserTest.cpp:248: void VariousChunkSizesTest(const TestChunk (&chunks)[N], size_t responses, int received_data, const char* completedData) On 2016/02/25 21:36:27, hiroshige wrote: > ditto > Line 214. Done. https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/Platform.h:323: virtual bool parseAdditionalHeaders(const char* bytes, size_t /* size */, WebURLResponse*, size_t* end) const { return false; } On 2016/02/25 21:48:29, Nate Chapin wrote: > Perhaps this should be named a bit more specifically? > parseMultipartHeadersFromBody? Done.
I added tests corresponding to ParseHeaders to PS6.
https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/MultipartImageResourceParserTest.cpp (right): https://codereview.chromium.org/1693183002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParserTest.cpp:44: Vector<Vector<char>> m_data; On 2016/02/26 22:21:51, yhirano wrote: > On 2016/02/25 21:36:27, hiroshige wrote: > > nit: These tests do not test how many times didReceiveData() is called, while > > the original tests do check by |client.received_data_|. > > Particularly, these tests do not test that didReceiveData() is not called when > > the body is empty. > > Is this OK? (I feel this might be OK, just want to double check whether this > > change is OK/intentional) > > I think it's OK because ImageResource doesn't update the image while receiving > data. Acknowledged. https://codereview.chromium.org/1693183002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.h (right): https://codereview.chromium.org/1693183002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.h:57: virtual void onePartInMultipartReceived(const ResourceResponse&) = 0; optional: I prefer including "responseReceived" in name to show the override of this by Resource is parallel to Resource::responseReceived(). https://codereview.chromium.org/1693183002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.h:58: virtual void multipartDataReceived(const char* bytes, size_t) = 0; optional: I prefer including "appendData" in name, because the data flows like: Resource::appendData() -> MultipartImageResourceParser::appendData() -> Resource::appendDataMultipart() -> Resource::appendDataInternal() and I feel this is more consistent than: Resource::appendData() -> MultipartImageResourceParser::appendData() -> Resource::multipartDataReceived() -> Resource::appendDataInternal()
blink lgtm.
yhirano@chromium.org changed reviewers: + jochen@chromium.org, torne@chromium.org
+jochen for content/. +torne in case you have a copyright related concern (I ported content/child/multipart_response_delegate.h having MPL to core/fetch/MultipartImageResourceParser.h).
On 2016/03/02 22:39:58, yhirano wrote: > +jochen for content/. > +torne in case you have a copyright related concern (I ported > content/child/multipart_response_delegate.h having MPL to > core/fetch/MultipartImageResourceParser.h). I'm not a copyright lawyer and can't tell you if this is okay. I just know how to debug issues with the copyright scanning script :)
can you please also ping the security team about the fact that you add a new parser here? https://codereview.chromium.org/1693183002/diff/300001/content/child/blink_pl... File content/child/blink_platform_impl.h (right): https://codereview.chromium.org/1693183002/diff/300001/content/child/blink_pl... content/child/blink_platform_impl.h:100: size_t, please add argument names in content/ https://codereview.chromium.org/1693183002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.h (right): https://codereview.chromium.org/1693183002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.h:11: * http://www.mozilla.org/MPL/ can you please ping open-source-third-party-reviews@google.com about this CL? I'm kinda surprised that the header has the mozilla copyright while it looks like an oilpan thing, but the cpp file does not? https://codereview.chromium.org/1693183002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.h:53: public: add WTF_NON_COPYABLE() https://codereview.chromium.org/1693183002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.h:56: virtual ~Client() {} = default; https://codereview.chromium.org/1693183002/diff/300001/third_party/WebKit/pub... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/1693183002/diff/300001/third_party/WebKit/pub... third_party/WebKit/public/platform/Platform.h:327: virtual bool parseMultipartHeadersFromBody(const char* bytes, size_t /* size */, WebURLResponse*, size_t* end) const { return false; } should this be a WebVector (bytes)?
yhirano@chromium.org changed reviewers: + tsepez@chromium.org
> can you please also ping the security team about the fact that you add a new parser here? +tsepez@ I add MultipartImageResourceParser based on multipart_resource_delegate. I will remove the latter in the subsequent CL (https://codereview.chromium.org/1710733002/).
non-owner lgtm.
https://codereview.chromium.org/1693183002/diff/300001/content/child/blink_pl... File content/child/blink_platform_impl.h (right): https://codereview.chromium.org/1693183002/diff/300001/content/child/blink_pl... content/child/blink_platform_impl.h:100: size_t, On 2016/03/04 12:30:34, jochen wrote: > please add argument names in content/ Done. https://codereview.chromium.org/1693183002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.h (right): https://codereview.chromium.org/1693183002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.h:11: * http://www.mozilla.org/MPL/ On 2016/03/04 12:30:34, jochen wrote: > can you please ping mailto:open-source-third-party-reviews@google.com about this CL? > > I'm kinda surprised that the header has the mozilla copyright while it looks > like an oilpan thing, but the cpp file does not? Done. The reason why the header has MPL but the cpp doesn't is multipart_response_delegate.{h, cc} do the same thing. https://codereview.chromium.org/1693183002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.h:53: public: On 2016/03/04 12:30:34, jochen wrote: > add WTF_NON_COPYABLE() Done. https://codereview.chromium.org/1693183002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.h:56: virtual ~Client() {} On 2016/03/04 12:30:34, jochen wrote: > = default; Done. https://codereview.chromium.org/1693183002/diff/300001/third_party/WebKit/pub... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/1693183002/diff/300001/third_party/WebKit/pub... third_party/WebKit/public/platform/Platform.h:327: virtual bool parseMultipartHeadersFromBody(const char* bytes, size_t /* size */, WebURLResponse*, size_t* end) const { return false; } On 2016/03/04 12:30:34, jochen wrote: > should this be a WebVector (bytes)? Making (const char*, size_t) a WebVector needs memory copy which I would like to avoid.
https://codereview.chromium.org/1693183002/diff/320001/content/child/web_url_... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1693183002/diff/320001/content/child/web_url_... content/child/web_url_loader_impl.cc:88: const char* kReplaceHeaders[] = { nit: const char* const https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp (right): https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:37: int pos = pushOverLine(m_data, 0); pos must be a size_t, too. https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:51: const auto& boundary = m_boundary; nit: why this local? https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:56: ASSERT(!m_isParsingTop); Nit: Seems pointless to assert given the assignment at 54. https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:65: if (parseHeaders()) { nit: lets invert this so you write if (!parseHeaders) return; Then you can avoid an else entirely. https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:75: ASSERT(!m_isParsingHeaders); ditto https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:80: // boundary on the same lines as Firefox. nit: as does Firefox. Otherwise it sounds like the string "Firefox" might be in the buffer. https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:88: if (dataSize > 0) { nit: size_t's are unsigned, so maybe just if (dataSize) https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:102: int offset = pushOverLine(m_data, boundaryEndPosition); again, size_t. https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:138: int offset = 0; make this a size_t, and return a size_t. https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:140: ++offset; Wouldn't this strip \n\n in addition to \r\n and \n. Is that what you wanted? https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:185: if (boundaryPosition >= 2) { nit: Our style is generally to turn these around, as in data[bondaryPosition -1] == '-' since gcc gives a warning if you mistakenly use assignment nowdays.
Patchset #9 (id:340001) has been deleted
Patchset #9 (id:360001) has been deleted
https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp (right): https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:51: const auto& boundary = m_boundary; On 2016/03/04 19:07:32, Tom Sepez wrote: > nit: why this local? Probably due to template instantiation problem in WTF::Vector. See comments in Line 78 in https://codereview.chromium.org/1693183002/diff/180001/third_party/WebKit/Sou... yhirano@, can we create a crbug entry and add a comment here with link to crbug?
https://codereview.chromium.org/1693183002/diff/320001/content/child/web_url_... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1693183002/diff/320001/content/child/web_url_... content/child/web_url_loader_impl.cc:88: const char* kReplaceHeaders[] = { On 2016/03/04 19:07:31, Tom Sepez wrote: > nit: const char* const Done. https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp (right): https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:37: int pos = pushOverLine(m_data, 0); On 2016/03/04 19:07:31, Tom Sepez wrote: > pos must be a size_t, too. Done. https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:51: const auto& boundary = m_boundary; On 2016/03/04 19:57:11, hiroshige wrote: > On 2016/03/04 19:07:32, Tom Sepez wrote: > > nit: why this local? > > Probably due to template instantiation problem in WTF::Vector. See comments in > Line 78 in > https://codereview.chromium.org/1693183002/diff/180001/third_party/WebKit/Sou... > > yhirano@, can we create a crbug entry and add a comment here with link to crbug? Done. https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:56: ASSERT(!m_isParsingTop); On 2016/03/04 19:07:32, Tom Sepez wrote: > Nit: Seems pointless to assert given the assignment at 54. Done. https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:65: if (parseHeaders()) { On 2016/03/04 19:07:31, Tom Sepez wrote: > nit: lets invert this so you write > if (!parseHeaders) > return; > > Then you can avoid an else entirely. Done. https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:75: ASSERT(!m_isParsingHeaders); On 2016/03/04 19:07:31, Tom Sepez wrote: > ditto Done. https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:80: // boundary on the same lines as Firefox. On 2016/03/04 19:07:31, Tom Sepez wrote: > nit: as does Firefox. > Otherwise it sounds like the string "Firefox" might be in the buffer. Done. https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:88: if (dataSize > 0) { On 2016/03/04 19:07:32, Tom Sepez wrote: > nit: size_t's are unsigned, so maybe just > if (dataSize) Done. https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:102: int offset = pushOverLine(m_data, boundaryEndPosition); On 2016/03/04 19:07:32, Tom Sepez wrote: > again, size_t. Done. https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:138: int offset = 0; On 2016/03/04 19:07:31, Tom Sepez wrote: > make this a size_t, and return a size_t. Done. https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:140: ++offset; On 2016/03/04 19:07:31, Tom Sepez wrote: > Wouldn't this strip \n\n in addition to \r\n and \n. Is that what you wanted? The logic has some problems but I would like to fix them later: the problems exist in the current multipart_response_delegate.cc. https://codereview.chromium.org/1693183002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp:185: if (boundaryPosition >= 2) { On 2016/03/04 19:07:31, Tom Sepez wrote: > nit: Our style is generally to turn these around, as in > > data[bondaryPosition -1] == '-' > > since gcc gives a warning if you mistakenly use assignment nowdays. Done.
lgtm
ok, I didn't know that this is a copy of an existing file. lgtm then
btw, was it possible to just use a WebVector<> internally? Passing around raw ptrs + size really isn't nice.
On 2016/03/07 12:00:51, jochen wrote: > btw, was it possible to just use a WebVector<> internally? Passing around raw > ptrs + size really isn't nice. I don't think we can use WebVector in the parser because WebVector lacks a bunch of operations such as append, prepend, remove.
jochen@chromium.org changed reviewers: + esprehn@chromium.org
On 2016/03/07 at 17:56:07, yhirano wrote: > On 2016/03/07 12:00:51, jochen wrote: > > btw, was it possible to just use a WebVector<> internally? Passing around raw > > ptrs + size really isn't nice. > > I don't think we can use WebVector in the parser because WebVector lacks a bunch of operations such as append, prepend, remove. +esprehn - it's too bad that we have to pass raw ptrs + size_t across the API
On 2016/03/08 15:45:07, jochen wrote: > On 2016/03/07 at 17:56:07, yhirano wrote: > > On 2016/03/07 12:00:51, jochen wrote: > > > btw, was it possible to just use a WebVector<> internally? Passing around > raw > > > ptrs + size really isn't nice. > > > > I don't think we can use WebVector in the parser because WebVector lacks a > bunch of operations such as append, prepend, remove. > > +esprehn - it's too bad that we have to pass raw ptrs + size_t across the API Jochen, are you OK with landing this change for now and modifying the API if/when WebVector gets usable for the purpose?
yes
Thanks!
The CQ bit was checked by yhirano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hiroshige@chromium.org, japhet@chromium.org, tsepez@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1693183002/#ps480001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1693183002/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1693183002/480001
Message was sent while issue was closed.
Description was changed from ========== Move multipart resource handling to core/fetch (1/2) Currently a multipart/x-mixed-replace response is parsed in the content layer and dispatched to clients in blink. It is problematic because the parser calls callbacks in a strange way and special handling code scatters from core/html to content/child. This change adds MultipartImageResourceParser in core/fetch. It is basically copied from multipart_response_delegate. BUG=570608 ========== to ========== Move multipart resource handling to core/fetch (1/2) Currently a multipart/x-mixed-replace response is parsed in the content layer and dispatched to clients in blink. It is problematic because the parser calls callbacks in a strange way and special handling code scatters from core/html to content/child. This change adds MultipartImageResourceParser in core/fetch. It is basically copied from multipart_response_delegate. BUG=570608 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:480001)
Message was sent while issue was closed.
Description was changed from ========== Move multipart resource handling to core/fetch (1/2) Currently a multipart/x-mixed-replace response is parsed in the content layer and dispatched to clients in blink. It is problematic because the parser calls callbacks in a strange way and special handling code scatters from core/html to content/child. This change adds MultipartImageResourceParser in core/fetch. It is basically copied from multipart_response_delegate. BUG=570608 ========== to ========== Move multipart resource handling to core/fetch (1/2) Currently a multipart/x-mixed-replace response is parsed in the content layer and dispatched to clients in blink. It is problematic because the parser calls callbacks in a strange way and special handling code scatters from core/html to content/child. This change adds MultipartImageResourceParser in core/fetch. It is basically copied from multipart_response_delegate. BUG=570608 Committed: https://crrev.com/79aba5a54291da8765270317b5e79d643d96cec6 Cr-Commit-Position: refs/heads/master@{#380431} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/79aba5a54291da8765270317b5e79d643d96cec6 Cr-Commit-Position: refs/heads/master@{#380431} |
