818675 - support a full share panel

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

Attachment: share.png

a simple recommend button is not sufficient for all use cases.  In particular it is not enough to support privacy config even for facebook.  This is a quick brain dump outlining this feature, along with images and prototype code.

The definition of this feature is pending agreement in product and ux.

goals of our share panel are simple; 

we provide an iframe in a panel that allows a social provider to load their own share ui.  This works essentially like all the other social panels

we send opengraph data to the provider when the user clicks on share.  this allows the provider to so some work without fetching and parsing a webpage somewhere.  we do not send actual images (on the page or otherwise generated views of the page) since that could cause privacy issues.

we do not require the share page to use any other socialapi functionality.  We do this so we can support oexchange share endpoints.

we support three use cases:

- vendor share ui is in a panel
- vendor share ui is in the sidebar, button is "one-click" which sends opengraph data to the sidebar
- vendor has one-click no-ui recommend support (what we already have in socialapi)

We do not support more than one of the above use-case at a time.

We may support quick-switching the panel to a different share provider.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: share prototype

working prototype that includes facebook share per image attachment.

Comment 2

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

(In reply to Shane Caraveo (:mixedpuppy) from comment #0)
> we support three use cases:
> 
> - vendor share ui is in a panel
> - vendor share ui is in the sidebar, button is "one-click" which sends
> opengraph data to the sidebar
> - vendor has one-click no-ui recommend support (what we already have in
> socialapi)

Do we really need to support all 3 use-cases?  IMO, it would be best to only support "share ui in a panel".

* re the sidebar: this would probably offer a jarring experience for the user - the sidebar may be closed so we should close it when done - but how do we know when "done" is?  Do we need to wait a few seconds after "done" so a status message can be seen?

* one-click no-ui: doesn't seem to be in the best interests of our users, who may expect some confirmation of what is going to happen and/or be able to set privacy information.  The fact is doesn't even work well for facebook makes me wonder what real use-cases there are.

> 
> We do not support more than one of the above use-case at a time.
> 
> We may support quick-switching the panel to a different share provider.

This too doesn't seem to support those other 2 use-cases well either: swapping out the sidebar seems painful and one-click might lead to the user accidentally sharing with the wrong provider if they don't see any UI.

Comment 3

[edA-qa mort-ora-y]

Currently we use the one-click support to open a panel in the sidebar, so for  us that one-click support is not really needed. I also agree that most users will expect some kind of confirmation (one reason why we show a panel in the sidebar).

The share panel would be a good thing and I am also going to mockup this behaviour using the current UI (using openPanel) in our provider.

Comment 4

[Shane Caraveo (:mixedpuppy)]

(In reply to edA-qa mort-ora-y from comment #3)
> Currently we use the one-click support to open a panel in the sidebar, so
> for  us that one-click support is not really needed. I also agree that most
> users will expect some kind of confirmation (one reason why we show a panel
> in the sidebar).
> 
> The share panel would be a good thing and I am also going to mockup this
> behaviour using the current UI (using openPanel) in our provider.

what do you expect to happen if the sidebar is closed?

Comment 5

[edA-qa mort-ora-y]

We have two behaviours we'd like to compare:

1. A Share Panel opens when you click the button. This would be a floating panel, thus the state of the sidebar is not initially relevant.

2. The sidebar shows a share panel. In this case we'd expect it to open if it were closed.

For point 1 I think we may also have a button somewhere in that share panel which would open up the sidebar for extended information. I would use this since extended navigation in the popup panel seems unnatural and might be difficult to control.

Comment 6

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #2)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #0)
> > we support three use cases:
> > 
> > - vendor share ui is in a panel
> > - vendor share ui is in the sidebar, button is "one-click" which sends
> > opengraph data to the sidebar
> > - vendor has one-click no-ui recommend support (what we already have in
> > socialapi)
> 
> Do we really need to support all 3 use-cases?  IMO, it would be best to only
> support "share ui in a panel".

not saying we must, those are the use cases that are in play right now.  I don't feel it is a great idea to have a bunch of different things happen from one ui element, but we can explore the idea and see what sticks.

> * re the sidebar: this would probably offer a jarring experience for the
> user - the sidebar may be closed so we should close it when done - but how
> do we know when "done" is?  Do we need to wait a few seconds after "done" so
> a status message can be seen?

I think there is a strong use case in a mechanism that supports recommendation systems in the sidebar as a browsing aid.  There is more than one provider working on features that lean in that direction.  Figuring out a way to support that, if possible, is what I am thinking about here.

I don't think it is a given that the sidebar would need to be closed again if it were opened based on user action.  

> * one-click no-ui: doesn't seem to be in the best interests of our users,
> who may expect some confirmation of what is going to happen and/or be able
> to set privacy information.  The fact is doesn't even work well for facebook
> makes me wonder what real use-cases there are.

I never cared much for the implementation because it always appeared to "do nothing".  I'm not certain that rules out the usefulness of a one-click system, but we certainly didn't get it right.

> > We do not support more than one of the above use-case at a time.
> > 
> > We may support quick-switching the panel to a different share provider.
> 
> This too doesn't seem to support those other 2 use-cases well either:
> swapping out the sidebar seems painful and one-click might lead to the user
> accidentally sharing with the wrong provider if they don't see any UI.

That is just about a share panel, not about the other use cases.

Comment 7

[Shane Caraveo (:mixedpuppy)]

The prototype has changes in the api:

- support for share url in manifest as well as via worker message
- send share panel config via user-recommend message, or create a new message
- possibly remove recommend support
- new dom event sent to share panel with OpenGraph data

This of course is in need of refinement.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Attachment: v2

this prototype removes "recommend" functionality completely and adds a panel/iframe that loads a share endpoint.  The share endpoint is defined in the manifest.  We only have share with the currently selected provider.  No fancy footwork trying to enforce "unshare" or keep track of shared state, some of that can come back later if this route is choosen.

Comment 9

[Asa Dotzler [:asa]]

Shane, can you provide screenshots of v2? Or does it still look like share.png in the attachments?

Comment 10

[Shane Caraveo (:mixedpuppy)]

(In reply to Asa Dotzler [:asa] from comment #9)
> Shane, can you provide screenshots of v2? Or does it still look like
> share.png in the attachments?

It looks the same as the current attachment.  In v2 the implementation changed, removing the old one-click recommend functionality and only providing a share panel as shown in that image.

Comment 11

[Shane Caraveo (:mixedpuppy)]

Attachment: share-multiprovider.1.jpg

Comment 12

[Shane Caraveo (:mixedpuppy)]

Attachment: share-multiprovider.2.jpg

Comment 13

[Shane Caraveo (:mixedpuppy)]

Attachment: share-multiprovider.3.jpg

Comment 14

[Shane Caraveo (:mixedpuppy)]

I just wanted to explore this idea, and it was pretty easy to do.  ugly but works, pretty can come later if we go this route.  

The first time you open the share panel, you get a "more" button at the bottom (attachment 696397 [details]).  Clicking on that presents a toolbar with provider icons (attachment 696398 [details]).  The third image is showing that I've selected a different provider to share with (attachment 696399 [details]).

When you re-open the share panel, the default social provider is reselected.

The nice thing about this is that we could support share-only providers, and the providers only have to support an oexchange style endpoint, no other socialapi stuff.

Comment 15

[Asa Dotzler [:asa]]

Comment on attachment 688944 [details]
share.png

OK. I think that sounds good.

Comment 16

[Shane Caraveo (:mixedpuppy)]

Attachment: share v3

Moved a lot of tests into the patch, I think this is ready for some feedback.  

Some background.

This share panel replaces the recommend button.

We use a panel for share, much like the status panels.  A share panel is not required to use anything from the socialapi (e.g. worker not necessary, though the tests use one).  We do two things to help the provider complete a share; we create a url from a template to load the panel (oexchange style) and second, for more advanced use, we send a DOM event to the panel after load that has data we have parsed from the page being shared.  

Share panels are expected to self-close, though we do close the panel if it unloads, which makes simple oexchange style endpoints mostly usable.

Optionally, and this may get removed, and at least needs UX, there is a "more" button on the share panel, which if clicked, gives the user all available share choices and allows them to switch between them.  If you would like an addon with a bunch of share endpoints, ping me.

From a privacy/security perspective, we need to think about whether giving urls of images to the share provider is an issue.  We do that to simplify the creation of an image picker.  We do not send images, just the urls.

Socal.jsm has a new class, OpenGraphBuilder, based on similar code from F1, and much of the tests are from that as well.  That class could move to its own module if that would be preferred.

Comment 17

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

Comment on attachment 698980 [details] [diff] [review]
share v3

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

I'm not sure if we should be including all of the page parsing that the OpenGraphBuilder is trying to do.

::: browser/base/content/browser-context.inc
@@ +184,5 @@
>        <menuseparator id="context-sep-stop"/>
> +      <menuitem id="context-sharepage"
> +                label="&sharePageCmd.label;"
> +                accesskey="&sharePageCmd.accesskey;"
> +                command="Social:SharePage"/>

I think this should go adjacent to the "Email Image..." menuitem.

::: browser/base/content/browser-social.js
@@ -622,3 @@
>    },
>  
> -  unsharePage: function SSB_unsharePage() {

How will users unshare a page?

@@ +665,5 @@
> +    // containing the open graph data.
> +    this._createFrame();
> +    let iframe = this.panel.firstChild;
> +    let og = new OpenGraphBuilder(gBrowser);
> +    let pageData = og.getData();

This should be done off the main thread. I can imagine some scenarios where getData may take too long.

::: browser/base/content/test/social/browser_share.js
@@ +49,5 @@
> +  {
> +    url: baseURL+"corpus/shorturl_link.html",
> +    options: {
> +      previews: ["http://farm5.static.flickr.com/4141/5411147304_9f3996ba27_m.jpg"],
> +      url: "http://www.flickr.com/photos/mixedpuppy/5411147304/",

These URLs probably aren't good for committing :)

::: browser/modules/Social.jsm
@@ +19,5 @@
> +XPCOMUtils.defineLazyServiceGetter(this, "unescapeService",
> +                                   "@mozilla.org/feed-unescapehtml;1",
> +                                   "nsIScriptableUnescapeHTML");
> +
> +function validateURL(url) {

It's not clear from the name of this function what it validates a URL against. Its only caller is OpenGraphBuilder._validURL, which is also poorly named. These two functions can get merged and a better name should be given.

@@ +20,5 @@
> +                                   "@mozilla.org/feed-unescapehtml;1",
> +                                   "nsIScriptableUnescapeHTML");
> +
> +function validateURL(url) {
> +  if (!/^(https?|ftps?):\/\/\w+(\.\w+)*(:\d+)?(\/.*)?$/.test(url)) return null;

Should probably be using nsIURI to check whatever you need here, instead of this regular expression.

@@ +261,5 @@
> +    };
> +  },
> +
> +  getPageTitle: function() {
> +    let metas = this.browser.contentDocument.querySelectorAll("meta[property='og:title']"), i, title, content;

It would be good to cache the reference to contentDocument since it is used often and traversing 'this.browser' each time isn't necessary.

Also, I would prefer to have each variable declared on its own line as close to its first use as possible. It was easy to not see |i, title, content| at the end here.

@@ +262,5 @@
> +  },
> +
> +  getPageTitle: function() {
> +    let metas = this.browser.contentDocument.querySelectorAll("meta[property='og:title']"), i, title, content;
> +    for (i = 0; i < metas.length; i++) {

for (let meta of metas) {

@@ +460,5 @@
> +    if (host && /^maps\.google\.[a-zA-Z]{2,5}/.test(host)) {
> +       return this._validURL(this.browser.contentDocument.getElementById("link").getAttribute("href"));
> +    }
> +
> +    return '';

nit: other places within this file use |return "";|

Comment 18

[Shane Caraveo (:mixedpuppy)]

(In reply to Jared Wein [:jaws] from comment #17)
> Comment on attachment 698980 [details] [diff] [review]
> share v3
> 
> Review of attachment 698980 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure if we should be including all of the page parsing that the
> OpenGraphBuilder is trying to do.

What is in there is based on quite a bit of time/experience making share work in F1, however this system is a bit different, so some of that can probably be scrubbed.  I'll go over it again and do some cleanup (including other comments you made on it below).

> ::: browser/base/content/browser-social.js
> @@ -622,3 @@
> >    },
> >  
> > -  unsharePage: function SSB_unsharePage() {
> 
> How will users unshare a page?

Through the providers website.  As well, a provider can see that a user previously shared a page, and can offer different options at that point.

> @@ +665,5 @@
> > +    // containing the open graph data.
> > +    this._createFrame();
> > +    let iframe = this.panel.firstChild;
> > +    let og = new OpenGraphBuilder(gBrowser);
> > +    let pageData = og.getData();
> 
> This should be done off the main thread. I can imagine some scenarios where
> getData may take too long.

Agreed.  Is there any good example of doing this type of document scanning off the main thread?


> ::: browser/base/content/test/social/browser_share.js
> @@ +49,5 @@
> > +  {
> > +    url: baseURL+"corpus/shorturl_link.html",
> > +    options: {
> > +      previews: ["http://farm5.static.flickr.com/4141/5411147304_9f3996ba27_m.jpg"],
> > +      url: "http://www.flickr.com/photos/mixedpuppy/5411147304/",
> 
> These URLs probably aren't good for committing :)

Haha, I forgot about those :)

Comment 19

[Shane Caraveo (:mixedpuppy)]

Attachment: share v3

simplified open graph parsing, addressed other feedback from Jared

Comment 20

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 699505 [details] [diff] [review]
share v3

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

(I'm gonna help look at this too)

Comment 21

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 699505 [details] [diff] [review]
share v3

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

(Apologies for the delay here, took a while to get the whole context for the patch back in my mind)

I've looked through the whole patch and it generally looks fine. There are a few areas that I have some comments for possible improvements:

- I'm not very fond of the inversion of control on the unload part. Are we expecting that the page will call window.close() when it finishes sharing? To do that I think we should use the approach that we did in MozSocialAPI.jsm (allow script to close window and watch DOMWindowClose -- http://mxr.mozilla.org/mozilla-central/source/toolkit/components/social/MozSocialAPI.jsm?force=1#182) and not messing up with the unload event which might be triggered in many more cases (page redirect, browser window closed, etc..)

- you will need to have a timeout to unload the panel in case it's been opened and dismissed, so it doesn't stay in memory until a share is successfully completed

- unless you're planning to expand it in the future, I think OpenGraphBuilder should be a singleton with a public function (and the other private ones) instead of an object constructor. The current code is fine but that way it will be less concerning to have the reference to the browser obj as a function parameter instead of as a member variable of an object that needs to be GC'ed.

And two more philosophical questions:
- It feels weird to have this OpenGraph parsing code in the tree but I assume you've already given lots of thought to this and this seemed the better way to do things

- The potential amount of personal data in an URL makes me think that we should disallow sharing of pages that specify Cache-Control: No-Store or other things that e-mails/intranet/bank websites use to protect the amount of data exposed beyond their internal session. This is sadly overused, and the distinction will seem apparently arbitrary to the users ("why I can share site X but not site Y?"), but I think it's something we must think about.

::: browser/base/content/browser-social.js
@@ +573,5 @@
> +      button.setAttribute("group", "share-providers");
> +      button.setAttribute("image", provider.iconURL);
> +      button.setAttribute("tooltiptext", provider.name);
> +      button.setAttribute("shareURL", provider.shareURL);
> +      button.setAttribute("oncommand", "SocialShareButton.sharePage(this.getAttribute('shareURL')); this.checked=true;");

The difference between the Social:SharePage command calling sharePage() and the buttons calling sharePage with an argument is odd

@@ +632,5 @@
> +    // different provider.  if the origin changed and we're unloading, this
> +    // happened from a provider switch in the panel
> +    let iframe = this.panel.firstChild;
> +    if (iframe.getAttribute("origin") != iframe.contentDocument.nodePrincipal.origin)
> +      return;

Do you think this if() will catch the use case mentioned in the comment, and nothing more?

If the frame is being unloaded because another one is being shown, I believe there are better ways to explicitly track that to avoid popup flickering instead of counting on this condition to work properly. I think this can also simplify some of the sharePage logic

@@ +672,5 @@
> +    let encodedURI = encodeURIComponent(pageData.url);
> +
> +    // keep last selection for share if we're on the same page still, user
> +    // may have closed panel then reopened it
> +    if (!shareURL && oldSrc.indexOf(encodedURI) > 0)

this will only work from the menu as shareURL is always non-null when called from the button

@@ +709,5 @@
> +        evt.initCustomEvent("OpenGraphData", true, true, JSON.stringify(pageData));
> +        iframe.contentDocument.documentElement.dispatchEvent(evt);
> +      }, true);
> +    }
> +    // always ensure that origin belongs to the endpoint

is it enforced somewhere else that the shareURL be https?

Also it'd probably be good to enforce that the url has the same origin as the provider

Comment 23

[Shane Caraveo (:mixedpuppy)]

Attachment: share v4

updated to current code, on top of bug 853151.  Per discussion with boriss, removes "more" button, moves provider selection to left vertical column.  Provider bar is still not styled.  Waiting on ui before getting more feedback/review.

Comment 24

[Shane Caraveo (:mixedpuppy)]

Attachment: share v4

should address feedback issues, also adds new context menus for links, images, video and selection.  improves share url template handling and adds support for "body" for emailers.

Comment 25

[Shane Caraveo (:mixedpuppy)]

Attachment: share v4

one other item: navigator.mozSocial.share to allow non-social providers to allow users to share (e.g. Cliqz share)

Comment 26

[Shane Caraveo (:mixedpuppy)]

Did a quick try build of bug 853151 and this bug at 
https://tbpl.mozilla.org/?tree=Try&rev=87d26b666ae1

Shows there are a few issues to got through still.

Comment 27

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Icons: Share icon (paper airplane) for URL bar of Win7 and OSX

Comment 28

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: Default share window

Comment 29

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Icon: Plus button to add a new social provider to share panel

Comment 30

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Icons: Share icon (paper airplane) for URL bar of Win7 and OSX (toolbar button edition!)

Comment 31

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Icon: Selector inset for OSX

Comment 32

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Icon: Selector inset for Win7

Comment 33

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: Share panel color and gradient spec

Comment 34

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 738800 [details] [diff] [review]
share v4

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

I'm just a bit worried that the right-click context menu will get ridiculously big with this feature. It looks like there's potential to show all three of "Share selection", "Share image", "Share link" at the same time.  Perhaps this could be a sub-menu, or just have a single "Share" and let the panel be smart about it and possibly allow for the choice to be chosen there? Just some thoughts.

Also, I think the corpus folder from the tests would be better name as opensearch, no?

::: browser/base/content/browser-social.js
@@ +395,5 @@
>    }
> +  // We need an element to use for sizing our panel.  See if the body defines
> +  // an id for that element, otherwise use the body itself.
> +  // XXX temporary support for alternate sizing element with id frame-contents
> +  // remove the 'frame-contents' id prior to landing

XXX undo this change?

@@ +591,5 @@
> +    return document.getElementById("social-share-panel");
> +  },
> +
> +  get iframe() {
> +    if (this.panel.firstChild == this.panel.lastChild)

hmm this is strange..  this.panel.childElementCount == 1 ?  And can you add a comment explaining that this is checking only for the toolbar vbox existence?

@@ +605,5 @@
> +    this.panel.hidden = false;
> +    // create and initialize the panel for this window
> +    let iframe = document.createElement("iframe");
> +    iframe.setAttribute("type", "content");
> +    iframe.setAttribute("class", "social-share-frame");

do you need to set origin in this iframe as well? or doesn't the content need the MozSocialAPI obj injected?

@@ +651,5 @@
> +    return document.getElementById("social-share-button");
> +  },
> +
> +  canShareUrl: function(aURI) {
> +    // a couple quick-n-easy checks

unecessary comment

@@ +653,5 @@
> +
> +  canShareUrl: function(aURI) {
> +    // a couple quick-n-easy checks
> +    if (PrivateBrowsingUtils.isWindowPrivate(window))
> +      return false;

it doesn't seem that checking for private browsing fits in the "canShareUrl" function (since it's about the state of the browser and not the URL).

I would personally merge the two canShare* functions and pass isPageShare as a parameter.

@@ +660,5 @@
> +    return true;
> +  },
> +
> +  canSharePage: function(aBrowser) {
> +    // a couple quick-n-easy checks

ditto

@@ +679,5 @@
> +    }
> +
> +    // Continue only if we have a 2xx status code.
> +    try {
> +      if (Math.floor(httpChannel.responseStatus / 100) != 2)

for this check you can replace it with .requestSucceeded:

http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#1151

(but it still needs to be inside the try catch)

@@ +737,5 @@
> +    // will call sharePage with an origin for us to switch to.
> +    this._createFrame();
> +    let iframe = this.iframe;
> +    let lastProviderOrigin = iframe.getAttribute("origin");
> +    

nit: whitespace

@@ +751,5 @@
> +    // either we're sharing the current browser page, or we're sharing a
> +    // specific url that is not the current browser. In the first case we can
> +    // validate cache-control headers, otherwise we cannot
> +    let shareURL = provider.shareURL;
> +    let isPageShare = !graphData || graphData.url == gBrowser.currentURI.spec;

what does it mean for this function to be called with no graphData? shouldn't it just bail out early?

@@ +768,5 @@
> +      if (graphData) {
> +        // overwrite data retreived from page with data given to us as a param
> +        for (let p in graphData) {
> +          pageData[p] = graphData[p];
> +        }        

nit: whitespace

@@ +776,5 @@
> +
> +    // quick support for existing share endpoints by supporting their
> +    // querystring arguments. parse the query string template and do
> +    // replacements where necessary the query names may be different than ours,
> +    // so we could see u=%{url} or url=%{url}

shouldn't this logic be part of OpenGraphBuilder instead of this front-end code?

::: browser/base/content/browser.js
@@ +3404,5 @@
>      URLBarSetURI();
>      XULBrowserWindow.asyncUpdateUI();
>      PlacesStarButton.updateState();
>      SocialMark.updateMarkState();
> +    SocialShare.update();

it's a bit confusing that some of these objects have both updateState() and update() (called from "social:provider-set"), whereas this one uses update() in both.

::: browser/base/content/test/social/corpus/shortlink_linkrel.html
@@ +1,3 @@
> +<html>
> +<head>
> +    <link rel="image_src" href="http://farm5.staticflickr.com/4147/5083482627_10dede5f3c_m.jpg" id="image-src" />

do you still need to change these urls as mentioned in comment 17?

::: toolkit/components/social/MozSocialAPI.jsm
@@ +165,5 @@
> +          dataOut.previews = [data.image];
> +
> +        chromeWindow.SocialShare.sharePage(null, dataOut);
> +      }
> +    },

not sure if we should throw errors here or fail silently (maybe returning false). I wonder if it's any privacy leak saying "Share is unavailable" or "Attempt to share without user input", but I guess it's no big deal because it's only the social frames that get these functions.

Comment 35

[Shane Caraveo (:mixedpuppy)]

(In reply to :Felipe Gomes from comment #34)
> Comment on attachment 738800 [details] [diff] [review]
> share v4
> 
> Review of attachment 738800 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm just a bit worried that the right-click context menu will get
> ridiculously big with this feature. It looks like there's potential to show
> all three of "Share selection", "Share image", "Share link" at the same
> time.  Perhaps this could be a sub-menu, or just have a single "Share" and
> let the panel be smart about it and possibly allow for the choice to be
> chosen there? Just some thoughts.

I think that would only happen in edge cases.  e.g. an image inside an A tag.  the user selects the image, right clicks on it, you'll get three share content menus.  without selection, you get two.  Without the A tag, you get one.

This is consistent with other similar content menu's that operate in multiple ways.  We can think more about this in a follow up.

> @@ +605,5 @@
> > +    this.panel.hidden = false;
> > +    // create and initialize the panel for this window
> > +    let iframe = document.createElement("iframe");
> > +    iframe.setAttribute("type", "content");
> > +    iframe.setAttribute("class", "social-share-frame");
> 
> do you need to set origin in this iframe as well? or doesn't the content
> need the MozSocialAPI obj injected?


That is set when the user does a share, prior to loading the iframe.


> @@ +653,5 @@
> > +
> > +  canShareUrl: function(aURI) {
> > +    // a couple quick-n-easy checks
> > +    if (PrivateBrowsingUtils.isWindowPrivate(window))
> > +      return false;
> 
> it doesn't seem that checking for private browsing fits in the "canShareUrl"
> function (since it's about the state of the browser and not the URL).
> 
> I would personally merge the two canShare* functions and pass isPageShare as
> a parameter.

Hmm, yeah, that can work.

> @@ +660,5 @@
> > +    return true;
> > +  },
> > +
> > +  canSharePage: function(aBrowser) {
> > +    // a couple quick-n-easy checks
> 
> ditto
> 
> @@ +679,5 @@
> > +    }
> > +
> > +    // Continue only if we have a 2xx status code.
> > +    try {
> > +      if (Math.floor(httpChannel.responseStatus / 100) != 2)
> 
> for this check you can replace it with .requestSucceeded:
> 
> http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/
> HttpBaseChannel.cpp#1151
> 
> (but it still needs to be inside the try catch)
> 
> @@ +737,5 @@
> > +    // will call sharePage with an origin for us to switch to.
> > +    this._createFrame();
> > +    let iframe = this.iframe;
> > +    let lastProviderOrigin = iframe.getAttribute("origin");
> > +    
> 
> nit: whitespace
> 
> @@ +751,5 @@
> > +    // either we're sharing the current browser page, or we're sharing a
> > +    // specific url that is not the current browser. In the first case we can
> > +    // validate cache-control headers, otherwise we cannot
> > +    let shareURL = provider.shareURL;
> > +    let isPageShare = !graphData || graphData.url == gBrowser.currentURI.spec;
> 
> what does it mean for this function to be called with no graphData?
> shouldn't it just bail out early?


No, graphData is an optional param.  graphData allows the caller to set some specific data.  The context menu uses that.  A simple page share will not set graphData, rather we parse the graphData out of the page itself using OpenGraphBuilder.



> @@ +768,5 @@
> > +      if (graphData) {
> > +        // overwrite data retreived from page with data given to us as a param
> > +        for (let p in graphData) {
> > +          pageData[p] = graphData[p];
> > +        }        
> 
> nit: whitespace
> 
> @@ +776,5 @@
> > +
> > +    // quick support for existing share endpoints by supporting their
> > +    // querystring arguments. parse the query string template and do
> > +    // replacements where necessary the query names may be different than ours,
> > +    // so we could see u=%{url} or url=%{url}
> 
> shouldn't this logic be part of OpenGraphBuilder instead of this front-end
> code?


I'll refactor into its own function, but it is not directly related to OpenGraphBuilder.  OpenGraphBuilder parses the page for data.  This logic builds a url from a template using a js object for the param data.


> ::: browser/base/content/browser.js
> @@ +3404,5 @@
> >      URLBarSetURI();
> >      XULBrowserWindow.asyncUpdateUI();
> >      PlacesStarButton.updateState();
> >      SocialMark.updateMarkState();
> > +    SocialShare.update();
> 
> it's a bit confusing that some of these objects have both updateState() and
> update() (called from "social:provider-set"), whereas this one uses update()
> in both.


The updateState functions call update, but also do other state related changes to the button.  The share button doesn't have any other state realted to it beyond what update is doing.  I could add an updateState function, but all it would do is call update.  I think all these classes could use some refactoring at this point.


> ::: browser/base/content/test/social/corpus/shortlink_linkrel.html
> @@ +1,3 @@
> > +<html>
> > +<head>
> > +    <link rel="image_src" href="http://farm5.staticflickr.com/4147/5083482627_10dede5f3c_m.jpg" id="image-src" />
> 
> do you still need to change these urls as mentioned in comment 17?

right.

> ::: toolkit/components/social/MozSocialAPI.jsm
> @@ +165,5 @@
> > +          dataOut.previews = [data.image];
> > +
> > +        chromeWindow.SocialShare.sharePage(null, dataOut);
> > +      }
> > +    },
> 
> not sure if we should throw errors here or fail silently (maybe returning
> false). I wonder if it's any privacy leak saying "Share is unavailable" or
> "Attempt to share without user input", but I guess it's no big deal because
> it's only the social frames that get these functions.

One situation would be that the panel has a share button, but the user has no share providers.  I think in that case, we should show the share panel with a builtin page explaining this.  This will eventually be replaced with webactivities.

Comment 36

[Shane Caraveo (:mixedpuppy)]

Attachment: share v4

feedback implemented per my responses in comment 35.  osx css implemented, working on windows css, linux css has not yet started.  I expect those css changes to be the only other changes that will happen, baring some major issue.

Comment 37

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 745394 [details] [diff] [review]
share v4

LGTM!

I still think we should find a better place for _generateShareEndpointURL rather than in the front-end (maybe SocialService? dunno), but it's nice that it's at least split into its own function. I'll leave that up to you (or to a follow up).

Comment 38

[Shane Caraveo (:mixedpuppy)]

Attachment: ordered providers

kept forgetting about the ordering of providers, this patch makes our provider list ordered.

Comment 39

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 745703 [details] [diff] [review]
ordered providers

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

Looks good. Worth mentioning in a comment why you can't do the SORT BY frecency (because the providers with no frecency won't show up as you mentioned on irc).

You should ask review from someone used to the DB APIs to figure out what's up with the stmt.params.hostname = .. not working

Comment 40

[Shane Caraveo (:mixedpuppy)]

moving the ordering of menus to bug 869209

Comment 41

[Shane Caraveo (:mixedpuppy)]

Attachment: share v4

fixes populating the share provider list when providers are added/removed
fixes opening the panel when the primary social provider is not a share provider
css/visual tweaks

carry forward r+ from felipe per irc

https://tbpl.mozilla.org/?tree=Try&rev=ab1fb0ad1967

Comment 42

[Shane Caraveo (:mixedpuppy)]

Attachment: share v4

patch rebased

https://hg.mozilla.org/integration/mozilla-inbound/rev/89d77682678c

Comment 43

[Phil Ringnalda (:philor)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/78b49aecbefa - at least Linux, so far, was unhappy in browser_social_multiprovider.js:

https://tbpl.mozilla.org/php/getParsedLog.php?id=22664293&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=22664651&tree=Mozilla-Inbound

Comment 44

[Phil Ringnalda (:philor)]

All the other platforms did join in too, eventually.

Comment 45

[Shane Caraveo (:mixedpuppy)]

Attachment: share v4

actual rebased patch.  

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d3a05e286a6

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d3a05e286a6

Comment 47

[Alfred Kayser]

What is the difference between social-share-button and social-mark-button?

Comment 48

[Lukas Blakk [:lsblakk] use ?needinfo]

This has been noted in the Aurora 23 release notes:

http://www.mozilla.org/en-US/firefox/23.0a2/auroranotes/

If you would like to make any changes or have questions/concerns please contact me directly.

Comment 49

[Florian Bender]

How can I test this feature? I can't find any way to enable the share icon, I can only make it appear when installing Facebook Messenger (SocialAPI) however it's disabled.

Comment 50

[Shane Caraveo (:mixedpuppy)]

(In reply to Florian Bender from comment #49)
> How can I test this feature? I can't find any way to enable the share icon,
> I can only make it appear when installing Facebook Messenger (SocialAPI)
> however it's disabled.

We currently only have Share for Facebook.  You'll have to enable Facebook to see Share.

Comment 51

[edA-qa mort-ora-y]

The deployed Cliqz provider also has a working share panel:
http://mozsocial.cliqz.com/
You must install with 23+ however otherwise you'll end up with the old manifest. Once installed share should be enabled on any webpage.

Comment 52

[Florian Bender]

Sorry for Bugspam, but I want to post this here as this will affect the base impl: 

(In reply to Florian Bender from Bug 879982 comment #2)
> It might be better though to adopt the model Safari uses: Show a simple list
> of (available) providers (incl. Mail) and upon a click, switch (or morph
> in)to the current Share doorhanger. This will mean that you need a second
> click to share with your "favorite" provider though it does not mean any
> more work for the other providers (furthermore, it delays loading of
> "unnecessary" content if you want to share with a different provider than
> the default one).

Comment 53

[edA-qa mort-ora-y]

I would avoid building too much native intelligence into the share panel button as it limits what providers could offer. For example, in the Cliqz example we could be the sole provider for some users as we additionally offer other sharing networks. The user could share to multiple networks very quickly, and easily alternate between private email and other configured email addresses.

Comment 54

[Florian Bender]

This is not any more native intelligence then there is now (the provider switcher just get's a dedicated view). And with Bug 879982, you'll never be a sole provider (because there's always the "Mail" pseudo-provider); the built-in "Mail" pseudo-provider should always link directly to your email program (e. g. the same as "File" -> "Send Link per email") and thus never needs a dedicated panel (moreover a dedicated mail panel like Safari has is IMO poor UX). 

However it may be interesting to set a "default" sharing provider which gets selected automatically (going "back" to the overview requires e.g. clicking the Share icon a second time – though this is probably not optimal UX).

Comment 55

[u279076]

QA has finished testing this panel and feels it's sufficient for release in Firefox 23.