539056 - Better Tab sync UI and discoverability

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Mike Connor [:mconnor]]

Initial thinking is something like the fennec page, but with filtering...

Comment 1

[Mike Connor [:mconnor]]

Attachment: wip

Creates a page (should probably make it about:synced-tabs or something, for prettiness) that provides a richer visualization of tabs (http://grab.by/1OSt)

Allows opening/bookmarking single/multiple entries (http://grab.by/1OSx), filtering by typing (on url/title: http://grab.by/1OSA) and is tolerably styled for Mac.  No idea how it looks on non-Mac, still playing with interaction before I get to the pretty.

This replaces the tabs from other computers submenu with a menuitem that opens the page (http://grab.by/1OS3), and adds a tab-like button to the tabbar (http://grab.by/1OS9).

Comment 2

[Mike Connor [:mconnor]]

Attachment: wip2 (needs major cleanup)

http://grab.by/1Rey

Comment 3

[Mike Connor [:mconnor]]

Attachment: wip3

getting there, need to carve out more time to hack when I have a working machine...

Comment 4

[Mike Connor [:mconnor]]

Attachment: wip4

Comment 5

[Mike Connor [:mconnor]]

Attachment: v1

Fixes this bug, bug 517821, bug 537604, and makes it really hard to hit bug 539534 or bug 550265 without trying (have to open the UI, uncheck the box, and then force a refresh... don't think it's worth a belt-and-suspenders approach there).

Comment 6

[Ed Lee :Mardak]

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

>+++ b/source/chrome/content/fx-tabs-bindings.xml
>+      <xul:hbox pack="start" align="center" onfocus="event.target.blur()" onselect="return false;">
I wonder if these events are getting triggered correctly. Or maybe there's different events for tabbing through the richlistitems.

>+++ b/source/chrome/content/fx-tabs.css
>+#tabs-display {
>+  background: #fff url(http://mozilla.seanmartell.com/weave/bg.png) repeat-x top;
! I wonder how much this adds to our size.

>+++ b/source/chrome/content/fx-tabs.js
>+  init: function () {
>+    win.gBrowser.setIcon(win.gBrowser.mTabs[i], "chrome://weave/skin/sync-16x16.png");
Setting a html:link rel="icon" doesn't work?

>+  createClientItem: function(attrs) {
>+    let client = document.createElement("richlistitem");
>+    // xxxmpc use attribute or property, not both!
>+    client.type = "client";
Why did you need to do it both ways? If you go the setAttribute route, you can do attrs.type = "client" to reuse the loop.

>+  createTabItem: function(attrs) {
These two seem suspiciously similar! ;) I guess you don't /have/ to pass in an extra "client" or "type" arguments to a shared function. :p

>+  filterTabs: function(event) {
>+    let val = event.target.value.toLowerCase();
I wonder if there's a good way to get a generic awesome search. We seem to reimplement this everywhere.. for location bar, form fill, people, tabs, etc. Does everything want case-insensitive, multi-word, on-boundary matching? Maybe.. maybe not..

>+  openSelected: function() {
>+        urls.push(items[i].getAttribute("url"));
>+      getTopWin().gBrowser.loadTabs(urls.reverse());
What's with this pushing and reversing? Do they get opened in backwards order? You can just urls.unshift() if you want to push from the other end.

>+  bookmarkSelectedTabs: function() {
>+    let items = this._tabsList.selectedItems;
>+    let URIs = [];
>+    let seenURLs = {};
Seems kinda odd to keep two separate structures for this. You could just use a single array of specs and do .indexOf(other) != -1. Then mass convert the filtered urls to URIs.

But actually.. there shouldn't be any dupes by this point anyway, no?

>+    if (URIs.length)
>+      PlacesUIUtils.showMinimalAddMultiBookmarkUI(URIs);
Can we hint at the name of the folder? Maybe the name of the client? [Folder name] might just be good enough.

>+  _generateTabList: function() {
>+    // clear out existing richlistitems
>+    let count = list.getRowCount();
>+    if (count > 0) {
>+      for (i = count - 1;i >= 0;i--)
>+        list.removeItemAt(i);
>+    }
For the download manager, it was faster and simpler to just shallow-clone the richlistbox parent and replace to get rid of the children:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/content/downloads.js#1164

>+    let haveTabs = false;
>+    for (let [guid, client] in Iterator(engine.getAllClients())) {
>+      haveTabs = true;
haveTabs isn't checked afterwards, so no tabs appears as a mysterious empty list. The ordering of clients is undefined, but I suppose no worse than what we have currently.

>+      let seenURLs = {};
>+      client.tabs.forEach(function({title, urlHistory, icon}) {
>+        let url = urlHistory[0];
>+        if (engine.locallyOpenTabMatchesURL(url) || url in seenURLs)
>+          return;
>+        seenURLs[url] = null;
So we're explicitly not re-showing duplicate urls even if they appear on multiple clients. Seems reasonable. Not sure if users will be confused if they expect certain clients to have a given tab and it's not there. But I suppose that's the same experience as having the tab already open locally.

>+        if (appendClient) {
>+          let attrs = {
>+            clientName: client.clientName,
>+            icon: Weave.Clients.isMobile(client.id) ? "chrome://weave/skin/mobile-icon.png"
>+                                                    : "chrome://weave/skin/desktop-icon.png",
Should this be just detected as part of the attribute? Either as clientType="mobile" or isMobile="true" and handling the image with css.

>+          let clientEnt = RemoteTabViewer.createClientItem(attrs);
>+          list.appendChild(clientEnt);
>+          appendClient = false;
>+          clientEnt.disabled = true;
Seems like disabled richlistitems can still be tabbed to and selected. I keep seeing the focus ring on it for some reason. Also, this entry can be context-menued and clicked. Perhaps clicking it selects all of its tabs? But right now, the context menu triggers an ineffective "Open This Tab".

>+        let attrs = {
>+          title: title || url,
>+          url:   url,
We'll get happy magic cropping for long stuff? :) Hopefully we don't need to handle tooltips...

>+          icon:  Weave.Utils.getIcon(icon, "chrome://global/skin/tree/item-grayscale.png")
What's this?

>+  adjustContextMenu: function(event) {
>+      let sf = el.getAttribute("showFor");
>+      if (sf) {
>+        el.hidden = sf != mode && sf != "all"; 
You could do this with bitfields and bit logic! ;) Nah... :p
Nit: unnecessary braces and trailing space

>+  _refetchTabs: function(force) {
>+    // if Clients hasn't synced yet this session, need to sync it as well
>+    if (Weave.Clients.lastSync == 0)
>+      Weave.Clients.sync();
Potentially a new client can be added even after already syncing this session. The bug you're avoiding here is to make sure we have the client information in-case the tab has some new client guid, yeah?

>+    let engine = Weave.Engines.get("tabs");
>+    let lastSync = engine.lastSync;
lastSync isn't used?

>+    // Force a sync only for the tabs engine
>+    engine.lastModified = null;
>+    engine.sync();
I suppose there's already been a bunch of engine.enabled before this code is reached, but it might still be a good sanity check to add a this.enabled inside of engine.sync at some point.

>+  handleClick: function(event) {
>+      let url = event.target.getAttribute("url");
>+      openUILink(url, event);
>+      let index = this._tabsList.getIndexOfItem(event.target);
>+      this._tabsList.removeItemAt(index);
Some of this could be shared with openSelected code for opening and cleaning up the list.

>+++ b/source/chrome/content/fx-tabs.xul
>+  </popupset>
>+    <richlistbox context="tabListContext" id="tabsList" seltype="multiple"
Text alignment is off by 2 spaces.

>+      <hbox align="center" style="background-color: transparent;">
>+        <hbox id="headers" align="center">
>+        <image src="chrome://weave/skin/sync-32x32.png" height="32" width="32"/>
>+        <!-- xxx fix CSS -->
But I guess good news is that these inner items will be spaced correctly! ;)

These things exist inside the richlistbox, so minor bug is that you can rightclick this area and get a context menu with just a separator and Refresh List.

>+++ b/source/chrome/content/fx-weave-overlay.js
> function FxWeaveGlue() {
>+  setTimeout(this.init, 0);
What are we running into? Unexpected events getting fired?

>+  WEAVE_TABS_URL: "about:weave-tabs",
I suppose here for now is fine. It might get moved to constants.js.in if we end up reusing this on Fennec.

>+  removeTabsUI: function () {
>+    // we need to do this manually because the tabbrowser cleanup chokes here
What's happening here? Any particular reason why we can't just lazily add these menu items and just hide/show them instead of remove/re-adding? Does the menu get rebuilt each time and apparently goes bad if there's extra items?

>+  handleEvent: function (event) {
>+    switch (event.type) {
>+      case "popupshowing":
Do we get every popupshowing event? We might end up inserting/removing a lot even if it's not the tabs menu? Not too major though..

>+  openTabsPage: function () {
Add openTabPage name to the function

>+    if (gBrowser) {
>+      let i = this.getPageIndex();
>+      if (i != -1) {
>+        gBrowser.selectTabAtIndex(i);
>+        return;
>+      }
>+      gBrowser.loadOneTab(this.WEAVE_TABS_URL, null, null, null, false);
>+    }
>+    else { // not in a browser window
>+      let win = getTopWin();
>+      if (win) {
>+        win.gBrowser.loadOneTab(this.WEAVE_TABS_URL, null, null, null, false);
This else no-gBrowser block should probably come first and try finding a gBrowser and only if it can't, do openDialog. Otherwise, slight bug in the else block always unconditionally opening the tab whereas it could find the open page.

>+++ b/source/chrome/content/fx-weave-overlay.xul
>-        <menuitem id="sync-no-tabs-menu-item" label="&syncNoTabsMenuItem.label;"
This string should be unused now.

>+++ b/source/chrome/content/notification.css
>+.weave-tab, .weave-tab:hover, .weave-tab:hover:active {
Yeah.. I've done it too. :( We need to rename this file or split it or something..

>+++ b/source/chrome/locale/en-US/fx-tabs.dtd
>+<!ENTITY tabs.searchText.label                   "Type here to find tabs...">
Can probably copy/paste the … from other string files like fx-prefs.dtd. Also, do we need ui? for strings like these? The location bar just says "Search for ..." instead of reminding that you need to type in the textbox.

>+++ b/source/chrome/locale/en-US/sync.properties
>+tabs.fromOtherComputers.label = Tabs From Other Computers
We have this string as "Tabs from my other computers" in Fennec:
http://mxr.mozilla.org/labs-central/source/weave/source/chrome/locale/en-US/fennec-tabs.dtd#1

>+++ b/source/components/Weave.js
>+function AboutWeaveService() {}
Perhaps we should call this AboutWeaveTabs in-case we add in other about:weave-foo uris.

>+++ b/source/modules/engines/clients.js
>+  isMobile: function isMobile(id) {
>+    return this._store._remoteClients[id].type == "mobile";
Might want a sanity check for [id] existing. I guess default to not mobile if it doesn't exist? I guess what you have now is that the clients engine needs to make sure to sync before calling this works.

Comment 7

[Mike Connor [:mconnor]]

(In reply to comment #6)
> (From update of attachment 435909 [details] [diff] [review])
> >+++ b/source/chrome/content/fx-tabs-bindings.xml
> >+      <xul:hbox pack="start" align="center" onfocus="event.target.blur()" onselect="return false;">
> I wonder if these events are getting triggered correctly. Or maybe there's
> different events for tabbing through the richlistitems.

It's imperfect, I'm going to file a bug to figure out how to make it perfect.

> >+++ b/source/chrome/content/fx-tabs.css
> >+#tabs-display {
> >+  background: #fff url(http://mozilla.seanmartell.com/weave/bg.png) repeat-x top;
> ! I wonder how much this adds to our size.

40k. It's not awesome, but it'll do for now. I'll see if I can get martell to design something smaller.  

> >+++ b/source/chrome/content/fx-tabs.js
> >+  init: function () {
> >+    win.gBrowser.setIcon(win.gBrowser.mTabs[i], "chrome://weave/skin/sync-16x16.png");
> Setting a html:link rel="icon" doesn't work?

Ugly vs. ugly, the one that doesn't require me to add xhtml-in-xul pain to my life wins.

> >+  createClientItem: function(attrs) {
> >+    let client = document.createElement("richlistitem");
> >+    // xxxmpc use attribute or property, not both!
> >+    client.type = "client";
> Why did you need to do it both ways? If you go the setAttribute route, you can
> do attrs.type = "client" to reuse the loop.

Because I was lazy at some point in... December?  Fixed.

> >+  createTabItem: function(attrs) {
> These two seem suspiciously similar! ;) I guess you don't /have/ to pass in an
> extra "client" or "type" arguments to a shared function. :p

They're fairly similar now, yeah.  They weren't in earlier iterations.  Fixed.

> >+  filterTabs: function(event) {
> >+    let val = event.target.value.toLowerCase();
> I wonder if there's a good way to get a generic awesome search. We seem to
> reimplement this everywhere.. for location bar, form fill, people, tabs, etc.
> Does everything want case-insensitive, multi-word, on-boundary matching?
> Maybe.. maybe not..

Swiss Army knives are pretty crappy knives.

> >+  openSelected: function() {
> >+        urls.push(items[i].getAttribute("url"));
> >+      getTopWin().gBrowser.loadTabs(urls.reverse());
> What's with this pushing and reversing? Do they get opened in backwards order?
> You can just urls.unshift() if you want to push from the other end.

I don't know why I did this.  I removed the unshift().

> >+  bookmarkSelectedTabs: function() {
> >+    let items = this._tabsList.selectedItems;
> >+    let URIs = [];
> >+    let seenURLs = {};
> Seems kinda odd to keep two separate structures for this. You could just use a
> single array of specs and do .indexOf(other) != -1. Then mass convert the
> filtered urls to URIs.
> 
> But actually.. there shouldn't be any dupes by this point anyway, no?

Indeed true, think this was cruft.  Removed!

> >+    if (URIs.length)
> >+      PlacesUIUtils.showMinimalAddMultiBookmarkUI(URIs);
> Can we hint at the name of the folder? Maybe the name of the client? [Folder
> name] might just be good enough.

This method doesn't allow that.  I think [folder name] is fine.

> >+  _generateTabList: function() {
> >+    // clear out existing richlistitems
> >+    let count = list.getRowCount();
> >+    if (count > 0) {
> >+      for (i = count - 1;i >= 0;i--)
> >+        list.removeItemAt(i);
> >+    }
> For the download manager, it was faster and simpler to just shallow-clone the
> richlistbox parent and replace to get rid of the children:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/content/downloads.js#1164

I have non-richlistitems inside of the richlistbox, or I could do that.  Getting the layout right without doing that was... not awesome.

> >+    let haveTabs = false;
> >+    for (let [guid, client] in Iterator(engine.getAllClients())) {
> >+      haveTabs = true;
> haveTabs isn't checked afterwards, so no tabs appears as a mysterious empty
> list. The ordering of clients is undefined, but I suppose no worse than what we
> have currently.

Yeah, indeed.  This is based on that code, so it has some of those limitations.  I think it's fine.

> >+      let seenURLs = {};
> >+      client.tabs.forEach(function({title, urlHistory, icon}) {
> >+        let url = urlHistory[0];
> >+        if (engine.locallyOpenTabMatchesURL(url) || url in seenURLs)
> >+          return;
> >+        seenURLs[url] = null;
> So we're explicitly not re-showing duplicate urls even if they appear on
> multiple clients. Seems reasonable. Not sure if users will be confused if they
> expect certain clients to have a given tab and it's not there. But I suppose
> that's the same experience as having the tab already open locally.

Indeed.

> >+        if (appendClient) {
> >+          let attrs = {
> >+            clientName: client.clientName,
> >+            icon: Weave.Clients.isMobile(client.id) ? "chrome://weave/skin/mobile-icon.png"
> >+                                                    : "chrome://weave/skin/desktop-icon.png",
> Should this be just detected as part of the attribute? Either as
> clientType="mobile" or isMobile="true" and handling the image with css.

list-style-image was not aligning in a way that was great.  Could do in the XBL, but meh.

> >+          let clientEnt = RemoteTabViewer.createClientItem(attrs);
> >+          list.appendChild(clientEnt);
> >+          appendClient = false;
> >+          clientEnt.disabled = true;
> Seems like disabled richlistitems can still be tabbed to and selected. I keep
> seeing the focus ring on it for some reason. Also, this entry can be
> context-menued and clicked. Perhaps clicking it selects all of its tabs? But
> right now, the context menu triggers an ineffective "Open This Tab".

Yeah, I need to fix this later, need some understanding of why we can't prevent this selection.

> >+        let attrs = {
> >+          title: title || url,
> >+          url:   url,
> We'll get happy magic cropping for long stuff? :) Hopefully we don't need to
> handle tooltips...

Yep, yay XUL.

> >+          icon:  Weave.Utils.getIcon(icon, "chrome://global/skin/tree/item-grayscale.png")
> What's this?

cruft, removed.

> >+  adjustContextMenu: function(event) {
> >+      let sf = el.getAttribute("showFor");
> >+      if (sf) {
> >+        el.hidden = sf != mode && sf != "all"; 
> You could do this with bitfields and bit logic! ;) Nah... :p

We need to create an object lesson for the perils of overly clever code. :P

> Nit: unnecessary braces and trailing space

fixed.

> >+  _refetchTabs: function(force) {
> >+    // if Clients hasn't synced yet this session, need to sync it as well
> >+    if (Weave.Clients.lastSync == 0)
> >+      Weave.Clients.sync();
> Potentially a new client can be added even after already syncing this session.
> The bug you're avoiding here is to make sure we have the client information
> in-case the tab has some new client guid, yeah?

Yes, without this, if you open the tab before a real sync, you can get weird issues with missing records.

> >+    let engine = Weave.Engines.get("tabs");
> >+    let lastSync = engine.lastSync;
> lastSync isn't used?

Nope... removed.

> >+    // Force a sync only for the tabs engine
> >+    engine.lastModified = null;
> >+    engine.sync();
> I suppose there's already been a bunch of engine.enabled before this code is
> reached, but it might still be a good sanity check to add a this.enabled inside
> of engine.sync at some point.

Yeah, this is part of where I felt like belt and suspenders was probably overkill.

> >+  handleClick: function(event) {
> >+      let url = event.target.getAttribute("url");
> >+      openUILink(url, event);
> >+      let index = this._tabsList.getIndexOfItem(event.target);
> >+      this._tabsList.removeItemAt(index);
> Some of this could be shared with openSelected code for opening and cleaning up
> the list.

Yeah, not sure there's enough to share to justify the function overhead.

> >+++ b/source/chrome/content/fx-tabs.xul
> >+  </popupset>
> >+    <richlistbox context="tabListContext" id="tabsList" seltype="multiple"
> Text alignment is off by 2 spaces.

fixed.

> >+      <hbox align="center" style="background-color: transparent;">
> >+        <hbox id="headers" align="center">
> >+        <image src="chrome://weave/skin/sync-32x32.png" height="32" width="32"/>
> >+        <!-- xxx fix CSS -->
> But I guess good news is that these inner items will be spaced correctly! ;)

Yeah...

> These things exist inside the richlistbox, so minor bug is that you can
> rightclick this area and get a context menu with just a separator and Refresh
> List.

The separator is a bug, the Refresh List bit isn't.

> >+++ b/source/chrome/content/fx-weave-overlay.js
> > function FxWeaveGlue() {
> >+  setTimeout(this.init, 0);
> What are we running into? Unexpected events getting fired?

The event handlers don't get set on the alltabs-menu binding otherwise.

> >+  WEAVE_TABS_URL: "about:weave-tabs",
> I suppose here for now is fine. It might get moved to constants.js.in if we end
> up reusing this on Fennec.

Yeah, I don't know if it will.  I suspect we won't share this UI.

> >+  removeTabsUI: function () {
> >+    // we need to do this manually because the tabbrowser cleanup chokes here
> What's happening here? Any particular reason why we can't just lazily add these
> menu items and just hide/show them instead of remove/re-adding? Does the menu
> get rebuilt each time and apparently goes bad if there's extra items?

Yes.  http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#3396

> >+  handleEvent: function (event) {
> >+    switch (event.type) {
> >+      case "popupshowing":
> Do we get every popupshowing event? We might end up inserting/removing a lot
> even if it's not the tabs menu? Not too major though..

No, we only have two elements we're listening on this for.

> >+  openTabsPage: function () {
> Add openTabPage name to the function

nothing else has the name...

> >+    if (gBrowser) {
> >+      let i = this.getPageIndex();
> >+      if (i != -1) {
> >+        gBrowser.selectTabAtIndex(i);
> >+        return;
> >+      }
> >+      gBrowser.loadOneTab(this.WEAVE_TABS_URL, null, null, null, false);
> >+    }
> >+    else { // not in a browser window
> >+      let win = getTopWin();
> >+      if (win) {
> >+        win.gBrowser.loadOneTab(this.WEAVE_TABS_URL, null, null, null, false);
> This else no-gBrowser block should probably come first and try finding a
> gBrowser and only if it can't, do openDialog. Otherwise, slight bug in the else
> block always unconditionally opening the tab whereas it could find the open
> page.

Fixed this to call win.gFxWeaveGlue.openTabsPage(); instead of win.gBrowser.loadOneTab

> >+++ b/source/chrome/content/fx-weave-overlay.xul
> >-        <menuitem id="sync-no-tabs-menu-item" label="&syncNoTabsMenuItem.label;"
> This string should be unused now.

removed.

> >+++ b/source/chrome/content/notification.css
> >+.weave-tab, .weave-tab:hover, .weave-tab:hover:active {
> Yeah.. I've done it too. :( We need to rename this file or split it or
> something..

file a followup? :P

> >+++ b/source/chrome/locale/en-US/fx-tabs.dtd
> >+<!ENTITY tabs.searchText.label                   "Type here to find tabs...">
> Can probably copy/paste the … from other string files like fx-prefs.dtd. Also,
> do we need ui? for strings like these? The location bar just says "Search for
> ..." instead of reminding that you need to type in the textbox.

Right… fixed. ;)

Yeah, I'm going to get faaborg to look at strings once we land this.

> >+++ b/source/chrome/locale/en-US/sync.properties
> >+tabs.fromOtherComputers.label = Tabs From Other Computers
> We have this string as "Tabs from my other computers" in Fennec:
> http://mxr.mozilla.org/labs-central/source/weave/source/chrome/locale/en-US/fennec-tabs.dtd#1

that's not what we've done in Firefox previously.  Meh.

> >+++ b/source/components/Weave.js
> >+function AboutWeaveService() {}
> Perhaps we should call this AboutWeaveTabs in-case we add in other
> about:weave-foo uris.

Meh, sure.

> >+++ b/source/modules/engines/clients.js
> >+  isMobile: function isMobile(id) {
> >+    return this._store._remoteClients[id].type == "mobile";
> Might want a sanity check for [id] existing. I guess default to not mobile if
> it doesn't exist? I guess what you have now is that the clients engine needs to
> make sure to sync before calling this works.

Yeah, we can make it sanity-check, but it'll seem broken on first use if the new client is a mobile one... not sure what's right, would rather be less clean and more accurate.

Comment 8

[Mike Connor [:mconnor]]

Attachment: v1.1

Comment 9

[Ed Lee :Mardak]

Attachment: diff v1 - v1.1

Comment 10

[Ed Lee :Mardak]

Comment on attachment 436120 [details] [diff] [review]
v1.1

(In reply to comment #7)
> > >+    let haveTabs = false;
> > >+    for (let [guid, client] in Iterator(engine.getAllClients())) {
> > >+      haveTabs = true;
> > haveTabs isn't checked afterwards, so no tabs appears as a mysterious empty
> > list. The ordering of clients is undefined, but I suppose no worse than what we
> > have currently.
> Yeah, indeed.  This is based on that code, so it has some of those limitations.
>  I think it's fine.
You removed haveTabs in the latest patch. But the user still doesn't know if the page is supposed to be blank or still loading or something. We should probably check for !haveTabs -> "no tabs synced" or something like that.

> Right… fixed. ;)
Did this not get saved as part of the patch?

Comment 11

[Mike Connor [:mconnor]]

(In reply to comment #10)
> You removed haveTabs in the latest patch. But the user still doesn't know if
> the page is supposed to be blank or still loading or something. We should
> probably check for !haveTabs -> "no tabs synced" or something like that.

If there's no tabs, the client will not be shown.  We insert the client item with the first tab actually inserted.  Filter also hides a client without any remaining tabs.  haveTabs was cruft from an old approach.

> > Right… fixed. ;)
> Did this not get saved as part of the patch?

Yeah, sorry, noticed that last night, didn't bother to attach the corrected patch.

Comment 12

[Mike Connor [:mconnor]]

http://hg.mozilla.org/labs/weave/rev/57d351858374

Comment 13

[Tracy Walker [:tracy]]

verified with 1.3 betas