1632035 - See if the providers manager or provider base class can manage query lifetimes instead of having every provider subclass do it

Status: RESOLVED DUPLICATE of bug 1652592

(Firefox :: Address Bar, task, P3)

Description

[Drew Willcoxon :adw]

Every provider subclass does something like this:

class ProviderSubclass {
  constructor() {
    // Maps the running queries by queryContext.
    this.queries = new Map();
  }
  async startQuery(queryContext, addCallback) {
    let instance = {};
    this.queries.set(queryContext, instance);
    ...
    this.queries.delete(queryContext);
  }
  cancelQuery(queryContext) {
    this.queries.delete(queryContext);
  }
}

It's not a big deal, but it would be nice if the providers manager or provider base class could take care of managing the lifetime of this.queries and its contents. Then subclasses would only need to access this.queries after they do something async.

We could even abstract that away so that subclasses could call a method like this.queryStillActive(queryContext) after an async operation. That method would be implemented in the base class of course.

Comment 1

[Marco Bonardo [:mak]]

I think this is a dupe of bug 1628016 or quite close.
My suggestion is that we should move from one provider per process to one provider per input field. Basically the controller should create a provider manager, that will instance providers. This is a bit less efficient when there are multiple inputs using the providers, but honestly it was a bit of overengineering on my side, because for a bit we'll only have one, and even having 2 or 3 instances of providers won't be that problematic.

At that point a provider can just track its state with simple boolean properties.

This is also because recently we started breaking assumptions, some providers are tracking state like they are not singletons, it's clear the approach didn't work.

Comment 2

[Drew Willcoxon :adw]

I agree, and that sounds good, but providers will still need to be able to check whether the current query after an async operation is the same as the one from before. Maybe we'll end up doing that without a provider.currentQuery property, but for now I think it's useful to keep this open. And who knows, maybe someone will have some free time and fix this one first, and then we can port that when we fix bug 1628016. But if you disagree, feel free to dupe, it's not a big deal.

Comment 3

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #2)

I agree, and that sounds good, but providers will still need to be able to check whether the current query after an async operation is the same as the one from before. Maybe we'll end up doing that without a provider.currentQuery property, but for now I think it's useful to keep this open. And who knows, maybe someone will have some free time and fix this one first, and then we can port that when we fix bug 1628016. But if you disagree, feel free to dupe, it's not a big deal.

My idea is that everything at that point would be serialized for the provider instance, if a new query starts, the previous one must be canceled (and that's what we already do), of course we could move some of that logic in the manager instead of requiring the provider to handle it, if that's what you mean to do here, sgtm.

Comment 4

[Drew Willcoxon :adw]

Yes, move shared logic that currently all providers implement into one place, and also give providers a simple way to answer the question, "is the query still active/current?" after they do something async.

Comment 5

[Marco Bonardo [:mak]]

I think I pretty much fixed this in bug 1652592