search

Rob--W / browser-action-jplib

Jetpack module to add a Browser action badge to the toolbar, using the chrome.browserAction syntax from Chromium
21 stars 9 forks source link

chrome-tabs-api throws a deprecation error #7

Closed oyiptong closed 10 years ago

oyiptong commented 10 years ago

Console trace:

console.error: tabsquirrel: DEPRECATED: tab.favicon is deprecated, please use require("sdk/places/favicon").getFavicon instead. Traceback (most recent call last): File "resource://gre/modules/commonjs/sdk/widget.js", line 428, in WidgetView__onEvent this._baseWidget._onEvent(type, this._public); File "resource://gre/modules/commonjs/sdk/widget.js", line 280, in _onEvent this._emit(type, eventData); File "resource://gre/modules/commonjs/sdk/deprecated/events.js", line 123, in _emit return this._emitOnObject.apply(this, args); File "resource://gre/modules/commonjs/sdk/deprecated/events.js", line 153, in _emitOnObject listener.apply(targetObj, params); File "resource://tabsquirrel-at-jetpack/browser-action/lib/browserAction.js", line 221, in createBadge/tbw<.onClick let chromeTab = chromeTabs.toChromeTab(tabs.activeTab); File "resource://tabsquirrel-at-jetpack/browser-action/lib/chrome-tabs-api.js", line 23, in sdkTabToChrome faviconUrl: sdkTab.favicon, File "resource://gre/modules/commonjs/sdk/tabs/tab-firefox.js", line 181, in TabTrait<.favicon 'tab.favicon is deprecated, ' + File "resource://gre/modules/commonjs/sdk/util/deprecate.js", line 16, in deprecateUsage let stack = get().slice(2);

oyiptong commented 10 years ago

this is with the addon sdk 1.14

oyiptong commented 10 years ago

in the release version, the module "sdk/places/favicon" doesn't seem to exist, but is present in the development version at:

https://github.com/mozilla/addon-sdk/blob/master/lib/sdk/places/favicon.js

Rob--W commented 10 years ago

It does not "throw", but it just logs a deprecation warning in dev mode.

The new Favicon SDK module is asynchronous, so it cannot be used. This property is only exposed because it was available in the sdk/tabs API, and it is also defined in the chrome.tabs.Tab type.

I have two choices:

Another option is to "lazy-access" the favicon property on demand (i.e. read), but it adds more complexity: the Tab object is a plain JavaScript object (just like Chromium), but if lazy access is desired, a reference to the original SDK Tab instance needs to be kept somewhere (e.g. in a WeakMap using the object as key). Considering that the favicon property is hardly ever used (I haven't seen any Chromium extension source code that uses this property), the option "remove the favicon property" sounds much more attractive than this alternative solution.

Remember that this module is trying to make it easy to port Chromium extensions to Firefox. This surrogate tab type is only provided for consistency with Chromium's API.

oyiptong commented 10 years ago

Excuse my lack of diligence in bug reporting.

Sounds reasonable to me.

Rob--W commented 10 years ago

No problem. If you feel that the issue has correctly been addressed, please close the issue. Otherwise, suggest what has to be improved.

oyiptong commented 10 years ago

The most correct thing to do may be to remove the favicon property, and later revisit if you ever need that functionality IMO.

Better make note of this divergence in API in some piece of documentation.