507258 - Implement JEP 21 - jetpack.toolbar

Status: RESOLVED FIXED

(Mozilla Labs :: Jetpack Prototype, defect, P1)

Description

[David Dahl :ddahl]

Implement JEP 21 - jetpack.toolbar

Comment 1

[David Dahl :ddahl]

Started the test and implementation. Cannot run tests on linux. Jetpack seems to be crashing. Filed bug 515432.

Comment 2

[David Dahl :ddahl]

Attachment: WIP: saving work to reboot and test on Windoze

WIP - nothing to see here:)

Comment 3

[David Dahl :ddahl]

Attachment: WIP: forgot to add the test

Comment 4

[David Dahl :ddahl]

Just curious - what is the best method to get the XUL Window document object inside a lazily-loaded js module like toolbar.js?

Comment 5

[David Dahl :ddahl]

Attachment: WIP: actually working patch, needs a large test suite

Comment 6

[David Dahl :ddahl]

Attachment: WIP 0.2

Comment 7

[David Dahl :ddahl]

Attachment: 0.3 Just need to clean up a few things

Getting there, a few things to clean up, almost ready for review

Comment 8

[David Dahl :ddahl]

Attachment: v 0.4 Works from test, closure-callback from button not fired

There is an issue where the toolbar tries to add the buttons 2 X, not sure what is happening there, an exception is thrown to keep Jetpack from adding the same button twice. 

Second issue is an error where the demo fails to get the right scope in the callback added to each button, an exception is thrown: 

SecureMembrane is not defined
[Break on this error] Filtered chrome url chrome://jetpack/content/js/secure-membrane.js Line 183

* sorry to ask for review by atul *

Comment 9

[Atul Varma [:atul]]

SecureMembranes were removed from the trunk yesterday, so that problem isn't there anymore.

I think the reason the toolbar is appearing twice is because you're using a WindowMediator to figure out what windows already exist--the thing is, BrowserWatchers actually already call onLoad() for every browser that already exist, precisely so clients wouldn't have to deal with making WindowMediators. :)  So you should be able to just get rid of all the WindowMediator code and things should work okay.

Also, you'll have to remove the call to allowForMemoryError() since I just removed that from the trunk earlier today.

Comment 10

[David Dahl :ddahl]

Ok, cool, thanks. I will have a new patch ready tomorrow.

Comment 11

[David Dahl :ddahl]

I am not sure how I would remove a button programmatically in this scenario then. I only see an "onUnLoad" method for the browserWatcher object. I guess that is why I was keeping track of each window on my own. Should I add a removeDomElement method to the BrowserWatcher object?

Comment 12

[David Dahl :ddahl]

Attachment: v0.5 - removed calls to allowForMemoryError()

I think i still need the internal window mediator until we figure out a way to generically remove dom elements from all windows based on id, etc

Comment 13

[David Dahl :ddahl]

Also, forgot to mention that the tests pass but I get this in the console:

Now running tests.test.js (line 143)
Found 20 test suites in external files.test.js (line 38)
ToolBarTests.testToolBarWorks succeededtest.js (line 68)
Memory differences: {"BrowserWatcher":2,"WindowWatcher":2}test.js (line 214)
0 out of 1 tests successful ( 1 failed ).test.js (line 228)

Comment 14

[Atul Varma [:atul]]

It looks like you're not freeing up your BrowserWatcher and WindowWatcher instances when the jetpack is unloaded.  You might want to add an "onUnload()" callback when creating a new BrowserWatcher.

BrowserWatcher should allow you to generically remove DOM elements from all windows based on ID, I think... You may want to separate out the parts of your code that should run as a "singleton" within the Jetpack Runtime vs. the parts that should run per-Jetpack.

You might also want to use MemoryTracker.track() to track the new DOM elements you're creating, e.g. toolbars.  That way you can be sure that the DOM elements are being removed when the Jetpack is unloaded.  For instance, the status bar panel tracks the iframe it creates.

Comment 15

[David Dahl :ddahl]

can I just add an unLoad property to each browserwatcher that is created? Something like: 

{onLoad: function (){...}, unLoad: function(this) { this.unload(); })

Comment 16

[David Dahl :ddahl]

Attachment: v 0.6 - WIP - not tested

Just saving my work to reboot:)

Comment 17

[David Dahl :ddahl]

Attachment: v 0.6.1 - throws when checking for id in DOM

The test suite throws my own exception (checking for an existing ID in the DOM) I thought I was cleaning up after each append() with a remove(). Atul, can we pow-pow on this?


Now running tests.test.js (line 143)
Found 21 test suites in external files.test.js (line 38)
Error NavBar button with ID of testTbButton already exists. ( Error: NavBar button with ID of testTbButton already exists. fileName=chrome://jetpack/content/js/toolbar.js )toolbar.js (line 154)
button appendedtest-toolbar.js (line 69)
button removedtest-toolbar.js (line 73)
Error BookmarkBar button with ID of testBbButton already exists. ( Error: BookmarkBar button with ID of testBbButton already exists. fileName=chrome://jetpack/content/js/toolbar.js )toolbar.js (line 236)
ToolBarTests.testToolBarWorks succeededtest.js (line 68)
Memory differences: {"BrowserWatcher":2,"WindowWatcher":2,"toolbarbutton":2}test.js (line 214)
0 out of 1 tests successful ( 1 failed ).test.js (line 228)
t.replace is not a function
[Break on this error] Filtered chrome url chrome://browser/content/browser.js

Comment 18

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.7 (WIP)

Here's my WIP. Changes:
* factored out so there's a single toolbar (and per Aza's joking suggestion, is called AtulBar)
* Made it so that buttons' ids are unique per jetpack, so we won't have issues with one jetpack trying to remove another's button
* figured out the BrowserWatcher stuff so buttons get removed and reloaded

Still to do:
* Clean out old code
* Clean up my code (a lot)
* Figure out some edge cases
* Some more tests

Comment 19

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.8 (WIP)

Still a WIP. I haven't done much testing of it, but I think it's mostly functional. Uploading so some contributors can help test!

Comment 20

[Christian Glahn]

hi Paul,

I installed the patch and run the tests. 

It turned out that the constructor expects a featureContext, which is not provided by the test case. I added some security checks, so the internal _featureContext isn't used if it is not set. 

Then I ran into the next problem. The tests try to access a "windowMediator" member of ToolBar, which does not exist. 

This means that the patch as it is does not pass the test.

Christian

Comment 21

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.9 (WIP)

Myk - I've tagged you for review for now. If you're not up for it, Drew could probably look at it too.

It's in pretty good shape now. I've touched up the problems the previous version had (eg. onclick didn't work, which is pretty important).

Tests are still not quite there. I need to add some error tests as well as actually make the current tests test something (not just assert(true)). So don't look at that.

Comment 22

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

Comment on attachment 422430 [details] [diff] [review]
Patch v0.9 (WIP)

(In reply to comment #21)
> Tests are still not quite there. I need to add some error tests as well as
> actually make the current tests test something (not just assert(true)). So
> don't look at that.

Ok, I didn't review the test code, just the implementation.


>+// CONSTANTS
>+Cu = Components.utils;

Nit: declare this via const (if it isn't already defined).  Alternately, remove it, since the rest of the patch doesn't use it.


>+const BUTTON_OPTIONS_ERROR = "Error: Malformed jetpack.toolbar button object literal.";

Nit: this message could be a bit more user-friendly; ideally, it would be multiple messages, each one identifying the specific problem encountered.


>+ToolBar.prototype = {
>+
>+  get navigation() {
>+    let bar = new AtulBar({
>+      id: "nav-bar",
>+      type: TOOLBAR_NORMAL,
>+      toolbar: this
>+    });
>+    return {
>+      append: function(options) {
>+        bar.append(options);
>+      }

Instead of creating a new object every time a consumer accesses the navigation and bookmark toolbar getters, as is currently the case, which is not only inefficient but will also confuse consumers expecting it to remain the same state-preserving object, memoize it to reuse the same object, i.e.:

  get navigation() {
    let bar = new AtulBar({
      id: "nav-bar",
      type: TOOLBAR_NORMAL,
      toolbar: this
    });
    let wrapper = {
      append: function(options) {
        bar.append(options);
      }
    };
    this.__defineGetter__("navigation", function() wrapper);
    return this.navigation;
  },


>+      //XXXzpao Remove will be added in a future release
>+      // remove: function(buttonID) {
>+      //   bar.remove(buttonID);
>+      // }

It's probably worth uncommenting this, as the patch contains an implementation that throws NS_ERROR_NOT_IMPLEMENTED, which is mildly more informative than the error the consumer will get when calling an undefined method.


>+function AtulBar(options) {

Love it!


>+  this.id = options.id;
>+  this.toolbar = options.toolbar;
>+  this.type = options.type;
>+}
>+AtulBar.prototype = {
>+  id: null,
>+  toolbar: null,
>+  type: null,

>+  buttons: {},

This creates a single collection of buttons shared across all instances of AtulBar, which is dangerous even when intentional (and thus would need to be carefully documented).

But I suspect this was actually intended to be an instance-specific store, given that buttons are indexed in this collection by their options.id values, which are not guaranteed to be unique across features.

In which case, it needs to be defined in the AtulBar constructor, i.e.:

function AtulBar(options) {
  ...
  this.buttons = {};
}
AtulBar.prototype = {
  ...
  buttons: null,


>+  append: function AtulBar_append(options) {
>+    //XXXzpao add better checking here with appropriate errors
>+    let self = this;
>+    if (options.id && this._isValidButtonID(null, options.id) &&

It's not clear why _isValidButtonID is a separate method.  It isn't used anywhere else, and all it does is return |options.id in this.buttons|, which would be simple to inline here.

But it's also not clear why this check is happening in the first place, since the options object doesn't get added to this.buttons until later in this method, so isValidButtonID should always return false at this point if this is the first time the button is being appended, which seems like it would prevent buttons from ever getting appended (unless they were already appended).

Perhaps that was meant to be a !this._isValidButtonID check (to prevent the same button from being appended twice)?


>+      // Add the browser watcher
>+      this.toolbar._browserWatchers.push(new BrowserWatcher({
>+        onLoad: function(window) {
>+          self._addButtonToWindow(window, options);
>+        },
>+        onUnload: function(window) {
>+          self._removeButtonFromWindow(window, self._generateButtonID(options.id));
>+          //XXXzpao This is pretty naive, but should be OK for now
>+          delete self.buttons[options.id];
>+        }
>+      }));
>+      this.buttons[options.id] = options;

For consistency and robustness, it may be worth adding the options to the collection of buttons in the watcher's onLoad (just as you remove it from that collection in the onUnload handler).

Ideally, this would happen in Toolbar, so AtulBar didn't have to maintain a reference to Toolbar (which creates a kind of circular reference, albeit an indirect one via a closure), although I'm not sure how to make that happen using the current architecture.


>+  remove: function AtulBar_remove(id) {
>+    //XXXzpao I think we'll need to iterate over the windows in this.toolbar
>+    throw Components.results.NS_ERROR_NOT_IMPLEMENTED

Nit: semi-colon at end of statement.


>+        button.onclick = function() {
>+          //XXXzpao Should we create a real URL from this? (eg. use urlFactory)
>+          window.document.commandDispatcher.focusedWindow.document.location = options.url;

This should behave consistently with other items on the toolbar with regards to things like targeting the load to the existing tab, a new tab, a new background tab, a new window, or a download (based on the modifier keys active during the click event).

(And perhaps it should honor restrictions based on the security principals of the feature and the URL, although it's probably enough to comment this for now and talk to Atul about doing the right thing security-wise in the reboot.)

Is there an existing function in the window's "window" object you can call, passing it the URL and the event that triggered the load?


>+      Components.utils.reportError(e);

Nit: use Cu.  Alternately, don't define Cu.


>+  _getOrCreateToolbar: function AtulBar__getOrCreateToolbar(window) {
>+    //XXXzpao Need to add ability to create toolbar.

Nit: since this doesn't actually create a toolbar, it would be better to call it simply _getToolbar.  As it is a private method, it can always be renamed later if it starts creating toolbars.


>+    //XXXzpao Consider creating a <toolbaritem> per jetpack so we could
>+    //        presumably move buttons together.
>+    return window.document.querySelector("#" + this.id);
>+  },
>+  _isValidButtonID: function AtulBar__isValidButtonID(window, id) {

Nit: remove the "window" parameter, as it isn't being used.  Alternately, inline this check the one place it is being used and remove the whole method.


>+jetpack.toolBar.navigation.append(button);
...
>+jetpack.toolBar.bookmarks.append(button2);

toolBar -> toolbar

Comment 23

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #22)
> >+// CONSTANTS
> >+Cu = Components.utils;
> 
> Nit: declare this via const (if it isn't already defined).  Alternately, remove
> it, since the rest of the patch doesn't use it.

Ah, I somehow missed the const... But I'm going to just remove this for now since utils is only being used once.

> >+const BUTTON_OPTIONS_ERROR = "Error: Malformed jetpack.toolbar button object literal.";
> 
> Nit: this message could be a bit more user-friendly; ideally, it would be
> multiple messages, each one identifying the specific problem encountered.

True, I'll break the options checking up so that I can throw more specific errors.

> >+ToolBar.prototype = {
> >+
> >+  get navigation() {
> >+    let bar = new AtulBar({
> >+      id: "nav-bar",
> >+      type: TOOLBAR_NORMAL,
> >+      toolbar: this
> >+    });
> >+    return {
> >+      append: function(options) {
> >+        bar.append(options);
> >+      }
> 
> Instead of creating a new object every time a consumer accesses the navigation
> and bookmark toolbar getters, as is currently the case, which is not only
> inefficient but will also confuse consumers expecting it to remain the same
> state-preserving object, memoize it to reuse the same object, i.e.:
> 
>   get navigation() {
>     let bar = new AtulBar({
>       id: "nav-bar",
>       type: TOOLBAR_NORMAL,
>       toolbar: this
>     });
>     let wrapper = {
>       append: function(options) {
>         bar.append(options);
>       }
>     };
>     this.__defineGetter__("navigation", function() wrapper);
>     return this.navigation;
>   },

Ahh, I definitely meant to do something along those lines. Thanks for catching it. I'm not a huge fan of that __defineGetter__ syntax, but it works (and seems to be used elsewhere, at least in Weave), so I'll probably do just that.

> >+      //XXXzpao Remove will be added in a future release
> >+      // remove: function(buttonID) {
> >+      //   bar.remove(buttonID);
> >+      // }
> 
> It's probably worth uncommenting this, as the patch contains an implementation
> that throws NS_ERROR_NOT_IMPLEMENTED, which is mildly more informative than the
> error the consumer will get when calling an undefined method.

Fair enough.

> >+  this.id = options.id;
> >+  this.toolbar = options.toolbar;
> >+  this.type = options.type;
> >+}
> >+AtulBar.prototype = {
> >+  id: null,
> >+  toolbar: null,
> >+  type: null,
> 
> >+  buttons: {},
> 
> This creates a single collection of buttons shared across all instances of
> AtulBar, which is dangerous even when intentional (and thus would need to be
> carefully documented).
> 
> But I suspect this was actually intended to be an instance-specific store,
> given that buttons are indexed in this collection by their options.id values,
> which are not guaranteed to be unique across features.
> 
> In which case, it needs to be defined in the AtulBar constructor, i.e.:
> 
> function AtulBar(options) {
>   ...
>   this.buttons = {};
> }
> AtulBar.prototype = {
>   ...
>   buttons: null,

Good catch. This is indeed what I intended. I've been writing too many services lately... I'll make sure to add a test that ensures you can in fact add buttons with the same id.

> >+  append: function AtulBar_append(options) {
> >+    //XXXzpao add better checking here with appropriate errors
> >+    let self = this;
> >+    if (options.id && this._isValidButtonID(null, options.id) &&
> 
> It's not clear why _isValidButtonID is a separate method.  It isn't used
> anywhere else, and all it does is return |options.id in this.buttons|, which
> would be simple to inline here.

It was initially more complicated and seemed to make sense to make it it's own method. That's not still the case though, so I'll inline it. I'm going to break this |if| up so I can have more error messages and will probably move all of that into it's own method.

> But it's also not clear why this check is happening in the first place, since
> the options object doesn't get added to this.buttons until later in this
> method, so isValidButtonID should always return false at this point if this is
> the first time the button is being appended, which seems like it would prevent
> buttons from ever getting appended (unless they were already appended).
> 
> Perhaps that was meant to be a !this._isValidButtonID check (to prevent the
> same button from being appended twice)?

_isValidButton already included the ! as !(in ... ) so that's why it worked. After it was clear the button was going to be added, options was added to this.buttons.

> >+      // Add the browser watcher
> >+      this.toolbar._browserWatchers.push(new BrowserWatcher({
> >+        onLoad: function(window) {
> >+          self._addButtonToWindow(window, options);
> >+        },
> >+        onUnload: function(window) {
> >+          self._removeButtonFromWindow(window, self._generateButtonID(options.id));
> >+          //XXXzpao This is pretty naive, but should be OK for now
> >+          delete self.buttons[options.id];
> >+        }
> >+      }));
> >+      this.buttons[options.id] = options;
> 
> For consistency and robustness, it may be worth adding the options to the
> collection of buttons in the watcher's onLoad (just as you remove it from that
> collection in the onUnload handler).

So the problem here is that the watcher's onLoad is called for each window (I think), so I can't add it during onLoad. I guess I could, but I'd be adding it each time. It'd just overwrite the existing object which shouldn't cause any harm.

I really wanted the |delete ...| in onUnload to be better. And actually I think what I have there might be wrong. I need to check when onUnload gets called (if it's for each window unload, then what I have is definitely wrong).

> Ideally, this would happen in Toolbar, so AtulBar didn't have to maintain a
> reference to Toolbar (which creates a kind of circular reference, albeit an
> indirect one via a closure), although I'm not sure how to make that happen
> using the current architecture.

I'll look into this (or something like it) because yes, that would be better.

> >+        button.onclick = function() {
> >+          //XXXzpao Should we create a real URL from this? (eg. use urlFactory)
> >+          window.document.commandDispatcher.focusedWindow.document.location = options.url;
> 
> This should behave consistently with other items on the toolbar with regards to
> things like targeting the load to the existing tab, a new tab, a new background
> tab, a new window, or a download (based on the modifier keys active during the
> click event).
> 
> (And perhaps it should honor restrictions based on the security principals of
> the feature and the URL, although it's probably enough to comment this for now
> and talk to Atul about doing the right thing security-wise in the reboot.)
> 
> Is there an existing function in the window's "window" object you can call,
> passing it the URL and the event that triggered the load?

I'll look for something. We obviously do it already, so there's probably something in gBrowser. If it's not easily reusible, I can duplicate that functionality. I had not thought about this at all, so nice catch.

> >+  _getOrCreateToolbar: function AtulBar__getOrCreateToolbar(window) {
> >+    //XXXzpao Need to add ability to create toolbar.
> 
> Nit: since this doesn't actually create a toolbar, it would be better to call
> it simply _getToolbar.  As it is a private method, it can always be renamed
> later if it starts creating toolbars.

Fine by me. I have a tendency to build for the future.

> >+    //XXXzpao Consider creating a <toolbaritem> per jetpack so we could
> >+    //        presumably move buttons together.
> >+    return window.document.querySelector("#" + this.id);
> >+  },
> >+  _isValidButtonID: function AtulBar__isValidButtonID(window, id) {
> 
> Nit: remove the "window" parameter, as it isn't being used.  Alternately,
> inline this check the one place it is being used and remove the whole method.

Will do as discussed above.

> >+jetpack.toolBar.navigation.append(button);
> ...
> >+jetpack.toolBar.bookmarks.append(button2);
> 
> toolBar -> toolbar

OH THANK GOODNESS. I hated writing toolBar. But that's how David did it and how the JEP syntax is (and it's consistent with statusBar). I'm more than happy to change it all to toolbar.

Comment 24

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

(In reply to comment #23)
> I'm not a huge fan of that __defineGetter__ syntax, but it works (and seems
> to be used elsewhere, at least in Weave), so I'll probably do just that.

Yeah, I too dislike the __defineGetter__ syntax, which defines a getter in the object that masks the getter in its prototype, but unfortunately it's the best option for memoizing a unary getter that is defined in a prototype, since, unlike unary getters defined in objects themselves (where you can do |delete this.getterName; return this.getterName = memoizedValue;|) you can't delete and then redefine a getter defined in a prototype (except by explicitly deleting/redefining it on the prototype object itself, but that has its own clunkinesses and only works for singletons like XPCOM service references anyway, which is not the case here).


> Good catch. This is indeed what I intended. I've been writing too many
> services lately... I'll make sure to add a test that ensures you can in fact
> add buttons with the same id.

Awesome!  Note: the other test it would be useful to write is the simple assertion that |jetpack.toolbar.navigation === jetpack.toolbar.navigation|!


> _isValidButton already included the ! as !(in ... ) so that's why it worked.

D'oh! Of course. :-)


> So the problem here is that the watcher's onLoad is called for each window
> (I think), so I can't add it during onLoad. I guess I could, but I'd be
> adding it each time. It'd just overwrite the existing object which shouldn't
> cause any harm.

Ah, indeed, that makes sense, in which case better just to add it once.


> Fine by me. I have a tendency to build for the future.

Eschew premature obfuscation! ;-)


> > >+jetpack.toolBar.navigation.append(button);
> > ...
> > >+jetpack.toolBar.bookmarks.append(button2);
> > 
> > toolBar -> toolbar
> 
> OH THANK GOODNESS. I hated writing toolBar. But that's how David did it and
> how the JEP syntax is (and it's consistent with statusBar). I'm more than
> happy to change it all to toolbar.

Maybe I'm looking at the wrong JEP, or it's been updated since, but JEP 21 <https://wiki.mozilla.org/Labs/Jetpack/JEP/21> seems to call for "toolbar", and your implementation actually exposes it that way (it's just the demo code that is using "toolBar").

For that matter, ToolBar -> Toolbar is also warranted, although optional (given that it is strictly internal).

The naming of "statusBar" is unfortunate (although changeable pre-1.0!), but "toolbar" seems to be the way to go, as the phrase "tool bar" has long since become the compound "toolbar" (even Firefox's relatively-conservative spelling checker recognizes it!).

Comment 25

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I finally figured out the magic of BrowserWatchers and am using them to great effect. I got remove working, figured out the issue I was having with windows closing, have most of the review comments addressed. New patch coming soon.

Comment 26

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.10

Everything from previous review and more, namely better tests, remove works, handles uninstalling/reinstalling.

Comment 27

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

Comment on attachment 423425 [details] [diff] [review]
Patch v0.10

Looking good, just a few more issues...


>+  Extension.addUnloadMethod(this, function() {
>+    for each (watcher in this._browserWatchers) {
>+      watcher.unload();
>+      delete watcher;
>+    }
>+  });

Nit: watcher in -> let watcher in

But as _browserWatchers is an unordered collection indexed by ID, this should actually be:

  Extension.addUnloadMethod(this, function() {
    for each (let buttonID in this._browserWatchers) {
      this._browserWatchers[buttonID].unload();
      delete this._browserWatchers[buttonID];
    }
  });


>+      button.setAttribute("image", options.imageDefault);

Out of curiosity, why is this property called "imageDefault" instead of simply "image"?


>+    } catch (e) {
>+      Components.utils.reportError(e);
>+    }

To avoid an inaccurate windowCount in the event an exception is caught here, rethrow the exception after reporting the error.


>+  _generateButtonID: function AtulBar__generateButtonID(id) {
>+    return ["jetpackbutton", this._type, this._toolbar._featureContext.id, id].join("-");
>+  },

Nit: it might make sense to call this something like _generateChromeID to distinguish its return value from the IDs by which options and browser watcher objects are indexed in the _buttons and _browserWatchers collections.


>+    <title>jetpack.toolBar demo</title>
...
>+jetpack.toolBar DEMO

Nit: toolBar -> toolbar


>+    console.log("I was clicked");

Hmm, this doesn't seem to display anything in my consoles.


>+  imageDefault: "moz-icon://.png?size=24",
...
>+  imageDefault: "moz-icon://.png?size=16",

Sadly, these don't seem to display any icons on Linux (Ubuntu 9.10), although moz-icon://goat?size=16&contentType=image/png works on Linux but not on Mac OS X (both URL styles work on Windows).  It might be bug 521495. *linux sigh*  It's probably ok to leave this as is.


One final issue: toolbar buttons on the bookmarks toolbar seem to obey the toolbar Icons/Text mode setting, at least their labels get hidden if the toolbar is set to "icons only" mode (and perhaps icons disappear in text-only mode? I didn't test because the icons weren't working for me on Linux; icons don't appear in the bookmarks toolbar on Mac; and my Windows box is not quite configured for testing this at the moment).

These buttons should ignore that setting (although the ones on the navigation toolbar should continue to obey it, as they currently do).

Comment 28

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.11

(In reply to comment #27)

> Nit: watcher in -> let watcher in
> 
> But as _browserWatchers is an unordered collection indexed by ID, this should
> actually be:
> 
>   Extension.addUnloadMethod(this, function() {
>     for each (let buttonID in this._browserWatchers) {
>       this._browserWatchers[buttonID].unload();
>       delete this._browserWatchers[buttonID];
>     }
>   });

Done.

> >+      button.setAttribute("image", options.imageDefault);
> 
> Out of curiosity, why is this property called "imageDefault" instead of simply
> "image"?

I'm pretty sure the original plan was to allow multiple images (eg, hover, mousedown) to better imitate other toolbar buttons. But I scrapped that.

So I changed it to just image and will update the JEP. It's a future and who knows what will actually happen with UI stuff anyway.

> >+    } catch (e) {
> >+      Components.utils.reportError(e);
> >+    }
> 
> To avoid an inaccurate windowCount in the event an exception is caught here,
> rethrow the exception after reporting the error.

Done. I haven't actually tested what happens here, but I haven't been able to actually throw an error.

> >+  _generateButtonID: function AtulBar__generateButtonID(id) {
> >+    return ["jetpackbutton", this._type, this._toolbar._featureContext.id, id].join("-");
> >+  },
> 
> Nit: it might make sense to call this something like _generateChromeID to
> distinguish its return value from the IDs by which options and browser watcher
> objects are indexed in the _buttons and _browserWatchers collections.

Done.

> >+    <title>jetpack.toolBar demo</title>
> ...
> >+jetpack.toolBar DEMO
> 
> Nit: toolBar -> toolbar

Done

> >+    console.log("I was clicked");
> 
> Hmm, this doesn't seem to display anything in my consoles.

It shows in my firebug console but not my JS console... meh.

> >+  imageDefault: "moz-icon://.png?size=24",
> ...
> >+  imageDefault: "moz-icon://.png?size=16",
> 
> Sadly, these don't seem to display any icons on Linux (Ubuntu 9.10), although
> moz-icon://goat?size=16&contentType=image/png works on Linux but not on Mac OS
> X (both URL styles work on Windows).  It might be bug 521495. *linux sigh* 
> It's probably ok to leave this as is.

Works here, so probably that bug. Do you know of any moz-icons that do work on Linux? I'll change those (or just use the dino)

> One final issue: toolbar buttons on the bookmarks toolbar seem to obey the
> toolbar Icons/Text mode setting, at least their labels get hidden if the
> toolbar is set to "icons only" mode (and perhaps icons disappear in text-only
> mode? I didn't test because the icons weren't working for me on Linux; icons
> don't appear in the bookmarks toolbar on Mac; and my Windows box is not quite
> configured for testing this at the moment).
> 
> These buttons should ignore that setting (although the ones on the navigation
> toolbar should continue to obey it, as they currently do).

Ok, so I changed the class so that they behave like bookmark buttons. This has the effect of matching OS though, so we won't see the image on OSX. I think that's ok (and perhaps better?). Is that OK behavior or should I make so the image is shown all the time.

Comment 29

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

Comment on attachment 423580 [details] [diff] [review]
Patch v0.11

> I'm pretty sure the original plan was to allow multiple images (eg, hover,
> mousedown) to better imitate other toolbar buttons. But I scrapped that.
> 
> So I changed it to just image and will update the JEP. It's a future and who
> knows what will actually happen with UI stuff anyway.

Sounds good.


> Works here, so probably that bug. Do you know of any moz-icons that do work on
> Linux? I'll change those (or just use the dino)

Hmm, I don't, unfortunately. :-/


> Ok, so I changed the class so that they behave like bookmark buttons. This has
> the effect of matching OS though, so we won't see the image on OSX. I think
> that's ok (and perhaps better?). Is that OK behavior or should I make so the
> image is shown all the time.

I think imitating the behavior of bookmark toolbar items on Mac OS X is the right thing to do in this case, so this is correct.


Looks good, r=myk!

Comment 30

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/labs/jetpack/rev/9feeca5fc27a

Thanks for reviewing Myk!

I filed bug 542459 to followup with more tests (mostly just for remove). But as
I mentioned above, we're fine to ship an RC without the tests and probably the
release (unless of course I happen to find a bug while writing the tests or
something shows up in RC testing).