598981 - change "tabs" module to use EventEmitter event registration model

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

**Work in progress** patch for this & https://bugzilla.mozilla.org/show_bug.cgi?id=601239 can be found here:

https://github.com/Gozala/addon-sdk/commit/8c06b1ed6e20357dc2b8a12a400b45d3cce60de6

Comment 2

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: v1

https://github.com/mozilla/addon-sdk/pull/31

This is not complete patch since tests and docs require update + some cleanup is required.
In any case it's good idea to start with a first round of review since changes are big and core functionality is in place already.

This patch is a fix for: Bug 598981, Bug 601239, Bug 593908

Comment 3

[Dietrich Ayala (:dietrich)]

Comment on attachment 489196 [details] [diff] [review]
v1

feedback+. approach looks good. a few nits (mostly) below.

a big concern to me is how the caller code looks now. seeing the examples converted over would help.

i wish Flightdeck had a searchable code cross-reference. would be nice to be able to see how people are using the Tabs API now, to see how much functionality is gained/lost here.

>+const thumbnailUtils = require("utils/thumbnail");

hm. would be great to expose thumbnail at the high-level too.

>+
>+// Array the inner instances of all the wrapped tabs.
>+const TABS = [];

nit: Array of the

>+
>+/**
>+ * Trait used to wrap tap elements.

nit: s/tap/tab/

>+  _onEvent: function _onEvent(type, tab) {
>+    //tabs._emit(type, tab);
>+    if (tab == this._public)
>+      this._emit(type, tab);
>+  },

remove commented out code

>+  /**
>+   * Document object of the page that is currently loaded in this tab.
>+   */
>+  get _contentDocument() this._browser.contentDocument,
>+  /**
>+   * Window object of the page that is currently loaded in this tab.
>+   */
>+  get _contentWindow() this._browser.contentWindow,
>+
>+  // Has been exposed by previous implementation, but i10s incompatible
>+  // and probably will be removed.
>+  get contentDocument() this._contentDocument,
>+  get contentWindow() this._contentWindow,

e10s. yeah will probably all need to be removed sooner or later.

>+  /**
>+   * URI of the favicon for the page currently loaded in this tab.
>+   * @type {String}
>+   */
>+  get faveicon() dataUtils.favicon(String(this.location)),

typo favicon


>+  /**
>+   * Weather or not tab is pinned (Is an app-tab).

s/Weather/Whether/

>+Tab.prototype = TabTrait.prototype
>+exports.Tab = Tab

semi-colons at end of line please

>diff --git a/packages/jetpack-core/lib/utils/data.js b/packages/jetpack-core/lib/utils/data.js

i'm not a big fan of this random bag of functions bundled here. i'd prefer just a utils.js if we do this.

>+let NetUtil = {};
>+Cu.import("resource://gre/modules/NetUtil.jsm", NetUtil);
>+NetUtil = NetUtil.NetUtil;

can you instead do:

Cu.import("resource://gre/modules/NetUtil.jsm", this);

?

>+const FaveiconService = Cc["@mozilla.org/browser/favicon-service;1"].
>+                          getService(Ci.nsIFaviconService);

typo: FaviconService

>+/**
>+ * Takes URI of the page and returns associated favicon URI.
>+ * If page under passed uri has no favicon then base64 encoded data URI of
>+ * default faveicon is returned.
>+ * @param {String} uri
>+ * @returns {String}
>+ */
>+exports.favicon = function favicon(uri) {

please turn this into a verb: getFaviconForURI, or something like that. a bit longer, but is self-documenting and EIBTI.

>+/**
>+ * Takes chrome URI and returns content under that URI.
>+ * @param {String} chromeURI
>+ * @returns {String}
>+ */
>+function chromeURIContent(chromeURI) {

ditto on verb-izing this. maybe getChromeURIContents()?

>+/**
>+ * Creates canvas element with a thumbnail of the passed window.
>+ * @param {Window} window
>+ * @returns {Element}
>+ */

Does this work with both XUL and HTML windows? Should specify in the comment.

>   _initWindowTabTracker: function _initWindowTabTracker() {
>+    this.tabs;
>+    let firstTab = null;
>+    // Some XULRunner apps may have more then one tab browser.
>+    for each (let tabContainer in this._tabContainers) {
>+      //
>+      let tabs = Array.slice(tabContainer.children);

nit: empty comment
nit: s/then/than/

Comment 4

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".

Comment 6

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Pull request was updated with a changes requested:
https://github.com/mozilla/addon-sdk/pull/31

Comment 7

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: v2

Also putting a patch.

Comment 8

[Dietrich Ayala (:dietrich)]

Comment on attachment 491472 [details] [diff] [review]
v2

r+. the changes below are mostly grammar nits or doc clarifications.

>+All the tabs and lists of tabs emit following event:

s/event/events/

>+<api name="TabList">
>+@class
>+The set sorted list of open tabs.

s/set/set of/

>+An instance of `TabList` represents a list of open tab. Tablist can represent

s/tab/tabs/

>+either list of tabs per window as in case with `BrowserWindow`'s `tabs` property

s/either a/
s/in case with/in the case of/

>+or list of all open tabs as in case of `tabs` object that is exported by this

s/in case/in the case/

>+<api name="active">
>+@property {Tab}
>+
>+The currently active tab in this list. This property can be set to a `tab`
>+object, which will focus that tab. If this is a list of all tabs setting this

s/all tabs/all tabs,/

>+property will focus parent window and bring the tab to the foreground.

s/focus parent/focus the parent/

>+Open a new tab. If this is a list of tabs for a window new tab will be open
>+on this window. If this is a list of all tabs new tab will be open in the active
>+window or in the new window depending on option being passed.

s/window new/window, the/
s/open/opened/
s/all tabs new/all tabs, the new/
s/on option/on the option/

>+@prop [pinned] {boolean}
>+If present and true, then new tab will be pinned as an app-tab.

s/then new/then the new/

>+    // Move the active tab one position to the right.
>+    tabs.active.index ++;

remove space

> <api name="title">
> @property {string}
> The title of the page currently loaded in the tab.
>-This property is read-only.
>+This property can be set to change tab title.
> </api>

s/change tab/change the tab/

>+All the windows and lists of windows emit following event:

events

> 
>+    // Open new window with two tabs.
>+    windows.openWindow({
>+      tabs: [
>+        "http://www.mysite.com",
>+        { url: "http:/mozilla.com",
>+          pinned: true                // Open this tab as apptab.
>+        }
>+      ]
>+    });
>+
> @param options {object}
> An object containing configurable options for how this window will be opened,
> as well as a callback for being notified when the window has fully opened.
>@@ -95,6 +115,13 @@ If the only option being used is `url`, then a bare string URL can be passed to
> String URL to be opened in the new window.
> This is a required property.
> 
>+@prop [tabs] {array}
>+Array of objects containing configurable options for tabs.
>+Property can be used to open new window with a several tabs. Each element
>+of the array is an options object passed to the
>+[TabList's open method](#module/addon-kit/tabs).
>+This is an optional property.

s/Property/This property/

also, in the example you pass a bare URL string, not an options object. fix the example or docs.

> <api name="tabs">
>-@property {object}
>-An object representing all the open tabs on the window. This object
>-has all the properties and methods of the `tabs` module.
>+@property {TabList}
>+An instance of [TabList](#module/addon-kit/tabs) representing live list of all
>+the open tabs for this window.
> This property is read-only.
> </api>

s/representing live/representing the live/