Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
248803003:
ServiceWorker: Store registration data in ServiceWorkerDatabase (Closed)
|
Created:
6 years, 8 months ago by nhiroki Modified:
6 years, 7 months ago CC:
chromium-reviews, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, jam, darin-cc_chromium.org, horo+watch_chromium.org, kinuko+watch, alecflett+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionServiceWorker: Store registration data in ServiceWorkerDatabase
This CL adds:
- proto file for RegistrationData and ResourceRecords
- reading/writing/deleting operations for RegistrationData
- unittests for these operations
BUG=364099
TEST=content_unittests --gtest_filter=ServiceWorkerDatabaseTest.*
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266760
Patch Set 1 : #
Total comments: 22
Patch Set 2 : address comments and focus on registration manipulation #Patch Set 3 : rebase on https://codereview.chromium.org/257593003/ #
Total comments: 12
Patch Set 4 : review fix #
Total comments: 12
Patch Set 5 : review fix #Patch Set 6 : rebase #Patch Set 7 : fix win build #
Messages
Total messages: 28 (0 generated)
Hi michaeln@ and jsbell@, This CL seems still rough and hasn't been well-tested yet, but I'd be greatful if you could take an early look. (Sorry for huge change... I mean to split this into some pieces later) Thanks!
Just a few observations. michaeln@ will have more. https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... File content/browser/service_worker/service_worker_database.cc (right): https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_database.cc:73: std::string RemovePrefix(const std::string& str, This is a little weird since you can't tell if it removed a prefix or not. All the current callers of RemovePrefix() do a StartsWithASCII() check first - maybe change the signature so it returns bool and the string is an out param? Or just DCHECK(StartsWithASCII(...)) ? https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_database.cc:82: std::ostringstream out; #include <sstream> for ostringstream? https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_database.cc:238: return true; If the iterator fails (!itr->status().ok()) should this HandleError? (ditto for all the other iteration use) https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_database.cc:262: if (!registration.ParseFromString(itr->value().ToString())) { Is there any other post-parsing sanity checking that we want to do? It's probably overly paranoid, but we could e.g. verify that the origins of the script/scope URLs match. https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_database.cc:688: void ServiceWorkerDatabase::HandleError( Can you add a TODO: for adding an UMA histogram?
wdyt about the api and schema changes outlined below? https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... File content/browser/service_worker/service_worker_database.cc (right): https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_database.cc:15: #include "content/browser/service_worker/service_worker_database.pb.h" so a build step on the .proto file produces c structs here... is that right? https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_database.cc:82: std::ostringstream out; should probably use #include "base/strings/stringprintf.h" for this instead, lots of usage of that in the project (vs 2 for oss in content/) https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_database.cc:216: bool ServiceWorkerDatabase::GetOriginsWithRegistrations( This method and GetNextAvailableIds will be called very early on in browser startup (prior to loading the initial page shown by the browser). The full table scan in this method gives me pause and makes me wonder if we should store unique origins separately in the schema for faster retrieval. Is there a difference in io required between iterating over keys with zerolen values vs iterating keys with larger values? Also, is there benefit to having all of the key values we'll be accessing early on prefixed similarly... ala "init-"? Was just talking to jsbell about that and it sounds like "yes" to both, so in light of that, i think we should tweek the schema: * add a new 'table' for unique origins * prefix schemaversion, nextids, and uniqueorigins with "initdata-" (or something). https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_database.cc:314: PutResourceRecordToBatch(*itr, registration.version_id(), &batch); If it helps, we could defer dealing with ResourceRecords for a while until we're happy with the reading/writing the RegistrationData. https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_database.cc:597: for (itr->Seek(kRegKeyPrefix); itr->Valid(); itr->Next()) { Oh no... linear scans. Let's massage things so this can seek directly to the right entry. Looks like we have two options: a. tweek the inputs to this function to include the 'origin' as well b. tweek the schema such that there's also an index by id I'm looking at the ServiceWorkerStorage class and requiring it to know origin + id in order to do anything is fine, so my vote is for a. https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... File content/browser/service_worker/service_worker_database.proto (right): https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_database.proto:30: optional string url = 2; since this is the first cut at it (and there is no version skew stuff), can we make all fields 'required' in both classes
Thank you for reviewing! Comment reply only. https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... File content/browser/service_worker/service_worker_database.cc (right): https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_database.cc:15: #include "content/browser/service_worker/service_worker_database.pb.h" On 2014/04/24 00:35:04, michaeln wrote: > so a build step on the .proto file produces c structs here... is that right? Yes, you're right. https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_database.cc:216: bool ServiceWorkerDatabase::GetOriginsWithRegistrations( On 2014/04/24 00:35:04, michaeln wrote: > This method and GetNextAvailableIds will be called very early on in browser > startup (prior to loading the initial page shown by the browser). The full table > scan in this method gives me pause and makes me wonder if we should store unique > origins separately in the schema for faster retrieval. I looked over some docs and sourcecode for leveldb: > Is there a difference in io required between iterating over keys with zerolen > values vs iterating keys with larger values? According to [1], key and value are adjacent in a block, so I think the former can be scanned more quickly than the latter. [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/leveld... > Also, is there benefit to having all of the key values we'll be accessing early > on prefixed similarly... ala "init-"? I think this is true. "Key Layout" section in [2] says "Adjacent keys (according to the database sort order) will usually be placed in the same block. Therefore the application can improve its performance by placing keys that are accessed together near each other and ..." [2] http://leveldb.googlecode.com/svn/trunk/doc/index.html > Was just talking to jsbell about that and it sounds like "yes" to both, so in > light of that, i think we should tweek the schema: > > * add a new 'table' for unique origins > * prefix schemaversion, nextids, and uniqueorigins with "initdata-" (or > something). As mentioned above, I agree with both of you. I'll tweak the schema as per you described. https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_database.cc:597: for (itr->Seek(kRegKeyPrefix); itr->Valid(); itr->Next()) { On 2014/04/24 00:35:04, michaeln wrote: > Oh no... linear scans. Let's massage things so this can seek directly to the > right entry. Looks like we have two options: > > a. tweek the inputs to this function to include the 'origin' as well > b. tweek the schema such that there's also an index by id > > I'm looking at the ServiceWorkerStorage class and requiring it to know origin + > id in order to do anything is fine, so my vote is for a. That sounds reasonable and I thought I'd like to change like (a). I'll do so. https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... File content/browser/service_worker/service_worker_database.proto (right): https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_database.proto:30: optional string url = 2; On 2014/04/24 00:35:04, michaeln wrote: > since this is the first cut at it (and there is no version skew stuff), can we > make all fields 'required' in both classes Hmm... if we mark them as required, - need to set all fields in unittests. I think that would be a bit messy. - cannot ignore deprecated fields in future versions, that is, we need to set them to dummy value even if they are no longer used. so I'd prefer to leave them. Wdyt?
Updated. PTAL. https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... File content/browser/service_worker/service_worker_database.cc (right): https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_database.cc:73: std::string RemovePrefix(const std::string& str, On 2014/04/23 22:55:54, jsbell wrote: > This is a little weird since you can't tell if it removed a prefix or not. All > the current callers of RemovePrefix() do a StartsWithASCII() check first - maybe > change the signature so it returns bool and the string is an out param? Or just > DCHECK(StartsWithASCII(...)) ? Done. https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_database.cc:82: std::ostringstream out; On 2014/04/24 00:35:04, michaeln wrote: > should probably use #include "base/strings/stringprintf.h" for this instead, > lots of usage of that in the project (vs 2 for oss in content/) Replaced them with StringPrintf. https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_database.cc:216: bool ServiceWorkerDatabase::GetOriginsWithRegistrations( On 2014/04/24 05:57:30, nhiroki wrote: > On 2014/04/24 00:35:04, michaeln wrote: > > This method and GetNextAvailableIds will be called very early on in browser > > startup (prior to loading the initial page shown by the browser). The full > table > > scan in this method gives me pause and makes me wonder if we should store > unique > > origins separately in the schema for faster retrieval. > > I looked over some docs and sourcecode for leveldb: > > > Is there a difference in io required between iterating over keys with zerolen > > values vs iterating keys with larger values? > > According to [1], key and value are adjacent in a block, so I think the former > can be scanned more quickly than the latter. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/leveld... > > > Also, is there benefit to having all of the key values we'll be accessing > early > > on prefixed similarly... ala "init-"? > > I think this is true. "Key Layout" section in [2] says "Adjacent keys (according > to the database sort order) will usually be placed in the same block. Therefore > the application can improve its performance by placing keys that are accessed > together near each other and ..." > > [2] http://leveldb.googlecode.com/svn/trunk/doc/index.html > > > Was just talking to jsbell about that and it sounds like "yes" to both, so in > > light of that, i think we should tweek the schema: > > > > * add a new 'table' for unique origins > > * prefix schemaversion, nextids, and uniqueorigins with "initdata-" (or > > something). > > As mentioned above, I agree with both of you. I'll tweak the schema as per you > described. Done. https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_database.cc:238: return true; On 2014/04/23 22:55:54, jsbell wrote: > If the iterator fails (!itr->status().ok()) should this HandleError? > > (ditto for all the other iteration use) Done. https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_database.cc:262: if (!registration.ParseFromString(itr->value().ToString())) { On 2014/04/23 22:55:54, jsbell wrote: > Is there any other post-parsing sanity checking that we want to do? > > It's probably overly paranoid, but we could e.g. verify that the origins of the > script/scope URLs match. For now, we seems to be able to check only script/scope URLs match. Added the check. https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_database.cc:314: PutResourceRecordToBatch(*itr, registration.version_id(), &batch); On 2014/04/24 00:35:04, michaeln wrote: > If it helps, we could defer dealing with ResourceRecords for a while until we're > happy with the reading/writing the RegistrationData. Okay, dropped them. https://codereview.chromium.org/248803003/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_database.cc:688: void ServiceWorkerDatabase::HandleError( On 2014/04/23 22:55:54, jsbell wrote: > Can you add a TODO: for adding an UMA histogram? Done.
fyi: factored out non-essential parts into a separate patch (https://codereview.chromium.org/257593003/)
looking pretty good https://codereview.chromium.org/248803003/diff/140001/content/browser/service... File content/browser/service_worker/service_worker_database.cc (right): https://codereview.chromium.org/248803003/diff/140001/content/browser/service... content/browser/service_worker/service_worker_database.cc:94: GURL origin = GURL(data.scope_url()).GetOrigin(); Maybe this class should entirely encapsulate the use of protobuffers as the serialization format. The generated classes have some gotchas associated with them. - can't have GURL data members - doesn't construct with default values Gotcha's that can make things a bit messy. > Hmm... if we mark them as required, > - need to set all fields in unittests. I think that would be a bit messy. > - cannot ignore deprecated fields in future versions, that is, we need to set > them to dummy value even if they are no longer used. > > so I'd prefer to leave them. Wdyt? I think the set of fields we have defined are fundamental and can't really go away, if something looks like its optional lets remove it instead. We could limit the use of the generated classes to these two methods: PutRegistrationDataToBatch and ParseRegistrationData. The ServiceWorkerDatabase public api could function in terms of normal c++ structs and the generated classes would be used for serialization/deserialization only. wdyt? https://codereview.chromium.org/248803003/diff/140001/content/browser/service... content/browser/service_worker/service_worker_database.cc:177: bool success = RemovePrefix(key, kUniqueOriginKey, &origin); This function is funky? Either it doesn't need to check the prefix or this method doesn't need to do the StartWithASCII check above. What if this was RemoveExpectedPrefix() and nuke the StartsWithAscii() above... or something. Maybe this class maybe could use a set of helpers to MakeThisOrThatKey(...) and ParseThisOrThatKey(...) for each kind of key and use those in place startswith and prefix manipulations. https://codereview.chromium.org/248803003/diff/140001/content/browser/service... content/browser/service_worker/service_worker_database.cc:257: PutUniqueOriginToBatch(GURL(registration.scope_url()).GetOrigin(), &batch); is leveldb smart about noticing when a write doesn't change anything? https://codereview.chromium.org/248803003/diff/140001/content/browser/service... content/browser/service_worker/service_worker_database.cc:440: bool ServiceWorkerDatabase::BumpNextAvailableIdIfNeeded( As coded this function will have problems if an id is bumped twice in a batch (which will be expected when we get to <resourceid,url> storage). It'd be nice to cache the next values in this class anyway, read them once and never read again, so bump becomes something like... if (next_xxx_ <= used_id) { next_xxx_ = used_id + 1; Write(key, next_xxx_; } I think we could seed cached values in LazyOpen() too. When starting with an empty db, don't bother reading them ever. Also don't bother writing any values until Bump() is called. When starting with an existing db, no key means next_id is "0". wdyt?
Thanks! Reply only. https://codereview.chromium.org/248803003/diff/140001/content/browser/service... File content/browser/service_worker/service_worker_database.cc (right): https://codereview.chromium.org/248803003/diff/140001/content/browser/service... content/browser/service_worker/service_worker_database.cc:94: GURL origin = GURL(data.scope_url()).GetOrigin(); On 2014/04/25 00:48:21, michaeln wrote: > Maybe this class should entirely encapsulate the use of protobuffers as the > serialization format. The generated classes have some gotchas associated with > them. > - can't have GURL data members > - doesn't construct with default values > Gotcha's that can make things a bit messy. > > > Hmm... if we mark them as required, > > - need to set all fields in unittests. I think that would be a bit messy. > > - cannot ignore deprecated fields in future versions, that is, we need to set > > them to dummy value even if they are no longer used. > > > > so I'd prefer to leave them. Wdyt? > > I think the set of fields we have defined are fundamental and can't really go > away, if something looks like its optional lets remove it instead. I see. Will make them as required. > We could limit the use of the generated classes to these two methods: > PutRegistrationDataToBatch and ParseRegistrationData. The ServiceWorkerDatabase > public api could function in terms of normal c++ structs and the generated > classes would be used for serialization/deserialization only. > > wdyt? At first I didn't want to have two similar struct/class, but now I'm thinking that can help to avoid such a mess. Okay, I'll try to resurrect ServiceWorkerDatabase::RegistrationData. https://codereview.chromium.org/248803003/diff/140001/content/browser/service... content/browser/service_worker/service_worker_database.cc:257: PutUniqueOriginToBatch(GURL(registration.scope_url()).GetOrigin(), &batch); On 2014/04/25 00:48:21, michaeln wrote: > is leveldb smart about noticing when a write doesn't change anything? AFAICS, leveldb always writes log record even if there isn't change. If we want to check the existence of the origin in advance, we need perform one read operation and may omit one write operation as follows: <current> origin exists : 1 write origin not exist : 1 write <check in advance> origin exists : 1 read origin not exist : 1 read & 1 write The check sounds reasonable. Okay, I'll change here to check it in advance.
super duper! now if only the cq would commit things in a timely fashion :)
https://codereview.chromium.org/248803003/diff/140001/content/browser/service... File content/browser/service_worker/service_worker_database.cc (right): https://codereview.chromium.org/248803003/diff/140001/content/browser/service... content/browser/service_worker/service_worker_database.cc:94: GURL origin = GURL(data.scope_url()).GetOrigin(); On 2014/04/25 00:48:21, michaeln wrote: > Maybe this class should entirely encapsulate the use of protobuffers as the > serialization format. The generated classes have some gotchas associated with > them. > - can't have GURL data members > - doesn't construct with default values Fyi, it's possible to populate a field with a default value. I'm not super positive about having similar structs (because protobuf-generated struct is usually expressive/useful enough in many cases, and that's how most google3 code is written), though GURL issue might be a bit annoying in chromium. I don't object to it, but I want to make sure if we're really strongly motivated not to use generated classes. > Gotcha's that can make things a bit messy. > > > Hmm... if we mark them as required, > > - need to set all fields in unittests. I think that would be a bit messy. > > - cannot ignore deprecated fields in future versions, that is, we need to set > > them to dummy value even if they are no longer used. > > > > so I'd prefer to leave them. Wdyt? > > I think the set of fields we have defined are fundamental and can't really go > away, if something looks like its optional lets remove it instead. > > We could limit the use of the generated classes to these two methods: > PutRegistrationDataToBatch and ParseRegistrationData. The ServiceWorkerDatabase > public api could function in terms of normal c++ structs and the generated > classes would be used for serialization/deserialization only. > > wdyt?
Updated the CL and removed 'wip' mark. Can you take another look? Thanks! https://codereview.chromium.org/248803003/diff/140001/content/browser/service... File content/browser/service_worker/service_worker_database.cc (right): https://codereview.chromium.org/248803003/diff/140001/content/browser/service... content/browser/service_worker/service_worker_database.cc:94: GURL origin = GURL(data.scope_url()).GetOrigin(); On 2014/04/25 03:43:27, kinuko wrote: > On 2014/04/25 00:48:21, michaeln wrote: > > Maybe this class should entirely encapsulate the use of protobuffers as the > > serialization format. The generated classes have some gotchas associated with > > them. > > - can't have GURL data members > > - doesn't construct with default values > > Fyi, it's possible to populate a field with a default value. > > I'm not super positive about having similar structs (because protobuf-generated > struct is usually expressive/useful enough in many cases, and that's how most > google3 code is written), though GURL issue might be a bit annoying in chromium. And there is base::Time issue. > I don't object to it, but I want to make sure if we're really strongly motivated > not to use generated classes. Hmm... either way seems to have both good and bad points. Since the original API uses non-protobuf-generated struct and I'd prefer to minimize changes in this CL as far as possible, can I use non-protobuf-generated struct for now? If using generated struct looks better, I'll rewrite them. > > Gotcha's that can make things a bit messy. > > > > > Hmm... if we mark them as required, > > > - need to set all fields in unittests. I think that would be a bit messy. > > > - cannot ignore deprecated fields in future versions, that is, we need to > set > > > them to dummy value even if they are no longer used. > > > > > > so I'd prefer to leave them. Wdyt? > > > > I think the set of fields we have defined are fundamental and can't really go > > away, if something looks like its optional lets remove it instead. > > > > We could limit the use of the generated classes to these two methods: > > PutRegistrationDataToBatch and ParseRegistrationData. The > ServiceWorkerDatabase > > public api could function in terms of normal c++ structs and the generated > > classes would be used for serialization/deserialization only. > > > > wdyt? > https://codereview.chromium.org/248803003/diff/140001/content/browser/service... content/browser/service_worker/service_worker_database.cc:177: bool success = RemovePrefix(key, kUniqueOriginKey, &origin); On 2014/04/25 00:48:21, michaeln wrote: > This function is funky? Either it doesn't need to check the prefix or this > method doesn't need to do the StartWithASCII check above. > > What if this was RemoveExpectedPrefix() and nuke the StartsWithAscii() above... > or something. > > Maybe this class maybe could use a set of helpers to MakeThisOrThatKey(...) and > ParseThisOrThatKey(...) for each kind of key and use those in place startswith > and prefix manipulations. Done. https://codereview.chromium.org/248803003/diff/140001/content/browser/service... content/browser/service_worker/service_worker_database.cc:257: PutUniqueOriginToBatch(GURL(registration.scope_url()).GetOrigin(), &batch); On 2014/04/25 02:06:11, nhiroki wrote: > On 2014/04/25 00:48:21, michaeln wrote: > > is leveldb smart about noticing when a write doesn't change anything? > > AFAICS, leveldb always writes log record even if there isn't change. > > If we want to check the existence of the origin in advance, we need perform one > read operation and may omit one write operation as follows: > > <current> > origin exists : 1 write > origin not exist : 1 write > > <check in advance> > origin exists : 1 read > origin not exist : 1 read & 1 write > > The check sounds reasonable. Okay, I'll change here to check it in advance. Let me address this in a following patch (added TODO comment). https://codereview.chromium.org/248803003/diff/140001/content/browser/service... content/browser/service_worker/service_worker_database.cc:440: bool ServiceWorkerDatabase::BumpNextAvailableIdIfNeeded( On 2014/04/25 00:48:21, michaeln wrote: > As coded this function will have problems if an id is bumped twice in a batch > (which will be expected when we get to <resourceid,url> storage). It'd be nice > to cache the next values in this class anyway, read them once and never read > again, so bump becomes something like... > > if (next_xxx_ <= used_id) { > next_xxx_ = used_id + 1; > Write(key, next_xxx_; > } > > I think we could seed cached values in LazyOpen() too. When starting with an > empty db, don't bother reading them ever. Also don't bother writing any values > until Bump() is called. When starting with an existing db, no key means next_id > is "0". > > wdyt? Good idea! Addressed it.
https://codereview.chromium.org/248803003/diff/200001/content/browser/service... File content/browser/service_worker/service_worker_database.cc (left): https://codereview.chromium.org/248803003/diff/200001/content/browser/service... content/browser/service_worker/service_worker_database.cc:137: if (IsEmpty() && !PopulateInitialData()) { fyi: Removed all write operations from LazyOpen(). Current schema version will be written in the database when first write operation is issued (see WriteBatch()).
lgtm https://codereview.chromium.org/248803003/diff/140001/content/browser/service... File content/browser/service_worker/service_worker_database.cc (right): https://codereview.chromium.org/248803003/diff/140001/content/browser/service... content/browser/service_worker/service_worker_database.cc:94: GURL origin = GURL(data.scope_url()).GetOrigin(); I think the serialization format is an impl detail and there's enough extra baggage with types and how to construct with default values (like having to understand an entirely different language). Google3'ers probably makes a lot heavier use of protobuffers then chromium/blink'ers do. I'd definitely prefer to use simple structs with chromium friendly types in them. https://codereview.chromium.org/248803003/diff/200001/content/browser/service... File content/browser/service_worker/service_worker_database.cc (left): https://codereview.chromium.org/248803003/diff/200001/content/browser/service... content/browser/service_worker/service_worker_database.cc:137: if (IsEmpty() && !PopulateInitialData()) { On 2014/04/25 11:05:01, nhiroki wrote: > fyi: Removed all write operations from LazyOpen(). Current schema version will > be written in the database when first write operation is issued (see > WriteBatch()). fantastique ;) https://codereview.chromium.org/248803003/diff/200001/content/browser/service... File content/browser/service_worker/service_worker_database.cc (right): https://codereview.chromium.org/248803003/diff/200001/content/browser/service... content/browser/service_worker/service_worker_database.cc:106: GURL origin = GURL(data.scope_url()).GetOrigin(); could use input.scope.GetOrigin() https://codereview.chromium.org/248803003/diff/200001/content/browser/service... content/browser/service_worker/service_worker_database.cc:473: ?? const int kFirstValidVersion = 1; if (parsed < kFirstValidVersion) { HandleError(..., leveldb::Status::Corruption("whoa")); return false; } https://codereview.chromium.org/248803003/diff/200001/content/browser/service... File content/browser/service_worker/service_worker_database.h (right): https://codereview.chromium.org/248803003/diff/200001/content/browser/service... content/browser/service_worker/service_worker_database.h:30: class ServiceWorkerResourceRecord; looks like these aren't needed in the .h file anymore https://codereview.chromium.org/248803003/diff/200001/content/browser/service... content/browser/service_worker/service_worker_database.h:132: bool IsEmpty(); IsEmpty() can be deleted here https://codereview.chromium.org/248803003/diff/200001/content/browser/service... File content/browser/service_worker/service_worker_database_unittest.cc (right): https://codereview.chromium.org/248803003/diff/200001/content/browser/service... content/browser/service_worker/service_worker_database_unittest.cc:177: // |origin4| has another registration, so should not remove it from the nit: this confused me i thought origin4 would be a different value, but origin4 == origin3. i think it would be more clear to use origin3 in all place where origin4 is used, and to comment that origin3 has two registrations in it
https://codereview.chromium.org/248803003/diff/200001/content/browser/service... File content/browser/service_worker/service_worker_database.cc (right): https://codereview.chromium.org/248803003/diff/200001/content/browser/service... content/browser/service_worker/service_worker_database.cc:106: GURL origin = GURL(data.scope_url()).GetOrigin(); On 2014/04/26 00:29:02, michaeln wrote: > could use input.scope.GetOrigin() Done. https://codereview.chromium.org/248803003/diff/200001/content/browser/service... content/browser/service_worker/service_worker_database.cc:473: On 2014/04/26 00:29:02, michaeln wrote: > ?? > > const int kFirstValidVersion = 1; > if (parsed < kFirstValidVersion) { > HandleError(..., leveldb::Status::Corruption("whoa")); > return false; > } Done. https://codereview.chromium.org/248803003/diff/200001/content/browser/service... File content/browser/service_worker/service_worker_database.h (right): https://codereview.chromium.org/248803003/diff/200001/content/browser/service... content/browser/service_worker/service_worker_database.h:30: class ServiceWorkerResourceRecord; On 2014/04/26 00:29:02, michaeln wrote: > looks like these aren't needed in the .h file anymore Done. https://codereview.chromium.org/248803003/diff/200001/content/browser/service... content/browser/service_worker/service_worker_database.h:132: bool IsEmpty(); On 2014/04/26 00:29:02, michaeln wrote: > IsEmpty() can be deleted here Done. https://codereview.chromium.org/248803003/diff/200001/content/browser/service... File content/browser/service_worker/service_worker_database_unittest.cc (right): https://codereview.chromium.org/248803003/diff/200001/content/browser/service... content/browser/service_worker/service_worker_database_unittest.cc:177: // |origin4| has another registration, so should not remove it from the On 2014/04/26 00:29:02, michaeln wrote: > nit: this confused me i thought origin4 would be a different value, but origin4 > == origin3. i think it would be more clear to use origin3 in all place where > origin4 is used, and to comment that origin3 has two registrations in it Done.
The CQ bit was checked by nhiroki@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nhiroki@chromium.org/248803003/240001
The CQ bit was unchecked by nhiroki@chromium.org
The CQ bit was checked by nhiroki@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nhiroki@chromium.org/248803003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_x64_rel
The CQ bit was checked by nhiroki@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nhiroki@chromium.org/248803003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by nhiroki@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nhiroki@chromium.org/248803003/260001
Message was sent while issue was closed.
Change committed as 266760 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
