Closed Bug 1656146 Opened 4 years ago Closed 4 years ago

Convert nsIPrinter.supportsDuplex to a Promise

Categories

(Core :: Printing: Setup, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
81 Branch
Tracking Status
firefox81 --- fixed

People

(Reporter: jwatt, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Whiteboard: [print2020_v81])

Attachments

(1 file, 1 obsolete file)

No description provided.

Some example code of a couple of approaches I've tried. Both require switching nsPrinterCUPS from using NS_DECL_ISUPPORTS to using NS_DECL_THREADSAFE_ISUPPORTS.

NS_IMETHODIMP
nsPrinterCUPS::GetSupportsDuplex(JSContext* aCx, Promise** aResult) {
  MOZ_ASSERT(aResult);

  if (mDuplexPromise) {
    *aResult = do_AddRef(mDuplexPromise).take();
    return NS_OK;
  }

  *aResult = nullptr;

  if (!XRE_IsParentProcess()) {
    return NS_ERROR_FAILURE;
  }

  nsIGlobalObject* global = xpc::CurrentNativeGlobal(aCx);
  if (NS_WARN_IF(!global)) {
    return NS_ERROR_FAILURE;
  }

  ErrorResult er;
  mDuplexPromise = Promise::Create(global, er);
  if (NS_WARN_IF(er.Failed())) {
    return er.StealNSResult();
  }


  /**
   * APPROACH 1:
   * ___________
   */

  RefPtr<nsPrinterCUPS> self = this;
  NS_DispatchBackgroundTask(
      NS_NewRunnableFunction("GetPrinterSupportsDuplexBgThread", [self]() {
        bool supportsDuplex = self->mShim.mCupsCheckDestSupported(
            CUPS_HTTP_DEFAULT, self->mPrinter, self->mPrinterInfo, CUPS_SIDES,
            CUPS_SIDES_TWO_SIDED_PORTRAIT);
        NS_DispatchToMainThread(NS_NewRunnableFunction(
            "GetPrinterSupportsDuplexMainThread", [self, supportsDuplex]() {
              self->mDuplexPromise->MaybeResolve(supportsDuplex);
            }));
      }));


  /**
   * APPROACH 2:
   * ___________
   */

  // A MozPromise to be resolved off-main thread:
  auto bgPromise = MakeRefPtr<GenericPromise::Private>(__func__);

  // Do the blocking call to query the printer on a background thread:
  // RefPtr<nsPrinterCUPS> self = this;
  NS_DispatchBackgroundTask(NS_NewRunnableFunction(
      "GetPrinterSupportsDuplexBgTask", [self, bgPromise]() {
        bool supportsDuplex = self->mShim.mCupsCheckDestSupported(
            CUPS_HTTP_DEFAULT, self->mPrinter, self->mPrinterInfo, CUPS_SIDES,
            CUPS_SIDES_TWO_SIDED_PORTRAIT);
        bgPromise->Resolve(supportsDuplex, __func__);
      }));

  // Resolve the JS Promise on the main thread after the MozPromise resolves:
  auto holder = MakeRefPtr<DOMMozPromiseRequestHolder<GenericPromise>>(global);
  RefPtr<Promise> outer = mDuplexPromise;
  bgPromise
      ->Then(
          global->EventTargetFor(TaskCategory::Other), __func__,
          [holder, outer](bool aResult) {
            holder->Complete();
            outer->MaybeResolve(aResult);
          },
          [holder, outer](nsresult aRv) {
            holder->Complete();
            outer->MaybeReject(aRv);
          })
      ->Track(*holder);



  *aResult = do_AddRef(mDuplexPromise).take();
  return NS_OK;
}

I think approach 1 should work without changing it if you use self = std::move(self) on the second closure (so you don't change the refcount off the main thread). We also have some helpers for this kind of stuff like nsMainThreadPtrHolder and such.

While at it, properly CC the printer classes, as they're going to hold
promises and it's very easy to introduce leaks otherwise.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

I think approach 1 should work without changing it if you use self = std::move(self) on the second closure (so you don't change the refcount off the main thread).

Unfortunately that did not work when I tried it. See:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f84003a8f275a0e8dc952229ba5a2257de410e9&selectedTaskRun=MumaSSbNTDmKSD4a1BB9JA.0

Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/88425cde6668 Introduce nsPrinterBase, and add facilities for async attributes queried in a background thread. r=heycam
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch
Attachment #9166965 - Attachment is obsolete: true
Assignee: jwatt → emilio
Blocks: 1651112
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: