Closed Bug 801056 Opened 9 years ago Closed 9 years ago

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

Categories

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

All
Windows 8.1
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: TimAbraldes, Assigned: ally)

References

Details

(Keywords: feature, Whiteboard: [metro-mvp][LOE:2][metro-it1][metro-it2][completed-elm])

Attachments

(3 files, 12 obsolete files)

137.11 KB, image/png
Details
13.46 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
3.25 KB, text/plain
Details
Awesomescreen should show "Top sites," "Bookmarks," "History," and "Synced tabs" per metro mvp document.

This bug is about adding a column for "Synced tabs"
Keywords: uiwanted
Whiteboard: [metro-mvp] → [metro-mvp][LOE:2]
Assignee: nobody → ywang
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"
Flags: needinfo?(tabraldes)
Duplicate of this bug: 801177
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.
Flags: needinfo?(tabraldes)
This helps. Thanks, Matt.
I will work with Stephen on this topic.
Priority: -- → P2
Summary: Add "synced tabs" to Awesomescreen → Add remote tabs list (tabs synced from other devices) to tab bar
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?
(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).
Summary: Add remote tabs list (tabs synced from other devices) to tab bar → Add remote tabs list (tabs synced from other devices) to 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.
Attachment #675327 - Attachment is obsolete: true
Assignee: ywang → nobody
Keywords: uiwanted
dozens? I have over 200-300 synced at any given time. 

I certainly have no ulterior motive for picking up this bug! >.>
Assignee: nobody → ally
Whiteboard: [metro-mvp][LOE:2] → [metro-mvp][LOE:2][metro-it1]
Depends on: 809171
stealing a page out of wesj's book, work-in-progress patch
Whiteboard: [metro-mvp][LOE:2][metro-it1] → [metro-mvp][LOE:2][metro-it1][metro-it2]
a little bit of clean up left (wonky line endings), optimization to avoid sync every time, and some through vetting before sending to review
Attachment #688082 - Attachment is obsolete: true
saner code & less ugly. Testing running into pref pane issue
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.
I haven't updated since that landed. Ill update & keep an eye out
Attachment #690697 - Attachment is obsolete: true
Attachment #691597 - Attachment is obsolete: true
Attached patch wip appears to work (but doesnt) (obsolete) — Splinter Review
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.
Attachment #692023 - Attachment is obsolete: true
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.)
(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
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).
the menu bug turned out to be more of a deck bug.
Attachment #692072 - Attachment is obsolete: true
Attached patch draft 0 part 1/1 (obsolete) — Splinter Review
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)
Attachment #692119 - Attachment is obsolete: true
Attachment #693153 - Flags: review?(mconnor)
Attachment #693153 - Flags: review?(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?
Attachment #693153 - Flags: review?(mbrubeck) → review-
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.
> 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?
Attached patch draft 1, part 1/2 (obsolete) — Splinter Review
- waiting on more info on tests (will be a separate patch anyway)
- waiting on sync review from mconnor
- should please mbrubeck better :)
Attachment #693153 - Attachment is obsolete: true
Attachment #693153 - Flags: review?(mconnor)
Attached patch draft 1*, part 1/2 (obsolete) — Splinter Review
now with the right file*
Attachment #697228 - Attachment is obsolete: true
Attachment #697229 - Flags: review?(mconnor)
(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.
No longer depends on: 809171
Attached patch wip part 2/2, tests (obsolete) — Splinter Review
Depends on: 825034
forward progress on the test is blocked by the mochitest regression in bug 825034.
patch is still waiting on sync review
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.
Attachment #697229 - Flags: review?(mconnor) → review+
Attachment #697229 - Attachment is obsolete: true
Attachment #698166 - Flags: review?(mbrubeck)
Attachment #698166 - Flags: review?(mbrubeck) → review+
Attached patch draft 0 part 2/2 tests (obsolete) — Splinter Review
Attachment #697787 - Attachment is obsolete: true
Attachment #698998 - Flags: review?(mbrubeck)
https://hg.mozilla.org/projects/elm/rev/71733f52f243  code landed
Whiteboard: [metro-mvp][LOE:2][metro-it1][metro-it2] → [metro-mvp][LOE:2][metro-it1][metro-it2][completed-elm][test code not landed]
Attachment #698998 - Attachment is patch: true
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.
Attachment #698998 - Flags: review?(mbrubeck) → review+
Attached file fixed tests
minor changes reviewed offline
Attachment #698998 - Attachment is obsolete: true
https://hg.mozilla.org/projects/elm/rev/d3303c5b067e test landed
Whiteboard: [metro-mvp][LOE:2][metro-it1][metro-it2][completed-elm][test code not landed] → [metro-mvp][LOE:2][metro-it1][metro-it2][completed-elm]
Blocks: 833133
Blocks: 833126
No longer blocks: 833126
No longer blocks: 747789
Blocks: 835554
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.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Summary: Add remote tabs list (tabs synced from other devices) to awesomescreen → Add remote tabs list (tabs synced from other devices) to Firefox Start
Blocks: 831615
No longer blocks: metro-sync
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.
Status: RESOLVED → REOPENED
Keywords: feature
Resolution: FIXED → ---
Whiteboard: [metro-mvp][LOE:2][metro-it1][metro-it2][completed-elm]
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.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Keywords: feature
Resolution: --- → FIXED
Whiteboard: [metro-mvp][LOE:2][metro-it1][metro-it2][completed-elm]
Flags: needinfo?(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.
Flags: needinfo?(asa)
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.