Closed Bug 935640 Opened 11 years ago Closed 10 years ago

Fix UX for sidebar selection

Categories

(Firefox Graveyard :: SocialAPI, defect)

26 Branch
x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 29

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

(Keywords: uiwanted, Whiteboard: [Australis:P-])

Attachments

(12 files, 10 obsolete files)

912.67 KB, image/png
Details
1.24 KB, image/png
Details
1.23 KB, image/png
Details
41.29 KB, image/png
Details
39.80 KB, image/png
Details
58.56 KB, image/png
Details
64.69 KB, patch
mixedpuppy
: review+
Details | Diff | Splinter Review
6.15 KB, patch
mixedpuppy
: review+
Details | Diff | Splinter Review
33.85 KB, patch
mixedpuppy
: review+
Details | Diff | Splinter Review
63.87 KB, patch
mixedpuppy
: review+
Details | Diff | Splinter Review
6.21 KB, patch
mixedpuppy
: review+
Details | Diff | Splinter Review
35.15 KB, patch
mixedpuppy
: review+
Details | Diff | Splinter Review
We need to get rid of the old toolbar button, but need some UI to select what provider is shown in the sidebar.
Blocks: 889427
The ux with this is particularly annoying at this point, we should up the priority on figuring this out.
Pre-Australis, every service provider that offers a sidebar will also have a menu item in the Firefox toolbar that produces a panel (2a in attached).  From within this panel, service providers will be encouraged to allow their sidebar to be toggled on and off.

While Firefox already provides a menu item to change sidebars (2b in attached), this toolbar panel will be the main way service provider sidebars are managed.  When two sidebars are enabled at the same time, a gear menu (2c in attached) appears in the top right of the sidebar when users mouse over it, allowing them to switch sidebars.  This gear only appears when two sidebars are “visible”: not when two are possible.
(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #2)
> Created attachment 831143 [details]
> Mockup: Showing a gear menu to switch sidebars when multiple sidebars are
> enabled

Boriss, I'm a little surprised in this mockup that an icon is displayed above a corner of the sidebar's web page. How likely is that web page to have some UI in its top right corner?
@Boriss, #2a in the mockup is not going to be added right now, and I question how important it is.  It could be done in a follow up bug if it is indeed important.
Flags: needinfo?(jboriss)
Attached patch part 1: remove toolbar button (obsolete) — Splinter Review
This is part 1 of 3.  The bulk of the UX changes are included here which covers:

- removal of old style toolbarbutton
- removal of tools->provider name menu
- addition of sidebar providers to view->sidebar menu
- new menu that "hovers" over the upper right corner of the sidebar
Attachment #832569 - Flags: feedback?(mhammond)
Attachment #832569 - Flags: feedback?(felipc)
Attached patch part 2: fix activation (obsolete) — Splinter Review
part 2 fixes activation.  Since we no longer have a permanent toolbar button, and providers are not required to have a toolbar button, we do not have a fixed location to hang a post-install activation panel.  We now always show the pre-install enable panel when the activation button is clicked.
Attachment #832572 - Flags: feedback?(mhammond)
Attachment #832572 - Flags: feedback?(felipc)
Attached patch paret 3: fix tests (obsolete) — Splinter Review
part 3 fixes the tests, updating or removing those tests that were specific to the toolbar button.
Attachment #832573 - Flags: feedback?(mhammond)
Attachment #832573 - Flags: feedback?(felipc)
this is a preliminary try to see if there are any major issues in the changes, as well as to provide a binary for Boriss to run through the ux.
sigh.  sorry for the spam.  here's the actual try

https://tbpl.mozilla.org/?tree=Try&rev=50e468bd4e70
Depends on: 930641
Blocks: 894806
Comment on attachment 832569 [details] [diff] [review]
part 1: remove toolbar button

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

It's hard not to like a patch that removes so much code :)

::: browser/base/content/browser-menubar.inc
@@ -482,5 @@
> -                            command="Social:ToggleNotifications"
> -                            label="&social.toggleNotifications.label;"
> -                            accesskey="&social.toggleNotifications.accesskey;"/>
> -                  <menuitem id="menu_focusChatBar"
> -                            label="&social.chatBar.label;"

this menu item hasn't been "moved", but deleted - is that the intent?  If so, I guess the strings should also be deleted?

::: browser/base/content/browser-sets.inc
@@ -217,5 @@
>                   command="Tools:DevToolsConnect"/>
> -
> -    <!-- SocialAPI broadcasters -->
> -    <broadcaster id="socialBroadcaster_userDetails"
> -                 notLoggedInLabel="&social.notLoggedIn.label;"/>

ditto for this string

::: browser/base/content/browser-social.js
@@ +284,5 @@
>        notificationPanel.setAttribute("oldorigin", oldOrigin);
>  
>        // Show the panel
>        notificationPanel.hidden = false;
> +      // XXX what do we do now?

good question :)  What *do* we do now?

@@ +781,5 @@
>      evt.initCustomEvent(aEnabled ? "socialFrameShow" : "socialFrameHide", true, true, {});
>      sbrowser.contentDocument.documentElement.dispatchEvent(evt);
>    },
>  
> +  updateButton: function SocialToolbar_updateButton() {

do we need to do this work if social is disabled?  I'm thinking of, eg, startup when social is disabled, we seem to be doing work that isn't necessary (ie, updating the "checked" attribute of a hidden item, and populating the provider menus.

@@ +874,5 @@
>        sbrowser.loadURI("about:socialerror?mode=tryAgain&url=" + url, null, null);
>      }
> +  },
> +
> +  // these will be seperate from Social soon

"separate" - and I'm not sure what the comment means - are you trying to say "these will move out of browser-social soon"?

::: browser/themes/shared/social/chat.inc.css
@@ +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/. */
>  %endif
>  
> +#social-sidebar-box > hbox {

I'm skipping this part, as I understand from our IRC chat that you might be moving away from an "overlaid" button.
Attachment #832569 - Flags: feedback?(mhammond) → feedback+
Comment on attachment 832572 [details] [diff] [review]
part 2: fix activation

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

Again, gotta love lots of code removal :)  IIUC, this is removing our special social panel and replacing it with the generic addon installed notification?  If that's correct, could you please post a screenshot of what that looks like?
Attachment #832572 - Flags: feedback?(mhammond) → feedback+
Comment on attachment 832573 [details] [diff] [review]
paret 3: fix tests

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

Looks OK, although I'm concerned we are dropping some test coverage and I don't see new test coverage for the new button.

::: browser/base/content/test/social/browser_social_activation.js
@@ +12,5 @@
>    for (let tab of tabsToRemove)
>      gBrowser.removeTab(tab);
>    tabsToRemove = [];
>    // theses tests use the notification panel but don't bother waiting for it
>    // to fully open - the end result is that the panel might stay open

just nuke the comment and the line

@@ -262,5 @@
>        });
>      });
>    },
>  
> -  testRemoveNonCurrentProvider: function(next) {

why has this been removed?  I don't see how we are testing this case in the new code.

::: browser/base/content/test/social/browser_social_mozSocial_API.js
@@ -1,1 @@
> -/* This Source Code Form is subject to the terms of the Mozilla Public

in theory, this is testing the mozSocial API by ensuring, eg, social_panel.html can get a worker port and post messages using it - it seems tweaking the icons etc is just a vehicle for ensuring things worked.  Where is this functionality now tested once this has been removed?

::: browser/base/content/test/social/browser_social_status.js
@@ +49,5 @@
>        Services.prefs.clearUserPref("social.remote-install.enabled");
>        // just in case the tests failed, clear these here as well
>        Services.prefs.clearUserPref("social.allowMultipleWorkers");
>        Services.prefs.clearUserPref("social.whitelist");
>        

even though this patch doesn't add it, please kill this trailing space

::: toolkit/components/social/SocialService.jsm
@@ +856,5 @@
>      if (!this.ambientNotificationIcons[notification.name] &&
>          Object.keys(this.ambientNotificationIcons).length >= 3) {
>        throw new Error("ambient notification limit reached");
>      }
> +    dump("**** ambient "+JSON.stringify(notification)+"\n");

I doubt you meant to include this :)
Attachment #832573 - Flags: feedback?(mhammond)
(In reply to Mark Hammond [:markh] from comment #12)
> Comment on attachment 832569 [details] [diff] [review]
> part 1: remove toolbar button
> 
> Review of attachment 832569 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It's hard not to like a patch that removes so much code :)

I'd downright giddy!

> ::: browser/base/content/browser-menubar.inc
> @@ -482,5 @@
> > -                            command="Social:ToggleNotifications"
> > -                            label="&social.toggleNotifications.label;"
> > -                            accesskey="&social.toggleNotifications.accesskey;"/>
> > -                  <menuitem id="menu_focusChatBar"
> > -                            label="&social.chatBar.label;"
> 
> this menu item hasn't been "moved", but deleted - is that the intent?  If
> so, I guess the strings should also be deleted?

yeah, once we get to review stage I'll remove any unused strings.  I am wondering about whether/how to keep the menu_focusChatBar since I think that was used for keyboard navigation somehow.

> ::: browser/base/content/browser-social.js
> @@ +284,5 @@
> >        notificationPanel.setAttribute("oldorigin", oldOrigin);
> >  
> >        // Show the panel
> >        notificationPanel.hidden = false;
> > +      // XXX what do we do now?
> 
> good question :)  What *do* we do now?

:)  Actually, patch #2 addresses this.

> @@ +781,5 @@
> >      evt.initCustomEvent(aEnabled ? "socialFrameShow" : "socialFrameHide", true, true, {});
> >      sbrowser.contentDocument.documentElement.dispatchEvent(evt);
> >    },
> >  
> > +  updateButton: function SocialToolbar_updateButton() {
> 
> do we need to do this work if social is disabled?  I'm thinking of, eg,
> startup when social is disabled, we seem to be doing work that isn't
> necessary (ie, updating the "checked" attribute of a hidden item, and
> populating the provider menus.

I'll double check that, though right now the View->Sidebar->provider menu's are how you would re-activate social, but I don't think that is quite right.

> @@ +874,5 @@
> >        sbrowser.loadURI("about:socialerror?mode=tryAgain&url=" + url, null, null);
> >      }
> > +  },
> > +
> > +  // these will be seperate from Social soon
> 
> "separate" - and I'm not sure what the comment means - are you trying to say
> "these will move out of browser-social soon"?

No, I meant that "provider" will be moved into SocialSidebar rather than keeping Social.provider.  The sidebar is the only place now that we would end up using a selected provider, and rather than using a pref we can just persist the origin on the sidebar.

> ::: browser/themes/shared/social/chat.inc.css

> I'm skipping this part, as I understand from our IRC chat that you might be
> moving away from an "overlaid" button.

Sure, I'll go through that with Boriss soon as possible.
(In reply to Mark Hammond [:markh] from comment #13)
> Comment on attachment 832572 [details] [diff] [review]
> part 2: fix activation
> 
> Review of attachment 832572 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Again, gotta love lots of code removal :)  IIUC, this is removing our
> special social panel and replacing it with the generic addon installed
> notification?  If that's correct, could you please post a screenshot of what
> that looks like?

You can see it in action already (without this patch) by activating my demo provider at http://mixedpuppy.github.io/socialapi-demo/

But I'll attach a screenshot anyway.
Attached image serviceactivation.png
This is the "pre-activation" panel that will be the only panel for services now (patch #2 is removing the second "post-activation" panel).
(In reply to Mark Hammond [:markh] from comment #14)
> Comment on attachment 832573 [details] [diff] [review]
> paret 3: fix tests
> 
> Review of attachment 832573 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks OK, although I'm concerned we are dropping some test coverage and I
> don't see new test coverage for the new button.

browser_social_status.js has the tests for the status button, which is the replacement for the toolbarbutton.   As well, there are still tests that look at the menus which are now located under View-Sidebar and on the gear button that currently hovers over the sidebar.

> ::: browser/base/content/test/social/browser_social_activation.js
> @@ +12,5 @@
> >    for (let tab of tabsToRemove)
> >      gBrowser.removeTab(tab);
> >    tabsToRemove = [];
> >    // theses tests use the notification panel but don't bother waiting for it
> >    // to fully open - the end result is that the panel might stay open
> 
> just nuke the comment and the line
> 
> @@ -262,5 @@
> >        });
> >      });
> >    },
> >  
> > -  testRemoveNonCurrentProvider: function(next) {
> 
> why has this been removed?  I don't see how we are testing this case in the
> new code.

That test relied on a) "undo" functionality of the post-activation panel that is removed in patch #2, and b) that the post-activation panel is shown after the provider is installed/enabled.  With these changes, neither case happens.  Providers are removed via the addon manager.  Given that, you cannot end up in the situation that this test was addressing.  There are other tests throughout the social tests that address removal of providers.

> ::: browser/base/content/test/social/browser_social_mozSocial_API.js
> @@ -1,1 @@
> > -/* This Source Code Form is subject to the terms of the Mozilla Public
> 
> in theory, this is testing the mozSocial API by ensuring, eg,
> social_panel.html can get a worker port and post messages using it - it
> seems tweaking the icons etc is just a vehicle for ensuring things worked. 
> Where is this functionality now tested once this has been removed?

testStatusPanel in browser_social_status.js would break if social_panel.html is unable to get a port.
Blocks: 940154
Whiteboard: [Australis:P-]
Comment on attachment 832569 [details] [diff] [review]
part 1: remove toolbar button

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

::: browser/base/content/browser-social.js
@@ -206,5 @@
>      // social being "active" (but also chromeless/PB)
>      let enabled = Social.providers.length > 0 && !this._chromeless &&
>                    !PrivateBrowsingUtils.isWindowPrivate(window);
>      let broadcaster = document.getElementById("socialActiveBroadcaster");
>      broadcaster.hidden = !enabled;

can this broadcaster be removed too?

@@ -838,5 @@
> -  // Called once, after window load, when the Social.provider object is
> -  // initialized.
> -  get _dynamicResizer() {
> -    delete this._dynamicResizer;
> -    this._dynamicResizer = new DynamicResizeWatcher();

IIUC DynamicResizeWatcher is staying for SocialShare, right?
Attachment #832569 - Flags: feedback?(felipc) → feedback+
Comment on attachment 832572 [details] [diff] [review]
part 2: fix activation

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

::: browser/base/content/browser-social.js
@@ +266,1 @@
>      }.bind(this));

no need for the .bind anymore

::: toolkit/components/social/SocialService.jsm
@@ +655,5 @@
>          break;
>      }
> +
> +    installer = new AddonInstaller(sourceURI, manifest, installCallback);
> +    this._showInstallNotification(aDOMDocument, installer);

moving this to outside the switch statement is strange. It looks like we'll proceed installation even if there was an error, e.g. entering the default branch.
Attachment #832572 - Flags: feedback?(felipc) → feedback+
Comment on attachment 832573 [details] [diff] [review]
paret 3: fix tests

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

Just skimmed over the test changes. My main concern is the same from Mark about removing the mozSocial API test
Attachment #832573 - Flags: feedback?(felipc) → feedback+
(In reply to :Felipe Gomes from comment #22)
> Comment on attachment 832569 [details] [diff] [review]
> part 1: remove toolbar button
> 
> Review of attachment 832569 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-social.js
> @@ -206,5 @@
> >      // social being "active" (but also chromeless/PB)
> >      let enabled = Social.providers.length > 0 && !this._chromeless &&
> >                    !PrivateBrowsingUtils.isWindowPrivate(window);
> >      let broadcaster = document.getElementById("socialActiveBroadcaster");
> >      broadcaster.hidden = !enabled;
> 
> can this broadcaster be removed too?

yes, next patch it will be.

> @@ -838,5 @@
> > -  // Called once, after window load, when the Social.provider object is
> > -  // initialized.
> > -  get _dynamicResizer() {
> > -    delete this._dynamicResizer;
> > -    this._dynamicResizer = new DynamicResizeWatcher();
> 
> IIUC DynamicResizeWatcher is staying for SocialShare, right?

yes
(In reply to :Felipe Gomes from comment #24)
> Comment on attachment 832573 [details] [diff] [review]
> paret 3: fix tests
> 
> Review of attachment 832573 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just skimmed over the test changes. My main concern is the same from Mark
> about removing the mozSocial API test

Ambient notifications are now tested in browser_social_status.js.  The messages we look at here are used in multiple tests elsewhere.  The check that we do not remember the url in history is handled on every test file in head.js.  I don't see anything here that isn't redundant multiple times via other tests.
Attached image sidebar-header.png
alternative proposal for sidebar ui
Attached image sidebar-menu.png
alternative proposal for sidebar ui

The original mockup places the gear icon over the provider content.  It is awkward because it easily conflicts with ui in the content area, even with simple mockups of services there I easily get conflicts.  This proposal copies the header from the left sidebar.  

I haven't entirely tweaked the css for the ui yet, so some padding/margins are not correct, but it is enough to illustrate this approach.
Flags: needinfo?(jboriss)
Flags: needinfo?(jboriss)
After a short irc discussion with Boriss, we decided to go forward with the sidebar header as shown in the last couple images I posted.  I've cleaned up css from what the images show.
Assignee: nobody → mixedpuppy
Attachment #832569 - Attachment is obsolete: true
Attachment #8340713 - Flags: feedback?(mhammond)
Attachment #8340713 - Flags: feedback?(felipc)
Flags: needinfo?(jboriss)
Attached patch part 2: fix activation (obsolete) — Splinter Review
refresh of this patch
Attachment #832572 - Attachment is obsolete: true
Attachment #8340714 - Flags: feedback?(mhammond)
Attachment #8340714 - Flags: feedback?(felipc)
Attached patch part 3: fix tests (obsolete) — Splinter Review
biggest change here is a fix to browser_social_multiprovider.js to handle the menu only being populated on popupshowing.
Attachment #832573 - Attachment is obsolete: true
Attachment #8340715 - Flags: feedback?(mhammond)
Attachment #8340715 - Flags: feedback?(felipc)
(In reply to Shane Caraveo (:mixedpuppy) from comment #29)
> Created attachment 8340713 [details] [diff] [review]
> part 1: remove toolbar button, add sidebar header w/control
> 
> After a short irc discussion with Boriss, we decided to go forward with the
> sidebar header as shown in the last couple images I posted.  I've cleaned up
> css from what the images show.

Just a further comment.  The provider menus in the sidebar and in View->Sidebars are only populated when the menu is shown.  Then we simply update the checked state if the provider is changed.  When the provider list changes, we clear the providers from the menu so it will be re-populated on the next popupshown.  Providers that are enabled in the addons manager will (if they have a sidebar) always show up in these menus.
Comment on attachment 8340713 [details] [diff] [review]
part 1: remove toolbar button, add sidebar header w/control

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

This looks fine to me (although I focused only on the code, not the new UI).

What is the plan re the backout branch of Australis?  ie, what happens if this lands, but Australis gets stuck on nightly for a few cycles?

::: browser/base/content/browser-social.js
@@ +953,5 @@
> +  },
> +
> +  clearProviderMenus: function() {
> +    // called when there is a change in the provider list, we clear all menus
> +    // first, then if the sidebar is visible, we populate that.

this comment seems misleading - the "then if the sidebar is visible, we populate that." doesn't seem to apply to this function.
Attachment #8340713 - Flags: feedback?(mhammond) → feedback+
Attachment #8340714 - Flags: feedback?(mhammond) → feedback+
Comment on attachment 8340715 [details] [diff] [review]
part 3: fix tests

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

::: browser/base/content/test/social/browser_social_activation.js
@@ -262,5 @@
>        });
>      });
>    },
>  
> -  testRemoveNonCurrentProvider: function(next) {

why is this test no longer necessary?

::: browser/base/content/test/social/browser_social_multiprovider.js
@@ +41,5 @@
>  
> +    // the menu is not populated until onpopupshowing, so wait for popupshown
> +    function theTest() {
> +      checkProviderMenu(gProviders[0]);
> +  

whitespace

::: toolkit/components/social/SocialService.jsm
@@ +858,5 @@
>      if (!this.ambientNotificationIcons[notification.name] &&
>          Object.keys(this.ambientNotificationIcons).length >= 3) {
>        throw new Error("ambient notification limit reached");
>      }
> +    dump("**** ambient "+JSON.stringify(notification)+"\n");

debug cruft
Attachment #8340715 - Flags: feedback?(mhammond) → feedback+
> What is the plan re the backout branch of Australis?  ie, what happens if
> this lands, but Australis gets stuck on nightly for a few cycles?

This patch actually is not an australis patch, it should apply fine to either though I'll be testing on holly.

> ::: browser/base/content/browser-social.js
> @@ +953,5 @@
> > +  },
> > +
> > +  clearProviderMenus: function() {
> > +    // called when there is a change in the provider list, we clear all menus
> > +    // first, then if the sidebar is visible, we populate that.
> 
> this comment seems misleading - the "then if the sidebar is visible, we
> populate that." doesn't seem to apply to this function.

Right, it's more a comment on when we re-populate the menu, but it's not necessary to describe there.
(In reply to Mark Hammond [:markh] from comment #35)
> ::: browser/base/content/test/social/browser_social_activation.js
> @@ -262,5 @@
> >        });
> >      });
> >    },
> >  
> > -  testRemoveNonCurrentProvider: function(next) {
> 
> why is this test no longer necessary?

answered in comment #18, but here it is again:

That test relied on a) "undo" functionality of the post-activation panel that is removed in patch #2, and b) that the post-activation panel is shown after the provider is installed/enabled.  With these changes, neither case happens.  Providers are removed via the addon manager.  Given that, you cannot end up in the situation that this test was addressing.  There are other tests throughout the social tests that address removal of providers.
part 1 patch on top of holly branch
Attachment #8344172 - Flags: review?(mhammond)
Attached patch holly-part 2: fix activation (obsolete) — Splinter Review
part 2 on top of holly
Attachment #8344173 - Flags: review?(mhammond)
Attached patch holly-part 3: fix tests (obsolete) — Splinter Review
part 3 on top of holly

The australis version of patches wouldn't apply due to other changes in some files.

try for holly: https://tbpl.mozilla.org/?tree=Try&rev=33f23cbae418
Attachment #8344174 - Flags: review?(mhammond)
Attachment #8344172 - Attachment is patch: true
Comment on attachment 8344172 [details] [diff] [review]
holly-part 1: remove toolbar button, add sidebar header w/control

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

I'm fine with this, although I think it's worth mconley or jaws signing off on the general strategy (ie, of landing two different versions of the same basic patch on 2 trees).  If you've already chatted with them on IRC or similar, then great!  But otherwise, please ping them.
Attachment #8344172 - Flags: review?(mhammond) → review+
Attachment #8344173 - Flags: review?(mhammond) → review+
Comment on attachment 8344174 [details] [diff] [review]
holly-part 3: fix tests

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

::: browser/base/content/test/social/browser_social_multiprovider.js
@@ +41,5 @@
>  
> +    // the menu is not populated until onpopupshowing, so wait for popupshown
> +    function theTest() {
> +      checkProviderMenu(gProviders[0]);
> +  

trailing spaces here and below

::: browser/base/content/test/social/browser_social_status.js
@@ +49,5 @@
>        Services.prefs.clearUserPref("social.remote-install.enabled");
>        // just in case the tests failed, clear these here as well
>        Services.prefs.clearUserPref("social.allowMultipleWorkers");
>        Services.prefs.clearUserPref("social.whitelist");
>        

trailing space here that might as well be removed while we are touching this
Attachment #8344174 - Flags: review?(mhammond) → review+
(In reply to Mark Hammond [:markh] from comment #41)
> Comment on attachment 8344172 [details] [diff] [review]
> holly-part 1: remove toolbar button, add sidebar header w/control
> 
> Review of attachment 8344172 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm fine with this, although I think it's worth mconley or jaws signing off
> on the general strategy (ie, of landing two different versions of the same
> basic patch on 2 trees).  If you've already chatted with them on IRC or
> similar, then great!  But otherwise, please ping them.

mconley and/or jaws:

The holly patches in this bug is not all that different from the australis patch.  Most of the difference is about slight changes in files that are being patched.  For all to many reasons this missed 28, though if possible I'd like to uplift, which is a big reason I created the holly patch.  I'm good with proceeding in whatever way you prefer.
Flags: needinfo?(mconley)
Flags: needinfo?(jaws)
Yeah, creating two sets of patches is the right approach here. After you land the mozilla-central patches, go ahead and land your r+'d Holly patches on Holly, and then request Aurora approval on them.
Flags: needinfo?(mconley)
Flags: needinfo?(jaws)
You will need to make sure to put Australis in the changeset of the Australis only patches so they get backed out in the process of merging to Holly. You'll also want to land you Holly patches on Holly and ask for the Holly version to get Aurora-approval (not the m-c one) but that may have already been clear :)
Attachment #8340713 - Flags: feedback?(felipc) → review?(mhammond)
Attachment #8340714 - Flags: feedback?(felipc) → review?(mhammond)
Attachment #8340715 - Flags: feedback?(felipc) → review?(mhammond)
Attached patch holly-part 3: fix tests (obsolete) — Splinter Review
whitespace fixes
Attachment #8345610 - Flags: review+
Blocks: 948746
Comment on attachment 8340713 [details] [diff] [review]
part 1: remove toolbar button, add sidebar header w/control

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

::: browser/base/content/browser-social.js
@@ +953,5 @@
> +  },
> +
> +  clearProviderMenus: function() {
> +    // called when there is a change in the provider list, we clear all menus
> +    // first, then if the sidebar is visible, we populate that.

you said you would remove this comment?
Attachment #8340713 - Flags: review?(mhammond) → review+
Attachment #8340714 - Flags: review?(mhammond) → review+
Comment on attachment 8340715 [details] [diff] [review]
part 3: fix tests

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

please make sure you also address the comments in my previous feedback.

::: browser/base/content/test/social/browser_social_activation.js
@@ +13,5 @@
>      gBrowser.removeTab(tab);
>    tabsToRemove = [];
>    // theses tests use the notification panel but don't bother waiting for it
>    // to fully open - the end result is that the panel might stay open
> +  //SocialUI.activationPanel.hidePopup();

why is this commented out?  It looks like it shouldn't
Attachment #8340715 - Flags: review?(mhammond) → review+
updated with comments and pushed
Attachment #8340713 - Attachment is obsolete: true
Attachment #8348529 - Flags: review+
updated patch on latest pull, pushed
Attachment #8340714 - Attachment is obsolete: true
Attachment #8348530 - Flags: review+
updated on latest pull and comments, pushed
Attachment #8340715 - Attachment is obsolete: true
Attachment #8348532 - Flags: review+
updated on top of latest pull
Attachment #8344172 - Attachment is obsolete: true
Attachment #8348536 - Flags: review+
updated on top of latest pull
Attachment #8344173 - Attachment is obsolete: true
Attachment #8348538 - Flags: review+
updated on top of latest pull
Attachment #8344174 - Attachment is obsolete: true
Attachment #8345610 - Attachment is obsolete: true
Attachment #8348539 - Flags: review+
Depends on: 956956
Depends on: 960198
Verified as fixed on latest Aurora (build ID: 20140310004003).
The UI is implemented accordingly to the attached mock-ups.

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0.
Status: RESOLVED → VERIFIED
Depends on: 1269746
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: