Page MenuHomePhabricator

Bug 1627892 - Make Manager derive from SafeRefCounted. r=#dom-workers-and-storage
ClosedPublic

Authored by sg on Apr 7 2020, 9:26 AM.

Details

Reviewers
ttung
Group Reviewers
Restricted Project
Commits
rMOZILLACENTRALed1e4e9212ef: Bug 1627892 - Make Manager derive from SafeRefCounted. r=ttung
Bugzilla Bug ID
1627892

Diff Detail

Repository
rMOZILLACENTRAL mozilla-central
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".
phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".
phab-bot removed a project: secure-revision.

Code analysis found 5 defects in the diff 256376:

  • 1 defect found by clang-tidy
  • 4 defects found by clang-format

You can run this analysis locally with:

  • ./mach clang-format -s -p dom/cache/Context.cpp dom/cache/Context.h (C/C++)
  • ./mach static-analysis check dom/cache/CacheParent.cpp (C/C++)

For your convenience, here is a patch that fixes all the clang-format defects (use it in your repository with hg import or git apply -p0).

The analysis task source-test-coverity-coverity failed, but we could not detect any issue.
Please check this task manually.

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

sg edited the summary of this revision. (Show Details)

The analysis task source-test-coverity-coverity failed, but we could not detect any issue.
Please check this task manually.

If you see a problem in this automated review, please report it here.

ttung added subscribers: asuth, ttung.

Thanks for the patch! @asuth should give it another look.

dom/cache/AutoUtils.h
15

The order of included files should be:

#include "mozilla/Attributes.h"
#include "mozilla/dom/SafeRefPtr.h"
#include "mozilla/dom/cache/CacheTypes.h"
#include "mozilla/dom/cache/Types.h"
#include "mozilla/dom/cache/TypeUtils.h"

Here and similar cases below

dom/cache/Manager.h
122

Rename to AcquireManager or change return type?

269–273

Would you do that in a follow-up patch or bug?

This revision is now accepted and ready to land.Apr 16 2020, 7:26 AM
janv added inline comments.
dom/cache/Manager.h
122

This is not a getter for a member variable, it's a static method, so I think the AcquireX convention doesn't apply here.

dom/cache/Manager.h
122

Ah, I see. Then, please ignore my previous comment.

sg added inline comments.
dom/cache/Manager.h
122

Hm, I think it actually should apply here analogously, and it should be named AcquireManager as Tom suggested (or at least the comment should be updated to say that for changing it later on).

dom/cache/Manager.h
122

What about GetOrCreate then ?

dom/cache/Manager.h
122

Hm? GetOrCreate is the other function above...

dom/cache/Manager.h
122

There are two methods:

static Result<SafeRefPtr<Manager>, nsresult> GetOrCreate(ManagerId* aManagerId);
static SafeRefPtr<Manager> Get(ManagerId* aManagerId);

How should they look like according to the naming guideline ?

dom/cache/Manager.h
122

As said before, the second function is not directly covered by what was discussed regarding naming, not so much because it is static, but because it has an argument, however, since it is some kind of getter returning a SafeRefPtr, I think it should be called Acquire* analogously.

The first function is not in the scope of the naming discussed at all. However, when the second function is called Acquire*, probably this should use Acquire is some way as well.

I don't think we need to include this in some guideline for now, since this seems like kind of a rare case.

So:

static Result<SafeRefPtr<Manager>, nsresult> AcquireOrCreateManager(ManagerId* aManagerId);
static SafeRefPtr<Manager> AcquireManager(ManagerId* aManagerId);

or just (because they are member functions of Manager)

static Result<SafeRefPtr<Manager>, nsresult> AcquireOrCreate(ManagerId* aManagerId);
static SafeRefPtr<Manager> Acquire(ManagerId* aManagerId);
269–273

Maybe in this case, it would be good to do it, since there are the static public functions on the same class that should be used for instantiations. I am just thinking if there are contexts (tests) where it would make sense to instantiate it directly.

dom/cache/Manager.h
122

Well, I'm not yet convinced about this, let's just put a comment here and we'll solve the naming for this later.
This pattern (Get and GetOrCreate) is used all over our code.

dom/cache/Manager.h
122

What are you not convinced about?

That we don't need a rule? Maybe I was under a wrong impression on the frequency of this. It's fine to include it in the guideline if it's more frequent than I thought.

Or about the use of Acquire here? I think we should discuss it now in that case, since this must be consistent with the rules. If we don't want to use Acquire here, we shouldn't either for the cases we discussed on Tuesday.

dom/cache/Manager.h
122

I'm not convinced about the use of Acquire especially for the GetOrCreate case. Yes, this is used in many places, for example IndexedDatabaseManager* IndexedDatabaseManager::Get and IndexedDatabaseManager::GetOrCreate. We didn't talk much about static getters (even with an argument) which behave like factory methods.

I wanted to postpone this discussion, but never mind, we can try to find a consensus here.

So if I wanted to convert IndexedDatabaseManager* IndexedDatabaseManager::Get I would probably use Maybe<IndexedDatabaseManager&> IndexedDatabaseManager::MaybeMutableRef (without const of course because it's a static method).

Now if I wanted to convert IndexedDatabaseManager* IndexedDatabaseManager::GetOrCreate as well, I'm not sure how would that look like.

MaybeMutableRefOrCreate ?

I think we need to figure out something else that would indicate that the method will create a new object (singleton in this case) if it doesn't exist yet.

One solution could be:

struct CreateIfNotExists {};

MaybeMutableRef(CreateIfNotExists{})
dom/cache/Manager.h
122

I meant:

struct CreateIfNotExists {};

Maybe<IndexedDatabaseManager&> IndexedDatabaseManager::MaybeMutableRef(const CreateIfNotExists&) {
  ...
}
dom/cache/Manager.h
122

I am a bit confused now. Can we stick with the class that we have here? There may be other cases, where the caller doesn't need to acquire a strong reference, and in that case the function wouldn't be called Acquire, obviously. But here the context is that the caller needs a strong reference (if that turned out to be wrong, then we wouldn't have the question about Acquire) and therefore the functions returns a SafeRefPtr (directly, or within a Result). And then, I naming rule I extrapolated from what we discussed applies which goes like If a function returns a SafeRefPtr, its name should contain Acquire.

I agree that AcquireOrCreate may not be a good name, since in the "create" case, the caller also acquires a strong reference, and the name sounds like these were *exclusive* options. What about AcquireMaybeCreate or AcquireCreateIfNonExistent?

dom/cache/Manager.h
122

I thought you want to solve this in general for all type of getters.
Anyway, AcquireCreateIfNonExistent is a good compromise for now.

sg added inline comments.
dom/cache/Manager.h
269–273

I did that now in this patch.