Summary: | SharedBufferChunkReader should be refactored | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jean-Yves Avenard [:jya] <jean-yves.avenard> | ||||||
Component: | Platform | Assignee: | Jean-Yves Avenard [:jya] <jean-yves.avenard> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | annulen, ews-watchlist, gyuyoung.kim, heycam, ryuan.choi, sergio, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | Other | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=170956 | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 233030 | ||||||||
Attachments: |
|
Description
Jean-Yves Avenard [:jya]
2021-11-24 14:00:08 PST
this task would allow for bug 233030 to be a tad smaller and is enough to do Created attachment 445164 [details]
Patch
Comment on attachment 445164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=445164&action=review > Source/WebCore/ChangeLog:10 > + SharedBufferChunkReader required the SharedBuffer to be flatten which > + would mutate the underlying SharedBuffer. "to be flattened" But I'd rewrite this to describe the change being made, e.g.: Avoid flattening a SharedBuffer when reading reading it through SharedBufferChunkReader. And maybe this can be the top line of the ChangeLog in place of "Rewrite SharedBufferChunkReader", which is a bit non-specific. > Source/WebCore/ChangeLog:11 > + the peek method was also incorrect if the data read overlapped multiple Similarly change this to describe the change being made: Fix a bug in the peek method where too many characters would be read if the requested size overlaps multiple segments. > Source/WebCore/ChangeLog:17 > + We add it to makefiles and export the header so that API tests can be written. (Probably doesn't need to be mentioned.) > Source/WebCore/platform/SharedBuffer.cpp:307 > + size_t segmentLength = std::min(currentSegment->segment->size(), remaining); > + data.append(currentSegment->segment->data(), segmentLength); "segmentLength" sounds like the length of the segment, but I don't immediately have a better suggestion. > Source/WebCore/platform/SharedBufferChunkReader.cpp:78 > auto currentCharacter = m_segment[m_segmentIndex++]; > if (currentCharacter != m_separator[m_separatorIndex]) { > if (m_separatorIndex > 0) { The existing code to check for separators doesn't work correctly with arbitrary separator strings. Not sure if it matters. But for example if the buffer contains "aaab" and the separator string is "aab", then it won't detect it as a separator, since at m_segmentIndex = 2, it'll detect a mismatch, and reset m_segmentIndex to 0. Leave a FIXME in here? > Source/WebCore/platform/SharedBufferChunkReader.cpp:98 > + if (++m_iteratorCurrent == m_iteratorEnd) { > m_reachedEndOfFile = true; Maybe set m_segment to nullptr just to be clean in here. > Source/WebCore/platform/SharedBufferChunkReader.cpp:137 > if (segmentLength > requestedSize) > segmentLength = requestedSize; Replace this with a std::min, since that's what we do elsewhere? > Source/WebCore/platform/SharedBufferChunkReader.h:63 > + const size_t m_bufferSize { 0 }; Can drop the initializer since both constructors initialize this. (And since it's const, the compiler will complain if a constructor stops initializing it.) > Source/WebCore/platform/SharedBufferChunkReader.h:66 > + bool m_reachedEndOfFile { false }; I think we can get rid of m_reachedEndOfFile, and check `m_iteratorCurrent != m_iteratorEnd` instead. (In a private helper function, if you like.) > Tools/TestWebKitAPI/Tests/WebCore/SharedBuffer.cpp:237 > + const char* const simpleText = "This is a stupid test."; s/stupid/simple/ if you want to be more positive. :-) Created attachment 445166 [details]
Patch
Apply comment
Committed r286171 (244554@main): <https://commits.webkit.org/244554@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 445166 [details]. |