1578370 - TriggerInitialDocumentTranslation should do error- and completion-handling at the end of the function instead of repeatedly in a bunch of if/else blocks

Status: RESOLVED FIXED

(Core :: Internationalization, task, P3)

Description

[:Gijs (he/him)]

Current code looks roughly like this:

  if (...) {
    if (promises.IsEmpty()) {
      mReady->MaybeResolveWithUndefined();
      InitialDocumentTranslationCompleted();
    } else {
      ...
      RefPtr<Promise> promise = Promise::All(aes.cx(), promises, rv);
      if (NS_WARN_IF(!promise || rv.Failed())) {
        mReady->MaybeRejectWithUndefined();
        InitialDocumentTranslationCompleted();
        return;
      }

      RefPtr<PromiseNativeHandler> l10nReadyHandler =
          new L10nReadyHandler(mReady, this);
      promise->AppendNativeHandler(l10nReadyHandler);
    }
  } else {
    RefPtr<Promise> promise = TranslateElements(elements, nullptr, rv);
    if (NS_WARN_IF(!promise || rv.Failed())) {
      mReady->MaybeRejectWithUndefined();
      InitialDocumentTranslationCompleted();
      return;
    }

    RefPtr<PromiseNativeHandler> l10nReadyHandler =
        new L10nReadyHandler(mReady, this);
    promise->AppendNativeHandler(l10nReadyHandler);
  }
  ConnectRoot(...)

The code would be easier to read and behave more consistently if it looked something like this:

RefPtr<Promise> promise;
if (...) {
  promise = Promise::All(aes.cx(), promises, rv); // also if promises list is empty; 
} else {
  promise = TranslateElements(elements, nullptr, rv);
}

if (NS_WARN_IF(!promise || rv.Failed())) {
  mReady->MaybeRejectWithUndefined();
  InitialDocumentTranslationCompleted();
  return;
}
ConnectRoot(...);
RefPtr<PromiseNativeHandler> l10nReadyHandler =
    new L10nReadyHandler(mReady, this);
promise->AppendNativeHandler(l10nReadyHandler);

The main question is whether doing this would delay initial translation and the DOMContentLoaded event in the cached case (ie where promises.IsEmpty() is true in the current code). Even if that is the case though, some variation of the above may still simplify the error case handling, keeping some kind of exception for the "fast" case that just invokes the l10nReadyHandler immediately.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1578370 - Refactor TriggerInitialDocumentTranslation for readability. r?smaug

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Gijs - does it look like what you were looking for?

Comment 3

[:Gijs (he/him)]

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #2)

Gijs - does it look like what you were looking for?

This was a suggestion from Olli in https://phabricator.services.mozilla.com/D43818#inline-267997 so I expect he'll say whether or not it was what he meant in review... but it's how I understood it, yes.

Did you do any testing to see what perf impact this has, given that (AIUI) it can delay when we fire DOMContentLoaded in the cached case?

Comment 4

[:Gijs (he/him)]

FWIW, if we want to avoid the extra scheduling, I believe in C++ you can simply inspect the promise. That is, you can check whether its state is resolved, and if so, not attach a resolution handler but just call InitialDocumentTranslationCompleted immediately and mark mReady resolved, too. Still, that's more complex than what you have atm and perhaps it's not needed.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/798cfa0a3402
Refactor TriggerInitialDocumentTranslation for readability. r=smaug

Comment 6

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/798cfa0a3402