Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1183713003:
Blob Consolidation & Registry Hookup (Closed)
Description[Blob] Blob memory consolidation & blobregistry migration.
The blob consolidation class will hold the resources for the new asynchronous protocol, where it will keep the memory around while we wait for the browser to request it.
This is a multi-part patch. The next steps for Builder migration are:
* Change other users of WebBlobRegistry to use the Builder pattern
(mock_web_blob_registry.cc)
* Change blink to use the builder pattern and stop using registerBlob
* Remove WebBlobData and registerBlob method
The next steps for asynchronous blob transport:
* Add BlobStorageDispatcher with new IPC messages, and add asynchronous
callbacks in BlobStorageContext
See:
https://bit.ly/BlobStorageRefactor
This depends on blink patch here:
https://codereview.chromium.org/1162773003
BUG=375297
Committed: https://crrev.com/8a2ebfa1cb92700c36390a6ce9e83e8810655a62
Cr-Commit-Position: refs/heads/master@{#336648}
Patch Set 1 #Patch Set 2 : Separated out RecordMemoryRead #
Total comments: 29
Patch Set 3 : Comments, fixes #Patch Set 4 : rebase #Patch Set 5 : Android build fix #
Total comments: 8
Patch Set 6 : removed accounting #Patch Set 7 : comment #
Total comments: 36
Patch Set 8 : Fixes #
Total comments: 19
Patch Set 9 : comments #Patch Set 10 : windows issue #Patch Set 11 : windows fix #Patch Set 12 : windows issue #
Total comments: 1
Patch Set 13 : file path fix #
Messages
Total messages: 36 (12 generated)
dmurph@chromium.org changed reviewers: + kinuko@chromium.org, michaeln@chromium.org
dmurph@chromium.org changed required reviewers: + michaeln@chromium.org
Hey Michael & Kinuko, PTAL at this patch. The BlobConsolidation represents the data structure we'll be using to temporarily store the blob data while the browser is requesting it. Thanks! Daniel patch gif: http://i.imgur.com/v03r2aI.gif
> patch gif: > http://i.imgur.com/v03r2aI.gif great patch gif :)
+CC wkorman, who is helping me be a better SWE
looks pretty good, i like the read method :) https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... File content/child/blob_storage/blob_consolidation.cc (right): https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... content/child/blob_storage/blob_consolidation.cc:84: ReadStatus BlobConsolidation::RecordMemoryRead(size_t consolidated_item_index, please put the method bodies in the .cc in the same order as declared in the .h file (so this one goes after ReadMemory) https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... content/child/blob_storage/blob_consolidation.cc:123: // We do a binary search to find the correct data to start with in the data fun ;) https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... content/child/blob_storage/blob_consolidation.cc:135: // so we do (index - 1). i'd say you don't need what's after the , in the comment. Nice to keep it one line. If you keep it, maybe say [mid - 1]. https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... content/child/blob_storage/blob_consolidation.cc:156: // read starting from 'mid' and 'offset_from_mid' nit: cap "R" and a period at the end https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... File content/child/blob_storage/blob_consolidation.h (right): https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... content/child/blob_storage/blob_consolidation.h:38: class CONTENT_EXPORT BlobConsolidation { Seeing the heavy use of blink:WebTypes in here, i was initially concerned that we'd be exposing more chromium code to the vagaries of WebStrings. But then I saw this was really an impl detail of your new the Builder so that's not really a concern. You might consider nesting this class inside of WebBlobRegistryImpl since i think that's the only expected consumer of it (modulo tests): WebBlobRegistryImpl::Builder and WebBlobRegistryImpl::Consolidator. https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... content/child/blob_storage/blob_consolidation.h:93: char* memory_out); maybe void* memory_out to avoid casting at callsites https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... content/child/blob_storage/blob_consolidation.h:104: // TODO(dmurph): do per-item accounting, and error if we exceed per-item size. How do you anticipate using the accounting? I'm wondering if it's really needed? https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... File content/child/blob_storage/blob_consolidation_unittest.cc (right): https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... content/child/blob_storage/blob_consolidation_unittest.cc:17: char* data = new char[str.size()]; Is the extra heap allocation needed? Does WebThreadSafeData(str.data(), str.size()) not work? It looks like |data| is leaked. https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... content/child/blob_storage/blob_consolidation_unittest.cc:92: static const size_t NUM_PARTS = 300; I think our style guide prefer we name constants like this kSomething and use THIS_STYLE for enumerated values. Is there any value to making these static? https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... content/child/blob_storage/blob_consolidation_unittest.cc:96: static const size_t NUM_SEGMENTS = TOTAL_MEMORY / SEGMENT_SIZE; maybe move these kSegmentSize and kNumSegments down to right above where they're used to in the loop to read https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... content/child/blob_storage/blob_consolidation_unittest.cc:100: char* data = new char[PART_SIZE]; looks like |data| is leaked here too https://codereview.chromium.org/1183713003/diff/20001/content/child/webblobre... File content/child/webblobregistry_impl.cc (left): https://codereview.chromium.org/1183713003/diff/20001/content/child/webblobre... content/child/webblobregistry_impl.cc:64: if (data_item.length == 0) { This filter for blink giving us empty items got lost in the refactor, it'd be nice to retain it in the new builder/consolidator world. https://codereview.chromium.org/1183713003/diff/20001/content/child/webblobre... File content/child/webblobregistry_impl.cc (right): https://codereview.chromium.org/1183713003/diff/20001/content/child/webblobre... content/child/webblobregistry_impl.cc:219: sender_->Send(new BlobHostMsg_AppendBlobDataItem(uuid_, item)); This message being sent looks out of place as does the duplicate comment.
Thanks for the comments! See below for some explanations. https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... File content/child/blob_storage/blob_consolidation.cc (right): https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... content/child/blob_storage/blob_consolidation.cc:84: ReadStatus BlobConsolidation::RecordMemoryRead(size_t consolidated_item_index, On 2015/06/17 at 03:17:55, michaeln wrote: > please put the method bodies in the .cc in the same order as declared in the .h file (so this one goes after ReadMemory) Done. https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... content/child/blob_storage/blob_consolidation.cc:135: // so we do (index - 1). On 2015/06/17 at 03:17:55, michaeln wrote: > i'd say you don't need what's after the , in the comment. Nice to keep it one line. If you keep it, maybe say [mid - 1]. Done. https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... content/child/blob_storage/blob_consolidation.cc:156: // read starting from 'mid' and 'offset_from_mid' On 2015/06/17 at 03:17:55, michaeln wrote: > nit: cap "R" and a period at the end Done. https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... File content/child/blob_storage/blob_consolidation.h (right): https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... content/child/blob_storage/blob_consolidation.h:38: class CONTENT_EXPORT BlobConsolidation { On 2015/06/17 at 03:17:55, michaeln wrote: > Seeing the heavy use of blink:WebTypes in here, i was initially concerned that we'd be exposing more chromium code to the vagaries of WebStrings. But then I saw this was really an impl detail of your new the Builder so that's not really a concern. > > You might consider nesting this class inside of WebBlobRegistryImpl since i think that's the only expected consumer of it (modulo tests): > WebBlobRegistryImpl::Builder and WebBlobRegistryImpl::Consolidator. This class is going to be used elsewhere in later patches (in new files blob_storage_dispatcher, blob_transport_temporary_holder, and the registry), I'd rather leave it out. Especially because it has non-trivial logic and tests. https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... content/child/blob_storage/blob_consolidation.h:93: char* memory_out); On 2015/06/17 at 03:17:55, michaeln wrote: > maybe void* memory_out to avoid casting at callsites Done. This has the side effect of me needing to cast it to a char* internally so I can do pointer arithmetic, just FYI https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... content/child/blob_storage/blob_consolidation.h:104: // TODO(dmurph): do per-item accounting, and error if we exceed per-item size. On 2015/06/17 at 03:17:55, michaeln wrote: > How do you anticipate using the accounting? I'm wondering if it's really needed? There are two cases where the memory is read: 1. on the 'shortcut' approach, where we send as much data as we can in the initial RPC. Here we only read, and we don't count that as 'using' memory 2. on a data request from the browser. After we read we can then count the memory as 'read', and discard it. I'm implementing a trivial version of this bookkeeping, with a TODO for better accounting if it's needed. Finally, when we get DONE on this method (or a blobdone ipc), we can destruct this object. There are many ways to do this, this seemed like a decent way. https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... File content/child/blob_storage/blob_consolidation_unittest.cc (right): https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... content/child/blob_storage/blob_consolidation_unittest.cc:17: char* data = new char[str.size()]; On 2015/06/17 at 03:17:55, michaeln wrote: > Is the extra heap allocation needed? Does WebThreadSafeData(str.data(), str.size()) not work? > > It looks like |data| is leaked. Ah, yeah, you're right. This was from a previous version that grabbed ownership of the pointer. https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... content/child/blob_storage/blob_consolidation_unittest.cc:92: static const size_t NUM_PARTS = 300; On 2015/06/17 at 03:17:55, michaeln wrote: > I think our style guide prefer we name constants like this kSomething and use THIS_STYLE for enumerated values. Is there any value to making these static? no value, they're global constants, so why not? https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... content/child/blob_storage/blob_consolidation_unittest.cc:96: static const size_t NUM_SEGMENTS = TOTAL_MEMORY / SEGMENT_SIZE; On 2015/06/17 at 03:17:55, michaeln wrote: > maybe move these kSegmentSize and kNumSegments down to right above where they're used to in the loop to read do you mind if I keep all of the constants in one spot? I made the name a bit more specific. https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... content/child/blob_storage/blob_consolidation_unittest.cc:100: char* data = new char[PART_SIZE]; On 2015/06/17 at 03:17:55, michaeln wrote: > looks like |data| is leaked here too thanks, fixed. https://codereview.chromium.org/1183713003/diff/20001/content/child/webblobre... File content/child/webblobregistry_impl.cc (left): https://codereview.chromium.org/1183713003/diff/20001/content/child/webblobre... content/child/webblobregistry_impl.cc:64: if (data_item.length == 0) { On 2015/06/17 at 03:17:55, michaeln wrote: > This filter for blink giving us empty items got lost in the refactor, it'd be nice to retain it in the new builder/consolidator world. Done. I added this check in the append methods in BlobConsolidation as well (as we'll be removing this section). https://codereview.chromium.org/1183713003/diff/20001/content/child/webblobre... File content/child/webblobregistry_impl.cc (right): https://codereview.chromium.org/1183713003/diff/20001/content/child/webblobre... content/child/webblobregistry_impl.cc:219: sender_->Send(new BlobHostMsg_AppendBlobDataItem(uuid_, item)); On 2015/06/17 at 03:17:55, michaeln wrote: > This message being sent looks out of place as does the duplicate comment. Whoops, you're right, thanks.
The CQ bit was checked by dmurph@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/1183713003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> blob_storage_dispatcher, blob_transport_temporary_holder, and the registry If you have more of where you're headed coded up, seeing more of it would help me to review the incremental fragments you polish and put up for review. If you've got more wip, might as well show it for context. Just checking. https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... File content/child/blob_storage/blob_consolidation.h (right): https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... content/child/blob_storage/blob_consolidation.h:38: class CONTENT_EXPORT BlobConsolidation { On 2015/06/17 18:29:50, dmurph wrote: > On 2015/06/17 at 03:17:55, michaeln wrote: > > Seeing the heavy use of blink:WebTypes in here, i was initially concerned that > we'd be exposing more chromium code to the vagaries of WebStrings. But then I > saw this was really an impl detail of your new the Builder so that's not really > a concern. > > > > You might consider nesting this class inside of WebBlobRegistryImpl since i > think that's the only expected consumer of it (modulo tests): > > WebBlobRegistryImpl::Builder and WebBlobRegistryImpl::Consolidator. > > This class is going to be used elsewhere in later patches (in new files > blob_storage_dispatcher, blob_transport_temporary_holder, and the registry), I'd > rather leave it out. Especially because it has non-trivial logic and tests. Got it, makes sense to leave it as a separate class. But given it's broader use, it would be good to reduce its dependencies on the non-thread-safe blink backed data types (WebString and WebUrl). In general, we prefer to translate from WebSpeak into chrome data type speak as near the WebKep api impl layer as possible for consistency throughout the codebase on what types we use. And in this case in particular the non-thread-safe data types will definitely be problematic (easy landmines to step on). This ConsolidatedItem struct is more or less a storage::DataElement with some special casing for multiple fragmented TYPE_BYTES segments within a single 'item'. Wdyt of translating from to DataElement in BuilderImpls appendXXX methods, with the exception of WebThreadSafeData handling. The consolidator interface could look more like this... struct ConsolidatedItem { DataElement element; std::vector<size_t> offsets; std::vector<blink::WebThreadSafeData> data; } void AddNonDataItem(const DataElement& data_not_of_type_bytes); void AddDataItem(const blink::WebThreadSafeData& data); wdyt? https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... content/child/blob_storage/blob_consolidation.h:93: char* memory_out); On 2015/06/17 18:29:50, dmurph wrote: > On 2015/06/17 at 03:17:55, michaeln wrote: > > maybe void* memory_out to avoid casting at callsites > > Done. This has the side effect of me needing to cast it to a char* internally > so I can do pointer arithmetic, just FYI That's a fair trade, better to make life easier for he consumers. https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... content/child/blob_storage/blob_consolidation.h:104: // TODO(dmurph): do per-item accounting, and error if we exceed per-item size. On 2015/06/17 18:29:50, dmurph wrote: > On 2015/06/17 at 03:17:55, michaeln wrote: > > How do you anticipate using the accounting? I'm wondering if it's really > needed? > > There are two cases where the memory is read: > 1. on the 'shortcut' approach, where we send as much data as we can in the > initial RPC. Here we only read, and we don't count that as 'using' memory > 2. on a data request from the browser. After we read we can then count the > memory as 'read', and discard it. I'm implementing a trivial version of this > bookkeeping, with a TODO for better accounting if it's needed. > > Finally, when we get DONE on this method (or a blobdone ipc), we can destruct > this object. > > There are many ways to do this, this seemed like a decent way. Incremental cleanup might be "nice", but I'd vote to start simplest of all, delete it when done. If/when it comes time to do incremental cleanup, it can be added in then with a meaningful impl that actually performs incremental deletes. > There are many ways to do this, this seemed like a decent way. When it does come time to do incremental cleanup would it make sense to delete as it's being read, so the consolidated item is drained so to speak? https://codereview.chromium.org/1183713003/diff/80001/content/child/blob_stor... File content/child/blob_storage/blob_consolidation.cc (right): https://codereview.chromium.org/1183713003/diff/80001/content/child/blob_stor... content/child/blob_storage/blob_consolidation.cc:60: if (length == 0) return; it's very unusual to see this as a one-liner in content's .cc codebase, please put a newline here (and elsewhere in this file) https://codereview.chromium.org/1183713003/diff/80001/content/child/blob_stor... content/child/blob_storage/blob_consolidation.cc:143: memmove(static_cast<char*>(memory_out) + memory_read, would memcpy() be preferable here? https://codereview.chromium.org/1183713003/diff/80001/content/child/blob_stor... File content/child/blob_storage/blob_consolidation_unittest.cc (right): https://codereview.chromium.org/1183713003/diff/80001/content/child/blob_stor... content/child/blob_storage/blob_consolidation_unittest.cc:94: static const size_t kNumReadSegments = kTotalMemory / kReadSegmentSize; thnx, new names helped a lot doesn't really matter but... why not static? extra stuff to parse when reading and why make the linker carve out global storage for these used local only values.
https://codereview.chromium.org/1183713003/diff/80001/content/child/blob_stor... File content/child/blob_storage/blob_consolidation.cc (right): https://codereview.chromium.org/1183713003/diff/80001/content/child/blob_stor... content/child/blob_storage/blob_consolidation.cc:60: if (length == 0) return; On 2015/06/18 at 00:10:48, michaeln wrote: > it's very unusual to see this as a one-liner in content's .cc codebase, please put a newline here (and elsewhere in this file) It's valid by the style guide, but I'll add it ;) https://codereview.chromium.org/1183713003/diff/80001/content/child/blob_stor... content/child/blob_storage/blob_consolidation.cc:143: memmove(static_cast<char*>(memory_out) + memory_read, On 2015/06/18 at 00:10:48, michaeln wrote: > would memcpy() be preferable here? sure. https://codereview.chromium.org/1183713003/diff/80001/content/child/blob_stor... File content/child/blob_storage/blob_consolidation_unittest.cc (right): https://codereview.chromium.org/1183713003/diff/80001/content/child/blob_stor... content/child/blob_storage/blob_consolidation_unittest.cc:94: static const size_t kNumReadSegments = kTotalMemory / kReadSegmentSize; On 2015/06/18 at 00:10:48, michaeln wrote: > thnx, new names helped a lot > > doesn't really matter but... why not static? extra stuff to parse when reading and why make the linker carve out global storage for these used local only values. I didn't know this kind of stuff polluted a global storage. I just always made my const values static so they didn't increase the size of the stack frame. I can change it though, doesn't matter (especially in test case)
https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... File content/child/blob_storage/blob_consolidation.h (right): https://codereview.chromium.org/1183713003/diff/20001/content/child/blob_stor... content/child/blob_storage/blob_consolidation.h:38: class CONTENT_EXPORT BlobConsolidation { On 2015/06/18 at 00:10:48, michaeln wrote: > On 2015/06/17 18:29:50, dmurph wrote: > > On 2015/06/17 at 03:17:55, michaeln wrote: > > > Seeing the heavy use of blink:WebTypes in here, i was initially concerned that > > we'd be exposing more chromium code to the vagaries of WebStrings. But then I > > saw this was really an impl detail of your new the Builder so that's not really > > a concern. > > > > > > You might consider nesting this class inside of WebBlobRegistryImpl since i > > think that's the only expected consumer of it (modulo tests): > > > WebBlobRegistryImpl::Builder and WebBlobRegistryImpl::Consolidator. > > > > This class is going to be used elsewhere in later patches (in new files > > blob_storage_dispatcher, blob_transport_temporary_holder, and the registry), I'd > > rather leave it out. Especially because it has non-trivial logic and tests. > > Got it, makes sense to leave it as a separate class. > > But given it's broader use, it would be good to reduce its dependencies on the non-thread-safe blink backed data types (WebString and WebUrl). In general, we prefer to translate from WebSpeak into chrome data type speak as near the WebKep api impl layer as possible for consistency throughout the codebase on what types we use. And in this case in particular the non-thread-safe data types will definitely be problematic (easy landmines to step on). > > This ConsolidatedItem struct is more or less a storage::DataElement with some special casing for multiple fragmented TYPE_BYTES segments within a single 'item'. Wdyt of translating from to DataElement in BuilderImpls appendXXX methods, with the exception of WebThreadSafeData handling. The consolidator interface could look more like this... > > struct ConsolidatedItem { > DataElement element; > std::vector<size_t> offsets; > std::vector<blink::WebThreadSafeData> data; > } > > void AddNonDataItem(const DataElement& data_not_of_type_bytes); > void AddDataItem(const blink::WebThreadSafeData& data); > > wdyt? Whoops, missed this comment. As discussed in person , I moved the storage to non-web types.
thnx for switching to /base friendly types, a couple small comments to look at... lgtm https://codereview.chromium.org/1183713003/diff/80001/content/child/blob_stor... File content/child/blob_storage/blob_consolidation_unittest.cc (right): https://codereview.chromium.org/1183713003/diff/80001/content/child/blob_stor... content/child/blob_storage/blob_consolidation_unittest.cc:17: return blink::WebThreadSafeData(str.c_str(), str.size()); (in the not that it matters bucket) I think in many/most basic string impls, c_str() vs data(), one is implemented in terms of the other. Since we don't need the null terminator here though, i think data() is a slightly better choice for this usecase. https://codereview.chromium.org/1183713003/diff/80001/content/child/blob_stor... content/child/blob_storage/blob_consolidation_unittest.cc:94: static const size_t kNumReadSegments = kTotalMemory / kReadSegmentSize; On 2015/06/19 21:42:05, dmurph wrote: > On 2015/06/18 at 00:10:48, michaeln wrote: > > thnx, new names helped a lot > > > > doesn't really matter but... why not static? extra stuff to parse when reading > and why make the linker carve out global storage for these used local only > values. > > I didn't know this kind of stuff polluted a global storage. I just always made > my const values static so they didn't increase the size of the stack frame. I > can change it though, doesn't matter (especially in test case) idk, maybe it doesn't in this case because nothing ever takes the addr of them and its optimized away, non-static locals probably gives complilers/linkers more freedom to be optimial in general. https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.cc (right): https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:13: using blink::WebURL; i think only WebThreadSafeData is used now https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.h (right): https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:19: #include "third_party/WebKit/public/platform/WebURL.h" ditto only WebThreadSafeData https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:49: // Store our own variables instead of a DataElement because we need to i'd leave this comment out, we're trading off right now which way to go on this minor difference, but once we're committed it doesn't really matter https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation_unittest.cc (right): https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation_unittest.cc:49: EXPECT_EQ(4lu, consolidation.total_memory()); Generally (actuall overwhelmingly) we don't use 'lu' in cases like this and instead just use 'u', for size_t expectations. Probably should follow that practice here too. https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation_unittest.cc:181: EXPECT_THAT(memory_part3, testing::ElementsAreArray(expected_memory_part3)); i'm not familiar with this array matching, i'm assuming it does what it sounds like it does :) https://codereview.chromium.org/1183713003/diff/120001/content/child/webblobr... File content/child/webblobregistry_impl.h (right): https://codereview.chromium.org/1183713003/diff/120001/content/child/webblobr... content/child/webblobregistry_impl.h:82: std::string content_type_; nit: these first two members could be const
looking good, mostly nits only https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.cc (right): https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:23: expected_modification_time(0) {} nit: please put empty line between function definitions https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:42: ConsolidatedItem* item; move this right before it starts to be used (i.e. on the line 48) Also it looks we used non-const ref for the same purpose in other methods (i.e. ConsolidateItem& item = consolidated_items_.back()), can we do the same here for consistency? https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:54: item->data.push_back(WebThreadSafeData()); nit: I think just item->data.push_back(data) works? https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:98: } nit: no need of { } for single-line body in chromium (here and elsewhere), though it's ok to have them too https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:112: // constraints nit: end comments with period https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:132: // We are at the last item or the next offset it greater than the one nit: it greater -> is greater ? https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.h (right): https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:33: // NOTE: this class does not to memory accounting or garbage collecting. The nit: does not to -> does not do? https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:70: double expected_modification_time); nit: put each argument on a separate line unless they fits on one line (here and below) https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:90: void* memory_out); Please explicitly note that |memory_out| must be non-null and pointing to a buffer that has at least consolidated_size bytes etc. Any reason we use void* but not char* here? https://codereview.chromium.org/1183713003/diff/120001/content/child/webblobr... File content/child/webblobregistry_impl.cc (right): https://codereview.chromium.org/1183713003/diff/120001/content/child/webblobr... content/child/webblobregistry_impl.cc:282: if (!shared_memory->Map(shared_memory_size)) CHECK(false); CHECK(shared_memory->Map(shared_memory_size)); ? https://codereview.chromium.org/1183713003/diff/120001/content/child/webblobr... File content/child/webblobregistry_impl.h (right): https://codereview.chromium.org/1183713003/diff/120001/content/child/webblobr... content/child/webblobregistry_impl.h:32: // TODO(dmurph): remove this after moving to createBuilder nit: end comments with period https://codereview.chromium.org/1183713003/diff/120001/content/child/webblobr... content/child/webblobregistry_impl.h:63: ThreadSafeSender* registry); nit: registry -> sender
Done. Test failures are from a leaky constructor that I'm fixing in this patch: https://codereview.chromium.org/1205683002 https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.cc (right): https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:13: using blink::WebURL; On 2015/06/22 at 23:20:21, michaeln wrote: > i think only WebThreadSafeData is used now Done. https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:23: expected_modification_time(0) {} On 2015/06/23 at 14:11:30, kinuko wrote: > nit: please put empty line between function definitions Done. https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:42: ConsolidatedItem* item; On 2015/06/23 at 14:11:30, kinuko wrote: > move this right before it starts to be used (i.e. on the line 48) > > Also it looks we used non-const ref for the same purpose in other methods (i.e. ConsolidateItem& item = consolidated_items_.back()), can we do the same here for consistency? Done. https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:54: item->data.push_back(WebThreadSafeData()); On 2015/06/23 at 14:11:30, kinuko wrote: > nit: I think just item->data.push_back(data) works? Done. https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:98: } On 2015/06/23 at 14:11:30, kinuko wrote: > nit: no need of { } for single-line body in chromium (here and elsewhere), though it's ok to have them too Done. https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:112: // constraints On 2015/06/23 at 14:11:30, kinuko wrote: > nit: end comments with period Done. https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.cc:132: // We are at the last item or the next offset it greater than the one On 2015/06/23 at 14:11:30, kinuko wrote: > nit: it greater -> is greater ? Done. https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.h (right): https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:19: #include "third_party/WebKit/public/platform/WebURL.h" On 2015/06/22 at 23:20:21, michaeln wrote: > ditto only WebThreadSafeData Done. https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:33: // NOTE: this class does not to memory accounting or garbage collecting. The On 2015/06/23 at 14:11:30, kinuko wrote: > nit: does not to -> does not do? Done. https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:49: // Store our own variables instead of a DataElement because we need to On 2015/06/22 at 23:20:21, michaeln wrote: > i'd leave this comment out, we're trading off right now which way to go on this minor difference, but once we're committed it doesn't really matter Done. https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:70: double expected_modification_time); On 2015/06/23 at 14:11:30, kinuko wrote: > nit: put each argument on a separate line unless they fits on one line (here and below) Done. https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:90: void* memory_out); On 2015/06/23 at 14:11:30, kinuko wrote: > Please explicitly note that |memory_out| must be non-null and pointing to a buffer that has at least consolidated_size bytes etc. > > Any reason we use void* but not char* here? Done. Michael asked to use void* to simplify the calling protocol, as shared memory gives us a void*. https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation_unittest.cc (right): https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation_unittest.cc:49: EXPECT_EQ(4lu, consolidation.total_memory()); On 2015/06/22 at 23:20:21, michaeln wrote: > Generally (actuall overwhelmingly) we don't use 'lu' in cases like this and instead just use 'u', for size_t expectations. Probably should follow that practice here too. Done. https://codereview.chromium.org/1183713003/diff/120001/content/child/blob_sto... content/child/blob_storage/blob_consolidation_unittest.cc:181: EXPECT_THAT(memory_part3, testing::ElementsAreArray(expected_memory_part3)); On 2015/06/22 at 23:20:21, michaeln wrote: > i'm not familiar with this array matching, i'm assuming it does what it sounds like it does :) Yep :) https://codereview.chromium.org/1183713003/diff/120001/content/child/webblobr... File content/child/webblobregistry_impl.cc (right): https://codereview.chromium.org/1183713003/diff/120001/content/child/webblobr... content/child/webblobregistry_impl.cc:282: if (!shared_memory->Map(shared_memory_size)) CHECK(false); On 2015/06/23 at 14:11:30, kinuko wrote: > CHECK(shared_memory->Map(shared_memory_size)); ? better, thanks :) https://codereview.chromium.org/1183713003/diff/120001/content/child/webblobr... File content/child/webblobregistry_impl.h (right): https://codereview.chromium.org/1183713003/diff/120001/content/child/webblobr... content/child/webblobregistry_impl.h:32: // TODO(dmurph): remove this after moving to createBuilder On 2015/06/23 at 14:11:30, kinuko wrote: > nit: end comments with period Done. https://codereview.chromium.org/1183713003/diff/120001/content/child/webblobr... content/child/webblobregistry_impl.h:63: ThreadSafeSender* registry); On 2015/06/23 at 14:11:30, kinuko wrote: > nit: registry -> sender Done. https://codereview.chromium.org/1183713003/diff/120001/content/child/webblobr... content/child/webblobregistry_impl.h:82: std::string content_type_; On 2015/06/22 at 23:20:21, michaeln wrote: > nit: these first two members could be const Done.
lgtm It'd be handy to run 'git cl format' for minor formatting checks once you're done. https://codereview.chromium.org/1183713003/diff/140001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.h (right): https://codereview.chromium.org/1183713003/diff/140001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:38: DONE nit: DONE -> OK maybe if DONE means no-error? nit: I feel putting DONE (or OK) first and the generic ERROR one last is more common, but it'd be super trivial & subjective https://codereview.chromium.org/1183713003/diff/140001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:76: // This gets the consolidated items constructed from the WebBlobData. This nit: The comment's a bit stale? Any of methods don't seem to take WebBlobData https://codereview.chromium.org/1183713003/diff/140001/content/child/webblobr... File content/child/webblobregistry_impl.cc (right): https://codereview.chromium.org/1183713003/diff/140001/content/child/webblobr... content/child/webblobregistry_impl.cc:182: : uuid_(uuid.utf8()), content_type_(contentType.utf8()), sender_(sender) { nit: contentType -> content_type https://codereview.chromium.org/1183713003/diff/140001/content/child/webblobr... content/child/webblobregistry_impl.cc:203: expectedModificationTime); nit: expectedModificationTime -> expected_modi... https://codereview.chromium.org/1183713003/diff/140001/content/child/webblobr... content/child/webblobregistry_impl.cc:211: expectedModificationTime); ditto https://codereview.chromium.org/1183713003/diff/140001/content/child/webblobr... content/child/webblobregistry_impl.cc:266: sender_->Send(new BlobHostMsg_FinishBuilding(uuid_, content_type_)); Hmm ok build() sends all the IPC messages, so it means 'build the blob in the browser process' https://codereview.chromium.org/1183713003/diff/140001/content/child/webblobr... content/child/webblobregistry_impl.cc:282: CHECK(shared_memory->Map(shared_memory_size)); On the second thought doing something that has side-effect in an assertion-like macro doesn't look great, probably something like following would be better: const bool mapped = shared_memory->Map(shared_memory_size); CHECK(mapped); https://codereview.chromium.org/1183713003/diff/140001/content/child/webblobr... File content/child/webblobregistry_impl.h (right): https://codereview.chromium.org/1183713003/diff/140001/content/child/webblobr... content/child/webblobregistry_impl.h:32: // TODO(dmurph): remove this after moving to createBuilder. nit: add a crbug URL. https://codereview.chromium.org/1183713003/diff/140001/content/child/webblobr... content/child/webblobregistry_impl.h:37: const blink::WebString& uuid, const blink::WebString& contentType); It's less copy&paste friendly, but in chromium contentType -> content_type https://codereview.chromium.org/1183713003/diff/140001/content/child/webblobr... content/child/webblobregistry_impl.h:73: double expectedModificationTime) override; ditto for some of the methods above. expectedModificationTime -> expected_modification_time Also it's usually easier to go without override (but just make them virtual) at the repo boundary (we're still not yet merged)
https://codereview.chromium.org/1183713003/diff/140001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation.h (right): https://codereview.chromium.org/1183713003/diff/140001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:38: DONE On 2015/06/25 at 05:04:19, kinuko wrote: > nit: DONE -> OK maybe if DONE means no-error? > > nit: I feel putting DONE (or OK) first and the generic ERROR one last is more common, but it'd be super trivial & subjective I don't put the SUCCESS case first because it could mean that an uninitialized ReadStatus (which often defaults to 0) would erroneously be SUCCESS, which could hide bugs. I think there was a cpp-tip about this at some point. https://codereview.chromium.org/1183713003/diff/140001/content/child/blob_sto... content/child/blob_storage/blob_consolidation.h:76: // This gets the consolidated items constructed from the WebBlobData. This On 2015/06/25 at 05:04:19, kinuko wrote: > nit: The comment's a bit stale? Any of methods don't seem to take WebBlobData Done. https://codereview.chromium.org/1183713003/diff/140001/content/child/webblobr... File content/child/webblobregistry_impl.cc (right): https://codereview.chromium.org/1183713003/diff/140001/content/child/webblobr... content/child/webblobregistry_impl.cc:182: : uuid_(uuid.utf8()), content_type_(contentType.utf8()), sender_(sender) { On 2015/06/25 at 05:04:19, kinuko wrote: > nit: contentType -> content_type Done. https://codereview.chromium.org/1183713003/diff/140001/content/child/webblobr... content/child/webblobregistry_impl.cc:203: expectedModificationTime); On 2015/06/25 at 05:04:19, kinuko wrote: > nit: expectedModificationTime -> expected_modi... Done. https://codereview.chromium.org/1183713003/diff/140001/content/child/webblobr... content/child/webblobregistry_impl.cc:211: expectedModificationTime); On 2015/06/25 at 05:04:19, kinuko wrote: > ditto Done. https://codereview.chromium.org/1183713003/diff/140001/content/child/webblobr... content/child/webblobregistry_impl.cc:282: CHECK(shared_memory->Map(shared_memory_size)); On 2015/06/25 at 05:04:19, kinuko wrote: > On the second thought doing something that has side-effect in an assertion-like macro doesn't look great, probably something like following would be better: > > const bool mapped = shared_memory->Map(shared_memory_size); > CHECK(mapped); Done. https://codereview.chromium.org/1183713003/diff/140001/content/child/webblobr... File content/child/webblobregistry_impl.h (right): https://codereview.chromium.org/1183713003/diff/140001/content/child/webblobr... content/child/webblobregistry_impl.h:32: // TODO(dmurph): remove this after moving to createBuilder. On 2015/06/25 at 05:04:19, kinuko wrote: > nit: add a crbug URL. Done. https://codereview.chromium.org/1183713003/diff/140001/content/child/webblobr... content/child/webblobregistry_impl.h:37: const blink::WebString& uuid, const blink::WebString& contentType); On 2015/06/25 at 05:04:20, kinuko wrote: > It's less copy&paste friendly, but in chromium contentType -> content_type Done. https://codereview.chromium.org/1183713003/diff/140001/content/child/webblobr... content/child/webblobregistry_impl.h:73: double expectedModificationTime) override; On 2015/06/25 at 05:04:20, kinuko wrote: > ditto for some of the methods above. expectedModificationTime -> expected_modification_time > > Also it's usually easier to go without override (but just make them virtual) at the repo boundary (we're still not yet merged) Done.
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeln@chromium.org, kinuko@chromium.org Link to the patchset: https://codereview.chromium.org/1183713003/#ps160001 (title: "comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1183713003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, michaeln@chromium.org Link to the patchset: https://codereview.chromium.org/1183713003/#ps200001 (title: "windows fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1183713003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
https://codereview.chromium.org/1183713003/diff/220001/content/child/blob_sto... File content/child/blob_storage/blob_consolidation_unittest.cc (right): https://codereview.chromium.org/1183713003/diff/220001/content/child/blob_sto... content/child/blob_storage/blob_consolidation_unittest.cc:137: consolidation.AddFileItem(base::FilePath(std::string("testPath")), 1, 10, ooops, filepath char type is platform dependent base::FilePath(FILE_PATH_LITERAL("xyz"))
On 2015/06/26 at 22:59:02, michaeln wrote: > https://codereview.chromium.org/1183713003/diff/220001/content/child/blob_sto... > File content/child/blob_storage/blob_consolidation_unittest.cc (right): > > https://codereview.chromium.org/1183713003/diff/220001/content/child/blob_sto... > content/child/blob_storage/blob_consolidation_unittest.cc:137: consolidation.AddFileItem(base::FilePath(std::string("testPath")), 1, 10, > ooops, filepath char type is platform dependent > > base::FilePath(FILE_PATH_LITERAL("xyz")) Thanks, I was struggling with this haha
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, michaeln@chromium.org Link to the patchset: https://codereview.chromium.org/1183713003/#ps240001 (title: "file path fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1183713003/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/8a2ebfa1cb92700c36390a6ce9e83e8810655a62 Cr-Commit-Position: refs/heads/master@{#336648} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
