Base on today's storage meeting with Jan. We'd like to implement this, since the code for the searching directory on UpgradeStorageFrom2_0To3_0Helper is similar to the code for other UpgradeStorageFrom1_0To2_0Helper class. We want to extract a base class to reuse the code.

Two major changes in this patch: 1. Extract out the DoUpgrade() 2. Extract out the base class for upgrading Before | After --------------------------------------------------------------- StorageDirectoryHelper | StorageDirectoryHelper / | \ | | 0_0to1_0 1_0to2_0 2_0to3_0 | UpgradeStroageBase | / \ | 0_0to1_0 UpgradeClientBase | / \ | 1_0to2_0 2_0to3_0 try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=78f68eb73d934748e13609f1710c3a9963c5733c

(In reply to Tom Tung [:tt] from comment #2) > Two major changes in this patch: > 1. Extract out the DoUpgrade() Sorry, s/DoUpgrade/UpgradeStorage/g The reason is that UpgradeStorageFrom0_0To1_0(), UpgradeStorageFrom1_0To2_0(), and UpgradeStorageFrom2_0To3_0() all do the similar logic here.

(In reply to Tom Tung [:tt] from comment #2) > try: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=78f68eb73d934748e13609f1710c3a9963c5733c I somehow pushed the patch for logging infomation to try :( try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d7a769478494d5a26c3765c27baf777fb7c7f9a

Comment on attachment 8905418 [details] [diff] [review] baseClass.patch Hi Jan, This patch still have some nits, but I'd like to ask for your quick feedback about whether extracting out |UpgradeStorage()| the two base classes (their functions) are in the right direction. Thanks! For what this patch does, please refer to comment #2 and #3.

I'll take a look when bug 1290481 lands.

Comment on attachment 8905418 [details] [diff] [review] baseClass.patch I'll rebase the patch, check it again, and then send it for review.

As the summary, this patch creates a helper class to reuse the code. Based on the result of hg diff --stat (200 insertions(+), 265 deletions(-)). It reduces 65 lines of code.

Patch for reusing code in QuotaManager::UpgradeStorageFrom*To*().

Comment on attachment 9001567 [details] [diff] [review] Bug 1395102 - P1: Create UpgradeStorageHelper class for reusing lines of code in 1_0To2_0 and 2_0To2_1. Review of attachment 9001567 [details] [diff] [review]: ----------------------------------------------------------------- Not sure, if this patch makes things simpler.

So, right now, we have the StorageDirectoryHelper base class that handles generic stuff like having methods for reading metadata files, maintaining an array of origin properties and getting information from principals on the main thread. Then there are derived classes: CreateOrUpgradeDirectoryMetadataHelper UpgradeStorageFrom0_0To1_0Helper UpgradeStorageFrom1_0To2_0Helper UpgradeStorageFrom2_0To2_1Helper RestoreDirectoryMetadata2Helper The last one RestoreDirectoryMetadata2Helper, is special, it works with just one directory and is not intended for upgrades, so let's ignore it for now. The first thing that I notice is that: CreateOrUpgradeDirectoryMetadataHelper::CreateOrUpgradeMetadataFiles(), UpgradeStorageFrom0_0To1_0Helper::DoUpgrade(), UpgradeStorageFrom1_0To2_0Helper::DoUpgrade(), UpgradeStorageFrom2_0To2_1Helper::DoUpgrade() all look very similar. So I think we should begin with that and create a new class that inherits from the StorageDirectoryHelper class and has a method DoUpgrade(). All those derived classes (except RestoreDirectoryMetadata2Helper) should then inherit from it. The new class will have one or two pure virtual methods for stuff that varies across the current CreateOrUpgradeMetadataFiles() and DoUpgrade methods.

(In reply to Jan Varga [:janv] from comment #16) > So, right now, we have the StorageDirectoryHelper base class that handles > generic stuff like having methods for reading metadata files, maintaining an > array of origin properties and getting information from principals on the > main thread. > > Then there are derived classes: > CreateOrUpgradeDirectoryMetadataHelper > UpgradeStorageFrom0_0To1_0Helper > UpgradeStorageFrom1_0To2_0Helper > UpgradeStorageFrom2_0To2_1Helper > RestoreDirectoryMetadata2Helper > > The last one RestoreDirectoryMetadata2Helper, is special, it works with just > one directory and is not intended for upgrades, so let's ignore it for now. > > The first thing that I notice is that: > CreateOrUpgradeDirectoryMetadataHelper::CreateOrUpgradeMetadataFiles(), > UpgradeStorageFrom0_0To1_0Helper::DoUpgrade(), > UpgradeStorageFrom1_0To2_0Helper::DoUpgrade(), > UpgradeStorageFrom2_0To2_1Helper::DoUpgrade() > all look very similar. > > So I think we should begin with that and create a new class that inherits > from the StorageDirectoryHelper class and has a method DoUpgrade(). All > those derived classes (except RestoreDirectoryMetadata2Helper) should then > inherit from it. > The new class will have one or two pure virtual methods for stuff that > varies across the current CreateOrUpgradeMetadataFiles() and DoUpgrade > methods. Thanks for providing me a direction to work on! Actually, I had part of this thought, and I did a part of that in P2 (only trying to merge DoUpgrade() together). But, it still had some redundancy stuff that needs to be removed. I'll address your comment and move it to P1.

Jan, this patch create an intermediate class to reuse code. It inherits from StorageDirectoryHelper class and is inherited by UpgradeStorageFrom0_0To1_0Helper, UpgradeStorageFrom1_0To2_0Helper, UpgradeStorageFrom2_0To2_1Helper, and CreateOrUpgradeDirectoryMetadataHelper classes. It provides three pure virtual functions: - IsOriginDirectoryEligible(): checking whether the directory file is okay to upgrade or not. If not, it will ignore the files - InitOriginPropsAndMaybeUpgradeDirectory(): initializing the origin props and upgrade the directory in certain situation. - UpdateOriginProps(): updating the origin props from the directory metadata file. I named it UpgradeOriginDirectories Helper for now since it mainly upgrades stuff under origin directories. Could you help me to review it? Thanks! If this patch is okay, then I guess the next step would be having another base class for UpgradeStorageFromXXtoXXHelper? The reason is that there are still many similarities between these classes and maybe we could reuse the code in QuotaManager::UpgradeStorageFromXXToXX since they all go through the persistence directories to call the corresponding helper classes.

Comment on attachment 9016576 [details] [diff] [review] P1 Review of attachment 9016576 [details] [diff] [review]: ----------------------------------------------------------------- This is a good start, but we can do more here... ::: dom/quota/ActorsParent.cpp @@ +1809,5 @@ > + : StorageDirectoryHelper(aDirectory, aPersistent) > + { } > + > + nsresult > + TraverseOriginDirectories(); Well, with the current setup, this should be a protected method... Anyway, I don't think we need to have separate DoUpgrade methods which call TraverseOriginDirectories. TraverseOriginDirectories can be renamed to DoUpgrade and then we don't need those separate DoUpgrade methods. This way it's also possible to unify the way how we call directory->Exists(). CreateOrUpgradeMetadataFiles() can be removed too, UpgradeOriginDirectoriesHelper::DoUpgrade should handle it. @@ +1859,4 @@ > MaybeUpgradeOriginDirectory(nsIFile* aDirectory); > > + void > + UpdateOriginProps(OriginProps& aOriginProps) override; I think we don't need 3 methods, maybe 2, but I have to think about it a bit more.

These methods: UpgradeStorageFrom0_0To1_0Helper::IsOriginDirectoryEligible UpgradeStorageFrom1_0To2_0Helper::IsOriginDirectoryEligible UpgradeStorageFrom2_0To2_1Helper::IsOriginDirectoryEligible contain identical code, so at very least, I would put that code to: UpgradeOriginDirectoriesHelper::IsOriginDirectoryEligible and let CreateOrUpgradeDirectoryMetadataHelper::IsOriginDirectoryEligible to override it. However, even CreateOrUpgradeDirectoryMetadataHelper::IsOriginDirectoryEligible can be eliminated by moving the moz-safe-about+++home check to OriginProps::Init and later (in a followup bug) to the origin parser. See the attached patch. Everything what I said in my previous comment is still valid, I'll let you to do that. I'll look at other virtual methods later.

(In reply to Jan Varga [:janv] from comment #20) Thanks for the feedback! Will address them as soon as I can! > Comment on attachment 9016576 [details] [diff] [review] > P1 > > Review of attachment 9016576 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is a good start, but we can do more here... > > ::: dom/quota/ActorsParent.cpp > @@ +1809,5 @@ > > + : StorageDirectoryHelper(aDirectory, aPersistent) > > + { } > > + > > + nsresult > > + TraverseOriginDirectories(); > > Well, with the current setup, this should be a protected method... Is it because that it should only be called by its children classes? I put it to the public because both DoUpgrade() and CreateOrUpgradeMetadataFiles() are public functions. Will check public/protected/private next time! > Anyway, I don't think we need to have separate DoUpgrade methods which call > TraverseOriginDirectories. TraverseOriginDirectories can be renamed to > DoUpgrade and then we don't need those separate DoUpgrade methods. > This way it's also possible to unify the way how we call directory->Exists(). > CreateOrUpgradeMetadataFiles() can be removed too, > UpgradeOriginDirectoriesHelper::DoUpgrade should handle it. I see. Thanks for correcting that. Actually, I have considered doing something like that, but I was afraid the function name "TraverseOriginDirectories()" was not clear enough for others. Naming "DoUpgrade" does make it clear and more direct. > @@ +1859,4 @@ > > MaybeUpgradeOriginDirectory(nsIFile* aDirectory); > > > > + void > > + UpdateOriginProps(OriginProps& aOriginProps) override; > > I think we don't need 3 methods, maybe 2, but I have to think about it a bit > more. I checked attachment 9017036 [details] [diff] [review] and I found it combined IsOriginDirectoryEligible() and OriginProps::Init((). That's the thing that I haven't considered. I think I learn a lesson! Thanks for telling me that! (In reply to Jan Varga [:janv] from comment #21) > Created attachment 9017036 [details] [diff] [review] > IsOriginDirectoryEligible optimization > > These methods: > UpgradeStorageFrom0_0To1_0Helper::IsOriginDirectoryEligible > UpgradeStorageFrom1_0To2_0Helper::IsOriginDirectoryEligible > UpgradeStorageFrom2_0To2_1Helper::IsOriginDirectoryEligible > contain identical code, so at very least, I would put that code to: > UpgradeOriginDirectoriesHelper::IsOriginDirectoryEligible > and let CreateOrUpgradeDirectoryMetadataHelper::IsOriginDirectoryEligible to > override it. Oh, I planed having another base class for these three upgrade classes to reuse them, but it's genuinely better to do things like that. > However, even > CreateOrUpgradeDirectoryMetadataHelper::IsOriginDirectoryEligible can be > eliminated by moving the moz-safe-about+++home check to OriginProps::Init > and later (in a followup bug) to the origin parser. That's a really brilliant way to me. I'll try to learn that and do things like that next time! Thanks! > See the attached patch. > Everything what I said in my previous comment is still valid, I'll let you > to do that. > > I'll look at other virtual methods later. Thanks again!

(In reply to Tom Tung [:tt] from comment #22) > > ::: dom/quota/ActorsParent.cpp > > @@ +1809,5 @@ > > > + : StorageDirectoryHelper(aDirectory, aPersistent) > > > + { } > > > + > > > + nsresult > > > + TraverseOriginDirectories(); > > > > Well, with the current setup, this should be a protected method... > > Is it because that it should only be called by its children classes? Yes, it should only be called by derived classes. > I put it to the public because both DoUpgrade() and > CreateOrUpgradeMetadataFiles() are public functions. Will check > public/protected/private next time! Yes, they are public, but it doesn't mean that TraverseOriginDirectories has to be public too.

Comment on attachment 9016576 [details] [diff] [review] P1 Er, sorry Jan if you're editing reviews...

Comment on attachment 9017112 [details] [diff] [review] P1 (v1.1) Addressed comments so far

Comment on attachment 9017112 [details] [diff] [review] P1 (v1.1) Review of attachment 9017112 [details] [diff] [review]: ----------------------------------------------------------------- See also my latest interdiff. ::: dom/quota/ActorsParent.cpp @@ +1822,5 @@ > + OriginProps& aOriginProps, > + bool* aRemoved) = 0; > + > + virtual void > + UpdateOriginProps(OriginProps& aOriginProps) = 0; It seems we need just one virtual method. @@ +8897,5 @@ > + if (removed) { > + continue; > + } > + > + UpdateOriginProps(originProps); These two virtual methods are called one by one, there's nothing else between these calls, so why not have just one virtual method ? @@ +8906,5 @@ > if (mOriginProps.IsEmpty()) { > return NS_OK; > } > > + rv = StorageDirectoryHelper::ProcessOriginDirectories(); Why is the StorageDirectoryHelper:: prefix necessary ? @@ +8946,5 @@ > { > AssertIsOnIOThread(); > MOZ_ASSERT(aDirectory); > > + if (!mPersistent) { This is not necessary either.

(In reply to Jan Varga [:janv] from comment #29) Thanks for the feedback! > ::: dom/quota/ActorsParent.cpp > @@ +1822,5 @@ > > + OriginProps& aOriginProps, > > + bool* aRemoved) = 0; > > + > > + virtual void > > + UpdateOriginProps(OriginProps& aOriginProps) = 0; > > It seems we need just one virtual method. That's true. > @@ +8897,5 @@ > > + if (removed) { > > + continue; > > + } > > + > > + UpdateOriginProps(originProps); > > These two virtual methods are called one by one, there's nothing else > between these calls, so why not have just one virtual method ? That's also true. I extracted these functions by the early continues, but it's not necessary. > @@ +8906,5 @@ > > if (mOriginProps.IsEmpty()) { > > return NS_OK; > > } > > > > + rv = StorageDirectoryHelper::ProcessOriginDirectories(); > > Why is the StorageDirectoryHelper:: prefix necessary ? Sorry, it was my self-note while reading the code. > @@ +8946,5 @@ > > { > > AssertIsOnIOThread(); > > MOZ_ASSERT(aDirectory); > > > > + if (!mPersistent) { > > This is not necessary either. Will remove it.

Addressed comments and applied inter-diff patch

Make UpgradeOriginDirectoriesHelper()'s destructor be virtual and put PrepareOriginDirectoriesHelper into "private"

Comment on attachment 9017534 [details] [diff] [review] P1 (v2) Review of attachment 9017534 [details] [diff] [review]: ----------------------------------------------------------------- Jan, I addressed the comment and applied your inter-diff. Other than that, I modified two other places and explain that below. Could you help me to review this patch? Thanks! ::: dom/quota/ActorsParent.cpp @@ +1813,5 @@ > + nsresult > + DoUpgrade(); > + > +protected: > + virtual ~UpgradeOriginDirectoriesHelper() Make this destructor be a virtual function so that destructors of its derived classes are able to find it. Although it doesn't destruct anything now, I reckon maybe it's better to do that. @@ +1818,5 @@ > + { } > + > +private: > + virtual nsresult > + PrepareOriginDirectory(OriginProps& aOriginProps, bool* aRemoved) = 0; Make PrepareOriginDirectory as a private function since it isn't called by its inherited classes.

Yeah, almost there, I'll take a look one more time tomorrow.

Comment on attachment 9017534 [details] [diff] [review] P1 (v2) Review of attachment 9017534 [details] [diff] [review]: ----------------------------------------------------------------- Ok, this looks almost ready, just address comments below. Once you do that, you can refresh the P2 patch and I'll look how it fits now. ::: dom/quota/ActorsParent.cpp @@ +1800,5 @@ > void > HandleTrailingSeparator(); > }; > > +class UpgradeOriginDirectoriesHelper We need to find better names for all these classes and I have something in my mind. For now, I think this should be called: RepositoryOperationBase We'll do a naming cleanup in another patch in this bug. @@ +1810,5 @@ > + : StorageDirectoryHelper(aDirectory, aPersistent) > + { } > + > + nsresult > + DoUpgrade(); Given the class name RepositoryOperationBase (doesn't contain the word upgrade) this should be called Run() or something like that. @@ +8827,5 @@ > { > AssertIsOnIOThread(); > > bool exists; > nsresult rv = mDirectory->Exists(&exists); As I mentioned in the comment 20, directory->Exists() calls should be optimized while we are here. For example, before we create a new CreateOrUpgradeDirectoryMetadataHelper, we check if the directory exists, then we call DoUpgrade() and that checks that again. I think DoUpgrade/Run should just assert that the directory exists: DebugOnly<bool> exists; MOZ_ASSERT(NS_SUCCEEDED(mDirectory->Exists(&exists))); MOZ_ASSERT(exists); @@ +8882,4 @@ > } > > mOriginProps.AppendElement(std::move(originProps)); > } The rv check is missing here: if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } It's because the loop may abort when |NS_SUCCEEDED(rv = entries->GetNextFile)| is not true, in that case we would miss that failure. However, you can rework this loop like it's done here: https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/dom/indexedDB/ActorsParent.cpp#18269

(In reply to Jan Varga [:janv] from comment #35) Thanks for the feedback! > Ok, this looks almost ready, just address comments below. > Once you do that, you can refresh the P2 patch and I'll look how it fits now. Will check if it fit and provides a P3 (original P2). > > +class UpgradeOriginDirectoriesHelper > > We need to find better names for all these classes and I have something in > my mind. > For now, I think this should be called: > RepositoryOperationBase > > We'll do a naming cleanup in another patch in this bug. Sure, will make a P2 patch for that. > @@ +1810,5 @@ > > + : StorageDirectoryHelper(aDirectory, aPersistent) > > + { } > > + > > + nsresult > > + DoUpgrade(); > > Given the class name RepositoryOperationBase (doesn't contain the word > upgrade) this should be called Run() or something like that. Will do that in P2. > @@ +8827,5 @@ > > { > > AssertIsOnIOThread(); > > > > bool exists; > > nsresult rv = mDirectory->Exists(&exists); > > As I mentioned in the comment 20, directory->Exists() calls should be > optimized while we are here. For example, before we create a new > CreateOrUpgradeDirectoryMetadataHelper, we check if the directory exists, > then we call DoUpgrade() and that checks that again. I think DoUpgrade/Run > should just assert that the directory exists: > DebugOnly<bool> exists; > MOZ_ASSERT(NS_SUCCEEDED(mDirectory->Exists(&exists))); > MOZ_ASSERT(exists); I see. Sorry for the misunderstanding. Before this patch, only CreateOrUpgradeDirectoryMetadataHelper and UpgradeStorageFrom0_0To1_0Helper check if the given directory exists or not. Currently there only two places that new a CreateOrUpgradeDirectoryMetadataHelper [1] [2]. In case [1], its directory is ensured existing by [3]. For case [2], its directory is ensured existing by [4]. As for UpgradeStorageFrom0_0To1_0Helper, I'm not sure whether [5] ensure the directory exists or not. Will dig a bit more into that. [1] https://searchfox.org/mozilla-central/rev/c9272ef398954288525e37196eada1e5a93d93bf/dom/quota/ActorsParent.cpp#4465 [2] https://searchfox.org/mozilla-central/rev/c9272ef398954288525e37196eada1e5a93d93bf/dom/quota/ActorsParent.cpp#4498 [3] https://searchfox.org/mozilla-central/rev/c9272ef398954288525e37196eada1e5a93d93bf/dom/quota/ActorsParent.cpp#4424-4433 [4] https://searchfox.org/mozilla-central/rev/c9272ef398954288525e37196eada1e5a93d93bf/dom/quota/ActorsParent.cpp#4481-4505 [5] https://searchfox.org/mozilla-central/rev/c9272ef398954288525e37196eada1e5a93d93bf/dom/quota/ActorsParent.cpp#4595 > @@ +8882,4 @@ > > } > > > > mOriginProps.AppendElement(std::move(originProps)); > > } > > The rv check is missing here: > if (NS_WARN_IF(NS_FAILED(rv))) { > return rv; > } > > It's because the loop may abort when |NS_SUCCEEDED(rv = > entries->GetNextFile)| is not true, in that case we would miss that failure. I see. I remember that you told me this in another bug, but I forgot while implementing this patch. Really sorry about that. Will put that in my note. > However, you can rework this loop like it's done here: > https://dxr.mozilla.org/mozilla-central/rev/ > c291143e24019097d087f9307e59b49facaf90cb/dom/indexedDB/ActorsParent.cpp#18269 Will try to do that

Comment on attachment 9017838 [details] [diff] [review] P1 (v3) It turns out that NS_NewLocalFile() creates an in-memory object without writing a file into the disk, so this patch adds an if-condition guard to ensure directory exists in QuotaManager::UpgradeStorageFrom0_0To1_0(). Addressed Comment 35. Will rename UpgradeOriginDirectoriesHelper and UpgradeOriginDirectoriesHelper::DoUpgrade in P2.

Comment on attachment 9017839 [details] [diff] [review] P2 (v1) Rename "UpgradeOriginDirectoriesHelper class" to "RepositoryOperationBase class" and "DoUpgrade()" to "Run()". I have a thought that rename "DoUpgrade()" to "ProcessRepository()", but I'm not sure if it's better. Thus, this patch uses Run() based on the comment.

Comment on attachment 9017848 [details] [diff] [review] P3 (v0) Jan, this is the idea in the original P2 patch. It tried to reuse the code in QuotaManager::UpgradeStorageFrom0_0To1_0(), QuotaManager::UpgradeStorageFrom1_0To2_0(), and QuotaManager::UpgradeStorageFrom2_0To2_1(). Could you have a look and maybe check whether the idea fits or not? Thanks!

Comment on attachment 9017838 [details] [diff] [review] P1 (v3) Review of attachment 9017838 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, just apply my new interdiff ::: dom/quota/ActorsParent.cpp @@ +8884,5 @@ > return rv; > } > > + bool removed; > + rv = PrepareOriginDirectory(originProps, &removed); This method shouldn't be called for obsolete origins, otherwise we can get empty strings passed to IsOriginInternal() I'll attach a patch for this.

Comment on attachment 9017839 [details] [diff] [review] P2 (v1) Review of attachment 9017839 [details] [diff] [review]: ----------------------------------------------------------------- Ok, but we should also rename StorageDirectoryHelper to StorageOperationBase in this patch.

Comment on attachment 9017848 [details] [diff] [review] P3 (v0) Review of attachment 9017848 [details] [diff] [review]: ----------------------------------------------------------------- This is a good idea, but I think we can avoid all these kStorageVX constants if we use C++ templates. Let me try something.

Comment on attachment 9017839 [details] [diff] [review] P2 (v1) Review of attachment 9017839 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, my mistake, Run() collides with existing Run() inherited from Runnable. ProcessRepository() is fine for now.

Addressed comment, applied attachment 9018168 [details] [diff] [review], and added commit message

Interdiff attachment 9017838 [details] [diff] [review] P1 (v3) and attachment 9018210 [details] [diff] [review] Should be the same as attachment 9018168 [details] [diff] [review]

Addressed the comment and added commit message

Interdiff for P2.

(In reply to Jan Varga [:janv] from comment #44) Thanks for the reviews! > This method shouldn't be called for obsolete origins, otherwise we can get > empty strings passed to IsOriginInternal() > I'll attach a patch for this. Thanks for catching this! I've applied that in P1 (In reply to Jan Varga [:janv] from comment #46) > Ok, but we should also rename StorageDirectoryHelper to StorageOperationBase > in this patch. Have applied that in P2 (In reply to Jan Varga [:janv] from comment #48) > Sorry, my mistake, Run() collides with existing Run() inherited from > Runnable. > ProcessRepository() is fine for now. I also found this today morning. Sorry for not checking that before sending to review, but it took me a while to find to place to call that [1]... Before sending to review, I had checked "thread->Dispatch()" but forgotten to check "NS_DispatchToMainThread()". Will check all of them or just checking whether Run() method is implemented by its parent classes or not next time. [1] https://searchfox.org/mozilla-central/rev/eef79962ba73f7759fd74da658f6e5ceae0fc730/dom/quota/ActorsParent.cpp#8254 (In reply to Jan Varga [:janv] from comment #49) > Created attachment 9018188 [details] [diff] [review] > interdiff for P3 Thanks! Will apply that

Comment on attachment 9018219 [details] [diff] [review] P3 (v1) Jan, thanks for your inter-diff patch! That is a way better than my original idea! This patch addressed using a template function to identify different versions of upgrade helper rather than original static const global variables. Could you help me to review it? Thanks!

Comment on attachment 9018219 [details] [diff] [review] P3 (v1) Review of attachment 9018219 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, looks good. I just found out that you tried to use C++ templates in your original WIP patch.

There's one more thing that could be abstracted somehow. UpgradeStorageFrom1_0To2_0Helper::MaybeUpgradeClients and UpgradeStorageFrom2_0To2_1Helper::MaybeUpgradeClients are very similar and we might need to add another one in a future storage upgrade. Could you take a look if it's possible to create a generic method for that ? I would probably put it to RepositoryOperationBase.

(In reply to Jan Varga [:janv] from comment #58) > There's one more thing that could be abstracted somehow. > UpgradeStorageFrom1_0To2_0Helper::MaybeUpgradeClients and > UpgradeStorageFrom2_0To2_1Helper::MaybeUpgradeClients are very similar and > we might need to add another one in a future storage upgrade. > Could you take a look if it's possible to create a generic method for that ? > I would probably put it to RepositoryOperationBase. Sure, I had a similar thought in original P1. Will check whether can I applied that idea to here.

Jan, this patch tried to reuse the code for MaybeUpgradeClients(). To do that, it moves that function to the base class and creates another virtual function MaybeUpgradeClient(). MaybeUpgradeClient() is empty in the base class and is implemented in version_1_to_2 and version_2_to_2_1 classes. Since it's empty, I'm not sure if it's better to put it in the place of implementation or just leave it in the place of class declaration. I put it in the place where declares the class now. Could you help me to review that? Thanks!

Comment on attachment 9018286 [details] [diff] [review] P4 (v0) Review of attachment 9018286 [details] [diff] [review]: ----------------------------------------------------------------- Not bad, but there's still some room for improvement, see my comments in the interdiff. ::: dom/quota/ActorsParent.cpp @@ +1817,5 @@ > virtual ~RepositoryOperationBase() > { } > > + nsresult > + MaybeUpgradeClients(const OriginProps& aOriginsProps); We can use templates here again. @@ +1884,5 @@ > private: > nsresult > + MaybeUpgradeClient(nsIFile* aClientDirectory, > + const nsAString& aLeafName, > + QuotaManager* aQuotaManger) override; There's still some code duplication that can be eliminated.

> Not bad, but there's still some room for improvement, see my comments in the > interdiff. *and* the interdiff

OK, with all the changes applied, ActorsParent.cpp is reduced by 3179 bytes or 160 lines which is great especially given we want to add a new storage upgrade (maybe more) that would need to duplicate all that stuff again if we didn't fix this bug.

(In reply to Jan Varga [:janv] from comment #63 Thanks for the feedback! I had a thought about using the template, but I gave up because I couldn't figure out the place for placing removing morgue directories stuff. Your inter-diff patch solves this by just simply putting it in PrepareOriginDirectory() which is great! Thanks! > > Not bad, but there's still some room for improvement, see my comments in the > interdiff. > > ::: dom/quota/ActorsParent.cpp > @@ +1817,5 @@ > > virtual ~RepositoryOperationBase() > > { } > > > > + nsresult > > + MaybeUpgradeClients(const OriginProps& aOriginsProps); > > We can use templates here again. > > @@ +1884,5 @@ > > private: > > nsresult > > + MaybeUpgradeClient(nsIFile* aClientDirectory, > > + const nsAString& aLeafName, > > + QuotaManager* aQuotaManger) override; > > There's still some code duplication that can be eliminated.

Comment on attachment 9018558 [details] [diff] [review] P4 (v1) temp Review of attachment 9018558 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/ActorsParent.cpp @@ +8874,5 @@ > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return rv; > + } > + > + nsCOMPtr<nsIFile> file; This can be placed just before GetNextFile call. @@ +8918,5 @@ > + // Unknown files during upgrade are allowed. Just warn if we find them. > + if (!IsOriginMetadata(leafName) && > + !IsTempMetadata(leafName)) { > + UNKNOWN_FILE_WARNING(leafName); > + } Hm, this will work OK, but if something is added after this if/else block, then we need to have |continue;| here. I would rather do the same thing as in the original MaybeUpgradeClients: if (!isDirectory) { // Unknown files during upgrade are allowed. Just warn if we find them. if (!IsOriginMetadata(leafName) && !IsTempMetadata(leafName)) { UNKNOWN_FILE_WARNING(leafName); } continue; } Client::Type clientType; rv = Client::TypeFromText(leafName, clientType); ...

Jan, this patch addressed your comment and inter-diff patch. Other than that, it adds an assertion for the function pointer. Could you help me to review it? Thanks!

Comment on attachment 9018563 [details] [diff] [review] P4 (v1) There are some new comment

Comment on attachment 9018586 [details] [diff] [review] P4 (v2) Addressed the comments. Other than that, it adds an assertion for the function pointer. Jan, could you help me review this patch? Thanks!

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=33b00a23a82cf5db85326d58abf8d8fa08b1c93

Update commit message

Pushed by shes050117@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0d01e0a4de95 P1 - Introduce a intermediate helper class to reuse code for upgrading origin directories; r=janv https://hg.mozilla.org/integration/mozilla-inbound/rev/a0de0c0ec6b0 P2 - Rename the intermediate class in P1 to RepositoryOperationBase; r=janv https://hg.mozilla.org/integration/mozilla-inbound/rev/47335300b337 P3 - Reuse the code for QuotaManager::UpgradeStorageFromxxToxx(); r=janv https://hg.mozilla.org/integration/mozilla-inbound/rev/c768d21501f9 P4 - Reuse the code for MaybeUpgradeClients() in the upgrade helpers; r=janv

https://hg.mozilla.org/mozilla-central/rev/0d01e0a4de95 https://hg.mozilla.org/mozilla-central/rev/a0de0c0ec6b0 https://hg.mozilla.org/mozilla-central/rev/47335300b337 https://hg.mozilla.org/mozilla-central/rev/c768d21501f9