Closed Bug 853151 Opened 7 years ago Closed 7 years ago

refactoring of recommend into SocialMark

Categories

(Firefox Graveyard :: SocialAPI, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 23

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

(Keywords: uiwanted)

Attachments

(3 files, 9 obsolete files)

Attached image 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
Attachment #727352 - Flags: ui-review?(jboriss)
Duplicate of this bug: 804930
Attached image social-marked.png
Attachment #727353 - Flags: ui-review?(jboriss)
Attached patch social-marks.patch (obsolete) — Splinter Review
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)
Attachment #727356 - Flags: feedback?(mhammond)
Attachment #727356 - Flags: feedback?(felipc)
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
              }
            }
          });
  },
Keywords: uiwanted
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.
Attached patch social-marks.patch (obsolete) — Splinter Review
updated patch removes default images I initially used while working on this patch.
Attachment #727356 - Attachment is obsolete: true
Attachment #727356 - Flags: feedback?(mhammond)
Attachment #727356 - Flags: feedback?(felipc)
Attachment #727940 - Flags: feedback?(mhammond)
Attachment #727940 - Flags: feedback?(felipc)
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.
Attachment #727940 - Flags: feedback?(mhammond) → feedback+
(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.
Attached patch social-marks.patch (obsolete) — Splinter Review
this version addresses all of mark's feedback on marks

:)
Attachment #727940 - Attachment is obsolete: true
Attachment #727940 - Flags: feedback?(felipc)
Attachment #729181 - Flags: review?(felipc)
Comment on attachment 729181 [details] [diff] [review]
social-marks.patch

cancelling review, there is one last thing I didn't address that is necessary.
Attachment #729181 - Flags: review?(felipc)
Attached patch social-marks.patch (obsolete) — Splinter Review
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?
Attachment #729181 - Attachment is obsolete: true
Attachment #729353 - Flags: review?(felipc)
Attachment #729353 - Flags: feedback?(gavin.sharp)
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.
Attachment #729353 - Flags: feedback?(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?
Attachment #729353 - Flags: feedback?(mhammond) → feedback+
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.
Attachment #729353 - Flags: feedback?(gavin.sharp) → feedback+
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.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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 on attachment 729353 [details] [diff] [review]
social-marks.patch

I'll make the annotations async as suggested before review.
Attachment #729353 - Flags: review?(felipc)
(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.
(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.
Attached patch social-marks.patch (obsolete) — Splinter Review
This patch makes our APIs async in preparation for async annotation API.
Attachment #729353 - Attachment is obsolete: true
Attachment #729845 - Flags: review?(felipc)
Attachment #727352 - Flags: ui-review?(jboriss) → ui-review+
Attachment #727353 - Flags: ui-review?(jboriss) → ui-review+
Assignee: nobody → mixedpuppy
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
Attachment #729845 - Flags: feedback+
(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.
Attached patch social-marks.patch (obsolete) — Splinter Review
added feedback.  added context menus for marking page and marking links.
Attachment #729845 - Attachment is obsolete: true
Attachment #729845 - Flags: review?(felipc)
Attachment #737045 - Flags: review?(felipc)
Attached patch social-marks.patch (obsolete) — Splinter Review
using promise and thread to make sure get/set annotation is async to the rest of the social code.
Attachment #737045 - Attachment is obsolete: true
Attachment #737045 - Flags: review?(felipc)
Attachment #738255 - Flags: review?(felipc)
Blocks: 806320
Summary: revamp the recommend button → refactoring of recommend into SocialMark
Blocks: 790811
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.
Attachment #738255 - Flags: review?(felipc) → review+
socialmark only try to verify test fixes
https://tbpl.mozilla.org/?tree=Try&rev=a227a6be9387
Attached patch social-marks.patch (obsolete) — Splinter Review
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
Attachment #738255 - Attachment is obsolete: true
Attachment #741481 - Flags: review+
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
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
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.
(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
(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.
(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).
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.)
(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.
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.
Attached patch social-marks.patch (obsolete) — Splinter Review
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.
Attachment #741481 - Attachment is obsolete: true
Attachment #744919 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/29c172213ff7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
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.
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
patch with paint/startup regression fix.  carrying forward r+

https://hg.mozilla.org/integration/mozilla-inbound/rev/71018176584e
Attachment #744919 - Attachment is obsolete: true
Attachment #745578 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/71018176584e
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
QA has finished testing this functionality and feels there's no blockers for release in Firefox 23, marking verified.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.