Closed Bug 702005 Opened 13 years ago Closed 9 years ago

Provide 'Tabs from Other Computers' button in customisation palette

Categories

(Firefox :: Toolbars and Customization, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1201331

People

(Reporter: tech4pwd, Assigned: mathu, Mentored)

References

Details

(Keywords: icon, Whiteboard: [good first bug][lang=js][fxsync])

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:11.0a1) Gecko/20111111 Firefox/11.0a1
Build ID: 20111111031514

Steps to reproduce:

There should be a Tabs from Other Computers/Tabs from Other Devices button available in the customisation palette.
Blocks: 714281, 716271
Summary: Provide 'Tabs from Other Computers' button → Provide 'Tabs from Other Computers' button in customisation palette
Component: Firefox Sync: UI → Toolbars
Product: Mozilla Services → Firefox
QA Contact: sync-ui → toolbars
Version: unspecified → Trunk
No longer blocks: 714281, 716271
Depends on: 714281, 716271
Severity: normal → enhancement
Yes! +1
Status: UNCONFIRMED → NEW
Ever confirmed: true
This doesn't block shipping Australis by any means, but it might be a nice inclusion. Willing to mentor if anybody wants to step up.
Whiteboard: [good first bug][mentor=mconley][lang=js]
(In reply to Mike Conley (:mconley) from comment #3)
> This doesn't block shipping Australis by any means, but it might be a nice
> inclusion. Willing to mentor if anybody wants to step up.

Mike, if you're still willing to mentor, I'm willing to take this on. I'd appreciate being pointed toward a starting point. :)
(In reply to Matt from comment #4)
> (In reply to Mike Conley (:mconley) from comment #3)
> > This doesn't block shipping Australis by any means, but it might be a nice
> > inclusion. Willing to mentor if anybody wants to step up.
> 
> Mike, if you're still willing to mentor, I'm willing to take this on. I'd
> appreciate being pointed toward a starting point. :)

Hey Matt!

Sounds good. The first thing you need to do is get a local build of Firefox up - specifically, the UX branch, which can be cloned from http://hg.mozilla.org/projects/ux.

This document outlines the steps to build on Windows, OS X and Linux: https://developer.mozilla.org/en/docs/Simple_Firefox_build

Alternatively, we've been producing some videos to walk you through how it's done: http://codefirefox.com/

Once you've got a build of UX completed, what you'll want to look at is this file:

browser/components/customizableui/src/CustomizableWidgets.jsm

In this function, we define the default widgets that come with Firefox.

The API isn't fully documented yet (we're working on that right now!) so you'll need to look at the other widgets defined in that file for clues, and ask in here (or IRC in #fx-team - I'm mconley) for help.

The widget is very simple - it just needs to open this URL: 

about:sync-tabs

We might want to have the button disabled if the user is not set up to sync tabs, but we'll get there. Opening a tab from the widget can be done with this code:

win.BrowserOpenSyncTabs(); (where win is the window that sends the command - there are a few examples of how this is derived in CustomizableWidgets.jsm).

Finally, we might want to put the whole definition in CustomizableWidgets.jsm inside an #ifdef on MOZ_SERVICES_SYNC, since BrowserOpenSyncTabs() is not available if MOZ_SERVICES_SYNC is not defined (see https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6292).

Let me know if you have any questions how to proceed - and don't forget to check the "Need more information from" and put my email address in the box so that I don't forget to respond. :)

Thanks for helping out!
Assignee: nobody → hattmammerly
Thanks for all the info, Mike! Spent a little longer than I'd like to admit setting up a test environment, but I'm good to go now.

Button created in the file mconley mentioned. Label and tooltip set in browser/locales/en-US/chrome/browser/customizableUI/customizableWidgets.properties. I was referred in IRC to shorlander for getting an icon made. The id of the button on my end is currently 'tabs-from-other-devices-button'. Can I do anything for this?
Flags: needinfo?(shorlander)
Creates a button for viewing tabs from other devices in the customization panel. Checks once on browser startup if user has sync set up; if not, the button is not shown (if the user enables sync, the browser has to be restarted to show the button). Button is currently iconless.
Attachment #8334238 - Flags: review?(mconley)
Flags: needinfo?(shorlander)
Comment on attachment 8334238 [details] [diff] [review]
bug702005-tabsfromotherdevicesbutton.diff

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

This looks really good! Just two suggestions, but they're really small. I think this is going to work nicely. :)

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> +const SYNC_PREFS_BRANCH = "services.sync.";

I don't see much value to having this constant extracted, since (at least for now), it's only used in one place (and only if MOZ_SERVICES_SYNC is defined).

@@ +783,5 @@
> +if (prefs.prefHasUserValue("username")) {
> +  CustomizableWidgets.push({
> +    id: "tabs-from-other-devices-button",
> +    removable: true,
> +    defaultArea: CustomizableUI.AREA_PANEL,

I don't think we want to set a defaultArea on this one - it's not starting in the menu-panel anyways, since it's not part of the menu panel default set.
Attachment #8334238 - Flags: review?(mconley) → review-
I took the 'check if sync is set up' code from http://hg.mozilla.org/mozilla-central/file/f2adb62d07eb/services/sync/Weave.js in which that const is also defined and used once. I figured it might be convention or something, so I copied. I substituted it for the string in this patch.

No longer sets a defaultArea.

Thanks for all your help, Mike!
Attachment #8334272 - Flags: review?(mconley)
Comment on attachment 8334272 [details] [diff] [review]
bug702005-tabsfromotherdevicesbutton.diff r2

># HG changeset patch
># Parent 07091a2cb8a22dd826c145e8a3e130a4df9c36e5
>Bug 705002 - added 'Tabs from other devices' widget to customization panel
>
>diff --git a/browser/components/customizableui/src/CustomizableWidgets.jsm b/browser/components/customizableui/src/CustomizableWidgets.jsm
>--- a/browser/components/customizableui/src/CustomizableWidgets.jsm
>+++ b/browser/components/customizableui/src/CustomizableWidgets.jsm
>@@ -771,8 +772,28 @@ const CustomizableWidgets = [{
>           CustomizableUI.removeListener(listener);
>           let panel = aDoc.getElementById(kPanelId);
>           panel.removeEventListener("popupshowing", updateButton);
>         }
>       };
>       CustomizableUI.addListener(listener);
>     }
>   }];
>+
>+#ifdef MOZ_SERVICES_SYNC
>+let prefs = Services.prefs.getBranch("services.sync.");
>+if (prefs.prefHasUserValue("username")) {
>+  CustomizableWidgets.push({
>+    id: "tabs-from-other-devices-button",
>+    removable: true,
>+    defaultArea: CustomizableUI.AREA_PANEL,
>+    onCommand: function(aEvent) {
>+      let win = aEvent.target &&
>+                aEvent.target.ownerDocument &&
>+                aEvent.target.ownerDocument.defaultView;
>+      if (win) {
>+        win.BrowserOpenSyncTabs();
>+      }
>+    }
>+  });
>+}
>+#endif
>+
>diff --git a/browser/locales/en-US/chrome/browser/customizableui/customizableWidgets.properties b/browser/locales/en-US/chrome/browser/customizableui/customizableWidgets.properties
>--- a/browser/locales/en-US/chrome/browser/customizableui/customizableWidgets.properties
>+++ b/browser/locales/en-US/chrome/browser/customizableui/customizableWidgets.properties
>@@ -10,16 +10,19 @@ history-panelmenu.tooltiptext = Historyâ
> privatebrowsing-button.label = New Private Window
> # LOCALIZATION NOTE(privatebrowsing-button.tooltiptext): %S is the keyboard shortcut
> privatebrowsing-button.tooltiptext = Open a new Private Browsing window (%S)
> 
> save-page-button.label = Save Page
> # LOCALIZATION NOTE(save-page-button.tooltiptext): %S is the keyboard shortcut
> save-page-button.tooltiptext = Save this page (%S)
> 
>+tabs-from-other-devices-button.label = View Tabs from Other Devices
>+tabs-from-other-devices-button.tooltiptext = View tabs from other devices
>+
> find-button.label = Find
> # LOCALIZATION NOTE(find-button.tooltiptext): %S is the keyboard shortcut
> find-button.tooltiptext = Find in this page (%S)
> 
> open-file-button.label = Open File
> # LOCALIZATION NOTE(open-file-button.tooltiptext): %S is the keyboard shortcut
> open-file-button.tooltiptext = Open file (%S)
Comment on attachment 8334272 [details] [diff] [review]
bug702005-tabsfromotherdevicesbutton.diff r2

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

Just one more iteration, and I think this is good to go.

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +779,5 @@
>    }];
> +
> +#ifdef MOZ_SERVICES_SYNC
> +let prefs = Services.prefs.getBranch("services.sync.");
> +if (prefs.prefHasUserValue("username")) {

Actually, this part can be safely replace with:

if (Services.prefs.prefHasUserValue("services.sync.username")) {

@@ +783,5 @@
> +if (prefs.prefHasUserValue("username")) {
> +  CustomizableWidgets.push({
> +    id: "tabs-from-other-devices-button",
> +    removable: true,
> +    defaultArea: CustomizableUI.AREA_PANEL,

defaultArea is still here. :)
Attachment #8334272 - Flags: review?(mconley) → review-
Well that's embarrassing. I guess that's what reviews are for!
Sorry for wasting a bit of your time with my carelessness.
Attachment #8334238 - Attachment is obsolete: true
Attachment #8334272 - Attachment is obsolete: true
Attachment #8334864 - Flags: review?(mconley)
Comment on attachment 8334864 [details] [diff] [review]
bug702005-tabsfromotherdevicesbutton.diff r3

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

So this has r+ from me, but we're still going to need an icon and CSS to make the icon appear for the widget (for the toolbar, palette, and menu-panel).

This icon probably doesn't have high priority from shorlander, but I'll make sure it gets on his radar. If / when it shows up, we can finish this one off. Thanks Matt!
Attachment #8334864 - Flags: review?(mconley) → review+
Mentor: mconley
Whiteboard: [good first bug][mentor=mconley][lang=js] → [good first bug][lang=js]
Stephen/Michael, do we have an icon for this? I didn't realize this bug has sat idle for so long. It looks like neither of you were flagged for it.
Flags: needinfo?(shorlander)
Flags: needinfo?(mmaslaney)
Madhava, can this be forwarded to one of the contractors? How does that process work?
Flags: needinfo?(mmaslaney) → needinfo?(madhava)
Keywords: icon
Blocks: 996638
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fxsync]
Superseeded by bug 1201331, should we close this?
Yeah, bug 1201331 is taking this quite a bit further than the patches attached here, so I think closing this as a dupe is fine. :mathu, thanks for the patch but apologies for the delay and for having this languish to the point where it has become superseded :(
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: needinfo?(shorlander)
Flags: needinfo?(madhava)
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: