808215 - Disable social API in private windows (for per-window Private Browsing builds)

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

In bug 807217 we disabled social during private browsing.

We need to decide whether social will be available in per-window private browsing mode.  

If we do want to enable it, I think there are a couple directions to go:

#1 -------------------

Social is enabled, but separate from non-PB mode windows.

Get frameworker to support more than one worker per provider.  Any windows not in PB mode would use the global frameworker.  Any windows with PB enabled would have their own frameworkers.

Downside is added overhead in loading a social provider and the additional workers, per pb-mode-window.  Upside is being able to use a different account in PB-mode windows.

#2 ----------------------

Social is initially disabled (hidden) in the PB mode window. The user can "enable" Social for that window, and perhaps set a pref to always enable.

Upside, we can share the frameworker that is already loaded.   Downside, you end up using the same login as the non-PB mode windows.

#3 ---------------

We do not allow any socialapi functionality in private browsing windows.

Downside, my favorite discreet dating service cannot make a social provider I would use.  Yes I'm joking wink wink.

-------------

I prefer #3 for a couple reasons.  No surprise to the user entering private browsing.  Somewhat lower overhead.  I think this would be a simpler implementation given the current socialapi architecture.

Comment 1

[(no longer active)]

Let's do #3.

Are you interested in working on this, Shane?  :-)

Comment 2

[(no longer active)]

Can someone please explain what needs to be done here?  Maybe we can get help from the appcoast folks on this...

Thanks!

Comment 4

[(no longer active)]

ping?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Felipe and markh have been thinking about this some. We're in the process of re-working some of the UI for multiprovider work, so we'll need to keep that in mind.

Comment 6

[(no longer active)]

(In reply to comment #5)
> Felipe and markh have been thinking about this some. We're in the process of
> re-working some of the UI for multiprovider work, so we'll need to keep that in
> mind.

Cool, thanks!

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Attachment: WIP

This patch is a WIP, but it does disable social in both "per window PB" builds and normal builds when "global PB" is enabled.  Most tests pass apart from our existing PB tests (which assume it stays enabled).

It still needs to do something about activation (ie, display some UI that indicates social is unavailable) and some extra cleanup.  It will also go stale as soon as the multi-provider work lands, but that's OK.

Comment 8

[(no longer active)]

Comment on attachment 688630 [details] [diff] [review]
WIP

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

::: browser/base/content/browser-social.js
@@ +214,5 @@
> +    return window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                 .getInterface(Ci.nsIWebNavigation)
> +                 .QueryInterface(Ci.nsIDocShell)
> +                 .QueryInterface(Ci.nsILoadContext)
> +                 .usePrivateBrowsing;

Nit: please do this instead:

  return PrivateBrowsingUtils.isWindowPrivate(window);

::: toolkit/components/social/MozSocialAPI.jsm
@@ +15,5 @@
> +  return win.QueryInterface(Ci.nsIInterfaceRequestor)
> +               .getInterface(Ci.nsIWebNavigation)
> +               .QueryInterface(Ci.nsIDocShell)
> +               .QueryInterface(Ci.nsILoadContext)
> +               .usePrivateBrowsing;

I think you can just use isWindowPrivate instead of this function.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Attachment: updated, few tests

Updated to use PrivateBrowsingUtils, a first cut at a "sorry, can't activate in PB mode" panel if an activation is attempted, and some tests (that don't yet pass)

All feedback welcome!

Comment 10

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20121206 Firefox/20.0
Build ID: 20121206040203

I was doing some exploratory testing on the latest-birch build.
Social API has the following behavior if is enabled while in the New Private Window:
1. After enabling it while in the New Private Window, the sidebar is also enabled in the normal window.
2. some of the data leaks into the normal browsing cache
3. Social API is not fully functional in the new Private Window - the feature can be enabled by setting the preferences social.enabled and social.active to true in about:config but:
- the sidebar is enabled but hovering the mouse over the notifications doesn't bring out the flyouts 
- there is no contacts/friends in the sidebar
- only one button (the Facebook button) is displayed in the navigation bar - See All Friends Requests, See All Messages and See All Notifications buttons are missing.

At the moment, the Social API can be activated in the old Private Browsing mode. Will this be the case for the new Private Window also (if so, please let me know so that I could file follow up bugs for the above issues) or the feature will be indeed impossible to activate?

Comment 11

[(no longer active)]

Do you need feedback from me?

Comment 12

[Mark Hammond [:markh] [:mhammond]]

(In reply to Simona B [QA] from comment #10)
> At the moment, the Social API can be activated in the old Private Browsing
> mode. Will this be the case for the new Private Window also (if so, please
> let me know so that I could file follow up bugs for the above issues) or the
> feature will be indeed impossible to activate?

This bug is about completely disabling Social in PB windows - we may work to reactivate it later, but step 1 is to simply disable it.

(In reply to Ehsan Akhgari [:ehsan] from comment #11)
> Do you need feedback from me?

Not really, but it would be welcome regardless :)  I do think this is on a reasonable path though, so feel free to ignore it.

Comment 13

[(no longer active)]

Comment on attachment 689573 [details] [diff] [review]
updated, few tests

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

I think this patch is definitely on the right track.  I have some comments below which will hopefully help make it better.

Mark, when do you think we can get this landed?

::: browser/base/content/browser-social.js
@@ +1,5 @@
>  // This Source Code Form is subject to the terms of the Mozilla Public
>  // 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/.
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "PrivateBrowsingUtils", "resource://gre/modules/PrivateBrowsingUtils.jsm");

This is not needed, since this file is #included inside browser.js.

@@ +235,5 @@
> +    return Social.enabled && !PrivateBrowsingUtils.isWindowPrivate(window);
> +  },
> +
> +  get active() {
> +    return Social.active && !PrivateBrowsingUtils.isWindowPrivate(window);

Note that these will affect both global and per-window PB builds (which may or may not be what you intended here.)

::: browser/base/content/browser.xul
@@ +231,5 @@
> +        <button default="true"
> +                autofocus="autofocus"
> +                label="&social.ok.label;"
> +                accesskey="&social.ok.accesskey;"
> +                oncommand="SocialUI.notificationPanelPrivateBrowsing.hidePopup();"/>

Nit: weird indentation.

::: browser/base/content/test/Makefile.in
@@ +328,5 @@
>                  browser_bug767836.js \
>                  browser_save_link.js \
>                  browser_save_private_link.js \
>                  browser_tabMatchesInAwesomebar.js \
> +                browser_social_globalPB.js \

Nit: we don't usually rename the original test file in these cases...

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +397,5 @@
>  # LOCALIZATION NOTE (social.activated.description): %1$S is the name of the social provider, %2$S is brandShortName (e.g. Firefox)
>  social.activated.description=You've turned on %1$S for %2$S.
>  
> +# LOCALIZATION NOTE (social.activated.description): %1$S is the name of the social provider, %2$S is brandShortName (e.g. Firefox)
> +social.private-browsing-no-activation.description=%1$S for %2$S can not be enabled in private browsing windows.  Please try again from a regular window.

Nit: we call these windows "private windows" in the UI.  You can say "... is not available in private windows.  Please try again in a normal window." for example.

Comment 14

[Mark Hammond [:markh] [:mhammond]]

(In reply to Ehsan Akhgari [:ehsan] from comment #13)

> Mark, when do you think we can get this landed?

After the multi-provider patch lands I'll clean this up and put it up for review.  I'd expect later this week...

Comment 15

[(no longer active)]

(In reply to Mark Hammond (:markh) from comment #14)
> (In reply to Ehsan Akhgari [:ehsan] from comment #13)
> 
> > Mark, when do you think we can get this landed?
> 
> After the multi-provider patch lands I'll clean this up and put it up for
> review.  I'd expect later this week...

Any updates on this?

Comment 16

[Mark Hammond [:markh] [:mhammond]]

I'm hoping bug 821262 can land first - it lays much of the framework for this patch.

Comment 17

[(no longer active)]

Gentle re-ping...  (This is currently the only known outstanding issue for which we don't have a patch at hand...)

Comment 18

[Mark Hammond [:markh] [:mhammond]]

Attachment: More tests and on top of patches in bug 821262

Here is a much smaller patch, but on-top of the patches in bug 821262.  The 2 main issues remaining are:

* The panel to indicate we are refusing to activate isn't anchored to anything (as no social elements are in the toolbar).  We might just want a "notification bar" (or whatever it is called - the yellow box across the top of the window) instead of a regular panel?

* A totally bizarre problem in the activation tests - for some reason, when the tests send an activation event from the PB window, the event appears to have an origin of the system principal - but it works fine when sent from a non-PB window.  See the todo() call in browser_social_activation.js.  Note however the panel *does* work when activation is attempted for real (but see above - it's currently an unanchored panel, so isn't ideal).

Comment 19

[(no longer active)]

Comment on attachment 697813 [details] [diff] [review]
More tests and on top of patches in bug 821262

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

::: toolkit/components/social/MozSocialAPI.jsm
@@ +40,5 @@
>  // and then calls attachToWindow as appropriate
>  function injectController(doc, topic, data) {
>    try {
>      let window = doc.defaultView;
> +    if (!window || PrivateBrowsingUtils.isWindowPrivate(window))

Note that isWindowPrivate works in global PB builds as well (and will return true for all windows in PB mode.)  Not sure if this is what you expected or not.

Comment 20

[Mark Hammond [:markh] [:mhammond]]

Attachment: Updated based on most recent patches in bug 821262

Updated based on the recent patches and a couple more ifdef blocks which should keep the status-quo in non-perwindow PB builds.  While we probably want to disable social in *all* builds (ie, including "global PB" builds), it probably makes sense to do that in a followup so we can land this asap.

I'm off next week, so if Shane wants to take this on too, that would be awesome :)

Comment 21

[Mark Hammond [:markh] [:mhammond]]

Shane, please see comment 20.

Comment 23

[(no longer active)]

ping?

Comment 24

[Shane Caraveo (:mixedpuppy)]

(In reply to :Ehsan Akhgari from comment #23)
> ping?

this patch depends on the patch in bug 821262 landing first.

Comment 25

[Shane Caraveo (:mixedpuppy)]

Attachment: update to latest with fixes

This fixes _updateActiveUI to avoid showing social toolbarbutton and menus in a private window.

Comment 26

[Mark Hammond [:markh] [:mhammond]]

Attachment: Updated to not depend on bug 821262

Introduces the same "SocialUI.enabled" and some of the test code from bug 821262.

Comment 27

[Mark Hammond [:markh] [:mhammond]]

No longer depends on bug 821262.

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 705725 [details] [diff] [review]
Updated to not depend on bug 821262

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

Thanks for getting a smaller patch put together. I think we should land this on Nightly and then request uplift after waiting a week or so to make sure that there are no other regressions with it, but overall it seems pretty straightforward.

::: toolkit/components/social/MozSocialAPI.jsm
@@ +7,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "SocialService", "resource://gre/modules/SocialService.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "PrivateBrowsingUtils", "resource://gre/modules/PrivateBrowsingUtils.jsm");

Should this be wrapped by #ifndef ... #endif?

Comment 29

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/82464c35d37a

Comment 30

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #29)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/82464c35d37a

This patch introduces a bug resulting in the removal of the toolbarbutton for social when social is disabled.

Comment 31

[Mark Hammond [:markh] [:mhammond]]

Also had test failures on some platforms.

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/09ebdefa8c78

Comment 32

[Mark Hammond [:markh] [:mhammond]]

Attachment: As re-landed

Relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/b994afc368df
Try: https://tbpl.mozilla.org/?tree=Try&rev=236c6f3424b9

This patch has a fix for the "active" UI problem Shane reported (r=him for that) and I tweaked the new tests a little (re=me :)

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b994afc368df

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b994afc368df

Comment 35

[Shane Caraveo (:mixedpuppy)]

Ehsan, does this need uplift to 20, or is 21 good?

Comment 36

[(no longer active)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #35)
> Ehsan, does this need uplift to 20, or is 21 good?

It does need to go to Aurora.

(And thanks for the fix!)

Comment 37

[(no longer active)]

Mark, can you please nominate this for Aurora?  Thanks!

Comment 38

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 705798 [details] [diff] [review]
As re-landed

[Approval Request Comment]
Bug caused by (feature/regressing bug #): PBnGen
User impact if declined: Social in a private window is not private.
Testing completed (on m-c, etc.): Landed on M-C nearly 2 weeks ago.
Risk to taking this patch (and alternatives if risky): Small risk, social only
String or UUID changes made by this patch: None

Comment 39

[bhavana bajaj [:bajaj]]

(In reply to Simona B [QA] from comment #10)
> Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20121206 Firefox/20.0
> Build ID: 20121206040203
> 
> I was doing some exploratory testing on the latest-birch build.
> Social API has the following behavior if is enabled while in the New Private
> Window:
> 1. After enabling it while in the New Private Window, the sidebar is also
> enabled in the normal window.
> 2. some of the data leaks into the normal browsing cache
> 3. Social API is not fully functional in the new Private Window - the
> feature can be enabled by setting the preferences social.enabled and
> social.active to true in about:config but:
> - the sidebar is enabled but hovering the mouse over the notifications
> doesn't bring out the flyouts 
> - there is no contacts/friends in the sidebar
> - only one button (the Facebook button) is displayed in the navigation bar -
> See All Friends Requests, See All Messages and See All Notifications buttons
> are missing.
> 
> At the moment, the Social API can be activated in the old Private Browsing
> mode. Will this be the case for the new Private Window also (if so, please
> let me know so that I could file follow up bugs for the above issues) or the
> feature will be indeed impossible to activate?

Simona, can you please help with verification here & some exploratory testing around this ? Thanks !

Comment 40

[bhavana bajaj [:bajaj]]

Comment on attachment 705798 [details] [diff] [review]
As re-landed

Approving on Aurora as this is needed for per-window  private browsing.Its well baked on m-c and I have requested QA to help with exploratory testing.

Comment 41

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ad1989520816

To help with the QA:

(In reply to Simona B [QA] from comment #10)
> At the moment, the Social API can be activated in the old Private Browsing
> mode. Will this be the case for the new Private Window also (if so, please
> let me know so that I could file follow up bugs for the above issues) or the
> feature will be indeed impossible to activate?

The feature should be impossible to activate and no social UI should be visible in private windows.

Comment 42

[Simona Badau, Desktop QA]

(In reply to bhavana bajaj [:bajaj] from comment #39)
> Simona, can you please help with verification here & some exploratory
> testing around this ? Thanks !

Verified that it's impossible to activate the Social API on the latest Nightly and that the social UI is not visible in the Private Browsing Window.

Verified on Windows 7, Ubuntu 12.04 and Mac OS X 10.7.5:
Build ID: 20130205031033
Mozilla/5.0 (Windows NT 6.1; rv:21.0) Gecko/20130205 Firefox/21.0
Mozilla/5.0 (X11; Linux i686; rv:21.0) Gecko/20130205 Firefox/21.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:21.0) Gecko/20130205 Firefox/21.0

During the exploratory testing around Social API, I noticed that in normal mode the Facebook button's icon is different. I filed bug 838533 for this matter.

Comment 43

[u279076]

Thanks Simona. This is also landed in Firefox 20. Would you mind verifying in the latest Aurora builds as well?

Comment 44

[Simona Badau, Desktop QA]

Verified as fixed on the latest Aurora - Social API can't be activated and the social UI is not visible while in the Private Browsing Window.

Verified on Win 7, Ubuntu 12.04 and Mac OS X 10.7:
Build ID: 20130206042020
Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20130206 Firefox/20.0
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20130206 Firefox/20.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:20.0) Gecko/20130206 Firefox/20.0