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)
Firefox
Toolbars and Customization
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)
(deleted),
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•13 years ago
|
Updated•13 years ago
|
Component: Firefox Sync: UI → Toolbars
Product: Mozilla Services → Firefox
QA Contact: sync-ui → toolbars
Version: unspecified → Trunk
Updated•13 years ago
|
Updated•13 years ago
|
Severity: normal → enhancement
Comment 3•11 years ago
|
||
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]
Assignee | ||
Comment 4•11 years ago
|
||
(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. :)
Comment 5•11 years ago
|
||
(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
Assignee | ||
Comment 6•11 years ago
|
||
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)
Assignee | ||
Comment 7•11 years ago
|
||
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 8•11 years ago
|
||
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-
Assignee | ||
Comment 9•11 years ago
|
||
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)
Assignee | ||
Comment 10•11 years ago
|
||
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 11•11 years ago
|
||
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-
Assignee | ||
Comment 12•11 years ago
|
||
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 13•11 years ago
|
||
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+
Updated•10 years ago
|
Mentor: mconley
Whiteboard: [good first bug][mentor=mconley][lang=js] → [good first bug][lang=js]
Comment 14•10 years ago
|
||
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)
Comment 15•9 years ago
|
||
Madhava, can this be forwarded to one of the contractors? How does that process work?
Flags: needinfo?(mmaslaney) → needinfo?(madhava)
Keywords: icon
Updated•9 years ago
|
Blocks: 996638
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fxsync]
Comment 16•9 years ago
|
||
Superseeded by bug 1201331, should we close this?
Comment 17•9 years ago
|
||
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.
Description
•