Closed Bug 1395102 Opened 2 years ago Closed Last year

Create the base class for UpgradeStorageFrom*_0To*_0Helper

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: tt, Assigned: tt)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 31 obsolete files)

26.12 KB, patch
Details | Diff | Splinter Review
14.19 KB, patch
Details | Diff | Splinter Review
7.82 KB, patch
Details | Diff | Splinter Review
11.36 KB, patch
Details | Diff | Splinter Review
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.
Attached patch wip.patch (obsolete) — Splinter Review
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
Attached patch baseClass.patch (obsolete) — Splinter Review
Attachment #8904144 - Attachment is obsolete: true
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.
Attachment #8905418 - Flags: feedback?(jvarga)
I'll take a look when bug 1290481 lands.
Priority: -- → P3
Assignee: ttung → nobody
Blocks: 1482662
Assignee: nobody → shes050117
Comment on attachment 8905418 [details] [diff] [review]
baseClass.patch

I'll rebase the patch, check it again, and then send it for review.
Attachment #8905418 - Flags: feedback?(jvarga)
Attached file patch (obsolete) —
Attachment #8905418 - Attachment is obsolete: true
Attached patch temp.patch (obsolete) — Splinter Review
Attachment #9000191 - Attachment is obsolete: true
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.
Attachment #9000192 - Attachment is obsolete: true
Attachment #9001567 - Flags: review?(jvarga)
Patch for reusing code in QuotaManager::UpgradeStorageFrom*To*().
Attached patch Change lines to match 80 rule. (obsolete) — Splinter Review
Blocks: 1423917
Priority: P3 → P2
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.
Attachment #9001567 - Flags: review?(jvarga)
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.
Attached patch P1 (obsolete) — Splinter Review
Attachment #9001567 - Attachment is obsolete: true
Attached patch P1 (obsolete) — Splinter Review
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.
Attachment #9016570 - Attachment is obsolete: true
Attachment #9016576 - Flags: review?(jvarga)
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.
Attached patch P1 (v1) (obsolete) — Splinter Review
Attachment #9016576 - Attachment is obsolete: true
Attachment #9016576 - Flags: review?(jvarga)
Comment on attachment 9016576 [details] [diff] [review]
P1

Er, sorry Jan if you're editing reviews...
Attachment #9016576 - Attachment is obsolete: false
Attached patch P1 (v1.1) (obsolete) — Splinter Review
Attachment #9017104 - Attachment is obsolete: true
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.
Attachment #9016576 - Attachment is obsolete: true
(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.
Attached patch P1 (v2) (obsolete) — Splinter Review
Addressed comments and applied inter-diff patch
Attachment #9017112 - Attachment is obsolete: true
Attached patch P1 (v2) (obsolete) — Splinter Review
Make UpgradeOriginDirectoriesHelper()'s destructor be virtual and put PrepareOriginDirectoriesHelper into "private"
Attachment #9017515 - Attachment is obsolete: true
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.
Attachment #9017534 - Flags: review?(jvarga)
Yeah, almost there, I'll take a look one more time tomorrow.
Attachment #9001569 - Attachment is obsolete: true
Attachment #9001571 - Attachment is obsolete: true
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
Attachment #9017534 - Flags: review?(jvarga) → feedback+
(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
Attached patch P1 (v3) (obsolete) — Splinter Review
Attachment #9017534 - Attachment is obsolete: true
Attached patch P2 (v1) (obsolete) — Splinter Review
Attached patch P3 (v0) temp (obsolete) — Splinter Review
Attached patch P3 (v0) (obsolete) — Splinter Review
Attachment #9017846 - Attachment is obsolete: true
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.
Attachment #9017838 - Attachment description: P1 (v3) temp → P1 (v3)
Attachment #9017838 - Flags: review?(jvarga)
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.
Attachment #9017839 - Flags: review?(jvarga)
Attachment #9017839 - Attachment description: P2 (v1) temp → P2 (v1)
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!
Attachment #9017848 - Attachment description: P3 (v0) temp → P3 (v0)
Attachment #9017848 - Flags: feedback?(jvarga)
Attachment #9017036 - Attachment is obsolete: true
Attachment #9017423 - Attachment is obsolete: true
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.
Attachment #9017838 - Flags: review?(jvarga) → review+
Attached patch interdiff for P1 (obsolete) — Splinter Review
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.
Attachment #9017839 - Flags: review?(jvarga) → review+
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.
Attachment #9017848 - Flags: feedback?(jvarga) → feedback+
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.
Attached patch interdiff for P3 (obsolete) — Splinter Review
Addressed comment, applied attachment 9018168 [details] [diff] [review], and added commit message
Attachment #9017838 - Attachment is obsolete: true
Attachment #9018168 - Attachment is obsolete: true
Attachment #9018210 - Flags: review+
Addressed the comment and added commit message
Attachment #9017839 - Attachment is obsolete: true
Attachment #9018215 - Flags: review+
Attached patch interdiff-p2.patch (obsolete) — Splinter Review
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
Attached patch P3 (v1) (obsolete) — Splinter Review
Attachment #9017848 - Attachment is obsolete: true
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!
Attachment #9018219 - Attachment description: P3 (v1) temp → P3 (v1)
Attachment #9018219 - Flags: review?(jvarga)
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.
Attachment #9018219 - Flags: review?(jvarga) → review+
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.
Attached patch P4 (v0) temp (obsolete) — Splinter Review
Attached patch P4 (v0) (obsolete) — Splinter Review
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!
Attachment #9018283 - Attachment is obsolete: true
Attachment #9018286 - Flags: review?(jvarga)
Attached patch interdiff for P4 (obsolete) — Splinter Review
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.
Attachment #9018286 - Flags: review?(jvarga) → feedback+
> 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.
Attached patch P4 (v1) temp (obsolete) — Splinter Review
Attachment #9018286 - Attachment is obsolete: true
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);
...
Attached patch P4 (v1) (obsolete) — Splinter Review
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!
Attachment #9018558 - Attachment is obsolete: true
Attachment #9018563 - Flags: review?(jvarga)
Comment on attachment 9018563 [details] [diff] [review]
P4 (v1)

There are some new comment
Attachment #9018563 - Flags: review?(jvarga)
Attached patch P4 (v2) (obsolete) — Splinter Review
Attachment #9018563 - Attachment is obsolete: true
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!
Attachment #9018586 - Flags: review?(jvarga)
Attachment #9018586 - Attachment description: P4 (v2) temp → P4 (v2)
Update commit message
Attachment #9018219 - Attachment is obsolete: true
Attachment #9018601 - Flags: review+
Attachment #9018586 - Flags: review?(jvarga) → review+
Attachment #9018188 - Attachment is obsolete: true
Attachment #9018211 - Attachment is obsolete: true
Attachment #9018216 - Attachment is obsolete: true
Attachment #9018371 - Attachment is obsolete: true
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
You need to log in before you can comment on or make changes to this bug.