853151 - refactoring of recommend into SocialMark

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

Attachment: social-unmarked.png

Based on meetings with Asa and Boriss, we're going to keep the recommend functionality but change it in a couple ways:

- recommend button remains, we start calling it "marks"
- mark button is toggle only, no panel
- mark button moves into the provider toolbar button until
    we make it multiprovider in v.2 (or
    figure out something better)
- will always be right-most button in toolbarbutton

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: social-marked.png

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: social-marks.patch

This is pretty close to review if not ready, there is just some stuff I'm still thinking about around the worker api for this (you can see the changes in SocialService.jsm)

Comment 4

[Shane Caraveo (:mixedpuppy)]

The recommend message is reduced to the following set:

  'social.user-recommend-prompt': function(port, msg) {
    port.postMessage({topic: 'social.user-recommend-prompt-response',
            data: {
              messages: {
                'markTooltip': "Tell me hearty thar be booty here",
                'unmarkTooltip': "Don't tell nay one",
              },
              images: {
                'mark': RECOMMEND_ICON,
                'unmark': RECOMMEND_ICON
              }
            }
          });
  },

Comment 5

[Shane Caraveo (:mixedpuppy)]

The one thing I'm thinking about around this patch is whether to maintain compatibility with the old value names (see comment #4 for new message format).  I don't think it is important to do that, since nobody is using this right now.  Opinions?  If in agreement, lets move the feedback request to review request.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Attachment: social-marks.patch

updated patch removes default images I initially used while working on this patch.

Comment 7

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

Comment on attachment 727940 [details] [diff] [review]
social-marks.patch

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

I think this functionality is a step backwards (ie, how will people know what this feature does, or what the implications of clicking this new button are?  How will they see pages they've marked?  How will they know if other people see what they have "marked"?)

But assuming it's going to happen anyway, this patch seems to be on the right path.

::: browser/base/content/browser-sets.inc
@@ +345,5 @@
>  # Cmd+I is conventially mapped to Info on MacOS X, thus it should not be
>  # overridden for other purposes there.
>      <key id="viewBookmarksSidebarWinKb" key="&bookmarksWinCmd.commandkey;" command="viewBookmarksSidebar" modifiers="accel"/>
>  #endif
>      

might as well kill this trailing whitespace while touching this area

::: browser/base/content/browser-social.js
@@ +585,5 @@
>  
> +  // Called when the Social.provider changes
> +  update: function SSB_updateButtonState() {
> +    let markButton = this.button;
> +    if (markButton) {

I think we are moving away from checking if such elements exist (ie, the if () can probably go)

@@ +610,3 @@
>    },
>  
> +  markPage: function SSB_markPage() {

I'd be inclined to call this "togglePageMark" or similar, and drop the unmarkPage function (ie, just do those 2 line inline here)

@@ +619,4 @@
>      }
> +  },
> +  
> +  unmarkPage: function() {

trailing whitespace

@@ +733,5 @@
>        }
>  
>        let tbi = document.getElementById("social-toolbar-item");
>        if (tbi) {
> +        let next = tbi.lastChild.previousSibling;

isn't this just:

while (tbi.firstChild != tbi.lastChild)
  tbi.removeChild(tbi.firstChild);
?

::: browser/base/content/test/social/browser_social_multiprovider.js
@@ +59,5 @@
>  
>  function checkUIStateMatchesProvider(provider) {
>    let profileData = getExpectedProfileData(provider);
> +  // Bug 789863 - mark button uses 'displayName', toolbar uses 'userName'
> +  // Check the "mark button"

It looks like the element socialUserDisplayName was removed from browser.xul?

::: browser/base/content/test/social/social_worker.js
@@ +121,5 @@
>            topic: "social.user-recommend-prompt-response",
>            data: {
>              images: {
>                // this one is relative to test we handle relative ones.
> +              mark: "/browser/browser/base/content/test/social/social_mark_image.png",

as per a comment later, it makes sense to me that the entire api should be renamed (ie, people aren't necessarily going to associate "recommend" with "mark".)  Indeed, "mark" seems an interesting choice of name in general - the name really doesn't imply anything to me :)

::: browser/modules/Social.jsm
@@ +229,3 @@
>    },
>  
> +  markPage: function Social_markPage(aURI) {

don't we need to track this per provider?  ie, it seems wrong that if you "mark" it using provider1, you can unmark it using provider2.

::: browser/themes/linux/browser.css
@@ +1468,4 @@
>    border-top-width: 0;
>  }
>  
>  #socialUserDisplayName:hover {

it looks like socialUserDisplayName was removed?  Are there any other removed element from the share panel that should have the css removed?

ditto for the other browser.css files

::: toolkit/components/social/SocialService.jsm
@@ +519,5 @@
>      // Accept *and validate* the user-recommend-prompt-response message from
>      // the provider.
>      let promptImages = {};
>      let promptMessages = {};
>      function reportError(reason) {

any reason we are still calling this "recommendInfo" at the api level?  Seeing we are changing the data from the provider, it would make sense to me to change the api name too.

Comment 8

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #7)

> @@ +733,5 @@
> >        }
> >  
> >        let tbi = document.getElementById("social-toolbar-item");
> >        if (tbi) {
> > +        let next = tbi.lastChild.previousSibling;
> 
> isn't this just:
> 
> while (tbi.firstChild != tbi.lastChild)
>   tbi.removeChild(tbi.firstChild);
> ?

No, we do not want to remove the first and last child nodes, which are the provider and mark buttons.  The loop removes everything between the two.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Attachment: social-marks.patch

this version addresses all of mark's feedback on marks

:)

Comment 10

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 729181 [details] [diff] [review]
social-marks.patch

cancelling review, there is one last thing I didn't address that is necessary.

Comment 11

[Shane Caraveo (:mixedpuppy)]

Attachment: social-marks.patch

This patch addresses the last item from Marks feedback, which was properly storing marks per-provider.  The previous implementation stored globally.  I've replaced a variable off the class with annotations in places.  That allows the mark state to live across sessions (without bookmarks) but the marks get removed if history is cleared.  

Gavin, is this use of annotations fine?

Comment 12

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 729353 [details] [diff] [review]
social-marks.patch

Mark, what do you think of the api changes?  There is only a config call that the provider can do at any time, rather than asking them for the config.

Comment 13

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

Comment on attachment 729353 [details] [diff] [review]
social-marks.patch

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

I think the provider sending this unsolicited is fine

::: browser/modules/Social.jsm
@@ +248,5 @@
> +    let url = this._getMarkablePageUrl(aURI);
> +    let providerList = [];
> +    try {
> +      let val = PlacesUtils.annotations.getPageAnnotation(aURI, "social/mark");
> +      providerList = JSON.parse(val);

isPageMarked seems to treat an exception getting the value as "normal", so I doubt we want the reportError() here - maybe it should only be written if the json parse fails?

Comment 14

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

Comment on attachment 729353 [details] [diff] [review]
social-marks.patch

Not sure which part of this you want me to comment on. Using Places annotations to track "marked" state across sessions in general seems like a fine idea.

Comment 15

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

Marco may have additional thoughts about the code specifically.

From a user perspective, how this interacts with the star button is likely to cause user confusion, particularly after bug 748894. Seems like we'd need to solve that somehow before we can land this.

Comment 16

[Marco Bonardo [:mak]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #14)
> Not sure which part of this you want me to comment on. Using Places
> annotations to track "marked" state across sessions in general seems like a
> fine idea.

Yes, the idea looks fine, but a synchronous API is not. Annotations are synchronous, but that's going to change as soon as we have resources for that, so I think your code should be ready to cope with an async storage for these annotations, you can easily wrap the current sync API in a fake-async helper for now, and we'll just replace it in-place when ready. You may register an annotation observer and keep it updated (the star button does something similar, it updates asynchronously and then keeps the status updated, to avoid IO).

Also notice in PB mode we don't store URIs, so you can't annotate them, this should be taken into account.

Finally, I definitely share Gavin's concern regarding the star icon, I think we should not re-use the star, would be just too confusing for the user.

Comment 17

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #15)
> From a user perspective, how this interacts with the star button is likely
> to cause user confusion, particularly after bug 748894. Seems like we'd need
> to solve that somehow before we can land this.

This is just a rev of the recommend functionality that already exists.  I discussed removal vs. keep/update with Asa and we decided to keep/update it and deal with the crossover of functionality post-social-v.1.

The star icon is not used, I only used it while working on the patch, thus it's inclusion in the screenshots.

Comment 18

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 729353 [details] [diff] [review]
social-marks.patch

I'll make the annotations async as suggested before review.

Comment 19

[Shane Caraveo (:mixedpuppy)]

(In reply to Marco Bonardo [:mak] (Away Mar 27 - Apr 2) from comment #16)
> Also notice in PB mode we don't store URIs, so you can't annotate them, this
> should be taken into account.

social is currently completely disabled in social, so I don't think this should be a problem.  I'll add pb checks anyway.

Comment 20

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #19)
> (In reply to Marco Bonardo [:mak] (Away Mar 27 - Apr 2) from comment #16)
> > Also notice in PB mode we don't store URIs, so you can't annotate them, this
> > should be taken into account.
> 
> social is currently completely disabled in social, so I don't think this
> should be a problem.  I'll add pb checks anyway.

actually, looking at this again, I don't feel it's necessary, we never show the UI in pb mode, so the additional check would be redundant.

Comment 21

[Shane Caraveo (:mixedpuppy)]

Attachment: social-marks.patch

This patch makes our APIs async in preparation for async annotation API.

Comment 22

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 729845 [details] [diff] [review]
social-marks.patch

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

feedback+ because I still need to give it a second pass, but it looks good. Here's some feedback:

The XUL structure that this is creating seems fragile, with the trickery to insert the other toolbar buttons using insertBefore, and to remove them by starting from the second-to-last child and going backwards..  Can't this button be created dynamically like the others? That would be ideal I think, but if it's not possible you could also let it before the other elements on XUL and use moz-box-ordinal-group to move it to the end.

::: browser/modules/Social.jsm
@@ +243,5 @@
> +      aCallback(!!val);
> +    }.bind(this));
> +  },
> +  
> +  togglePageMark: function(aURI, aCallback) {

I'm usually not a fan of toggle functions because what will it do is uncertain. It's fine for the UI command to be toggle because of the button, but then to reach the backend I think it'd be better if the caller were to determine if the page is marked or not (or what it thinks is the truth, e.g. what is being shown to the user), and then call a specific function to mark or unmark it

Comment 23

[Shane Caraveo (:mixedpuppy)]

(In reply to :Felipe Gomes from comment #22)
> Comment on attachment 729845 [details] [diff] [review]
> social-marks.patch
> 
> Review of attachment 729845 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> feedback+ because I still need to give it a second pass, but it looks good.
> Here's some feedback:
> 
> The XUL structure that this is creating seems fragile, with the trickery to
> insert the other toolbar buttons using insertBefore, and to remove them by
> starting from the second-to-last child and going backwards..  Can't this
> button be created dynamically like the others? That would be ideal I think,
> but if it's not possible you could also let it before the other elements on
> XUL and use moz-box-ordinal-group to move it to the end.

Neither of these work well.  The mark and notification buttons can be updated separately at different times, so adding the mark button dynamically will just complicate things.  Using the ordinal complicates css (if even possible) since a lot of the styling depends on :last-child, which does not take into account ordinal.

> ::: browser/modules/Social.jsm
> @@ +243,5 @@
> > +      aCallback(!!val);
> > +    }.bind(this));
> > +  },
> > +  
> > +  togglePageMark: function(aURI, aCallback) {
> 
> I'm usually not a fan of toggle functions because what will it do is
> uncertain. It's fine for the UI command to be toggle because of the button,
> but then to reach the backend I think it'd be better if the caller were to
> determine if the page is marked or not (or what it thinks is the truth, e.g.
> what is being shown to the user), and then call a specific function to mark
> or unmark it

Dueling feedback.  IMO either way doesn't matter, happy to change it back, but I wont change it again.

Comment 24

[Shane Caraveo (:mixedpuppy)]

Attachment: social-marks.patch

added feedback.  added context menus for marking page and marking links.

Comment 25

[Shane Caraveo (:mixedpuppy)]

Attachment: social-marks.patch

using promise and thread to make sure get/set annotation is async to the rest of the social code.

Comment 26

[Shane Caraveo (:mixedpuppy)]

Did a quick try build of bug 818675 and this bug at 
https://tbpl.mozilla.org/?tree=Try&rev=87d26b666ae1

Shows there are a few issues to got through still.

Comment 27

[Shane Caraveo (:mixedpuppy)]

socialmark only try to verify test fixes
https://tbpl.mozilla.org/?tree=Try&rev=a227a6be9387

Comment 28

[Shane Caraveo (:mixedpuppy)]

Attachment: social-marks.patch

updated to current m-c.  fixed mochitest-plain tests and probably async test failure on windows.  carrying forward r+.  new try

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

Comment 29

[Shane Caraveo (:mixedpuppy)]

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

Comment 30

[Ed Morley [:emorley]]

Backed out for:
https://tbpl.mozilla.org/php/getParsedLog.php?id=22228530&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ed39a9db0fb1

Comment 31

[aja+bugzilla]

FYI: saw following symptoms:
nav bar shows large set of icons after search and before bookmark arrow button
...looking almost as if an entire icon sprite file was being shown instead of just a single icon.

Comment 32

[Francesco Lodolo [:flod]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #0)
> - recommend button remains, we start calling it "marks"

Can someone explain the reasoning behind this choice? It could definitely help localizers finding a good translation, maybe this explanation could be even worth a localization comment in browser.dtd

Comment 33

[Shane Caraveo (:mixedpuppy)]

(In reply to Francesco Lodolo [:flod] from comment #32)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #0)
> > - recommend button remains, we start calling it "marks"
> 
> Can someone explain the reasoning behind this choice? It could definitely
> help localizers finding a good translation, maybe this explanation could be
> even worth a localization comment in browser.dtd

It is marking a url for something, whether as a bookmark, a "like", "save for later" or +1.  I'm open to different names if someone has a better thought.

Comment 34

[Francesco Lodolo [:flod]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #33)
> It is marking a url for something, whether as a bookmark, a "like", "save
> for later" or +1.  I'm open to different names if someone has a better
> thought.

Given a "mark/unmark" button I would have no idea about its function. I get a "mark as something", just "mark" sounds like something is missing (but English is not my primary language).

Comment 35

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

IIUC, these labels (and hence the term "Mark") will never be exposed to the user but will instead come from providers - so, eg, for Facebook the term exposed will be "Like this page".  Shane tells me over IRC that these labels are to help the tests pass, but IMO we should find another way to make the tests pass and use empty labels on the UI elements (like a number of other social elements already do) - ie, this patch should be able to be made to work without introducing *any* new strings (although the patch should probably remove any existing strings which fall into the same category - eg, the old "recommend" ones.)

Comment 36

[Francesco Lodolo [:flod]]

(In reply to Mark Hammond (:markh) from comment #35)
> IIUC, these labels (and hence the term "Mark") will never be exposed to the
> user but will instead come from providers ...

Thanks Mark for the explanation. 

If that's the case I completely agree with you: If strings are not visible in the interface and needed just to pass tests, I don't see why they should be localizable.

Comment 37

[Shane Caraveo (:mixedpuppy)]

Finally figured out the test failure on winxp: 
https://tbpl.mozilla.org/?tree=Try&rev=3f9f645be152

The winxp-debug-only test failure was the result of a minor async issue that probably only showed up in some extreme case.

The previous changes to fix the test commented on in comment 28 is removed, the real error in that was a small logic error in nsContextMenu.js.  That also removes the entries in the dtd discussed in comment 32 and onwards.

Will run a full try now and update the patch here.

Comment 38

[Shane Caraveo (:mixedpuppy)]

Attachment: social-marks.patch

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

fixed the tests.  carrying forward r+, the changes between what felipe r+'d and this patch are trivial.

Comment 39

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/29c172213ff7

Comment 40

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/29c172213ff7

Comment 41

[Boris Zbarsky [:bzbarsky]]

This seems to have caused significant performance regressions:

Regression: Mozilla-Inbound-Non-PGO - Tp5 Optimized - Win7 - 19.9% increase
Regression: Mozilla-Inbound-Non-PGO - Ts, Paint - Win7 - 8.24% increase
Regression: Mozilla-Inbound-Non-PGO - Tp5 Optimized - Ubuntu HW 12.04 - 22.5% increase
Regression: Mozilla-Inbound-Non-PGO - Tp5 Optimized - Ubuntu HW 12.04 x64 - 25.5% increase
Regression: Mozilla-Inbound-Non-PGO - Paint - Win7 - 30.7% increase

etc, etc.

Comment 42

[Matt Brubeck (:mbrubeck)]

Backed out on inbound because of the regressions, sorry:
https://hg.mozilla.org/integration/mozilla-inbound/rev/414f3b4cf669
https://groups.google.com/d/topic/mozilla.dev.tree-management/28Q5OKh_HDA/discussion

Comment 43

[Shane Caraveo (:mixedpuppy)]

looks like I got the issue resolved:

https://tbpl.mozilla.org/?tree=Try&rev=38a551992a33

running a full try at:

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

Comment 44

[Shane Caraveo (:mixedpuppy)]

Attachment: social-marks.patch

patch with paint/startup regression fix.  carrying forward r+

https://hg.mozilla.org/integration/mozilla-inbound/rev/71018176584e

Comment 45

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/71018176584e

Comment 46

[u279076]

QA has finished testing this functionality and feels there's no blockers for release in Firefox 23, marking verified.