Bug 1953970 - Fix TypeScript issues raised in BookmarkHTMLUtils.sys.mjs and SyncedBookmarksMirror.sys.mjs. r=places-reviewers,daisuke.

This also changes Sqlite.sys.mjs so that TypeScript can use the OpenedConnection type from SyncedBookmarksMirror.sys.mjs.

Differential Revision: https://phabricator.services.mozilla.com/D241427

toolkit/components/places/BookmarkHTMLUtils.sys.mjs

@@ -91,6 +91,8 @@ function base64EncodeString(aString) {
 /**
  * Provides HTML escaping for use in HTML attributes and body of the bookmarks
  * file, compatible with the old bookmarks system.
+ *
+ * @param {string} aText
  */
 function escapeHtmlEntities(aText) {
   return (aText || "")
@@ -104,6 +106,8 @@ function escapeHtmlEntities(aText) {
 /**
  * Provides URL escaping for use in HTML attributes of the bookmarks file,
  * compatible with the old bookmarks system.
+ *
+ * @param {string} aText
  */
 function escapeUrl(aText) {
   return (aText || "").replace(/"/g, "%22");
@@ -121,21 +125,20 @@ export var BookmarkHTMLUtils = Object.freeze({
   /**
    * Loads the current bookmarks hierarchy from a "bookmarks.html" file.
    *
-   * @param aSpec
+   * @param {string} aSpec
    *        String containing the "file:" URI for the existing "bookmarks.html"
    *        file to be loaded.
-   * @param [options.replace]
+   * @param {object} [options]
+   * @param {boolean} [options.replace]
    *        Whether we should erase existing bookmarks before loading.
    *        Defaults to `false`.
-   * @param [options.source]
+   * @param {number} [options.source]
    *        The bookmark change source, used to determine the sync status for
    *        imported bookmarks. Defaults to `RESTORE` if `replace = true`, or
    *        `IMPORT` otherwise.
    *
    * @returns {Promise<number>} The number of imported bookmarks, not including
-   *                           folders and separators.
+   *   folders and separators. Rejects if there is an issue.
-   * @resolves When the new bookmarks have been created.
-   * @rejects JavaScript exception.
    */
   async importFromURL(
     aSpec,
@@ -170,20 +173,19 @@ export var BookmarkHTMLUtils = Object.freeze({
   /**
    * Loads the current bookmarks hierarchy from a "bookmarks.html" file.
    *
-   * @param aFilePath
+   * @param {string} aFilePath
    *        OS.File path string of the "bookmarks.html" file to be loaded.
-   * @param [options.replace]
+   * @param {object} options
+   * @param {boolean} [options.replace]
    *        Whether we should erase existing bookmarks before loading.
    *        Defaults to `false`.
-   * @param [options.source]
+   * @param {number} [options.source]
    *        The bookmark change source, used to determine the sync status for
    *        imported bookmarks. Defaults to `RESTORE` if `replace = true`, or
    *        `IMPORT` otherwise.
    *
    * @returns {Promise<number>} The number of imported bookmarks, not including
-   *                            folders and separators
+   *   folders and separators. Rejects if there is an issue.
-   * @resolves When the new bookmarks have been created.
-   * @rejects JavaScript exception.
    */
   async importFromFile(
     aFilePath,
@@ -225,12 +227,11 @@ export var BookmarkHTMLUtils = Object.freeze({
   /**
    * Saves the current bookmarks hierarchy to a "bookmarks.html" file.
    *
-   * @param aFilePath
+   * @param {string} aFilePath
    *        OS.File path string for the "bookmarks.html" file to be created.
    *
-   * @returns {Promise}
+   * @returns {Promise<number>} The exported bookmarks count. Rejects if there
-   * @resolves To the exported bookmarks count when the file has been created.
+   *   is an issue.
-   * @rejects JavaScript exception.
    */
   async exportToFile(aFilePath) {
     let [bookmarks, count] = await lazy.PlacesBackups.getBookmarksTree();
@@ -417,7 +418,7 @@ BookmarkImporter.prototype = {
   /**
    * Handles <hr> as a separator.
    *
-   * @note Separators may have a title in old html files, though Places dropped
+   * Separators may have a title in old html files, though Places dropped
    * support for them.
    * We also don't import ADD_DATE or LAST_MODIFIED for separators because
    * pre-Places bookmarks did not support them.
@@ -438,6 +439,8 @@ BookmarkImporter.prototype = {
    * associated with the heading will be created when the tag has been closed
    * and we know the title (we don't know to create a new folder or to merge
    * with an existing one until we have the title).
+   *
+   * @param {Element} aElt
    */
   _handleHeadBegin: function handleHeadBegin(aElt) {
     let frame = this._curFrame;
@@ -657,7 +660,7 @@ BookmarkImporter.prototype = {
         this._curFrame.inDescription = true;
         break;
       case "hr":
-        this._handleSeparator(aElt);
+        this._handleSeparator();
         break;
     }
   },
@@ -707,12 +710,14 @@ BookmarkImporter.prototype = {
 
   /**
    * Converts a string date in seconds to a date object
+   *
+   * @param {string} seconds
    */
-  _convertImportedDateToInternalDate:
+  _convertImportedDateToInternalDate(seconds) {
-    function convertImportedDateToInternalDate(aDate) {
     try {
-        if (aDate && !isNaN(aDate)) {
+      let parsed = parseInt(seconds);
-          return new Date(parseInt(aDate) * 1000); // in bookmarks.html this value is in seconds
+      if (!isNaN(parsed)) {
+        return new Date(parsed * 1000); // in bookmarks.html this value is in seconds
       }
     } catch (ex) {
       // Do nothing.
@@ -794,9 +799,7 @@ BookmarkImporter.prototype = {
   /**
    * Imports the bookmarks from the importer into the places database.
    *
-   * @param {BookmarkImporter} importer The importer from which to get the
+   * @returns {Promise<number>} The number of imported bookmarks, not including
-   *                                    bookmark information.
-   * @returns {number} The number of imported bookmarks, not including
    *   folders and separators
    */
   async _importBookmarks() {
@@ -830,7 +833,7 @@ BookmarkImporter.prototype = {
    * Imports data into the places database from the supplied url.
    *
    * @param {string} href The url to import data from.
-   * @returns {number} The number of imported bookmarks, not including
+   * @returns {Promise<number>} The number of imported bookmarks, not including
    *   folders and separators.
    */
   async importFromURL(href) {
@@ -923,6 +926,9 @@ BookmarkExporter.prototype = {
     })();
   },
 
+  /**
+   * @type {?nsIConverterOutputStream}
+   */
   _converterOut: null,
 
   _write(aText) {

toolkit/components/places/SyncedBookmarksMirror.sys.mjs

@@ -47,6 +47,16 @@
  *   issues.
  */
 
+// TODO: Import Barrier from AsyncShutdown.sys.mjs - needs modifications to
+// make it easier for TypeScript.
+// TODO: Import PlacesItem from bookmarks.sys.mjs - needs it setting up for
+// TypeScript.
+/**
+ * @typedef {any} Barrier
+ * @import {OpenedConnection} from "resource://gre/modules/Sqlite.sys.mjs"
+ * @typedef {any} PlacesItem
+ */
+
 const lazy = {};
 
 ChromeUtils.defineESModuleGetters(lazy, {
@@ -115,6 +125,10 @@ class LogAdapter {
   error(message) {
     this.log.error(message);
   }
+
+  info(message) {
+    this.log.info(message);
+  }
 }
 
 /**
@@ -183,7 +197,7 @@ class ProgressTracker {
    * Returns the shutdown blocker state. This is included in shutdown hang
    * crash reports, in the `AsyncShutdownTimeout` annotation.
    *
-   * @see    `fetchState` in `AsyncShutdown` for more details.
+   * @see fetchState in `AsyncShutdown` for more details.
    * @returns {object} A stringifiable object with the recorded steps.
    */
   fetchState() {
@@ -245,7 +259,7 @@ export class SyncedBookmarksMirror {
       recordStepTelemetry,
       recordValidationTelemetry,
       finalizeAt = lazy.PlacesUtils.history.shutdownClient.jsclient,
-    } = {}
+    }
   ) {
     this.db = db;
     this.wasCorrupt = wasCorrupt;
@@ -275,6 +289,7 @@ export class SyncedBookmarksMirror {
    * newest schema version. Automatically recreates the mirror if it's corrupt;
    * throws on failure.
    *
+   * @param  {object} options
    * @param  {string} options.path
    *         The path to the mirror database file, either absolute or relative
    *         to the profile path.
@@ -289,12 +304,11 @@ export class SyncedBookmarksMirror {
    *         problems: Array)`, where `took` is the time taken to run
    *         validation in milliseconds, `checked` is the number of items
    *         checked, and `problems` is an array of named problem counts.
-   * @param  {AsyncShutdown.Barrier} [options.finalizeAt]
+   * @param  {Barrier} [options.finalizeAt]
    *         A shutdown phase, barrier, or barrier client that should
    *         automatically finalize the mirror when triggered. Exposed for
    *         testing.
-   * @returns {SyncedBookmarksMirror}
+   * @returns {Promise<SyncedBookmarksMirror>}
-   *         A mirror ready for use.
    */
   static async open(options) {
     let db = await lazy.PlacesUtils.promiseUnsafeWritableDBConnection();
@@ -337,7 +351,7 @@ export class SyncedBookmarksMirror {
    * timestamp as the "high water mark" for all downloaded records. Each sync
    * downloads and stores records that are strictly newer than this time.
    *
-   * @returns {number}
+   * @returns {Promise<number>}
    *         The high water mark time, in seconds.
    */
   async getCollectionHighWaterMark() {
@@ -367,7 +381,9 @@ export class SyncedBookmarksMirror {
    *        The collection last modified time, in seconds.
    */
   async setCollectionLastModified(lastModifiedSeconds) {
-    let lastModified = Math.floor(lastModifiedSeconds * 1000);
+    let lastModified = Math.floor(
+      /** @type {number} */ (lastModifiedSeconds) * 1000
+    );
     if (!Number.isInteger(lastModified)) {
       throw new TypeError("Invalid collection last modified time");
     }
@@ -390,7 +406,7 @@ export class SyncedBookmarksMirror {
    * Returns the bookmarks collection sync ID. This corresponds to
    * `PlacesSyncUtils.bookmarks.getSyncId`.
    *
-   * @returns {string}
+   * @returns {Promise<string>}
    *         The sync ID, or `""` if one isn't set.
    */
   async getSyncId() {
@@ -451,6 +467,7 @@ export class SyncedBookmarksMirror {
    *
    * @param {PlacesItem[]} records
    *        Sync records to store in the mirror.
+   * @param {object} [options]
    * @param {boolean} [options.needsMerge]
    *        Indicates if the records were changed remotely since the last sync,
    *        and should be merged into the local tree. This option is set to
@@ -528,6 +545,7 @@ export class SyncedBookmarksMirror {
    * on `SQLITE_BUSY`; synchronous consumers will fail after waiting for 100ms.
    * See bug 1305563, comment 122 for details.
    *
+   * @param  {object} [options]
    * @param  {number} [options.localTimeSeconds]
    *         The current local time, in seconds.
    * @param  {number} [options.remoteTimeSeconds]
@@ -541,7 +559,7 @@ export class SyncedBookmarksMirror {
    *         An abort signal that can be used to interrupt a merge when its
    *         associated `AbortController` is aborted. If omitted, the merge can
    *         still be interrupted when the mirror is finalized.
-   * @returns {Object.<string, BookmarkChangeRecord>}
+   * @returns {Promise<{ [x: string]: BookmarkChangeRecord; }>}
    *         A changeset containing locally changed and reconciled records to
    *         upload to the server, and to store in the mirror once upload
    *         succeeds.
@@ -774,7 +792,7 @@ export class SyncedBookmarksMirror {
    * Fetches the GUIDs of all items in the remote tree that need to be merged
    * into the local tree.
    *
-   * @returns {string[]}
+   * @returns {Promise<string[]>}
    *   Remotely changed GUIDs that need to be merged into Places.
    */
   async fetchUnmergedGuids() {
@@ -1145,7 +1163,7 @@ export class SyncedBookmarksMirror {
    * @param  {AbortSignal} signal
    *         Stops fetching records when the associated `AbortController`
    *         is aborted.
-   * @returns {object}
+   * @returns {Promise<{ changeRecords: any; count: number }>}
    *         A `{ changeRecords, count }` tuple, where `changeRecords` is a
    *         changeset containing Sync record cleartexts for outgoing items and
    *         tombstones, keyed by their Sync record IDs, and `count` is the
@@ -1395,6 +1413,7 @@ export class SyncedBookmarksMirror {
    * shutdown, but may also be called explicitly when the mirror is no longer
    * needed.
    *
+   * @param {object} options
    * @param {boolean} [options.alsoCleanup]
    *                  If specified, drop all temp tables, views, and triggers,
    *                  and detach from the mirror database before closing the
@@ -1481,10 +1500,10 @@ function isDatabaseCorrupt(error) {
   }
   if (error.errors) {
     return error.errors.some(
-      error =>
+      e =>
-        error instanceof Ci.mozIStorageError &&
+        e instanceof Ci.mozIStorageError &&
-        (error.result == Ci.mozIStorageError.CORRUPT ||
+        (e.result == Ci.mozIStorageError.CORRUPT ||
-          error.result == Ci.mozIStorageError.NOTADB)
+          e.result == Ci.mozIStorageError.NOTADB)
     );
   }
   return false;
@@ -1495,7 +1514,7 @@ function isDatabaseCorrupt(error) {
  * migrates the mirror schema to the latest version, and creates temporary
  * tables, views, and triggers.
  *
- * @param {Sqlite.OpenedConnection} db
+ * @param {OpenedConnection} db
  *        The Places database connection.
  * @param {string} path
  *        The full path to the mirror database file.
@@ -1527,7 +1546,7 @@ async function attachAndInitMirrorDatabase(db, path) {
 /**
  * Migrates the mirror database schema to the latest version.
  *
- * @param {Sqlite.OpenedConnection} db
+ * @param {OpenedConnection} db
  *        The mirror database connection.
  * @param {number} currentSchemaVersion
  *        The current mirror database schema version.
@@ -1577,7 +1596,7 @@ async function migrateMirrorSchema(db, currentSchemaVersion) {
  * Initializes a new mirror database, creating persistent tables, indexes, and
  * roots.
  *
- * @param {Sqlite.OpenedConnection} db
+ * @param {OpenedConnection} db
  *        The mirror database connection.
  */
 async function initializeMirrorDatabase(db) {
@@ -1655,7 +1674,7 @@ async function initializeMirrorDatabase(db) {
  * Drops all temp tables, views, and triggers used for merging, and detaches
  * from the mirror database.
  *
- * @param {Sqlite.OpenedConnection} db
+ * @param {OpenedConnection} db
  *        The mirror database connection.
  */
 async function cleanupMirrorDatabase(db) {
@@ -1680,7 +1699,7 @@ async function cleanupMirrorDatabase(db) {
  * from these roots - however, malformed records from the server which create
  * a different root *will* be created in the mirror - just not applied.
  *
- * @param {Sqlite.OpenedConnection} db
+ * @param {OpenedConnection} db
  *        The mirror database connection.
  */
 async function createMirrorRoots(db) {
@@ -1728,7 +1747,7 @@ async function createMirrorRoots(db) {
 /**
  * Creates temporary tables, views, and triggers to apply the mirror to Places.
  *
- * @param {Sqlite.OpenedConnection} db
+ * @param {OpenedConnection} db
  *        The mirror database connection.
  */
 async function initializeTempMirrorEntities(db) {
@@ -2094,7 +2113,7 @@ function validateTag(rawTag) {
  * @param  {Function} [recordTiming]
  *         An optional function with the signature `(time: Number)`, where
  *         `time` is the measured time.
- * @returns The return value of the timed function.
+ * @returns {Promise<any>} The return value of the timed function.
  */
 async function withTiming(name, func, recordTiming) {
   lazy.MirrorLog.debug(name);
@@ -2572,7 +2591,7 @@ function bagToNamedCounts(bag, names) {
  * cancellations.
  *
  * @param  {AbortSignal} finalizeSignal
- * @param  {AbortSignal?} signal
+ * @param  {AbortSignal?} interruptSignal
  * @returns {AbortSignal}
  */
 function anyAborted(finalizeSignal, interruptSignal = null) {

toolkit/modules/Sqlite.sys.mjs

@@ -1661,7 +1661,7 @@ function wrapStorageConnection(options) {
  *        (object) Options to control behavior of connection. See
  *        `openConnection`.
  */
-function OpenedConnection(connection, identifier, options = {}) {
+export function OpenedConnection(connection, identifier, options = {}) {
   // Store all connection data in a field distinct from the
   // witness. This enables us to store an additional reference to this
   // field without preventing garbage collection of
@@ -1697,7 +1697,7 @@ function convertStorageTransactionType(type) {
   return OpenedConnection.TRANSACTION_TYPES[type];
 }
 
-OpenedConnection.prototype = Object.freeze({
+OpenedConnection.prototype = {
   TRANSACTION_DEFAULT: "DEFAULT",
   TRANSACTION_DEFERRED: "DEFERRED",
   TRANSACTION_IMMEDIATE: "IMMEDIATE",
@@ -2058,7 +2058,11 @@ OpenedConnection.prototype = Object.freeze({
       stepDelayMs
     );
   },
-});
+};
+// This is frozen after the prototype has been assigned to allow TypeScript
+// identify the properties in the prototype. Ideally we'd change this to be a
+// class definition.
+OpenedConnection.prototype = Object.freeze(OpenedConnection.prototype);
 
 export var Sqlite = {
   // The maximum time to wait before considering a transaction stuck and