1132301 - [User story] As a desktop client user I want to share a conversation URL through my favorite social network so it's easy to share

Status: VERIFIED FIXED

(Hello (Loop) :: Client, enhancement, P1)

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: WIP: feature complete patch

Comment 3

[Mike de Boer [:mikedeboer]]

Hi Michael! Can you provide two SVG and one PNG assets for me, based on https://bugzilla.mozilla.org/attachment.cgi?id=8569938:

1) SVG: Icon for 'Add a Service' menu item in the share menu (a 'Plus' glyph)
2) SVG: Icon for inside the share menu/ panel when the Share button is not available (see bottom part of the mockup)
3) PNG: Hello logo that can serve as a preview image for the Hello link clicker as shown in the Facebook share panel.

Thanks!

Comment 4

[Mike de Boer [:mikedeboer]]

Hi Shane, I have three questions for you wrt OpenGraph data:

1) For G+, LinkedIn and Facebook sharing (perhaps others as well), you can see a short blurb that is put next to the URL preview image and below the URL title that contains an excerpt of the page that belongs to the URL. Can we manipulate that text to display something we define specifically (thus can also localize) through the OpenGraph data that we pass to `SocialShare.sharePage()`?
2) If we pass a preview image in the OpenGraph data, will that show up as the image next to the blurb I mentioned above?
3) For the GMail sharing provider, can we manipulate the email body text to be different than the title we supply via the OpenGraph data?

Thanks!

Comment 5

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: share button should emit notifications when it is added to or removed from a customizable area

Comment 6

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: add navigator.mozLoop methods to allow interaction between Loop and the Social API

Shane, can you review if I'm interacting with Social API correctly here?

Comment 7

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3: hide the Loop dropdowns when the content window loses focus

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Patch 4: add a Share Link button in the Loop conversation window

Mark, just a feedback request for now, because this patch is missing some assets I requested from Michael. But adding those will not change the architecture here, unless you suggest another.

Comment 9

[Mike de Boer [:mikedeboer]]

Test coverage for part/ patch 4 is coming up in a part 5.

Comment 10

[Mike de Boer [:mikedeboer]]

Patch 4 also fixes the mis-aligned icons for the dropdown menus in the panel.

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8580749 [details] [diff] [review]
Patch 1: share button should emit notifications when it is added to or removed from a customizable area

As far as the patch goes, r+.  

I'm curious why we would add these rather than just use the CUI listener in the loop code where you want to get the notifications.

Comment 12

[Shane Caraveo (:mixedpuppy)]

(In reply to Mike de Boer [:mikedeboer] from comment #4)
> Hi Shane, I have three questions for you wrt OpenGraph data:
> 
> 1) For G+, LinkedIn and Facebook sharing (perhaps others as well), you can
> see a short blurb that is put next to the URL preview image and below the
> URL title that contains an excerpt of the page that belongs to the URL. Can
> we manipulate that text to display something we define specifically (thus
> can also localize) through the OpenGraph data that we pass to
> `SocialShare.sharePage()`?
> 2) If we pass a preview image in the OpenGraph data, will that show up as
> the image next to the blurb I mentioned above?
> 3) For the GMail sharing provider, can we manipulate the email body text to
> be different than the title we supply via the OpenGraph data?
> 
> Thanks!

Some of these we can modify the text in the call to share.  Others we may need to have a page off the services website that has the opengraph tags for the share services to consume.  We should be able to easily manipulate the body text for GMail.

Comment 13

[Mike de Boer [:mikedeboer]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #12)
> Some of these we can modify the text in the call to share.  Others we may
> need to have a page off the services website that has the opengraph tags for
> the share services to consume.  We should be able to easily manipulate the
> body text for GMail.

Alright, cool! Do you mean that I'd have to inspect the source of the respective share providers' share URL and see how they consume the OpenGraphData event?

Comment 14

[Shane Caraveo (:mixedpuppy)]

(In reply to Mike de Boer [:mikedeboer] from comment #13)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #12)
> > Some of these we can modify the text in the call to share.  Others we may
> > need to have a page off the services website that has the opengraph tags for
> > the share services to consume.  We should be able to easily manipulate the
> > body text for GMail.
> 
> Alright, cool! Do you mean that I'd have to inspect the source of the
> respective share providers' share URL and see how they consume the
> OpenGraphData event?

None of the major sites use the event.  The OpenGraph data is also used to generate a url from a url template.  What data the various share providers use in the url varies from provider to provider.  Facebook only has the "url" in the template, which means Facebook fetches the url you give them, looks at og meta tags to get any text, images, etc.  GMail has a body tag where we can define what's in the email, some providers have title, etc.  If it will help to chat, grab me on irc.

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 8580750 [details] [diff] [review]
Patch 2: add navigator.mozLoop methods to allow interaction between Loop and the Social API

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

Its a little strange that the share button is disabled on pages like about:home, especially when it makes sense to start up FF and attempt to jump straight into a conversation. It certainly confused me when I started FF and then tried to share the conversation but found the button disabled.

Please could you file a bug about that for UX to consider (or ensure there is one)

::: browser/components/loop/MozLoopAPI.jsm
@@ +899,5 @@
> +     *
> +     * @return {Boolean} `true` if the widget is available and `false` when it's
> +     *                   still in the Customization palette.
> +     */
> +    isSocialShareButtonAvailable: {

I'm partially thinking we should put these functions in their own sub-namespace, but I'm not sure its actually worth it.

@@ +968,5 @@
> +
> +    /**
> +     * Returns a sorted list of Social Providers that can share URLs. See
> +     * `updateSocialProvidersCache()` for more information.
> +     * 

nit: trailing whitespace

::: browser/components/loop/test/mochitest/head.js
@@ +70,5 @@
>      });
>    });
>  }
>  
> +function waitForCondition(condition, nextTest, errorMsg) {

I wonder if we shouldn't start filing bugs on the core testing framework to get these harmonised in one location rather than copy/pasting them all the time.

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 8580754 [details] [diff] [review]
Patch 4: add a Share Link button in the Loop conversation window

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

In general looks good. I'm not quite sure about the roomStore additions, I almost think they should be to activeRoomStore, but I can also see why you put them there.

Comment 17

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: Hello_New_Assets.zip

Updated assets attached.

Comment 18

[Mike de Boer [:mikedeboer]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #11)
> Comment on attachment 8580749 [details] [diff] [review]
> Patch 1: share button should emit notifications when it is added to or
> removed from a customizable area
> 
> As far as the patch goes, r+.  
> 
> I'm curious why we would add these rather than just use the CUI listener in
> the loop code where you want to get the notifications.

This is because the Loop window(s), especially the conversation ones have a lifetime that is not connected to any browser window. The CustomizableUI object is, so when a window is closed, the listener(s) will be removed as well. If the chat window is undocked, it will continue to exist even though its opener window may be gone.
That's why I preferred to rely on global notifications, rather than window-scoped event listeners.

Comment 19

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.1: add navigator.mozLoop methods to allow interaction between Loop and the Social API

Added support for the `body` graphData property for GMail - see comment 14.

Comment 20

[Mike de Boer [:mikedeboer]]

Comment on attachment 8582438 [details] [diff] [review]
Patch 2.1: add navigator.mozLoop methods to allow interaction between Loop and the Social API

Carrying over r=Standard8,mixedpuppy

Comment 21

[Mike de Boer [:mikedeboer]]

Attachment: Patch 4.1: add a Share Link button in the Loop conversation window

- Added support for the `body` graphData for GMail
 - Added support for dropdown anchoring in the mixin
 - And a whole lot of other goodies

Currently working on the tests for this part (will be Patch 5).

Comment 22

[Mike de Boer [:mikedeboer]]

Attachment: Patch 5: unit tests covering patch 4

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 8582446 [details] [diff] [review]
Patch 4.1: add a Share Link button in the Loop conversation window

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

Testing this against latest fx-team, I'm seeing a couple of issues:

- The "Invite somone to join you" text is overlapped by the link action buttons
- There's a scroll bar which shows blank space to the right of the video.

::: browser/components/loop/content/js/roomViews.jsx
@@ +83,5 @@
> +        return provider.origin == origin;
> +      })[0];
> +
> +      // XXXmikedeboer: this might later contain the roomToken to retrieve more
> +      //                context details to share.

I'm not sure we need this comment unless we're positive its going to happen, if so, then we should file a bug for it and reference it here.

@@ +92,5 @@
> +      }));
> +    },
> +
> +    render: function() {
> +      // Don't render a thing yet when no data has been fetched yet.

nit: drop the first yet.

::: browser/components/loop/content/shared/img/icons-16x16.svg
@@ +164,5 @@
>        h-8.3C1.65,2,1,2.68,1,3.52v6.24c0,0.83,0.65,1.51,1.45,1.51h0.52V5.43c0-0.84,0.65-1.51,1.45-1.51h6.61l-0.81,
>        0.81H5.25 c-0.8,0-1.45,0.68-1.45,1.51v4.91l-1.89,1.89l0.99,0.99l1-1l8.3-8.3L14.21,2.72z"/>
>    </g>
>  </defs>
> +<use id="add"                 xlink:href="#add-shape"/>

Please add the new icons (add + share) to the ui-showcase.

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +286,5 @@
>      /**
> +     * Handles an update of the position of the Share widget, notified by the
> +     * mozLoop API.
> +     */
> +    _handleShareWidgetUpdate: function() {

I think we should make these new handle functions dispatch actions (like we do with _handleRoom{Update,Delete} already).

The reason is this gives us consistency - information coming in from external interfaces is always handled as actions, hence we can more easily track it and side effects are less likely.

I'm also thinking that in future if we want to split out some of these mozLoop interfaces or create multiple stores (due to complexity), then having everything going through actions already will make that easier.

::: browser/components/loop/standalone/content/index.html
@@ +137,5 @@
>        }, false);
>      </script>
> +
> +    <noscript>
> +      <img src="img/logo.png" border="0" alt="Logo"/>

I can't see any reason to have this in this bug. I think we should split it out somewhere else. On my browser its also shown half way down the page (scrolled off the bottom). I'm also tempted to say we should have some default noscript text here even if its english only.

Comment 24

[Mike de Boer [:mikedeboer]]

(In reply to Mark Banner (:standard8) from comment #23)
> Testing this against latest fx-team, I'm seeing a couple of issues:
> 
> - The "Invite somone to join you" text is overlapped by the link action
> buttons

I don't see this. Odd. How can I reproduce this?

> - There's a scroll bar which shows blank space to the right of the video.

The above might be due to this issue. I added `body { overflow: hidden }` at some point, but removed it from this patch. It seems I need to re-add it!

> I can't see any reason to have this in this bug. I think we should split it
> out somewhere else. On my browser its also shown half way down the page
> (scrolled off the bottom). I'm also tempted to say we should have some
> default noscript text here even if its english only.

I added it to this patch, because the Facebook & G+ share providers fetch the page on their servers and prune it for content to use in the preview that is shown on the timeline after publishing the intent.
We can add a generic intro text in the <noscript> block too, but that one won't be localizable without pre-processing on the server that serves the LC page. I think that's something we should discuss and define in a separate bug.
The fact that it's shown half way down the page is not an issue. It's essential only that the image is there for a server side script to find, not for the user to see.

Comment 25

[Mike de Boer [:mikedeboer]]

Attachment: Patch 4.2: add a Share Link button in the Loop conversation window

Patch with comments addressed.

Comment 26

[Mike de Boer [:mikedeboer]]

Attachment: Patch 5.1: unit tests covering patch 4

Comment 27

[Mark Banner (:standard8)]

Comment on attachment 8583773 [details] [diff] [review]
Patch 4.2: add a Share Link button in the Loop conversation window

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

Thanks for the note about the noscript image. That helps explain why.

r=Standard8

Comment 28

[Mark Banner (:standard8)]

Comment on attachment 8583774 [details] [diff] [review]
Patch 5.1: unit tests covering patch 4

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

Looks good, just a couple of minor nits. r=Standard8

::: browser/components/loop/test/desktop-local/roomViews_test.js
@@ +452,5 @@
>    });
> +
> +  describe("SocialShareDropdown", function() {
> +    var view;
> +    var fakeProvider = {

nit: normally we initialise these inside the beforeEach to prevent issues with tests accidentally modifying them and having unexpected side-effects on other tests.

::: browser/components/loop/test/shared/roomStore_test.js
@@ +73,5 @@
>        rooms: [],
>        activeRoom: {}
>      };
>  
> +    var mozL10n = document.mozL10n || navigator.mozL10n;

This doesn't seem to be doing anything, I've tried removing it and tests still pass without.

Comment 29

[Mike de Boer [:mikedeboer]]

Thanks for all the reviews, folks!

Pushed to fx-team as:

https://hg.mozilla.org/integration/fx-team/rev/3d3ff867d390
https://hg.mozilla.org/integration/fx-team/rev/7d3633bdd4a3
https://hg.mozilla.org/integration/fx-team/rev/d241102c104c
https://hg.mozilla.org/integration/fx-team/rev/0e57fb772be5
https://hg.mozilla.org/integration/fx-team/rev/ef364246350a

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-bc test failures and leaks. Feel free to re-land once you've run it through Try.
https://hg.mozilla.org/integration/fx-team/rev/3f3bb18b9f04

https://treeherder.mozilla.org/logviewer.html#?job_id=2443830&repo=fx-team
https://treeherder.mozilla.org/logviewer.html#?job_id=2443139&repo=fx-team

Comment 31

[Mike de Boer [:mikedeboer]]

Try push with possible fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=555d45669d66

Comment 32

[Mike de Boer [:mikedeboer]]

Pushed strings only. *sigh*

https://hg.mozilla.org/integration/fx-team/rev/b9caf6715b7e

Comment 33

[Mike de Boer [:mikedeboer]]

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

Comment 34

[Axel Hecht [:Pike]]

FTR, I don't see why this should preland strings, and not just ride the trains when it's ready.

Comment 35

[Mike de Boer [:mikedeboer]]

Shane, no matter what I try, I keep failing mochitests on our infra - not locally!! - for Patch 2.
See https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fb2a808e1a6 for an example.

As you can see, it consistently leaks _two_ 'about:mozilla' windows, which I have no idea where they're coming from. Do you have any ideas?

Comment 36

[Shane Caraveo (:mixedpuppy)]

Only place I see you using about:mozilla is in browser_mozLoop_sharingListener.js.  Tabs are added and removed, but you don't wait on the removal of the tabs, I would try that.  I also see various other js failures in there, such as https connection failures with loop, not sure at what point it is happening or whether it would be a contributing factor.

05:48:25     INFO -  578 INFO Console message: [JavaScript Error: "An error occurred during a connection to fail-handshake.example.com:443.
05:48:25     INFO -  SSL received an unexpected Client Hello handshake message.
05:48:25     INFO -  (Error code: ssl_error_rx_unexpected_client_hello)

I'm out today, can look further next week if you need help.

Comment 37

[Shane Caraveo (:mixedpuppy)]

With leaks like this I often try --repeat 20 or something to see if it will force a leak.  Tried that here, but other problems crop up.

Running with ."/mach mochitest-browser --repeat 2 browser/components/loop/test/mochitest/" I get a number of errors.  kShareProvider isn't removed in the new test file (could cause more issues if socialapi tests were run after loop tests), and some errors in browser/components/loop/test/mochitest/browser_toolbarbutton.js  (which I didn't look into)

This fixes the provider removal:

registerCleanupFunction(function() {
  yield new Promise(resolve => SocialService.disableProvider(kShareProvider.origin, resolve));
  yield new Promise(resolve => SocialService.disableProvider(kShareProviderInvalid.origin, resolve));
  Assert.strictEqual(Social.providers.length, 0, "all providers should be removed");
  SocialShare.uninit();
});

IMO you can also remove the worker script from kShareProvider, shouldn't be necessary for your tests. 


There is also lots of these in the output:

JavaScript error: resource:///modules/loop/MozLoopAPI.jsm, line 1059: TypeError: targetWindow.CustomEvent is not a constructor

I'm not able to reproduce the leak of about:mozilla windows from browser_mozLoop_sharingListener.js.  I suspect you need to wait on the tab removal, ensure the leak is not from that test.  e.g.

gBrowser.removeTab(gBrowser.selectedTab);
gBrowser.tabContainer.addEventListener("TabClose", function onTabClose() {
  gBrowser.tabContainer.removeEventListener("TabClose", onTabClose);
  // continue after this
});

Comment 38

[Mike de Boer [:mikedeboer]]

Shane, thanks so much for that analysis! I'll be off trying these things and more. Super helpful.

Mark, I think you linked the wrong bug just now...

Comment 39

[Mike de Boer [:mikedeboer]]

owait, you didn't. oops.

Comment 40

[Mike de Boer [:mikedeboer]]

Try push with latest updates. Fingers crossed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e1c1a2029e7

Comment 41

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fadd5241dabd

Comment 42

[Mike de Boer [:mikedeboer]]

Alright, that try run is looking remarkably green! I unbitrotted the other patches and pushed 'em to fx-team:

https://hg.mozilla.org/integration/fx-team/rev/47a5d0ecb127
https://hg.mozilla.org/integration/fx-team/rev/e5d7fd205107
https://hg.mozilla.org/integration/fx-team/rev/a2b8f0ddd738
https://hg.mozilla.org/integration/fx-team/rev/a2075595f6fd
https://hg.mozilla.org/integration/fx-team/rev/c99243d545e2

Comment 43

[Liz Henry (:lizzard) (relman/hg->git project)]

Mike it looks like some of this landed before 39 merged to aurora, and some after.  Is 38 affected at all? Thanks.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Backed out for intermittent browser_mozLoop_socialShare.js failures.
https://hg.mozilla.org/integration/fx-team/rev/cad808749b13

https://treeherder.mozilla.org/logviewer.html#?job_id=2621218&repo=fx-team

Comment 45

[Shane Caraveo (:mixedpuppy)]

interesting that didn't show up in try.  @mdeboer, make a copy of share.html for your tests and remove the port related lines in the event handler, they are not necessary for your tests, and [I'm pretty certain] are the culprit here.

Comment 46

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=17841126e006

Comment 47

[Mike de Boer [:mikedeboer]]

(In reply to Liz Henry (:lizzard) from comment #43)
> Mike it looks like some of this landed before 39 merged to aurora, and some
> after.  Is 38 affected at all? Thanks.

Hi Liz, Fx38 is not affected. This feature is targeted for Fx39.

Comment 48

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.1: share button should emit notifications when it is added to or removed from a customizable area

Comment 49

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.2: add navigator.mozLoop methods to allow interaction between Loop and the Social API

Comment 50

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3.1: hide the Loop dropdowns when the content window loses focus

Comment 51

[Mike de Boer [:mikedeboer]]

Attachment: Patch 4.3: add a Share Link button in the Loop conversation window

Comment 52

[Mike de Boer [:mikedeboer]]

Attachment: Patch 5.2: unit tests covering patch 4

Comment 53

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/ba3e210c445d
https://hg.mozilla.org/integration/fx-team/rev/94398fb3a5dd
https://hg.mozilla.org/integration/fx-team/rev/e96199c31eeb
https://hg.mozilla.org/integration/fx-team/rev/bc502f2631df
https://hg.mozilla.org/integration/fx-team/rev/f88eb80d00b8

Comment 54

[Mike de Boer [:mikedeboer]]

Try run I pushed before landing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fbd41eaab01

Comment 55

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ba3e210c445d
https://hg.mozilla.org/mozilla-central/rev/94398fb3a5dd
https://hg.mozilla.org/mozilla-central/rev/e96199c31eeb
https://hg.mozilla.org/mozilla-central/rev/bc502f2631df
https://hg.mozilla.org/mozilla-central/rev/f88eb80d00b8

Comment 56

[Mike de Boer [:mikedeboer]]

Attachment: Patch 6: allow data: URIs as img source in Loop documents

Forgot to add this :/ This patch make the favicons for the social providers show up.

Comment 57

[Mike de Boer [:mikedeboer]]

Comment on attachment 8590481 [details] [diff] [review]
Patch 1.1: share button should emit notifications when it is added to or removed from a customizable area

Approval Request Comment
[Feature/regressing bug #]: Loop/ Hello new Social Share feature
[User impact if declined]: It will become significantly easier for a user to share a room URL through the Social API provided methods of sharing via enabled providers, like Twitter, Facebook, etc.
[Describe test coverage new/current, TreeHerder]: Landed on m-c, tests added and passing.
[Risks and why]: moderate, this is a new feature.
[String/UUID change made/needed]: n/a, strings have already landed during the regular Fx39 cycle.

Comment 58

[Mike de Boer [:mikedeboer]]

Comment on attachment 8590482 [details] [diff] [review]
Patch 2.2: add navigator.mozLoop methods to allow interaction between Loop and the Social API

Please see comment 57 for the approval request.

Comment 59

[Mike de Boer [:mikedeboer]]

Comment on attachment 8590483 [details] [diff] [review]
Patch 3.1: hide the Loop dropdowns when the content window loses focus

Please see comment 57 for the approval request.

Comment 60

[Mike de Boer [:mikedeboer]]

Comment on attachment 8590486 [details] [diff] [review]
Patch 4.3: add a Share Link button in the Loop conversation window

Please see comment 57 for the approval request.

Comment 61

[Mike de Boer [:mikedeboer]]

Comment on attachment 8590490 [details] [diff] [review]
Patch 5.2: unit tests covering patch 4

Please see comment 57 for the approval request.

Comment 62

[Mark Banner (:standard8)]

Comment on attachment 8591577 [details] [diff] [review]
Patch 6: allow data: URIs as img source in Loop documents

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

Yep, I can't think of any issues enabling these.

Comment 63

[Mike de Boer [:mikedeboer]]

Thanks! Pushed to fx-team as: https://hg.mozilla.org/integration/fx-team/rev/a3f65d0021f7

Comment 64

[Ryan VanderMeulen [:RyanVM]]

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

Comment 65

[Mike de Boer [:mikedeboer]]

Comment on attachment 8591577 [details] [diff] [review]
Patch 6: allow data: URIs as img source in Loop documents

Please see comment 57 for the approval request.

Comment 66

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8590481 [details] [diff] [review]
Patch 1.1: share button should emit notifications when it is added to or removed from a customizable area

Approving for uplift as this has had a day on m-c and it's aimed for release for 39.

Comment 67

[Liz Henry (:lizzard) (relman/hg->git project)]

Mike, you marked this as not needing qe verification, but since this adds new UI and behavior, I think it would benefit from some testing. That doesn't have to be verifying every aspect of each patch. 

Let's at least give a heads up to QE that this is coming in 39. 

Florin I leave it to you how you want to handle this; maybe you can add the URL sharing interface to your team's exploratory testing for 39 once this lands. Thanks!

Comment 68

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8eafef4aa1c3
https://hg.mozilla.org/releases/mozilla-aurora/rev/f1064a1b4a2a
https://hg.mozilla.org/releases/mozilla-aurora/rev/c3999b13c644
https://hg.mozilla.org/releases/mozilla-aurora/rev/d83f5a2c9521
https://hg.mozilla.org/releases/mozilla-aurora/rev/719d7e41abea
https://hg.mozilla.org/releases/mozilla-aurora/rev/1687db474cd2

Comment 69

[Ryan VanderMeulen [:RyanVM]]

Backed out for various test failures.
https://hg.mozilla.org/releases/mozilla-aurora/rev/9b2e30c662d6

https://treeherder.mozilla.org/logviewer.html#?job_id=769158&repo=mozilla-aurora
https://treeherder.mozilla.org/logviewer.html#?job_id=769162&repo=mozilla-aurora

Comment 70

[Ryan VanderMeulen [:RyanVM]]

And https://treeherder.mozilla.org/logviewer.html#?job_id=769027&repo=mozilla-aurora

Comment 71

[Mike de Boer [:mikedeboer]]

Pushed rebased patches to Aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/bd27054a2741
https://hg.mozilla.org/releases/mozilla-aurora/rev/0aa651d86b6b
https://hg.mozilla.org/releases/mozilla-aurora/rev/6ba5c50ac782
https://hg.mozilla.org/releases/mozilla-aurora/rev/739c89e40bcd
https://hg.mozilla.org/releases/mozilla-aurora/rev/60e700d0c808
https://hg.mozilla.org/releases/mozilla-aurora/rev/de154381b9bf

Comment 72

[Bogdan Maris, Desktop QA]

I tested this implementation across platforms (Windows 7 64-bit, Windows 8.1 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit) using latest Aurora and Nightly. I found a few issues that were added to the dependencies. Based on my testing I`m marking this bug verified fixed.
For more information about testing please see the etherpad https://etherpad.mozilla.org/ShareLink-Hello.

Comment 73

[Liz Henry (:lizzard) (relman/hg->git project)]

Mike, or maybe Gavin, do the dependent bugs here need fixing and uplifting for this feature to be released in 39?  Who is the right person for me to talk with about making that judgement call or what the priority of the newly uncovered bugs should be?

Comment 74

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

RT+Mike should have you covered.

Comment 75

[Mike de Boer [:mikedeboer]]

Liz, these bugs are not blockers for release in 39. I _am_ working on them, though.

Comment 76

[Romain Testard [:RT]]

(In reply to Mike de Boer [:mikedeboer] from comment #75)
> Liz, these bugs are not blockers for release in 39. I _am_ working on them,
> though.

Agreed, these are not blockers for Fx39.

Comment 77

[Liz Henry (:lizzard) (relman/hg->git project)]

Great. Thanks Mike. Please do request uplift if you need it!

Comment 78

[Sylvestre Ledru [:Sylvestre]]

Release Note Request (optional, but appreciated)
[Why is this notable]: New feature
[Suggested wording]: Hello link sharing
[Links (documentation, blog post, etc)]: Is there anything?

Comment 79

[Romain Testard [:RT]]

[Why is this notable]: New feature
[Suggested wording]: Share Hello URLs with social networks
[Links (documentation, blog post, etc)]: We could (should?) have a blog post and a SUMO page for launch. 

Joni can you please help us with a SUMO page to help users with the "Share" feature?
Fabio - is there a blog post planned for the Hello Fx39 share feature?

Comment 80

[Joni Chan]

Sure, I've set up a placeholder. The article will be up in a couple of hours.

If we need a URL for the release notes, please use this one: https://support.mozilla.org/kb/share-firefox-hello-social

If we need an in-product link, please let me know and I'll set up a special URL for it.

Comment 81

[Romain Testard [:RT]]

Thanks Joni! No in-product link is needed.

Comment 82

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

This was added to Firefox39 release notes on 06-11-2015. Updating the relnote-firefox flag to reflect that.

Comment 83

[Fabio Rios [:frios]]

(In reply to Romain Testard [:RT] from comment #79)
> [Why is this notable]: New feature
> [Suggested wording]: Share Hello URLs with social networks
> [Links (documentation, blog post, etc)]: We could (should?) have a blog post
> and a SUMO page for launch. 
> 
> Fabio - is there a blog post planned for the Hello Fx39 share feature?

Per Whistler meeting, we will continue promoting new features on Hello. There will be a blog post which includes a mention of Hello share feature for 39. 

Draft of Hello portion:

"Today, we’re announcing that Firefox Share has been integrated into Hello.
{SCREENSHOT}
Hello beta, which we’ve been developing with our close partner, Telefonica, is the only in-browser video chat tool that doesn’t require an account or extra software downloads. You might recall that we recently added screensharing to Hello to make it easier to share anything you’re looking at in your conversation. Now you can also send a link to friends via the social network or email of your choice to invite them to a Hello video conversation."

Comment 84

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

(In reply to Fabio Rios [:frios] from comment #83)
> Hello beta, which we’ve been developing with our close partner, Telefonica,

The first half of this sounds awkward. "Hello beta, which we've been ..." in particular. To me it sounds awkward because I perceived "Hello" as a greeting, not a proper noun. I then understood "beta" to be a reference to the beta population. So reading "Hello beta, which we've been ..." sounds like "Hi beta users, which we've been ..." which doesn't make grammatical sense.

I'm not sure the "beta" wording is necessary to call out here. This would all sound much less ambiguous if it was written as "Firefox Hello, which we've been ...".

Comment 85

[Romain Testard [:RT]]

(In reply to Fabio Rios [:frios] from comment #83)
> (In reply to Romain Testard [:RT] from comment #79)
> > [Why is this notable]: New feature
> > [Suggested wording]: Share Hello URLs with social networks
> > [Links (documentation, blog post, etc)]: We could (should?) have a blog post
> > and a SUMO page for launch. 
> > 
> > Fabio - is there a blog post planned for the Hello Fx39 share feature?
> 
> Per Whistler meeting, we will continue promoting new features on Hello.
> There will be a blog post which includes a mention of Hello share feature
> for 39. 
> 
> Draft of Hello portion:
> 
> "Today, we’re announcing that Firefox Share has been integrated into Hello.
> {SCREENSHOT}
> Hello beta, which we’ve been developing with our close partner, Telefonica,
> is the only in-browser video chat tool that doesn’t require an account or
> extra software downloads. You might recall that we recently added
> screensharing to Hello to make it easier to share anything you’re looking at
> in your conversation. Now you can also send a link to friends via the social
> network or email of your choice to invite them to a Hello video
> conversation."

Is "Firefox Share" a product name?
Agreed with jaws too, I don't think we referred to "Hello Beta" before, should we rather refer to "Firefox Hello"?

Comment 86

[Fabio Rios [:frios]]

(In reply to Romain Testard [:RT] from comment #85)
> (In reply to Fabio Rios [:frios] from comment #83)
> > (In reply to Romain Testard [:RT] from comment #79)
> > > [Why is this notable]: New feature
> > > [Suggested wording]: Share Hello URLs with social networks
> > > [Links (documentation, blog post, etc)]: We could (should?) have a blog post
> > > and a SUMO page for launch. 
> > > 
> > > Fabio - is there a blog post planned for the Hello Fx39 share feature?
> > 
> > Per Whistler meeting, we will continue promoting new features on Hello.
> > There will be a blog post which includes a mention of Hello share feature
> > for 39. 
> > 
> > Draft of Hello portion:
> > 
> > "Today, we’re announcing that Firefox Share has been integrated into Hello.
> > {SCREENSHOT}
> > Hello beta, which we’ve been developing with our close partner, Telefonica,
> > is the only in-browser video chat tool that doesn’t require an account or
> > extra software downloads. You might recall that we recently added
> > screensharing to Hello to make it easier to share anything you’re looking at
> > in your conversation. Now you can also send a link to friends via the social
> > network or email of your choice to invite them to a Hello video
> > conversation."
> 
> Agreed with jaws too, I don't think we referred to "Hello Beta" before,
> should we rather refer to "Firefox Hello"?

We want to make sure to call out that Hello is still in beta as it has a label. I agree with you guys we should say Firefox Hello, I'll get that updated. We can treat the beta label in a way that makes sense, perhaps italics. 

> Is "Firefox Share" a product name?
Yes, this is the name that has been used since first launch. I refer to it as "shareplane" internally.