1395102 - Create the base class for UpgradeStorageFrom*_0To*_0Helper

Status: RESOLVED FIXED

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

Description

[Tom Tung [:tt, :ttung]]

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.

Comment 1

[Tom Tung [:tt, :ttung]]

Attachment: wip.patch

Comment 2

[Tom Tung [:tt, :ttung]]

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

Comment 3

[Tom Tung [:tt, :ttung]]

(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.

Comment 4

[Tom Tung [:tt, :ttung]]

(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 5

[Tom Tung [:tt, :ttung]]

Attachment: baseClass.patch

Comment 6

[Tom Tung [:tt, :ttung]]

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.

Comment 7

[Jan Varga [:janv]]

I'll take a look when bug 1290481 lands.

Comment 8

[Tom Tung [:tt, :ttung]]

Comment on attachment 8905418 [details] [diff] [review]
baseClass.patch

I'll rebase the patch, check it again, and then send it for review.

Comment 9

[Tom Tung [:tt, :ttung]]

Attachment: patch

Comment 10

[Tom Tung [:tt, :ttung]]

Attachment: temp.patch

Comment 11

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1395102 - P1: Create UpgradeStorageHelper class for reusing lines of code in 1_0To2_0 and 2_0To2_1.

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.

Comment 12

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1395102 - P1: Create UpgradeStorageHelper class for reusing lines of code in 1_0To2_0 and 2_0To2_1.

Comment 13

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1395102 - P2: Create UpgradeStorage() to reuse lines of code for QuotaManager::UpgradeStorageFrom*To*.

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

Comment 14

[Tom Tung [:tt, :ttung]]

Attachment: Change lines to match 80 rule.

Comment 15

[Jan Varga [:janv]]

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.

Comment 16

[Jan Varga [:janv]]

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.

Comment 17

[Tom Tung [:tt, :ttung]]

(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.

Comment 18

[Tom Tung [:tt, :ttung]]

Attachment: P1

Comment 19

[Tom Tung [:tt, :ttung]]

Attachment: 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 20

[Jan Varga [:janv]]

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.

Comment 21

[Jan Varga [:janv]]

Attachment: 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.
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.

Comment 22

[Tom Tung [:tt, :ttung]]

(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!

Comment 23

[Jan Varga [:janv]]

(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 24

[Tom Tung [:tt, :ttung]]

Attachment: P1 (v1)

Comment 25

[Tom Tung [:tt, :ttung]]

Comment on attachment 9016576 [details] [diff] [review]
P1

Er, sorry Jan if you're editing reviews...

Comment 26

[Tom Tung [:tt, :ttung]]

Attachment: P1 (v1.1)

Comment 27

[Tom Tung [:tt, :ttung]]

Comment on attachment 9017112 [details] [diff] [review]
P1 (v1.1)

Addressed comments so far

Comment 28

[Jan Varga [:janv]]

Attachment: Merge InitOriginPropsAndMaybeUpgradeDirectory and UpdateOriginProps

Comment 29

[Jan Varga [:janv]]

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.

Comment 30

[Tom Tung [:tt, :ttung]]

(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.

Comment 31

[Tom Tung [:tt, :ttung]]

Attachment: P1 (v2)

Addressed comments and applied inter-diff patch

Comment 32

[Tom Tung [:tt, :ttung]]

Attachment: P1 (v2)

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

Comment 33

[Tom Tung [:tt, :ttung]]

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.

Comment 34

[Jan Varga [:janv]]

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

Comment 35

[Jan Varga [:janv]]

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

Comment 36

[Tom Tung [:tt, :ttung]]

(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 37

[Tom Tung [:tt, :ttung]]

Attachment: P1 (v3)

Comment 38

[Tom Tung [:tt, :ttung]]

Attachment: P2 (v1)

Comment 39

[Tom Tung [:tt, :ttung]]

Attachment: P3 (v0) temp

Comment 40

[Tom Tung [:tt, :ttung]]

Attachment: P3 (v0)

Comment 41

[Tom Tung [:tt, :ttung]]

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 42

[Tom Tung [:tt, :ttung]]

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 43

[Tom Tung [:tt, :ttung]]

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 44

[Jan Varga [:janv]]

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 45

[Jan Varga [:janv]]

Attachment: interdiff for P1

Comment 46

[Jan Varga [:janv]]

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 47

[Jan Varga [:janv]]

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 48

[Jan Varga [:janv]]

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.

Comment 49

[Jan Varga [:janv]]

Attachment: interdiff for P3

Comment 50

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1395102 - P1 - Introduce a intermediate helper class to reuse code for upgrading origin directories; r=janv

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

Comment 51

[Tom Tung [:tt, :ttung]]

Attachment: interdiff-p1.patch

Interdiff attachment 9017838 [details] [diff] [review] P1 (v3) and attachment 9018210 [details] [diff] [review]

Should be the same as attachment 9018168 [details] [diff] [review]

Comment 52

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1395102 - P2 - Rename the intermediate class in P1 to RepositoryOperationBase; r=janv

Addressed the comment and added commit message

Comment 53

[Tom Tung [:tt, :ttung]]

Attachment: interdiff-p2.patch

Interdiff for P2.

Comment 54

[Tom Tung [:tt, :ttung]]

(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 55

[Tom Tung [:tt, :ttung]]

Attachment: P3 (v1)

Comment 56

[Tom Tung [:tt, :ttung]]

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 57

[Jan Varga [:janv]]

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.

Comment 58

[Jan Varga [:janv]]

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.

Comment 59

[Tom Tung [:tt, :ttung]]

(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.

Comment 60

[Tom Tung [:tt, :ttung]]

Attachment: P4 (v0) temp

Comment 61

[Tom Tung [:tt, :ttung]]

Attachment: P4 (v0)

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 62

[Jan Varga [:janv]]

Attachment: interdiff for P4

Comment 63

[Jan Varga [:janv]]

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.

Comment 64

[Jan Varga [:janv]]

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

*and* the interdiff

Comment 65

[Jan Varga [:janv]]

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.

Comment 66

[Tom Tung [:tt, :ttung]]

(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 67

[Tom Tung [:tt, :ttung]]

Attachment: P4 (v1) temp

Comment 68

[Jan Varga [:janv]]

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);
...

Comment 69

[Tom Tung [:tt, :ttung]]

Attachment: P4 (v1)

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 70

[Tom Tung [:tt, :ttung]]

Comment on attachment 9018563 [details] [diff] [review]
P4 (v1)

There are some new comment

Comment 71

[Tom Tung [:tt, :ttung]]

Attachment: P4 (v2)

Comment 72

[Tom Tung [:tt, :ttung]]

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!

Comment 73

[Tom Tung [:tt, :ttung]]

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

Comment 74

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1395102 - P3 - Reuse the code for QuotaManager::UpgradeStorageFromxxToxx(); r=janv

Update commit message

Comment 75

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1395102 - P4 - Reuse the code for MaybeUpgradeClients() in the upgrade helpers; r=janv

Comment 76

[Pulsebot]

Pushed by [e-mail]:
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

Comment 77

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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