801056 - Add remote tabs list (tabs synced from other devices) to Firefox Start

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Browser, defect, P2)

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Awesomescreen should show "Top sites," "Bookmarks," "History," and "Synced tabs" per metro mvp document.

This bug is about adding a column for "Synced tabs"

Comment 1

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Attachment: Sync tabs toggle btn

Comment 2

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Synced tabs should be accessed from Tab Panel instead of Awesome screen.

Once Sync is set up, users should be able to toggle the btn on the tab panel to load tabs from other computer. 

Would like to hear your rationale/ use case of adding a column for "Synced tabs"

Comment 4

[Matt Brubeck (:mbrubeck)]

On the tab strip is fine. I'll update the bug summary.

Yuan, Stephen: How should the list of remote tabs look?  We don't have thumbnails for remote tabs -- just the title and URL, and the name of the remote device.  On Android we display them as a vertical list, grouped by device, like in attachment 609480 [details], but this won't work in Metro Firefox's horizontal tab strip.

Comment 5

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

This helps. Thanks, Matt.
I will work with Stephen on this topic.

Comment 6

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

I explored a few directions on accessing remote tabs from tab panel and discussed with frank today. 
1. Considering the remote tabs work just like links (select to open in a tab), instead of fully functional tabs(capability of closing or adding a new remote tab). Displaying them in the Tab Strip might cause confusion for users. 
2. Toggling between local and remote tabs will make a heavy change on the UI. That will bring more complexity to the existing 2 tab view modes. 

So we would like to go for the original direction: having remote tabs from start screen.


My question for Matt or Tim: What information can we get from synced tabs? only title and URL? would it be possible to have info like: fav icon, last time viewed?

Comment 7

[Matt Brubeck (:mbrubeck)]

(In reply to Yuan Wang(:Yuan) – Firefox UX Team from comment #6)
> My question for Matt or Tim: What information can we get from synced tabs?
> only title and URL? would it be possible to have info like: fav icon, last
> time viewed?

For each tab we have the title, URL, favicon, "last used" time (the most recent time the tab was opened or selected), and "client".

Each "client" is a Firefox profile connected to the user's sync account.  For each client we have a unique ID, a name (by default something like "Matt's Nightly on Windows"), and a type (desktop or mobile).

Comment 8

[Matt Brubeck (:mbrubeck)]

Attachment: mockup from Yuan with remote tabs on awesomescreen

Wireframes from Yuan with remote tabs on awesomescreen:
http://cl.ly/image/3v3S1v2X3S28/o

As a follow-up we will want to think about how to best provide access to a very large number of tabs, for the small number of users with dozens or hundreds of open tabs.  As another follow-up we could think about using this space to promote Firefox Sync for people who are not using it yet.

Comment 9

[Allison Naaktgeboren :ally]

dozens? I have over 200-300 synced at any given time. 

I certainly have no ulterior motive for picking up this bug! >.>

Comment 10

[Allison Naaktgeboren :ally]

Attachment: wip, currently displaying blank grids

stealing a page out of wesj's book, work-in-progress patch

Comment 11

[Allison Naaktgeboren :ally]

Attachment: wip, see remaining todo items tabs.js

a little bit of clean up left (wonky line endings), optimization to avoid sync every time, and some through vetting before sending to review

Comment 12

[Allison Naaktgeboren :ally]

Attachment: wip w/async calls & voodoo observers

saner code & less ugly. Testing running into pref pane issue

Comment 13

[Matt Brubeck (:mbrubeck)]

Bug 816552 fixed some similar pref panel issues for me; have you updated to elm tip recently?  If you are seeing it on tip, maybe it's a similar problem.

Comment 14

[Allison Naaktgeboren :ally]

I haven't updated since that landed. Ill update & keep an eye out

Comment 15

[Allison Naaktgeboren :ally]

Attachment: wip, with disappearing error console

Comment 16

[Allison Naaktgeboren :ally]

Attachment: wip appears to work (but doesnt)

This appears to function as desired on the first run.

however if you enable the error console,
- the error console appears correctly in the menu
- the pref panel contains the content of the error console
- the error console panel is blank

if you then disable the error console, then the menu never appears again.

Comment 17

[Allison Naaktgeboren :ally]

I further note that if you remove the <menuitem-console> from the the menu, the preferences pane contains the preferences data, but the hidden call on the menuitem-remotetabs doesnt affect the menu (ie the menu entry shows up even though 'hidden' has been set to true.)

Comment 18

[Matt Brubeck (:mbrubeck)]

(In reply to :Ally Naaktgeboren from comment #17)
> I further note that if you remove the <menuitem-console> from the the menu,
> the preferences pane contains the preferences data, but the hidden call on
> the menuitem-remotetabs doesnt affect the menu (ie the menu entry shows up
> even though 'hidden' has been set to true.)

This could be a bug in the custom <menulist> binding.  It looks like it should do the correct thing with hidden menuitems, but that might not be well-tested.

http://hg.mozilla.org/projects/elm/file/df59355959c2/browser/metro/base/content/bindings/bindings.xml#l387
http://hg.mozilla.org/projects/elm/file/df59355959c2/browser/metro/base/content/helperui/MenuUI.js

Comment 19

[Allison Naaktgeboren :ally]

Bug caught. Both symptoms appear to be caused by hiding the container. I could find nothing in the deck, vbox, or other xul documentation about why this would break the world in this manner. :wesj thinks that the selectedindex is taking visibility into account some places (but not others).

Comment 20

[Allison Naaktgeboren :ally]

Attachment: wip, includes debugging code from container issue

the menu bug turned out to be more of a deck bug.

Comment 21

[Allison Naaktgeboren :ally]

Attachment: draft 0 part 1/1

mconnor for sync focused review, mbrubeck for metro focused review.

NB: in keeping with yuan's flat view, tabs are not sorted by device (similar to how they are not sorted on android)

Comment 22

[Matt Brubeck (:mbrubeck)]

Comment on attachment 693153 [details] [diff] [review]
draft 0 part 1/1

Review of attachment 693153 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great overall; I'd just like to see a number of minor changes and style nits.

Some basic automated "smoke tests" would be great (but can be a separate patch in this bug, or a follow-up bug).

General nit:  Several files have mixed tabs and spaces, which among other things makes them hard to read in Bugzilla's web views.  Please use spaces only.  Please also strip trailing whitespace added by this patch, if it's not too much trouble, though that's not as critical.

::: browser/metro/base/content/browser-ui.js
@@ +728,2 @@
>        case "cmd_remoteTabs":
> +#endif

I'm not sure if all these ifdefs are actually useful... we can keep them for now but if it simplifies our code at any point then we should just require MOZ_SERVICES_SYNC to be enabled.  (Since we never build without it, it's highly likely that disabling it would already break something or other.)

@@ +1275,5 @@
>      let panel = aPanelId ? document.getElementById(aPanelId) : this._panels.selectedPanel;
> +		// anaaktge
> +    //Components.utils.reportError("in switch pane: "+ aPanelId);
> +		try {
> +		//let sr = new XMLSerializer();

Reminder to remove debugging code here.

::: browser/metro/base/content/tabs.js
@@ +15,5 @@
> + * @param    aSetUIAccess The UI element that should be hidden when Sync is
> + *                          disabled. Must sanely support 'hidden' attribute.
> + *                          You may only have one UI access point at this time.
> + */
> + function RemoteTabsView(aSet, aSetUIAccess) {

Please remove the leading space before "function" (and thanks for humoring my whitespace obsessions).

@@ +58,5 @@
> +  },
> +  hideUIAccess: function hideUIAccess(isShown) {
> +    let currentShown = this._setUIAccess.hidden;
> +    if (currentShown != isShown) {
> +      this._setUIAccess.hidden = !currentShown;

Sorry, the naming here is too confusing to me.  You call hideUIAccess(true) to show the UI, and hideUIAccess(false) to hide it?

How about something more like: "function setUIAcessVisible(aVisible) { this._setUIAccess.hidden = !aVisible; }"

@@ +68,5 @@
> +    let tabsEngine = Weave.Service.engineManager.get("tabs");
> +    let list = this._set;
> +
> +		for (let [guid, client] in Iterator(tabsEngine.getAllClients())) {
> +			let seenURLs = {};

Do we really want to reset seenURLs for each client, or should we move this line outside of the loop?  (Currently, it looks like the same URL might be added to the grid twice if it's open on multiple clients.)

@@ +112,5 @@
> +  get _grid() { return document.getElementById("start-remotetabs-grid"); },
> +
> +  init: function init() {
> +  	let vbox = document.getElementById("start-remotetabs");
> +		this._view = new RemoteTabsView(this._grid, vbox);

This file seems to have an especially bad case of mixed tabs and spaces.

@@ +115,5 @@
> +  	let vbox = document.getElementById("start-remotetabs");
> +		this._view = new RemoteTabsView(this._grid, vbox);
> +    if (!this._view.isSyncEnabled()) {
> +      this._view.hideUIAccess(true);
> +    }

The "if" statement here is shared by both view controllers; maybe it should move into the RemoteTabsView constructor.

::: browser/metro/base/jar.mn
@@ +97,5 @@
>    content/LoginManagerChild.js                 (content/LoginManagerChild.js)
>    content/video.js                             (content/video.js)
>  #ifdef MOZ_SERVICES_SYNC
>    content/sync.js                              (content/sync.js)
> +  content/tabs.js                              (content/tabs.js)

I know this fits in with XUL Fennec's traditional file naming conventions, but I'd like to move toward file names that are more closely linked to the interfaces/objects they define.  Maybe RemoteTabs.js?

Comment 23

[Matt Brubeck (:mbrubeck)]

Comment on attachment 693153 [details] [diff] [review]
draft 0 part 1/1

>+			let seenURLs = {};

Another minor change: please use "let seenURLs = new Set();"

>+				if (tabsEngine.locallyOpenTabMatchesURL(url) || url in seenURLs) {

"seenURLs.has(url)"

>+				seenURLs[url] = null;

and "seenURLs.add(url)".

I think your original code was perfectly safe, but it made me do a mental audit for the various Object-as-Map pitfalls that Set avoids in the first place.

Comment 24

[Allison Naaktgeboren :ally]

> Some basic automated "smoke tests" would be great (but can be a separate
> patch in this bug, or a follow-up bug).

Sounds reasonable. Can you point me at some that we use for our ui?

> 
> General nit:  Several files have mixed tabs and spaces.

It's not that it is too much trouble (and I strongly believe in hardtabs), it's that something keeps either re-adding them or ignoring the changes when I strip them (you've helped me strip the tabs on irc). I need to figure out why that keeps biting me anyway.
> 
> ::: browser/metro/base/content/browser-ui.js
> @@ +728,2 @@
> >        case "cmd_remoteTabs":
> > +#endif
> 
> I'm not sure if all these ifdefs are actually useful... we can keep them for
> now but if it simplifies our code at any point then we should just require
> MOZ_SERVICES_SYNC to be enabled.  (Since we never build without it, it's
> highly likely that disabling it would already break something or other.)

The wrapping isn't about us right now, it is for when we merge into m-c. Some things build with sync disabled. Mike Connor could speak to whether or not we can safely remove it (but I'm pretty we can't).

> @@ +68,5 @@
> > +    let tabsEngine = Weave.Service.engineManager.get("tabs");
> > +    let list = this._set;
> > +
> > +		for (let [guid, client] in Iterator(tabsEngine.getAllClients())) {
> > +			let seenURLs = {};
> 
> Do we really want to reset seenURLs for each client, or should we move this
> line outside of the loop?  (Currently, it looks like the same URL might be
> added to the grid twice if it's open on multiple clients.)

I think that *might* be part of the expected behavior for sync. However, I can't find it written down anywhere either way. :mconnor?

Comment 25

[Allison Naaktgeboren :ally]

Attachment: draft 1, part 1/2

- waiting on more info on tests (will be a separate patch anyway)
- waiting on sync review from mconnor
- should please mbrubeck better :)

Comment 26

[Allison Naaktgeboren :ally]

Attachment: draft 1*, part 1/2

now with the right file*

Comment 27

[Matt Brubeck (:mbrubeck)]

(In reply to :Ally Naaktgeboren from comment #24)
> > Some basic automated "smoke tests" would be great (but can be a separate
> > patch in this bug, or a follow-up bug).
> 
> Sounds reasonable. Can you point me at some that we use for our ui?

The best example right now is:
http://hg.mozilla.org/projects/elm/file/default/browser/metro/base/tests/browser_downloads.js

We don't need anything that elaborate to start out with, though; the original version of browser_downloads.js was much simpler.

> The wrapping isn't about us right now, it is for when we merge into m-c.
> Some things build with sync disabled. Mike Connor could speak to whether or
> not we can safely remove it (but I'm pretty we can't).

Code in /browser/metro will only be built with sync enabled.  The ifdefs are more important for shared code like /toolkit that is included in both sync- and non-sync-enabled products.

> > Do we really want to reset seenURLs for each client, or should we move this
> > line outside of the loop?  (Currently, it looks like the same URL might be
> > added to the grid twice if it's open on multiple clients.)
> 
> I think that *might* be part of the expected behavior for sync. However, I
> can't find it written down anywhere either way. :mconnor?

If we were grouping the tabs by client (and showing the client names) like desktop and Android do, then it would make sense to show the same page multiple times for multiple clients.  But if we are flattening the tabs into a single list, I don't think it makes any sense to show the same page multiple times in the list.

Comment 28

[Allison Naaktgeboren :ally]

Attachment: wip part 2/2, tests

Comment 29

[Allison Naaktgeboren :ally]

forward progress on the test is blocked by the mochitest regression in bug 825034.
patch is still waiting on sync review

Comment 30

[Mike Connor [:mconnor]]

Comment on attachment 697229 [details] [diff] [review]
draft 1*, part 1/2

Review of attachment 697229 [details] [diff] [review]:
-----------------------------------------------------------------

Ship it!

::: browser/metro/base/content/RemoteTabs.js
@@ +3,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/. */
> +
> +'use strict';
> +const Cu = Components.utils;

This seems to be the only invocation of Components.utils, so I wouldn't bother with the const here.

@@ +48,5 @@
> +  observe: function(subject, topic, data) {
> +    switch (topic) {
> +      case "weave:service:setup-complete":
> +        this.populateTabs();
> +        this.populateGrid();

I don't think you need to call populateGrid if you're calling populateTabs, since the sync finishing should trigger grid population with the sync:finish observer.

@@ +55,5 @@
> +      case "weave:service:sync:finish":
> +        this.populateGrid();
> +        break;
> +      case "weave:service:start-over":
> +        this.setUIAccessVisible(true);

should be false, start-over means Sync is disabled.

@@ +61,5 @@
> +    }
> +  },
> +
> +  setUIAccessVisible: function setUIAccessVisible( aVisible) {
> +    this.setUIAccess.hidden = !aVisible;

Should be this._setUIAccess

@@ +93,5 @@
> +    Weave.Service.scheduler.scheduleNextSync(0);
> +
> +    // make sure clients has sync'ed recently
> +    if (Weave.Service.clientsEngine.lastSync == 0)
> +      Weave.Service.clientsEngine.sync();

I don't think we need this at all if there's a full sync happening.  I apparently said to leave this in, in which case it was mconnor-needs-PTO brain at work.

::: browser/metro/base/content/browser.xul
@@ +388,5 @@
> +#ifdef MOZ_SERVICES_SYNC
> +        <box id="remotetabs-container" flex="1">
> +          <richgrid id="remotetabs-list" seltype="single" flex="1"/>
> +        </box>
> +#endif

All of the #ifdefs can go.  I agree with mbrubeck's assertion here.

::: browser/metro/locales/en-US/chrome/browser.dtd
@@ +35,5 @@
>  <!ENTITY startTopSitesHeader.label        "Top Sites">
>  <!ENTITY startBookmarksHeader.label       "Bookmarks">
>  <!ENTITY startHistoryHeader.label         "Recent History">
>  <!ENTITY startDownloadsHeader.label       "Downloads">
> +<!ENTITY startRemoteTabsHeader.label      "Tabs from other devices">

This is "Tabs From Other Devices" on the classic side, and the other items are all capitalized in this style, so please make it consistent.

Comment 31

[Allison Naaktgeboren :ally]

Attachment: draft 2, part 1/2, has r=mconnor

Comment 32

[Allison Naaktgeboren :ally]

Attachment: draft 0 part 2/2 tests

Comment 33

[Allison Naaktgeboren :ally]

https://hg.mozilla.org/projects/elm/rev/71733f52f243  code landed

Comment 34

[Matt Brubeck (:mbrubeck)]

Comment on attachment 698998 [details] [diff] [review]
draft 0 part 2/2 tests

Review of attachment 698998 [details] [diff] [review]:
-----------------------------------------------------------------

r=mbrubeck with comments fixed

::: browser/metro/base/tests/browser_remotetabs.js
@@ +25,5 @@
> +  Weave.Status._authManager.username = "jane doe"; // must set username before key
> +  Weave.Status._authManager.basicPassword = "goatcheesesalad";
> +  Weave.Status._authManager.syncKey = "a-bcdef-abcde-acbde-acbde-acbde";
> +  // check that it worked
> +  isnot(Weave.Status.checkSetup(), Weave.CLIENT_NOT_CONFIGURED, "Sync is be enabled");

typo nit - "is be"

@@ +30,5 @@
> +
> +  // start page grid should be visible
> +  isnot(vbox.hidden, "false", "remote tabs should be visible in start page when sync is enabled");
> +  // container link should be visible
> +  isnot(menulink.hidden, "false", "link to container should be visible when sync is enabled");

These assertions are backwards - if the element should be visible, then we want "is(element.hidden, false)"

The element.hidden property is a boolean (unlike element.getAttribute("hidden") which returns a string), which might explain why these assertions are passing currently.

Comment 35

[Allison Naaktgeboren :ally]

Attachment: fixed tests

minor changes reviewed offline

Comment 36

[Allison Naaktgeboren :ally]

https://hg.mozilla.org/projects/elm/rev/d3303c5b067e test landed

Comment 37

[Matt Brubeck (:mbrubeck)]

Resolving bugs in the Firefox for Metro product that are fixed on the elm branch.  Sorry for the bugspam.  Search your email for "bugspam-elm" if you want to find and delete all of these messages at once.

Comment 38

[Asa Dotzler [:asa]]

Blocked verifying this because sync setup is failing for me. I can pair but the Connect to Sync message dialog on Metro just keeps spinning and if I kill it, then go to Options, I'm returned to a setup as if I never finished the previous.

Comment 39

[Matt Brubeck (:mbrubeck)]

These patches landed and have not been backed out; please file follow-up bugs for any remaining issues that do not already have open bugs.

Comment 40

[Asa Dotzler [:asa]]

Yeah. we can leave this resolved. But we cannot verify until Sync setup is actually working. We'll leave this in a resolved but unverified state until we can actually test it.