|
|
Description[BlobAsync] Patch 1: BlobStorageRegistry
Note: This patch is a no-op, and will be hooked up in a later patch.
Patches:
1: https://codereview.chromium.org/1287303002
2: https://codereview.chromium.org/1288373002
3: https://codereview.chromium.org/1292523002
4: https://codereview.chromium.org/1098853003
Hookup: https://codereview.chromium.org/1234813004
BUG=375297
Committed: https://crrev.com/d0e06a58f51ee83a670829425cd65a5693909eee
Cr-Commit-Position: refs/heads/master@{#354050}
Patch Set 1 #
Total comments: 45
Patch Set 2 : comments #
Total comments: 20
Patch Set 3 : comments #Patch Set 4 : correction #
Total comments: 20
Patch Set 5 : comments #Patch Set 6 : comments #Patch Set 7 : comments #Patch Set 8 : Added export to struct #
Messages
Total messages: 27 (7 generated)
dmurph@chromium.org changed reviewers: + kinuko@chromium.org, michaeln@chromium.org
Hello Michael & Kinuko: I'm splitting the giant part 1 patch into segments that are easier to review. This is part one. Sorry for the giant patch, this should be easier to review. Daniel
looking good https://codereview.chromium.org/1287303002/diff/1/content/browser/blob_storag... File content/browser/blob_storage/blob_storage_registry_unittest.cc (right): https://codereview.chromium.org/1287303002/diff/1/content/browser/blob_storag... content/browser/blob_storage/blob_storage_registry_unittest.cc:63: BlobRegistryEntry* entry = registry.RegisterBlobUUID(kBlob); nit: I'd put empty line before line 63 rather than after line 63, to make it clearer that the block l.58-62 is testing initial state before registering anything https://codereview.chromium.org/1287303002/diff/1/content/browser/blob_storag... content/browser/blob_storage/blob_storage_registry_unittest.cc:77: EXPECT_TRUE(registry.RegisterURLMapping(kURL2, kBlob2)); nit: test registry.IsURLMapped(kURL2) here for completeness https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... File storage/browser/blob/blob_storage_registry.cc (right): https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.cc:62: BlobRegistryEntry* entry = temp->second; scoped_ptr<BlobRegistryEntry> entry(temp->second); and remove line 71 https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.cc:77: if (blob_map_.find(uuid) != blob_map_.end()) nit: prefer ContainsKey() in stl_util.h in general https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.cc:145: return url_to_uuid_.find(blob_url) != url_to_uuid_.end(); nit: ContainsKey https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... File storage/browser/blob/blob_storage_registry.h (right): https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.h:33: // Second, the we are asynchronously transporting data to the browser. nit: 'the we' -> 'we' https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.h:35: // Third, we construct the blob when we have all of the data nit: period at the end of comments https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.h:40: enum BlobFlags { EXCEEDED_MEMORY = 1 << 1 }; nit: I'd call this just Flags, or rename the following field/methods to use 'blob_flags' and '{Set,Unset,Is}BlobFlag' so that it becomes easier to associate https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.h:57: void UnsetFlag(int flag); No one's calling these for now? If so I'd just drop these from this CL https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.h:79: // does not exist. Just returning UNKNOWN doesn't tell if it exists or not, would it be ok? If we will be calling this only when we're sure IsUUIDRegistered() is true we could just DCHECK if given uuid doesn't exist, and it could make things simpler some times. https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.h:108: friend class ViewBlobInternalsJob; nit: this friend's not necessary yet?
thnx for spitting the patch up, i have no excuse for being so slow to reply and owe a big apology for that i read ahead somewhat to see there this is going and in particular to see how this class is to be used (i had some unsent comments on your older larger patch that i'll clean up and send as well) https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... File storage/browser/blob/blob_storage_registry.cc (right): https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.cc:11: #include "storage/browser/blob/blob_storage_registry.h" you can put the main include up top followed by a blank line https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.cc:43: return ((flags & flag) != 0); probably can inline the flag getters/settings and use the is_flag_set() naming style https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.cc:51: flags &= !flag; do you want the bitwise operator here? https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.cc:59: while (begin != end) { this is oddly formed for each loop, would a for (const auto& pair : blob_map_) work here? https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.cc:65: if (entry && !entry->construction_complete_callbacks.empty()) { Can |entry| ever be null? i don't see how it can? https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.cc:77: if (blob_map_.find(uuid) != blob_map_.end()) Is attempting to reuse a uuid ever expected? I can see given bogus ipc message inputs from a renderer that this might happen, but otherwise i don't think it would ever be expected. We do need to check test for valid ipc message inputs, but those tests should be higher up in the stack closer to where the msg is received. At this lower level, i'd rather see us asserting that we're getting good inputs from our caller. Otherwise there's a little more code and these runtime checks will actually be done twice in the product, once at the higher level closer to weeding out bad ipc inputs and again here. Not the end of the world to be doing so, but to not be doing so would be a slight improvement. I have similar questions/comments about all of the existance tests and early returns. Which are actually expected at runtime and which would be indicative of a bug higher up? https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... File storage/browser/blob/blob_storage_registry.h (right): https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.h:27: class STORAGE_EXPORT BlobStorageRegistry { Terminology nit that i think may be worth reconciling? In existing storage/browser code, the term 'registered' applies to only to public url mappings, the urls are registered and then later revoked. Blobs themselves are 'built' incrementally or 'added' in complete form, they exist or they don't. The mixing-and-matching of different terminology makes the code a little more difficult to follow). I'd vote to be consistent about terminology in the storage/browser code. https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.h:42: struct BlobRegistryEntry { since we're in a class named BlobRegistry the prefix on this class name is redundant, we could probably get away with 'Entry' here https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.h:65: BlobRegistryEntry* RegisterBlobUUID(const std::string& uuid); Here's a stab not applying 'register' as a verb to the blob proper (or to the url mappings either for that matter). wdyt? Entry* CreateEntry(const string& uuid); Entry* GetEntry(const string& uuid); void DeleteEntry(const string& uuid); bool CreateUrlMapping(...); bool DeleteUrlMapping(...); bool HasUrlMapping(...); Entry* GetEntryForUrl(...); https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.h:79: // does not exist. On 2015/08/14 10:12:18, kinuko wrote: > Just returning UNKNOWN doesn't tell if it exists or not, would it be ok? If we > will be calling this only when we're sure IsUUIDRegistered() is true we could > just DCHECK if given uuid doesn't exist, and it could make things simpler some > times. Is this helper really needed at all? If i understand where the design is going, the BlobStorageContext is the only real consumer of this class and it should only be trying to twiddle these bits on blobs that are in the act of being built. https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.h:85: bool TestAndSetState(const std::string& uuid, This helper could go on the Entry class itself.
Hey guys, Sorry this took so long, I was working on the reader patch which I need as a prerequisite for all of these. Let me know what you think about renaming the class. https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... File storage/browser/blob/blob_storage_registry.cc (right): https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.cc:11: #include "storage/browser/blob/blob_storage_registry.h" On 2015/08/18 at 02:25:46, michaeln wrote: > you can put the main include up top followed by a blank line Whoops, thanks. https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.cc:43: return ((flags & flag) != 0); On 2015/08/18 at 02:25:46, michaeln wrote: > probably can inline the flag getters/settings and use the is_flag_set() naming style done. https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.cc:51: flags &= !flag; On 2015/08/18 at 02:25:46, michaeln wrote: > do you want the bitwise operator here? Yes. This is the current behavior in the system. I'm not changing this right now. https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.cc:59: while (begin != end) { On 2015/08/18 at 02:25:46, michaeln wrote: > this is oddly formed for each loop, would a for (const auto& pair : blob_map_) work here? Done. https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.cc:62: BlobRegistryEntry* entry = temp->second; On 2015/08/14 at 10:12:18, kinuko wrote: > scoped_ptr<BlobRegistryEntry> entry(temp->second); > > and remove line 71 Done. https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.cc:65: if (entry && !entry->construction_complete_callbacks.empty()) { On 2015/08/18 at 02:25:46, michaeln wrote: > Can |entry| ever be null? i don't see how it can? Nope. Removed. https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.cc:77: if (blob_map_.find(uuid) != blob_map_.end()) On 2015/08/18 at 02:25:46, michaeln wrote: > Is attempting to reuse a uuid ever expected? I can see given bogus ipc message inputs from a renderer that this might happen, but otherwise i don't think it would ever be expected. We do need to check test for valid ipc message inputs, but those tests should be higher up in the stack closer to where the msg is received. > > At this lower level, i'd rather see us asserting that we're getting good inputs from our caller. Otherwise there's a little more code and these runtime checks will actually be done twice in the product, once at the higher level closer to weeding out bad ipc inputs and again here. Not the end of the world to be doing so, but to not be doing so would be a slight improvement. > > I have similar questions/comments about all of the existance tests and early returns. Which are actually expected at runtime and which would be indicative of a bug higher up? Tests accidentally do this a lot, as I've done myself. This actually ends up being the one check that gets made, other than in the blob_storage_host. The context just calls the registry in most cases and handles the error case when the registry says something doesn't exist, etc. Plus, it's much easier to test this code and it's edge cases. https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.cc:145: return url_to_uuid_.find(blob_url) != url_to_uuid_.end(); On 2015/08/14 at 10:12:18, kinuko wrote: > nit: ContainsKey Done. https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... File storage/browser/blob/blob_storage_registry.h (right): https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.h:27: class STORAGE_EXPORT BlobStorageRegistry { On 2015/08/18 at 02:25:46, michaeln wrote: > Terminology nit that i think may be worth reconciling? > > In existing storage/browser code, the term 'registered' applies to only to public url mappings, the urls are registered and then later revoked. Blobs themselves are 'built' incrementally or 'added' in complete form, they exist or they don't. The mixing-and-matching of different terminology makes the code a little more difficult to follow). I'd vote to be consistent about terminology in the storage/browser code. Hm. I thought 'Registry' did a pretty good job of describing the function of the class, which is to store mappings from key to data. What about: * BlobStorageBackend * BlobStorageMap * BlobStorageStorage (kidding) https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.h:33: // Second, the we are asynchronously transporting data to the browser. On 2015/08/14 at 10:12:18, kinuko wrote: > nit: 'the we' -> 'we' Done. https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.h:35: // Third, we construct the blob when we have all of the data On 2015/08/14 at 10:12:18, kinuko wrote: > nit: period at the end of comments Done. https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.h:40: enum BlobFlags { EXCEEDED_MEMORY = 1 << 1 }; On 2015/08/14 at 10:12:19, kinuko wrote: > nit: I'd call this just Flags, or rename the following field/methods to use 'blob_flags' and '{Set,Unset,Is}BlobFlag' so that it becomes easier to associate Done. https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.h:42: struct BlobRegistryEntry { On 2015/08/18 at 02:25:46, michaeln wrote: > since we're in a class named BlobRegistry the prefix on this class name is redundant, we could probably get away with 'Entry' here Sounds great, done. https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.h:57: void UnsetFlag(int flag); On 2015/08/14 at 10:12:18, kinuko wrote: > No one's calling these for now? If so I'd just drop these from this CL No one is calling anything in this patch haha. These (and everything else from these 4 patches) will be called from the hookup patch here: https://codereview.chromium.org/1234813004 I'm updating the descriptions to include links to all patches. https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.h:65: BlobRegistryEntry* RegisterBlobUUID(const std::string& uuid); On 2015/08/18 at 02:25:47, michaeln wrote: > Here's a stab not applying 'register' as a verb to the blob proper (or to the url mappings either for that matter). wdyt? > > Entry* CreateEntry(const string& uuid); > Entry* GetEntry(const string& uuid); > void DeleteEntry(const string& uuid); > > bool CreateUrlMapping(...); > bool DeleteUrlMapping(...); > bool HasUrlMapping(...); > Entry* GetEntryForUrl(...); Sounds good. https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.h:79: // does not exist. On 2015/08/18 at 02:25:47, michaeln wrote: > On 2015/08/14 10:12:18, kinuko wrote: > > Just returning UNKNOWN doesn't tell if it exists or not, would it be ok? If we > > will be calling this only when we're sure IsUUIDRegistered() is true we could > > just DCHECK if given uuid doesn't exist, and it could make things simpler some > > times. > > Is this helper really needed at all? If i understand where the design is going, the BlobStorageContext is the only real consumer of this class and it should only be trying to twiddle these bits on blobs that are in the act of being built. On 2015/08/14 at 10:12:18, kinuko wrote: > Just returning UNKNOWN doesn't tell if it exists or not, would it be ok? If we will be calling this only when we're sure IsUUIDRegistered() is true we could just DCHECK if given uuid doesn't exist, and it could make things simpler some times. So this actually helps me avoid an extra map lookup in calling code. There are many instances where I want to check to is 'does the entry not exist or is it in an unknown state', and this help that. Otherwise I'm doing this pattern: if (!r.IsUUIDRegistered(...)) { return } State state = r.GetBlobState(...); if (state == UNKNOWN) return // do logic Instead, I can just do State state = r.GetBlobState(...); if (state == unknown) { // error handling return; } https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.h:85: bool TestAndSetState(const std::string& uuid, On 2015/08/18 at 02:25:47, michaeln wrote: > This helper could go on the Entry class itself. It's more useful like this currently, as it removes the code that checks to see if it exists or not. https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.h:108: friend class ViewBlobInternalsJob; On 2015/08/14 at 10:12:19, kinuko wrote: > nit: this friend's not necessary yet? Not yet....
Ping on this patch :)
https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... File storage/browser/blob/blob_storage_registry.cc (right): https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.cc:51: flags &= !flag; On 2015/09/15 00:54:34, dmurph wrote: > On 2015/08/18 at 02:25:46, michaeln wrote: > > do you want the bitwise operator here? > > Yes. This is the current behavior in the system. I'm not changing this right > now. Why, your changing most all other aspects of the current system? This line of code is mixing bitwise and boolean logic in a confusing way, unless there's a reason to not clean it up, why not clean it up? https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.cc:77: if (blob_map_.find(uuid) != blob_map_.end()) On 2015/09/15 00:54:34, dmurph wrote: > On 2015/08/18 at 02:25:46, michaeln wrote: > > Is attempting to reuse a uuid ever expected? I can see given bogus ipc message > inputs from a renderer that this might happen, but otherwise i don't think it > would ever be expected. We do need to check test for valid ipc message inputs, > but those tests should be higher up in the stack closer to where the msg is > received. > > > > At this lower level, i'd rather see us asserting that we're getting good > inputs from our caller. Otherwise there's a little more code and these runtime > checks will actually be done twice in the product, once at the higher level > closer to weeding out bad ipc inputs and again here. Not the end of the world to > be doing so, but to not be doing so would be a slight improvement. > > > > I have similar questions/comments about all of the existance tests and early > returns. Which are actually expected at runtime and which would be indicative of > a bug higher up? > > Tests accidentally do this a lot, as I've done myself. Fix the test, it's buggy. > This actually ends up being the one check that gets made, other than in the > blob_storage_host. The context just calls the registry in most cases and > handles the error case when the registry says something doesn't exist, etc. > > Plus, it's much easier to test this code and it's edge cases. Ok, but what happens is callers have to handle the null case, and their callers have to handle the return path from there, etc... net more code thats never expected or supposed to run that nonetheless is there and needs to be tested (which is yet more code :) Thats one reason why I prefer making strong assertions rather than branches in low level code, it reduces the number of ways control can flow. The other is clarity. Theres code to handle the not found case. Does that mean it's expected to occur in the product, you'd think so since theres code for it. So when somebody does see it happening in the product... instead of recognizing the problem is upstream, they "fix" it by adding yet more code to the downstream control flow branch that shouldn't happen (or even be there imho). https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... File storage/browser/blob/blob_storage_registry.h (right): https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.h:79: // does not exist. On 2015/09/15 00:54:34, dmurph wrote: > On 2015/08/18 at 02:25:47, michaeln wrote: > > On 2015/08/14 10:12:18, kinuko wrote: > > > Just returning UNKNOWN doesn't tell if it exists or not, would it be ok? If > we > > > will be calling this only when we're sure IsUUIDRegistered() is true we > could > > > just DCHECK if given uuid doesn't exist, and it could make things simpler > some > > > times. > > > > Is this helper really needed at all? If i understand where the design is > going, the BlobStorageContext is the only real consumer of this class and it > should only be trying to twiddle these bits on blobs that are in the act of > being built. > > On 2015/08/14 at 10:12:18, kinuko wrote: > > Just returning UNKNOWN doesn't tell if it exists or not, would it be ok? If we > will be calling this only when we're sure IsUUIDRegistered() is true we could > just DCHECK if given uuid doesn't exist, and it could make things simpler some > times. > > So this actually helps me avoid an extra map lookup in calling code. There are > many instances where I want to check to is 'does the entry not exist or is it in > an unknown state', and this help that. > > Otherwise I'm doing this pattern: > if (!r.IsUUIDRegistered(...)) { > return > } > > State state = r.GetBlobState(...); > if (state == UNKNOWN) > return > // do logic > > Instead, I can just do > State state = r.GetBlobState(...); > if (state == unknown) { > // error handling > return; > } Personally, i'm not a fan of the convenience methods. Why have fooExists() at all, just get it and use it? Entry* e = r.GetEntry(uuid); if (!e || e->state == unknown) return; // error handling https://codereview.chromium.org/1287303002/diff/20001/storage/browser/blob/bl... File storage/browser/blob/blob_storage_registry.cc (right): https://codereview.chromium.org/1287303002/diff/20001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.cc:38: BlobState state) indent is (waaaaayyyy :) off https://codereview.chromium.org/1287303002/diff/20001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.cc:49: // any dangling URL requests around. Alternatively, just don't callback. The registry class is real long lived. It's only destroyed at the end-of-time (very apocalyptic). Running code when half the objects in the system are deleted opens the door to going down unexpected code paths, and all for what? To tell some small part of the exiting browser that the blob it was waiting for is never going to be built. Since there aren't any consumers yet expecting/dependingon post-apocalyptic callbacks, i'd vote to define them out of existence and to just delete the callbacks w/o calling them. Good chance of sparing us a few shutdown bugs. https://codereview.chromium.org/1287303002/diff/20001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.cc:64: auto* entry = new Entry(1, BlobState::RESERVED); Entry* instead of auto (slightly less magical) https://codereview.chromium.org/1287303002/diff/20001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.cc:108: const std::string& uuid) { maybe DCHECK(!BlobUrlHasRef(blob_url)); https://codereview.chromium.org/1287303002/diff/20001/storage/browser/blob/bl... File storage/browser/blob/blob_storage_registry.h (right): https://codereview.chromium.org/1287303002/diff/20001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:55: bool IsFlagSet(int flag) { return ((flags & flag) != 0); } could be const https://codereview.chromium.org/1287303002/diff/20001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:57: void UnsetFlag(int flag); these could all be inlined as oneliners https://codereview.chromium.org/1287303002/diff/20001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:61: virtual ~BlobStorageRegistry(); are there subclasses? if not then nix virtual https://codereview.chromium.org/1287303002/diff/20001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:72: bool HasEntry(const std::string& uuid) const; not really needed given GetEntry() https://codereview.chromium.org/1287303002/diff/20001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:76: Entry* GetBlobEntry(const std::string& uuid); nit: GetEntry to harmonize with Create/Delete/Has methods. and could be const https://codereview.chromium.org/1287303002/diff/20001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:102: Entry* GetBlobEntryFromURL(const GURL& url, std::string* uuid); nit: ditto GetEntryFromURL naming
Done! Thanks for the comments! https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... File storage/browser/blob/blob_storage_registry.cc (right): https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.cc:51: flags &= !flag; On 2015/10/09 at 02:16:14, michaeln wrote: > On 2015/09/15 00:54:34, dmurph wrote: > > On 2015/08/18 at 02:25:46, michaeln wrote: > > > do you want the bitwise operator here? > > > > Yes. This is the current behavior in the system. I'm not changing this right > > now. > > Why, your changing most all other aspects of the current system? This line of code is mixing bitwise and boolean logic in a confusing way, unless there's a reason to not clean it up, why not clean it up? Done. https://codereview.chromium.org/1287303002/diff/1/storage/browser/blob/blob_s... storage/browser/blob/blob_storage_registry.cc:77: if (blob_map_.find(uuid) != blob_map_.end()) On 2015/10/09 at 02:16:14, michaeln wrote: > On 2015/09/15 00:54:34, dmurph wrote: > > On 2015/08/18 at 02:25:46, michaeln wrote: > > > Is attempting to reuse a uuid ever expected? I can see given bogus ipc message > > inputs from a renderer that this might happen, but otherwise i don't think it > > would ever be expected. We do need to check test for valid ipc message inputs, > > but those tests should be higher up in the stack closer to where the msg is > > received. > > > > > > At this lower level, i'd rather see us asserting that we're getting good > > inputs from our caller. Otherwise there's a little more code and these runtime > > checks will actually be done twice in the product, once at the higher level > > closer to weeding out bad ipc inputs and again here. Not the end of the world to > > be doing so, but to not be doing so would be a slight improvement. > > > > > > I have similar questions/comments about all of the existance tests and early > > returns. Which are actually expected at runtime and which would be indicative of > > a bug higher up? > > > > Tests accidentally do this a lot, as I've done myself. > > Fix the test, it's buggy. > > > This actually ends up being the one check that gets made, other than in the > > blob_storage_host. The context just calls the registry in most cases and > > handles the error case when the registry says something doesn't exist, etc. > > > > Plus, it's much easier to test this code and it's edge cases. > > Ok, but what happens is callers have to handle the null case, and their callers have to handle the return path from there, etc... net more code thats never expected or supposed to run that nonetheless is there and needs to be tested (which is yet more code :) Thats one reason why I prefer making strong assertions rather than branches in low level code, it reduces the number of ways control can flow. > > The other is clarity. Theres code to handle the not found case. Does that mean it's expected to occur in the product, you'd think so since theres code for it. So when somebody does see it happening in the product... instead of recognizing the problem is upstream, they "fix" it by adding yet more code to the downstream control flow branch that shouldn't happen (or even be there imho). Ok, I made it a dcheck https://codereview.chromium.org/1287303002/diff/20001/storage/browser/blob/bl... File storage/browser/blob/blob_storage_registry.cc (right): https://codereview.chromium.org/1287303002/diff/20001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.cc:38: BlobState state) On 2015/10/09 at 02:16:14, michaeln wrote: > indent is (waaaaayyyy :) off Done. https://codereview.chromium.org/1287303002/diff/20001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.cc:49: // any dangling URL requests around. On 2015/10/09 at 02:16:14, michaeln wrote: > Alternatively, just don't callback. The registry class is real long lived. It's only destroyed at the end-of-time (very apocalyptic). Running code when half the objects in the system are deleted opens the door to going down unexpected code paths, and all for what? To tell some small part of the exiting browser that the blob it was waiting for is never going to be built. > > Since there aren't any consumers yet expecting/dependingon post-apocalyptic callbacks, i'd vote to define them out of existence and to just delete the callbacks w/o calling them. Good chance of sparing us a few shutdown bugs. Done. Left a comment. https://codereview.chromium.org/1287303002/diff/20001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.cc:64: auto* entry = new Entry(1, BlobState::RESERVED); On 2015/10/09 at 02:16:14, michaeln wrote: > Entry* instead of auto (slightly less magical) Done. https://codereview.chromium.org/1287303002/diff/20001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.cc:108: const std::string& uuid) { On 2015/10/09 at 02:16:14, michaeln wrote: > maybe DCHECK(!BlobUrlHasRef(blob_url)); Done. https://codereview.chromium.org/1287303002/diff/20001/storage/browser/blob/bl... File storage/browser/blob/blob_storage_registry.h (right): https://codereview.chromium.org/1287303002/diff/20001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:55: bool IsFlagSet(int flag) { return ((flags & flag) != 0); } On 2015/10/09 at 02:16:14, michaeln wrote: > could be const Removed instead, as per other comment. https://codereview.chromium.org/1287303002/diff/20001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:57: void UnsetFlag(int flag); On 2015/10/09 at 02:16:14, michaeln wrote: > these could all be inlined as oneliners Removed instead, as per other comment. https://codereview.chromium.org/1287303002/diff/20001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:61: virtual ~BlobStorageRegistry(); On 2015/10/09 at 02:16:14, michaeln wrote: > are there subclasses? if not then nix virtual Done. https://codereview.chromium.org/1287303002/diff/20001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:72: bool HasEntry(const std::string& uuid) const; On 2015/10/09 at 02:16:14, michaeln wrote: > not really needed given GetEntry() Done. https://codereview.chromium.org/1287303002/diff/20001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:76: Entry* GetBlobEntry(const std::string& uuid); On 2015/10/09 at 02:16:14, michaeln wrote: > nit: GetEntry to harmonize with Create/Delete/Has methods. > and could be const Done. https://codereview.chromium.org/1287303002/diff/20001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:102: Entry* GetBlobEntryFromURL(const GURL& url, std::string* uuid); On 2015/10/09 at 02:16:14, michaeln wrote: > nit: ditto GetEntryFromURL naming Done.
Sorry for super slow review! https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... File storage/browser/blob/blob_storage_registry.cc (right): https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.cc:105: } nit: some one-line block has {} while some do not, maybe we should be consistent? https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... File storage/browser/blob/blob_storage_registry.h (right): https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:41: enum Flags { EXCEEDED_MEMORY = 1 << 1 }; Looks like this one's no longer (or at least not for now) needed https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:49: bool exceeded_memory; at this point this bool is not really used either? maybe just remove this for now and re-add later when we start to use this? https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:55: Entry(int refcount, BlobState state); maybe should we delete default constructor? (Entry() = delete) https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:76: bool HasEntry(const std::string& uuid) const; feels a bit duplicated with GetEntry? https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:80: Entry* GetEntry(const std::string& uuid) const; returning a non-const pointer to internal map from const method is not really recommended, could this return const ptr or the method be non-const? https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:102: typedef std::map<GURL, std::string> URLMap; nit: prefer using rather than typedef
lgtm https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... File storage/browser/blob/blob_storage_registry.h (right): https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:31: UNKNOWN = 0, Thnx for flattening the helpers out. I think there's no longer a need for the UNKNOWN state. I looks like it was there only to provide a possible return value to the reg class helpers when asked about entries that didn't exist. An entry proper is never actually in the UNKNOWN state. https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:80: Entry* GetEntry(const std::string& uuid) const; On 2015/10/12 14:34:58, kinuko wrote: > returning a non-const pointer to internal map from const method is not really > recommended, could this return const ptr or the method be non-const? ooops, i suggested this in a earlier comment, i may have given you a bad tip. it can't return a const ptr because the caller has to be able to alter the entry's state and refcount. https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:101: typedef std::map<std::string, Entry*> BlobMap; we might want to use ScopedPtrHashMap instead, wdyt?
https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... File storage/browser/blob/blob_storage_registry.cc (right): https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.cc:105: } On 2015/10/12 at 14:34:58, kinuko wrote: > nit: some one-line block has {} while some do not, maybe we should be consistent? Fixed. https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... File storage/browser/blob/blob_storage_registry.h (right): https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:31: UNKNOWN = 0, On 2015/10/12 at 19:50:13, michaeln wrote: > Thnx for flattening the helpers out. > > I think there's no longer a need for the UNKNOWN state. I looks like it was there only to provide a possible return value to the reg class helpers when asked about entries that didn't exist. An entry proper is never actually in the UNKNOWN state. This is a protection against uninitialized entries, and is a common pattern. You put the '0' value of the enumeration as an unknown error state, and if you ever encounter that, then that enum wasn't initialized at all. https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:41: enum Flags { EXCEEDED_MEMORY = 1 << 1 }; On 2015/10/12 at 14:34:58, kinuko wrote: > Looks like this one's no longer (or at least not for now) needed Whoops, yeah, I didn't remove it. https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:49: bool exceeded_memory; On 2015/10/12 at 14:34:58, kinuko wrote: > at this point this bool is not really used either? maybe just remove this for now and re-add later when we start to use this? This bool is definitely used right now (instead of flags) to signify if the blob has exceeded the available memory. This is the current behavior of the system. (I haven't rebased the later patches yet to include the removal of flags yet). https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:55: Entry(int refcount, BlobState state); On 2015/10/12 at 14:34:58, kinuko wrote: > maybe should we delete default constructor? (Entry() = delete) Done. https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:76: bool HasEntry(const std::string& uuid) const; On 2015/10/12 at 14:34:58, kinuko wrote: > feels a bit duplicated with GetEntry? Removed. https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:80: Entry* GetEntry(const std::string& uuid) const; On 2015/10/12 at 19:50:13, michaeln wrote: > On 2015/10/12 14:34:58, kinuko wrote: > > returning a non-const pointer to internal map from const method is not really > > recommended, could this return const ptr or the method be non-const? > > ooops, i suggested this in a earlier comment, i may have given you a bad tip. it can't return a const ptr because the caller has to be able to alter the entry's state and refcount. Done. https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:101: typedef std::map<std::string, Entry*> BlobMap; On 2015/10/12 at 19:50:13, michaeln wrote: > we might want to use ScopedPtrHashMap instead, wdyt? Sure. https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:102: typedef std::map<GURL, std::string> URLMap; On 2015/10/12 at 14:34:58, kinuko wrote: > nit: prefer using rather than typedef Done.
https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... File storage/browser/blob/blob_storage_registry.h (right): https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_storage_registry.h:31: UNKNOWN = 0, On 2015/10/12 20:08:28, dmurph wrote: > On 2015/10/12 at 19:50:13, michaeln wrote: > > Thnx for flattening the helpers out. > > > > I think there's no longer a need for the UNKNOWN state. I looks like it was > there only to provide a possible return value to the reg class helpers when > asked about entries that didn't exist. An entry proper is never actually in the > UNKNOWN state. > > This is a protection against uninitialized entries, and is a common pattern. You > put the '0' value of the enumeration as an unknown error state, and if you ever > encounter that, then that enum wasn't initialized at all. Is it common to start the first value at 1 instead of 0 and to not define the enum value that should never be used?
On 2015/10/12 at 22:17:14, michaeln wrote: > https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... > File storage/browser/blob/blob_storage_registry.h (right): > > https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... > storage/browser/blob/blob_storage_registry.h:31: UNKNOWN = 0, > On 2015/10/12 20:08:28, dmurph wrote: > > On 2015/10/12 at 19:50:13, michaeln wrote: > > > Thnx for flattening the helpers out. > > > > > > I think there's no longer a need for the UNKNOWN state. I looks like it was > > there only to provide a possible return value to the reg class helpers when > > asked about entries that didn't exist. An entry proper is never actually in the > > UNKNOWN state. > > > > This is a protection against uninitialized entries, and is a common pattern. You > > put the '0' value of the enumeration as an unknown error state, and if you ever > > encounter that, then that enum wasn't initialized at all. > > Is it common to start the first value at 1 instead of 0 and to not define the enum value that should never be used? Well, I'm not sure what happens with enum classes actually. With enum classes, you wouldn't be able to check for the '0' case because it's strong type checked. I can remove it if you want, I don't care that much.
On 2015/10/12 22:53:39, dmurph wrote: > On 2015/10/12 at 22:17:14, michaeln wrote: > > > https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... > > File storage/browser/blob/blob_storage_registry.h (right): > > > > > https://codereview.chromium.org/1287303002/diff/60001/storage/browser/blob/bl... > > storage/browser/blob/blob_storage_registry.h:31: UNKNOWN = 0, > > On 2015/10/12 20:08:28, dmurph wrote: > > > On 2015/10/12 at 19:50:13, michaeln wrote: > > > > Thnx for flattening the helpers out. > > > > > > > > I think there's no longer a need for the UNKNOWN state. I looks like it > was > > > there only to provide a possible return value to the reg class helpers when > > > asked about entries that didn't exist. An entry proper is never actually in > the > > > UNKNOWN state. > > > > > > This is a protection against uninitialized entries, and is a common pattern. > You > > > put the '0' value of the enumeration as an unknown error state, and if you > ever > > > encounter that, then that enum wasn't initialized at all. > > > > Is it common to start the first value at 1 instead of 0 and to not define the > enum value that should never be used? > > Well, I'm not sure what happens with enum classes actually. With enum classes, > you wouldn't be able to check for the '0' case because it's strong type checked. > I can remove it if you want, I don't care that much. sure, i'd vote to remove it. with previous revs of this cl i had been wondering if UNKNOWN was a state an entry could be in. i saw that it was used as a return value to mean "entry not found", but since it was in the enum, it wasn't (and still isn't) clear that it was a verbotten state value. if it really is not a valid state value... i think it'd be better to just not define it.
lgtm I'm still not sure if we need exceeded_memory in this CL as no one seems to set it yet but it's ok to have it. For enum state I don't have strong opinion but if we can remove it we can remove it :)
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/1287303002/#ps120001 (title: "comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1287303002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1287303002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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/1287303002/#ps140001 (title: "Added export to struct")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1287303002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1287303002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/d0e06a58f51ee83a670829425cd65a5693909eee Cr-Commit-Position: refs/heads/master@{#354050}
Message was sent while issue was closed.
capuchinomoccaz@gmail.com changed reviewers: + CapuchinomoccaZ@gmail.com
Message was sent while issue was closed.
lgtm อีเมลและกูเกิลเพลหยุด ทำงาน |