This would replace the functionality of the Tabs from Other Devices in-content page (Bug 988317). UX: https://mozilla.invisionapp.com/share/NA47717JG#/screens/101713710

The code is based on aboutSyncTabs.js but for xul trees. TODO: Tab filtering, tree container icon doesn't toggle when closed

The tree style should look correct now. TODO: filter tabs, hide context menu separator when only "Refresh list" item is visible

Adds tab filtering.

Menus now trigger the sidebar rather than about:sync-tabs. TODO: tests. There's an initial lag while Sync loads... not sure how to get around that.

How it currently looks

Looking good! "Tabs from other devices" should become "Synced tabs". And filter field does not require placeholder (i.e. "Type here to find tabs").

I see that Android has a "Synced tabs" string here: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/en-US/android_strings.dtd#305. Mark, is there a way to transfer that string to desktop without additional l10n work, or should we go ahead and not worry about it?

Comment on attachment 8674995 [details] [diff] [review] create a Sync tabs sidebar Review of attachment 8674995 [details] [diff] [review]: ----------------------------------------------------------------- Sadly, I think I'd get the sack if I r+'d a patch introducing a new .xul file without an excellent reason ;) It would be great if we can do this with plain HTML - see https://hg.mozilla.org/mozilla-central/file/5721360e23cd/browser/components/readinglist for the (old, now deleted) readinglist sidebar implementation that uses HTML and might offer some ideas. But otherwise this is looking quite good - sadly I've a lot of comments that might be a little painful... ::: browser/base/content/browser-menubar.inc @@ +371,5 @@ > #ifdef MOZ_SERVICES_SYNC > <menuitem id="sync-tabs-menuitem" > class="syncTabsMenuItem" > label="&syncTabsMenu2.label;" > + command="viewTabsSidebar" I'm a little confused about how this change works with bug 1210586 (ie, that bug seems to imply this menu item should do something different than open the sidebar - but I'm not sure exactly what that is yet). I've needinfo'd rfeeley in that bug to help clarify. ::: browser/base/content/browser-sets.inc @@ +150,5 @@ > > + <broadcaster id="viewTabsSidebar" autoCheck="false" sidebartitle="&syncTabsMenu2.label;" > + type="checkbox" group="sidebar" > + sidebarurl="chrome://browser/content/tabs/tabsPanel.xul" > + oncommand="SidebarUI.toggle('viewTabsSidebar');"/> I assume we will want this hidden when Sync's not configured (or if not, I expect we'd want a promo to signup in the content rather than showing an empty list). I predict that we might also want custom content for single-device users (again, probably a promo to tell them to setup a second device). I don't care how much of that is tackled in this bug, but some of it should be (ie, the "hide when sync not configured" is probably a minimum) - can you please chat with rfeeley about this? (Assuming that is what we do, it should be a fairly trivial change to browser-syncui to make that happen - it already disables/hides sync-related menu items) ::: browser/components/places/content/tabsPanel.js @@ +13,5 @@ > + > +XPCOMUtils.defineLazyModuleGetter(this, "Promise", > + "resource://gre/modules/Promise.jsm"); > + > +//#ifdef MOZ_SERVICES_CLOUDSYNC So yeah, CloudSync - to follow up on our conversation in IRC, I think we can get away without supporting cloud-sync here, but only if about:sync-tabs doesn't go away. I'm not sure what the thinking is about about:sync-tabs and we can probably ignore it for now, but it's worth keeping in mind when discussion about about:sync-tabs comes up. @@ +20,5 @@ > +//#else > +//let CloudSync = null; > +//#endif > + > +var RemoteTabViewer = { we should use let/const where we can here (which is probably everywhere ;) @@ +41,5 @@ > + this.buildList(true); > + }, > + > + uninit: function () { > + Services.obs.removeObserver(this, "weave:service:login:finish"); Both these observers can probably be replace with an observer of topic=="weave:engine:sync:finish" and data=="tabs" - that tells us when new tabs are available, and I don't think this code needs to care about a full sync or login completing (and I think attempting to force a tabs sync at login:finish time isn't quite the right thing to do) @@ +75,5 @@ > + let filteredTabs = this._filterTabs(query); > + this._tabsTreeView = this._tabsTree.view = createTreeView(filteredTabs, this._tabsByClient); > + }, > + > + onTabsListClick: function (event) { can we do this in a dblclick event handler and avoid the additional checks below? @@ +115,5 @@ > + } > + > + let uri = Weave.Utils.makeURI(item.url); > + let title = item.title; > + PlacesUIUtils.showBookmarkDialog({ action: "add" I expect we will want to emulate the existing "bookmark" behaviour, where the single click just silently bookmarks and adds the blue "star" and subsequent clicks offers a popup which allows you to remove or edit the bookmark. IOW, we want this behaviour to be as close as possible to that when an open tab is bookmarked. @@ +177,5 @@ > + _clearTabListState: function () { > + this._tabsByClient = {}; > + }, > + > + buildList: function (force) { I know this came from aboutSyncTabs, but IMO we should change it to a promise API that returns the "previous" promise if one is already in-flight. I think we probably want to kill that |force| param and add a new method to force a sync (which doesn't need to interact with this code - the observers will just fire and do the right thing) @@ +193,5 @@ > + > + if (Weave.Service.isLoggedIn && this._refetchTabs(force)) { > + this._generateWeaveTabList(this._tabs); > + } else { > + //XXXzpao We should say something about not being logged in & not having data this gets back to earlier comments about what to do when the user isn't signed in - we should probably just remove the comment for now. @@ +274,5 @@ > + } > + }; > + > + return CloudSync().tabs.getRemoteTabs() > + .then(updateTabList, Promise.reject); I don't think we need this Promise.reject call - IIUC, the semantics if this should be the same as without it (but please correct me if I'm wrong) @@ +278,5 @@ > + .then(updateTabList, Promise.reject); > + }, > + > + adjustContextMenu: function (event) { > + let mode = "all"; handling multi-select seems like overkill to me, so I'd be happy to kill this and all other related code (eg, I expect the bookmarking experience would be simpler without it) - but I guess I also don't care if people feel strongly it should remain. ::: browser/components/places/content/tabsPanel.xul @@ +1,1 @@ > +<?xml version="1.0"?> <!-- -*- Mode: xml; indent-tabs-mode: nil; -*- --> I don't think browser/components/places is the right place for this - browser/components/synctabs/ makes more sense to me (just "tabs" might confused people IMO) - and similarly the filenames and broadcasters should be use "synctabs" instead of the unclear "tabs". (There are no plans to turn this into a general tabs sidebar where local tabs are also shown, right?) @@ +16,5 @@ > +<page id="tabs-display" > + xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" > + title="&tabs.otherDevices.label;" > + onload="RemoteTabViewer.init()" > + onunload="RemoteTabViewer.uninit()" can you please verify that hiding the sidebar or switching to another does an unload? I want to be sure we aren't listening for the observer notifications etc when it's not visible. ::: browser/components/places/jar.mn @@ +26,5 @@ > # ditto for the bookmarks sidebar > content/browser/bookmarks/bookmarksPanel.xul (content/bookmarksPanel.xul) > content/browser/bookmarks/bookmarksPanel.js (content/bookmarksPanel.js) > content/browser/bookmarks/sidebarUtils.js (content/sidebarUtils.js) > + content/browser/tabs/tabsPanel.xul (content/tabsPanel.xul) I'm a bit confused here - (ie, there's no "tabs/" part in the source of the file itself) - but jar manifests always confused me :) Regardless, I don't think "places" is the right place for this as it isn't really places related.

(In reply to Zachary Carter [:zaach] from comment #8) > I see that Android has a "Synced tabs" string here: > https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/ > en-US/android_strings.dtd#305. Mark, is there a way to transfer that string > to desktop without additional l10n work, or should we go ahead and not worry > about it? I don't think that is possible as the platforms use different l10n strategies, and adding the string here for localizers to do as normal is fine given we aren't looking towards uplifts IIUC.

(In reply to Mark Hammond [:markh] from comment #9) > Comment on attachment 8674995 [details] [diff] [review] > create a Sync tabs sidebar > > Review of attachment 8674995 [details] [diff] [review]: > ----------------------------------------------------------------- > > Sadly, I think I'd get the sack if I r+'d a patch introducing a new .xul > file without an excellent reason ;) It would be great if we can do this with > plain HTML - see > https://hg.mozilla.org/mozilla-central/file/5721360e23cd/browser/components/ > readinglist for the (old, now deleted) readinglist sidebar implementation > that uses HTML and might offer some ideas. > > But otherwise this is looking quite good - sadly I've a lot of comments that > might be a little painful... Darn, I totally wasn't thinking about XUL-death when I started on this, alas. Re-creating a generic tree component in HTML would probably extend the work on this patch quite a bit, but I could whip up something for this particular sidebar's needs in less time. > > ::: browser/base/content/browser-menubar.inc > @@ +371,5 @@ > > #ifdef MOZ_SERVICES_SYNC > > <menuitem id="sync-tabs-menuitem" > > class="syncTabsMenuItem" > > label="&syncTabsMenu2.label;" > > + command="viewTabsSidebar" > > I'm a little confused about how this change works with bug 1210586 (ie, that > bug seems to imply this menu item should do something different than open > the sidebar - but I'm not sure exactly what that is yet). I've needinfo'd > rfeeley in that bug to help clarify. Bug 1210586 would replace this with a menu, is what I gather. > > @@ +177,5 @@ > > + _clearTabListState: function () { > > + this._tabsByClient = {}; > > + }, > > + > > + buildList: function (force) { > > I know this came from aboutSyncTabs, but IMO we should change it to a > promise API that returns the "previous" promise if one is already in-flight. > I think we probably want to kill that |force| param and add a new method to > force a sync (which doesn't need to interact with this code - the observers > will just fire and do the right thing) I think once we remove CloudSync it won't be asynchronous anymore. Thanks for the feedback!

(In reply to Zachary Carter [:zaach] from comment #11) > Darn, I totally wasn't thinking about XUL-death when I started on this, > alas. Re-creating a generic tree component in HTML would probably extend the > work on this patch quite a bit, but I could whip up something for this > particular sidebar's needs in less time. Yeah, we totally don't need a tree IMO - the XUL tree is particularly good at huge data sets, and I don't think we will get that huge, and imposing some relatively large limit sounds fine (ie, we don't need to cater to 500 remote tabs!) > Bug 1210586 would replace this with a menu, is what I gather. Yeah - so I think this patch probably shouldn't bother changing it for now (unless there's a reason I missed) > I think once we remove CloudSync it won't be asynchronous anymore. I've got a patch that should land in the next month or so to make Sync promise-based, including the calls into the tab engine you make here, so it would be great if you can make it promise based anyway :)

I feel we need a reasonable plan around how we deal with CloudSync. After this land, how will CloudSync users view their synced tabs?

(In reply to Chris Karlof [:ckarlof] from comment #13) > I feel we need a reasonable plan around how we deal with CloudSync. We do indeed. > After this land, how will CloudSync users view their synced tabs? That sounds like more of a problem when we remove about:sync-tabs rather than when we add the sidebar. So this might be a valid reason to leave the history menu alone, and keep that as the entry-point to about:sync-tabs? Or maybe only enable that menu item when couldsync is configured? We could also consider including CloundSync tabs in the sidebar etc, but TBH I've got no idea how to configure cloudsync and thus no way of testing it.

I'm going to base the HTML theme on the old reading list code that Mark linked to. Ryan, do you know of any larger initiative to coordinate the UX/UI of new HTML interfaces?

See above^^

The same CSS can be used for HTML and XUL documents so IMO we should be using the same styles so that users can't tell the difference between XUL and HTML implementations. Unifying on element names, class names, etc. for the existing styles to match on is something that should be coordinated. One idea would be to have HTML web components mapping to XUL element names that share styles and we can gradually remove the XUL implementation once we're done porting to the web component.

(In reply to Matthew N. [:MattN] from comment #17) > The same CSS can be used for HTML and XUL documents so IMO we should be > using the same styles so that users can't tell the difference between XUL > and HTML implementations. Unifying on element names, class names, etc. for > the existing styles to match on is something that should be coordinated. One > idea would be to have HTML web components mapping to XUL element names that > share styles and we can gradually remove the XUL implementation once we're > done porting to the web component. I'm worried about how tight the coupling would be with the XUL implementation. We might need to mimic the behaviors of the XUL elements for the styles to apply correctly and that's a deep rabbit hole. Ryan Feeley dug up a build with Reading list and the style looks pretty consistent (https://irccloud.mozilla.com/file/Rt10Xo1L/Screenshot+2015-10-27+11.07.57.png) so this might be a non-issue for now.

I think getting the hierarchical tree working well is the harder part. Don't forget you have to be able to expand and collapse the different devices containing tabs.

This is a really rough draft that lacks a number of features. It will display tabs/clients using HTML instead of XUL, but styles are weird. There's a SyncedTabs.jsm module in services to abstract getting a list of synced tabs. Rather than a list it returns an object where clients are indexed by their id. Each client has a tabs property with an array of tabs.

The standard list and filter works. Needs: client collapsing/opening, tree-style selections, link opening, and style.

Fixed: branch opening/closing, opening tab URLs, selection Next: keyboard navigation, styling

Keyboard navigation is functional, but sprinkling in XUL for the context menu is not.

Comment on attachment 8691725 [details] [diff] [review] WIP create a Sync tabs sidebar Review of attachment 8691725 [details] [diff] [review]: ----------------------------------------------------------------- This is looking good at a first glance. I'll apply the patch and have a play next! ::: browser/components/syncedtabs/moz.build @@ +3,5 @@ > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +JAR_MANIFESTS += ['jar.mn'] > + > +#EXTRA_JS_MODULES.readinglist += [ We should kill all the commented lines here. ::: browser/components/syncedtabs/sidebar.js @@ +14,5 @@ > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > +Cu.import("resource://services-common/utils.js"); > +Cu.import("resource://services-sync/main.js"); > +Cu.import("resource://services-sync/SyncedTabs.jsm"); > + In the short term you could use the log "Sync.RemoteTabs", which SynedTabs.jsm uses - that module already has a hacky way of arranging for the output to go to stderr, which might be useful here (ie, I suspect you currently can't see any logging output from this module, which is why there are also console.log calls) @@ +24,5 @@ > + log.debug("Initializing"); > + > + addEventListener("unload", () => this.uninit()); > + > + Services.obs.addObserver(this, "weave:engine:sync:finish", false); I don't think you'll need this observer any more or this.observe - you are listening for the SyncedTabs notification in a different object. @@ +50,5 @@ > + uninit: function () { > + Services.obs.removeObserver(this, "weave:engine:sync:finish"); > + }, > + > + openUrl: function (url) { History works this way, but I'm wondering if we might end up needing to use window.openUILink() from a "click" handler, as that will respect mouse-button choices (ie, middle-click will open in a new tab). This is more an FYI and may not make sense here. @@ +96,5 @@ > + return null; > + }, > + > + onClick: function (event) { > + const BUTTON_LEFT_CLICK = 0; we should hoist this constant up to save duplicating it multiple times. @@ +192,5 @@ > + }, > + > + bookmarkSingleTab: function () { > + let item = this.selectedItem; > + if (! item.url) { nit: there shouldn't be a space between "!" and the expression. @@ +196,5 @@ > + if (! item.url) { > + return; > + } > + > + let uri = Weave.Utils.makeURI(item.url); Use |Services.io.newURI(item.url, null, null);| (which is the canonical way to create a URI object outside of weave) here and you can avoid importing Weave completely. @@ +200,5 @@ > + let uri = Weave.Utils.makeURI(item.url); > + let title = item.title; > + > + window.top.PlacesCommandHook.bookmarkLink(PlacesUtils.bookmarksMenuFolderId, uri, title) > + .catch(Components.utils.reportError); Use |Cu| instead of Components.utils @@ +299,5 @@ > + _change(isUpdateble) { > + let selectedParent = this._selectedRow[0]; > + let selectedChild = this._selectedRow[1]; > + let rowSelected = false; > + let data = JSON.parse(JSON.stringify(this.data)); let data = Cu.cloneInto(this.data, {}); is a less obvious but better way to clone an object. @@ +301,5 @@ > + let selectedChild = this._selectedRow[1]; > + let rowSelected = false; > + let data = JSON.parse(JSON.stringify(this.data)); > + > + data.forEach((client, index) => { I think we prefer a for...of loop these days @@ +328,5 @@ > + > + /** > + * Moves the row selection from a child to its parent. > + */ > + _selectParentRow() { it seems strange to me that the "store" object also handles selection in this way - but maybe I'm not understanding it properly yet :) @@ +370,5 @@ > + }); > + } > +}); > + > +function TabListView( The distinction between a "RemoteTabViewer" and a "TabListView" isn't clear to me, so some comments about what the responsibility of the objects are would probably help. ::: browser/components/syncedtabs/sidebar.xhtml @@ +48,5 @@ > + </div> > + </div> > + </div> > + > + <div id="emptyListInfo" hidden="true">&syncedTabs.sidebar.emptyText;</div> I wonder if we are going to need some UI while we are doing a first fetch? The most recent patch in the SyncedTabs.jsm but adds haveSyncedThisSession property for this purpose (ie, whhile this returns true we haven't yet synced tabs, so 0 clients/tabs is probably just a reflection of that rather than really meaning no clients/tabs) ::: browser/locales/en-US/chrome/browser/browser.dtd @@ +782,5 @@ > > <!-- LOCALIZATION NOTE (syncTabsMenu2.label): This appears in the history menu --> > <!ENTITY syncTabsMenu2.label "Tabs From Other Devices"> > > +<!ENTITY syncedTabs.label "Synced tabs"> We should probably name this syncedTabs.sidebar.label or similar - eg, my menu patch adds the same string: <!ENTITY syncTabsMenu3.label "Synced Tabs"> But we are discouraged from reusing strings in different contexts, so I think we are going to need the duplication, so more context in the name will help us and localizers.

Zach, the "SyncedTabs" module was just pushed to fx-team, so should be available in the tree soon. Note a couple of changes that will impact you: * The observer name has changed, but instead of hard-coding a string you can now use SyncedTabs.TOPIC_TABS_CHANGED. * haveSyncedThisSession has been renamed to hasSyncedThisSession, but I don't think your patch uses that.

Added Mark's feedback, although ellipsis are still not displaying. Still working on getting the context menu to trigger.

This is the current look using HTML. Ryan: as you can tell the highlighted row background and open/close tree branch icon don't match up with XUL trees. So, a couple questions: should we try to match it pixel perfect with XUL trees, or is there a more modern style we should aim for? If pixel perfect, where or from whom should we obtain the assets for the classic XUL tree style for each platform?

Much better to match style of other sidebars than introduce a new aesthetic to the user. This was also a recommendation made during our new user onboarding audit. So that means your screenshot would look more like this.

Now with working context menu.

Comment on attachment 8695065 [details] [diff] [review] WIP create a Sync tabs sidebar Review of attachment 8695065 [details] [diff] [review]: ----------------------------------------------------------------- This is looking good! I think we should get rfeeley onto this ASAP - I'll needinfo him, but maybe we can grab him at Orlando. ::: browser/base/content/browser-menubar.inc @@ +241,5 @@ > <menuitem id="menu_historySidebar" > key="key_gotoHistory" > observes="viewHistorySidebar" > label="&historyButton.label;"/> > +#ifdef MOZ_SERVICES_SYNC you can kill the MOZ_SERVICES_SYNC checks ::: browser/base/content/browser.xul @@ +474,5 @@ > acceskey="&emeNotificationsDontAskAgain.accesskey;" > oncommand="gEMEHandler.onDontAskAgain(this);"/> > </menupopup> > + > + <menupopup id="SyncedTabsSidebarContext"> the indentation should be the same as the element above @@ +482,5 @@ > + showFor="single"/> > + <menuitem label="&tabs.context.bookmarkSingleTab.label;" > + accesskey="&tabs.context.bookmarkSingleTab.accesskey;" > + id="syncedTabsBookmarkSelected" > + showFor="single"/> "showfor" would be better I think - I can't recall seeing camelCase attribute names. ::: browser/components/syncedtabs/moz.build @@ +12,5 @@ > + > +#XPCSHELL_TESTS_MANIFESTS += ['test/xpcshell/xpcshell.ini'] > + > +with Files('**'): > + BUG_COMPONENT = ('Firefox', 'Synced tabs') It doesn't look like that component exists - maybe just "Sync" will do? ::: browser/components/syncedtabs/sidebar.js @@ +37,5 @@ > + return getChromeWindow().document.getElementById("SyncedTabsSidebarContext"); > +} > + > +var RemoteTabComponent = { > + init: function () { I meant to mention this before - we should be more modern and drop "function" everywhere here - eg |init() {| @@ +57,5 @@ > + this._clientTemplate, > + this._tabTemplate > + ); > + > + Cc["@mozilla.org/eventlistenerservice;1"] ack - even though I suggested this code, I just determined that "Services.els" exists - ie, Services.els.addSystemEventListener(...) - and ditto below where you remove it. @@ +226,5 @@ > + let id = event.target.getAttribute("id"); > + switch (id) { > + case "syncedTabsOpenSelected": > + this.openSelected(); > + break; break should be indented (and below) @@ +245,5 @@ > + } > + > + let menu = getContextMenu(); > + this.adjustContextMenu(menu); > + menu.openPopupAtScreen(event.screenX, event.screenY, true); even though I suggested this code, openPopupAtScreen takes an optional 4th param, the event triggering it. TBH I'm not 100% what this does entirely, but at the least it allows menu.triggerNode to be what was clicked on - So just pass event as the 4th param. ::: browser/components/syncedtabs/test/xpcshell/test_EventEmitter.js @@ +1,2 @@ > +/* Any copyright is dedicated to the Public Domain. > + 2 * http://creativecommons.org/publicdomain/zero/1.0/ */ I think that "2" might be a typo :) ::: browser/locales/en-US/chrome/browser/browser.dtd @@ +774,5 @@ > <!-- LOCALIZATION NOTE (syncTabsMenu2.label): This appears in the history menu --> > <!ENTITY syncTabsMenu2.label "Tabs From Other Devices"> > > +<!ENTITY syncedTabs.label "Synced tabs"> > +<!ENTITY syncedTabs.sidebar.label "Synced tabs"> line this string up with the one below ::: browser/themes/shared/syncedtabs/sidebar.inc.css @@ +70,5 @@ > +} > + > +.item-icon-container, > +.item-twisty-container { > + min-width: 16px; can these just be width and height? @@ +83,5 @@ > + > +.item-title-container { > + display: flex; > + flex-flow: row; > + -moz-padding-start: 4px; do we need this? It seems surprising we'd need -moz-padding-start and plain-ol padding. @@ +99,5 @@ > + > +.item[hidden] { > + opacity: 0; > + max-height: 0; > + transition: opacity 150ms ease-in-out, max-height 150ms ease-in-out 150ms; do these transitions work for you? When I tried the earlier version of the patch it seemed to do nothing. @@ +109,5 @@ > +} > + > +#search-box.compact { > + padding: 0px; > + font-size: 11px; I think we prefer something like "1.1em" here (and below in #search-box)

Oh - I also think we'll need to work out how to make the "search" box look more like the others - ie, with the search or clear icons at the right.

Ryan, I think we need to make some decisions here around the same "states" we handled in the SyncedTabs menu - ie: * What do we show when the user isn't signed in, or when they need to reauth, or when "tabs" are disabled, or when there are zero client records. IIUC, it would just be completely blank in that case. * What do we do for client records with either zero tabs in total, or zero tabs that match the current filter? Maybe we can show you this patch at Orlando and chat about it there?

(In reply to Mark Hammond [:markh] from comment #32) > Ryan, I think we need to make some decisions here around the same "states" > we handled in the SyncedTabs menu Agreed! There are currently a few ways to open the sidebar: 1. The View > Sidebar menu 2. The "View Synced Tabs Sidebar" toggle in the menu item 3. The option in the Sidebar toolbar button 4. Keyboard shortcut (to be determined) I am very cognizant of user’s tolerance of UI for features they aren't using, so I'd prefer that we hide the UI to view the sidebar in most cases. > * What do we show when the user isn't signed in Options to view the sidebar should be hidden in all examples above. > or when they need to reauth We should tell them about it in the sidebar itself. I'll make the screen today. > or when "tabs" are disabled Options to view the sidebar should be hidden in all examples above. > or when there are zero client records. > * What do we do for client records with either zero tabs in total We should tell them about it in the sidebar itself. I'll make the screen today. > IIUC, it would just be completely blank in that case. Pretty much. Just a short message. > or zero tabs that match the current filter? Same as history and bookmarks sidebar. Would just be blank. > Maybe we can show you this patch at Orlando and chat about it there? I'm dying to see it! Will update the invision with the extra states today.

(In reply to Ryan Feeley [:rfeeley] from comment #33) > (In reply to Mark Hammond [:markh] from comment #32) > > > * What do we show when the user isn't signed in > > Options to view the sidebar should be hidden in all examples above. > What if they have the sidebar open and then they sign out (or are disconnected)?

> What if they have the sidebar open and then they sign out (or are disconnected)? I suppose we should show the "please sign in state" like the sidebar has. I'll work on this one too.

OS X style arrows for the tree

This impleements the alternative sidebar states: - not authed or need to reauth - tab syncing is disabled - no other clients connected - initial tab fetch is in progress - and the normal tab list Also, clients with no tabs will show up in the list but with copy indicating that it has no tabs.

Comment on attachment 8699340 [details] [diff] [review] WIP create a Sync tabs sidebar Review of attachment 8699340 [details] [diff] [review]: ----------------------------------------------------------------- Some comments from running the code: * I don't get twisties on Windows, which is probably just due to missing CSS - but we might as well get them in soonish if you think it's somewhat stable. * The search box looks like the border is too "strong" - maybe just a grey border? It's also missing the magnifying glass and 'x' icons. * The "sign in to sync" state is showing the "Sign in to Sync to ..." message in a very dim gray text, which I doubt rfeeley will like. * I think the "no open tabs" message will want to be indented to line up with tabs * Every time a Sync completes you lose the selection, which is annoying when you interact with it, and quite likely seeing a sync is explicitly requested. In general, we probably want ux-review from rfeeley asap. To the code... :) ... I didn't get to the tests, but I'm out of time today and I think there's enough here :) A general comment is that the complexity seems to be higher than would be ideal (but yeah, some complexity is obviously inevitable) ::: browser/base/content/browser-menubar.inc @@ +241,5 @@ > <menuitem id="menu_historySidebar" > key="key_gotoHistory" > observes="viewHistorySidebar" > label="&historyButton.label;"/> > +#ifdef MOZ_SERVICES_SYNC we can remove the #ifdef ::: browser/base/content/browser.xul @@ +478,5 @@ > + <menupopup id="SyncedTabsSidebarContext"> > + <menuitem label="&tabs.context.openTab.label;" > + accesskey="&tabs.context.openTab.accesskey;" > + id="syncedTabsOpenSelected" > + showFor="single"/> IIUC we don't intend to support multiple selections so it looks like we can kill showFor? (I guess one issue might be when there is no item selected - but it seems impossible to unselect an item once one is selected, so I wonder if we should just auto-select the first device? ::: browser/components/syncedtabs/EventEmitter.js @@ +1,1 @@ > +"use strict"; this needs a license header - but in general I'm not convinced it needs its own file, but if it does stay that way, I think the name of the file and object should change - IIUC it's not dealing with "events". (I'm not bothered by that name if it is inline in the file that uses it as the context is clearer there) But is this file actually used? (in general, this seems a little like an over-abstraction given there seems to be exactly 2 .emit() calls and exactly 2 .on() calls, both using "change" as the event, and chained together (ie, there's exactly 1 change event). I guess I'm not *too( bothered by this, especially if it ends up being more than just synced tabs in the future - but I am having trouble keeping this code in my head as I read it). "Less is more" is one of my favorite mottos :) @@ +4,5 @@ > +}; > + > +EventEmitter.prototype = { > + on: function (event, fct) { > + if (!this._events) { can we just initialize this in the constructor and not bother checking if this._events? It also looks like this might be better as a map? ::: browser/components/syncedtabs/remoteTabComponent.js @@ +6,5 @@ > + > +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components; > + > +Cu.import("resource:///modules/PlacesUIUtils.jsm"); > +Cu.import("resource://gre/modules/PlacesUtils.jsm", window); I doubt we need "window" here. @@ +36,5 @@ > + return getChromeWindow().document.getElementById("SyncedTabsSidebarContext"); > +} > + > + > +const EventEmitter = function () { it looks like EventEmitter.js isn't actually used? Which seems fine to me :) Comments there apply here too, apart from the name. @@ +85,5 @@ > +}; > + > +SyncedTabsSidebarComponent.prototype = { > + init() { > + addEventListener("unload", () => this.uninit()); Maybe this could be in sidebar.js? I kinda like the symmetry of .uninit() being called by whoever calls .init() - eg, this basically makes .uninit() private, so maybe a leading _ on that? Up to you :) @@ +131,5 @@ > + if (this._SyncedTabs.isConfiguredToSyncTabs) { > + if (this._SyncedTabs.hasSyncedThisSession) { > + return this._SyncedTabs.getTabClients().then(clients => { > + if (clients.length) { > + return "tabs-container"; https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style says "Don't put an else right after a return. Delete the else, it's unnecessary and increases indentation level." which I think will improve the readability of this function considerably. Task.jsm is always a good option too ;) @@ +153,5 @@ > + return this.getPanelStatus().then(panelId => this._deckStore.selectPanel(panelId)); > + }, > + > + openAndroidLink(event) { > + let href = Services.prefs.getCharPref("identity.mobilepromo.android") + "synced-tabs"; we should maybe use a different suffix here so we can tell whether people are following the link from the panel or the sidebar (and below) @@ +171,5 @@ > + getChromeWindow().gSyncUI.openSetup(); > + } > +}; > + > +const DeckStore = function (panels, selected) { DeckStore and DeckView are adding cognitive overhead (ie, it's not obvious from a quick scan what they are actually doing - so some comments would probably help. @@ +199,5 @@ > + deck.getElementsByClassName("sync-prefs")[0].addEventListener("click", props.onSyncPrefClick); > +}; > + > +DeckView.prototype = { > + render(state) { do we need .render and .update? @@ +223,5 @@ > +TabListComponent.prototype = { > + init() { > + log.debug("Initializing TabListComponent"); > + > + addEventListener("unload", () => this.uninit()); same comment here about public vs private I guess. @@ +256,5 @@ > + this._syncedTabsStore.getData(query); > + }, > + > + /** > + * Find the parent item element, from a given child element. strange to see one of the only documented methods being a private one ;) @@ +406,5 @@ > + let id = event.target.getAttribute("id"); > + switch (id) { > + case "syncedTabsOpenSelected": > + this.openSelected(); > + break; break should be indented. @@ +429,5 @@ > + }, > + > + adjustContextMenu(menu) { > + let mode = "all"; > + let item = this._list.querySelector('.selected'); see other comment about showFor - ISTM that if we just auto-select an item this entire method can die @@ +451,5 @@ > +}; > + > + > +/** > + * SyncedTabStore SyncedTab*s*Store @@ +453,5 @@ > + > +/** > + * SyncedTabStore > + * > + * This module encapsulates all the state of a synced tabs list view. s/module/object/ I guess? @@ +455,5 @@ > + * SyncedTabStore > + * > + * This module encapsulates all the state of a synced tabs list view. > + */ > +const SyncedTabsStore = function (SyncedTabs) { This object seems quite complex - it appears to be more about the selection, ordering and collapsed/expanded state than actual "storage". @@ +469,5 @@ > + let selectedChild = this._selectedRow[1]; > + let rowSelected = false; > + let data = Cu.cloneInto(this.data, {}); > + > + data.forEach((client, index) => { for..of? At least you can |break| ;) ::: browser/components/syncedtabs/sidebar.xhtml @@ +18,5 @@ > + <script src="chrome://browser/content/syncedtabs/sidebar.js" type="application/javascript;version=1.8"></script> > + <script type="application/javascript" src="chrome://browser/content/utilityOverlay.js"/> > + > + <link rel="stylesheet" type="text/css" media="all" href="chrome://browser/skin/syncedtabs/sidebar.css"/> > + <link rel="stylesheet" type="text/css" media="all" href="chrome://browser/content/places/places.css"/> do we need the 2 places.css sheets here? @@ +21,5 @@ > + <link rel="stylesheet" type="text/css" media="all" href="chrome://browser/skin/syncedtabs/sidebar.css"/> > + <link rel="stylesheet" type="text/css" media="all" href="chrome://browser/content/places/places.css"/> > + <link rel="stylesheet" type="text/css" media="all" href="chrome://global/skin/"/> > + <link rel="stylesheet" type="text/css" media="all" href="chrome://browser/skin/places/places.css"/> > + <link rel="stylesheet" type="text/css" media="all" href="chrome://global/content/textbox.css"/> I'd guess were not getting much useful from these textbox pages either - but I guess the .textbox-search-* classes might help with the search filter icons @@ +79,5 @@ > + <div class="tabs-fetching sync-state"> > + <p>&syncedTabs.sidebar.fetching.label;</p> > + </div> > + <div class="notAuthedInfo sync-state"> > + <p>&syncedTabs.sidebar.notsignedin.label;</p> I've asked on #l10n if we are "allowed" to reuse these strings, and sadly, we can't :( > <flod> markh: I would use 2 separate strings, only for one reason. The hamburger menu is a festival of truncations, given how small it is > <flod> I'd like a change to use a shorter string there, and a longer one in the sidebar (which is resizable) > <flod> otherwise, if the UI looks the same and the string is used in the same content, it would be safe-ish to do (I believe "otherwise" there means "if it wasn't for sharing with the hamburger menu") So we need to add new identical strings but with the new id prefixed somehow to reflect "sidebar" :(

I'll attach a gif showing the current interaction. The behavior ostensibly matches up with XUL trees and XUL search boxes. Mark, you'll have to let me know how Windows looks as I haven't set up a VM/Windows build yet. Still TODO: tests!

Check out that interaction. mm

Comment on attachment 8701306 [details] [diff] [review] WIP create a Sync tabs sidebar Review of attachment 8701306 [details] [diff] [review]: ----------------------------------------------------------------- It looks like alot of the comments from the last round weren't addressed, so I'm clearing the feedback request for now. ::: browser/base/content/browser-doctype.inc @@ +21,5 @@ > %aboutHomeDTD; > <!ENTITY % syncBrandDTD SYSTEM "chrome://browser/locale/syncBrand.dtd"> > %syncBrandDTD; > +<!ENTITY % aboutSyncTabsDTD SYSTEM "chrome://browser/locale/aboutSyncTabs.dtd"> > +%aboutSyncTabsDTD; I think we should consider renaming aboutSyncTabs.dtd to just syncTabs.dtd or similar as one day aboutSyncTabs will die (I'm guessing it is OK to reuse the strings here as they areboth used in menus with the same basic context) ::: browser/base/content/browser-menubar.inc @@ +241,5 @@ > <menuitem id="menu_historySidebar" > key="key_gotoHistory" > observes="viewHistorySidebar" > label="&historyButton.label;"/> > +#ifdef MOZ_SERVICES_SYNC I think this is the 3rd time I've said to remove the #ifdef here :) ::: browser/base/content/browser.xul @@ +482,5 @@ > + showFor="single"/> > + <menuitem label="&tabs.context.bookmarkSingleTab.label;" > + accesskey="&tabs.context.bookmarkSingleTab.accesskey;" > + id="syncedTabsBookmarkSelected" > + showFor="single"/> My last feedback suggested killing showFor and simplifying all the special logic by simply ensuring one item is always selected. ::: browser/components/syncedtabs/EventEmitter.js @@ +2,5 @@ > + > +const EventEmitter = function () { > +}; > + > +EventEmitter.prototype = { I made a number of comments here that don't seem to have been addressed. ::: browser/components/syncedtabs/remoteTabComponent.js @@ +37,5 @@ > +} > + > + > +// Simple event emitter abstraction for storage objects to use. > +const EventEmitter = function () { this is still duplicated ::: browser/locales/en-US/chrome/browser/browser.dtd @@ +790,5 @@ > > <!-- LOCALIZATION NOTE (syncTabsMenu3.label): This appears in the history menu and history panel --> > <!ENTITY syncTabsMenu3.label "Synced Tabs"> > > +<!ENTITY syncedTabs.sidebar.label "Synced Tabs"> I think we should move this into the syncTabs.dtd (alternatively, copy the strings we use from aboutSyncTabs.dtd here and don't rename and share that other .dtd) @@ +806,5 @@ > +<!-- LOCALIZATION NOTE (syncedTabs.sidebar.mobilePromo2.*): the following strings will be used to > + create a single sentence with active links. > + The resulting sentence in English is: "Get Firefox for Android or Firefox for iOS." --> > + > +<!ENTITY syncedTabs.sidebar.mobilePromo2.start "Get"> Let's do this funky dance the same way the synced tabs panel does rather than Sync prefs (ie, https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizableWidgets.jsm#315)

(In reply to Mark Hammond [:markh] from comment #41) > Comment on attachment 8701306 [details] [diff] [review] > WIP create a Sync tabs sidebar > > Review of attachment 8701306 [details] [diff] [review]: > ----------------------------------------------------------------- > > It looks like alot of the comments from the last round weren't addressed, so > I'm clearing the feedback request for now. > Heh, yes, those bits still need to be cleaned up– sorry about that. I refactored a lot of the component code and added comments. The next patch will have things refactored a bit more to facilitate testing.

(In reply to Mark Hammond [:markh] from comment #30) > Comment on attachment 8695065 [details] [diff] [review] > WIP create a Sync tabs sidebar > > @@ +109,5 @@ > > +} > > + > > +#search-box.compact { > > + padding: 0px; > > + font-size: 11px; > > I think we prefer something like "1.1em" here (and below in #search-box) This was copied from the XUL search box style, which uses px. Should we deviate?

(In reply to Mark Hammond [:markh] from comment #41) > Comment on attachment 8701306 [details] [diff] [review] > WIP create a Sync tabs sidebar > > Review of attachment 8701306 [details] [diff] [review]: > ::: browser/base/content/browser.xul > @@ +482,5 @@ > > + showFor="single"/> > > + <menuitem label="&tabs.context.bookmarkSingleTab.label;" > > + accesskey="&tabs.context.bookmarkSingleTab.accesskey;" > > + id="syncedTabsBookmarkSelected" > > + showFor="single"/> > > My last feedback suggested killing showFor and simplifying all the special > logic by simply ensuring one item is always selected. I've killed showFor but didn't change the selection behavior so that it stays consistent with XUL.

(In reply to Zachary Carter [:zaach] from comment #43) > This was copied from the XUL search box style, which uses px. Should we > deviate? Nah, that's fine - but maybe add a comment indicating the styles are consistent with xul.

Cleans up the old feedback and sets up state storage tests using sinon.

Comment on attachment 8703759 [details] [diff] [review] WIP create a Sync tabs sidebar Review of attachment 8703759 [details] [diff] [review]: ----------------------------------------------------------------- \o/ - This is looking awesome :) Ted, this patch is adding sinon.js as a testing module - see the last 2 files in this patch - are you OK with that living there? Loop already has it as browser/extensions/loop/test/shared/vendor/sinon-1.16.1.js but I think it makes more sense to have it here (and maybe open a followup for loop to use this version too?) ::: browser/base/content/browser.xul @@ +477,5 @@ > + > + <menupopup id="SyncedTabsSidebarContext"> > + <menuitem label="&syncedTabs.context.openTab.label;" > + accesskey="&syncedTabs.context.openTab.accesskey;" > + id="syncedTabsOpenSelected" /> a trivial nit, but (almost) all other tags in this file don't have a space before the closing /> (here and below) ::: browser/components/syncedtabs/EventEmitter.jsm @@ +31,5 @@ > + if (!this._events.has(event)) { > + return; > + } > + for (let listener of this._events.get(event).values()) { > + listener.apply(this, Array.prototype.slice.call(arguments, 1)); We should use the js spread operator to avoid using |arguments| here. I'm also wondering if we should be catching and reporting exceptions so the first listener failing doesn't prevent others from being called. ::: browser/components/syncedtabs/SyncedTabsDeckStore.js @@ +26,5 @@ > + this._panels = []; > +}; > + > +Object.assign(SyncedTabsDeckStore.prototype, EventEmitter.prototype, { > + _change(isUpdateble) { s/Updateble/Updatable/ everywhere, and I think you might as well specify a default of false for the param - it makes it clearer that the function expects to be called without that param being specified, and I think it means you could sanely avoid the "!!" (we don't do anything to enforce, eg, the type of the panelId param in selectPanel, so I don't see a need to special-case this boolean param if the default is specified) ::: browser/components/syncedtabs/SyncedTabsListStore.js @@ +31,5 @@ > +Object.assign(SyncedTabsListStore.prototype, EventEmitter.prototype, { > + // This internal method triggers the "change" event that views > + // listen for. It denormalizes the state so that it's easier for > + // the view to deal with. updateType hints to the view what > + // actuallly needs to be rerendered or just updated, and can be s/lll/ll/ @@ +33,5 @@ > + // listen for. It denormalizes the state so that it's easier for > + // the view to deal with. updateType hints to the view what > + // actuallly needs to be rerendered or just updated, and can be > + // empty (to (re)render everything), "createList" (to rerender the tab list), > + // or "update" (to just update attributes of existing nodes). these strings smell a little - it seems like it might be clearer to have symbolic names defined somethere = eg, something like: UPDATETYPE_CHANGE: "change", etc, but suppliers and consumers of the value use the name rather than the value (eg, let's say we wanted to add a new value, how would we reasonably find all such suppliers and consumers?) ::: browser/components/syncedtabs/remoteTabComponent.js @@ +67,5 @@ > + init() { > + Services.obs.addObserver(this, this._SyncedTabs.TOPIC_TABS_CHANGED, false); > + > + // Go ahead and trigger sync > + this._SyncedTabs.syncTabs(true); I'm not sure we should pass |true| here, but I'm open to the possibility :) I'm almost thinking that it would be nice to grow a UI (ie, button) for "sync now", but we can consider that later. Also, that function returns a promise, so there should be a .catch that logs. @@ +85,5 @@ > + "singleDeviceInfo", > + "tabs-disabled" > + ]); > + // Set the initial panel to display > + this.updatePanel(); IIUC, this.updatePanel returns a promise but the 3 callers of the function here don't consume it. We should either add .catch for both such calls, or add the .catch on .updatePanel() and don't return the promise (I'm not sure yet if there are callers of that outside this file that does want the promise - I'd guess so, or it would have a leading _ on its name) @@ +110,5 @@ > + > + getPanelStatus() { > + return this._fxAccounts.accountStatus().then(exists => { > + if (!exists) { > + return "notAuthedInfo"; similar comment here re the constants used to represent the state. @@ +125,5 @@ > + } > + return "singleDeviceInfo"; > + }); > + }, (err) => { > + return "notAuthedInfo"; Do we expect this to be hit, or is it a "just in case"? If the latter, maybe it should be a trailing .catch() with a log of the error? @@ +158,5 @@ > + * SyncedTabsDeckView > + * > + * Instances of SyncedTabsDeckView render DOM nodes from a given state. > + * No state is kept internaly and the DOM will completely > + * rerender unless the state flags `isUpdatable`, which helps "isUpdatable" is spelt correctly in the comment here, but still wrong in the code :) @@ +295,5 @@ > + > + onFilterFocus(event) { > + this._syncedTabsListStore.focusInput(); > + }, > + onFilterBlur(event) { nit - newline here @@ +379,5 @@ > + * Handle a keydown event on the list box. > + * @param {Event} event - Triggering event. > + */ > + onListKeyDown(event) { > + if (event.keyCode == KeyEvent.DOM_VK_DOWN) { I don't care that much, but consider a switch/case @@ +451,5 @@ > + case "syncedTabsBookmarkSelected": > + this.bookmarkSingleTab(); > + break; > + case "syncedTabsRefresh": > + this._SyncedTabs.syncTabs(true); re my other comment - I think this is the place where we *do* want that param to be specified. ::: browser/components/syncedtabs/sidebar.xhtml @@ +6,5 @@ > +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" > + "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd" [ > + <!ENTITY % browserDTD SYSTEM "chrome://browser/locale/browser.dtd"> > + %browserDTD; > + <!ENTITY % aboutSyncTabsDTD SYSTEM "chrome://browser/locale/aboutSyncTabs.dtd"> is there a reason why we are still using this .dtd? IIRC I made some comments about this in the last feedback round. @@ +65,5 @@ > + <div class="tabs-container"> > + <div class="sidebar-search-container"> > + <div class="search-box compact"> > + <div class="textbox-input-box"> > + <input type="text" class="tabsFilter textbox-input" /> nit: remove space before /> ::: browser/components/syncedtabs/test/xpcshell/test_EventEmitter.js @@ +4,5 @@ > +"use strict"; > + > +let { EventEmitter } = Cu.import("resource:///modules/syncedtabs/EventEmitter.jsm", {}); > + > +let assert = new Assert(); is this necessary? Most tests just call Assert.ok() without constructing a new object. @@ +37,5 @@ > + assert.ok(spy1.calledOnce); > + assert.ok(spy2.calledTwice); > +}); > + > +function run_test() { I'm not sure this is needed either - I think (but might be wrong) that recently just the add_task calls are enough. ::: browser/components/syncedtabs/test/xpcshell/test_RemoteTabComponent.js @@ +1,2 @@ > +/* Any copyright is dedicated to the Public Domain. > + 2 * http://creativecommons.org/publicdomain/zero/1.0/ */ some leading cruft there. @@ +19,5 @@ > +} > + > +add_task(function* prepare() { > + assert.ok(false, 'should be true'); > +}); I guess this test is still a WIP :) ::: browser/themes/osx/jar.mn @@ +8,5 @@ > skin/classic/browser/sanitizeDialog.css > skin/classic/browser/aboutSessionRestore-window-icon.png > skin/classic/browser/aboutSyncTabs.css > +* skin/classic/browser/syncedtabs/sidebar.css (syncedtabs/sidebar.css) > + skin/classic/browser/syncedtabs/arrow-open.svg (syncedtabs/arrow-open.svg) we should keep these names in parens aligned. ::: browser/themes/osx/syncedtabs/arrow-closed.svg @@ +1,1 @@ > +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="15" height="18" viewBox="0 0 15 18"> the svg files need a license header and indentation - see https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/SVG_Guidelines ::: browser/themes/shared/syncedtabs/sidebar.inc.css @@ +19,5 @@ > + padding: 3em 1em; > + text-align: center; > +} > + > +.list, .item-tabs-list { I think we typically use a newline between rules (I'm not too bothered, and if there are a number of places in our existing CSS where we don't, then ignore me :) @@ +65,5 @@ > + padding-inline-start: 20px; > +} > + > +.item-icon-container, > +.item-twisty-container { but either way, we should be consistent in this file. ::: browser/themes/shared/syncedtabs/twisty-closed.svg @@ +1,5 @@ > +<?xml version="1.0" encoding="UTF-8" standalone="no"?> > +<!-- This Source Code Form is subject to the terms of the Mozilla Public > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > +<svg See that other link I sent - we should remove all namespaces that aren't used @@ +15,5 @@ > + id="svg2"> > + <defs > + id="defs4"> > + <linearGradient > + id="linearGradient3792"> We should give better names to the IDs we need to use and kill the IDs we don't. @@ +53,5 @@ > + xlink:href="#linearGradient3792" > + gradientUnits="userSpaceOnUse" > + gradientTransform="matrix(1.0256411,0,0,1.0256411,0.88461522,0.8846152)" /> > + </defs> > + <metadata and I think this metadata can die ::: browser/themes/windows/syncedtabs/sidebar.css @@ +1,1 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public I guess we are going to need a linux version of this too. I'm also a little surprised there isn't more shared between OSX and Windows (although I haven't actually checked how possible that is)

(In reply to Mark Hammond [:markh] from comment #47) > Comment on attachment 8703759 [details] [diff] [review] > WIP create a Sync tabs sidebar > > Review of attachment 8703759 [details] [diff] [review]: > ----------------------------------------------------------------- > @@ +15,5 @@ > > + id="svg2"> > > + <defs > > + id="defs4"> > > + <linearGradient > > + id="linearGradient3792"> > > We should give better names to the IDs we need to use and kill the IDs we > don't. > The SVG seems to be generated (iirc I got these from a gist you posted a while back), so I'm not sure the naming can improve much. But I've tossed the unreferenced IDs. > > ::: browser/themes/windows/syncedtabs/sidebar.css > @@ +1,1 @@ > > +/* This Source Code Form is subject to the terms of the Mozilla Public > > I guess we are going to need a linux version of this too. I'm also a little > surprised there isn't more shared between OSX and Windows (although I > haven't actually checked how possible that is) Linux should be in there. Yeah, the styles are mostly borrowed from the XUL versions for each platform, with most differences related to icons and their sizing.

Alright, this should be the last big-ish refactor. I moved anything DOM related out of the components and in to the views, which made components much easier to test. The last bit should be to finish view tests.

Fixes the issue mentioned in Mark's feedback where TabListStore would signal updateType using a string. Now we pass a couple of boolean flags instead.

Comment on attachment 8704965 [details] [diff] [review] WIP create a Sync tabs sidebar Review of attachment 8704965 [details] [diff] [review]: ----------------------------------------------------------------- Why the OSX .png changes? But looking good! Are you planning on simple browser-chrome tests? ::: browser/base/content/browser.xul @@ +481,5 @@ > + id="syncedTabsOpenSelected"/> > + <menuitem label="&syncedTabs.context.bookmarkSingleTab.label;" > + accesskey="&syncedTabs.context.bookmarkSingleTab.accesskey;" > + id="syncedTabsBookmarkSelected"/> > + <menuseparator /> nit - space before "/>" ::: browser/components/syncedtabs/EventEmitter.jsm @@ +39,5 @@ > + } catch (e) { > + Cu.reportError(e); > + } > + } > + } trivial nit - add a trailing "," here so the addition of new stuff doesn't touch the line above. ::: browser/components/syncedtabs/SyncedTabsDeckComponent.js @@ +40,5 @@ > + // used to stub during tests > + this._getChromeWindow = getChromeWindowMock || getChromeWindow; > + > + this._deckStore = deckStore || new SyncedTabsDeckStore(); > + this._syncedTabsListStore = listStore || new SyncedTabsListStore(SyncedTabs); extra space after the "||" here. @@ +49,5 @@ > + SyncedTabs: SyncedTabs > + }); > +}; > + > +SyncedTabsDeckComponent.PANELS = { can't we just stick this in the prototype? @@ +67,5 @@ > + Services.obs.addObserver(this, this._SyncedTabs.TOPIC_TABS_CHANGED, false); > + > + // Go ahead and trigger sync > + this._SyncedTabs.syncTabs() > + .catch(Cu.reportError); I think you should align the "."s (but not bothered really :) @@ +104,5 @@ > + > + getPanelStatus() { > + return this._fxAccounts.accountStatus().then(exists => { > + if (!exists) { > + return SyncedTabsDeckComponent.PANELS.NOT_AUTHED_INFO; you can use |this| instead of |SyncedTabsDeckComponent| if you want @@ +119,5 @@ > + } > + return SyncedTabsDeckComponent.PANELS.SINGLE_DEVICE_INFO; > + }); > + }, (err) => { > + Cu.reportError(err); it still looks like this needs a .catch instead of that 2nd err param - exceptions in the lines above will not be caught as written, where a trailing .catch would. (That error *would* be cause below in updatePanel(), but it wouldn't trigger the .selectPanel of this fallback panel ID) ::: browser/components/syncedtabs/SyncedTabsListStore.js @@ +38,5 @@ > + _change(updateType) { > + let selectedParent = this._selectedRow[0]; > + let selectedChild = this._selectedRow[1]; > + let rowSelected = false; > + let data = Cu.cloneInto(this.data, {}); We should probably document why we need to clone this.data - the loop below doesn't seem to mutate it, so it's not clear why this.data can't be used. @@ +72,5 @@ > + // If this were React the view would be smart enough > + // to not re-render the whole list unless necessary. But it's > + // not, so updateType is a hint to the view of what actually > + // needs to be rerendered. > + this.emit("change", { clients: data, updateType, filter: this.filter, inputFocused: !!this.inputFocused }); I don't see why we need the "!!" here (apart from the fact it will be undefined as the object is created - but I think you should just fix that (ie, initialize it in the ctor or the prototype)) @@ +147,5 @@ > + (parent >= maxParentRow) ? maxParentRow - 1 : > + parent; > + let childRow = (parentRow === -1 || this.filter || typeof child === "undefined" || child < -1) ? -1 : > + (child >= this.data[parentRow].tabs.length) ? this.data[parentRow].tabs.length - 1 : > + child; I think these are too complex and it's worth a few extra lines and/or comments to make them readable.

Added browser tests and fixed markh's feedback

Comment on attachment 8707666 [details] [diff] [review] WIP create a Sync tabs sidebar Review of attachment 8707666 [details] [diff] [review]: ----------------------------------------------------------------- This is looking great - I don't see any other issues other than to verify it looks and works fine on the 3 platforms we care about. As discussed on IRC, Zach will push a try and try to rope Ryan and possibly others into running the build. ::: browser/components/syncedtabs/SyncedTabsListStore.js @@ +221,5 @@ > + .then(result => { > + this.data = result; > + this._change(updateType); > + }) > + .catch(Cu.reportError); strange indentation here. ::: browser/components/syncedtabs/TabListComponent.js @@ +98,5 @@ > + }, > + > + onBookmarkTab(uri, title) { > + this._window.top.PlacesCommandHook.bookmarkLink(this._window.PlacesUtils.bookmarksMenuFolderId, uri, title) > + .catch(Cu.reportError); strange alignment here too :) ::: browser/components/syncedtabs/test/browser/browser_sidebar_syncedtabslist.js @@ +1,2 @@ > +/* Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ */ We don't require license headers in test these days. @@ +242,5 @@ > + Assert.ok(node.classList.contains("item"), > + "Node should have .item class"); > + if (item.client) { > + // tab items > + Assert.equal(node.querySelector(".item-title").textContent, item.title, lots of strange indentation here and below @@ +262,5 @@ > + } > +}; > + > +registerCleanupFunction(function*() { > + delete window.sinon; we should add a comment here saying why we are doing this (although it looks like you are also doing it in tail.js - one should die and the other should get the comment (OTOH though, it probably makes sense to add it to head.js next to where sinon is loaded) ::: browser/components/syncedtabs/test/xpcshell/test_EventEmitter.js @@ +1,1 @@ > +/* Any copyright is dedicated to the Public Domain. you can kill all copyright banners in tests ::: browser/themes/linux/syncedtabs/sidebar.css @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +%include ../../shared/syncedtabs/sidebar.inc.css > + I think we should add a comment here indicating that in general, we are trying to make our HTML look like a XUL tree. ::: browser/themes/osx/syncedtabs/sidebar.css @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +%include ../../shared/syncedtabs/sidebar.inc.css > + ditto here and all other platform-specific .css

Hey Ryan, can you r? the UX of the latest build? archive.mozilla.org/pub/firefox/try-builds/zcarter@mozilla.com-0f2837e222127fe126ca64a23eaa7585bd72b687/try-macosx64-debug/

new new build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e0bcd9565dc

Incorporated feedback from rfeeley on the UI

Shoot me a new build when you have one.

Very nice Zach! Looks good, favicons appear, no issues from my 5 min review.

Fixes test leak

Here's a new build with the UI updates: https://archive.mozilla.org/pub/firefox/try-builds/zcarter@mozilla.com-b7424d4a23f66109aee53e2a4da39167fe7144a9/

Fixes some of the Windows styles.

Comment on attachment 8710788 [details] [diff] [review] Create a Synced tabs sidebar Review of attachment 8710788 [details] [diff] [review]: ----------------------------------------------------------------- Issues found when running this: * twisties are wrong on Windows (see below) * "Bookmark this page" doesn't do anything for me. * Pressing down-arrow when at the bottom of the list moves to the top of the list, which is unexpected and not how lists generally work. * The current selection in the list is always lost as each Sync happens - but there seems to be special code to handle this case based on previous review comments. * The "search" box is supposed to have a magnifying glass like "bookmarks" has. * The alignment of the tree looks all wrong on Windows and the color around the search box is wrong - I'll attach a screen-shot I understand we want to get this into 46 and can fix these in followups, so I'll give it r+ (with the inline nits addressed), however, I'd still like ckarlof and rfeeley to sign off on landing it in this state as they may feel strongly they'd prefer to get it more polished so it doesn't reflect poorly on us for those first few weeks of it living on Nightly. ::: browser/components/syncedtabs/SyncedTabsDeckComponent.js @@ +158,5 @@ > + this._window.openUILink(url, event); > + }, > + > + openSyncPrefs() { > + this._getChromeWindow(this._window).gSyncUI.openSetup(); we should pass an entry-point here - "tabs-sidebar" will probably do. ::: browser/components/syncedtabs/TabListComponent.js @@ +72,5 @@ > + }, > + > + onFilterFocus() { > + this._store.focusInput(); > + }, add a blank line here for consistency ::: browser/components/syncedtabs/test/browser/browser_sidebar_syncedtabslist.js @@ +57,5 @@ > + let SyncedTabs = window.SidebarUI.browser.contentWindow.SyncedTabs; > + syncedTabsDeckComponent._accountStatus.restore(); > + SyncedTabs._internal.getTabClients.restore(); > + SyncedTabs._internal = originalSyncedTabsInternal; > + let defer = Promise.defer(); if we write this as: yield new Promise(resolve => { window.SidebarUI.browser.contentWindow.addEventListener("unload", function listener() { window.SidebarUI.browser.contentWindow.removeEventListener("unload", listener); resolve(); }); SidebarUI.hide(); }); we can avoid the .defer() from Promise.jsm ::: browser/components/syncedtabs/test/browser/head.js @@ +2,5 @@ > + > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > +Cu.import("resource://gre/modules/Services.jsm"); > +Cu.import("resource://gre/modules/Task.jsm"); > +Cu.import("resource://gre/modules/Promise.jsm"); We should try and remove the use of Promise.jsm, and it looks like that 1 test doesn't actually use Task.jsm (so that can be removed too) @@ +11,5 @@ > +let loader = Cc["@mozilla.org/moz/jssubscript-loader;1"].getService(Ci.mozIJSSubScriptLoader); > +loader.loadSubScript("resource://testing-common/sinon-1.16.1.js"); > + > +registerCleanupFunction(function*() { > + delete window.sinon; I think a comment explaining why this dance is necessary would be nice. ::: browser/components/syncedtabs/test/xpcshell/head.js @@ +2,5 @@ > + > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > +Cu.import("resource://gre/modules/Services.jsm"); > +Cu.import("resource://gre/modules/Task.jsm"); > +Cu.import("resource://gre/modules/Promise.jsm"); ditto here - remove Promise.jsm and Task.jsm ::: browser/components/syncedtabs/test/xpcshell/test_SyncedTabsListStore.js @@ +37,5 @@ > + } > +]; > + > +add_task(function* testGetDataEmpty() { > + var store = new SyncedTabsListStore(SyncedTabs); use let here (and the same thing further down this file) ::: browser/themes/osx/syncedtabs/arrow-closed.svg @@ +1,1 @@ > +<?xml version="1.0" encoding="UTF-8" standalone="no"?> I don't understand why this is only used on OSX - the other sidebars all use these "arrow" style twisties on all platforms.

This is from Windows. The issues I see are: * The color on the left and right of the search box has a different background color. * The twisties are not the arrow ones used by the XUL tree (ie, as used in bookmarks, history, etc) * The spacing of the icons looks wrong (but that's obviously subjective - rfeeley gets the final say there :)

Chris, can you please see the top of comment 64 and the screenshot in comment 65, and in conjunction with rfeeley decide if you are happy for this to land now and polish in followups?

Doh - it would help if I needinfo'd! See comment 66.

I am testing it a little bit and I was able to get it into a state where selecting "Synced Tabs" for the menu bar doesn't do anything. I'm also a little confused about the expectations around how the sidebar would be opened. Obviously a quality feature is more important that any specific release. It would be great to get more testing on this, and landed it in Nightly will make it easier for the Softvision folks to help with that. How about this: 1) Land the patch with a flag around exposing affordances to open the sidebar restricting it to Nightly. 2) Let it ride the trains and we can make a decision later if and when we want to expose it.

(In reply to Pulsebot from comment #70) > https://hg.mozilla.org/integration/mozilla-inbound/rev/07c32801e3be Hey Zaach, thanks for the patch, i did the merge now from this changes to mozilla-central :) 2 little nits : - we call a bug fixed when the commits land on mozilla-central and a tool normally marks then the bug as fixed :) - regarding the checkin from comment #70 seems this is the push from the nits from the review, i guess normally this are included into the inital patch :)

https://hg.mozilla.org/mozilla-central/rev/b66a58e676f8 https://hg.mozilla.org/mozilla-central/rev/07c32801e3be

https://hg.mozilla.org/mozilla-central/rev/b66a58e676f8 https://hg.mozilla.org/mozilla-central/rev/07c32801e3be

Zachary, thank you so much for adding Sinon.js to the tree! And also for putting it in the right place. I can't wait to start playing with it! It's going to brings lots of joy, especially for XPCShell tests. I fact, my hope is that we can choose for XPCShell tests instead of Mochitests more often now. Please don't be shy to broadcast these type of changes to firefox-dev, or dev-platform for a broader audience!

Setting status flag for proper tracking.

Verified fixed on Windows 7 64bit, Ubuntu 12.04 32bit and Mac OSX 10.9.5 using Firefox 47 RC (buildID: 20160531183335) and also exploratory testing related to "Synced Tabs Sidebar" was performed on Firefox 47 Beta 7.