780987 - Call social.user-recommend-prompt during initialization of the Social API functionality

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

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

We need to call social.user-recommend-prompt during initialization of the Social API functionality and get back the social.user-recommend-prompt-response so that we can use the provider-based images for the share button in the location bar.

Comment 1

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

Attachment: WIP patch

This was the patch that I had started working on. Putting it here in case it helps anyone else.

Comment 2

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

Attachment: WIP patch

This is a WIP patch.  It keeps (almost) all changes localized to the shareButton class which seems a little cleaner than spreading it out to, eg, the WorkerAPI module.  It isn't complete as we need some closure on the fact we now need up to *4* strings and images to implement this correctly - see my recent mail on this - but feedback appreciated in the meantime...

Comment 3

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

Comment on attachment 652026 [details] [diff] [review]
WIP patch

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

At a high level this looks good to me. Just so you know, I disabled the browser_social_shareButton test when I landed bug 777176 since it was causing issues. I filed bug 780010 to fix up the shareButton test to use the head.js methods.

::: browser/base/content/browser-social.js
@@ +160,5 @@
>      this.updateButtonHiddenState();
>      this.updateProfileInfo();
>    },
>  
> +  // XXX - can this be done in onpopupshowing?

I'd rather not do it in onpopupshowing since it will slow the loading of the popup.

@@ +250,5 @@
>      let status = document.getElementById("share-button-status");
>      if (status) {
> +      // XXX - this should also be capable of reflecting that the page was
> +      // unshared (ie, it needs to manage three-states: (1) nothing done, (2)
> +      // shared, (3) shared then unshared)

Yeah, I agree. Thanks for catching this.

Comment 4

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

Attachment: Get recommend images and strings from provider

New patch that is largely complete.  Of particular note:

* This includes a straw-man api for the user-recommend-prompt message.  See the change to the test social_worker.js for an example of what is implemented - basically each image must have a URL and can optionally include 'x' and 'y' element if the image is a sprite.

* We validate the data in the -response message and check the resolved image URL starts with http://, https:// or data:.  We then stick the quoted version of the URL directly in the .background style element wrapped in quotes.  Hopefully wrapping it in quotes will handle if the URL has a ')' in it (which encodeURI doesn't touch).

* Test coverage is reasonable but lacking in tests for invalid data being returned by the provider.

* All "default" strings and images have been removed - I can't see a need to support these defaults - if a provider doesn't return all the stuff we need, we just don't enable sharing for that provider.

* I believe we could handle HiDPI displays simply by including the desired image size in the user-recommend message - it would be "16px" for the normal case and "32px" for the HiDPI case - but this hasn't been done.

Comment 5

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

Comment on attachment 652688 [details] [diff] [review]
Get recommend images and strings from provider

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

The share image files were removed from the jar manifests, but still need to be removed from the source tree.

::: browser/base/content/browser-social.js
@@ +211,5 @@
> +      // resolve potentially relative URLs then check the scheme is acceptable.
> +      imageInfo.url = Services.io.newURI(Social.provider.origin, null, null).resolve(imageInfo.url);
> +      if (imageInfo.url.indexOf("http://") != 0 &&
> +          imageInfo.url.indexOf("https://") != 0 &&
> +          imageInfo.url.indexOf("data:") != 0) {

Please use nsIURI.scheme for these checks (https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIURI?redirectlocale=en-US&redirectslug=nsIURI#Attributes).

@@ +227,5 @@
> +        return reportError('messages["' + sub + '"] is not a valid string');
> +      }
> +    }
> +    this.promptImages = data.images;
> +    this.promptMessages = data.messages;

I don't think this does enough sanitization. There can be other properties in these objects that will be carried over unexpectedly which can cause problems. I'd rather see a new object get created that only has the whitelisted and validated properties.

@@ +318,5 @@
> +    let bg = 'url("' + encodeURI(imageInfo.url) + '")';
> +    if (imageInfo.x || imageInfo.y) {
> +      bg += ' ' + imageInfo.x + "px " + imageInfo.y + "px";
> +    }
> +    shareButton.style.background = bg;

Instead of building up the shorthand CSS value for background, please use shareButton.style.backgroundImage and shareButton.style.backgroundPosition. Also, does this proposed API explicitly state that the offsets will need to be in pixel units?

::: browser/themes/winstripe/browser.css
@@ +1639,5 @@
> +  width: 16px;
> +  height: 16px;
> +  background-repeat: no-repeat;
> +}
> +  

nit: trailing whitespace

Comment 6

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

Attachment: updated patch addressing review comments

New patch, but it is blocked by bug 780010 as the social button tests have been disabled.

Comment 7

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

Attachment: updated patch with feedback comments addressed and on top of bug 780010

This patch is on top of bug 780010.  It is very similar to the previous patch but with all comments by Jaws addressed.

Part of the try run at https://tbpl.mozilla.org/?tree=Try&rev=1e0b679f0a6b

Comment 8

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

Oh - I also meant to re-emphasize that this patch has a proposal for the user-recommend-prompt API, but that proposal hasn't specifically been addressed.

The relevant part of the patch which demonstrates the API is below - if anyone is unhappy with that, speak now or forever hold your peace :)

The "test worker" now responds to the social.user-recommend-prompt message with:

+        port.postMessage({
+          topic: "social.user-recommend-prompt-response",
+          data: {
+            images: {
+              share: {
+                // this one is relative to test we handle relative ones.
+                url: "browser/browser/base/content/test/social_share_image.png",
+                x: 0,
+                y: 0,
+              },
+              shared: {
+                // absolute to check we handle them
+                url: "https://example.com/browser/browser/base/content/test/social_share_image.png"
,
+                x: -16,
+                y: 0,
+              }
+            },
+            messages: {
+              shareTooltip: "Share this page",
+              unshareTooltip: "Unshare this page",
+              sharedLabel: "This page has been shared",
+              unsharedLabel: "This page is no longer shared",
+            }
+          }
+        });

Comment 9

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

I replied earlier with a question about the API. I don't think we should be using unit-less offsets.

(In reply to Jared Wein [:jaws] from comment #5)
> @@ +318,5 @@
> > +    let bg = 'url("' + encodeURI(imageInfo.url) + '")';
> > +    if (imageInfo.x || imageInfo.y) {
> > +      bg += ' ' + imageInfo.x + "px " + imageInfo.y + "px";
> > +    }
> > +    shareButton.style.background = bg;
> 
> Instead of building up the shorthand CSS value for background, please use
> shareButton.style.backgroundImage and shareButton.style.backgroundPosition.
> Also, does this proposed API explicitly state that the offsets will need to
> be in pixel units?

I also don't like how we're using negative values to show positive right/bottom movement (even though it is similar to CSS background position values).

Comment 10

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

Do we really need sprite support for the moment? Why not start with a simpler API of just allowing the provider to specify multiple images? Simpler all around, we can revisit other options later.

Comment 11

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

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #10)
> Do we really need sprite support for the moment? Why not start with a
> simpler API of just allowing the provider to specify multiple images?
> Simpler all around, we can revisit other options later.

One of our partners was previously using a sprite image as, best I can tell, they don't have a standard asset with just the recommend image.  Obviously they could make one (and by that same logic, sprite support will never be truly needed)

Do you want to ask them, or do you just want me to rip out the 10 or so lines which have that support?  If you do want it ripped out, should I also remove the nested object for each image, making it a flatter object much like the strings?

Comment 12

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

(In reply to Mark Hammond (:markh) from comment #11)
> Do you want to ask them, or do you just want me to rip out the 10 or so
> lines which have that support?  If you do want it ripped out, should I also
> remove the nested object for each image, making it a flatter object much
> like the strings?

I'm worried more about conceptual complexity rather than implementation complexity. Let's keep things simple and just use a flat object with individual images. If partners end up having difficulty with that system, we can easily revisit and add those 10 lines.

Comment 13

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

Attachment: Remove support for sprites, images now a flat object

Also made the key names in "images" more consistent.  Relevant part of the test that demonstrates the API is now:

+      case "social.user-recommend-prompt":
+        port.postMessage({
+          topic: "social.user-recommend-prompt-response",
+          data: {
+            images: {
+              // this one is relative to test we handle relative ones.
+              share: "browser/browser/base/content/test/social_share_image.png",
+              // absolute to check we handle them too.
+              unshare: "https://example.com/browser/browser/base/content/test/social_share_image.png"
+            },
+            messages: {
+              shareTooltip: "Share this page",
+              unshareTooltip: "Unshare this page",
+              sharedLabel: "This page has been shared",
+              unsharedLabel: "This page is no longer shared",
+            }
+          }
+        });
+        break;

Comment 14

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

Attachment: updated due to slight bitrot

Carrying r+ forward.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f79e4c7902a1

Comment 15

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

Backed out in e53008d2ca14 due to talos redness.

Comment 16

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

Attachment: Check port is not null before using it

This patch is the same as before, except that the value of |port| is checked before we attempt to use it.  Still carrying the r+ forward.

Try run, with talos enabled this time, at https://tbpl.mozilla.org/?tree=Try&rev=0cf474fbb6bf

Comment 17

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b22f48262d1

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8b22f48262d1

Comment 19

[Alfred Kayser]

Why is the icon image on the share button set as background image?
    shareButton.style.backgroundImage = 'url("' + encodeURI(imageURL) + '")';
This should have been .src.

Comment 20

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

(In reply to Alfred Kayser from comment #19)
> Why is the icon image on the share button set as background image?
>     shareButton.style.backgroundImage = 'url("' + encodeURI(imageURL) + '")';
> This should have been .src.

Thanks for the feedback. In the future, please file a new bug and mark it as blocking the bug that you find the issue in. If you write a patch for this you can flag me for review :)