Closed Bug 1336308 Opened 8 years ago Closed 8 years ago

Documentation and follow-ups for tab abstraction code

Categories

(WebExtensions :: General, defect, P3)

Product:
WebExtensions
Component:
General
Type:
defect

Tracking

(firefox54 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

(Whiteboard: triaged)

Attachments

(5 files)

Bug 1336308: Part 1 - Fix the capitalization of innerWindowID properties.
(deleted), text/x-review-board-request
aswan
: review+
Details
Bug 1336308: Part 2 - Add a TabBase.sendMessage and TabBase.capture helper methods.
(deleted), text/x-review-board-request
aswan
: review+
Details
Bug 1336308: Part 3 - Add inline documentation for tabs API.
(deleted), text/x-review-board-request
aswan
: review+
Details
Bug 1336308: Part 4 - Rename `tab` variables that refer to native tabs to avoid confusion.
(deleted), text/x-review-board-request
aswan
: review+
Details
Bug 1336308: Part 5 - Add documentation for the Android-specific tab API helpers.
(deleted), text/x-review-board-request
aswan
: review+
Details
No description provided.
Comment on attachment 8833151 [details] Bug 1336308: Part 1 - Fix the capitalization of innerWindowID properties. https://reviewboard.mozilla.org/r/109368/#review110490 ::: browser/components/extensions/test/browser/browser_ext_browserAction_popup_preload.js:8 (Diff revision 1) > "use strict"; > > let scriptPage = url => `<html><head><meta charset="utf-8"><script src="${url}"></script></head><body>${url}</body></html>`; > > add_task(function* testBrowserActionClickCanceled() { > + let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.com/"); were the changes in this file meant to be in this patch?
Comment on attachment 8833151 [details] Bug 1336308: Part 1 - Fix the capitalization of innerWindowID properties. https://reviewboard.mozilla.org/r/109368/#review110490 > were the changes in this file meant to be in this patch? Yes. :( The incorrect capitalization of `browser.innerWindowId` led to the value being undefined, and this passing for the initial tab that actually had no initial inner window ID when it should have failed...
Comment on attachment 8833151 [details] Bug 1336308: Part 1 - Fix the capitalization of innerWindowID properties. https://reviewboard.mozilla.org/r/109368/#review110654
Attachment #8833151 - Flags: review?(aswan) → review+
Comment on attachment 8833152 [details] Bug 1336308: Part 2 - Add a TabBase.sendMessage and TabBase.capture helper methods. https://reviewboard.mozilla.org/r/109370/#review110656 It looks like you dropped permission enforcement for captureVisibleTab(). Or did it just move somewhere non-obvious? ::: toolkit/components/extensions/ExtensionTabs.jsm:58 (Diff revision 1) > + let {browser} = this; > + let {innerWindowID} = this; merge these two lines?
Comment on attachment 8833152 [details] Bug 1336308: Part 2 - Add a TabBase.sendMessage and TabBase.capture helper methods. https://reviewboard.mozilla.org/r/109370/#review110656 The permission check is already done in the schema. The redundant check was just dead code.
Comment on attachment 8833152 [details] Bug 1336308: Part 2 - Add a TabBase.sendMessage and TabBase.capture helper methods. https://reviewboard.mozilla.org/r/109370/#review110706
Attachment #8833152 - Flags: review?(aswan) → review+
Comment on attachment 8833153 [details] Bug 1336308: Part 3 - Add inline documentation for tabs API. https://reviewboard.mozilla.org/r/109372/#review110710 ::: browser/components/extensions/ext-utils.js:322 (Diff revision 1) > } > > + /** > + * Emits a "tab-attached" event for the given tab element. > + * > + * @param {NativeTab} tab you typedef'ed this in a different file, does that carry over to here or does the typedef need to be repeated here? ::: browser/components/extensions/ext-utils.js:509 (Diff revision 1) > + * of the convert() method, representing that data. > + * > + * @param {Extension} extension > + * The extension for which to convert the data. > + * @param {Object} tabData > + * Session store data for a closed tab. nit: can you add something like "in the format returned by SessionStore.getClosedTabData()" ::: browser/components/extensions/ext-utils.js:679 (Diff revision 1) > + * of the convert() method, representing that data. > + * > + * @param {Extension} extension > + * The extension for which to convert the data. > + * @param {Object} windowData > + * Session store data for a closed window. same comment as above ::: toolkit/components/extensions/ExtensionTabs.jsm:200 (Diff revision 1) > } > > + /** > + * @property {string | null} url > + * Returns the current URL of this tab if the extension has permission > + * to read it, or nul otherwise. null? ::: toolkit/components/extensions/ExtensionTabs.jsm:212 (Diff revision 1) > } > > + /** > + * @property {nsIURI | null} uri > + * Returns the current URI of this tab if the extension has permission > + * to read it, or nul otherwise. same as above ::: toolkit/components/extensions/ExtensionTabs.jsm:235 (Diff revision 1) > > > + /** > + * @property {nsIURI | null} title > + * Returns the current title of this tab if the extension has permission > + * to read it, or nul otherwise. same again ::: toolkit/components/extensions/ExtensionTabs.jsm:257 (Diff revision 1) > + } > + > + /** > + * @property {nsIURI | null} faviconUrl > + * Returns the current faviron URL of this tab if the extension has permission > + * to read it, or nul otherwise. and again ::: toolkit/components/extensions/ExtensionTabs.jsm:439 (Diff revision 1) > > return true; > } > > + /** > + * Converts this tab object to a JSON-compatible object containing the values nit: instead of JSON-compatible, say this is the extension-visible format used by methods in the browser.tabs namespace ::: toolkit/components/extensions/ExtensionTabs.jsm:485 (Diff revision 1) > + * Inserts a script or stylesheet in the given tab, and returns a promise > + * which resolves when the operation has completed. > + * > + * @param {BaseContext} context > + * The extension context for which to perform the injection. > + * @param {InjectDetails} details i don't think the contents of InjectDetails is documented anywhere? ::: toolkit/components/extensions/ExtensionTabs.jsm:666 (Diff revision 1) > > return "normal"; > } > > + /** > + * Converts this window object to a JSON-compatible object which may be same remark as above ::: toolkit/components/extensions/ExtensionTabs.jsm:838 (Diff revision 1) > + > + set state(state) { > + throw new Error("Not implemented"); > + } > + > + /* eslint-disable valid-jsdoc */ what's invalid here? ::: toolkit/components/extensions/ExtensionTabs.jsm:946 (Diff revision 1) > + * classes, which track the opening and closing of tabs, and manage mapping them > + * between numeric IDs and native tab objects. nit: "manage mapping them" -> "manage the mapping" ::: toolkit/components/extensions/ExtensionTabs.jsm:977 (Diff revision 1) > + */ > + init() { > + throw new Error("Not implemented"); > + } > + > + /* eslint-disable valid-jsdoc */ why? ::: toolkit/components/extensions/ExtensionTabs.jsm:1081 (Diff revision 1) > + * classes, which track the opening and closing of windows, and manage mapping > + * them between numeric IDs and native tab objects. same as previous comment ::: toolkit/components/extensions/ExtensionTabs.jsm:1211 (Diff revision 1) > } > throw new ExtensionError(`Invalid window ID: ${id}`); > } > > - get haveListeners() { > + /** > + * @property {boolean} haveListeners missing _ ::: toolkit/components/extensions/ExtensionTabs.jsm:1289 (Diff revision 1) > } > > + /** > + * @param {Event} event > + * A DOM event to handle. > + * @private labeling this as private is helpful for people who want to use this class, but I think it also be helpful to document (perhaps not with jsdoc, just with inline comments in the body) how this is used. ie, this is the callback for the "load" event, used to notify open listeners. similarly for observe() below ::: toolkit/components/extensions/ExtensionTabs.jsm:1595 (Diff revision 1) > } > } > } > > + /** > + * Converts the given native tab to a JSON-compatible object which may be same comment as above about how to describe the return value ::: toolkit/components/extensions/ExtensionTabs.jsm:1614 (Diff revision 1) > + * Returns a TabBase wrapper for the tab with the given ID. > + * > + * @param {integer} id > + * The ID of the tab for which to return a wrapper. > + * > + * @returns{TAbBase} nit: CApitlization ::: toolkit/components/extensions/ExtensionTabs.jsm:1651 (Diff revision 1) > > this._windows = new DefaultWeakMap(window => this.wrapWindow(window)); > } > > + /** > + * Converts the given browser window to a JSON-compatible object which may be same as above
Attachment #8833153 - Flags: review?(aswan) → review+
Comment on attachment 8833153 [details] Bug 1336308: Part 3 - Add inline documentation for tabs API. https://reviewboard.mozilla.org/r/109372/#review110710 > you typedef'ed this in a different file, does that carry over to here or does the typedef need to be repeated here? It doesn't really make a difference, at this point. The typedef doesn't need to be repeated, but at some point we're going to have to add more metadata to give JSDoc more information on namespacing so we can start generating HTML docs that make sense. > null? Fingers started getting numb by this point... > same as above Copy pasta ftw. > nit: instead of JSON-compatible, say this is the extension-visible format used by methods in the browser.tabs namespace Well, the fact that it's JSON-compatible is important, but I'll add that note as well. > i don't think the contents of InjectDetails is documented anywhere? It's documented in the schema files. I think we should be able to convert those types to something JSDoc understands easily enough. > what's invalid here? The @returns validation complains if you use it in a function that doesn't have a return statement, which means all of these abstract functions, as well as the star functions. I'm considering filing an ESLint bug to get that fixed. I guess I should add a comment. > missing _ :( You'd think the validator would complain about that. I really wish property docstrings worked the same as method docstrings... > nit: CApitlization Heh. And missing space. Classic case of accidentally typing something without realizing vim is currently focused...
Priority: -- → P3
Whiteboard: triaged
Comment on attachment 8833548 [details] Bug 1336308: Part 4 - Rename `tab` variables that refer to native tabs to avoid confusion. https://reviewboard.mozilla.org/r/109764/#review111166 Big +1 on using these names consistently. Given the size of the patch, I didn't examine each individual change or check for any cases that may have been overlooked, if there's anything that you think requires a closer look let me know.
Attachment #8833548 - Flags: review?(aswan) → review+
Comment on attachment 8833549 [details] Bug 1336308: Part 5 - Add documentation for the Android-specific tab API helpers. https://reviewboard.mozilla.org/r/109766/#review111170 ::: mobile/android/components/extensions/ext-utils.js:48 (Diff revision 2) > class BrowserProgressListener { > constructor(browser, listener, flags) { > this.listener = listener; > this.browser = browser; > this.filter = new BrowserStatusFilter(this, flags); > + this.browser.addProgressListener(this.filter, flags); doesn't this already happen in `addBrowserProgressListener()`? same with remove
Attachment #8833549 - Flags: review?(aswan) → review+
Comment on attachment 8833549 [details] Bug 1336308: Part 5 - Add documentation for the Android-specific tab API helpers. https://reviewboard.mozilla.org/r/109766/#review111170 > doesn't this already happen in `addBrowserProgressListener()`? same with remove It did originally. I moved it here since, when I was writing the documentation, I realized that made a lot more sense.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd5c80083bc2173955dd56bf9eb2663e8ad9bba Bug 1336308: Part 1 - Fix the capitalization of innerWindowID properties. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/46af1a86f5af07f8402541adbe70b4f1ee7171ae Bug 1336308: Part 2 - Add a TabBase.sendMessage and TabBase.capture helper methods. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/4b08a6a4a7faa12759cd3b6cba50cdc9f9761730 Bug 1336308: Part 3 - Add inline documentation for tabs API. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/ed963c260aef0284094facd6cd14cca29a4fd1e4 Bug 1336308: Part 4 - Rename `tab` variables that refer to native tabs to avoid confusion. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/7129bea759429f6670c350db880431cfed385456 Bug 1336308: Part 5 - Add documentation for the Android-specific tab API helpers. r=aswan
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: