commit a08ead36b243d8adeea116f9787fdadf15ba3e4a
parent a06fb5aec4e9d145b2b96f03c557736df3e83f14
Author: Stephen Thompson <sthompson@mozilla.com>
Date: Mon, 8 Dec 2025 21:30:31 +0000
Bug 2000077 - sqlite persistence for tab notes r=jswinarton,tabbrowser-reviewers
Basic persistence of tab notes in a new tabnotes.sqlite database in the profile root directory.
Currently has a single table `tabnotes` with a canonical URL, note text, and creation timestamp. The TabNotesController manages the startup/shutdown cycle for the TabNotes singleton, while TabNotes talks to the database directly.
We only have an async database connection available on the main thread, so this forced the TabNotes API to become async. This is a little awkward in some cases like the tab context menu where things normally run synchronously, but hopefully there aren't too many race conditions.
This patch doesn't include any optimizations for URL matching. I was planning to store a hashed value of the canonical URL in each row in order to use the same optimizations available in places.sqlite, but I found that the places code registered a custom C++ hashing function with sqlite for use in the places database. As a result, I think optimizations should be tackled in a separate bug(s).
Differential Revision: https://phabricator.services.mozilla.com/D275025
Diffstat:
11 files changed, 436 insertions(+), 81 deletions(-)
diff --git a/browser/components/tabbrowser/content/tabbrowser.js b/browser/components/tabbrowser/content/tabbrowser.js
@@ -9741,10 +9741,12 @@ var TabContextMenu = {
let contextEditNote = document.getElementById("context_editNote");
if (gBrowser._tabNotesEnabled) {
let noteURL = this.contextTab.canonicalUrl;
- let hasNote = this.TabNotes.has(noteURL);
contextAddNote.disabled = !noteURL;
- contextAddNote.hidden = hasNote;
- contextEditNote.hidden = !hasNote;
+
+ this.TabNotes.has(noteURL).then(hasNote => {
+ contextAddNote.hidden = hasNote;
+ contextEditNote.hidden = !hasNote;
+ });
} else {
contextAddNote.hidden = true;
contextEditNote.hidden = true;
diff --git a/browser/components/tabbrowser/content/tabnote-menu.js b/browser/components/tabbrowser/content/tabnote-menu.js
@@ -150,35 +150,41 @@
this.#currentTab = tab;
let url = this.#currentTab.canonicalUrl;
- if (url) {
- let note = TabNotes.get(url);
+ if (!url) {
+ return;
+ }
+
+ TabNotes.get(url).then(note => {
if (note) {
this.createMode = false;
- this.#noteField.value = note;
+ this.#noteField.value = note.text;
} else {
this.createMode = true;
}
- } else {
- this.createMode = true;
- }
- this.#panel.addEventListener(
- "popupshown",
- () => {
- this.#noteField.focus();
- },
- {
- once: true,
- }
- );
- this.#panel.openPopup(tab, {
- position: this.#panelPosition,
+
+ this.#panel.addEventListener(
+ "popupshown",
+ () => {
+ this.#noteField.focus();
+ },
+ {
+ once: true,
+ }
+ );
+ this.#panel.openPopup(tab, {
+ position: this.#panelPosition,
+ });
});
}
saveNote() {
let url = this.#currentTab.canonicalUrl;
let note = this.#noteField.value;
- TabNotes.set(url, note);
+
+ if (url && note.length) {
+ TabNotes.set(url, note);
+ }
+
this.#panel.hidePopup();
}
}
diff --git a/browser/components/tabnotes/TabNotes.sys.mjs b/browser/components/tabnotes/TabNotes.sys.mjs
@@ -2,77 +2,241 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+/** @import { OpenedConnection } from "resource://gre/modules/Sqlite.sys.mjs" */
+
+import { Sqlite } from "resource://gre/modules/Sqlite.sys.mjs";
+
+/**
+ * @param {string} url
+ * The canonical URL of a tab note to look up
+ */
+const GET_NOTE_BY_URL = `
+SELECT
+ id,
+ canonical_url,
+ created,
+ note_text
+FROM tabnotes
+WHERE
+ canonical_url = :url
+`;
+
+/**
+ * @param {string} url
+ * The canonical URL to associate the new tab note
+ * @param {string} note
+ * The sanitized text for a tab note
+ */
+const CREATE_NOTE = `
+INSERT INTO tabnotes
+ (canonical_url, created, note_text)
+VALUES
+ (:url, unixepoch("now"), :note)
+RETURNING
+ id, canonical_url, created, note_text
+
+`;
+
+/**
+ * @param {string} url
+ * The canonical URL for the existing tab note
+ * @param {string} note
+ * The sanitized text for a tab note
+ */
+const UPDATE_NOTE = `
+UPDATE
+ tabnotes
+SET
+ note_text = :note
+WHERE
+ canonical_url = :url
+RETURNING
+ id, canonical_url, created, note_text
+`;
+
+/**
+ * @param {string} url
+ * The canonical URL of a tab note to delete
+ */
+const DELETE_NOTE = `
+DELETE FROM
+ tabnotes
+WHERE
+ canonical_url = :url
+`;
+
+/**
+ * Get the number of rows affected by the last INSERT/UPDATE/DELETE statement.
+ *
+ * @see https://sqlite.org/lang_corefunc.html#changes
+ */
+const RETURN_CHANGED = `
+SELECT changes();
+`;
+
/**
* Provides the CRUD interface for tab notes.
*/
export class TabNotesStorage {
- /** @type {Map<URL, string>} */
- #store;
+ DATABASE_FILE_NAME = Object.freeze("tabnotes.sqlite");
+
+ /** @type {OpenedConnection|undefined} */
+ #connection;
- constructor() {
- this.reset();
+ /**
+ * @param {object} [options={}]
+ * @param {string} [options.basePath=PathUtils.profileDir]
+ * Base file path to a folder where the database file should live.
+ * Defaults to the current profile's root directory.
+ * @returns {Promise<void>}
+ */
+ init(options) {
+ const basePath = options?.basePath ?? PathUtils.profileDir;
+ return Sqlite.openConnection({
+ path: PathUtils.join(basePath, this.DATABASE_FILE_NAME),
+ }).then(async connection => {
+ this.#connection = connection;
+ await this.#connection.execute("PRAGMA journal_mode = WAL");
+ await this.#connection.execute("PRAGMA wal_autocheckpoint = 16");
+
+ let currentVersion = await this.#connection.getSchemaVersion();
+
+ if (currentVersion == 1) {
+ // tabnotes schema is up to date
+ return;
+ }
+
+ if (currentVersion == 0) {
+ // version 0: create `tabnotes` table
+ await this.#connection.executeTransaction(async () => {
+ await this.#connection.execute(`
+ CREATE TABLE IF NOT EXISTS "tabnotes" (
+ id INTEGER PRIMARY KEY,
+ canonical_url TEXT NOT NULL,
+ created INTEGER NOT NULL,
+ note_text TEXT NOT NULL
+ );`);
+ await this.#connection.setSchemaVersion(1);
+ });
+ }
+ });
+ }
+
+ /**
+ * @returns {Promise<void>}
+ */
+ deinit() {
+ if (this.#connection) {
+ return this.#connection.close().then(() => {
+ this.#connection = null;
+ });
+ }
+ return Promise.resolve();
}
/**
* Retrieve a note for a URL, if it exists.
*
- * @param {URL} url
+ * @param {string} url
* The URL that the note is associated with
- * @returns {string | undefined }
+ * @returns {Promise<TabNoteRecord|undefined>}
*/
- get(url) {
- return this.#store.get(url);
+ async get(url) {
+ const results = await this.#connection.executeCached(GET_NOTE_BY_URL, {
+ url,
+ });
+ if (!results?.length) {
+ return undefined;
+ }
+ const [result] = results;
+ const record = this.#mapDbRowToRecord(result);
+ return record;
}
/**
* Set a note for a URL.
*
- * @param {URL} url
+ * @param {string} url
* The URL that the note should be associated with
* @param {string} note
* The note itself
- * @returns {string}
+ * @returns {Promise<TabNoteRecord>}
* The actual note that was set after sanitization
+ * @throws {RangeError}
+ * if `url` is not a valid URL or `note` is empty
*/
- set(url, note) {
- let existingNote = this.get(url);
+ async set(url, note) {
+ if (!URL.canParse(url)) {
+ throw new RangeError("Tab notes must be associated to a valid URL");
+ }
+ if (!note) {
+ throw new RangeError("Tab note text must be provided");
+ }
+
+ let existingNote = await this.get(url);
let sanitized = this.#sanitizeInput(note);
- this.#store.set(url, sanitized);
- if (!existingNote) {
- Services.obs.notifyObservers(null, "TabNote:Created", url.toString());
- } else if (existingNote && existingNote != sanitized) {
- Services.obs.notifyObservers(null, "TabNote:Edited", url.toString());
+
+ if (existingNote && existingNote.text == sanitized) {
+ return Promise.resolve(existingNote);
}
- return sanitized;
+ return this.#connection.executeTransaction(async () => {
+ if (!existingNote) {
+ const insertResult = await this.#connection.executeCached(CREATE_NOTE, {
+ url,
+ note: sanitized,
+ });
+
+ const insertedRecord = this.#mapDbRowToRecord(insertResult[0]);
+ Services.obs.notifyObservers(null, "TabNote:Created", url);
+ return insertedRecord;
+ }
+
+ const updateResult = await this.#connection.executeCached(UPDATE_NOTE, {
+ url,
+ note: sanitized,
+ });
+
+ const updatedRecord = this.#mapDbRowToRecord(updateResult[0]);
+ Services.obs.notifyObservers(null, "TabNote:Edited", url);
+ return updatedRecord;
+ });
}
/**
* Delete a note for a URL.
*
- * @param {URL} url
+ * @param {string} url
* The URL of the note
- * @returns {boolean}
+ * @returns {Promise<boolean>}
* True if there was a note and it was deleted; false otherwise
*/
- delete(url) {
- let wasDeleted = this.#store.delete(url);
- if (wasDeleted) {
- Services.obs.notifyObservers(null, "TabNote:Removed", url.toString());
- }
- return wasDeleted;
+ async delete(url) {
+ return this.#connection.executeTransaction(async () => {
+ await this.#connection.executeCached(DELETE_NOTE, { url });
+ /** @type {mozIStorageRow[]} */
+ const changes = await this.#connection.execute(RETURN_CHANGED);
+ const wasDeleted = changes?.length && changes[0].getInt64(0) > 0;
+
+ if (wasDeleted) {
+ Services.obs.notifyObservers(null, "TabNote:Removed", url.toString());
+ }
+
+ return wasDeleted;
+ });
}
/**
* Check if a URL has a note.
*
- * @param {URL} url
+ * @param {string} url
* The URL of the note
- * @returns {boolean}
+ * @returns {Promise<boolean>}
* True if a note is associated with this URL; false otherwise
*/
- has(url) {
- return this.#store.has(url);
+ async has(url) {
+ const record = await this.get(url);
+ return record !== undefined;
}
/**
@@ -81,12 +245,34 @@ export class TabNotesStorage {
* @returns {void}
*/
reset() {
- this.#store = new Map();
+ this.#connection.execute(`
+ DELETE FROM "tabnotes"`);
}
+ /**
+ * Given user-supplied note text, returns sanitized note text.
+ *
+ * @param {string} value
+ * @returns {string}
+ */
#sanitizeInput(value) {
return value.slice(0, 1000);
}
+
+ /**
+ * @param {mozIStorageRow} row
+ * Row returned with the following data shape:
+ * [id: number, canonical_url: string, created: number, note_text: string]
+ * @returns {TabNoteRecord}
+ */
+ #mapDbRowToRecord(row) {
+ return {
+ id: row.getDouble(0),
+ canonicalUrl: row.getString(1),
+ created: Temporal.Instant.fromEpochMilliseconds(row.getDouble(2) * 1000),
+ text: row.getString(3),
+ };
+ }
}
// Singleton object accessible from all windows
diff --git a/browser/components/tabnotes/TabNotesController.sys.mjs b/browser/components/tabnotes/TabNotesController.sys.mjs
@@ -50,6 +50,7 @@ class TabNotesControllerClass {
*/
init() {
if (lazy.TAB_NOTES_ENABLED) {
+ lazy.TabNotes.init();
TOPICS.forEach(topicName => Services.obs.addObserver(this, topicName));
lazy.logConsole.debug("init", TOPICS);
} else {
@@ -101,6 +102,7 @@ class TabNotesControllerClass {
);
}
});
+ lazy.TabNotes.deinit();
lazy.logConsole.debug("quit", TOPICS);
}
}
@@ -120,7 +122,10 @@ class TabNotesControllerClass {
const gBrowser = browser.getTabBrowser();
const tab = gBrowser.getTabForBrowser(browser);
tab.canonicalUrl = canonicalUrl;
- tab.hasTabNote = lazy.TabNotes.has(canonicalUrl);
+ lazy.TabNotes.has(canonicalUrl).then(hasTabNote => {
+ tab.hasTabNote = hasTabNote;
+ });
+
lazy.logConsole.debug("CanonicalURL:Identified", tab, canonicalUrl);
}
break;
diff --git a/browser/components/tabnotes/test/browser/browser.toml b/browser/components/tabnotes/test/browser/browser.toml
@@ -2,5 +2,8 @@
prefs = [
"browser.tabs.notes.enabled=true",
]
+support-files = [
+ "head.js",
+]
["browser_tab_notes_menu.js"]
diff --git a/browser/components/tabnotes/test/browser/browser_tab_notes_menu.js b/browser/components/tabnotes/test/browser/browser_tab_notes_menu.js
@@ -4,12 +4,8 @@
"use strict";
-const { TabNotes } = ChromeUtils.importESModule(
- "moz-src:///browser/components/tabnotes/TabNotes.sys.mjs"
-);
-
-registerCleanupFunction(() => {
- TabNotes.reset();
+registerCleanupFunction(async () => {
+ await TabNotes.reset();
});
/**
@@ -95,7 +91,7 @@ async function addTabWithCanonicalUrl(url = "https://www.example.com") {
return promise;
}
-add_task(async function test_openTabNotePanelFromContextMenu() {
+add_task(async function test_tabContextMenu_prefDisabled() {
// open context menu with tab notes disabled
await SpecialPowers.pushPrefEnv({
set: [["browser.tabs.notes.enabled", false]],
@@ -109,13 +105,19 @@ add_task(async function test_openTabNotePanelFromContextMenu() {
"'Add Note' is hidden from context menu when pref disabled"
);
await closeContextMenu(tabContextMenu);
+ BrowserTestUtils.removeTab(tab);
await SpecialPowers.popPrefEnv();
+});
+add_task(async function test_openTabNotePanelFromContextMenu() {
// open context menu with tab notes enabled
await SpecialPowers.pushPrefEnv({
set: [["browser.tabs.notes.enabled", true]],
});
- tabContextMenu = await getContextMenu(tab, "tabContextMenu");
+ let tab = BrowserTestUtils.addTab(gBrowser, "https://www.example.com");
+ await BrowserTestUtils.browserLoaded(tab.linkedBrowser);
+ let addNoteElement = document.getElementById("context_addNote");
+ let tabContextMenu = await getContextMenu(tab, "tabContextMenu");
Assert.ok(
!addNoteElement.hidden,
"'Add Note' is visible in context menu when pref enabled"
@@ -178,12 +180,18 @@ add_task(async function test_saveTabNote() {
let tabNoteMenu = await openTabNoteMenu(tab);
tabNoteMenu.querySelector("textarea").value = "Lorem ipsum dolor";
let menuHidden = BrowserTestUtils.waitForPopupEvent(tabNoteMenu, "hidden");
+ let tabNoteCreated = observeTabNoteCreated(canonicalUrl);
tabNoteMenu.querySelector("#tab-note-editor-button-save").click();
- await menuHidden;
+ await Promise.all([menuHidden, tabNoteCreated]);
- Assert.equal(TabNotes.get(canonicalUrl), "Lorem ipsum dolor");
+ const tabNote = await TabNotes.get(canonicalUrl);
+ Assert.equal(
+ tabNote.text,
+ "Lorem ipsum dolor",
+ "the text entered into the textarea should have been saved as a note"
+ );
- TabNotes.delete(canonicalUrl);
+ await TabNotes.delete(canonicalUrl);
BrowserTestUtils.removeTab(tab);
await SpecialPowers.popPrefEnv();
diff --git a/browser/components/tabnotes/test/browser/head.js b/browser/components/tabnotes/test/browser/head.js
@@ -0,0 +1,31 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+const { TabNotes } = ChromeUtils.importESModule(
+ "moz-src:///browser/components/tabnotes/TabNotes.sys.mjs"
+);
+
+/**
+ * @param {string} url
+ * @returns {Promise<[undefined, string]>}
+ */
+function observeTabNoteCreated(url) {
+ return TestUtils.topicObserved("TabNote:Created", (_, data) => data == url);
+}
+
+/**
+ * @param {string} url
+ * @returns {Promise<[undefined, string]>}
+ */
+function observeTabNoteEdited(url) {
+ return TestUtils.topicObserved("TabNote:Edited", (_, data) => data == url);
+}
+
+/**
+ * @param {string} url
+ * @returns {Promise<[undefined, string]>}
+ */
+function observeTabNoteRemoved(url) {
+ return TestUtils.topicObserved("TabNote:Removed", (_, data) => data == url);
+}
diff --git a/browser/components/tabnotes/test/unit/head.js b/browser/components/tabnotes/test/unit/head.js
@@ -0,0 +1,34 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+const { TestUtils } = ChromeUtils.importESModule(
+ "resource://testing-common/TestUtils.sys.mjs"
+);
+const { TabNotes } = ChromeUtils.importESModule(
+ "moz-src:///browser/components/tabnotes/TabNotes.sys.mjs"
+);
+
+/**
+ * @param {string} url
+ * @returns {Promise<[undefined, string]>}
+ */
+function observeTabNoteCreated(url) {
+ return TestUtils.topicObserved("TabNote:Created", (_, data) => data == url);
+}
+
+/**
+ * @param {string} url
+ * @returns {Promise<[undefined, string]>}
+ */
+function observeTabNoteEdited(url) {
+ return TestUtils.topicObserved("TabNote:Edited", (_, data) => data == url);
+}
+
+/**
+ * @param {string} url
+ * @returns {Promise<[undefined, string]>}
+ */
+function observeTabNoteRemoved(url) {
+ return TestUtils.topicObserved("TabNote:Removed", (_, data) => data == url);
+}
diff --git a/browser/components/tabnotes/test/unit/test_tab_notes.js b/browser/components/tabnotes/test/unit/test_tab_notes.js
@@ -2,44 +2,102 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-const { TabNotes } = ChromeUtils.importESModule(
- "moz-src:///browser/components/tabnotes/TabNotes.sys.mjs"
-);
+add_setup(async () => {
+ await TabNotes.init({ basePath: PathUtils.tempDir });
+});
+
+registerCleanupFunction(async () => {
+ await TabNotes.reset();
+ await TabNotes.deinit();
+ IOUtils.remove(
+ PathUtils.join(PathUtils.tempDir, TabNotes.DATABASE_FILE_NAME),
+ {
+ ignoreAbsent: true,
+ }
+ );
+});
add_task(async function tabNotesBasicStorageTests() {
let url = "https://example.com/abc";
let value = "some note";
+ let updatedValue = "some other note";
- let result = TabNotes.set(url, value);
- Assert.equal(result, value, "TabNotes.set returns note value");
+ let tabNoteCreated = observeTabNoteCreated(url);
+ let firstSavedNote = await TabNotes.set(url, value);
+ Assert.ok(firstSavedNote, "TabNotes.set returns the saved tab note");
+ Assert.ok(await tabNoteCreated, "observers were notified of TabNote:Created");
+ Assert.equal(
+ firstSavedNote.canonicalUrl,
+ url,
+ "TabNotes.set stores the right URL"
+ );
+ Assert.equal(
+ firstSavedNote.text,
+ value,
+ "TabNotes.set stores the right note text"
+ );
+ Assert.ok(firstSavedNote.created, "TabNotes.set stores a creation timestamp");
Assert.equal(
- TabNotes.has(url),
+ await TabNotes.has(url),
true,
"TabNotes.has indicates that URL has a note"
);
+ Assert.deepEqual(
+ await TabNotes.get(url),
+ firstSavedNote,
+ "TabNotes.get returns previously set note"
+ );
+
+ let tabNoteEdited = observeTabNoteEdited(url);
+ let editedSavedNote = await TabNotes.set(url, updatedValue);
+
+ Assert.ok(editedSavedNote, "TabNotes.set returns the updated tab note");
+ Assert.ok(await tabNoteEdited, "observers were notified of TabNote:Edited");
Assert.equal(
- TabNotes.get(url),
- value,
- "TabNotes.get returns previously set note value"
+ editedSavedNote.canonicalUrl,
+ url,
+ "TabNotes.set should keep the same URL when updating"
+ );
+ Assert.equal(
+ editedSavedNote.text,
+ updatedValue,
+ "TabNotes.set saved the new note text"
+ );
+ Assert.equal(
+ Temporal.Instant.compare(editedSavedNote.created, firstSavedNote.created),
+ 0,
+ "TabNotes.set should not change the creation timestamp when updating an existing note"
+ );
+
+ let wasDeleted = await TabNotes.delete("URL that does not have a tab note");
+ Assert.ok(
+ !wasDeleted,
+ "TabNotes.delete should return false if nothing was deleted"
);
- TabNotes.delete(url);
+ let tabNoteRemoved = observeTabNoteRemoved(url);
+ wasDeleted = await TabNotes.delete(url);
+ Assert.ok(await tabNoteRemoved, "observers were notified of TabNote:Removed");
+ Assert.ok(
+ wasDeleted,
+ "TabNotes.delete should return true if something was deleted"
+ );
Assert.equal(
- TabNotes.has(url),
+ await TabNotes.has(url),
false,
"TabNotes.has indicates that the deleted URL no longer has a note"
);
Assert.equal(
- TabNotes.get(url),
+ await TabNotes.get(url),
undefined,
"TabNotes.get returns undefined for URL that does not have a note"
);
- TabNotes.reset();
+ await TabNotes.reset();
});
add_task(async function tabNotesSanitizationTests() {
@@ -47,10 +105,24 @@ add_task(async function tabNotesSanitizationTests() {
let tooLongValue = "x".repeat(1500);
let correctValue = "x".repeat(1000);
- TabNotes.set(url, tooLongValue);
- let result = TabNotes.get(url);
+ await TabNotes.set(url, tooLongValue);
+ let result = await TabNotes.get(url);
+
+ Assert.equal(result.text, correctValue, "TabNotes.set truncates note length");
+
+ await TabNotes.reset();
+});
- Assert.equal(result, correctValue, "TabNotes.set truncates note length");
+add_task(async function tabNotesErrors() {
+ await Assert.rejects(
+ TabNotes.set("not a valid URL", "valid note text"),
+ /RangeError/,
+ "invalid URLs should not be allowed in TabNotes.set"
+ );
- TabNotes.reset();
+ await Assert.rejects(
+ TabNotes.set("https://example.com", ""),
+ /RangeError/,
+ "empty note text should not be allowed in TabNotes.set"
+ );
});
diff --git a/browser/components/tabnotes/test/unit/xpcshell.toml b/browser/components/tabnotes/test/unit/xpcshell.toml
@@ -2,5 +2,6 @@
prefs = [
"browser.tabs.notes.enabled=true",
]
+head = "head.js"
["test_tab_notes.js"]
diff --git a/browser/components/tabnotes/types/tabnotes.ts b/browser/components/tabnotes/types/tabnotes.ts
@@ -14,3 +14,10 @@ interface CanonicalURLIdentifiedEvent {
canonicalUrl: string;
};
}
+
+interface TabNoteRecord {
+ id: number;
+ canonicalUrl: string;
+ created: Temporal.Instant;
+ text: string;
+}
