Closed Bug 1294400 Opened 9 years ago Closed 8 years ago

Implement Storage Pressure event to notify UI

Categories

(Core :: Storage: Quota Manager, defect, P2)

Product:
Core
Component:
Storage: Quota Manager
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: shawnjohnjr, Assigned: shawnjohnjr)

References

(Blocks 1 open bug)

Details

(Whiteboard: [storage-v1])

Attachments

(2 files, 20 obsolete files)

Bug 1294400 - Implement Storage Pressure event to notify UI, r=janv
3.70 KB, patch
Details | Diff | Splinter Review
Bug 1294400 - Add test cases for storage pressure event, r=janv
4.26 KB, patch
Details | Diff | Splinter Review
Implement Storage pressure event to notify UI 6.1. Storage Pressure When the user agent notices it comes under storage pressure and it cannot free up sufficient space by clearing network storage and non-persistent boxes within site storage, then the user agent should alert the user and offer a way to clear persistent boxes.
I guess this is considered in V1 scope, so set P2. Please feel free to change it accordingly.
Priority: -- → P2
(In reply to Shawn Huang [:shawnjohnjr] from comment #0) > Implement Storage pressure event to notify UI > > 6.1. Storage Pressure > > When the user agent notices it comes under storage pressure and it cannot > free up sufficient space by clearing network storage and non-persistent > boxes within site storage, then the user agent should alert the user and > offer a way to clear persistent boxes. Hi Tina, Do you have any UX specification how we can define storage pressure? It's pretty vague for me here, since this is storage api v1 scope.
Global limit is calculated as 50% of free disk space. Origin eviction: deleting entire origin's worth of data until the storage amount goes under the global limit. Group limit: 20% of the global limit. Each origin is part of a group (group of origins). There's one group for each eTLD+1 domain. The group limit is also called the "hard limit": it doesn't trigger origin eviction. The global limit is a "soft limit" since there's a chance that some space will be freed and the operation can continue. If the group limit is exceeded, or if origin eviction couldn't free enough space, the browser will throw a QuotaExceededError.
Whiteboard: storage-v1
We need to notify total usage and current usage for UI.
(In reply to Shawn Huang [:shawnjohnjr] from comment #4) > We need to notify total usage and current usage for UI. Per online talk with Fisher, we should have a pressure event with extra usage numbers.
Assignee: nobody → shuang
Blocks: 1323395
I have wip patch now, but it depends on bug 1298329.
Hi Fischer The WIP patch attached sends pressure event, topic: "QuotaManager::StoragePressure". You have to apply patches in bug 1298329 first.
Flags: needinfo?(fliu)
Attached patch 0001-WIP-1323395.patch (obsolete) — — Splinter Review
Hi Shawn, Based on the agreement, the contract of storage pressure event is - subject is null - topic is "QuotaManager::StoragePressure" - data is estimated disk space usage in Bytes This WIP test patch observes the storage pressure event, then, - if the firefox-used space < 5GB, then warn user to free some disk space - if the firefox-used space >= 5GB, then warn user to clear some data stored on firefox by websites Please try, thanks. P.S: - The UI is not final - The handling of consecutive pressure events is not yet done
Flags: needinfo?(fliu) → needinfo?(shuang)
I guess I have to create new test case, currently we don't have mochitest to test disk full case, only xpcshell.
Sorry for delay. I still need more time to add mochitest test case. I will provide tests for you as soon as possible.
Flags: needinfo?(shuang)
I hit this assertion during test. Assertion failure: uint64_t(aDest) >= uint64_t(aArg), at /home/shawnjohnjr/code/mozilla-inbound/dom/quota/ActorsParent.cpp:1251 #01: nsTArray_Impl<RefPtr<mozilla::dom::quota::OriginInfo>, nsTArrayInfallibleAllocator>::operator[](unsigned long) (/home/shawnjohnjr/code/mozilla-inbound/obj-x86_64-pc-linux-gnu/dist/include/nsTArray.h:1198) #02: mozilla::dom::quota::QuotaManager::LockedRemoveQuotaForOrigin(mozilla::dom::quota::PersistenceType, nsACString_internal const&, nsACString_internal const&) (/home/shawnjohnjr/code/mozilla-inbound/dom/quota/ActorsParent.cpp:5180) #03: mozilla::dom::quota::QuotaManager::RemoveQuotaForOrigin(mozilla::dom::quota::PersistenceType, nsACString_internal const&, nsACString_internal const&) (/home/shawnjohnjr/code/mozilla-inbound/dom/quota/QuotaManager.h:171) #04: mozilla::dom::quota::(anonymous namespace)::OriginClearOp::DoDirectoryWork(mozilla::dom::quota::QuotaManager*) (/home/shawnjohnjr/code/mozilla-inbound/dom/quota/ActorsParent.cpp:6751 (discriminator 2)) #05: DirectoryWork (/home/shawnjohnjr/code/mozilla-inbound/dom/quota/ActorsParent.cpp:5785) #06: nsThread::ProcessNextEvent(bool, bool*) (/home/shawnjohnjr/code/mozilla-inbound/xpcom/threads/nsThread.cpp:1213) #07: NS_ProcessNextEvent(nsIThread*, bool) (/home/shawnjohnjr/code/mozilla-inbound/xpcom/glue/nsThreadUtils.cpp:381) #08: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) (/home/shawnjohnjr/code/mozilla-inbound/ipc/glue/MessagePump.cpp:368) #09: MessageLoop::RunInternal() (/home/shawnjohnjr/code/mozilla-inbound/ipc/chromium/src/base/message_loop.cc:233) #10: MessageLoop::AutoRunState::~AutoRunState() (/home/shawnjohnjr/code/mozilla-inbound/ipc/chromium/src/base/message_loop.cc:490) #11: nsThread::ThreadFunc(void*) (/home/shawnjohnjr/code/mozilla-inbound/xpcom/threads/nsThread.cpp:469) #12: _pt_root (/home/shawnjohnjr/code/mozilla-inbound/nsprpub/pr/src/pthreads/ptthread.c:219) #13: start_thread (/build/glibc-t3gR2i/glibc-2.23/nptl/pthread_create.c:333) #14: __clone (/build/glibc-t3gR2i/glibc-2.23/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:111) #15: ??? (???:???)
http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#5253 void GroupInfo::LockedRemoveOriginInfo(const nsACString& aOrigin) { AssertCurrentThreadOwnsQuotaMutex(); for (uint32_t index = 0; index < mOriginInfos.Length(); index++) { if (mOriginInfos[index]->mOrigin == aOrigin) { AssertNoUnderflow(mUsage, mOriginInfos[index]->mUsage);
(In reply to Shawn Huang [:shawnjohnjr] from comment #14) > I hit this assertion during test. > > Assertion failure: uint64_t(aDest) >= uint64_t(aArg), at > /home/shawnjohnjr/code/mozilla-inbound/dom/quota/ActorsParent.cpp:1251 > #01: nsTArray_Impl<RefPtr<mozilla::dom::quota::OriginInfo>, > nsTArrayInfallibleAllocator>::operator[](unsigned long) > (/home/shawnjohnjr/code/mozilla-inbound/obj-x86_64-pc-linux-gnu/dist/include/ > nsTArray.h:1198) I found this is because i did not apply the latest patch in bug 1298329. So it's not a problem.
Hey Fischer, I've tested the current patch pressure event and I can see pressure event dialog pop out.
(In reply to Shawn Huang [:shawnjohnjr] from comment #19) > Hey Fischer, > I've tested the current patch pressure event and I can see pressure event > dialog pop out. Thanks Shawn. Just applied [1] and other related bugs patches. Able to observe the QuotaManager::StoragePressure event and the storage pressure notification can appear. [1] attachment 8828206 [details] [diff] [review]: bug-1294400.patch
(In reply to Fischer [:Fischer] from comment #21) > (In reply to Shawn Huang [:shawnjohnjr] from comment #19) > > Hey Fischer, > > I've tested the current patch pressure event and I can see pressure event > > dialog pop out. > Thanks Shawn. > Just applied [1] and other related bugs patches. Able to observe the > QuotaManager::StoragePressure event and the storage pressure notification > can appear. > > [1] attachment 8828206 [details] [diff] [review]: bug-1294400.patch Great!
Since we created dom/quota/test for mochitests. I think we don't need to add it in indexeddb folder anymore. I will revise this patch.
(In reply to Shawn Huang [:shawnjohnjr] from comment #23) > Since we created dom/quota/test for mochitests. I think we don't need to add > it in indexeddb folder anymore. I will revise this patch. I was wrong. Moving out tests from indexeddb folder requires a lot of helper functions to quota folder.
Hi Fischer, Did your test cases cover the prompt shown after pressure event triggered? I guess I shall convert this test case from mochitest to chrome browser test.
Flags: needinfo?(fliu)
Whiteboard: storage-v1 → [storage-v1]
Component: DOM → DOM: Quota Manager
Comment on attachment 8848012 [details] [diff] [review] Bug 1294400 - Implement Storage Pressure event to notify UI Review of attachment 8848012 [details] [diff] [review]: ----------------------------------------------------------------- Bug 1298329 landed, except for test_persist_quotaExceeded.js, I wonder if we can review c++ first? As I said in the storage meeting, it would make more sense to use the same test in Bug 1298329 to reduce testing time. So I added into test_persist_quotaExceeded.js. As Jan suggested in the storage meeting two months ago, it should be ok to just notify frontend via NotifyObserver.
Attachment #8848012 - Flags: feedback?(btseng)
Comment on attachment 8848012 [details] [diff] [review] Bug 1294400 - Implement Storage Pressure event to notify UI Cancel f?, since Bevis is going to have business trip and can't feedback over 10 days.
Attachment #8848012 - Flags: feedback?(btseng)
Comment on attachment 8848012 [details] [diff] [review] Bug 1294400 - Implement Storage Pressure event to notify UI Review of attachment 8848012 [details] [diff] [review]: ----------------------------------------------------------------- Hi Jan, I'm not sure if you have time to review c++ first. As I said in the storage meeting last week, it would make more sense to use the same test in Bug 1298329 to reduce testing time. So I added into test_persist_quotaExceeded.js. As you suggested in the storage meeting two months ago, it should be ok to just notify frontend via NotifyObserver. And let frontend side set timer to decide when to show warning dialog to the users. This what Bug 1323395 already implemented, and landed. The requirement is to notify observer with the current usage. I'm a bit worry about the direction is wrong. Please let me know if you have any concern in advance.
Attachment #8848012 - Flags: review?(jvarga)
Yes, I do have time. I'll take a look.
Comment on attachment 8848012 [details] [diff] [review] Bug 1294400 - Implement Storage Pressure event to notify UI Review of attachment 8848012 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/test/unit/test_persist_quotaExceeded.js @@ +37,5 @@ > const dataSize = 1024 * 1024; > > let success = false; > + > + Cu.import("resource://gre/modules/Services.jsm"); This usually goes to the top of the file. @@ +195,5 @@ > // The persisted db is only bounded by global limit so we can write at least > // one object into it. > yield* fillUpDatabase(databases[1], true /* expectedAbleToStore */); > > + ok(receivedPressureEvent, "Pressure event received"); So here you expect the event was received, but there are other tests below that expect the same. I'm not sure if you expect just one event or multiple events, if the latter then you need to reset the variable somewhere. ::: dom/quota/ActorsParent.cpp @@ +2963,5 @@ > quotaManager->LockedCollectOriginsForEviction(delta, locks); > > if (!sizeToBeFreed) { > + // Notify pressure event. > + quotaManager->LockedDispatchStoragePressureEvent(); I think we shouldn't hold the lock while dispatching runnables.
Attachment #8848012 - Flags: review?(jvarga) → feedback+
(In reply to Jan Varga [:janv] from comment #37) > Comment on attachment 8848012 [details] [diff] [review] > Bug 1294400 - Implement Storage Pressure event to notify UI > > Review of attachment 8848012 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/indexedDB/test/unit/test_persist_quotaExceeded.js > @@ +195,5 @@ > > // The persisted db is only bounded by global limit so we can write at least > > // one object into it. > > yield* fillUpDatabase(databases[1], true /* expectedAbleToStore */); > > > > + ok(receivedPressureEvent, "Pressure event received"); > > So here you expect the event was received, but there are other tests below > that expect the same. > I'm not sure if you expect just one event or multiple events, if the latter > then you need to reset the variable somewhere. I will reset the variable. > > ::: dom/quota/ActorsParent.cpp > @@ +2963,5 @@ > > quotaManager->LockedCollectOriginsForEviction(delta, locks); > > > > if (!sizeToBeFreed) { > > + // Notify pressure event. > > + quotaManager->LockedDispatchStoragePressureEvent(); > > I think we shouldn't hold the lock while dispatching runnables. Is it ok to add MutexAutoUnlock like what |LockedCollectOriginsForEviction| did? In |QuotaManager::LockedDispatchStoragePressureEvent|, { MutexAutoUnlock autoUnlock(mQuotaMutex); // This function might be called by different thread. RefPtr<StoragePressureRunnable> storagePressureRunnable = new StoragePressureRunnable(mTemporaryStorageUsage); MOZ_ALWAYS_SUCCEEDS(NS_DispatchToMainThread(storagePressureRunnable)); }
(In reply to Shawn Huang [:shawnjohnjr] from comment #38) > > @@ +2963,5 @@ > > > quotaManager->LockedCollectOriginsForEviction(delta, locks); > > > > > > if (!sizeToBeFreed) { > > > + // Notify pressure event. > > > + quotaManager->LockedDispatchStoragePressureEvent(); > > > > I think we shouldn't hold the lock while dispatching runnables. > > Is it ok to add MutexAutoUnlock like what |LockedCollectOriginsForEviction| > did? Yes, something like that... > > In |QuotaManager::LockedDispatchStoragePressureEvent|, > > { > MutexAutoUnlock autoUnlock(mQuotaMutex); > > // This function might be called by different thread. > > RefPtr<StoragePressureRunnable> storagePressureRunnable = > new StoragePressureRunnable(mTemporaryStorageUsage); > MOZ_ALWAYS_SUCCEEDS(NS_DispatchToMainThread(storagePressureRunnable)); > } but I'm not sure if we need a separate method for this, mTemporaryStorageUsage can be accessed from QuotaObject too.
(In reply to Jan Varga [:janv] from comment #39) > (In reply to Shawn Huang [:shawnjohnjr] from comment #38) > > > > In |QuotaManager::LockedDispatchStoragePressureEvent|, > > > > { > > MutexAutoUnlock autoUnlock(mQuotaMutex); > > > > // This function might be called by different thread. > > > > RefPtr<StoragePressureRunnable> storagePressureRunnable = > > new StoragePressureRunnable(mTemporaryStorageUsage); > > MOZ_ALWAYS_SUCCEEDS(NS_DispatchToMainThread(storagePressureRunnable)); > > } > > but I'm not sure if we need a separate method for this, > mTemporaryStorageUsage can be accessed from QuotaObject too. Oh. Yes, you're totally right. I will remove the extra method and move it into QuotaObject.
Comment on attachment 8859594 [details] [diff] [review] Bug 1294400 - Implement Storage Pressure event to notify UI (v2) Review of attachment 8859594 [details] [diff] [review]: ----------------------------------------------------------------- Fix issues that are addressed in Comment 37, Comment 39.
Attachment #8859594 - Flags: review?(jvarga)
Comment on attachment 8859594 [details] [diff] [review] Bug 1294400 - Implement Storage Pressure event to notify UI (v2) Review of attachment 8859594 [details] [diff] [review]: ----------------------------------------------------------------- Regarding the commit message, I appreciate that you pay attention to current discussion in dev-platform. However, I checked https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Checkin_comment and it says "Please say what changes are made, not what problem was fixed" Your commit message starts ok, but I'm not sure if we are supposed to copy parts of specifications. Something like this would be enough I think: "Notify observers that we cannot free up sufficient space. Pass the current usage as the observer subject." ::: dom/indexedDB/test/unit/test_persist_quotaExceeded.js @@ +8,2 @@ > var testGenerator = testSteps(); > +let receivedPressureEvent = false; I think it would be nicer if fillUpDatabase() took this as an output argument. Then you don't have to reset it each time. ::: dom/quota/ActorsParent.cpp @@ +2882,5 @@ > + return NS_ERROR_FAILURE; > + } > + > + nsAutoString usage(NS_ConvertUTF8toUTF16( > + nsPrintfCString("%" PRIu64, mUsage))); We can pass this as a number (not a string). See NotifyChannelActiveRunnable in AudioChannelService.cpp @@ +3032,5 @@ > > uint64_t sizeToBeFreed = > quotaManager->LockedCollectOriginsForEviction(delta, locks); > > if (!sizeToBeFreed) { So, where's the autounlock stuff ?
Attachment #8859594 - Flags: review?(jvarga)
(In reply to Jan Varga [:janv] from comment #43) > Comment on attachment 8859594 [details] [diff] [review] > Bug 1294400 - Implement Storage Pressure event to notify UI (v2) > > Review of attachment 8859594 [details] [diff] [review]: > ----------------------------------------------------------------- > > Regarding the commit message, I appreciate that you pay attention to current > discussion in dev-platform. > However, I checked > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/ > Committing_Rules_and_Responsibilities#Checkin_comment > and it says "Please say what changes are made, not what problem was fixed" > > Your commit message starts ok, but I'm not sure if we are supposed to copy > parts of specifications. > > Something like this would be enough I think: > "Notify observers that we cannot free up sufficient space. Pass the current > usage as the observer subject." > Originally I want to express "why" we have to make this change in the commit message, just because the storage specification suggests to do. I agree that it's not ok to copy parts of specifications. I will remove it. > ::: dom/indexedDB/test/unit/test_persist_quotaExceeded.js > @@ +8,2 @@ > > var testGenerator = testSteps(); > > +let receivedPressureEvent = false; > > I think it would be nicer if fillUpDatabase() took this as an output > argument. > Then you don't have to reset it each time. Sure. I will do it. > > ::: dom/quota/ActorsParent.cpp > @@ +2882,5 @@ > > + return NS_ERROR_FAILURE; > > + } > > + > > + nsAutoString usage(NS_ConvertUTF8toUTF16( > > + nsPrintfCString("%" PRIu64, mUsage))); > > We can pass this as a number (not a string). > > See NotifyChannelActiveRunnable in AudioChannelService.cpp Okay. I will change it. > > @@ +3032,5 @@ > > > > uint64_t sizeToBeFreed = > > quotaManager->LockedCollectOriginsForEviction(delta, locks); > > > > if (!sizeToBeFreed) { > > So, where's the autounlock stuff ? I misunderstood the last comment. I will add it.
Comment on attachment 8859991 [details] [diff] [review] Bug 1294400 - Implement Storage Pressure event to notify UI (v3) Review of attachment 8859991 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/ActorsParent.cpp @@ +2881,5 @@ > + if (NS_WARN_IF(!obsSvc)) { > + return NS_ERROR_FAILURE; > + } > + > + nsAutoString usage(mUsage); "... not a string" Did you check NotifyChannelActiveRunnable::Run() in AudioChannelService.cpp ? Especially code around nsISupportsPRUint64 Is there a reason to not use it ? The other patch will have to be modified too, to process the subject (not data) in Observe()
(In reply to Jan Varga [:janv] from comment #47) > Comment on attachment 8859991 [details] [diff] [review] > Bug 1294400 - Implement Storage Pressure event to notify UI (v3) > > Review of attachment 8859991 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/quota/ActorsParent.cpp > @@ +2881,5 @@ > > + if (NS_WARN_IF(!obsSvc)) { > > + return NS_ERROR_FAILURE; > > + } > > + > > + nsAutoString usage(mUsage); > > "... not a string" > > Did you check NotifyChannelActiveRunnable::Run() in AudioChannelService.cpp ? > Especially code around nsISupportsPRUint64 > > Is there a reason to not use it ? > > The other patch will have to be modified too, to process the subject (not > data) in Observe() My bad. I did not know I should put mUsage into subject in Comment 43, I guess I misunderstood again. I can't make connection between mWindowID and mUsage, since mUsage is just usage data not like id in subject. I can make the change. And I will also need to make change for browser_storagePressure_notification.js. Since Bug 1323395 already landed.
It seems to me that we usually carry identifiers in subject such as inner/outer window ids, child process ids. Is mUsage suitable for the purposes of this convention?
Well, maybe I'm missing something here. I always thought that the observer service and nsIObserver is just a generic way how to send and receive notifications. There are 3 arguments here: subject, topic and data Since it's a generic service, we can use the subject and data as we want. The usage is a 64bit number, it can be quite big, so serializing and deserializing into a string is something we can try to eliminate. That's why a suggested to use the subject for it. Please, ping someone else to make sure that you can use the subject here.
Hi baku, Can we use subject to wrap the current disk usage in observer? It seems to me that we usually carry identifiers in subject such as inner/outer window ids, child process ids. Can you help to comment?
Flags: needinfo?(amarchesini)
Comment on attachment 8859991 [details] [diff] [review] Bug 1294400 - Implement Storage Pressure event to notify UI (v3) Review of attachment 8859991 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/ActorsParent.cpp @@ +2884,5 @@ > + > + nsAutoString usage(mUsage); > + obsSvc->NotifyObservers(nullptr, > + "QuotaManager::StoragePressure", > + usage.get()); I totally agree with janv, please use nsISupportsPRUint64 instead a string. Just do: nsCOMPtr<nsISupportsPRUint64> wrapper = do_CreateInstance(NS_SUPPORTS_PRUINT64_CONTRACTID); if (NS_WARN_IF(!wrapper)) { return NS_ERROR_FAILURE; } wrapper->SetData(mUsage); return obsSvc->NotifyObservers(wrapper, "QuotaManager::StoragePressure", u""); @@ +2885,5 @@ > + nsAutoString usage(mUsage); > + obsSvc->NotifyObservers(nullptr, > + "QuotaManager::StoragePressure", > + usage.get()); > + return NS_OK; return what NotifyObservers returns
I see. I will change on both sides.
Flags: needinfo?(amarchesini)
Attachment #8859991 - Attachment is obsolete: true
Attachment #8859991 - Flags: review?(jvarga)
Comment on attachment 8860061 [details] [diff] [review] Bug 1294400 - Implement Storage Pressure event to notify UI (v3) Review of attachment 8860061 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/test/unit/test_persist_quotaExceeded.js @@ +8,2 @@ > var testGenerator = testSteps(); > +let receivedPressureEvent = false; This is now only accessed in fillUpDatabase(), I would place it after |let success = false| @@ +30,5 @@ > is(request.resultCode, Components.results.NS_OK, "Persisted succeeded"); > is(request.result, true, "The origin is persisted"); > } > > +function* fillUpDatabase(db, expectedAtLeastOneAdd, resetFlag) What about renaming resetFlag to expectedPressureEvent ? @@ +74,5 @@ > > info("Got abort event"); > > + if (resetFlag) { > + ok(receivedPressureEvent, "Pressure event received"); "Pressure event was received" @@ +75,5 @@ > info("Got abort event"); > > + if (resetFlag) { > + ok(receivedPressureEvent, "Pressure event received"); > + receivedPressureEvent = false; You don't need to reset the flag here if you move the declaration as I suggested above. @@ +197,5 @@ > info("Fill the persisted db"); > > // The persisted db is only bounded by global limit so we can write at least > // one object into it. > + yield* fillUpDatabase(databases[1], true /* expectedAtLeastOneAdd */, true); replace with: yield* fillUpDatabase(databases[1], /* expectedAtLeastOneAdd */ true, /* expectedPressureEvent */ true); here and below ::: dom/quota/ActorsParent.cpp @@ +2854,5 @@ > /******************************************************************************* > + * Quota object class declarations > + ******************************************************************************/ > + > +class QuotaObject::StoragePressureRunnable final All declarations are grouped in the beginning of the file. I would place this before: |class QuotaManager::CreateRunnable final| @@ +2857,5 @@ > + > +class QuotaObject::StoragePressureRunnable final > + : public Runnable > +{ > + uint64_t mUsage; Nit: const uint64_t mUsage @@ +2873,5 @@ > +}; > + > +NS_IMETHODIMP > +QuotaObject:: > +StoragePressureRunnable::Run() This should go before: void QuotaObject::AddRef() {
Attachment #8860061 - Flags: review?(jvarga)
(In reply to Jan Varga [:janv] from comment #56) > Comment on attachment 8860061 [details] [diff] [review] > Bug 1294400 - Implement Storage Pressure event to notify UI (v3) > > Review of attachment 8860061 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/indexedDB/test/unit/test_persist_quotaExceeded.js > @@ +8,2 @@ > > var testGenerator = testSteps(); > > +let receivedPressureEvent = false; > > This is now only accessed in fillUpDatabase(), I would place it after |let > success = false| Sure. > > @@ +30,5 @@ > > is(request.resultCode, Components.results.NS_OK, "Persisted succeeded"); > > is(request.result, true, "The origin is persisted"); > > } > > > > +function* fillUpDatabase(db, expectedAtLeastOneAdd, resetFlag) > > What about renaming resetFlag to expectedPressureEvent ? I will improve variable naming next time. > > @@ +74,5 @@ > > > > info("Got abort event"); > > > > + if (resetFlag) { > > + ok(receivedPressureEvent, "Pressure event received"); > > "Pressure event was received" > > @@ +75,5 @@ > > info("Got abort event"); > > > > + if (resetFlag) { > > + ok(receivedPressureEvent, "Pressure event received"); > > + receivedPressureEvent = false; > > You don't need to reset the flag here if you move the declaration as I > suggested above. Sure. > > @@ +197,5 @@ > > info("Fill the persisted db"); > > > > // The persisted db is only bounded by global limit so we can write at least > > // one object into it. > > + yield* fillUpDatabase(databases[1], true /* expectedAtLeastOneAdd */, true); > > replace with: > yield* fillUpDatabase(databases[1], > /* expectedAtLeastOneAdd */ true, > /* expectedPressureEvent */ true); Ok. > here and below > > ::: dom/quota/ActorsParent.cpp > @@ +2854,5 @@ > > /******************************************************************************* > > + * Quota object class declarations > > + ******************************************************************************/ > > + > > +class QuotaObject::StoragePressureRunnable final > > All declarations are grouped in the beginning of the file. > I would place this before: > |class QuotaManager::CreateRunnable final| Ok. I thought I should put class declarations next to QuotaObject. I will try to avoid this style issue again. > @@ +2857,5 @@ > > + > > +class QuotaObject::StoragePressureRunnable final > > + : public Runnable > > +{ > > + uint64_t mUsage; > > Nit: const uint64_t mUsage > > @@ +2873,5 @@ > > +}; > > + > > +NS_IMETHODIMP > > +QuotaObject:: > > +StoragePressureRunnable::Run() > > This should go before: > void > QuotaObject::AddRef() > { Sorry, I will pay more attention to this kind of style issues.
Thanks for your comments. Fix issues addressed in Comment 56.
Attachment #8860354 - Flags: review?(jvarga)
Comment on attachment 8860354 [details] [diff] [review] Bug 1294400 - Implement Storage Pressure event to notify UI (v4) Review of attachment 8860354 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks! Now I have to actually finish review of test_persist_quotaExceeded.js
Attachment #8860354 - Flags: review?(jvarga) → review+
(In reply to Jan Varga [:janv] from comment #59) > Comment on attachment 8860354 [details] [diff] [review] > Bug 1294400 - Implement Storage Pressure event to notify UI (v4) > > Review of attachment 8860354 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good, thanks! > Now I have to actually finish review of test_persist_quotaExceeded.js Thank you for your patience. I will rebase once you finish reviewing test_persist_quotaExceeded.js.
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1745eb467987598982454f284612758b0c30332&selectedJob=93628352 eslint complained that: TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/dom/indexedDB/test/unit/test_persist_quotaExceeded.js:43:3 | addObserver's third parameter can be omitted when it's false. (mozilla/no-useless-parameters) I will remove the third parameter.
Pushed by shuang@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2538c8b58798 Implement Storage Pressure event to notify UI, r=janv
Track test case in Bug 1389380.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: