Closed Bug 1014332 Opened 10 years ago Closed 10 years ago

enable share button by default with default selection of share providers

Categories

(Firefox Graveyard :: SocialAPI, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

Attachments

(8 files, 29 obsolete files)

840.60 KB, image/png
Details
7.09 MB, video/quicktime
Details
35.10 KB, patch
jaws
: review+
Details | Diff | Splinter Review
7.62 KB, patch
mixedpuppy
: review+
Details | Diff | Splinter Review
34.68 KB, patch
mixedpuppy
: review+
Details | Diff | Splinter Review
7.69 KB, patch
mixedpuppy
: review+
Details | Diff | Splinter Review
25.16 KB, patch
jaws
: review+
Details | Diff | Splinter Review
21.81 KB, patch
jaws
: review+
Details | Diff | Splinter Review
Attached video sharedefaults.mov (obsolete) —
The attached movie of the prototype probably explains this best, but here are top level bullet points.

- share button always visible/enabled (unless user customizes it away)
- there is a "+" button with a panel that lists providers
- providers are one-click activated from the panel, allowing users to quickly share
- a share provider may support more than share, but only share providers will appear in the share panel
- we may have a "most popular" or "sponsored" placement
- we want to show providers based on a users frecency to give them a selection of the most relevant providers for them

Intended implementation:

- panel will load a webpage from our directory (listed in the existing social.directories pref, currently activations.cdn.mozilla.net) that will contain the layout/design.  A link header in that page will contain the url to a json file on the directory (or perhaps we include the manifest in the page itself)

- firefox loads the json file, which contains a list of manifests, compares that against frecency, sends a dom event or port message to the page with the top 3 matches (that are not installed)

- since our initial list of share providers will be less than 20, this should be enough in the short term.  We can manage the json file to limit it to a shortlist of the most popular providers if the list grows large enough.
Looks great!

(In reply to Shane Caraveo (:mixedpuppy) from comment #0)
> Created attachment 8426697 [details]
> sharedefaults.mov
> 
> The attached movie of the prototype probably explains this best, but here
> are top level bullet points.
> 
> - share button always visible/enabled (unless user customizes it away)

Yes, if the user enabled the share button via a provider's website.  Otherwise, it would appear in the customize menu or Additional Tools menu. (discussed in person)

If the panel was enabled via a website, it would display only the selected provider, that provider's favicon, and a "+" to add other providers (#2 in the attached image).

> - we want to show providers based on a users frecency to give them a
> selection of the most relevant providers for them

This is ideal for as soon as it could happen.  Particularly for showing available providers in order of frecency score.
Flags: firefox-backlog+
Attached patch activation via share panel (obsolete) — Splinter Review
Initial run at patch for activating from the share panel.

- does not include ordering by frecency (I'm inclined to do that in a follow up, needs more thought)
- in the long run we'll probably only show 6 rather than the scrollable div (see updated video)
Assignee: nobody → mixedpuppy
Attachment #8434356 - Flags: feedback?(mhammond)
Attached video shareactivation.mov
new movie, moving towards panel design by boris.  this patch does not need to be blocked by the design of the panel since that is in a web page.
Attachment #8426697 - Attachment is obsolete: true
@boriss, see the movie then lets chat about some of the design differences.
Flags: needinfo?(jboriss)
Comment on attachment 8434356 [details] [diff] [review]
activation via share panel

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

Looks OK in general to me.  Not that keen on the inProductActivation stuff - it adds cognitive overload for reasons I don't understand.  browser-social is getting really quite complex these days, but sadly a way to help fix that doesn't leap out at me.  And as usual, "moar tests" ;)

::: browser/base/content/browser-social.js
@@ +246,3 @@
>          if (provider.postActivationURL) {
> +          // allow share above to work before opening a new tab
> +          setTimeout(() => {

I don't quite understand this - how do we know the share above actually worked?  Alot of what happens in sharePage seems async, so I'm not sure how a single run around the event loop ensures anything significant actually happened.

@@ +465,5 @@
>        return this.panel.lastChild;
>    },
>  
> +  get inProductActivation () {
> +    return Services.prefs.getBoolPref("social.inProductActivation");

what does this mean?  A comment explaining what inProductActivation actually means and why it is in a pref would be good (until now, I'd been assuming it was an attribute of how the share actually happened, not a user pref)

@@ +521,3 @@
>      if (!provider || !provider.shareURL) {
>        let providers = [p for (p of Social.providers) if (p.shareURL)];
>        provider = providers.length > 0  && providers[0];

this looks a little suspect - isn't provider going to be false instead of null in some cases here?

@@ +732,5 @@
>      iframe.setAttribute("origin", provider.origin);
>      iframe.setAttribute("src", shareEndpoint);
> +    this._openPanel();
> +  },
> +  

few trailing spaces

::: browser/base/content/browser.xul
@@ +247,4 @@
>        <vbox class="social-share-toolbar">
> +        <arrowscrollbox id="social-share-provider-buttons" orient="vertical" flex="1">
> +          <toolbarbutton id="add-share-provider" class="toolbarbutton share-provider-button" type="radio"
> +                         group="share-providers" tooltiptext="Find more Share services..."

obviously you will want real strings here (I'm sure that's just because this is a WIP, but seeing you didn't mention it, I did ;)

::: modules/libpref/src/init/all.js
@@ +3979,5 @@
>  // comma separated list of domain origins (e.g. https://domain.com) for
>  // directory websites (e.g. AMO) that can install providers for other sites
>  pref("social.directories", "https://activations.cdn.mozilla.net");
> +// activation from inside of Firefox is possible if inProductActivation is true
> +pref("social.inProductActivation", true);

I really don't understand this.  We pass the value around even though it comes from a pref which seems strange, and there's no UI to toggle this value, so it looks like we expect this to always be true.  Why does it exist at all?

::: toolkit/components/social/SocialService.jsm
@@ +664,1 @@
>        case "whitelist":

please add a comment here saying you are intentionally falling though (it wasn't really necessary before as there was no code, so it was somewhat obvious)
Attachment #8434356 - Flags: feedback?(mhammond) → feedback+
(In reply to Shane Caraveo (:mixedpuppy) from comment #4)
> @boriss, see the movie then lets chat about some of the design differences.

It's really coming along!  We do need final pixel-perfect measurements and fonts - would a spec with all of those measured be helpful?  It'd essentially be a description of what's in the attachment https://bug1014332.bugzilla.mozilla.org/attachment.cgi?id=8431210
Flags: needinfo?(jboriss)
Whiteboard: p=5
(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #6)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #4)
> > @boriss, see the movie then lets chat about some of the design differences.
> 
> It's really coming along!  We do need final pixel-perfect measurements and
> fonts - would a spec with all of those measured be helpful?  It'd
> essentially be a description of what's in the attachment
> https://bug1014332.bugzilla.mozilla.org/attachment.cgi?id=8431210

Yeah, that would be helpful.
Flags: needinfo?(jboriss)
(In reply to [:markh away until August 18] Mark Hammond from comment #5)
> Comment on attachment 8434356 [details] [diff] [review]
> activation via share panel
> 
> Review of attachment 8434356 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks OK in general to me.  Not that keen on the inProductActivation stuff -
> it adds cognitive overload for reasons I don't understand.  browser-social
> is getting really quite complex these days, but sadly a way to help fix that
> doesn't leap out at me.  And as usual, "moar tests" ;)

"inProductActivation" is a flag to bypass the enable panel when activating from within firefox.  I'm moving much of that logic over to bug 1029942.  The share panel will take advantage of this as well, but I was requested to have the ability to pref it off as well.  We may push this up the train a bit, but keep it pref'd off for releases.

> ::: browser/base/content/browser-social.js
> @@ +246,3 @@
> >          if (provider.postActivationURL) {
> > +          // allow share above to work before opening a new tab
> > +          setTimeout(() => {
> 
> I don't quite understand this - how do we know the share above actually
> worked?  Alot of what happens in sharePage seems async, so I'm not sure how
> a single run around the event loop ensures anything significant actually
> happened.

The initial part of getting the current page data isn't async (ie. calling into OpenGraphBuilder), and that is the part I need to happen prior to opening the new tab.  I do need to examine this more.

> @@ +465,5 @@
> >        return this.panel.lastChild;
> >    },
> >  
> > +  get inProductActivation () {
> > +    return Services.prefs.getBoolPref("social.inProductActivation");
> 
> what does this mean?  A comment explaining what inProductActivation actually
> means and why it is in a pref would be good (until now, I'd been assuming it
> was an attribute of how the share actually happened, not a user pref)

I'll add a comment per my response on this above.

> @@ +521,3 @@
> >      if (!provider || !provider.shareURL) {
> >        let providers = [p for (p of Social.providers) if (p.shareURL)];
> >        provider = providers.length > 0  && providers[0];
> 
> this looks a little suspect - isn't provider going to be false instead of
> null in some cases here?

That could be changed, but isn't a part the changes in this patch.

> ::: browser/base/content/browser.xul
> @@ +247,4 @@
> >        <vbox class="social-share-toolbar">
> > +        <arrowscrollbox id="social-share-provider-buttons" orient="vertical" flex="1">
> > +          <toolbarbutton id="add-share-provider" class="toolbarbutton share-provider-button" type="radio"
> > +                         group="share-providers" tooltiptext="Find more Share services..."
> 
> obviously you will want real strings here (I'm sure that's just because this
> is a WIP, but seeing you didn't mention it, I did ;)

correct.

> ::: modules/libpref/src/init/all.js
> @@ +3979,5 @@
> >  // comma separated list of domain origins (e.g. https://domain.com) for
> >  // directory websites (e.g. AMO) that can install providers for other sites
> >  pref("social.directories", "https://activations.cdn.mozilla.net");
> > +// activation from inside of Firefox is possible if inProductActivation is true
> > +pref("social.inProductActivation", true);
> 
> I really don't understand this.  We pass the value around even though it
> comes from a pref which seems strange, and there's no UI to toggle this
> value, so it looks like we expect this to always be true.  Why does it exist
> at all?

refer to my comment above
Attached patch activation via share panel (obsolete) — Splinter Review
addresses feedback from previous f+.  tests still to come.
Attachment #8434356 - Attachment is obsolete: true
Attachment #8446264 - Flags: feedback?(mhammond)
Attachment #8446264 - Flags: feedback?(felipc)
We want this to hit for Fx33 and uplift for 32 so we can do some user testing during beta cycle for 32.
Attached patch activation via share panel (obsolete) — Splinter Review
now with tests
Attachment #8446264 - Attachment is obsolete: true
Attachment #8446264 - Flags: feedback?(mhammond)
Attachment #8446264 - Flags: feedback?(felipc)
Attachment #8454007 - Flags: review?(felipc)
not for 32.
Attached file activation via share panel (obsolete) —
adds:
- some refactoring of activation bypass code from bug 1029942
- simplification of install code in SocialService._installProvider

https://tbpl.mozilla.org/?tree=Try&rev=cef3a22276b8
Attachment #8454007 - Attachment is obsolete: true
Attachment #8454007 - Flags: review?(felipc)
Attachment #8458940 - Flags: review?(felipc)
Attachment #8458940 - Attachment is obsolete: true
Attachment #8458940 - Flags: review?(felipc)
This second part of the patch has a one line change in customization, then all the fallout from test changes required by that one line.

https://tbpl.mozilla.org/?tree=Try&rev=c26fc0160aaa
Attachment #8465534 - Flags: review?(jaws)
Comment on attachment 8465529 [details] [diff] [review]
part 1: activation via share panel

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

r- mainly for the HiDPI and the tests getting disabled if the feature gets disabled. Otherwise the other issues seem mild.

::: browser/base/content/aboutSocialError.xhtml
@@ +46,5 @@
>        let queryString = url.replace(/^about:socialerror\??/, "");
>  
>        let modeMatch = queryString.match(/mode=([^&]+)/);
>        let mode = modeMatch && modeMatch[1] ? modeMatch[1] : "";
> +      let directoryMatch = queryString.match(/directory=([^&]+)/);

Please use URLSearchParams instead of regular expressions.

::: browser/base/content/browser-social.js
@@ +305,5 @@
>  
>    // called on tab/urlbar/location changes and after customization. Update
>    // anything that is tab specific.
>    updateState: function() {
> +    SocialShare.update();

Have you looked at how this might affect TART scores?

@@ +542,5 @@
>      let providers = [p for (p of Social.providers) if (p.shareURL)];
>      let hbox = document.getElementById("social-share-provider-buttons");
>      // selectable providers are inserted before the provider-menu seperator,
>      // remove any menuitems in that area
> +    while (hbox.firstChild != hbox.lastChild) {

This will now leave one menuitem behind whereas the previous code removed all children. The associated comment is now either out of date or is pointing out a bug in this code change.

@@ +563,3 @@
>      }
>      if (!this.defaultButton) {
> +      this.defaultButton = this.activationPanelEnabled ? hbox.lastChild : hbox.firstChild;

It would be nice to select these by id or className so that it is less fragile if someone adds another element at the beginning or end of the hbox' childNodes.

@@ +615,5 @@
>      }
> +
> +    // enable or disable the activation panel
> +    let hbox = document.getElementById("social-share-provider-buttons");
> +    hbox.lastChild.hidden = !this.activationPanelEnabled;

Same comment here about lastChild being somewhat ambiguous/fragile.

@@ +707,5 @@
> +      return;
> +    }
> +    // check the menu button
> +    let hbox = document.getElementById("social-share-provider-buttons");
> +    let btn = hbox.querySelector("[origin='"+provider.origin+"']");

nit: space around binary operators (`+` in this case)

@@ +792,5 @@
> +
> +    }, true);
> +    this._openPanel();
> +  },
> +  

nit, whitespace here and in browser_share.js

::: browser/base/content/browser.xul
@@ -243,5 @@
>             type="arrow"
>             orient="horizontal"
>             onpopupshowing="SocialShare.onShowing()"
> -           onpopuphidden="SocialShare.onHidden()"
> -           hidden="true">

This panel should remain hidden by default for perf reasons. You can unhide this panel before it is about to be shown for the first time.

::: browser/base/content/test/social/browser_share.js
@@ -109,5 @@
>        is(SocialUI.enabled, true, "SocialUI is enabled");
>        checkSocialUI();
>        // share should not be enabled since we only have about:blank page
>        let shareButton = SocialShare.shareButton;
> -      is(shareButton.disabled, true, "share button is disabled");

is the share button always enabled now? what about on pages such as about:home?

@@ +277,5 @@
> +  },
> +  testSharePanelActivation: function(next) {
> +    let activationPanelEnabled = Services.prefs.getBoolPref("social.share.activationPanelEnabled");
> +    if (!activationPanelEnabled) {
> +      //skipping this test if activation is not enabled

Can the test enable activation? I worry that if it gets disabled it will break without any notice since the tests will stop running.

::: browser/base/content/test/social/share_activate.html
@@ +1,1 @@
> +<html>

This should have a license header.

@@ +1,3 @@
> +<html>
> +<head>
> +	<title>Activation test</title>

replace this <tab> character with spaces.

::: browser/themes/shared/menupanel.inc.css
@@ +184,5 @@
>    -moz-image-region: rect(0px, 96px, 16px, 80px);
>  }
> +
> +#add-share-provider {
> +  list-style-image: url(chrome://browser/skin/menuPanel-small.png);

HiDPI?

::: modules/libpref/src/init/all.js
@@ +4086,5 @@
> +pref("social.share.activationPanelEnabled", false);
> +#else
> +pref("social.share.activationPanelEnabled", true);
> +#endif
> +pref("social.shareDirectory", "https://activations.cdn.mozilla.net/en-US/sharePanel.html");

this is not the fault of this patch, but these prefs should be in firefox.js not in all.js since they don't make sense for non-Firefox applications. can you please file a bug to move these prefs?

::: toolkit/components/social/SocialService.jsm
@@ +621,5 @@
>  
> +  _installProvider: function(aDOMDocument, manifest, options, installCallback) {
> +    // a manifest is required, we'll catch a missing manifest below.
> +    if (!manifest)
> +      throw new Error("Cannot install provider without manifest data");

This comment seems redundant.
Attachment #8465529 - Flags: review-
Comment on attachment 8465534 [details] [diff] [review]
part 2: activation via share panel

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

::: browser/components/customizableui/test/browser_876944_customize_mode_create_destroy.js
@@ +26,5 @@
>  
>  // Creating and destroying a widget should correctly deal with panel placeholders
>  add_task(function testPanelPlaceholders() {
>    let panel = document.getElementById(CustomizableUI.AREA_PANEL);
> +  is(panel.querySelectorAll(".panel-customization-placeholder").length, isInWin8() ? 3 : 1, "The number of placeholders should be correct.");

Can you please file a bug to rename isInWin8() to isMetroSupportedAndEnabled() for the customizableui tests?

::: browser/components/customizableui/test/browser_880382_drag_wide_widgets_in_panel.js
@@ +457,3 @@
>    addSwitchToMetroButtonInWindows8(placementsAfterMove);
> +  if (isInWin8()) {
> +    placementsAfterMove.splice(10, 1);

Did you run these with a metro-enabled build? Do you know what the plans are for keeping the Metro code in-tree? Could this code just be removed?

::: browser/components/customizableui/test/browser_890140_orphaned_placeholders.js
@@ +92,5 @@
>    is(getVisiblePlaceholderCount(panel), 1, "Should only have 1 visible placeholder after re-entering");
>  
> +  for (let i in buttonsToMove) {
> +    CustomizableUI.addWidgetToArea(buttonsToMove[i], CustomizableUI.AREA_PANEL);
> +    

nit, whitespace

@@ +138,5 @@
>    simulateItemDrag(btn, zoomControls);
>    ok(CustomizableUI.inDefaultState, "Should be in default state again.");
>  });
>  
> +// The default placements should have one placeholder at the bottom (or 3 in win8).

s/3 in win8/3 in metro-enabled win8/
Attachment #8465534 - Flags: review?(jaws) → review+
> Can you please file a bug to rename isInWin8() to
> isMetroSupportedAndEnabled() for the customizableui tests?

bug 1047503

> :::
> browser/components/customizableui/test/
> browser_880382_drag_wide_widgets_in_panel.js
> @@ +457,3 @@
> >    addSwitchToMetroButtonInWindows8(placementsAfterMove);
> > +  if (isInWin8()) {
> > +    placementsAfterMove.splice(10, 1);
> 
> Did you run these with a metro-enabled build? Do you know what the plans are
> for keeping the Metro code in-tree? Could this code just be removed?

I'll run a full try after updating the patches that includes mochitest-metro-chrome.
> ::: browser/base/content/browser-social.js
> @@ +305,5 @@
> >  
> >    // called on tab/urlbar/location changes and after customization. Update
> >    // anything that is tab specific.
> >    updateState: function() {
> > +    SocialShare.update();
> 
> Have you looked at how this might affect TART scores?

running a full try.  This is called during onLocationChange.

> @@ +542,5 @@
> >      let providers = [p for (p of Social.providers) if (p.shareURL)];
> >      let hbox = document.getElementById("social-share-provider-buttons");
> >      // selectable providers are inserted before the provider-menu seperator,
> >      // remove any menuitems in that area
> > +    while (hbox.firstChild != hbox.lastChild) {
> 
> This will now leave one menuitem behind whereas the previous code removed
> all children. The associated comment is now either out of date or is
> pointing out a bug in this code change.

it was out of date

> @@ +563,3 @@
> >      }
> >      if (!this.defaultButton) {
> > +      this.defaultButton = this.activationPanelEnabled ? hbox.lastChild : hbox.firstChild;
> 
> It would be nice to select these by id or className so that it is less
> fragile if someone adds another element at the beginning or end of the hbox'
> childNodes.

I've changed this somewhat, though we purposely add provider buttons before lastChild.  lastChild is the panel for adding new providers.

> ::: browser/base/content/test/social/browser_share.js
> @@ -109,5 @@
> >        is(SocialUI.enabled, true, "SocialUI is enabled");
> >        checkSocialUI();
> >        // share should not be enabled since we only have about:blank page
> >        let shareButton = SocialShare.shareButton;
> > -      is(shareButton.disabled, true, "share button is disabled");
> 
> is the share button always enabled now? what about on pages such as
> about:home?

The disabled attribute is no longer used, we only set the attribute, and that is checked in this test as well as others.

> @@ +277,5 @@
> > +  },
> > +  testSharePanelActivation: function(next) {
> > +    let activationPanelEnabled = Services.prefs.getBoolPref("social.share.activationPanelEnabled");
> > +    if (!activationPanelEnabled) {
> > +      //skipping this test if activation is not enabled
> 
> Can the test enable activation? I worry that if it gets disabled it will
> break without any notice since the tests will stop running.

I changed that so it always runs the test now.

> ::: browser/base/content/test/social/share_activate.html
> @@ +1,1 @@
> > +<html>
> 
> This should have a license header.

Did that, but no other html file in the browser test dir has a license header.



> HiDPI?

added.

> ::: modules/libpref/src/init/all.js
> @@ +4086,5 @@
> > +pref("social.share.activationPanelEnabled", false);
> > +#else
> > +pref("social.share.activationPanelEnabled", true);
> > +#endif
> > +pref("social.shareDirectory", "https://activations.cdn.mozilla.net/en-US/sharePanel.html");
> 
> this is not the fault of this patch, but these prefs should be in firefox.js
> not in all.js since they don't make sense for non-Firefox applications. can
> you please file a bug to move these prefs?

moved these changes to firefox.js.  The other (preexisting) prefs are necessary for toolkit xpcshell tests to run.
feedback added, carry forward r+
Attachment #8465534 - Attachment is obsolete: true
Attachment #8466507 - Flags: review+
This covers everything except the question on the TART test.  I'm running a full try, but am going to look and see if there is a better way to handle the initial update for the button (thus only f?)
Attachment #8465529 - Attachment is obsolete: true
Attachment #8466509 - Flags: feedback?(jaws)
Attached patch part 3: fix context menu tests (obsolete) — Splinter Review
fixes mochitest-plain tests for the context menu
Attachment #8468052 - Flags: review?(jaws)
Attached patch part 3: fix context menu tests (obsolete) — Splinter Review
remove some unintentional changes in previous version
Attachment #8468052 - Attachment is obsolete: true
Attachment #8468052 - Flags: review?(jaws)
Attachment #8468076 - Flags: review?(jaws)
Comment on attachment 8466509 [details] [diff] [review]
part 1: activation via share panel

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

granted feedback, but held off on review due to response about TART as well as RELEASE_BUILD question for you.

::: browser/app/profile/firefox.js
@@ +1586,5 @@
>  pref("social.sidebar.unload_timeout_ms", 10000);
>  
> +// activation from inside of share panel is possible if activationPanelEnabled
> +// is true. Pref'd off for release while usage testing is done through beta.
> +#ifdef RELEASE_BUILD

RELEASE_BUILD is true for beta, see http://mxr.mozilla.org/mozilla-central/source/js/src/configure.in#2713. 

Maybe I am reading this comment wrong, but it seems to imply that you want testing in Nightly, Aurora, AND Beta, and to only disable the feature in Release. If that is your goal, then this will not work as you intend it to.

::: browser/base/content/aboutSocialError.xhtml
@@ +47,5 @@
>  
> +      let mode = searchParams.get('mode');
> +      config.directory = searchParams.get('directory');
> +      config.origin = searchParams.get('origin');
> +      let encodedURL = searchParams.get('url');

nit, double quotes.

::: browser/base/content/test/social/browser_share.js
@@ +38,5 @@
> +    if (iframe.contentDocument.location.href != "data:text/plain;charset=utf8,") {
> +      iframe.removeEventListener(eventName, load, true);
> +      executeSoon(callback);
> +    }
> +  }, true);  

nit, same trailing whitespace from previous review
Attachment #8466509 - Flags: feedback?(jaws) → feedback+
Comment on attachment 8468076 [details] [diff] [review]
part 3: fix context menu tests

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

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +144,5 @@
>  <!ENTITY sharePageCmd.label "Share This Page">
>  <!ENTITY sharePageCmd.commandkey "S">
>  <!ENTITY sharePageCmd.accesskey "s">
>  <!ENTITY shareLinkCmd.label "Share This Link">
> +<!ENTITY shareLinkCmd.accesskey "h">

Note, I'm not asking for these entity name to be revved because nothing fundamental has changed with what menuitems can be shown at the same time. This patch only makes it so that share-page can be shown when right-clicking on a canvas element, so it was possible before that share-page and share-image would be shown at the same time already.
Attachment #8468076 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #27)
> Comment on attachment 8468076 [details] [diff] [review]
> part 3: fix context menu tests
> 
> Review of attachment 8468076 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/locales/en-US/chrome/browser/browser.dtd
> @@ +144,5 @@
> >  <!ENTITY sharePageCmd.label "Share This Page">
> >  <!ENTITY sharePageCmd.commandkey "S">
> >  <!ENTITY sharePageCmd.accesskey "s">
> >  <!ENTITY shareLinkCmd.label "Share This Link">
> > +<!ENTITY shareLinkCmd.accesskey "h">
> 
> Note, I'm not asking for these entity name to be revved because nothing
> fundamental has changed with what menuitems can be shown at the same time.
> This patch only makes it so that share-page can be shown when right-clicking
> on a canvas element, so it was possible before that share-page and
> share-image would be shown at the same time already.

Actually, it is making it so share is *not* available on canvas, it was before.  There is no way to share canvas data right now, so no point in allowing it.  The test breakage uncovered that we were allowing share on canvas.  Access key changes are due to conflicts (also uncovered by the tests now that share is visible by default) with other menu items.
Ah, thanks for the correction.
Comment on attachment 8466509 [details] [diff] [review]
part 1: activation via share panel

no tart regressions appearing, @jaws gave r+ via irc

http://compare-talos.mattn.ca/?oldRevs=cd1a49c0ad14&newRev=5faa17b0024e&server=graphs.mozilla.org&submit=true

Will land with RELEASE_BUILD flags and follow up in bug 1052531 since we really want to run this on beta.  Will also follow up with a new patch for other minor feedback from jaws.
Attachment #8466509 - Flags: review+
share panel used will go live when pushed via bug 1052591
Depends on: 1052591
https://hg.mozilla.org/mozilla-central/rev/4dbbffc0b5cd
https://hg.mozilla.org/mozilla-central/rev/753ebd9be952
https://hg.mozilla.org/mozilla-central/rev/ac87dabb3890
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Depends on: 1052762
Backed out for causing bug 1052762. Sorry I didn't catch this earlier - got burned by coalescing :(

https://hg.mozilla.org/mozilla-central/rev/413e60042ff1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 34 → ---
Attached patch some test cleanups (obsolete) — Splinter Review
while trying to figure out why one plat was failing, I noticed some js errors and cleaned up the causes of them.  Seems to have fixed the failure these patches ran into.

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

full try running now:

https://tbpl.mozilla.org/?tree=Try&rev=0a838734d550
Attachment #8473408 - Flags: review?(jaws)
Comment on attachment 8473408 [details] [diff] [review]
some test cleanups

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

::: browser/base/content/test/social/browser_social_chatwindow.js
@@ +263,5 @@
>                                     function() {
>                                      ok(!chats.selectedChat, "multiprovider chats are all closed");
> +                                    port0.close();
> +                                    port1.close();
> +                                    port2.close();

for posterity, the ordering of the closing of the ports here doesn't matter, but they do need to stay open until the test finishes

::: browser/base/content/test/social/browser_social_errorPage.js
@@ +38,5 @@
>    SocialFlyout.open(url, 0, panelCallback);
> +  // wait for both open and loaded before callback. Since the test doesn't close
> +  // the panel between opens, we cannot rely on events here. We need to ensure
> +  // popupshown happens before we finish out the tests.
> +  waitForCondition(function() SocialFlyout.panel.state == "open" &&

nit, please use a full function() { return ... } here because this one is now two lines
Attachment #8473408 - Flags: review?(jaws) → review+
(In reply to Shane Caraveo (:mixedpuppy) from comment #35)
> while trying to figure out why one plat was failing, I noticed some js
> errors and cleaned up the causes of them.  Seems to have fixed the failure
> these patches ran into.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=2de5a6dd557b

https://tbpl.mozilla.org/php/getParsedLog.php?id=45992703&tree=Try is the same as bug 1052762, no?
Depends on: 1054650
https://hg.mozilla.org/mozilla-central/rev/b3632bd4c076
https://hg.mozilla.org/mozilla-central/rev/6aee7a646f7e
https://hg.mozilla.org/mozilla-central/rev/93cd3fc35cfc
https://hg.mozilla.org/mozilla-central/rev/bfe0d5c41edd

Hopefully the new intermittents get attention reasonably soon. If they prove to be too frequent once the volume picks up next week, I'll back this out again otherwise.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Points: --- → 5
Whiteboard: p=5
Depends on: 1055586
Depends on: 1056527
Depends on: 1054792
Blocks: 1055590
Comment 42 was the backout.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 34 → ---
(In reply to Ed Morley [:edmorley] from comment #41)
> Sorry, had to revert this for causing the top orange bug 1052762 and also
> bug 1054792:

Also bug 968887
Shouldn't this have a migration for existing profiles so the button moves to the panel there, too?
Depends on: 1060038
Finally got green and fixed the CART regression: 

https://tbpl.mozilla.org/?tree=Try&rev=aa75a64ba378 
http://compare-talos.mattn.ca/?oldRevs=88fc6b3f36a0&newRev=17e8389e546b&server=graphs.mozilla.org&submit=true

The new patches have a number of changes requiring re-review, I'll outline the changes first (before uploading).

- changed share button from xul element to a CUI widget and introduced a broadcaster/observer to handle the button disabled state.  This allows the removal of any socialapi code during tabswitch or menu panel opening.  That helped to address the CART regression[1].  Some changes with the marks button also appears in the patch now, taking advantage of the same broadcaster.

- because the share button is now a CUI widget and is always visible, bug 1055586 is no longer an issue.

- the activationPanelEnabled pref is now specific to allowing activations from the share panel (rather than also controlling if the share button was visible, etc).  If it is false, a default about:providerdirectory page is shown with a link to the directory.  If true, a frame is loaded with providers that can be directly activated.  This removes all the show/hide logic which also contributed to the CART issue and simplifies some of the logic there.

- handled bug 1056527 (removal of socialapi buttons from nav-bar defaultset)

- removed worker from errorPage tests and added async waiting on offline/online changes, the combination fixed several oranges, most prominently bug 1052762 and bug 1054650 (the errorPage test failures).

- presumably bug 1054792 is no longer an issue from this patch.  I was getting it in earlier tries with less re-runs of bc1, but it is much more rare.

[1] The cart regression was caused by socialapi doing a bunch of state detection, showing/hidding/etc during tab switching.  When entering customization, a tab switch is performed.  We were already doing a bunch there, but the original patches added even more.

Since all the errors were happening on linux, I was only testing against that.  A new try for all platforms is running here: https://tbpl.mozilla.org/?tree=Try&rev=6af7c818b0bf
Attachment #8471728 - Attachment is obsolete: true
Attachment #8482432 - Flags: review?(jaws)
Attachment #8466507 - Attachment is obsolete: true
Attached patch part 3: fix context menu tests (obsolete) — Splinter Review
Attachment #8468076 - Attachment is obsolete: true
Attachment #8482434 - Flags: review?(jaws)
Attached patch partf 4: async test cleanups (obsolete) — Splinter Review
Attachment #8473899 - Attachment is obsolete: true
Attachment #8482435 - Flags: review?(jaws)
Attachment #8482433 - Flags: review?(jaws)
Depends on: 968887
Comment on attachment 8482432 [details] [diff] [review]
part 1: share button widget and activation via share panelsharepage

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

I've looked through most of this patch but it is just too big to wrap my head around all of the changes. Now that you know what you'd like to do in order to complete this bug, can you go back and break this patch apart into separate steps that are necessary to complete it? I'm curious if the act of doing so will reveal some changes that weren't necessary or that could be done more concisely.
Attachment #8482432 - Flags: review?(jaws) → review-
Comment on attachment 8482433 [details] [diff] [review]
part2: CUI test fixes after moving share button to menu-panel

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

::: browser/base/content/test/social/browser_share.js
@@ +148,5 @@
> +      });
> +
> +      PanelUI.panel.addEventListener("popuphidden", function onpopupshown() {
> +        PanelUI.panel.removeEventListener("popuphidden", onpopupshown);
> +  

nit, whitespace here and elsewhere

::: browser/components/customizableui/test/browser_880382_drag_wide_widgets_in_panel.js
@@ +22,5 @@
>                               "find-button",
>                               "preferences-button",
>                               "add-ons-button",
> +                             "developer-button",
> +                             "social-share-button"];

Should 'Share' be the last button on in the grid? I think we should talk with UX to make sure that this button is added in the best spot.
Attachment #8482433 - Flags: review?(jaws) → review+
Comment on attachment 8482434 [details] [diff] [review]
part 3: fix context menu tests

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

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +144,5 @@
>  <!ENTITY sharePageCmd.label "Share This Page">
>  <!ENTITY sharePageCmd.commandkey "S">
>  <!ENTITY sharePageCmd.accesskey "s">
>  <!ENTITY shareLinkCmd.label "Share This Link">
> +<!ENTITY shareLinkCmd.accesskey "h">

Can you change the string IDs for the labels and accesskeys (since they need to be consistent) and add a localizer note that the shareLinkCmd should have a different accesskey from the shareImageCmd, shareSelectCmd, and shareVideoCmd?
Attachment #8482434 - Flags: review?(jaws) → review+
Comment on attachment 8482435 [details] [diff] [review]
partf 4: async test cleanups

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

r+ with the below issues addressed.

::: browser/base/content/test/social/browser_social_chatwindow.js
@@ +197,5 @@
>            // chat to appear, and thus become selected.
>            chatbar.selectedChat.close();
>            is(chatbar.selectedChat, second, "second chat is selected");
>            closeAllChats();
> +          port.close();

if this test throws before this line, the port will leak. can you change this so that the port is guaranteed to close?

::: browser/base/content/test/social/browser_social_errorPage.js
@@ +21,5 @@
> +  if (!goOffline) {
> +    Services.prefs.setIntPref('network.proxy.type', origProxyType);
> +  }
> +  if (goOffline != Services.io.offline) {
> +    info("initial offline state "+Services.io.offline);

nit, spaces around binary operators

@@ +27,5 @@
> +    Services.obs.addObserver(function offlineChange(subject, topic, data) {
> +      Services.obs.removeObserver(offlineChange, "network:offline-status-changed");
> +      info("offline state changed to "+Services.io.offline);
> +      is(expect, Services.io.offline, "network:offline-status-changed successful toggle");
> +      deferred.resolve(true);

The `true` here, and below, isn't necessary since there is no code that stores the result of the yield in this test.

@@ +150,5 @@
>                panelCallbackCount++;
>              },
>              function() { // the "load" callback.
> +              todo_is(panelCallbackCount, 0, "Bug 833207 - should be no callback when error page loads.");
> +              ok(panel.firstChild.contentDocument.location.href.indexOf("about:socialerror?")==0, "flyout is on social error page");

let href = panel.firstChild.contentDocument.location.href;
is(href.indexOf("about:socialerror?"), 0, "flyout is on social error page");
Attachment #8482435 - Flags: review?(jaws) → review+
This patch provides all necessary changes to add a "+" panel to the share panel and have it show about:providerdirectory.  This about page will alternately load an iframe from the directory site in the preference if enabled, otherwise it shows a default page with a link to the directory site.  This alone does not enable activation from the panel.
Attachment #8482432 - Attachment is obsolete: true
Attachment #8484579 - Flags: review?(jaws)
Attachment #8484582 - Flags: review?(jaws)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #51)

> I've looked through most of this patch but it is just too big to wrap my
> head around all of the changes. Now that you know what you'd like to do in
> order to complete this bug, can you go back and break this patch apart into
> separate steps that are necessary to complete it? I'm curious if the act of
> doing so will reveal some changes that weren't necessary or that could be
> done more concisely.

I've tried to split it out as logically as I could, there are now 4 smaller patches.  I'm not 100% certain that there are not some minor items that may be mixed between the two, but nothing obvious sticking out that I see.  They apply in order of the patch #.
Comment on attachment 8484582 [details] [diff] [review]
part 1.3: make the share button a CUI widget

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

The patch mostly looks fine, but the patch will need to be changed based on the feedback from Madhava.

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +393,5 @@
>    }, {
> +    id: "social-share-button",
> +    tooltiptext: "social-share-button.label",
> +    label: "social-share-button.tooltiptext",
> +    defaultArea: CustomizableUI.AREA_PANEL,

I talked with Madhava and he said that we should not place the button in the panel by default. He would like to put the button in the customization palette first and then run some experiments to see if it gets enough traction in the palette or if it should be added to the panel/toolbar.
Attachment #8484582 - Flags: review?(jaws) → review-
Attachment #8484579 - Attachment is patch: true
Attachment #8484579 - Attachment mime type: text/x-patch → text/plain
Comment on attachment 8484579 [details] [diff] [review]
part 1.1: about:providerdirectory page showing in share panel

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

::: browser/base/content/aboutProviderDirectory.xhtml
@@ +32,5 @@
> +    </div>
> +    <div id="activation" hidden="true">
> +      <p>&social.directory.intro.text;</p>
> +      <div><iframe id="activation-frame"/></div>
> +      <p class="link" onclick="openDirectory()">&social.directory.viewmore.text;</p>

This should be using an <a> instead of a <p>.

@@ +42,5 @@
> +
> +    Cu.import("resource://gre/modules/Services.jsm");
> +
> +    function openDirectory() {
> +      let url = Services.prefs.getCharPref("social.directories").split(',')[0];

When will social.directories have more than one entry? Why are we handling comma-separated values here, I don't see the need for it elsewhere in the patches.

::: browser/base/content/browser-social.js
@@ +659,5 @@
> +    }
> +    // check the menu button
> +    let hbox = document.getElementById("social-share-provider-buttons");
> +    let btn = hbox.querySelector("[origin='" + provider.origin + "']");
> +    btn.checked = true;

Please add a null check here for btn.

@@ +739,5 @@
> +        iframe.removeEventListener("unload", panelBrowserOnload, true);
> +        SocialShare._dynamicResizer.stop();
> +      }, true);
> +
> +    }, true);

Please remove this blank line.

::: browser/components/about/AboutRedirector.cpp
@@ +48,5 @@
>      nsIAboutModule::ALLOW_SCRIPT |
>      nsIAboutModule::HIDE_FROM_ABOUTABOUT },
> +  { "providerdirectory", "chrome://browser/content/aboutProviderDirectory.xhtml",
> +    nsIAboutModule::ALLOW_SCRIPT |
> +    nsIAboutModule::HIDE_FROM_ABOUTABOUT },

This needs a security review since it is adding an entry to this file that does not specify URI_SAFE_FOR_UNTRUSTED_CONTENT.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +139,5 @@
>  <!ENTITY bookmarkThisPageCmd.label "Bookmark This Page">
>  <!ENTITY editThisBookmarkCmd.label "Edit This Bookmark">
>  <!ENTITY bookmarkThisPageCmd.commandkey "d">
>  <!ENTITY markPageCmd.commandkey "l">
> +<!ENTITY findShareServices.label "Find more Share services…">

Please add a localization note about the ellipsis.

@@ +691,5 @@
>  <!ENTITY social.learnMore.accesskey "l">
>  <!ENTITY social.closeNotificationItem.label "Not Now">
>  
> +<!ENTITY social.directory.label "Activations Directory">
> +<!ENTITY social.directory.text "You can activate share services from the directory.">

Please capitalize Share here.

@@ +693,5 @@
>  
> +<!ENTITY social.directory.label "Activations Directory">
> +<!ENTITY social.directory.text "You can activate share services from the directory.">
> +<!ENTITY social.directory.button "Take me there!">
> +<!ENTITY social.directory.intro.text "Click on a service to add it to Firefox.">

This needs to use &brandShortName;

::: browser/themes/shared/aboutProviderDirectory.css
@@ +68,5 @@
> +  box-shadow: 0 1px 1px hsla(210,15%,25%,.2) inset,
> +              0 0 2px hsla(210,15%,25%,.4) inset;
> +  transition-property: background-color, border-color, box-shadow;
> +  transition-duration: 10ms;
> +  transition-timing-function: linear;

Why are we using `ease` for exiting :hover and :hover:active, `ease` for entering :hover, but `linear` for entering :hover:active? Also, `ease` is the initial value, so it does not need to be specified unless it is being used to override a previously applied value.

@@ +86,5 @@
> +  font-size: .8em;
> +}
> +.link {
> +  text-decoration: none;
> +  color: blue;

`blue` is not the color that we use for links, and this will break with high-contrast environments.
Attachment #8484579 - Flags: review?(jaws) → review-
Attachment #8484581 - Attachment is patch: true
Attachment #8484581 - Attachment mime type: text/x-patch → text/plain
Comment on attachment 8484581 [details] [diff] [review]
part 1.2: error page handling for about:providerdirectory

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

::: browser/base/content/aboutSocialError.xhtml
@@ +41,5 @@
>        tryAgainCallback: reloadProvider
>      }
>  
>      function parseQueryString() {
> +      var searchParams = new URLSearchParams(location.href.split("?")[1]);

nit, please use `let` here. Would probably be better to use location.search and trim off the beginning question mark. Also, you should lowercase this since URLSearchParams is case-sensitive.
Attachment #8484581 - Flags: review?(jaws) → review+
Comment on attachment 8484583 [details] [diff] [review]
part 1.4: enable activating providers from the new share panel

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

::: browser/base/content/test/social/browser_share.js
@@ +11,5 @@
>    shareURL: "https://example.com/browser/browser/base/content/test/social/share.html"
>  };
> +let activationPage = "https://example.com/browser/browser/base/content/test/social/share_activate.html";
> +
> +function waitForProviderEnabled(cb) {

Please use a promise here. We have a promiseObserverNotified function in customizableui/tests/head.js that you can use.

@@ +33,5 @@
> +}
> +
> +function waitForEvent(iframe, eventName, callback) {
> +  iframe.addEventListener(eventName, function load() {
> +    info("page load is "+iframe.contentDocument.location.href);

nit, spaces around binary operators

@@ +283,5 @@
> +    // make the iframe so we can wait on the load
> +    SocialShare._createFrame();
> +    let iframe = SocialShare.iframe;
> +
> +    waitForEvent(iframe, "load", () => {

New tests should be using promises since they are easier to read and won't introduce so much code nesting.

::: toolkit/components/social/SocialService.jsm
@@ -654,5 @@
> -        installer = new AddonInstaller(sourceURI, manifest, installCallback);
> -        this._showInstallNotification(aDOMDocument, installer);
> -        break;
> -      default:
> -        throw new Error("SocialService.installProvider: Invalid install type "+installType+"\n");

Do we still need to whitelist the install types?
Attachment #8484583 - Flags: review?(jaws) → feedback+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #61)
> Comment on attachment 8484579 [details] [diff] [review]
> part 1.1: about:providerdirectory page showing in share panel

> @@ +42,5 @@
> > +
> > +    Cu.import("resource://gre/modules/Services.jsm");
> > +
> > +    function openDirectory() {
> > +      let url = Services.prefs.getCharPref("social.directories").split(',')[0];
> 
> When will social.directories have more than one entry? Why are we handling
> comma-separated values here, I don't see the need for it elsewhere in the
> patches.

social.directories has been comma delineated from the start (when it was added), it was modeled after similar prefs for amo and marketplace.  In reality it is used primarily to add additional test directories, but I don't think this is the place to change how the pref works if we wanted to.

> ::: browser/components/about/AboutRedirector.cpp
> @@ +48,5 @@
> >      nsIAboutModule::ALLOW_SCRIPT |
> >      nsIAboutModule::HIDE_FROM_ABOUTABOUT },
> > +  { "providerdirectory", "chrome://browser/content/aboutProviderDirectory.xhtml",
> > +    nsIAboutModule::ALLOW_SCRIPT |
> > +    nsIAboutModule::HIDE_FROM_ABOUTABOUT },
> 
> This needs a security review since it is adding an entry to this file that
> does not specify URI_SAFE_FOR_UNTRUSTED_CONTENT.

will request one

> ::: browser/themes/shared/aboutProviderDirectory.css
> @@ +68,5 @@
> > +  box-shadow: 0 1px 1px hsla(210,15%,25%,.2) inset,
> > +              0 0 2px hsla(210,15%,25%,.4) inset;
> > +  transition-property: background-color, border-color, box-shadow;
> > +  transition-duration: 10ms;
> > +  transition-timing-function: linear;
> 
> Why are we using `ease` for exiting :hover and :hover:active, `ease` for
> entering :hover, but `linear` for entering :hover:active? Also, `ease` is
> the initial value, so it does not need to be specified unless it is being
> used to override a previously applied value.

This panel was using the same css as the error page to get the same look and feel.  I've rejigged this patch to avoid duplication (and making more shared css), but what you are asking about pre-exists in aboutSocialError.css.  If it's important, we can clean that up in another bug?
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #63)
> Comment on attachment 8484583 [details] [diff] [review]
> part 1.4: enable activating providers from the new share panel

> ::: toolkit/components/social/SocialService.jsm
> @@ -654,5 @@
> > -        installer = new AddonInstaller(sourceURI, manifest, installCallback);
> > -        this._showInstallNotification(aDOMDocument, installer);
> > -        break;
> > -      default:
> > -        throw new Error("SocialService.installProvider: Invalid install type "+installType+"\n");
> 
> Do we still need to whitelist the install types?

whitelist only exists at this point to support localStorage in the frameworker, and only one provider needs that at this point.  There is a bug somewhere to deal with that.  Install has no need for a whitelist.
Comment on attachment 8482433 [details] [diff] [review]
part2: CUI test fixes after moving share button to menu-panel

this patch will no longer be necessary since the button will default to palette.
Attachment #8482433 - Attachment is obsolete: true
- moves aboutSocialError.css to shared css (was the same in all platforms) and uses that in aboutProviderDirectory.css to avoid duplications

- needs sec-review for new about: page (thus dveditz?)
Attachment #8486792 - Flags: review?(jaws)
Attachment #8486792 - Flags: review?(dveditz)
Attachment #8484579 - Attachment is obsolete: true
feedback done, carry forward r+ from jaws
Attachment #8486795 - Flags: review+
Attachment #8484581 - Attachment is obsolete: true
button now defaults to the palette, however it now retains the original feature of showing up in the toolbar when a share provider is activated, if the button is in the palette.
Attachment #8486798 - Flags: review?(jaws)
Attachment #8484582 - Attachment is obsolete: true
This doesn't go all the way with promises right now, that can be done in another bug.  I'd rather not get distracted (in this bug) with rewriting all the tests.
Attachment #8484583 - Attachment is obsolete: true
Attachment #8486800 - Flags: review?(jaws)
Attached patch partf 4: async test cleanups (obsolete) — Splinter Review
feedback taken, carry forward r+ from jaws
Attachment #8486802 - Flags: review+
New try since there is a number of changes caused by making the palette the default location for the button.

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

or 

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=99bd82eb4cae
Attached patch part 4: async test cleanups (obsolete) — Splinter Review
minor fixes after last try (oranges due to button defaulting to palette)
Attachment #8482435 - Attachment is obsolete: true
Attachment #8487421 - Flags: review+
Attachment #8486802 - Attachment is obsolete: true
minor fixes after last try (oranges due to button defaulting to palette)

new try https://tbpl.mozilla.org/?tree=Try&rev=da27e76d0b2c
Attachment #8482434 - Attachment is obsolete: true
Attachment #8487422 - Flags: review+
Comment on attachment 8486792 [details] [diff] [review]
part 1.1: about:providerdirectory page showing in share panel

You'll need to follow the steps at https://wiki.mozilla.org/Security/Reviews/Review_Request_Form to request a security review. Sorry for not linking you to this page earlier.
Attachment #8486792 - Flags: review?(dveditz)
Flags: needinfo?(mixedpuppy)
Comment on attachment 8486792 [details] [diff] [review]
part 1.1: about:providerdirectory page showing in share panel

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

::: browser/themes/osx/browser.css
@@ +1314,5 @@
>    }
>  
> +  #add-share-provider {
> +    list-style-image: url(chrome://browser/skin/menuPanel-small@2x.png);
> +    -moz-image-region: rect(0px, 192px, 32px, 160px);

nit, just use 0 (no units).
Attachment #8486792 - Flags: review?(jaws) → review+
Flags: sec-review?(curtisk)
Comment on attachment 8486798 [details] [diff] [review]
part 1.3: make the share button a CUI widget

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

::: browser/base/content/browser-social.js
@@ +1350,5 @@
> +        yield node;
> +    }
> +  },
> +  update: function() {
> +    // querySelectorAll does not work on the menu panel the panel, so we have to

s/menu panel the panel/menu panel/
Attachment #8486798 - Flags: review?(jaws) → review+
Comment on attachment 8486800 [details] [diff] [review]
part 1.4: enable activating providers from the new share panel

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

::: browser/base/content/test/social/browser_share.js
@@ +33,5 @@
> +}
> +
> +function waitForEvent(iframe, eventName, callback) {
> +  iframe.addEventListener(eventName, function load() {
> +    info("page load is "+iframe.contentDocument.location.href);

nit, spaces around binary operators

@@ +307,5 @@
> +      let subframe = iframe.contentDocument.getElementById("activation-frame");
> +      waitForCondition(() => {
> +          // sometimes the iframe is ready before the panel is open, we need to
> +          // wait for both conditions
> +          return SocialShare.panel.state == "open" && subframe.contentDocument && subframe.contentDocument.readyState == "complete";

nit, please place a line break after the second &&, so:
return SocialShare.panel.state == "open" && subframe.contentDocument &&
       subframe.contentDocument.readyState == "complete";

@@ +316,5 @@
> +          let port = provider.getWorkerPort();
> +          ok(!!port, "got port");
> +          port.onmessage = function (e) {
> +            let topic = e.data.topic;
> +            info("got topic "+topic+"\n");

nit, spaces around binary operators
Attachment #8486800 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #75)
> Comment on attachment 8486792 [details] [diff] [review]
> part 1.1: about:providerdirectory page showing in share panel
> 
> You'll need to follow the steps at
> https://wiki.mozilla.org/Security/Reviews/Review_Request_Form to request a
> security review. Sorry for not linking you to this page earlier.

had talked with yvan who suggested to just get review from dveditz, but I'll touch base with curtisk.
Flags: needinfo?(mixedpuppy)
I don't think this needs a formal big type of review. Assigning flag to dveditz
Flags: sec-review?(curtisk) → sec-review?(dveditz)
@dveditz, only looking to sec-review the attachment 8486792 [details] [diff] [review] where the new about: page is.
address review nits
Attachment #8486798 - Attachment is obsolete: true
Attachment #8487952 - Flags: review+
address review nits
Attachment #8486800 - Attachment is obsolete: true
Attachment #8487954 - Flags: review+
fix applying patch after prior review nit fixes
Attachment #8487421 - Attachment is obsolete: true
Attachment #8487956 - Flags: review+
Clearing tracking33 as Shane tells me that this is going to ride the 35 train.
Blocks: 1067342
Comment on attachment 8486795 [details] [diff] [review]
part 1.2: error page handling for about:providerdirectory

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

::: browser/base/content/aboutSocialError.xhtml
@@ +45,5 @@
> +      let searchParams = new URLSearchParams(location.href.split("?")[1]);
> +      let mode = searchParams.get("mode");
> +      config.directory = searchParams.get("directory");
> +      config.origin = searchParams.get("origin");
> +      let encodedURL = searchParams.get("url");

I filed bug 1073886 on concerns I about passing/trusting the URL parameter here -- it may not actually be a registered URL. Since it would take a second bug for web content to be able to load about:socialerror it can be addressed separately in that bug.
Comment on attachment 8487952 [details] [diff] [review]
part 1.3: make the share button a CUI widget

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

::: browser/base/content/browser-social.js
@@ +295,5 @@
>      return Social.providers.length > 0;
>    },
>  
> +  canShareOrMarkPage: function(aURI) {
> +    // we do not enable sharing from private sessions

Why not? PB is about leaving no local traces while sharing is an explicit action to publicize information to your social network. This seems analogous to creating bookmarks, which we do allow people to do from Private Browsing. This will make the sharing feature useless for habitual PB users (but I don't know how many users fall into that group).

@@ +299,5 @@
> +    // we do not enable sharing from private sessions
> +    if (PrivateBrowsingUtils.isWindowPrivate(window))
> +      return false;
> +
> +    if (!aURI || !(aURI.schemeIs('http') || aURI.schemeIs('https')))

For me this would be more comprehensible if you flip the sense to remove the NOTs:

if (aURI && (aURI.schemIs('http') || aURI.schemeIs('https')))
  return true;
return false;

or even:

return (aURI && (aURI.schemIs('http') || aURI.schemeIs('https')));
Comment on attachment 8486792 [details] [diff] [review]
part 1.1: about:providerdirectory page showing in share panel

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

::: browser/base/content/aboutProviderDirectory.xhtml
@@ +50,5 @@
> +    }
> +    
> +    if (Services.prefs.getBoolPref("social.share.activationPanelEnabled")) {
> +      let url = Services.prefs.getCharPref("social.shareDirectory");
> +      document.getElementById("activation-frame").setAttribute("src", url);

This approach is safe only because we trust the directory (ourselves) and it's loaded securely (the pref uses https:, and as of Firefox 32 mozilla sites including the CDN have their certs pinned). Is it worth documenting those assumptions in firefox.js as comments, or here? Could enforce the https: bit, or at least log a warning if insecure, but that would be more to help prevent future mistakes rather than a security precaution since attackers can run https://evil.com as easily as http://evil.com.

However, search hijackers DO change our prefs when it suits them, and if there's some advantage to them replacing our share providers with their own (using the icons of the popular ones, I'm sure) they will. Does this /need/ to be a user-settable pref? It's a worry, but I guess if someone has profile access to change these prefs they wouldn't need to phish the share providers. Although if we succeed in locking down add-ons more they might have fewer options. Unless they have access to the install directory in which case all bets are off.

Maybe adding the domain to the panel would help. Instead of a plain block of icons there could be a heading (outside the frame, in about:providerdirectory proper) that says  "Share providers offered through <directory domain>". Then if the pref is changed it's clear to the user that the source might be suspect.

The share panel itself might want to add text, or at least hover text, on the icons. Not everyone will recognize all of them. I suppose the counter argument is that if you don't recognize it you don't want to use it, but it might be a nice discovery surface.
Comment on attachment 8487954 [details] [diff] [review]
part 1.4: enable activating providers from the new share panel

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

::: toolkit/components/social/SocialService.jsm
@@ +625,2 @@
>  
> +    let installType = getOriginActivationType(aDOMDocument.nodePrincipal.origin);

getOriginActivationType() will not return "internal" for the new about:providerdirectory. That function checks the scheme against "moz-safe-about" which is used for about urls that are marked URI_SAFE_FOR_UNTRUSTED_CONTENT. Since you want powers in this one you didn't add that flag in AboutRedirector, so the scheme will be regular "about". Just add that one to getOriginActivationType().

BUT-- now that I've looked at that it means that EVERY about: url can silently add providers. Are we sure that they're all immune from injection? No, that's why we've made so many of them unpowered by marking them URI_SAFE_FOR_UNTRUSTED_CONTENT.

I worry most about some of the error page ones. They get content injected into them on purpose. We try to strip out scripts but we can't be 100% certain.

I don't mind updating getOriginActivationType() to call all chrome-powered about: urls "internal", but I'm unhappy with all "moz-safe-about". Is there somewhere other than about:home snippets that needs that power? Perhaps just check that the URI is "about:home" exactly, and leave all the others as "foreign".
Looks like we've reasonably protected the acquisition of panel content short of someone hacking either the server or Firefox locally.

The social panels are tailor made for phishing: once they're installed there are no affordances that let users see they really are from the site whose logo is all over them. That makes it all the more important to let users know where a panel is from when it's being installed and we currently don't do that. A solution to that might be to add a mini read-only URL bar to the top of the panel (rejected as ugly, I'm sure), or at least hover-text of the URL on the left-column buttons that let you select providers (image 2 of the mockup in attachment 8431210 [details]). The mini URL-bar is better because it would show the true URL of the contents; the url on the button/icon could be a phishy redirect. Still, hover-text may be more aesthetically acceptable and is probably good enough.

My biggest concern is that _all_ unsafe about: URLs are empowered to silently install social providers. Some of them were marked unsafe for a reason. In addition this allows add-on-installed about: pages to have this power, too. I'm not too keen about:home has this power either (don't entirely trust quickly generated marketing-driven snippets to be bug-free), but if it must let's at least make sure that's as far as it goes.
Depends on: 1073886, 1073863
Flags: sec-review?(dveditz) → sec-review+
Depends on: 1073917
I also think that we need to allow this functionality in PB.  Note especially that you can configure Firefox to always turn on PB mode for all Windows, so with the permanent PB feature on, the Share button will be completely disabled.
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #91)
> I also think that we need to allow this functionality in PB.  Note
> especially that you can configure Firefox to always turn on PB mode for all
> Windows, so with the permanent PB feature on, the Share button will be
> completely disabled.

I don't want to conflate PB with this bug (otherwise this bug will get 10 times longer).  The reason we don't work in PB is the frameworker which is a singleton per provider in the instance, and that would create a leak between PB and non-PB windows.  We need to switch away from the frameworker and use a service worker or shared worker instead, and ensure that PB windows have a worker separate from non-PB windows.  Bug 898706.
No longer blocks: 1055590
(In reply to Shane Caraveo (:mixedpuppy) from comment #92)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #91)
> > I also think that we need to allow this functionality in PB.  Note
> > especially that you can configure Firefox to always turn on PB mode for all
> > Windows, so with the permanent PB feature on, the Share button will be
> > completely disabled.
> 
> I don't want to conflate PB with this bug (otherwise this bug will get 10
> times longer).  The reason we don't work in PB is the frameworker which is a
> singleton per provider in the instance, and that would create a leak between
> PB and non-PB windows.  We need to switch away from the frameworker and use
> a service worker or shared worker instead, and ensure that PB windows have a
> worker separate from non-PB windows.  Bug 898706.

Oh...  sadface. :(  But I guess that's fine for now.
Depends on: 1075251
(In reply to Daniel Veditz [:dveditz] from comment #88)
> Comment on attachment 8486792 [details] [diff] [review]
> part 1.1: about:providerdirectory page showing in share panel
> 
> Review of attachment 8486792 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Maybe adding the domain to the panel would help. Instead of a plain block of
> icons there could be a heading (outside the frame, in
> about:providerdirectory proper) that says  "Share providers offered through
> <directory domain>". Then if the pref is changed it's clear to the user that
> the source might be suspect.

will handle that through bug 1075251
(In reply to Daniel Veditz [:dveditz] from comment #89)
> Comment on attachment 8487954 [details] [diff] [review]
> part 1.4: enable activating providers from the new share panel
> 
> Review of attachment 8487954 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/social/SocialService.jsm
> @@ +625,2 @@
> >  
> > +    let installType = getOriginActivationType(aDOMDocument.nodePrincipal.origin);
> 
> getOriginActivationType() will not return "internal" for the new
> about:providerdirectory. 

That is ok since it loads a frame from the directory that does the activations, or if that is turned off it simply provides a link to the directory.

> I don't mind updating getOriginActivationType() to call all chrome-powered
> about: urls "internal", but I'm unhappy with all "moz-safe-about". Is there
> somewhere other than about:home snippets that needs that power? Perhaps just
> check that the URI is "about:home" exactly, and leave all the others as
> "foreign".

I'll whitelist the pages we want to allow activation from instead of relying on the scheme.
(In reply to Daniel Veditz [:dveditz] from comment #90)
> The social panels are tailor made for phishing: once they're installed there
> are no affordances that let users see they really are from the site whose
> logo is all over them. That makes it all the more important to let users
> know where a panel is from when it's being installed and we currently don't
> do that. A solution to that might be to add a mini read-only URL bar to the
> top of the panel (rejected as ugly, I'm sure), or at least hover-text of the
> URL on the left-column buttons that let you select providers (image 2 of the
> mockup in attachment 8431210 [details]). The mini URL-bar is better because
> it would show the true URL of the contents; the url on the button/icon could
> be a phishy redirect. Still, hover-text may be more aesthetically acceptable
> and is probably good enough.

You can get to page info from the context menu, but that is non-obvious.  A hover tip on the icons is simple to do so I'll add that.
minor bitrot fixed, carry forward r+
Attachment #8486795 - Attachment is obsolete: true
Attachment #8501225 - Flags: review+
Changes (per reviews/discussion w/dveditz) from previous review:

- new tooltip node used for buttons in share panel, shows both name and origin of provider
- minor code cleanup in canShareOrMarkPage
Attachment #8487952 - Attachment is obsolete: true
Attachment #8501228 - Flags: review?(jaws)
Difference from previous review is a change in getOriginActivationType to use system principal and a specific check for moz-safe-about:home
Attachment #8487954 - Attachment is obsolete: true
Attachment #8501230 - Flags: review?(jaws)
Comment on attachment 8501228 [details] [diff] [review]
part 1.3: make the share button a CUI widget

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

::: browser/base/content/browser-social.js
@@ +537,5 @@
> +  createTooltip: function(event) {
> +    let tt = event.target;
> +    let provider = Social._getProviderFromOrigin(tt.triggerNode.getAttribute("origin"));
> +    tt.firstChild.setAttribute("value", provider.name);
> +    tt.lastChild.setAttribute("value", provider.origin);

firstChild and lastChild here are too ambiguous. Can you please put an ID on the children of the <tooltip> node and reference that ID here?
Attachment #8501228 - Flags: review?(jaws) → review+
Comment on attachment 8501230 [details] [diff] [review]
part 1.4: enable activating providers from the new share panel

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

r=me for this and the previous comment, based on the changes in seen in the interdiffs.
Attachment #8501230 - Flags: review?(jaws) → review+
Iteration: --- → 35.3
Flags: qe-verify?
Depends on: 1084419
Depends on: 1088257
Depends on: 1301351
Depends on: 1301354
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.