commit 0d57175fa3907c9b05f23691207abd82cfc7e1f1
parent 5b48f60d634c71a18d577fc7e8e7921d032556f5
Author: Stephen Thompson <sthompson@mozilla.com>
Date: Fri, 12 Dec 2025 21:05:20 +0000
Bug 2005382 - refactor TabNotes for tab events instead of observers r=jswinarton,tabbrowser-reviewers
Allows communicating more structured information out of the TabNotes API and should allow an easier telemetry integration.
Differential Revision: https://phabricator.services.mozilla.com/D275824
Diffstat:
9 files changed, 191 insertions(+), 190 deletions(-)
diff --git a/browser/components/tabbrowser/content/tabbrowser.js b/browser/components/tabbrowser/content/tabbrowser.js
@@ -9774,10 +9774,9 @@ var TabContextMenu = {
let contextAddNote = document.getElementById("context_addNote");
let contextEditNote = document.getElementById("context_editNote");
if (gBrowser._tabNotesEnabled) {
- let noteURL = this.contextTab.canonicalUrl;
- contextAddNote.disabled = !noteURL;
+ contextAddNote.disabled = !this.TabNotes.isEligible(this.contextTab);
- this.TabNotes.has(noteURL).then(hasNote => {
+ this.TabNotes.has(this.contextTab).then(hasNote => {
contextAddNote.hidden = hasNote;
contextEditNote.hidden = !hasNote;
});
diff --git a/browser/components/tabbrowser/content/tabnote-menu.js b/browser/components/tabbrowser/content/tabnote-menu.js
@@ -66,7 +66,9 @@
#panel;
#noteField;
#titleNode;
+ /** @type {MozTabbrowserTab} */
#currentTab = null;
+ /** @type {boolean} */
#createMode;
connectedCallback() {
@@ -146,15 +148,16 @@
return "bottomleft topleft";
}
+ /**
+ * @param {MozTabbrowserTab} tab
+ */
openPanel(tab) {
- this.#currentTab = tab;
- let url = this.#currentTab.canonicalUrl;
-
- if (!url) {
+ if (!TabNotes.isEligible(tab)) {
return;
}
+ this.#currentTab = tab;
- TabNotes.get(url).then(note => {
+ TabNotes.get(tab).then(note => {
if (note) {
this.createMode = false;
this.#noteField.value = note.text;
@@ -178,11 +181,10 @@
}
saveNote() {
- let url = this.#currentTab.canonicalUrl;
let note = this.#noteField.value;
- if (url && note.length) {
- TabNotes.set(url, note);
+ if (TabNotes.isEligible(this.#currentTab) && note.length) {
+ TabNotes.set(this.#currentTab, note);
}
this.#panel.hidePopup();
diff --git a/browser/components/tabnotes/TabNotes.sys.mjs b/browser/components/tabnotes/TabNotes.sys.mjs
@@ -63,15 +63,8 @@ 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();
+RETURNING
+ id, canonical_url, created, note_text
`;
/**
@@ -135,15 +128,29 @@ export class TabNotesStorage {
}
/**
- * Retrieve a note for a URL, if it exists.
+ * @param {MozTabbrowserTab} tab
+ * @returns {boolean}
+ */
+ isEligible(tab) {
+ if (tab?.canonicalUrl && URL.canParse(tab.canonicalUrl)) {
+ return true;
+ }
+ return false;
+ }
+
+ /**
+ * Retrieve a note for a tab, if it exists.
*
- * @param {string} url
- * The URL that the note is associated with
+ * @param {MozTabbrowserTab} tab
+ * The tab to check for a note
* @returns {Promise<TabNoteRecord|undefined>}
*/
- async get(url) {
+ async get(tab) {
+ if (!this.isEligible(tab)) {
+ return undefined;
+ }
const results = await this.#connection.executeCached(GET_NOTE_BY_URL, {
- url,
+ url: tab.canonicalUrl,
});
if (!results?.length) {
return undefined;
@@ -154,88 +161,109 @@ export class TabNotesStorage {
}
/**
- * Set a note for a URL.
+ * Set a note for a tab.
*
- * @param {string} url
- * The URL that the note should be associated with
+ * @param {MozTabbrowserTab} tab
+ * The tab that the note should be associated with
* @param {string} note
* The note itself
* @returns {Promise<TabNoteRecord>}
* The actual note that was set after sanitization
* @throws {RangeError}
- * if `url` is not a valid URL or `note` is empty
+ * if `tab` is not eligible for a tab note or `note` is empty
*/
- async set(url, note) {
- if (!URL.canParse(url)) {
- throw new RangeError("Tab notes must be associated to a valid URL");
+ async set(tab, note) {
+ if (!this.isEligible(tab)) {
+ throw new RangeError("Tab notes must be associated to an eligible tab");
}
if (!note) {
throw new RangeError("Tab note text must be provided");
}
- let existingNote = await this.get(url);
+ let existingNote = await this.get(tab);
let sanitized = this.#sanitizeInput(note);
if (existingNote && existingNote.text == sanitized) {
- return Promise.resolve(existingNote);
+ return existingNote;
}
return this.#connection.executeTransaction(async () => {
if (!existingNote) {
const insertResult = await this.#connection.executeCached(CREATE_NOTE, {
- url,
+ url: tab.canonicalUrl,
note: sanitized,
});
const insertedRecord = this.#mapDbRowToRecord(insertResult[0]);
- Services.obs.notifyObservers(null, "TabNote:Created", url);
+ tab.dispatchEvent(
+ new CustomEvent("TabNote:Created", {
+ bubbles: true,
+ detail: {
+ note: insertedRecord,
+ },
+ })
+ );
return insertedRecord;
}
const updateResult = await this.#connection.executeCached(UPDATE_NOTE, {
- url,
+ url: tab.canonicalUrl,
note: sanitized,
});
const updatedRecord = this.#mapDbRowToRecord(updateResult[0]);
- Services.obs.notifyObservers(null, "TabNote:Edited", url);
+ tab.dispatchEvent(
+ new CustomEvent("TabNote:Edited", {
+ bubbles: true,
+ detail: {
+ note: updatedRecord,
+ },
+ })
+ );
return updatedRecord;
});
}
/**
- * Delete a note for a URL.
+ * Delete a note for a tab.
*
- * @param {string} url
- * The URL of the note
+ * @param {MozTabbrowserTab} tab
+ * The tab that has a note
* @returns {Promise<boolean>}
* True if there was a note and it was deleted; false otherwise
*/
- 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;
+ async delete(tab) {
+ /** @type {mozIStorageRow[]} */
+ const deleteResult = await this.#connection.executeCached(DELETE_NOTE, {
+ url: tab.canonicalUrl,
+ });
- if (wasDeleted) {
- Services.obs.notifyObservers(null, "TabNote:Removed", url.toString());
- }
+ if (deleteResult?.length > 0) {
+ const deletedRecord = this.#mapDbRowToRecord(deleteResult[0]);
+ tab.dispatchEvent(
+ new CustomEvent("TabNote:Removed", {
+ bubbles: true,
+ detail: {
+ note: deletedRecord,
+ },
+ })
+ );
+ return true;
+ }
- return wasDeleted;
- });
+ return false;
}
/**
- * Check if a URL has a note.
+ * Check if a tab has a note.
*
- * @param {string} url
- * The URL of the note
+ * @param {MozTabbrowserTab} tab
+ * The tab to check for a tab note
* @returns {Promise<boolean>}
* True if a note is associated with this URL; false otherwise
*/
- async has(url) {
- const record = await this.get(url);
+ async has(tab) {
+ const record = await this.get(tab);
return record !== undefined;
}
diff --git a/browser/components/tabnotes/TabNotesController.sys.mjs b/browser/components/tabnotes/TabNotesController.sys.mjs
@@ -25,9 +25,12 @@ XPCOMUtils.defineLazyPreferenceGetter(
false
);
-const EVENTS = ["CanonicalURL:Identified"];
-
-const TOPICS = ["TabNote:Created", "TabNote:Edited", "TabNote:Removed"];
+const EVENTS = [
+ "CanonicalURL:Identified",
+ "TabNote:Created",
+ "TabNote:Edited",
+ "TabNote:Removed",
+];
/**
* Orchestrates the tab notes life cycle.
@@ -51,8 +54,6 @@ 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 {
lazy.logConsole.info("Tab notes disabled");
}
@@ -95,22 +96,12 @@ class TabNotesControllerClass {
*/
quit() {
if (lazy.TAB_NOTES_ENABLED) {
- TOPICS.forEach(topicName => {
- try {
- Services.obs.removeObserver(this, topicName);
- } catch (e) {
- lazy.logConsole.warn(
- `Failed to remove self from topic '${topicName}'`
- );
- }
- });
lazy.TabNotes.deinit();
- lazy.logConsole.debug("quit", TOPICS);
}
}
/**
- * @param {CanonicalURLIdentifiedEvent} event
+ * @param {CanonicalURLIdentifiedEvent|TabNoteCreatedEvent|TabNoteRemovedEvent} event
*/
handleEvent(event) {
switch (event.type) {
@@ -124,29 +115,19 @@ class TabNotesControllerClass {
const gBrowser = browser.getTabBrowser();
const tab = gBrowser.getTabForBrowser(browser);
tab.canonicalUrl = canonicalUrl;
- lazy.TabNotes.has(canonicalUrl).then(hasTabNote => {
+ lazy.TabNotes.has(tab).then(hasTabNote => {
tab.hasTabNote = hasTabNote;
});
lazy.logConsole.debug("CanonicalURL:Identified", tab, canonicalUrl);
}
break;
- }
- }
-
- /**
- * @param {nsISupports} subject
- * @param {string} topic
- * @param {string} data
- */
- observe(subject, topic, data) {
- switch (topic) {
case "TabNote:Created":
{
// A new tab note was created for a specific canonical URL. Ensure that
// all tabs with the same canonical URL also indicate that there is a
// tab note.
- const canonicalUrl = data;
+ const { canonicalUrl } = event.target;
for (const win of lazy.BrowserWindowTracker.orderedWindows) {
for (const tab of win.gBrowser.tabs) {
if (tab.canonicalUrl == canonicalUrl) {
@@ -162,7 +143,7 @@ class TabNotesControllerClass {
// A new tab note was removed from a specific canonical URL. Ensure that
// all tabs with the same canonical URL also indicate that there is no
// longer a tab note.
- const canonicalUrl = data;
+ const { canonicalUrl } = event.target;
for (const win of lazy.BrowserWindowTracker.orderedWindows) {
for (const tab of win.gBrowser.tabs) {
if (tab.canonicalUrl == canonicalUrl) {
diff --git a/browser/components/tabnotes/test/browser/browser_tab_notes_menu.js b/browser/components/tabnotes/test/browser/browser_tab_notes_menu.js
@@ -61,36 +61,6 @@ async function closeTabNoteMenu() {
return menuHidden;
}
-/**
- * Adds a tab, waits for it to load, and waits for the canonical URL to be
- * identified from the loaded contents of the tab. This ensures that the tab
- * meets the requirements to have a tab note.
- *
- * @param {string} [url="https://www.example.com"]
- * @returns {Promise<{tab: MozTabbrowserTab, canonicalUrl: string}>}
- */
-async function addTabWithCanonicalUrl(url = "https://www.example.com") {
- const { promise, resolve } = Promise.withResolvers();
- BrowserTestUtils.addTab(
- gBrowser,
- url,
- {
- skipAnimation: true,
- },
- tab => {
- BrowserTestUtils.waitForEvent(
- window,
- "CanonicalURL:Identified",
- false,
- e => e.target == tab.linkedBrowser
- ).then(e => {
- resolve({ tab, canonicalUrl: e.detail.canonicalUrl });
- });
- }
- );
- return promise;
-}
-
add_task(async function test_tabContextMenu_prefDisabled() {
// open context menu with tab notes disabled
await SpecialPowers.pushPrefEnv({
@@ -176,22 +146,23 @@ add_task(async function test_saveTabNote() {
await SpecialPowers.pushPrefEnv({
set: [["browser.tabs.notes.enabled", true]],
});
- let { tab, canonicalUrl } = await addTabWithCanonicalUrl();
+ let tab = BrowserTestUtils.addTab(gBrowser, "https://www.example.com");
+ await BrowserTestUtils.browserLoaded(tab.linkedBrowser);
let tabNoteMenu = await openTabNoteMenu(tab);
tabNoteMenu.querySelector("textarea").value = "Lorem ipsum dolor";
let menuHidden = BrowserTestUtils.waitForPopupEvent(tabNoteMenu, "hidden");
- let tabNoteCreated = observeTabNoteCreated(canonicalUrl);
+ let tabNoteCreated = BrowserTestUtils.waitForEvent(tab, "TabNote:Created");
tabNoteMenu.querySelector("#tab-note-editor-button-save").click();
await Promise.all([menuHidden, tabNoteCreated]);
- const tabNote = await TabNotes.get(canonicalUrl);
+ const tabNote = await TabNotes.get(tab);
Assert.equal(
tabNote.text,
"Lorem ipsum dolor",
"the text entered into the textarea should have been saved as a note"
);
- await TabNotes.delete(canonicalUrl);
+ await TabNotes.delete(tab);
BrowserTestUtils.removeTab(tab);
await SpecialPowers.popPrefEnv();
diff --git a/browser/components/tabnotes/test/browser/head.js b/browser/components/tabnotes/test/browser/head.js
@@ -5,27 +5,3 @@
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
@@ -5,30 +5,9 @@
const { TestUtils } = ChromeUtils.importESModule(
"resource://testing-common/TestUtils.sys.mjs"
);
+const { BrowserTestUtils } = ChromeUtils.importESModule(
+ "resource://testing-common/BrowserTestUtils.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
@@ -17,18 +17,32 @@ registerCleanupFunction(async () => {
);
});
+/**
+ * Fake for MozTabbrowserTab.
+ */
+class FakeTab extends EventTarget {
+ /**
+ * @param {string} canonicalUrl
+ */
+ constructor(canonicalUrl) {
+ super();
+ /** @type {string} */
+ this.canonicalUrl = canonicalUrl;
+ }
+}
+
add_task(async function tabNotesBasicStorageTests() {
- let url = "https://example.com/abc";
+ let tab = new FakeTab("https://example.com/abc");
let value = "some note";
let updatedValue = "some other note";
- let tabNoteCreated = observeTabNoteCreated(url);
- let firstSavedNote = await TabNotes.set(url, value);
+ let tabNoteCreated = BrowserTestUtils.waitForEvent(tab, "TabNote:Created");
+ let firstSavedNote = await TabNotes.set(tab, 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,
+ tab.canonicalUrl,
"TabNotes.set stores the right URL"
);
Assert.equal(
@@ -39,25 +53,25 @@ add_task(async function tabNotesBasicStorageTests() {
Assert.ok(firstSavedNote.created, "TabNotes.set stores a creation timestamp");
Assert.equal(
- await TabNotes.has(url),
+ await TabNotes.has(tab),
true,
- "TabNotes.has indicates that URL has a note"
+ "TabNotes.has indicates that the tab has a note"
);
Assert.deepEqual(
- await TabNotes.get(url),
+ await TabNotes.get(tab),
firstSavedNote,
"TabNotes.get returns previously set note"
);
- let tabNoteEdited = observeTabNoteEdited(url);
- let editedSavedNote = await TabNotes.set(url, updatedValue);
+ let tabNoteEdited = BrowserTestUtils.waitForEvent(tab, "TabNote:Edited");
+ let editedSavedNote = await TabNotes.set(tab, updatedValue);
Assert.ok(editedSavedNote, "TabNotes.set returns the updated tab note");
Assert.ok(await tabNoteEdited, "observers were notified of TabNote:Edited");
Assert.equal(
editedSavedNote.canonicalUrl,
- url,
+ tab.canonicalUrl,
"TabNotes.set should keep the same URL when updating"
);
Assert.equal(
@@ -71,28 +85,29 @@ add_task(async function tabNotesBasicStorageTests() {
"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");
+ const tabWithoutCanonicalUrl = new FakeTab(undefined);
+ let wasDeleted = await TabNotes.delete(tabWithoutCanonicalUrl);
Assert.ok(
!wasDeleted,
"TabNotes.delete should return false if nothing was deleted"
);
- let tabNoteRemoved = observeTabNoteRemoved(url);
- wasDeleted = await TabNotes.delete(url);
- Assert.ok(await tabNoteRemoved, "observers were notified of TabNote:Removed");
+ let tabNoteRemoved = BrowserTestUtils.waitForEvent(tab, "TabNote:Removed");
+ wasDeleted = await TabNotes.delete(tab);
+ Assert.ok(await tabNoteRemoved, "listeners were notified of TabNote:Removed");
Assert.ok(
wasDeleted,
"TabNotes.delete should return true if something was deleted"
);
Assert.equal(
- await TabNotes.has(url),
+ await TabNotes.has(tab),
false,
"TabNotes.has indicates that the deleted URL no longer has a note"
);
Assert.equal(
- await TabNotes.get(url),
+ await TabNotes.get(tab),
undefined,
"TabNotes.get returns undefined for URL that does not have a note"
);
@@ -100,28 +115,52 @@ add_task(async function tabNotesBasicStorageTests() {
await TabNotes.reset();
});
+add_task(function tabNotesIsEligible() {
+ Assert.ok(
+ TabNotes.isEligible(new FakeTab("https://example.com/")),
+ "tab with a valid canonical URL is eligible"
+ );
+ Assert.ok(
+ !TabNotes.isEligible(new FakeTab(undefined)),
+ "tab with no canonical URL is ineligible"
+ );
+ Assert.ok(
+ !TabNotes.isEligible(new FakeTab("not a valid URL")),
+ "tab with an unparseable canonical URL is ineligible"
+ );
+});
+
add_task(async function tabNotesSanitizationTests() {
- let url = "https://example.com/";
+ let tab = new FakeTab("https://example.com/");
let tooLongValue = "x".repeat(1500);
let correctValue = "x".repeat(1000);
- await TabNotes.set(url, tooLongValue);
- let result = await TabNotes.get(url);
+ const savedNote = await TabNotes.set(tab, tooLongValue);
- Assert.equal(result.text, correctValue, "TabNotes.set truncates note length");
+ Assert.equal(
+ savedNote.text,
+ correctValue,
+ "TabNotes.set truncates note length"
+ );
await TabNotes.reset();
});
add_task(async function tabNotesErrors() {
await Assert.rejects(
- TabNotes.set("not a valid URL", "valid note text"),
+ TabNotes.set(new FakeTab(undefined), "valid note text"),
+ /RangeError/,
+ "tabs without canonical URLs are not eligible for tab notes"
+ );
+
+ await Assert.rejects(
+ TabNotes.set(new FakeTab("not a valid URL"), "valid note text"),
/RangeError/,
"invalid URLs should not be allowed in TabNotes.set"
);
await Assert.rejects(
- TabNotes.set("https://example.com", ""),
+ TabNotes.set(new FakeTab("https://example.com"), ""),
/RangeError/,
"empty note text should not be allowed in TabNotes.set"
);
diff --git a/browser/components/tabnotes/types/tabnotes.ts b/browser/components/tabnotes/types/tabnotes.ts
@@ -2,6 +2,8 @@
* 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/. */
+type MozTabbrowserTab = EventTarget & { canonicalUrl: string };
+
type CanonicalURLSource = "link" | "opengraph" | "jsonLd" | "fallback";
type CanonicalURLSourceResults = {
[source in CanonicalURLSource]: string | null;
@@ -23,6 +25,30 @@ interface TabNoteRecord {
text: string;
}
+interface TabNoteCreatedEvent extends CustomEvent {
+ type: "TabNote:Created";
+ target: MozTabbrowserTab;
+ detail: {
+ note: TabNoteRecord;
+ };
+}
+
+interface TabNoteEditedEvent extends CustomEvent {
+ type: "TabNote:Edited";
+ target: MozTabbrowserTab;
+ detail: {
+ note: TabNoteRecord;
+ };
+}
+
+interface TabNoteRemovedEvent extends CustomEvent {
+ type: "TabNote:Removed";
+ target: MozTabbrowserTab;
+ detail: {
+ note: TabNoteRecord;
+ };
+}
+
type TabbrowserWebProgressListener<
ListenerName extends keyof nsIWebProgressListener,
F = nsIWebProgressListener[ListenerName],
