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

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jaws, Assigned: markh)

Tracking

17 Branch
Firefox 17
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Fx17])

Attachments

(1 attachment, 7 obsolete attachments)

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.
Created attachment 651954 [details] [diff] [review]
WIP patch

This was the patch that I had started working on. Putting it here in case it helps anyone else.
Assignee: jaws → mhammond
(Assignee)

Comment 2

5 years ago
Created attachment 652026 [details] [diff] [review]
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...
Attachment #651954 - Attachment is obsolete: true
Attachment #652026 - Flags: feedback?(jaws)
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.
Attachment #652026 - Flags: feedback?(jaws) → feedback+
(Assignee)

Comment 4

5 years ago
Created attachment 652688 [details] [diff] [review]
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.
Attachment #652026 - Attachment is obsolete: true
Attachment #652688 - Flags: feedback?(mixedpuppy)
Attachment #652688 - Flags: feedback?(jaws)
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
Attachment #652688 - Flags: feedback?(jaws) → feedback+
(Assignee)

Updated

5 years ago
Depends on: 780010
(Assignee)

Comment 6

5 years ago
Created attachment 653669 [details] [diff] [review]
updated patch addressing review comments

New patch, but it is blocked by bug 780010 as the social button tests have been disabled.
Attachment #652688 - Attachment is obsolete: true
Attachment #652688 - Flags: feedback?(mixedpuppy)
Whiteboard: [Fx17]
(Assignee)

Comment 7

5 years ago
Created attachment 654530 [details] [diff] [review]
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
Attachment #653669 - Attachment is obsolete: true
Attachment #654530 - Flags: review?(gavin.sharp)
(Assignee)

Comment 8

5 years ago
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",
+            }
+          }
+        });
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).
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.
(Assignee)

Comment 11

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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?
(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.
(Assignee)

Comment 13

5 years ago
Created attachment 654914 [details] [diff] [review]
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;
Attachment #654530 - Attachment is obsolete: true
Attachment #654530 - Flags: review?(gavin.sharp)
Attachment #654914 - Flags: review?(gavin.sharp)
Attachment #654914 - Flags: review?(mixedpuppy)
Attachment #654914 - Flags: review?(mixedpuppy) → review+
(Assignee)

Comment 14

5 years ago
Created attachment 655507 [details] [diff] [review]
updated due to slight bitrot

Carrying r+ forward.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f79e4c7902a1
Attachment #654914 - Attachment is obsolete: true
Attachment #654914 - Flags: review?(gavin.sharp)
Attachment #655507 - Flags: review+
(Assignee)

Updated

5 years ago
Flags: in-testsuite+
(Assignee)

Comment 15

5 years ago
Backed out in e53008d2ca14 due to talos redness.
(Assignee)

Comment 16

5 years ago
Created attachment 655537 [details] [diff] [review]
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
Attachment #655507 - Attachment is obsolete: true
Attachment #655537 - Flags: review+
(Assignee)

Comment 17

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b22f48262d1
https://hg.mozilla.org/mozilla-central/rev/8b22f48262d1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17

Comment 19

5 years ago
Why is the icon image on the share button set as background image?
    shareButton.style.backgroundImage = 'url("' + encodeURI(imageURL) + '")';
This should have been .src.
(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 :)
Depends on: 804068
You need to log in before you can comment on or make changes to this bug.