Closed
Bug 961049
Opened 10 years ago
Closed 9 years ago
Update quota manager to use PBackground
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: janv, Assigned: janv)
References
(Depends on 1 open bug, Blocks 2 open bugs)
Details
Attachments
(16 files)
338.54 KB,
patch
|
Details | Diff | Splinter Review | |
9.15 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
34.74 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
5.15 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
4.50 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
7.12 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
166.48 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
69.47 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
39.05 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
9.42 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
1.58 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
1.71 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
1.12 KB,
patch
|
Details | Diff | Splinter Review | |
17.05 KB,
patch
|
Details | Diff | Splinter Review | |
5.14 KB,
patch
|
Details | Diff | Splinter Review | |
2.15 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•10 years ago
|
Depends on: PBackground
Blocks: webgl-shader-cache
Assignee | ||
Comment 1•9 years ago
|
||
Kyle, Andrea, let me know which one of you is more comfortable with reviewing this patch. Thanks.
Assignee: nobody → Jan.Varga
Status: NEW → ASSIGNED
Flags: needinfo?(khuey)
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 2•9 years ago
|
||
Comment on attachment 8675278 [details] [diff] [review] patch Luke, please review dom/asmjscache changes. Ben, please review dom/cache changes.
Attachment #8675278 -
Flags: review?(luke)
Attachment #8675278 -
Flags: review?(bkelly)
Comment 3•9 years ago
|
||
can you split the patch in smaller patches? It seems to me that there are a lot of independent code. Small patches are easy to follow (at least for me). Thanks!
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 4•9 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #3) > can you split the patch in smaller patches? It seems to me that there are a > lot of independent code. Small patches are easy to follow (at least for me). > Thanks! Ok, so are you willing to review all the changes (split into smaller patches), except dom/asmjscache and dom/cache ?
Comment 5•9 years ago
|
||
Comment on attachment 8675278 [details] [diff] [review] patch Since Jan is splitting patches, dropping the review flag for now.
Attachment #8675278 -
Flags: review?(bkelly)
Assignee | ||
Updated•9 years ago
|
Attachment #8675278 -
Flags: review?(luke)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8675808 -
Flags: review?(luke)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8675810 -
Flags: review?(bkelly)
Comment 8•9 years ago
|
||
Comment on attachment 8675810 [details] [diff] [review] Part 6: Quota Manager on PBackground cache changes Review of attachment 8675810 [details] [diff] [review]: ----------------------------------------------------------------- Nice! I don't see any major issues, just some extra cleanup we can do. r=me with comments. ::: dom/cache/Context.cpp @@ +378,5 @@ > + nsCOMPtr<nsIPrincipal> principal = managerId->Principal(); > + nsresult rv = QuotaManager::GetInfoFromPrincipal(principal, > + &mQuotaInfo.mGroup, > + &mQuotaInfo.mOrigin, > + &mQuotaInfo.mIsApp); Please file a follow-up bug to remove this GetInfoFromPrincipal(). Instead, we should just save all this information when we create the ManagerId: https://dxr.mozilla.org/mozilla-central/source/dom/cache/ManagerId.cpp#27 This will let us avoid a main thread dispatch. Please set the follow-up bug to block bug 1110136. ::: dom/cache/Manager.cpp @@ +248,2 @@ > > + // The factory was destroyed between when abort started on main thread and Is this comment still accurate? It seems this can't happen any more because its all on the background thread. @@ +314,1 @@ > StaticMutexAutoLock lock(sMutex); Do we still need sMutex? It seems everything is running on a single thread now.
Attachment #8675810 -
Flags: review?(bkelly) → review+
I'll let baku do the first pass, and I can review if more is needed.
Flags: needinfo?(khuey)
Updated•9 years ago
|
Attachment #8675808 -
Flags: review?(luke) → review+
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8676866 -
Flags: review?(amarchesini)
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8676868 -
Flags: review?(amarchesini)
Updated•9 years ago
|
Attachment #8676866 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8676871 -
Flags: review?(amarchesini)
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8676875 -
Flags: review?(amarchesini)
Assignee | ||
Updated•9 years ago
|
Attachment #8675808 -
Attachment description: dom/asmjscache changes → Part 5: QuotaManager on PBackground asmjscache changes
Assignee | ||
Updated•9 years ago
|
Attachment #8675810 -
Attachment description: dom/cache changes → Part 6: Quota Manager on PBackground cache changes
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #8676880 -
Flags: review?(amarchesini)
Assignee | ||
Comment 15•9 years ago
|
||
Attachment #8676881 -
Flags: review?(amarchesini)
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8676882 -
Flags: review?(amarchesini)
Comment 17•9 years ago
|
||
Comment on attachment 8676868 [details] [diff] [review] Part 2: Remove Utilities.h Review of attachment 8676868 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/UsageInfo.h @@ +81,5 @@ > + static void > + IncrementUsage(uint64_t* aUsage, uint64_t aDelta) > + { > + // Watch for overflow! > + if ((UINT64_MAX - *aUsage) < aDelta) { This will not work. do: MOZ_ASSERT(aUsage); CheckUint64 value = *aUsage; value += aData; if (value.isValid()) { *aUsage = value.value(); } else { *aUsage = UINT64_MAX; }
Attachment #8676868 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 18•9 years ago
|
||
Attachment #8676883 -
Flags: review?(amarchesini)
Updated•9 years ago
|
Attachment #8676871 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 19•9 years ago
|
||
Latest try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c521dbd16743 Just some usual intermittent failures and permaoranges.
Comment 20•9 years ago
|
||
Comment on attachment 8676875 [details] [diff] [review] Part 4: QuotaManager on PBackground core changes Review of attachment 8676875 [details] [diff] [review]: ----------------------------------------------------------------- Let me know what you think about this approach of managing the state machine. The rest looks good. As usual :) ::: dom/quota/ActorsChild.cpp @@ +38,5 @@ > + > +#ifdef DEBUG > + > +void > +QuotaChild::AssertIsOnOwningThread() const I have seen this pattern quite often in your code. Would be nice to have a base class for this. @@ +57,5 @@ > + > + if (mService) { > + mService->ClearBackgroundActor(); > +#ifdef DEBUG > + mService = nullptr; any reason to do it only in debug builds? @@ +154,5 @@ > + AssertIsOnOwningThread(); > + > + if (mRequest) { > + mRequest->ClearBackgroundActor(); > +#ifdef DEBUG ditto. @@ +204,5 @@ > + > +#ifdef DEBUG > + > +void > +QuotaRequestChild::AssertIsOnOwningThread() const yep, definitely would be nice to have a base class :) @@ +223,5 @@ > + mRequest->SetError(aResponse); > +} > + > +void > +QuotaRequestChild::HandleResponse(const RequestResponse& aResponse) don't pass this argument if you don't want to use it. Are you planning to use this in other patches? If not, maybe a better name can be chosen. @@ +252,5 @@ > + case RequestResponse::TClearOriginResponse: > + case RequestResponse::TClearAppResponse: > + case RequestResponse::TClearAllResponse: > + case RequestResponse::TResetAllResponse: > + HandleResponse(aResponse); HandleResponse(); ::: dom/quota/ActorsChild.h @@ +38,5 @@ > +#endif > + > +public: > + void > + AssertIsOnOwningThread() const Yep, a QuotaDebugging class ? ::: dom/quota/ActorsParent.cpp @@ +347,5 @@ > + > + void > + AddCallback(nsIRunnable* aCallback) > + { > + AssertIsOnOwningThread(); MOZ_ASSERT(aCallback); @@ +374,5 @@ > + > +class QuotaManager::ShutdownRunnable final > + : public nsRunnable > +{ > + bool& mDone; // touched only on the main-thread. @@ +1033,5 @@ > + virtual bool > + Init(Quota* aQuota); > + > +protected: > + QuotaRequestBase(bool aExclusive) explicit @@ +1081,3 @@ > }; > > class OriginClearOp final ? @@ +2230,5 @@ > + nsTArray<nsCOMPtr<nsIRunnable>> callbacks; > + mCallbacks.SwapElements(callbacks); > + > + for (nsCOMPtr<nsIRunnable>& callback : callbacks) { > + callback->Run(); do you care about the error result here? @@ +2263,5 @@ > + case State::Completed: > + default: > + MOZ_CRASH("Bad state!"); > + } > + I think this state machine can be improved. What about if: struct StateMap { State mState; bool mMainThread; } sStateMap[] = { { Initial, true }, { CreatingManager, false }, { RegisteringObserver, true }, { CallingCallbacks, false } }; here you do: switch (mState) { case State::Initial: ManageResult(Init()); break; case CreatingManager: ManageREsult(CreateManager()); ... } void ManageResult(nsresult rv) { if (NS_SUCCEEDED(rv)) { mState = sStateMap[mState+1].mState; if (sStateMap[mState].mMainThread) { Dispatch here or there(); } } else if (mState != CallingCallbacks) { mState = CallingCallbacks(); ... } and you don't change the state in each function. @@ +4939,5 @@ > + MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(this))); > + } else { > + AdvanceState(); > + > + nsresult rv = FinishInit(); Instead calling this directly, can you just do: AdvanceState(); Run(); @@ +4963,5 @@ > + AdvanceState(); > + > + MOZ_ALWAYS_TRUE(NS_SUCCEEDED(mOwningThread->Dispatch(this, > + NS_DISPATCH_NORMAL))); > + Again, I find this management of the state machine very confusing. I prefer to have a table of State-Thread and let 1 single function to manage the advanceState plus the dispatching. ::: dom/quota/QuotaManagerService.cpp @@ +104,5 @@ > +protected: > + RefPtr<RequestBase> mRequest; > + > +public: > + PendingRequestInfo(RequestBase* aRequest) explicit ? ::: dom/quota/QuotaRequests.cpp @@ +91,5 @@ > + return NS_OK; > +} > + > +UsageRequest::UsageRequest(nsIPrincipal* aPrincipal, > + nsIQuotaUsageCallback* aCallback) indentation @@ +161,5 @@ > + > +NS_IMETHODIMP > +UsageRequest::GetFileUsage(uint64_t* aFileUsage) > +{ > + AssertIsOnOwningThread(); MOZ_ASSERT(aFileUsage); @@ +184,5 @@ > + > +NS_IMETHODIMP > +UsageRequest::SetCallback(nsIQuotaUsageCallback* aCallback) > +{ > + AssertIsOnOwningThread(); MOZ_ASSERT(aCallback); ? Do we care?
Attachment #8676875 -
Flags: review?(amarchesini) → review+
Updated•9 years ago
|
Attachment #8676880 -
Flags: review?(amarchesini) → review+
Comment 21•9 years ago
|
||
Comment on attachment 8676881 [details] [diff] [review] Part 8: Move getFileReferences() from PContent under new protocol PBackgroundIndexedDBUtils Review of attachment 8676881 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/ActorsChild.cpp @@ +3101,5 @@ > + > + if (mManager) { > + mManager->ClearBackgroundActor(); > +#ifdef DEBUG > + mManager = nullptr; why only in DEBUG mode? ::: dom/indexedDB/ActorsParent.cpp @@ +27203,5 @@ > +GetFileReferencesHelper::DispatchAndReturnFileReferences(int32_t* aMemRefCnt, > + int32_t* aDBRefCnt, > + int32_t* aSliceRefCnt, > + bool* aResult) > +{ MOZ_ASSERT(all the pointers?) ::: ipc/glue/BackgroundParentImpl.cpp @@ +197,5 @@ > auto > +BackgroundParentImpl::AllocPBackgroundIndexedDBUtilsParent() > + -> PBackgroundIndexedDBUtilsParent* > +{ > + using mozilla::dom::indexedDB::AllocPBackgroundIndexedDBUtilsParent; well... I don't see why you want to do it when you can do: return mozilla::dom::indexedDB::AllocPBackgroundIndexedUtilsParent(); or move it to the other using. @@ +209,5 @@ > +bool > +BackgroundParentImpl::DeallocPBackgroundIndexedDBUtilsParent( > + PBackgroundIndexedDBUtilsParent* aActor) > +{ > + using mozilla::dom::indexedDB::DeallocPBackgroundIndexedDBUtilsParent; ditto.
Attachment #8676881 -
Flags: review?(amarchesini) → review+
Comment 22•9 years ago
|
||
Comment on attachment 8676882 [details] [diff] [review] Part 9: Fix asynchronous deletion of files to work on PBackground Review of attachment 8676882 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/IndexedDatabaseManager.cpp @@ +746,5 @@ > +IndexedDatabaseManager::NoteBackgroundThread(nsIEventTarget* aBackgroundThread) > +{ > + MOZ_ASSERT(IsMainProcess()); > + MOZ_ASSERT(NS_IsMainThread()); > + MOZ_ASSERT(aBackgroundThread); MOZ_ASSERT(!mBackgroundThread || mBackgroundThread == aBackgroundThread) ?
Attachment #8676882 -
Flags: review?(amarchesini) → review+
Updated•9 years ago
|
Attachment #8676883 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 23•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5ac1bad0686
Assignee | ||
Comment 24•9 years ago
|
||
do_register_cleanup() is also used by XPCShell test harness to send "profile-before-change" and in this case the notification is sent before quota manager is created (and a profile-before-change observer is registered) so quota manager doesn't have a chance to do cleanup during shutdown, thus some runnables try to run after xpcom shutdown This particular test actually doesn't need to do its cleanup through registering a cleanup function, so I simply moved the cleanup to the end of the test.
Attachment #8690163 -
Flags: review?(amarchesini)
Assignee | ||
Comment 25•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=84a1f714dfd1
Updated•9 years ago
|
Attachment #8690163 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 26•9 years ago
|
||
Assignee | ||
Comment 27•9 years ago
|
||
Assignee | ||
Comment 28•9 years ago
|
||
Assignee | ||
Comment 29•9 years ago
|
||
Assignee | ||
Comment 30•9 years ago
|
||
(In reply to Ben Kelly [:bkelly] from comment #8) > Comment on attachment 8675810 [details] [diff] [review] > Part 6: Quota Manager on PBackground cache changes > > Review of attachment 8675810 [details] [diff] [review]: > ----------------------------------------------------------------- > > Nice! I don't see any major issues, just some extra cleanup we can do. > r=me with comments. > > ::: dom/cache/Context.cpp > @@ +378,5 @@ > > + nsCOMPtr<nsIPrincipal> principal = managerId->Principal(); > > + nsresult rv = QuotaManager::GetInfoFromPrincipal(principal, > > + &mQuotaInfo.mGroup, > > + &mQuotaInfo.mOrigin, > > + &mQuotaInfo.mIsApp); > > Please file a follow-up bug to remove this GetInfoFromPrincipal(). Instead, > we should just save all this information when we create the ManagerId: > > https://dxr.mozilla.org/mozilla-central/source/dom/cache/ManagerId.cpp#27 > > This will let us avoid a main thread dispatch. > > Please set the follow-up bug to block bug 1110136. Done, bug 1226900. > > ::: dom/cache/Manager.cpp > @@ +248,2 @@ > > > > + // The factory was destroyed between when abort started on main thread and > > Is this comment still accurate? It seems this can't happen any more because > its all on the background thread. Yeah, I remove the comment. > > @@ +314,1 @@ > > StaticMutexAutoLock lock(sMutex); > > Do we still need sMutex? It seems everything is running on a single thread > now. We actually don't need sBackgroundThread anymore, so I removed it.
Assignee | ||
Comment 31•9 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #17) > Comment on attachment 8676868 [details] [diff] [review] > Part 2: Remove Utilities.h > > Review of attachment 8676868 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/quota/UsageInfo.h > @@ +81,5 @@ > > + static void > > + IncrementUsage(uint64_t* aUsage, uint64_t aDelta) > > + { > > + // Watch for overflow! > > + if ((UINT64_MAX - *aUsage) < aDelta) { > > This will not work. > do: > > MOZ_ASSERT(aUsage); > CheckUint64 value = *aUsage; > value += aData; > if (value.isValid()) { > *aUsage = value.value(); > } else { > *aUsage = UINT64_MAX; > } Ok, fixed.
Assignee | ||
Comment 32•9 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #20) > Comment on attachment 8676875 [details] [diff] [review] > Part 4: QuotaManager on PBackground core changes > > Review of attachment 8676875 [details] [diff] [review]: > ----------------------------------------------------------------- > > Let me know what you think about this approach of managing the state machine. > The rest looks good. As usual :) Ok, thanks. I created a new method GetNextState() for it. I tried the sStateMap, but I think it would be too complicated/ugly. Enums can't be easily used as array indexes. > > ::: dom/quota/ActorsChild.cpp > @@ +38,5 @@ > > + > > +#ifdef DEBUG > > + > > +void > > +QuotaChild::AssertIsOnOwningThread() const > > I have seen this pattern quite often in your code. > Would be nice to have a base class for this. Yeah, I created BackgroundThreadObject class for this. > > @@ +57,5 @@ > > + > > + if (mService) { > > + mService->ClearBackgroundActor(); > > +#ifdef DEBUG > > + mService = nullptr; > > any reason to do it only in debug builds? This is a pattern that is used in indexedDB code, I think we don't need to clear it for the app to work correctly. It's just for debugging I think. > > @@ +154,5 @@ > > + AssertIsOnOwningThread(); > > + > > + if (mRequest) { > > + mRequest->ClearBackgroundActor(); > > +#ifdef DEBUG > > ditto. See my comment above. > > @@ +204,5 @@ > > + > > +#ifdef DEBUG > > + > > +void > > +QuotaRequestChild::AssertIsOnOwningThread() const > > yep, definitely would be nice to have a base class :) This one is a bit special since it forwards to parent protocol implementation and I didn't want to create super generic and super complicated base class. > > @@ +223,5 @@ > > + mRequest->SetError(aResponse); > > +} > > + > > +void > > +QuotaRequestChild::HandleResponse(const RequestResponse& aResponse) > > don't pass this argument if you don't want to use it. > Are you planning to use this in other patches? If not, maybe a better name > can be chosen. Ok, removed the arg, but I kept the name. > > @@ +252,5 @@ > > + case RequestResponse::TClearOriginResponse: > > + case RequestResponse::TClearAppResponse: > > + case RequestResponse::TClearAllResponse: > > + case RequestResponse::TResetAllResponse: > > + HandleResponse(aResponse); > > HandleResponse(); Ok. > > ::: dom/quota/ActorsChild.h > @@ +38,5 @@ > > +#endif > > + > > +public: > > + void > > + AssertIsOnOwningThread() const > > Yep, a QuotaDebugging class ? BackgroundThreadObject > > ::: dom/quota/ActorsParent.cpp > @@ +347,5 @@ > > + > > + void > > + AddCallback(nsIRunnable* aCallback) > > + { > > + AssertIsOnOwningThread(); > > MOZ_ASSERT(aCallback); Ok. > > @@ +374,5 @@ > > + > > +class QuotaManager::ShutdownRunnable final > > + : public nsRunnable > > +{ > > + bool& mDone; > > // touched only on the main-thread. Ok. > > @@ +1033,5 @@ > > + virtual bool > > + Init(Quota* aQuota); > > + > > +protected: > > + QuotaRequestBase(bool aExclusive) > > explicit Ok. > > @@ +1081,3 @@ > > }; > > > > class OriginClearOp > > final ? Ok. > > @@ +2230,5 @@ > > + nsTArray<nsCOMPtr<nsIRunnable>> callbacks; > > + mCallbacks.SwapElements(callbacks); > > + > > + for (nsCOMPtr<nsIRunnable>& callback : callbacks) { > > + callback->Run(); > > do you care about the error result here? Added Unused. > > @@ +2263,5 @@ > > + case State::Completed: > > + default: > > + MOZ_CRASH("Bad state!"); > > + } > > + > > I think this state machine can be improved. What about if: > > struct StateMap { > State mState; > bool mMainThread; > } sStateMap[] = { > { Initial, true }, > { CreatingManager, false }, > { RegisteringObserver, true }, > { CallingCallbacks, false } > }; > > here you do: > switch (mState) { > case State::Initial: > ManageResult(Init()); > break; > case CreatingManager: > ManageREsult(CreateManager()); > ... > } > > void ManageResult(nsresult rv) { > if (NS_SUCCEEDED(rv)) { > mState = sStateMap[mState+1].mState; > if (sStateMap[mState].mMainThread) { > Dispatch here or there(); > } > } else if (mState != CallingCallbacks) { > mState = CallingCallbacks(); > ... > } > > and you don't change the state in each function. I did something like this, I hope you're ok with it. > > @@ +4939,5 @@ > > + MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(this))); > > + } else { > > + AdvanceState(); > > + > > + nsresult rv = FinishInit(); > > Instead calling this directly, can you just do: > > AdvanceState(); > Run(); Ok. > > @@ +4963,5 @@ > > + AdvanceState(); > > + > > + MOZ_ALWAYS_TRUE(NS_SUCCEEDED(mOwningThread->Dispatch(this, > > + NS_DISPATCH_NORMAL))); > > + > > Again, I find this management of the state machine very confusing. > I prefer to have a table of State-Thread and let 1 single function to manage > the advanceState plus the dispatching. Ok. > > ::: dom/quota/QuotaManagerService.cpp > @@ +104,5 @@ > > +protected: > > + RefPtr<RequestBase> mRequest; > > + > > +public: > > + PendingRequestInfo(RequestBase* aRequest) > > explicit ? Ok. > > ::: dom/quota/QuotaRequests.cpp > @@ +91,5 @@ > > + return NS_OK; > > +} > > + > > +UsageRequest::UsageRequest(nsIPrincipal* aPrincipal, > > + nsIQuotaUsageCallback* aCallback) > > indentation Ok. > > @@ +161,5 @@ > > + > > +NS_IMETHODIMP > > +UsageRequest::GetFileUsage(uint64_t* aFileUsage) > > +{ > > + AssertIsOnOwningThread(); > > MOZ_ASSERT(aFileUsage); Ok. > > @@ +184,5 @@ > > + > > +NS_IMETHODIMP > > +UsageRequest::SetCallback(nsIQuotaUsageCallback* aCallback) > > +{ > > + AssertIsOnOwningThread(); > > MOZ_ASSERT(aCallback); ? Do we care? Hm, one can also clear the callback, so passing null should be allowed.
Assignee | ||
Comment 33•9 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #21) > Comment on attachment 8676881 [details] [diff] [review] > Part 8: Move getFileReferences() from PContent under new protocol > PBackgroundIndexedDBUtils > > Review of attachment 8676881 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/indexedDB/ActorsChild.cpp > @@ +3101,5 @@ > > + > > + if (mManager) { > > + mManager->ClearBackgroundActor(); > > +#ifdef DEBUG > > + mManager = nullptr; > > why only in DEBUG mode? I already commented on this. > > ::: dom/indexedDB/ActorsParent.cpp > @@ +27203,5 @@ > > +GetFileReferencesHelper::DispatchAndReturnFileReferences(int32_t* aMemRefCnt, > > + int32_t* aDBRefCnt, > > + int32_t* aSliceRefCnt, > > + bool* aResult) > > +{ > > MOZ_ASSERT(all the pointers?) Ok. > > ::: ipc/glue/BackgroundParentImpl.cpp > @@ +197,5 @@ > > auto > > +BackgroundParentImpl::AllocPBackgroundIndexedDBUtilsParent() > > + -> PBackgroundIndexedDBUtilsParent* > > +{ > > + using mozilla::dom::indexedDB::AllocPBackgroundIndexedDBUtilsParent; > > well... I don't see why you want to do it when you can do: > return mozilla::dom::indexedDB::AllocPBackgroundIndexedUtilsParent(); Oh, this was just a copy paste. I think it was used to workaround ugly line wrapping. Fixed. > > or move it to the other using. > > @@ +209,5 @@ > > +bool > > +BackgroundParentImpl::DeallocPBackgroundIndexedDBUtilsParent( > > + PBackgroundIndexedDBUtilsParent* aActor) > > +{ > > + using mozilla::dom::indexedDB::DeallocPBackgroundIndexedDBUtilsParent; > > ditto. Ok.
Assignee | ||
Comment 34•9 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #22) > Comment on attachment 8676882 [details] [diff] [review] > Part 9: Fix asynchronous deletion of files to work on PBackground > > Review of attachment 8676882 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/indexedDB/IndexedDatabaseManager.cpp > @@ +746,5 @@ > > +IndexedDatabaseManager::NoteBackgroundThread(nsIEventTarget* aBackgroundThread) > > +{ > > + MOZ_ASSERT(IsMainProcess()); > > + MOZ_ASSERT(NS_IsMainThread()); > > + MOZ_ASSERT(aBackgroundThread); > > MOZ_ASSERT(!mBackgroundThread || mBackgroundThread == aBackgroundThread) ? Well, we call this only once currently. I don't think we should change the assertion for now.
Assignee | ||
Comment 35•9 years ago
|
||
(In reply to Jan Varga [:janv] from comment #25) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=84a1f714dfd1 Ok, try looks good, so if no one objects, I'll try to land it this weekend.
Comment 36•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fedd09393d20 https://hg.mozilla.org/integration/mozilla-inbound/rev/6fc2ddb81059 https://hg.mozilla.org/integration/mozilla-inbound/rev/950f004242c5 https://hg.mozilla.org/integration/mozilla-inbound/rev/27a67b5cc591 https://hg.mozilla.org/integration/mozilla-inbound/rev/c53f530ef89c https://hg.mozilla.org/integration/mozilla-inbound/rev/a1d1731150f6 https://hg.mozilla.org/integration/mozilla-inbound/rev/100da64f36d9 https://hg.mozilla.org/integration/mozilla-inbound/rev/dc9bcf0af39e https://hg.mozilla.org/integration/mozilla-inbound/rev/0cac3146a3a3 https://hg.mozilla.org/integration/mozilla-inbound/rev/16f42df56f89 https://hg.mozilla.org/integration/mozilla-inbound/rev/8ff7083db0ea
Comment 37•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fedd09393d20 https://hg.mozilla.org/mozilla-central/rev/6fc2ddb81059 https://hg.mozilla.org/mozilla-central/rev/950f004242c5 https://hg.mozilla.org/mozilla-central/rev/27a67b5cc591 https://hg.mozilla.org/mozilla-central/rev/c53f530ef89c https://hg.mozilla.org/mozilla-central/rev/a1d1731150f6 https://hg.mozilla.org/mozilla-central/rev/100da64f36d9 https://hg.mozilla.org/mozilla-central/rev/dc9bcf0af39e https://hg.mozilla.org/mozilla-central/rev/0cac3146a3a3 https://hg.mozilla.org/mozilla-central/rev/16f42df56f89 https://hg.mozilla.org/mozilla-central/rev/8ff7083db0ea
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•