Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
633273002:
Added quota client for serviceworker. Enables 'clear past <time> data'. (Closed)
|
Created:
6 years, 2 months ago by dmurph Modified:
6 years, 2 months ago CC:
chromium-reviews, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, jam, kinuko+serviceworker, nhiroki, darin-cc_chromium.org, horo+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdded quota client for serviceworker. Enables 'clear past <time> data'.
BUG=419287
Committed: https://crrev.com/f6916df879bf23bacf994880359c980f4249e324
Cr-Commit-Position: refs/heads/master@{#299918}
Committed: https://crrev.com/c80cbb840ad0ba704fdfa913135adcdcf8ba94b4
Cr-Commit-Position: refs/heads/master@{#300405}
Patch Set 1 #
Total comments: 71
Patch Set 2 : Logic to Core #
Total comments: 35
Patch Set 3 : Fix test, merge, style, and debug stuff #Patch Set 4 : Small fixes #Patch Set 5 : Rebased #Patch Set 6 : Removed unnecessary IO thread switch #
Total comments: 2
Patch Set 7 : final #Patch Set 8 : Fix for crashing, added IsAlive on wrapper #
Total comments: 2
Patch Set 9 : Moved alive check to wrapper. #Patch Set 10 : Rebase #Messages
Total messages: 42 (11 generated)
dmurph@chromium.org changed reviewers: + jsbell@chromium.org, michaeln@chromium.org
Hey Guys, PTAL and let me know if I should do any architectural changes. Tested working locally, but there may be an issue w/ the quota manager that I'm exploring.
On 2014/10/08 01:41:24, dmurph wrote: > Hey Guys, > > PTAL and let me know if I should do any architectural changes. > > Tested working locally, but there may be an issue w/ the quota manager that I'm > exploring. Found the issue, it was with IndexedDB. It reports kQuotaErrorNotSupported for some reason for deleting data in past 'time'
Thanks for tackling this! Needs tests (as you're working on). Lots of style nits - `git cl format` is your friend. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:58: if (quota_manager_proxy) { When is this NULL? https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:190: namespace { nit: I think we like having blank lines inside at the start/end of namespace {} blocks. Whatever clang-format says, though. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:200: const bool* success, const ServiceWorkerContext::ResultCallback& callback) { nit: Per chromium style, put each arg on a separate line unless the whole declaration fits on one line. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:214: void EmptySuccessCallback(bool success) {} nit: leave this with { } wrapped to minimize diff https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:237: std::set<GURL> applicable_patterns; For brevity, how about just 'scopes' ? (We'll probably rename 'pattern' to 'scope' generally at some point, to match the spec.) https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:243: if (origin == registration_info.pattern.GetOrigin()) { nit: can drop the braces here https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_context_wrapper.h (right): https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.h:78: void DeleteForOrigin(const GURL& origin_url, const ResultCallback& done); `done` is a bit unusual - it should probably be a noun. Usually `continuation`, `callback` or `done_callback`. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.h:117: const ResultCallback& done, nit: callbacks usually come first or last... is there a reason to have it in the middle? https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_quota_client.cc (right): https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.cc:1: #include "content/browser/service_worker/service_worker_quota_client.h" Needs copyright header https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.cc:9: namespace content { nit: blanks inside namespace {} (I think) https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.cc:17: if (restrict_on_host && info.origin.host() != host) { nit: don't need braces here https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.cc:33: if (info.origin == origin) { nit: don't need braces here https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.cc:37: // FIXME: Expose real usage numbers; Chromium style is: TODO(dmurph): ... Can you document what is currently being reported? (e.g. 1 registration = 1 byte?) https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.cc:67: if (type != storage::StorageType::kStorageTypePersistent) { My bad: should be kStorageTypeTemporary (here and below) https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.cc:111: nit: remove extra blank line https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_quota_client.h (right): https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.h:8: #include "storage/browser/quota/quota_client.h" nit: include order https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.h:18: nit: remove extra blank line https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.h:20: virtual ID id() const OVERRIDE; nit: 'OVERRIDE' -> 'override' https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_storage.cc:464: storage::StorageType::kStorageTypePersistent, Persistent->Temporary (again, sorry, my bad) https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_version.h:298: base::Time creation_time_; Where is this used?
On 2014/10/08 01:57:14, dmurph wrote: > Found the issue, it was with IndexedDB. It reports kQuotaErrorNotSupported for > some reason for deleting data in past 'time' Can you file a bug w/ repro on this?
What meta data do we want to expose around dates/times/sizes? Looks like that should be reflected in ServiceWorkerRegistrationInfo and ServiceWorkerVersionInfo and also in ServiceWorkerUsageInfo for the public api. At the level of the public api, does this make sense? create_date() // when first registered? modification_date() // when last updated? last_access_date() // when last used in some capacity? size() // byte cound of script resources saved to disk? https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:193: if (!success) { nit: {}'s not needed https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:201: if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) { Do you ever expect this to be called on some other thread? It looks like you can DCHECK that you're on the IO thread. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:210: delete success; I'm not sure it's safe to assume that the final callback will get called. You might want to look at wither scope_ptr<bool> and base::Passed or base::Owned to get the closure to handled deletion of success when the closure is finally destructed. See comments in callback.h for usage. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:247: bool *success = new bool(true); maybe call this overall_success for clarity throughout to differentiate from a single success? https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:248: base::Closure barrier = base::BarrierClosure( neat, hadn't seen barrier closure before https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:250: base::Bind(&SuccessReportingCallback, success, done)); base::Owned(overall_success) here or maybe base::Passed(&localscopedptr_to_success)? https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:254: pattern, base::Bind(&SuccessCollectorCallback, barrier, success)); base::Unretained(overall_success) here? https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_quota_client.cc (right): https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.cc:30: DLOG(ERROR) << "Getting origin " << origin.spec(); This and other in here aren't really error logging. Maybe back these out prior to landing once you're comfortable with the control flow and convinced its doing what you expect. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.cc:38: callback.Run(serviceworkers); Be careful with reporting bogus values to the quota manger, i don't think it tolerates inconsistencies very well. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.cc:71: context_->GetAllOriginsInfo(base::Bind(&ReportUsage, callback, origin)); Might be nice to have a more focused getter for a single origin that ultimately makes use of ServiceWorkerDatabase::GetRegistrationsForOrigin. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_quota_client.h (right): https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.h:36: friend class content::ServiceWorkerContextWrapper; is the content:: qualifier needed? https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_storage.cc:461: quota_manager_proxy_->NotifyStorageModified( We'll also have to call NotifyStorageModified for deletions and do *something* in the DeleteAndStartOver case to start afresh. FindRegistrationForXXX calls that find something might be a good place to call NotifyStorageAccessed. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_version.cc:104: creation_time_(base::Time::Now()), oh, i thought this would mean when version was created in storage? What meta data do we want to expose around dates/times/sizes? Looks like that should be reflected in ServiceWorkerRegistrationInfo and ServiceWorkerVersionInfo and also in ServiceWorkerUsageInfo for the public api. At the level of the public api, does this make sense? create_date() // when first registered? modification_date() // when last updated? last_access_date() // when last used in some capacity? size() // byte cound of script resources saved to disk?
https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:280: void ServiceWorkerContextWrapper::InitInternal( > Please give architectural input I think it'd be nicer of the 'client' class where implemented in terms of the 'core' instead of the 'wrapper'? I think of the 'wrapper' as a necessary evil in order to interop with stuff in chrome the run on the ui thread. Mostly sw core logic is happily single threaded on the IO thread. The 'client' class fits that bill as well as the QM proper. I'd be nice to eliminate the rawptr held by the client to the wrapper. The 'core' supports a weakptr and the client is only used on the io thread. Maybe the initialization has two parts. - creation on the ui thread and handed to RegisterClient on the UI thread - then give it a weakptr to the core in here You'd have to be careful about handing it off to RegistereClient and then retaining a ptr of some kind in order to poke at it again in InitInternal, but RefPtr to the rescue for that.
Lots of replies, and a couple questions. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:58: if (quota_manager_proxy) { On 2014/10/08 19:26:41, jsbell wrote: > When is this NULL? Other classes checked if the proxy was null, but it looks like that shouldn't be possible. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:190: namespace { On 2014/10/08 19:26:41, jsbell wrote: > nit: I think we like having blank lines inside at the start/end of namespace {} > blocks. Whatever clang-format says, though. clang-format didn't change it https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:193: if (!success) { On 2014/10/08 23:09:15, michaeln wrote: > nit: {}'s not needed I would like them https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:200: const bool* success, const ServiceWorkerContext::ResultCallback& callback) { On 2014/10/08 19:26:40, jsbell wrote: > nit: Per chromium style, put each arg on a separate line unless the whole > declaration fits on one line. Done. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:201: if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) { On 2014/10/08 23:09:15, michaeln wrote: > Do you ever expect this to be called on some other thread? It looks like you can > DCHECK that you're on the IO thread. Yes, this comes from the database runner, so I need to switch it back onto the io thread for the callback (or else quotamanager will blow up). https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:210: delete success; On 2014/10/08 23:09:15, michaeln wrote: > I'm not sure it's safe to assume that the final callback will get called. You > might want to look at wither scope_ptr<bool> and base::Passed or base::Owned to > get the closure to handled deletion of success when the closure is finally > destructed. See comments in callback.h for usage. Done. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:214: void EmptySuccessCallback(bool success) {} On 2014/10/08 19:26:41, jsbell wrote: > nit: leave this with { } wrapped to minimize diff Done. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:237: std::set<GURL> applicable_patterns; On 2014/10/08 19:26:40, jsbell wrote: > For brevity, how about just 'scopes' ? > > (We'll probably rename 'pattern' to 'scope' generally at some point, to match > the spec.) Done. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:243: if (origin == registration_info.pattern.GetOrigin()) { On 2014/10/08 19:26:41, jsbell wrote: > nit: can drop the braces here I would rather keep them :) https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:247: bool *success = new bool(true); On 2014/10/08 23:09:15, michaeln wrote: > maybe call this overall_success for clarity throughout to differentiate from a > single success? Done. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:248: base::Closure barrier = base::BarrierClosure( On 2014/10/08 23:09:15, michaeln wrote: > neat, hadn't seen barrier closure before Acknowledged. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:250: base::Bind(&SuccessReportingCallback, success, done)); On 2014/10/08 23:09:15, michaeln wrote: > base::Owned(overall_success) here or maybe > base::Passed(&localscopedptr_to_success)? > Done. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:254: pattern, base::Bind(&SuccessCollectorCallback, barrier, success)); On 2014/10/08 23:09:15, michaeln wrote: > base::Unretained(overall_success) here? Unneeded. Only applicable to refcounted objects or where the object where the method is being called on isn't refcounted. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:280: void ServiceWorkerContextWrapper::InitInternal( On 2014/10/08 23:28:46, michaeln wrote: > > Please give architectural input > > I think it'd be nicer of the 'client' class where implemented in terms of the > 'core' instead of the 'wrapper'? I think of the 'wrapper' as a necessary evil in > order to interop with stuff in chrome the run on the ui thread. Mostly sw core > logic is happily single threaded on the IO thread. The 'client' class fits that > bill as well as the QM proper. > > I'd be nice to eliminate the rawptr held by the client to the wrapper. The > 'core' supports a weakptr and the client is only used on the io thread. Maybe > the initialization has two parts. > - creation on the ui thread and handed to RegisterClient on the UI thread > - then give it a weakptr to the core in here > You'd have to be careful about handing it off to RegistereClient and then > retaining a ptr of some kind in order to poke at it again in InitInternal, but > RefPtr to the rescue for that. After offline discussion, we decided that the functionality should move to the core (unregister multiple workers), but the client should hook up to the wrapper for easy testability (easy to mock out wrapper). https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_context_wrapper.h (right): https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.h:78: void DeleteForOrigin(const GURL& origin_url, const ResultCallback& done); On 2014/10/08 19:26:41, jsbell wrote: > `done` is a bit unusual - it should probably be a noun. Usually `continuation`, > `callback` or `done_callback`. Done. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.h:117: const ResultCallback& done, On 2014/10/08 19:26:41, jsbell wrote: > nit: callbacks usually come first or last... is there a reason to have it in the > middle? Done. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_quota_client.cc (right): https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.cc:1: #include "content/browser/service_worker/service_worker_quota_client.h" On 2014/10/08 19:26:41, jsbell wrote: > Needs copyright header Done. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.cc:9: namespace content { On 2014/10/08 19:26:41, jsbell wrote: > nit: blanks inside namespace {} (I think) clang format left it alone https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.cc:17: if (restrict_on_host && info.origin.host() != host) { On 2014/10/08 19:26:41, jsbell wrote: > nit: don't need braces here I would like to keep them https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.cc:30: DLOG(ERROR) << "Getting origin " << origin.spec(); On 2014/10/08 23:09:15, michaeln wrote: > This and other in here aren't really error logging. Maybe back these out prior > to landing once you're comfortable with the control flow and convinced its doing > what you expect. Done. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.cc:33: if (info.origin == origin) { On 2014/10/08 19:26:41, jsbell wrote: > nit: don't need braces here I would like to keep https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.cc:37: // FIXME: Expose real usage numbers; On 2014/10/08 19:26:41, jsbell wrote: > Chromium style is: TODO(dmurph): ... > > Can you document what is currently being reported? (e.g. 1 registration = 1 > byte?) Done. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.cc:38: callback.Run(serviceworkers); On 2014/10/08 23:09:15, michaeln wrote: > Be careful with reporting bogus values to the quota manger, i don't think it > tolerates inconsistencies very well. Cool, I changed this to always report 0, so having deltas of 0 won't mess stuff up. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.cc:67: if (type != storage::StorageType::kStorageTypePersistent) { On 2014/10/08 19:26:41, jsbell wrote: > My bad: should be kStorageTypeTemporary (here and below) Done. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.cc:71: context_->GetAllOriginsInfo(base::Bind(&ReportUsage, callback, origin)); On 2014/10/08 23:09:15, michaeln wrote: > Might be nice to have a more focused getter for a single origin that ultimately > makes use of ServiceWorkerDatabase::GetRegistrationsForOrigin. Isn't that what this does? https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.cc:111: On 2014/10/08 19:26:41, jsbell wrote: > nit: remove extra blank line Done. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_quota_client.h (right): https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.h:8: #include "storage/browser/quota/quota_client.h" On 2014/10/08 19:26:41, jsbell wrote: > nit: include order Done. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.h:18: On 2014/10/08 19:26:41, jsbell wrote: > nit: remove extra blank line Done. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.h:20: virtual ID id() const OVERRIDE; On 2014/10/08 19:26:41, jsbell wrote: > nit: 'OVERRIDE' -> 'override' Done. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_quota_client.h:36: friend class content::ServiceWorkerContextWrapper; On 2014/10/08 23:09:15, michaeln wrote: > is the content:: qualifier needed? Done. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_storage.cc:461: quota_manager_proxy_->NotifyStorageModified( On 2014/10/08 23:09:16, michaeln wrote: > We'll also have to call NotifyStorageModified for deletions and do *something* > in the DeleteAndStartOver case to start afresh. FindRegistrationForXXX calls > that find something might be a good place to call NotifyStorageAccessed. We don't care about deletions for cache clearing, right? There are a variety of other quota manager methods that can be called as well, including: NotifyOriginInUse NotifyOriginNoLongerInUse NotifyStorageAccessed I'm thinking it might be better to grab these in a following patch. What do you think? https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_storage.cc:464: storage::StorageType::kStorageTypePersistent, On 2014/10/08 19:26:41, jsbell wrote: > Persistent->Temporary (again, sorry, my bad) Done. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_version.cc:104: creation_time_(base::Time::Now()), On 2014/10/08 23:09:16, michaeln wrote: > oh, i thought this would mean when version was created in storage? > > What meta data do we want to expose around dates/times/sizes? Looks like that > should be reflected in ServiceWorkerRegistrationInfo and > ServiceWorkerVersionInfo and also in ServiceWorkerUsageInfo for the public api. > > At the level of the public api, does this make sense? > create_date() // when first registered? > modification_date() // when last updated? > last_access_date() // when last used in some capacity? > size() // byte cound of script resources saved to disk? Mind if I do this in a next patch? https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_version.h:298: base::Time creation_time_; On 2014/10/08 19:26:41, jsbell wrote: > Where is this used? Oops, removed.
A few nits. Can you double check that I'm correct about the reason the test case passes? https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_context_core.cc (right): https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_context_core.cc:222: callback.Run(SERVICE_WORKER_ERROR_ABORT); Should this be posted instead of running synchronously? It'd be unusual (from the caller's perspective) to be async sometimes and sync others. https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_context_core.cc:266: for (std::vector<ServiceWorkerRegistrationInfo>::const_iterator it = This looks like a great place to use range-based for + auto: for (const auto& registration_info : registrations) { ... } https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_context_unittest.cc (right): https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_context_unittest.cc:280: GURL origin1_p3("http://www.example.com:8080/again"); http://example.com and http://example.com:8080 are different origins (origin is scheme + host + port), so this naming is a bit odd.. https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_context_unittest.cc:344: origin1, I believe this is only succeeding because registration_id3 doesn't actually belong to origin1, so it's NOT_FOUND because it never existed, not because the unregister succeeded. Maybe change all of these to use the original GURL and call .GetOrigin(), rather than origin1 / origin2 to avoid this? https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_context_wrapper.cc:188: void ConvertUnregisterToBool( How about: StatusCodeToBoolCallbackAdapter ? https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_database.cc (right): https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_database.cc:218: DLOG(ERROR) << "Scope URL '" << data.scope_url() << "' and/or script url '" Why add this during this CL? https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_quota_client.cc (right): https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_quota_client.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. Per http://www.chromium.org/developers/coding-style no '(c)' https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_quota_client.h (right): https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_quota_client.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. Per http://www.chromium.org/developers/coding-style no '(c)' https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_quota_client.h:40: explicit ServiceWorkerQuotaClient(ServiceWorkerContextWrapper* context); nit: can you wrap after the ( so that 'CONTENT_EXPORT explicit ServiceWorkerQuotaClient(' are on one line ? https://codereview.chromium.org/633273002/diff/20001/storage/browser/quota/qu... File storage/browser/quota/quota_manager.cc (right): https://codereview.chromium.org/633273002/diff/20001/storage/browser/quota/qu... storage/browser/quota/quota_manager.cc:563: static_cast<int>((*iter)->id()))); Is the logging below the only reason for passing the ID through? If so, remove? If not, consider changing this to a range-based for loop, e.g. for(const QuotaClient* client : manager()->clients_) { ... } https://codereview.chromium.org/633273002/diff/20001/storage/browser/quota/qu... storage/browser/quota/quota_manager.cc:573: DLOG(ERROR) << "Error count " << error_count_; remove? https://codereview.chromium.org/633273002/diff/20001/storage/browser/quota/qu... storage/browser/quota/quota_manager.cc:592: DLOG(ERROR) << "Got status " << status << " from client " << id; remove? https://codereview.chromium.org/633273002/diff/20001/storage/browser/quota/qu... storage/browser/quota/quota_manager.cc:649: DLOG(ERROR) << "Error count " << error_count_; remove?
https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_context_core.cc (right): https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_context_core.cc:222: callback.Run(SERVICE_WORKER_ERROR_ABORT); On 2014/10/13 20:54:25, jsbell wrote: > Should this be posted instead of running synchronously? It'd be unusual (from > the caller's perspective) to be async sometimes and sync others. Not changing to match implementations above, as discussed. https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_context_core.cc:266: for (std::vector<ServiceWorkerRegistrationInfo>::const_iterator it = On 2014/10/13 20:54:25, jsbell wrote: > This looks like a great place to use range-based for + auto: > > for (const auto& registration_info : registrations) { > ... > } Done. https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_context_unittest.cc (right): https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_context_unittest.cc:280: GURL origin1_p3("http://www.example.com:8080/again"); On 2014/10/13 20:54:25, jsbell wrote: > http://example.com and http://example.com:8080 are different origins (origin is > scheme + host + port), so this naming is a bit odd.. Done. https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_context_unittest.cc:344: origin1, On 2014/10/13 20:54:25, jsbell wrote: > I believe this is only succeeding because registration_id3 doesn't actually > belong to origin1, so it's NOT_FOUND because it never existed, not because the > unregister succeeded. > > Maybe change all of these to use the original GURL and call .GetOrigin(), rather > than origin1 / origin2 to avoid this? Done. https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_context_wrapper.cc:188: void ConvertUnregisterToBool( On 2014/10/13 20:54:25, jsbell wrote: > How about: StatusCodeToBoolCallbackAdapter ? Done. https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_database.cc (right): https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_database.cc:218: DLOG(ERROR) << "Scope URL '" << data.scope_url() << "' and/or script url '" On 2014/10/13 20:54:25, jsbell wrote: > Why add this during this CL? I ran into this during testing, it should make it easier for other to spot mistakes they make in tests in the future. https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_quota_client.cc (right): https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_quota_client.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/10/13 20:54:25, jsbell wrote: > Per http://www.chromium.org/developers/coding-style no '(c)' Done. https://codereview.chromium.org/633273002/diff/20001/storage/browser/quota/qu... File storage/browser/quota/quota_manager.cc (right): https://codereview.chromium.org/633273002/diff/20001/storage/browser/quota/qu... storage/browser/quota/quota_manager.cc:563: static_cast<int>((*iter)->id()))); On 2014/10/13 20:54:26, jsbell wrote: > Is the logging below the only reason for passing the ID through? > > If so, remove? > > If not, consider changing this to a range-based for loop, e.g. > > for(const QuotaClient* client : manager()->clients_) { > ... > } Removed, sorry, debugging stuff still around. https://codereview.chromium.org/633273002/diff/20001/storage/browser/quota/qu... storage/browser/quota/quota_manager.cc:573: DLOG(ERROR) << "Error count " << error_count_; On 2014/10/13 20:54:26, jsbell wrote: > remove? Done. https://codereview.chromium.org/633273002/diff/20001/storage/browser/quota/qu... storage/browser/quota/quota_manager.cc:592: DLOG(ERROR) << "Got status " << status << " from client " << id; On 2014/10/13 20:54:26, jsbell wrote: > remove? Done. https://codereview.chromium.org/633273002/diff/20001/storage/browser/quota/qu... storage/browser/quota/quota_manager.cc:649: DLOG(ERROR) << "Error count " << error_count_; On 2014/10/13 20:54:26, jsbell wrote: > remove? Done.
https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:122: BrowserThread::UI, Not related to your cl... Oh, the wrapper class is coded up such that it assumes the Register and Unregister methods are invoked on the UI thread. So it's intent on delivering the completion callbacks on the UI thread too. That behavior actually conflicts with the doc comments in the public .h file. Originally reg/unreg where intended to be called on the UI thread, but it we decided to make the interface callabled from the IO thread instead when it came time to add the 'GetInfo' methods (except for the Terminate method). Apparently the pre-existing impl of reg/unreg wasn't modified accordingly. This class is crufty :( I don't think there is a caller of the public reg/unreg methods atm. https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_storage.cc:461: quota_manager_proxy_->NotifyStorageModified( On 2014/10/11 00:02:26, dmurph wrote: > On 2014/10/08 23:09:16, michaeln wrote: > > We'll also have to call NotifyStorageModified for deletions and do *something* > > in the DeleteAndStartOver case to start afresh. FindRegistrationForXXX calls > > that find something might be a good place to call NotifyStorageAccessed. > > We don't care about deletions for cache clearing, right? I believe we do. > There are a variety of other quota manager methods that can be called as well, > including: > NotifyOriginInUse > NotifyOriginNoLongerInUse > NotifyStorageAccessed > > I'm thinking it might be better to grab these in a following patch. What do you > think? sgtm https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_context_core.cc (right): https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_context_core.cc:233: namespace { please put the anon namespace helpers up at the top of the file, there are some places where we don't do that but overwhelmingly they go up top https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_context_core.cc:246: if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) { You should be able to DCHECK_CURRENTLY_ON(IO) right here. Unreg completion callbacks are raised on the IO thread. This class s/b entirely single threaded, and if it isn't, thats a bug. https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_context_core.cc:282: job_coordinator_->Unregister( The core::unreg() method does some work upon completion of the unregistration, it notifies outside agencies about the unregistration. Are you intentionally skipping that part? If not, looks like you could call core::unreg here instead coordination->unreg. https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_quota_client.h (right): https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_quota_client.h:42: scoped_refptr<ServiceWorkerContextWrapper> context_; ty! https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:462: if (quota_manager_proxy_.get() != nullptr) { nit: don't need to comparison to nullptr
dmurph@chromium.org changed reviewers: + sky@chromium.org
dmurph@chromium.org changed required reviewers: + michaeln@chromium.org, sky@chromium.org
+sky for owners. Michael is on vacation today, we're aiming to try to get this CL in tomorrow (10/15) https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/633273002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_context_wrapper.cc:122: BrowserThread::UI, On 2014/10/14 00:50:31, michaeln wrote: > Not related to your cl... > > Oh, the wrapper class is coded up such that it assumes the Register and > Unregister methods are invoked on the UI thread. So it's intent on delivering > the completion callbacks on the UI thread too. > > That behavior actually conflicts with the doc comments in the public .h file. > Originally reg/unreg where intended to be called on the UI thread, but it we > decided to make the interface callabled from the IO thread instead when it came > time to add the 'GetInfo' methods (except for the Terminate method). Apparently > the pre-existing impl of reg/unreg wasn't modified accordingly. This class is > crufty :( > > I don't think there is a caller of the public reg/unreg methods atm. Should I add a TODO anywhere? https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_context_core.cc (right): https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_context_core.cc:233: namespace { On 2014/10/14 00:50:31, michaeln wrote: > please put the anon namespace helpers up at the top of the file, there are some > places where we don't do that but overwhelmingly they go up top Done. https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_context_core.cc:246: if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) { On 2014/10/14 00:50:31, michaeln wrote: > You should be able to DCHECK_CURRENTLY_ON(IO) right here. Unreg completion > callbacks are raised on the IO thread. This class s/b entirely single threaded, > and if it isn't, thats a bug. It's a bug then. This callback is called from the database helper. I can fix that in another CL if you like. TODO added https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_context_core.cc:282: job_coordinator_->Unregister( On 2014/10/14 00:50:31, michaeln wrote: > The core::unreg() method does some work upon completion of the unregistration, > it notifies outside agencies about the unregistration. Are you intentionally > skipping that part? If not, looks like you could call core::unreg here instead > coordination->unreg. Done. https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_quota_client.h (right): https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_quota_client.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/10/13 20:54:26, jsbell wrote: > Per http://www.chromium.org/developers/coding-style no '(c)' Done. https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_quota_client.h:40: explicit ServiceWorkerQuotaClient(ServiceWorkerContextWrapper* context); On 2014/10/13 20:54:26, jsbell wrote: > nit: can you wrap after the ( so that 'CONTENT_EXPORT explicit > ServiceWorkerQuotaClient(' are on one line ? Done. https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/633273002/diff/20001/content/browser/service_... content/browser/service_worker/service_worker_storage.cc:462: if (quota_manager_proxy_.get() != nullptr) { On 2014/10/14 00:50:31, michaeln wrote: > nit: don't need to comparison to nullptr Done.
What specific files do you need me to review?
Whoops, nevermind, git cl upload thought I needed to add you but it looks like that's not needed for gypi files. Sorry!
dmurph@chromium.org changed reviewers: - sky@chromium.org
dmurph@chromium.org changed required reviewers: - sky@chromium.org
removing sky
lgtm https://codereview.chromium.org/633273002/diff/280001/content/browser/service... File content/browser/service_worker/service_worker_context_wrapper.h (right): https://codereview.chromium.org/633273002/diff/280001/content/browser/service... content/browser/service_worker/service_worker_context_wrapper.h:78: const ResultCallback& done); since this method isn't in the public ServiceWorkerContext interface, maybe move it out from under the comment suggesting otherwise
https://codereview.chromium.org/633273002/diff/280001/content/browser/service... File content/browser/service_worker/service_worker_context_wrapper.h (right): https://codereview.chromium.org/633273002/diff/280001/content/browser/service... content/browser/service_worker/service_worker_context_wrapper.h:78: const ResultCallback& done); On 2014/10/15 22:21:36, michaeln wrote: > since this method isn't in the public ServiceWorkerContext interface, maybe move > it out from under the comment suggesting otherwise Done.
The CQ bit was checked by dmurph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/633273002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dmurph@chromium.org changed reviewers: + sky@chromium.org
dmurph@chromium.org changed required reviewers: + sky@chromium.org
Adding sky@ for content/browser/storage_partition_impl.cc approval.
LGTM
The CQ bit was checked by dmurph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/633273002/300001
Message was sent while issue was closed.
Committed patchset #7 (id:300001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/f6916df879bf23bacf994880359c980f4249e324 Cr-Commit-Position: refs/heads/master@{#299918}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:300001) has been created in https://codereview.chromium.org/654323003/ by kareng@google.com. The reason for reverting is: causing crashes https://code.google.com/p/chromium/issues/detail?id=424831.
Hey Michael, I caused some crashing because the wrapper was shutdown, yet the quota manager was still using the client. I added an IsAlive method to the wrapper which the quota client now checks, and a test to verify this logic. Can you take a quick look at my changes? FYI sky: no changes for the file you reviewed. Thanks, Daniel
https://codereview.chromium.org/633273002/diff/320001/content/browser/service... File content/browser/service_worker/service_worker_quota_client.cc (right): https://codereview.chromium.org/633273002/diff/320001/content/browser/service... content/browser/service_worker/service_worker_quota_client.cc:63: !context_->IsAlive()) { Wdyt about putting this test into wrapper class directly, so other call to wrapper.GetAllOriginsInfo and .DeleteForOrigin that may happen in other code paths are also taken care of? We may not need to new IsAlive method if those methods fended for themselves.
Switched to handling 'alive' in the wrapper. https://codereview.chromium.org/633273002/diff/320001/content/browser/service... File content/browser/service_worker/service_worker_quota_client.cc (right): https://codereview.chromium.org/633273002/diff/320001/content/browser/service... content/browser/service_worker/service_worker_quota_client.cc:63: !context_->IsAlive()) { On 2014/10/20 20:08:22, michaeln wrote: > Wdyt about putting this test into wrapper class directly, so other call to > wrapper.GetAllOriginsInfo and .DeleteForOrigin that may happen in other code > paths are also taken care of? We may not need to new IsAlive method if those > methods fended for themselves. I'm good with that.
thnx, lgtm and fyi jkarlin is trying to reland his quota client patch right now too.
The CQ bit was checked by dmurph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/633273002/360001
Message was sent while issue was closed.
Committed patchset #10 (id:360001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/c80cbb840ad0ba704fdfa913135adcdcf8ba94b4 Cr-Commit-Position: refs/heads/master@{#300405} |
