891219 - implement new social bookmark button

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

The socialmark button will change to a toolbarbutton that behaves like the bookmark button.  The first click on the button will load a url (from the manifest) into an iframe contained in an attached panel.  Second click will show the resulting page.  Multiple mark buttons may exist in the toolbar.

The url in the manifest will be a url template, same as used with share.  One additional get param will be added, state=[marked,unmarked].

The manifest will add:

markURL - url template
markIcon - TBD we need icons for low res, retina, marked and unmarked states.  We could have an entry for each, or rely on a combined icon image.

TBD need to consider compatibility if any providers use the existing socialmarks button, cliqz might be, have not seen anyone else use this yet.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: remove socialmarks

since the socialmarks implementation is moving to one button per provider and needs to support more than one button, the implementation differs in a couple areas significantly.  the first patch is a complete removal of the existing socialmarks functionality to make readability of the new implementation patch easier.

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: new-marks

implementation supports one button per provider and buttons are customizable/removable.  The buttons are persisted by the same helper class added in bug 891225.

Comment 3

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

Comment on attachment 789250 [details] [diff] [review]
new-marks

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

In general, this looks good to me.

::: browser/base/content/browser-social.js
@@ +360,5 @@
>    },
>  
> +  // should be called on tab/urlbar/location changes.  Update anything that
> +  // is tab specific.
> +  updateState: function() {

maybe make this name more like updateForLocationChange or something?

@@ +1619,5 @@
> +    return this._toolbarHelper;
> +  },
> +
> +  _createButton: function(provider) {
> +    if (!provider.statusURL)

is this check actually necessary?  (I don't like these checks as it implies confusion by the caller in many cases)

::: browser/base/content/browser.css
@@ +654,5 @@
> +toolbarbutton[type="socialmark"] > panel {
> +  transition: height 150ms ease-out, width 150ms ease-out;
> +  height: 285px;
> +  width: 260px;
> +}

I'm clueless about this stuff, but these magic numbers concern me.  Do they need to match the size of other buttons on the toolbar?  If so, maybe some other CSS changes could be made such that we inherit them?

::: browser/base/content/social.xml
@@ +87,5 @@
> +        let provider = this.provider;
> +        this.panel.hidden = false;
> +        let DynamicResizeWatcher = Cu.import("resource:///modules/Social.jsm", {}).DynamicResizeWatcher;
> +        this._dynamicResizer = new DynamicResizeWatcher();
> +        this._dynamicResizer.start(this.panel, this.panel.firstChild);          

whitespace here and a few lines down

@@ +156,5 @@
> +        if (!uri)
> +          return;
> +
> +        // we made it this far, use it
> +        this.setAttribute('image', uri.spec);

not using list-style-image here?

::: browser/modules/Social.jsm
@@ +277,3 @@
>        }
>        aCallback(!!val);
>      }.bind(this));

use fat arrows to avoid .this binding, and we probably need a final .then(null, Cu.reportError)

@@ +307,1 @@
>            });

ditto about final .then (which sucks, but there you have it!)

@@ +321,5 @@
> +        providerList.splice(providerList.indexOf(origin), 1);
> +        promiseSetAnnotation(aURI, providerList).then(function() {
> +          if (aCallback)
> +            schedule(function() { aCallback(false); } );
> +        });

stooopid promises - .then...

::: toolkit/components/social/SocialService.jsm
@@ +484,5 @@
>      }
>    },
>  
>    _manifestFromData: function(type, data, principal) {
> +    let sameOriginRequired = ['workerURL', 'sidebarURL', 'shareURL', 'statusURL', 'markURL'];

same comment as previous patch - doesn't this make these new attributes compulsory?

Comment 4

[Shane Caraveo (:mixedpuppy)]

Attachment: remove socialmarks

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: new-marks

patch update

Comment 6

[Shane Caraveo (:mixedpuppy)]

Attachment: remove socialmarks

Comment 7

[Shane Caraveo (:mixedpuppy)]

Attachment: new-marks

still need to add context menu's, so feedback only.  updated on top of bug 891225 and tests added.

Comment 8

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

Comment on attachment 794257 [details] [diff] [review]
new-marks

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

The first 3 feedback comments from my previous review haven't been addressed or commented on, so I'm assuming you just forgot to go through that.

Comment 9

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #3)
> Comment on attachment 789250 [details] [diff] [review]
> new-marks
> 
> Review of attachment 789250 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In general, this looks good to me.
> 
> ::: browser/base/content/browser-social.js
> @@ +360,5 @@
> >    },
> >  
> > +  // should be called on tab/urlbar/location changes.  Update anything that
> > +  // is tab specific.
> > +  updateState: function() {
> 
> maybe make this name more like updateForLocationChange or something?

it is also called aftercustomization, basically any time that the buttons need to update state.  

> @@ +1619,5 @@
> > +    return this._toolbarHelper;
> > +  },
> > +
> > +  _createButton: function(provider) {
> > +    if (!provider.statusURL)
> 
> is this check actually necessary?  (I don't like these checks as it implies
> confusion by the caller in many cases)

yes, per the same explanation in bug 891225 (status and marks are using the same basic outline for button handling), though it did catch a bug.

> ::: browser/base/content/browser.css
> @@ +654,5 @@
> > +toolbarbutton[type="socialmark"] > panel {
> > +  transition: height 150ms ease-out, width 150ms ease-out;
> > +  height: 285px;
> > +  width: 260px;
> > +}
> 
> I'm clueless about this stuff, but these magic numbers concern me.  Do they
> need to match the size of other buttons on the toolbar?  If so, maybe some
> other CSS changes could be made such that we inherit them?

TBH I don't recall why I did that, so I'll have to look into it.

Comment 10

[Shane Caraveo (:mixedpuppy)]

Attachment: remove socialmarks

Comment 11

[Shane Caraveo (:mixedpuppy)]

Attachment: new-marks

updated with comments (sorry about forgetting those before), and latest modifications from bug 891225.

Comment 12

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

Comment on attachment 794867 [details] [diff] [review]
new-marks

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

::: browser/base/content/browser-social.js
@@ +95,5 @@
>      try {
>        switch (topic) {
>          case "social:provider-installed":
>            // some of our tests bypass install, thus we have no manifest
> +          if (data) {

this will need to change based on that other review I just did but forgot the bug number of :)

::: browser/base/content/browser.css
@@ +660,5 @@
> +  -moz-binding: url("chrome://browser/content/social.xml#toolbarbutton-marks");
> +}
> +toolbarbutton[type="socialmark"] > panel {
> +  transition: height 150ms ease-out, width 150ms ease-out;
> +  height: 285px;

you said you were going to look into these magic numbers?

::: browser/base/content/social.xml
@@ +3,5 @@
> +<bindings id="socialMarkBindings"
> +    xmlns="http://www.mozilla.org/xbl"
> +    xmlns:xbl="http://www.mozilla.org/xbl"
> +    xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> +  

trailing space

@@ +77,5 @@
> +        this.setAttribute("origin", provider.origin);
> +        this.panel.hidePopup();
> +        this.panel.hidden = true;
> +        this.pageData = null;
> +        // TODO are we annotated for this page?

we probably want a bug for this TODO and reference it here.

@@ +87,5 @@
> +        let provider = this.provider;
> +        this.panel.hidden = false;
> +        let DynamicResizeWatcher = Cu.import("resource:///modules/Social.jsm", {}).DynamicResizeWatcher;
> +        this._dynamicResizer = new DynamicResizeWatcher();
> +        this._dynamicResizer.start(this.panel, this.panel.firstChild);          

trailing spaces here and a few lines down

::: browser/base/content/test/social/Makefile.in
@@ +44,5 @@
>                   social_flyout.html \
>                   social_window.html \
>                   social_worker.js \
>                   share.html \
> +                 checked.jpg \

I don't see these 2 new files in the patch?

::: browser/modules/Social.jsm
@@ +341,2 @@
>    },
>    

existing trailing whitespace that might as well be fixed.

@@ +498,4 @@
>  
>  this.OpenGraphBuilder = {
> +  generateEndpointURL: function(URLTemplate, pageData) {
> +    // support for existing oexchange style endpoints by supporting their

I know this was just moved, but was it carefully reviewed before?  Is there any risk of malicious open-graph data in a page screwing with us?

Comment 13

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #12)
> Comment on attachment 794867 [details] [diff] [review]

> ::: browser/base/content/browser.css
> @@ +660,5 @@
> > +  -moz-binding: url("chrome://browser/content/social.xml#toolbarbutton-marks");
> > +}
> > +toolbarbutton[type="socialmark"] > panel {
> > +  transition: height 150ms ease-out, width 150ms ease-out;
> > +  height: 285px;
> 
> you said you were going to look into these magic numbers?

yeah, but I didn't figure I needed to right away.

> ::: browser/base/content/test/social/Makefile.in
> @@ +44,5 @@
> >                   social_flyout.html \
> >                   social_window.html \
> >                   social_worker.js \
> >                   share.html \
> > +                 checked.jpg \
> 
> I don't see these 2 new files in the patch?

they are there, look again.

> @@ +498,4 @@
> >  
> >  this.OpenGraphBuilder = {
> > +  generateEndpointURL: function(URLTemplate, pageData) {
> > +    // support for existing oexchange style endpoints by supporting their
> 
> I know this was just moved, but was it carefully reviewed before?  Is there
> any risk of malicious open-graph data in a page screwing with us?

yes on reviewed and two secreviews on top of that (where I specifically talked about this class).

Comment 14

[Shane Caraveo (:mixedpuppy)]

Attachment: remove socialmarks

updated

Comment 15

[Shane Caraveo (:mixedpuppy)]

Attachment: new-marks

A few larger changes in this version.  This should be much closer to ready, I still want to think about the context menu ui.

- feedback added
- css removed, iirc it was an early hack to size the panel before I got everything hooked up.  I had to update sizeSocialPanelToContent use in the button xbl as well as guard against lack of computedStyle when the button panel is loaded prior to being shown.  this also led to fixing use of the dynamic resizer.
- context menus reimplemented (with simple test for existence)
- enabled/disabled state for button implemented (with tests)
- some behavior fixes to make mark buttons work exactly like the bookmark button in regard to context menu use

Comment 16

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

Comment on attachment 796337 [details] [diff] [review]
new-marks

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

I think CheckSocialUI in head.js should also grow something for marks?

::: browser/base/content/browser-context.inc
@@ +36,5 @@
>        <menuitem id="context-bookmarklink"
>                  label="&bookmarkThisLinkCmd.label;"
>                  accesskey="&bookmarkThisLinkCmd.accesskey;"
>                  oncommand="gContextMenu.bookmarkLink();"/>
> +      <menu id="context-marklink" label="&social.marklink.label;" accesskey="&social.marklink.accesskey;">

please wrap this line

::: browser/base/content/browser-social.js
@@ +1610,5 @@
> +SocialMarks = {
> +  update: function() {
> +    // signal each button to update itself
> +    let currentButtons = document.querySelectorAll('toolbarbutton[type="socialmark"]');
> +    for (let elt of currentButtons) {

nit: this file tends to not use braces for single-line blocks, so nuke the braces here and a few other spots in SocialMarks.

@@ +1633,5 @@
> +
> +  _populateContextPopup: function(menu, providers) {
> +    let popup = menu.firstChild;
> +    // remove previous menu entries
> +    while (popup.firstChild) {

eg, this function has a couple single-line blocks

::: browser/base/content/browser.css
@@ +660,5 @@
> +  -moz-binding: url("chrome://browser/content/social.xml#toolbarbutton-marks");
> +}
> +toolbarbutton[type="socialmark"] > panel {
> +  transition: height 150ms ease-out, width 150ms ease-out;
> +}

surprised to see a transition here, but I'll assume it's been asked for by UX or that other similar buttons do the same thing.

::: browser/base/content/social.xml
@@ +100,5 @@
> +        let endpoint = OpenGraphBuilder.generateEndpointURL(URLTemplate, this.pageData);
> +
> +        // setup listeners
> +        this.addEventListener("DOMContentLoaded", function DOMContentLoaded(event) {
> +          if (event.target != this.contentDocument)

nit: it looks like the style of this file is that single-line blocks *do* get braces, so this should be consistent (or the others above etc should be changed :)

@@ +216,5 @@
> +        // the presence icon for a chat user, we simply use favicon style
> +        // updating
> +        let link = event.originalTarget;
> +        let rel = link.rel && link.rel.toLowerCase();
> +        if (!link || !link.ownerDocument || !rel || !link.href)

more such blocks here.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +642,5 @@
>  <!ENTITY social.chatBar.commandkey "c">
>  <!ENTITY social.chatBar.label "Focus chats">
>  <!ENTITY social.chatBar.accesskey "c">
>  
> +<!ENTITY social.markpage.accesskey "m">

these access keys look wrong - the letter 'm' doesn't appear in the label, and the term "mark" will mean nothing to users.

::: browser/modules/Social.jsm
@@ +437,5 @@
>    onSecurityChange: function SPL_onSecurityChange() {},
>  };
>  
> +// The minimum sizes for the auto-resize panel code.
> +const PANEL_MIN_HEIGHT = 100;

I missed this before, but I'm inclined to say these consts should go at the top of the file?

Comment 17

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #16)
> Comment on attachment 796337 [details] [diff] [review]
> new-marks
> 
> Review of attachment 796337 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think CheckSocialUI in head.js should also grow something for marks?

It is a dynamically added button that is only (thus far) added in the browser_social_marks.js test file, the rest of the time it wouldn't be there.  I'm not sure there is value in testing the button.  The context menus are a part of xul, so I've added tests on those in CheckSocialUI.

> ::: browser/base/content/browser.css
> @@ +660,5 @@
> > +  -moz-binding: url("chrome://browser/content/social.xml#toolbarbutton-marks");
> > +}
> > +toolbarbutton[type="socialmark"] > panel {
> > +  transition: height 150ms ease-out, width 150ms ease-out;
> > +}
> 
> surprised to see a transition here, but I'll assume it's been asked for by
> UX or that other similar buttons do the same thing.

that wasn't necessary, the transition is defined on the panel element itself.

> ::: browser/base/content/social.xml
> @@ +100,5 @@
> > +        let endpoint = OpenGraphBuilder.generateEndpointURL(URLTemplate, this.pageData);
> > +
> > +        // setup listeners
> > +        this.addEventListener("DOMContentLoaded", function DOMContentLoaded(event) {
> > +          if (event.target != this.contentDocument)
> 
> nit: it looks like the style of this file is that single-line blocks *do*
> get braces, so this should be consistent (or the others above etc should be
> changed :)

since it is a new file, I removed the couple places where there were braces on single-line.

> ::: browser/locales/en-US/chrome/browser/browser.dtd
> @@ +642,5 @@
> >  <!ENTITY social.chatBar.commandkey "c">
> >  <!ENTITY social.chatBar.label "Focus chats">
> >  <!ENTITY social.chatBar.accesskey "c">
> >  
> > +<!ENTITY social.markpage.accesskey "m">
> 
> these access keys look wrong - the letter 'm' doesn't appear in the label,
> and the term "mark" will mean nothing to users.

"mark" isn't exposed to users.  keys fixed.

Comment 18

[Shane Caraveo (:mixedpuppy)]

Attachment: socialmark-contextmenu.png

@Boriss, this attachment shows the context menu functionality supporting the social bookmark button.  Since there can be more than one social bookmark provider, we need to have multiple selection.  However, having only one is a bit weird.  I could have a separate single-click menu in the context menu, it complicates code and adds additional strings, so I wanted to know what you think first.

Comment 19

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

(In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> Created attachment 799074 [details]
> socialmark-contextmenu.png
> 
> @Boriss, this attachment shows the context menu functionality supporting the
> social bookmark button.  Since there can be more than one social bookmark
> provider, we need to have multiple selection.  However, having only one is a
> bit weird.  I could have a separate single-click menu in the context menu,
> it complicates code and adds additional strings, so I wanted to know what
> you think first.

The existence of a "Save Page To..." and "Save Page As..." seems quite confusing. Is the page context menu the best place for this?

Comment 20

[Shane Caraveo (:mixedpuppy)]

(In reply to Jared Wein [:jaws] from comment #19)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> > Created attachment 799074 [details]
> > socialmark-contextmenu.png
> > 
> > @Boriss, this attachment shows the context menu functionality supporting the
> > social bookmark button.  Since there can be more than one social bookmark
> > provider, we need to have multiple selection.  However, having only one is a
> > bit weird.  I could have a separate single-click menu in the context menu,
> > it complicates code and adds additional strings, so I wanted to know what
> > you think first.
> 
> The existence of a "Save Page To..." and "Save Page As..." seems quite
> confusing. Is the page context menu the best place for this?

The text could change.   What other context menu is there? afaik the context menu is the only way to select links/images/videos/etc and "bookmark" them.

Comment 21

[Shane Caraveo (:mixedpuppy)]

Attachment: remove socialmarks

Comment 22

[Shane Caraveo (:mixedpuppy)]

Attachment: new-marks

pending ui-review, and maybe text change in the dtd, I feel this is ready for review.  The dependency bug finally got all green on it's tests, so here we go for the last large patch.

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

Comment 23

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

Comment on attachment 799153 [details] [diff] [review]
new-marks

Let's get UX (including comment 19) sorted out first.

Comment 24

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

(In reply to Shane Caraveo (:mixedpuppy) from comment #20)
> (In reply to Jared Wein [:jaws] from comment #19)
> > (In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> > > Created attachment 799074 [details]
> > > socialmark-contextmenu.png
> > > 
> > > @Boriss, this attachment shows the context menu functionality supporting the
> > > social bookmark button.  Since there can be more than one social bookmark
> > > provider, we need to have multiple selection.  However, having only one is a
> > > bit weird.  I could have a separate single-click menu in the context menu,
> > > it complicates code and adds additional strings, so I wanted to know what
> > > you think first.
> > 
> > The existence of a "Save Page To..." and "Save Page As..." seems quite
> > confusing. Is the page context menu the best place for this?
> 
> The text could change.   What other context menu is there? afaik the context
> menu is the only way to select links/images/videos/etc and "bookmark" them.

Yes, the screenshot above shows the page context menu, not a link/image/video context menu. Isn't there already a Share button that would work for the immediate page? If an item like this would be needed, then it should only appear on images and video, as we have removed the "Email Link..." feature from links and pages.

The "Email *" context menus only appear on media, and this one should follow that practice. This is due to three reasons: 1, there are immediate and visible ways to email a link to a webpage using the Firefox menu; 2, webpages have a visible URL that can be copy+pasted; 3, our telemetry statistics show that the majority of users who "share" an item using the context menu are sharing media (image/audio/video). This may be an effect of (1) and (2).

Comment 25

[Shane Caraveo (:mixedpuppy)]

(In reply to Jared Wein [:jaws] from comment #24)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #20)
> > (In reply to Jared Wein [:jaws] from comment #19)
> > > (In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> > > > Created attachment 799074 [details]
> > > > socialmark-contextmenu.png
> > > > 
> > > > @Boriss, this attachment shows the context menu functionality supporting the
> > > > social bookmark button.  Since there can be more than one social bookmark
> > > > provider, we need to have multiple selection.  However, having only one is a
> > > > bit weird.  I could have a separate single-click menu in the context menu,
> > > > it complicates code and adds additional strings, so I wanted to know what
> > > > you think first.
> > > 
> > > The existence of a "Save Page To..." and "Save Page As..." seems quite
> > > confusing. Is the page context menu the best place for this?
> > 
> > The text could change.   What other context menu is there? afaik the context
> > menu is the only way to select links/images/videos/etc and "bookmark" them.
> 
> Yes, the screenshot above shows the page context menu, not a
> link/image/video context menu. Isn't there already a Share button that would
> work for the immediate page? If an item like this would be needed, then it
> should only appear on images and video, as we have removed the "Email
> Link..." feature from links and pages.

This is modeled on the bookmarks functionality, which also has this menuitem as well as the button.

> The "Email *" context menus only appear on media, and this one should follow
> that practice. This is due to three reasons: 1, there are immediate and
> visible ways to email a link to a webpage using the Firefox menu; 2,
> webpages have a visible URL that can be copy+pasted; 3, our telemetry
> statistics show that the majority of users who "share" an item using the
> context menu are sharing media (image/audio/video). This may be an effect of
> (1) and (2).

Not necessarily arguing against removing the "save page to" menuitem....

#1 Right now there is no other menuitem outside the context menu.  I could move that to the file menu, but is that the appropriate place for this?  What is better for discoverability/accessibility?

I don't think #2 fits this use case.  There isn't an external app to paste into, and the primary point of this is a one-click ux.

#3 is covered as well, though not shown in the image as I was primarily asking for ux feedback on the structure of the menuitem (ie. the submenu only vs. differing menu's depending on how many providers support the feature).

Comment 26

[Shane Caraveo (:mixedpuppy)]

Attachment: marksingle.png

Comment 27

[Shane Caraveo (:mixedpuppy)]

Attachment: markmulti.png

Comment 28

[Jennifer Morrow [:Boriss] (UX)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> Created attachment 799074 [details]
> socialmark-contextmenu.png
> 
> @Boriss, this attachment shows the context menu functionality supporting the
> social bookmark button.  Since there can be more than one social bookmark
> provider, we need to have multiple selection.  However, having only one is a
> bit weird.  I could have a separate single-click menu in the context menu,
> it complicates code and adds additional strings, so I wanted to know what
> you think first.

Having one line for one provider and a multi-selector for multiple providers, as you proposed, makes sense.

On the current build, I'm seeing the context menu language say "Share This Page" rather than "Send Link To," which is consistent with the language we use in the current Share button's context menu.  

To stay consistent, let's surface a single provider as:

[ICON] Share Page/Image with [SERVICE]

And multiple providers as:

"Share Page With..." which would give the expansion arrow seen in attachment 799722 [details]

Comment 29

[Jennifer Morrow [:Boriss] (UX)]

Please disregard Comment 28.  I skimmed too quickly and thought this bug referred to the Share Panel, not the Social Mark Button (SMB).

SMBs allow the user to add services, but one at a time.  Because of this, I think there's less concern with showing them individually.  If a user has installed two separate services, two more lines in the context menu is not a huge problem.  Not even three more.  We could pick an arbitrary number to show before overflow: let's say 5.  

So, until there are 6 SMBs, let's display each with an icon in the context menu.  What would be ideal is to let providers choose the verb they use.  For instance:

[POCKET ICON] Pocket This Page
[FACEBOOK ICON] Like This Page
[EVERNOTE ICON] Clip This Page

No, it wouldn't have the social provider name, but it would incorporate the core verb the service offers and provide the icon.  After 5 items, we could have an overflow menu that says "Send Page To..." and then have the overflow menu.  And yes, it could lead to duplicate strings, but these would be differentiated by the icon preceding the string.

(In reply to Jared Wein [:jaws] from comment #24)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #20)
> > (In reply to Jared Wein [:jaws] from comment #19)
> > > (In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> > > > Created attachment 799074 [details]
> > > > socialmark-contextmenu.png
> Yes, the screenshot above shows the page context menu, not a
> link/image/video context menu. Isn't there already a Share button that would
> work for the immediate page? If an item like this would be needed, then it
> should only appear on images and video, as we have removed the "Email
> Link..." feature from links and pages.

After going back and forth a bit, I agree, jaws: let's stay with this now, for consistency, and only show mark options on objects.  It gives us consistency with email, after all.  However, I think a better longer-term solution would be to include Email as a kind of mark/share and be consistent across pages and objects.  After all, it's quite an assumption that the user only want to "share" a page and "mark" an object if they're in the context menu.  Also, this creates a bit of a shifting context menu that, to users, may feel buggy.  The difference between clicking on an image and a page can be arbitrary, causing the option to mark and share to feel random.

Comment 30

[Shane Caraveo (:mixedpuppy)]

Hashed this out with Boriss on vidyo.

- for now we'll do both page level and object level in the context menu to provide consistency in showing the providers in most places a user would use the context menu
- up to 3 providers will show up in the context menu
- if more then 3 providers, all providers will go into a submenu
- the text on the menuitems and menus will be "Save Page/Link To ProviderName/..."

We'll try this out and adjust if necessary.

Comment 31

[Shane Caraveo (:mixedpuppy)]

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #29)

> [POCKET ICON] Pocket This Page
> [FACEBOOK ICON] Like This Page
> [EVERNOTE ICON] Clip This Page
> 
> No, it wouldn't have the social provider name, but it would incorporate the
> core verb the service offers and provide the icon.  After 5 items, we could
> have an overflow menu that says "Send Page To..." and then have the overflow
> menu.  And yes, it could lead to duplicate strings, but these would be
> differentiated by the icon preceding the string.

Until we support localization in the manifest we have to stick to "Save Page To ProviderName" which can be localized.

Comment 32

[Shane Caraveo (:mixedpuppy)]

Attachment: imagecontext.png

Comment 33

[Shane Caraveo (:mixedpuppy)]

Attachment: pagecontext.png

Comment 34

[Shane Caraveo (:mixedpuppy)]

Attachment: new-marks

implements context menu per discussion with @Boriss

Comment 35

[Shane Caraveo (:mixedpuppy)]

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

Comment 36

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

Comment on attachment 800471 [details] [diff] [review]
new-marks

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

Looks good!

::: browser/base/content/browser.css
@@ +659,5 @@
> +toolbarbutton[type="socialmark"] {
> +  -moz-binding: url("chrome://browser/content/social.xml#toolbarbutton-marks");
> +}
> +toolbarbutton[type="socialmark"] > .toolbarbutton-icon {
> +  width: 16px;

are these actually needed?  No other toolbar buttons in that file specify the size.

::: browser/base/content/nsContextMenu.js
@@ +320,5 @@
> +    let linkmenus = document.getElementsByClassName("context-markpage");
> +    [m.hidden = !enablePageMarkItems for (m of linkmenus)];
> +
> +    let enableLinkMarks = markProviders.length > 0 && ((this.onLink && !this.onMailtoLink)
> +                                                 || this.onPlainTextLink);

indentation on this line is strange.

@@ +1590,5 @@
>      }
>    },
> +  markLink: function CM_markLink(origin) {
> +    // send link to social, if it is the page url linkURI will be null
> +    SocialMarks.markLink(origin, this.linkURI && this.linkURI.spec);

I'd personally prefer this.linkURI ? this.linkURI.spec : null - otherwise it looks more like the intent is a bool param.

::: browser/base/content/social.xml
@@ +1,1 @@
> +<?xml version="1.0"?>

I think this should be named "socialmarks.xml" to match "socialchat.xml" and so it doesn't imply this is a dumping ground for general social bindings.

::: browser/base/content/test/social/browser_social_marks.js
@@ +36,5 @@
> +    workerURL: "https://" + origin + ".example.com/browser/browser/base/content/test/social/social_worker.js",
> +    markURL: "https://" + origin + ".example.com/browser/browser/base/content/test/social/social_mark.html?url=%{url}",
> +    markedIcon: "https://" + origin + ".example.com/browser/browser/base/content/test/social/unchecked.jpg",
> +    unmarkedIcon: "https://" + origin + ".example.com/browser/browser/base/content/test/social/checked.jpg",
> +  

trailing whitespace

@@ +64,5 @@
> +      Services.prefs.clearUserPref("social.remote-install.enabled");
> +      // just in case the tests failed, clear these here as well
> +      Services.prefs.clearUserPref("social.allowMultipleWorkers");
> +      Services.prefs.clearUserPref("social.whitelist");
> +      

trailing space

@@ +78,5 @@
> +        // provider buttons are properly removed. (e.g on try, window-controls
> +        // was not present in currentsetAtStart, but present on the second
> +        // window)
> +        let tb1 = w1.document.getElementById("nav-bar");
> +        info("tb0 "+toolbar.currentSet);

I'd be inclined to drop some of these infos() too - they look more much like debugging than real info.

@@ +131,5 @@
> +        });
> +      });
> +    });
> +  },
> +  testButtonOnInstall: function(next) {

please put blank lines between each test.

@@ +274,5 @@
> +        info("INSTALLATION FINISHED");
> +        executeSoon(callback);
> +        return;
> +      }
> +      info("INSTALLING "+manifest.origin);

spaces around operators (but really I think these infos should be removed, or at least made not ALLCAPS) - ditto a few lines above.

@@ +311,5 @@
> +          }, "mark button added to currentset");
> +        });
> +      });
> +    }
> +    

trailing spaces

Comment 37

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #36)
> Comment on attachment 800471 [details] [diff] [review]
> new-marks
> 
> Review of attachment 800471 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good!
> 
> ::: browser/base/content/browser.css
> @@ +659,5 @@
> > +toolbarbutton[type="socialmark"] {
> > +  -moz-binding: url("chrome://browser/content/social.xml#toolbarbutton-marks");
> > +}
> > +toolbarbutton[type="socialmark"] > .toolbarbutton-icon {
> > +  width: 16px;
> 
> are these actually needed?  No other toolbar buttons in that file specify
> the size.

They are. Since we're sourcing the image from a website, we have no promise that the icon is the correct size.  Without this (and using the image attribute on the button) the image overflows the button (ie. cannot limit size with css listStyleImage).  I've seen that in a couple tests as I watch them, and want to clean it up for the other social buttons as well, but thought I'd catch this one here.

> ::: browser/base/content/social.xml
> @@ +1,1 @@
> > +<?xml version="1.0"?>
> 
> I think this should be named "socialmarks.xml" to match "socialchat.xml" and
> so it doesn't imply this is a dumping ground for general social bindings.

My intent is to move the social bindings into social.xml during the refactoring phase, no need for multiple files here.  If you prefer the name change right now, I can always change it later.

I'll get address the other feedback (and any additional) in the morning.

Comment 38

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

(In reply to Shane Caraveo (:mixedpuppy) from comment #37)
> > are these actually needed?  No other toolbar buttons in that file specify
> > the size.
> 
> They are...

Ok, thanks.

> > I think this should be named "socialmarks.xml" to match "socialchat.xml" and
> > so it doesn't imply this is a dumping ground for general social bindings.
> 
> My intent is to move the social bindings into social.xml during the
> refactoring phase, no need for multiple files here.  If you prefer the name
> change right now, I can always change it later.

Yeah, I'd go with a new name - I'd probably need some convincing that unifying them into a single file is actually an improvement.

Thanks!

Comment 39

[Shane Caraveo (:mixedpuppy)]

Attachment: delete-marks

Comment 40

[Shane Caraveo (:mixedpuppy)]

Attachment: new-marks

Updated with comments from review.

Comment 41

[Shane Caraveo (:mixedpuppy)]

remove old marks: https://hg.mozilla.org/integration/fx-team/rev/87000f7e9158
new implementation: https://hg.mozilla.org/integration/fx-team/rev/9dacb5c8b255

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/87000f7e9158
https://hg.mozilla.org/mozilla-central/rev/9dacb5c8b255

Comment 43

[Francesco Lodolo [:flod]]

Sorry but this won't work. You need to rename existing entities if you change the strings, this was almost impossible to see because you removed and added the strings in two different patches/commits. Filing a follow-up bug for this.

Comment 44

[Asa Dotzler [:asa]]

Shane, can you help with a one sentence release note here? Something like "Firefox's Social API now supports Social Marks (link to dev doc?)" would be ideal.

Comment 45

[Shane Caraveo (:mixedpuppy)]

"Firefox's Social API now supports Social Bookmarking for multiple providers through  its SocialMarks functionality, see https://developer.mozilla.org/en-US/docs/Social_API for more information."

(I have to update the docs with the new functionality)