Merge pull request #18578 from mozilla/revert-18573-cancel_ai_requests

Revert "[Editor] Dispatch changes in prefs enableAltTextModelDownload and enableGuessAltText to the viewer (bug 1912024)"
mozilla · Aug 7, 2024 · e099840 · e099840
2 parents fef2853 + 6c005ea
commit e099840
Show file tree

Hide file tree

Showing 6 changed files with 51 additions and 162 deletions.
diff --git a/src/display/editor/stamp.js b/src/display/editor/stamp.js
@@ -141,11 +141,9 @@ class StampEditor extends AnnotationEditor {
       this._uiManager.useNewAltTextFlow &&
       this.#bitmap
     ) {
-      try {
-        // The alt-text dialog isn't opened but we still want to guess the alt
-        // text.
-        this.mlGuessAltText();
-      } catch {}
+      // The alt-text dialog isn't opened but we still want to guess the alt
+      // text.
+      this.mlGuessAltText();
     }
 
     this.div.focus();
@@ -157,11 +155,8 @@ class StampEditor extends AnnotationEditor {
     }
 
     const { mlManager } = this._uiManager;
-    if (!mlManager) {
-      throw new Error("No ML.");
-    }
-    if (!(await mlManager.isEnabledFor("altText"))) {
-      throw new Error("ML isn't enabled for alt text.");
+    if (!mlManager || !(await mlManager.isEnabledFor("altText"))) {
+      return null;
     }
     const { data, width, height } =
       imageData ||
@@ -175,18 +170,9 @@ class StampEditor extends AnnotationEditor {
         channels: data.length / (width * height),
       },
     });
-    if (!response) {
-      throw new Error("No response from the AI service.");
-    }
-    if (response.error) {
-      throw new Error("Error from the AI service.");
-    }
-    if (response.cancel) {
+    if (!response || response.error || !response.output) {
       return null;
     }
-    if (!response.output) {
-      throw new Error("No valid response from the AI service.");
-    }
     const altText = response.output;
     await this.setGuessedAltText(altText);
     if (updateAltTextData && !this.hasAltTextData()) {

diff --git a/web/app.js b/web/app.js
@@ -404,7 +404,9 @@ const PDFViewerApplication = {
     } else {
       eventBus = new EventBus();
     }
-    this.mlManager?.setEventBus(eventBus, this._globalAbortController.signal);
+    if (this.mlManager) {
+      this.mlManager.eventBus = eventBus;
+    }
     this.eventBus = eventBus;
 
     this.overlayManager = new OverlayManager();

diff --git a/web/app_options.js b/web/app_options.js
@@ -193,12 +193,12 @@ const defaultOptions = {
   enableAltTextModelDownload: {
     /** @type {boolean} */
     value: true,
-    kind: OptionKind.VIEWER + OptionKind.PREFERENCE + OptionKind.EVENT_DISPATCH,
+    kind: OptionKind.VIEWER + OptionKind.PREFERENCE,
   },
   enableGuessAltText: {
     /** @type {boolean} */
     value: true,
-    kind: OptionKind.VIEWER + OptionKind.PREFERENCE + OptionKind.EVENT_DISPATCH,
+    kind: OptionKind.VIEWER + OptionKind.PREFERENCE,
   },
   enableHighlightEditor: {
     // We'll probably want to make some experiments before enabling this

diff --git a/web/firefoxcom.js b/web/firefoxcom.js
@@ -307,15 +307,11 @@ class FirefoxScripting {
 }
 
 class MLManager {
-  #abortSignal = null;
-
   #enabled = null;
 
-  #eventBus = null;
-
   #ready = null;
 
-  #requestResolvers = null;
+  eventBus = null;
 
   hasProgress = false;
 
@@ -334,32 +330,6 @@ class MLManager {
     this.enableGuessAltText = enableGuessAltText;
   }
 
-  setEventBus(eventBus, abortSignal) {
-    this.#eventBus = eventBus;
-    this.#abortSignal = abortSignal;
-    eventBus._on(
-      "enablealttextmodeldownload",
-      ({ value }) => {
-        if (this.enableAltTextModelDownload === value) {
-          return;
-        }
-        if (value) {
-          this.downloadModel("altText");
-        } else {
-          this.deleteModel("altText");
-        }
-      },
-      abortSignal
-    );
-    eventBus._on(
-      "enableguessalttext",
-      ({ value }) => {
-        this.toggleService("altText", value);
-      },
-      abortSignal
-    );
-  }
-
   async isEnabledFor(name) {
     return this.enableGuessAltText && !!(await this.#enabled?.get(name));
   }
@@ -369,17 +339,16 @@ class MLManager {
   }
 
   async deleteModel(name) {
-    if (name !== "altText" || !this.enableAltTextModelDownload) {
+    if (name !== "altText") {
       return;
     }
     this.enableAltTextModelDownload = false;
     this.#ready?.delete(name);
     this.#enabled?.delete(name);
-    await this.toggleService("altText", false);
-    await FirefoxCom.requestAsync(
-      "mlDelete",
-      MLManager.#AI_ALT_TEXT_MODEL_NAME
-    );
+    await Promise.all([
+      this.toggleService("altText", false),
+      FirefoxCom.requestAsync("mlDelete", MLManager.#AI_ALT_TEXT_MODEL_NAME),
+    ]);
   }
 
   async loadModel(name) {
@@ -389,7 +358,7 @@ class MLManager {
   }
 
   async downloadModel(name) {
-    if (name !== "altText" || this.enableAltTextModelDownload) {
+    if (name !== "altText") {
       return null;
     }
     this.enableAltTextModelDownload = true;
@@ -400,53 +369,18 @@ class MLManager {
     if (data?.name !== "altText") {
       return null;
     }
-    const resolvers = (this.#requestResolvers ||= new Set());
-    const resolver = Promise.withResolvers();
-    resolvers.add(resolver);
-
     data.service = MLManager.#AI_ALT_TEXT_MODEL_NAME;
-    const requestPromise = FirefoxCom.requestAsync("mlGuess", data);
-
-    requestPromise
-      .then(response => {
-        if (resolvers.has(resolver)) {
-          resolver.resolve(response);
-          resolvers.delete(resolver);
-        }
-      })
-      .catch(reason => {
-        if (resolvers.has(resolver)) {
-          resolver.reject(reason);
-          resolvers.delete(resolver);
-        }
-      });
-
-    return resolver.promise;
-  }
-
-  async #cancelAllRequests() {
-    if (!this.#requestResolvers) {
-      return;
-    }
-    for (const resolver of this.#requestResolvers) {
-      resolver.resolve({ cancel: true });
-    }
-    this.#requestResolvers.clear();
-    this.#requestResolvers = null;
+    return FirefoxCom.requestAsync("mlGuess", data);
   }
 
   async toggleService(name, enabled) {
-    if (name !== "altText" || this.enableGuessAltText === enabled) {
+    if (name !== "altText") {
       return;
     }
 
     this.enableGuessAltText = enabled;
-    if (enabled) {
-      if (this.enableAltTextModelDownload) {
-        await this.#loadAltTextEngine(false);
-      }
-    } else {
-      this.#cancelAllRequests();
+    if (enabled && this.enableAltTextModelDownload) {
+      await this.#loadAltTextEngine(false);
     }
   }
 
@@ -469,7 +403,7 @@ class MLManager {
     if (listenToProgress) {
       this.hasProgress = true;
       const callback = ({ detail }) => {
-        this.#eventBus.dispatch("loadaiengineprogress", {
+        this.eventBus.dispatch("loadaiengineprogress", {
           source: this,
           detail,
         });
@@ -478,9 +412,7 @@ class MLManager {
           window.removeEventListener("loadAIEngineProgress", callback);
         }
       };
-      window.addEventListener("loadAIEngineProgress", callback, {
-        signal: this.#abortSignal,
-      });
+      window.addEventListener("loadAIEngineProgress", callback);
       promise.then(ok => {
         if (!ok) {
           this.hasProgress = false;

diff --git a/web/genericcom.js b/web/genericcom.js
@@ -79,10 +79,6 @@ class FakeMLManager {
     this.enableAltTextModelDownload = enableAltTextModelDownload;
   }
 
-  setEventBus(eventBus, abortSignal) {
-    this.eventBus = eventBus;
-  }
-
   async isEnabledFor(_name) {
     return this.enableGuessAltText;
   }

diff --git a/web/new_alt_text_manager.js b/web/new_alt_text_manager.js
@@ -137,10 +137,6 @@ class NewAltTextManager {
       this.#toggleDisclaimer();
     });
 
-    eventBus._on("enableguessalttext", ({ value }) => {
-      this.#toggleGuessAltText(value, /* isInitial = */ false);
-    });
-
     this.#overlayManager.register(dialog);
   }
 
@@ -251,12 +247,13 @@ class NewAltTextManager {
         this.#imageData,
         /* updateAltTextData = */ false
       );
-      if (altText) {
-        this.#guessedAltText = altText;
-        this.#wasAILoading = this.#isAILoading;
-        if (this.#isAILoading) {
-          this.#addAltText(altText);
-        }
+      if (altText === null) {
+        throw new Error("No valid response from the AI service.");
+      }
+      this.#guessedAltText = altText;
+      this.#wasAILoading = this.#isAILoading;
+      if (this.#isAILoading) {
+        this.#addAltText(altText);
       }
     } catch (e) {
       console.error(e);
@@ -461,8 +458,6 @@ class ImageAltTextSettings {
 
   #createModelButton;
 
-  #downloadModelButton;
-
   #dialog;
 
   #eventBus;
@@ -491,7 +486,6 @@ class ImageAltTextSettings {
     this.#dialog = dialog;
     this.#aiModelSettings = aiModelSettings;
     this.#createModelButton = createModelButton;
-    this.#downloadModelButton = downloadModelButton;
     this.#showAltTextDialogButton = showAltTextDialogButton;
     this.#overlayManager = overlayManager;
     this.#eventBus = eventBus;
@@ -514,61 +508,40 @@ class ImageAltTextSettings {
       this.#togglePref.bind(this, "enableNewAltTextWhenAddingImage")
     );
 
-    deleteModelButton.addEventListener("click", this.#delete.bind(this, true));
-    downloadModelButton.addEventListener(
-      "click",
-      this.#download.bind(this, true)
-    );
-
-    closeButton.addEventListener("click", this.#finish.bind(this));
+    deleteModelButton.addEventListener("click", async () => {
+      await mlManager.deleteModel("altText");
 
-    eventBus._on("enablealttextmodeldownload", ({ value }) => {
-      if (value) {
-        this.#download(false);
-      } else {
-        this.#delete(false);
-      }
+      aiModelSettings.classList.toggle("download", true);
+      createModelButton.disabled = true;
+      createModelButton.setAttribute("aria-pressed", false);
+      this.#setPref("enableGuessAltText", false);
+      this.#setPref("enableAltTextModelDownload", false);
     });
 
-    this.#overlayManager.register(dialog);
-  }
-
-  async #download(isFromUI = false) {
-    if (isFromUI) {
-      this.#downloadModelButton.disabled = true;
-      this.#downloadModelButton.firstChild.setAttribute(
+    downloadModelButton.addEventListener("click", async () => {
+      downloadModelButton.disabled = true;
+      downloadModelButton.firstChild.setAttribute(
         "data-l10n-id",
         "pdfjs-editor-alt-text-settings-downloading-model-button"
       );
 
-      await this.#mlManager.downloadModel("altText");
+      await mlManager.downloadModel("altText");
 
-      this.#downloadModelButton.firstChild.setAttribute(
+      aiModelSettings.classList.toggle("download", false);
+      downloadModelButton.firstChild.setAttribute(
         "data-l10n-id",
         "pdfjs-editor-alt-text-settings-download-model-button"
       );
-
-      this.#createModelButton.disabled = false;
+      createModelButton.disabled = false;
+      createModelButton.setAttribute("aria-pressed", true);
       this.#setPref("enableGuessAltText", true);
-      this.#mlManager.toggleService("altText", true);
+      mlManager.toggleService("altText", true);
       this.#setPref("enableAltTextModelDownload", true);
-      this.#downloadModelButton.disabled = false;
-    }
-
-    this.#aiModelSettings.classList.toggle("download", false);
-    this.#createModelButton.setAttribute("aria-pressed", true);
-  }
-
-  async #delete(isFromUI = false) {
-    if (isFromUI) {
-      await this.#mlManager.deleteModel("altText");
-      this.#setPref("enableGuessAltText", false);
-      this.#setPref("enableAltTextModelDownload", false);
-    }
+      downloadModelButton.disabled = false;
+    });
 
-    this.#aiModelSettings.classList.toggle("download", true);
-    this.#createModelButton.disabled = true;
-    this.#createModelButton.setAttribute("aria-pressed", false);
+    closeButton.addEventListener("click", this.#finish.bind(this));
+    this.#overlayManager.register(dialog);
   }
 
   async open({ enableGuessAltText, enableNewAltTextWhenAddingImage }) {