Closed Bug 1000766 Opened 10 years ago Closed 10 years ago

Desktop client needs ability to view contacts

Categories

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

Product:
Hello (Loop)
Component:
Client
Type:
defect
Points:
5

Tracking

(firefox34 verified, firefox35 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla35
Iteration:
35.2
Tracking Flags:
Tracking Status
firefox34 --- verified
firefox35 --- verified

People

(Reporter: RT, Assigned: mikedeboer)

References

(
URL
)

Details

(Whiteboard: [contacts][first release needed] [loop-uplift][fig:wontverify])

User Story

As a signed-in FF browser user I can view a list of my local contacts with names and pictures so that I can identify them prior to calling them.

Attachments

(4 files, 17 obsolete files)

contacts.png
63.09 KB, image/png
Details
Patch 2.1: add support for Gravatar avatars
5.52 KB, patch
Paolo
: review+
Details | Diff | Splinter Review
Patch v1.8: Add a contacts list to the contacts tab
41.67 KB, patch
Paolo
: review+
lmandel
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Patch 3.1: add a visual separator between available and blocked contacts
4.08 KB, patch
Paolo
: feedback+
Details | Diff | Splinter Review 
      No description provided.
User Story: (updated)
No longer blocks: 972015
Blocks: 972015
Priority: -- → P2
Whiteboard: [s=ui32]
Target Milestone: --- → mozilla32
Whiteboard: [s=ui32] → p=?
Target Milestone: mozilla32 → mozilla33
Details on contacts display:
* Only the top 5 most frequent/recently contacted contacts are displayed to avoid having a list of 1000 contacts to scroll through. 
* Each contact line displays the friendly name of the contact (if no friendly name display ID), the contact avatar (if no avatar display default avatar), an indicator if the contact was imported from Google, the date/time of the last interaction with the contact as well as an indicator about the call type (audio only or audio+video).
Attached image contacts.png — 

    
  
Priority: P2 → P1
Target Milestone: mozilla33 → mozilla34
Depends on: 1036192

    
  
Whiteboard: p=? → [p=?, contacts]

    
  
Depends on: 1038716

    
  
Target Milestone: mozilla34 → mozilla35
Whiteboard: [p=?, contacts] → [p=?, contacts][first release needed]
Depends on: 1046587

    
  
Points: --- → 5
Whiteboard: [p=?, contacts][first release needed] → [contacts][first release needed]

    
  
Whiteboard: [contacts][first release needed] → [p=5, contacts][first release needed]

    
  
Priority: P1 → P3
Target Milestone: mozilla35 → mozilla34

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP for early feedback (obsolete)
        — 
          — Splinter Review
      

    


  
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED

    
    

    
  
Marco, I'm taking this bug for the current iteration.
Iteration: --- → 34.1
Flags: needinfo?(mmucci)
Whiteboard: [p=5, contacts][first release needed] → [p=5, contacts][first release needed][qa+]

    
  

        Attachment #8466190 -
        Flags: feedback?(nperriault)

    
    

    
  
Added to Iteration 34.1
Flags: needinfo?(mmucci) → firefox-backlog+
Whiteboard: [p=5, contacts][first release needed][qa+] → [contacts][first release needed] [qa+]

    
    

    
  
Comment on attachment 8466190 [details] [diff] [review]
WIP for early feedback

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

Just gave a quick look to the code, the Tab thing is on the right track, though I raised some concern with some bits of the design… Let's pair on this monday :)

::: browser/components/loop/content/js/panel.jsx
@@ +29,5 @@
> +    },
> +
> +    onSelect: function(which) {
> +      if (this.props.onSelect)
> +        this.props.onSelect(which);

I'm surprised not seeing component state updated when a new tab is selected…

@@ +37,5 @@
> +      var cx = React.addons.classSet;
> +      return (
> +        <div className="tab-view-container">
> +          <ul className="tab-view">
> +            <li className={cx({"selected": this.state.selectedTab == "call"})}

This probably wants to be extracted as its own TabMenuItem component.

@@ +38,5 @@
> +      return (
> +        <div className="tab-view-container">
> +          <ul className="tab-view">
> +            <li className={cx({"selected": this.state.selectedTab == "call"})}
> +                data-tip="Call Tab"

Curious: what is this used for? :)

@@ +39,5 @@
> +        <div className="tab-view-container">
> +          <ul className="tab-view">
> +            <li className={cx({"selected": this.state.selectedTab == "call"})}
> +                data-tip="Call Tab"
> +                onClick={this.onSelect.bind(this, "call")}>

Debatable nit: I'd avoid binding here (don't you get a notice from React about this in the console?)

Rather, I'd check for a data attribute identifying the tab id from the received event:

<li data-tab-id="contacts" onClick={this.handleSelectTab}…

handleSelectTab: function(event) {
  this.setState({selectedTab: event.target.dataset["tab-id"]})
}

@@ +48,5 @@
> +                onClick={this.onSelect.bind(this, "contacts")}>
> +              <i className="fa fa-book"></i>
> +            </li>
> +          </ul>
> +          {this.props.children}

I could see a .map operation returning the Tab component with its selected prop updated according to current state.selectedTab value.

@@ +279,5 @@
> +      which = which || "call";
> +      this.setState({selectedTab: which});
> +      this.refs.tabView.setState({selectedTab: which});
> +
> +      if (which == "contacts" && !this.contactListenersAdded) {

Most of this block should be moved to the ContactsList componentDidMount hook.

@@ +398,5 @@
> +        this.panelView.setProps({
> +          client: client
> +        });
> +      }
> +      this.panelView.selectTab();

I don't get why this whole setup is needed… Why don't we just load the PanelView once then update its state according to routing events?

::: browser/components/loop/content/panel.html
@@ +6,5 @@
>    <head>
>      <meta charset="utf-8">
>      <title>Loop Panel</title>
>      <link rel="stylesheet" type="text/css" href="loop/shared/css/common.css">
> +    <link rel="stylesheet" type="text/css" href="http://netdna.bootstrapcdn.com/font-awesome/4.1.0/css/font-awesome.min.css">

Heh, I don't think we'll want to keep this :)

        Attachment #8466190 -
        Flags: feedback?(nperriault) → feedback+
Iteration: 34.1 → 34.2
QA Contact: anthony.s.hughes

    
  
Assignee: mdeboer → nobody
Status: ASSIGNED → NEW
Iteration: 34.2 → ---

    
  
Blocks: 1000112

    
    

    
  
Paolo and I looked over this bug, and pretty quickly came to the conclusion that it's not really in a state where it can easily be picked up and driven forward just by writing tests. In particular:

* only Mike can answer the questions in Niko's feedback bug, and those might change the structure of the patch non-trivially
* the visual uplift stuff has caused significant bitrot, and is going to require non-trivial context to merge without introducing CSS issues
* attempting to refactor the existing code into tabs as well as implement contacts in a tab in one patch isn't really a good fit for someone's first React patch
 
So the conclusion we came to was that it probably makes the most sense to wait for Mike to get back, and than Paolo sounded interested in joining the pairing session/discussion between Mike and Niko mentioned in comment 6.

    
  
Depends on: 1064414

    
  
Assignee: nobody → mdeboer

    
    

    
  
(In reply to Romain Testard [:RT] from comment #1)
> Details on contacts display:
> * Only the top 5 most frequent/recently contacted contacts are displayed to
> avoid having a list of 1000 contacts to scroll through. 

Romain, this is a wild deviation from the principle of a contacts list; I'd like to know more about the reasoning behind this limitation:
1) Is it to optimize UX? If so, how can the user get to this list of 5 most popular contacts from day 0 (say, a fresh Google Contacts import)?
2) How will contact imports be visualized? If we only keep displaying 5 contacts, it will look like the import failed.
3) How will a user find the less popular contacts to call?
Status: NEW → ASSIGNED
Flags: needinfo?(rtestard)

    
  
Depends on: 1065275

    
  
Iteration: --- → 35.1
Priority: P3 → P1
Whiteboard: [contacts][first release needed] [qa+] → [contacts][first release needed] [qa+][loop-uplift]
Target Milestone: mozilla34 → mozilla35
Flags: qe-verify+
Whiteboard: [contacts][first release needed] [qa+][loop-uplift] → [contacts][first release needed] [loop-uplift]
Iteration: 35.1 → ---

    
    

    
  
(In reply to Mike de Boer [:mikedeboer] from comment #8)
> (In reply to Romain Testard [:RT] from comment #1)
> > Details on contacts display:
> > * Only the top 5 most frequent/recently contacted contacts are displayed to
> > avoid having a list of 1000 contacts to scroll through. 
> 
> Romain, this is a wild deviation from the principle of a contacts list; I'd
> like to know more about the reasoning behind this limitation:
> 1) Is it to optimize UX? If so, how can the user get to this list of 5 most
> popular contacts from day 0 (say, a fresh Google Contacts import)?
This is a similar approach to Android address book or Hangouts client where you build the most frequent/recent list as you use your product. I assumed we start with the full list and as we get enough history we can start displaying the list. Darrin do you agree?

> 2) How will contact imports be visualized? If we only keep displaying 5
> contacts, it will look like the import failed.
Fair point, Darrin do you have thoughts here?

> 3) How will a user find the less popular contacts to call?
Using the contact list filtering would help to find a contact although it got de-scoped for FF34

This is a FF35 item given it depends on contact list filtering, which itself has now been pushed to FF35.
Flags: needinfo?(rtestard) → needinfo?(dhenein)

    
    

    
  
I discussed things with Darrin yesterday and we came to the following conclusion:

 - the 5 contacts list is a list that will appear in a different tab than the address book tab, thus will need to be implemented in a different bug
 - the address book/ contacts list should be a view that lists _all_ the contacts in the database. This will be implemented in this bug.
 - the contacts list filtering will be implemented as quickly as possible, with the intent to make it for Fx 34. (nit: Fx is the correct abbreviation for Firefox, not FF ;) )

We'll be taking Darrin's Loop MVP spec (see URL field) as primary guideline for the implementation.
URL: https://people.mozilla.org/~dhenein/l...
Flags: needinfo?(dhenein)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP for early feedback - redux (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8466190 -
        Attachment is obsolete: true

        Attachment #8488636 -
        Flags: feedback?(paolo.mozmail)

        Attachment #8488636 -
        Flags: feedback?(nperriault)

    
    

    
  
Incremental, interval-based list build up and `shouldComponentUpdate` integration not yet in this patch!

    
    

    
  
Comment on attachment 8488636 [details] [diff] [review]
WIP for early feedback - redux

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

I started reviewing, then I realized you were reusing the component code from the mockups… Don't pay too much attention to my first comments ;)

Side note: I really think this should be redone from scratch, iteratively, possibly reusing part of the provided semantics & style definitions.

I'll hold on reviewing for now so we can discuss and make a decision first.

::: browser/base/content/browser.xul
@@ +268,5 @@
>             position="topcenter topright"/>
>  
>      <panel id="loop-notification-panel"
>             class="loop-panel social-panel"
> +           noautohide="true"

Don't forget to remove this :p

::: browser/components/loop/content/js/contacts.jsx
@@ +37,5 @@
> +
> +  var SearchBar = React.createClass({
> +    render: function() {
> +      return (
> +        <div className="searchBarWrapper">

Nit: how about using a regular <form/> tag? We could always prevent its submission.

@@ +39,5 @@
> +    render: function() {
> +      return (
> +        <div className="searchBarWrapper">
> +          <input ref="searchInput" className="searchBar" value={this.props.val}
> +                 onChange={this.props.handleChange} placeholder="Search..."/>

Placeholder text needs translation.

@@ +48,5 @@
> +
> +  var ContactDetail = React.createClass({
> +    getInitialState: function() {
> +      return {
> +        isDropdownVisible: false,

Please check the DropdownMenuMixin which probably features what you want already.

@@ +58,5 @@
> +      this.props.onClick(this.props.key);
> +    },
> +
> +    hideDropdown: function() {
> +      $(this.refs.callDropdown.getDOMNode()).hide();

I which we could avoid using jQuery and even relying on hiding DOM nodes by hand here. Just testing for state value in render() should allow us to decide if we want to render the submenu or not.

@@ +74,5 @@
> +      }
> +      this.setState({
> +        isDropdownVisible: !this.state.isDropdownVisible
> +      });
> +    },

Same remark as above. Also, please ensure to have a look at different dropdown menus already implemented and using the aforementioned mixin.

@@ +77,5 @@
> +      });
> +    },
> +
> +    toggleBlocked: function(){
> +      this.props.user.blocked = !this.state.blocked;

Props should always be kept immutable. You need to rely on state to change this value.

Have a look at how I've done it in http://jsbin.com/danaka/2/edit

@@ +84,5 @@
> +      });
> +    },
> +
> +    render: function(){
> +      var blocked = this.state.blocked ? " blocked" : "";

This wants to be managed using React.addons.classSet I guess.

Note: this is were I realized this was basically the code from the mockups.

::: browser/components/loop/content/panel.html
@@ +17,5 @@
>  
>      <script type="text/javascript" src="loop/shared/libs/react-0.11.1.js"></script>
>      <script type="text/javascript" src="loop/libs/l10n.js"></script>
>      <script type="text/javascript" src="loop/shared/libs/jquery-2.1.0.js"></script>
> +    <script type="text/javascript" src="loop/shared/libs/jquery.velocity.min.js"></script>

Huuuu, no please ^^'

    
    

    
  
Comment on attachment 8488636 [details] [diff] [review]
WIP for early feedback - redux

I've not much more to add to what NiKo said, I'd also like to see this patch to be split in multiple dependent bugs.

Apart from that, the main issue I observed while testing is that the dropdown menu for contacts on the lower part of the screen triggers an overflow of the scrollable area. Is there a way to change the orientation of the popup, or another solution that prevents this from happening?

        Attachment #8488636 -
        Flags: feedback?(paolo.mozmail)

    
    

    
  


  
I limited the scope of this patch a lot compared to the previous WIP patch(es).

I can't get the gravatar images to show up, so I'm looking for feeback in that area!

        Attachment #8488636 -
        Attachment is obsolete: true

        Attachment #8488636 -
        Flags: feedback?(nperriault)

        Attachment #8490083 -
        Flags: review?(nperriault)
Iteration: --- → 35.2

    
    

    
  
Comment on attachment 8490083 [details] [diff] [review]
Patch v1: Add a contacts list to the contacts tab

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

This looks pretty good as a first step and heads towards the right direction. I wasn't capable of applying the patch cleanly locally, so I'm only commenting relying on pure theory here.

::: browser/components/loop/content/js/contacts.jsx
@@ +11,5 @@
> +loop.contacts = (function(_, mozL10n) {
> +  "use strict";
> +
> +  // Aliasing translation function as __ for concision.
> +  var __ = mozL10n.get;

Nit: we're in the process of removing this alias elsewhere, so please use mozL10n.get directly in this file.

@@ +13,5 @@
> +
> +  // Aliasing translation function as __ for concision.
> +  var __ = mozL10n.get;
> +  // Number of contacts to add to the list at the same time.
> +  var kContactsChunksize = 100;

Nit (flamewar prone): I know this is gecko naming convention, but for Loop (Web) content code we've been generally avoiding using it… I suggest that with postpone making a decision about this later on, with its own bug filed, in order to keep things consistent for now.

Also, as this is supposedly desktop only code, couldn't we use a const here? could be appropriately named CONTACTS_CHUNK_SIZE.

@@ +33,5 @@
> +      this.props.handleContactClick(this.props.key);
> +    },
> +
> +    render: function() {
> +      var username = this.props.contact.name[0].split(" ");

Couldn't we just get some Contact model object passed in, so we could simply this.props.contact.getUsername()? Could be addressed in a follow-up patch though.

@@ +37,5 @@
> +      var username = this.props.contact.name[0].split(" ");
> +      var contactCSSClass = React.addons.classSet({
> +        contact: true,
> +        selected: this.props.selected,
> +        blocked: this.state.blocked

I'm wondering why `selected` is handled with a prop, while `blocked` is kept in state. As I can't find any place where state.blocked is updated, it could probably be a prop.

@@ +47,5 @@
> +          email = address;
> +          return true;
> +        }
> +        return false;
> +      });

var email = this.props.contact.getPreferredEmailAddress() (or even contact.email.prefered) would be nice.

While updating the contacts API is definitely not in the scope of this bug, in the meanwhile I think we'll want to extract this kind of computation in private methods attached to the component prototype.

@@ +51,5 @@
> +      });
> +
> +      return (
> +        <div onClick={this.handleContactClick} className={contactCSSClass}>
> +          <div className="avatar" data-avatar={"http://www.gravatar.com/avatar/" + email.md5value + ".jpg?s=80"}></div>

Let's extract the gravatar URL computation to a method, so we can write <div className="avatar" data-avatar={this._getGravatarUrl()}/>

Also please note you can safely use self-closing tags in JSX.

@@ +53,5 @@
> +      return (
> +        <div onClick={this.handleContactClick} className={contactCSSClass}>
> +          <div className="avatar" data-avatar={"http://www.gravatar.com/avatar/" + email.md5value + ".jpg?s=80"}></div>
> +          <div className="details">
> +            <div className="username"><strong>{username.shift()}</strong> {username.join(" ")}

username.shift() seems a bit odd, I'd rather see contactObj.displayName or similar. And joining back the parts inline in JSX should be avoided imho.

@@ +55,5 @@
> +          <div className="avatar" data-avatar={"http://www.gravatar.com/avatar/" + email.md5value + ".jpg?s=80"}></div>
> +          <div className="details">
> +            <div className="username"><strong>{username.shift()}</strong> {username.join(" ")}
> +              <i className={this.props.contact.category[0] == "google" ? "icon icon-google" : ""}></i>
> +              <i className={this.state.blocked ? "icon icon-blocked" : ""}></i>

How about using React.addons.classSet as well for these two nodes? Also, self-closing tags could be used here.

@@ +77,5 @@
> +        selected: -1,
> +        faded: !!this.props.faded,
> +        contacts: this.props.contacts || {}
> +      };
> +      return state;

Nit: you can directly return the object literal here.

@@ +81,5 @@
> +      return state;
> +    },
> +
> +    componentDidMount: function() {
> +      var contactsAPI = navigator.mozLoop.contacts;

Actually I wish this dependency was passed in through a prop, to ease testing by injecting a mocked version.

As we're in the process of redesigning the way we deal with deps, state, events and actions but aren't quite there yet, this is probably good enough for now; but please note this will probably be subject to change later on.

@@ +82,5 @@
> +    },
> +
> +    componentDidMount: function() {
> +      var contactsAPI = navigator.mozLoop.contacts;
> +      var list = this;

Nit: How about binding instead of aliasing? Matter of taste obviously, but I tend to favor the former approach.

@@ +103,5 @@
> +
> +        addContactsInChunks(contacts);
> +
> +        // Listen for contact changes/ updates.
> +        contactsAPI.on("add", function(e, contact) {

We need to handle errors here. We could create an higher order function for that:

function errorHandler(fn) {
  return function(err, contact) {
    if (err) {
      // do smthg useful with the error, eg. updating the UI through state change
      return;
    }
    fn(contact);
  };
}

Usage:

contactsAPI.on("add", errorHandler(list.handleContactAdd));

@@ +170,5 @@
> +      return (
> +        <div className="listWrapper">
> +          <div ref="listSlider" className="listPanels">
> +            <div className={"list " + (this.state.faded ? "faded": "")}>
> +                <ul>

Nit: indentation.

        Attachment #8490083 -
        Flags: review?(nperriault) → feedback+

    
    

    
  
(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #16)
> @@ +13,5 @@
> > +
> > +  // Aliasing translation function as __ for concision.
> > +  var __ = mozL10n.get;
> > +  // Number of contacts to add to the list at the same time.
> > +  var kContactsChunksize = 100;
> 
> Nit (flamewar prone): I know this is gecko naming convention, but for Loop
> (Web) content code we've been generally avoiding using it… I suggest that
> with postpone making a decision about this later on, with its own bug filed,
> in order to keep things consistent for now.

I know that coming from the desktop team makes me very sensitive to these kind of things, because I _know_ how hard it is to keep the code-base consistent wrt formatting and style. Modern desktop code (i.e. everything in /browser/) uses the convention I use above and since this part of Loop is desktop, we shall use that style. In other words: I do not intend to change this. Apology for the, perhaps, strong words, but I think it's relatively important.

> 
> Also, as this is supposedly desktop only code, couldn't we use a const here?
> could be appropriately named CONTACTS_CHUNK_SIZE.

This is dependent on the Javascript version we explicitly use. At this point we can't use Harmony features, because it's not a default in content scripts.

> 
> @@ +33,5 @@
> > +      this.props.handleContactClick(this.props.key);
> > +    },
> > +
> > +    render: function() {
> > +      var username = this.props.contact.name[0].split(" ");
> 
> Couldn't we just get some Contact model object passed in, so we could simply
> this.props.contact.getUsername()? Could be addressed in a follow-up patch
> though.

I'd prefer that, because I don't intend to add an API layer between the spec format and consumer code.


> 
> @@ +37,5 @@
> > +      var username = this.props.contact.name[0].split(" ");
> > +      var contactCSSClass = React.addons.classSet({
> > +        contact: true,
> > +        selected: this.props.selected,
> > +        blocked: this.state.blocked
> 
> I'm wondering why `selected` is handled with a prop, while `blocked` is kept
> in state. As I can't find any place where state.blocked is updated, it could
> probably be a prop.

Blocked is a model state, which will be mutable in a follow-up bug - bug 1017052 - and `selected` is a UI property controlled by the parent list, not persisted by a model.

> var email = this.props.contact.getPreferredEmailAddress() (or even
> contact.email.prefered) would be nice.
> 
> While updating the contacts API is definitely not in the scope of this bug,
> in the meanwhile I think we'll want to extract this kind of computation in
> private methods attached to the component prototype.

Good idea I'll change that :)

> Let's extract the gravatar URL computation to a method, so we can write <div
> className="avatar" data-avatar={this._getGravatarUrl()}/>
> 
> Also please note you can safely use self-closing tags in JSX.

Will do!

> username.shift() seems a bit odd, I'd rather see contactObj.displayName or
> similar. And joining back the parts inline in JSX should be avoided imho.

Hmm, yeah. This is code smell to me to, but this is one part where the model spec and the UI spec are incompatible. I'll factor this out to a member function on the prototype, but other than that there's not much more I can do.

> @@ +103,5 @@
> > +
> > +        addContactsInChunks(contacts);
> > +
> > +        // Listen for contact changes/ updates.
> > +        contactsAPI.on("add", function(e, contact) {
> 
> We need to handle errors here. We could create an higher order function for
> that:
> 
> function errorHandler(fn) {
>   return function(err, contact) {
>     if (err) {
>       // do smthg useful with the error, eg. updating the UI through state
> change
>       return;
>     }
>     fn(contact);
>   };
> }
> 
> Usage:
> 
> contactsAPI.on("add", errorHandler(list.handleContactAdd));

I'm afraid that's not correct: the Contacts API does _not_ emit errors. The EventEmitter inherited from the devtools passes the event/ eventName as the first param. All other arguments are passed as second and n+1th params.

The mutation methods of the Contacts API _do_ pass along errors. If an error occurs, there will be no event emitted as the mutation was cancelled by it.

    
    

    
  


  
New patch with comments addressed and working gravatar images - Using attr() for properties other than `content` will be supported after bug 435426 is fixed.

        Attachment #8490083 -
        Attachment is obsolete: true

        Attachment #8490693 -
        Flags: review?(nperriault)

    
    

    
  


  
Missed some debug code :/

        Attachment #8490693 -
        Attachment is obsolete: true

        Attachment #8490693 -
        Flags: review?(nperriault)

        Attachment #8490694 -
        Flags: review?(nperriault)

    
    

    
  
(In reply to Mike de Boer [:mikedeboer] from comment #17)
> since this part of Loop is
> desktop, we shall use that style. In other words: I do not intend to change
> this.

So please use that style completely, like naming args with a lowercase "a" and so on… The key is consistency, always. Also, please ensure filing a bug about moving all the current Loop content codebase to use the Gecko js coding conventions, and let's have a constru^W^W interesting conversation there.

> This is dependent on the Javascript version we explicitly use. At this point
> we can't use Harmony features, because it's not a default in content scripts.

`const` works in regular Web content for me. Is the limitation you mention scoped to sandboxed content specifically? Possibly Loop only? In such case that sounds like a bug to me, and something we might to consider adding support for.

> > Couldn't we just get some Contact model object passed in, so we could simply
> > this.props.contact.getUsername()? Could be addressed in a follow-up patch
> > though.
> 
> I'd prefer that, because I don't intend to add an API layer between the spec
> format and consumer code.

Well, maybe we could also discuss the API spec a little, is it upposed to be frozen forever?

> > username.shift() seems a bit odd, I'd rather see contactObj.displayName or
> > similar. And joining back the parts inline in JSX should be avoided imho.
> 
> Hmm, yeah. This is code smell to me to, but this is one part where the model
> spec and the UI spec are incompatible. 

Are you suggesting that an "API layer between the spec format and consumer code" would be convenient here?

> The EventEmitter inherited from the devtools passes the event/ eventName as the
> first param.

Ah. maybe renaming the `e` arg to somthing like `event` — sorry, `aEvent` would be appropriate, then.

    
    

    
  
(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #20)
> So please use that style completely, like naming args with a lowercase "a"
> and so on… The key is consistency, always. Also, please ensure filing a bug
> about moving all the current Loop content codebase to use the Gecko js
> coding conventions, and let's have a constru^W^W interesting conversation
> there.

I don't think it's worth booting up a discussion at this point. Let's dedicate the precious time we have to actual work.
If you want me to use ALL_CAPS style, I'll do that. Whatever gets me an r+, TBH.

> `const` works in regular Web content for me. Is the limitation you mention
> scoped to sandboxed content specifically? Possibly Loop only? In such case
> that sounds like a bug to me, and something we might to consider adding
> support for.

Alright, I'll investigate which newer JS features we can already use.

> Well, maybe we could also discuss the API spec a little, is it upposed to be
> frozen forever?

Frozen might be a tad strong wording, but it's supposed to follow the MozContact API as is in use by FxOS.

> Are you suggesting that an "API layer between the spec format and consumer
> code" would be convenient here?

No, I'm saying that right now I'd like to implement helper methods for dealing with the spec format in contacts.jsx, as they're mostly about formatting the data in a way that it can be presented in the view.

> Ah. maybe renaming the `e` arg to somthing like `event` — sorry, `aEvent`
> would be appropriate, then.

That's what I did in the patch that's pending review atm.

    
    

    
  


  
Now using JS 1.8+

        Attachment #8490694 -
        Attachment is obsolete: true

        Attachment #8490694 -
        Flags: review?(nperriault)

        Attachment #8490767 -
        Flags: review?(nperriault)

    
    

    
  


  
small indentation fix.

        Attachment #8490767 -
        Attachment is obsolete: true

        Attachment #8490767 -
        Flags: review?(nperriault)

        Attachment #8490771 -
        Flags: review?(nperriault)

    
    

    
  
Comment on attachment 8490771 [details] [diff] [review]
Patch v1.2: Add a contacts list to the contacts tab

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

The code looks good, but I didn't find an easy way to test a build manually with contacts actually loaded; hints appreciated!

Also, the patch still needs tests. I suggest we pair on writing them, so we can land this baby asap :)

Side note: I'm surprised with the different SVG assets for different sizes… As per IRC discussion, we need to hear the rationale behind this.

Last, the platform part of the code (LoopContacts.jsm) should be reviewed by a platform reviewer; flagging :Standard8 here.

::: browser/components/loop/LoopContacts.jsm
@@ +139,5 @@
> +            stringStream.data = value.trim().toLowerCase();
> +            hasher.updateFromStream(stringStream, -1);
> +            return hasher.finish(true).replace("==", "");
> +          }
> +        }

It's probably worth having a platform reviewer reviwing these parts of the patch.

::: browser/components/loop/content/js/panel.jsx
@@ +534,5 @@
> +    /**
> +     * mozLoop Contacts API object
> +     * @type {Object}
> +     */
> +    contactsAPI: null,

Careful, the panel router is being removed in bug 1068580, which is about to land.

        Attachment #8490771 -
        Flags: review?(nperriault) → review?(standard8)

    
    

    
  
You can load a list of temporary contacts by replacing the code in `ContactsList#componentDidMount` with the contents of the pastebin: https://pastebin.mozilla.org/6529624

Have fun!

    
    

    
  


  
Unbitrotted patch.

        Attachment #8490771 -
        Attachment is obsolete: true

        Attachment #8490771 -
        Flags: review?(standard8)

        Attachment #8491365 -
        Flags: review?(standard8)

    
    

    
  


  
Unbitrotting, again.

        Attachment #8491365 -
        Attachment is obsolete: true

        Attachment #8491365 -
        Flags: review?(standard8)

        Attachment #8491381 -
        Flags: review?(standard8)

    
    

    
  
Comment on attachment 8491381 [details] [diff] [review]
Patch v1.2: Add a contacts list to the contacts tab

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

r+ just for the MozLoopContacts.jsm parts. Though it would be good to get the tests extended to ensure the md5value field is working as expected.

        Attachment #8491381 -
        Flags: review?(standard8) → review+

    
    

    
  
+    getGravatarUrl: function(email, size = 40) {
+      return "http://www.gravatar.com/avatar/" + email.md5value + ".jpg?s=" + size;
+    },

Needinfo on Maire, because we need to make sure the fact we're using Gravatar gets into the privacy review, and I've forgotten who's doing that.
Flags: needinfo?(mreavy)

    
    

    
  
(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #24)
> The code looks good, but I didn't find an easy way to test a build manually
> with contacts actually loaded; hints appreciated!

For a simpler version that allows you to test live mutations and multiple additions, run this or a variant in a Browser privileged scratchpad with the "list" array populated:

Cu.import("resource:///modules/loop/LoopContacts.jsm");
Task.spawn(function* () {
  yield new Promise((resolve, reject) => {
    LoopContacts.removeAll(err => err ? reject(err) : resolve());
  });
  for (let contact of list) {
    yield new Promise((resolve, reject) => {
      LoopContacts.add(contact, err => err ? reject(err) : resolve());
    });
  }
});

Complete code at <https://pastebin.mozilla.org/6537401>.

    
    

    
  
(In reply to Mark Banner (:standard8) from comment #29)
> r+ just for the MozLoopContacts.jsm parts. Though it would be good to get
> the tests extended to ensure the md5value field is working as expected.

I'll address that in a follow-up patch that contains all the tests.

    
    

    
  
One thing I noticed is that "contact.id" is externally provided. This matches what is specified in the design document:

https://wiki.mozilla.org/Loop/Architecture/Address_Book

According to the document, the ID should be unique in the local store, but our code does not include a check for this, as the "key" parameter to the IndexedDB "add" is not provided:

http://mxr.mozilla.org/mozilla-central/source/browser/components/loop/LoopContacts.jsm#379

In fact, if you run the code in comment 31 twice (without the removal part), the front-end code will become confused by the presence of duplicates in the database.

As a start, we should probably generate a string-format GUID anew for each contact at the time of addition. One thing to define is whether the GUID should be lowercase or uppercase, as those will be treated differently.

    
    

    
  
I tested this with more than five contacts and the layout is broken.

    
    

    
  
(In reply to Mark Banner (:standard8) from comment #30)
> +    getGravatarUrl: function(email, size = 40) {
> +      return "http://www.gravatar.com/avatar/" + email.md5value + ".jpg?s="
> + size;
> +    },
> 
> Needinfo on Maire, because we need to make sure the fact we're using
> Gravatar gets into the privacy review, and I've forgotten who's doing that.

Mika is doing our privacy review.  I've added her to the cc of this bug.
Flags: needinfo?(mreavy)

    
    

    
  


  
Now using the truly unique _guid property.

Carrying over r=Standard8

        Attachment #8491381 -
        Attachment is obsolete: true

        Attachment #8491464 -
        Flags: review?(nperriault)

    
    

    
  


  
Now with 'removeAll' event handler and list overflow fix.

        Attachment #8491464 -
        Attachment is obsolete: true

        Attachment #8491464 -
        Flags: review?(nperriault)

        Attachment #8491525 -
        Flags: review?(nperriault)

    
    

    
  


  
Another patch, now with panel unit test additions to make sure that we can land this patch separately from the unit tests that are currently in development.

        Attachment #8491525 -
        Attachment is obsolete: true

        Attachment #8491525 -
        Flags: review?(nperriault)

        Attachment #8491531 -
        Flags: review?(nperriault)

    
    

    
  
Comment on attachment 8491531 [details] [diff] [review]
Patch v1.5: Add a contacts list to the contacts tab

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

::: browser/locales/en-US/chrome/browser/loop/loop.properties
@@ +85,5 @@
>  ## and click the 'New Contact' button to see the fields.
>  new_contact_name_placeholder=Name
>  new_contact_email_placeholder=Email
> +
> +contacts_blocked_contacts=Blocked Contacts

Hm, since we are string frozen on Aurora, should we land this new string and the separator as a separate bug or patch, to facilitate a possible uplift?

    
    

    
  


  
Now with string changes factored out to make uplift easier, default profile picture to match design spec (if gravatar doesn't have an image for that user), proper MD5 hex values for hashed email addresses.

        Attachment #8491531 -
        Attachment is obsolete: true

        Attachment #8491531 -
        Flags: review?(nperriault)

    
  

        Attachment #8491563 -
        Flags: review?(paolo.mozmail)

    
    

    
  
Comment on attachment 8491531 [details] [diff] [review]
Patch v1.5: Add a contacts list to the contacts tab

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

Unfortunately I have to stop midway now. I see you posted a newer version anyways, so I'll take a look at that, or even a newer one when ready.

::: browser/components/loop/LoopContacts.jsm
@@ +272,5 @@
> +  if (computeProperties) {
> +    for (let propName of Object.getOwnPropertyNames(computeProperties)) {
> +      let propDef = computeProperties[propName];
> +      let params = propDef.takes.map(param => obj[param] || null);
> +      obj[propName] = propDef.compute(...params);

I believe this generic mechanism is overkill for what we need, and maybe even unrelated to the best eventual solution, that could be a lazy getter called only when items scroll into view, which would perform much better with lots of contacts.

Since we only have to compute this for one of the e-mails of the contact (the main one),  for caching reasons we can keep it into a WeakMap in the front-end, associated to the contact object. We are notified when a contact changes, so we can invalidate the WeakMap item at that point.

Anyways, in order not to be blocked on the privacy review mentioned in comment 35, I suggest separating the Gravatar portions (including this code) into a separate bug, and just have items without the icon for the moment.

::: browser/components/loop/content/js/contacts.jsx
@@ +31,5 @@
> +      this.props.handleContactClick(this.props.key);
> +    },
> +
> +    getContactNames: function() {
> +      let names = this.props.contact.name[0].split(" ");

This assumes that a name is always present, but the model does not guarantee it.

@@ +34,5 @@
> +    getContactNames: function() {
> +      let names = this.props.contact.name[0].split(" ");
> +      return {
> +        firstName: names.shift(),
> +        lastName: names.join(" ")

firstName and lastName are not necessarily what is returned here... not only because of multi-word names, but also because, in some languages, these would be exactly reversed. This can work as an approximation for now, but a code comment is welcome.

Also, splitting on " " may not work if the spacing is non-ASCII, but again this approximation works for now.

@@ +39,5 @@
> +      };
> +    },
> +
> +    getPreferredEmail: function() {
> +      let email = this.props.contact.email[0];

Same here. An empty email results in "undefined" instead of an empty string being returned.

@@ +89,5 @@
> +  const ContactsList = React.createClass({
> +    propTypes: {
> +      contactsAPI: React.PropTypes.object.isRequired,
> +      contacts: React.PropTypes.object,
> +      faded: React.PropTypes.bool

Is "faded" used?

@@ +96,5 @@
> +    getInitialState: function() {
> +      return {
> +        selected: -1,
> +        faded: !!this.props.faded,
> +        contacts: this.props.contacts || {}

Mirroring props into state is generally something frowned upon. Unfortunately I don't have time for a deeper investigation now, but will do on the next pass, while you address other comments.

::: browser/components/loop/content/js/panel.js
@@ +530,5 @@
>  
>      React.renderComponent(PanelView({
>        client: client, 
> +      notifications: notifications, 
> +      contactsAPI: navigator.mozLoop.contacts}), document.querySelector("#main"));

I know NiKo mentioned you could do dependency injection like this, but since this is not actually used in the tests you just posted, and he also mentioned the other version would have been fine (given some possible future reworking), I suggest you revert to the original solution where you did not pass the API as a prop. This is also consistent with how we mock mozLoop in general, for the moment.

        Attachment #8491531 -
        Attachment is obsolete: false

    
  

        Attachment #8491531 -
        Attachment is obsolete: true

    
    

    
  
(In reply to :Paolo Amadini from comment #43)
> I believe this generic mechanism is overkill for what we need, and maybe
> even unrelated to the best eventual solution, that could be a lazy getter
> called only when items scroll into view, which would perform much better
> with lots of contacts.

I don't really see why this is overkill and why calculating the md5 hash when a contact scrolls into view is cheaper than generating it on save and caching it.
The way this works now is that the hash is calculated when `validateContact()` is called, which is only when a contact is added or updated (i.e. persisted in the data-store). This is as lazy as I can think of doing it.

> Since we only have to compute this for one of the e-mails of the contact
> (the main one),  for caching reasons we can keep it into a WeakMap in the
> front-end, associated to the contact object. We are notified when a contact
> changes, so we can invalidate the WeakMap item at that point.
> 
> Anyways, in order not to be blocked on the privacy review mentioned in
> comment 35, I suggest separating the Gravatar portions (including this code)
> into a separate bug, and just have items without the icon for the moment.

At this point I'm not sure what implications are of the privacy review. I think it's not good to block this bug on it; what we need to do is set up a privacy review and remove the feature _if_ it appears to be a concern.

> This assumes that a name is always present, but the model does not guarantee
> it.

The model does not guarantee anything to be present, it's weak that way. We can't program by contract and have to approach problems pragmatically.
Regardless, I'd like to turn the problem around; any data that the UI spec requires to be present in the model should be there. This means adding a validation step for required fields.

> 
> @@ +34,5 @@
> > +    getContactNames: function() {
> > +      let names = this.props.contact.name[0].split(" ");
> > +      return {
> > +        firstName: names.shift(),
> > +        lastName: names.join(" ")
> 
> firstName and lastName are not necessarily what is returned here... not only
> because of multi-word names, but also because, in some languages, these
> would be exactly reversed. This can work as an approximation for now, but a
> code comment is welcome.
> 
> Also, splitting on " " may not work if the spacing is non-ASCII, but again
> this approximation works for now.
> 
> @@ +39,5 @@
> > +      };
> > +    },
> > +
> > +    getPreferredEmail: function() {
> > +      let email = this.props.contact.email[0];
> 
> Same here. An empty email results in "undefined" instead of an empty string
> being returned.
> 
> @@ +89,5 @@
> > +  const ContactsList = React.createClass({
> > +    propTypes: {
> > +      contactsAPI: React.PropTypes.object.isRequired,
> > +      contacts: React.PropTypes.object,
> > +      faded: React.PropTypes.bool
> 
> Is "faded" used?
> 
> @@ +96,5 @@
> > +    getInitialState: function() {
> > +      return {
> > +        selected: -1,
> > +        faded: !!this.props.faded,
> > +        contacts: this.props.contacts || {}
> 
> Mirroring props into state is generally something frowned upon.
> Unfortunately I don't have time for a deeper investigation now, but will do
> on the next pass, while you address other comments.
> 
> ::: browser/components/loop/content/js/panel.js
> @@ +530,5 @@
> >  
> >      React.renderComponent(PanelView({
> >        client: client, 
> > +      notifications: notifications, 
> > +      contactsAPI: navigator.mozLoop.contacts}), document.querySelector("#main"));
> 
> I know NiKo mentioned you could do dependency injection like this, but since
> this is not actually used in the tests you just posted, and he also
> mentioned the other version would have been fine (given some possible future
> reworking), I suggest you revert to the original solution where you did not
> pass the API as a prop. This is also consistent with how we mock mozLoop in
> general, for the moment.

    
    

    
  
Crap, hit 'Save Changes' too soon :/ Sorry.

    
  
Blocks: 1069816

    
    

    
  
(In reply to Mike de Boer [:mikedeboer] from comment #44)
> (In reply to :Paolo Amadini from comment #43)
> > I believe this generic mechanism is overkill for what we need, and maybe
> > even unrelated to the best eventual solution, that could be a lazy getter
> > called only when items scroll into view, which would perform much better
> > with lots of contacts.
> 
> I don't really see why this is overkill

I'm sorry for the confusion, my comment was addressing three different and unrelated issues, I guess that just running out of time and submitting the review without re-reading it wasn't the best thing to do! :-( Will try to dedicate to it all the time it needs today.

The first part of the sentence is only related to a pure code complexity consideration. The current code is implemented as a generic mechanism for all fields, but according to my expectations at least, it is unlikely that it will be used for other fields. This, however, is a minor issue and not my primary concern.

> and why calculating the md5 hash
> when a contact scrolls into view is cheaper than generating it on save and
> caching it.

And also the performance note is a secondary concern, and I put it there mostly in support of the theory that a generic mechanism may, in some cases, need to be changed back to a non-generic one, in this case if performance characteristics of different fields change.

The real, underlying issue I'm concerned about was not framed explicitly. Apologies!



I believe the "md5value" field should not be saved in the data store at all.



To address this review, I read the documentation of the data store in more detail. One point was of particular interest to me:

https://wiki.mozilla.org/Loop/Architecture/Address_Book#Format
"To allow for future integration with the Contacts API and/or potential integration with contact synchronization across devices (including Firefox OS devices), we will use objects with properties having the same structure as those used by mozContact."

And the mozContact API is described here:
https://developer.mozilla.org/en-US/docs/Web/API/mozContact

If we add a new property to the structure, not documented anywhere, it may pose problems in the future. For example, the property may be lost in synchronization, leading to failures in our code, or the need to add special code to recompute on demand, or misalignment if the e-mail is changed by another client, or other unforeseen consequences.

My point is that, if we diverge from the design, it should be a conscious decision, and not just a performance optimization.

I missed the fact that Mark Banner had already reviewed that part of the code, otherwise I may just have skipped over all this research. However, since I see no explicit acknowledgement in Mark's comments, nor any update to the design wiki, now I'd like to hear confirmation that this is indeed the expected outcome.

I'm of course fine if Mark just wants to go for this differentiation/expansion route and document it in the wiki, or if he wants further discussion with other team members on the design.
Flags: needinfo?(standard8)

    
    

    
  
To follow up, I just talked to Mike and got feedback from Mark over IRC, we're keeping the model as originally designed and moving the Gravatar implementation to a separate bug, probably with an API to request URLs on demand from e-mail addresses. We also reviewed more of the patch in real time, an updated version is in the works.
Flags: needinfo?(standard8)

    
  
Depends on: 1069918

    
    

    
  


  

        Attachment #8491563 -
        Attachment is obsolete: true

        Attachment #8491563 -
        Flags: review?(paolo.mozmail)

        Attachment #8492151 -
        Flags: review?(paolo.mozmail)

    
    

    
  


  
Last minute change.

        Attachment #8492151 -
        Attachment is obsolete: true

        Attachment #8492151 -
        Flags: review?(paolo.mozmail)

        Attachment #8492166 -
        Flags: review?(paolo.mozmail)

    
    

    
  


  

        Attachment #8491564 -
        Attachment is obsolete: true

        Attachment #8491567 -
        Attachment is obsolete: true

        Attachment #8491564 -
        Flags: review?(paolo.mozmail)

        Attachment #8492167 -
        Flags: review?(paolo.mozmail)

    
    

    
  
Comment on attachment 8492166 [details] [diff] [review]
Patch v1.7: Add a contacts list to the contacts tab

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

Thank you for the simplification work here, the patch looks crisp clear now!

::: browser/components/loop/content/js/contacts.jsx
@@ +152,5 @@
> +      };
> +
> +      let shownContacts = _.groupBy(this.state.contacts, function(contact) {
> +        return contact.blocked ? "blocked" : "available";
> +      });

Please add a very simple sorting on the resulting arrays (either name and GUID, or just GUID).

        Attachment #8492166 -
        Flags: review?(paolo.mozmail) → review+

    
    

    
  
Comment on attachment 8491564 [details] [diff] [review]
Patch 2: add a visual separator between available and blocked contacts

Did you mark this attachment as obsolete intentionally? Once updated to match the other patches, it looks good to me.

        Attachment #8491564 -
        Flags: review+

    
    

    
  
Comment on attachment 8492167 [details] [diff] [review]
Patch 2.1: add support for Gravatar avatars

Code looks good, assuming it has been tested and it looks correctly with a real avatar, both when present and when missing. Also, I don't know if avatars can have transparency, but if so that should be tested too.

If the network request fails, we should ensure we don't show a broken image link.

Actually, this attachment should be moved to a different bug for tracking purposes, and the string to a different one as well.

        Attachment #8492167 -
        Flags: review?(paolo.mozmail) → review+

    
    

    
  


  
Paolo, I'd like your stamp of approval wrt sorting again. Thanks!

        Attachment #8492166 -
        Attachment is obsolete: true

        Attachment #8492186 -
        Flags: review?(paolo.mozmail)

    
    

    
  
Comment on attachment 8492186 [details] [diff] [review]
Patch v1.8: Add a contacts list to the contacts tab

// If names are equal, compare against unique ids make sure we have

"to make sure"

        Attachment #8492186 -
        Flags: review?(paolo.mozmail) → review+

    
    

    
  
Comment on attachment 8492189 [details] [diff] [review]
Patch 3.1: add a visual separator between available and blocked contacts

I'm probably not the best reviewer for "common.css", but the other changes look straightforward.

        Attachment #8492189 -
        Flags: review?(paolo.mozmail) → feedback+

    
  
Blocks: 1069962

    
  
Blocks: 1069965

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/4b348553c600
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED

    
    

    
  
Paul, please verify this in the latest Nightly and the Try-server builds here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/rjesup@wgate.com-f9eb2cbac352

We'll also want to make sure we have a Moztrap test for 34/35.
Flags: needinfo?(paul.silaghi)
Flags: in-moztrap?
QA Contact: anthony.s.hughes → paul.silaghi
Whiteboard: [contacts][first release needed] [loop-uplift] → [contacts][first release needed] [loop-uplift][fig:verifyme]

    
    

    
  
You will be able to add contacts and test this when bug 1038257 is resolved.

    
  
Blocks: 1075544

    
    

    
  
(In reply to :Paolo Amadini from comment #61)
> You will be able to add contacts and test this when bug 1038257 is resolved.

Paul, the dependency is now fixed. Can you please test this on Nightly? Unless we get a new Fig build with bug 1038257 we won't be able to pre-verify it for uplift.
Whiteboard: [contacts][first release needed] [loop-uplift][fig:verifyme] → [contacts][first release needed] [loop-uplift][fig:wontverify]

    
    

    
  
Verified fixed FF 35.0a1 (2014-10-03) Win 7, OS X 10.9.5, Ubuntu 13.04
Status: RESOLVED → VERIFIED

          status-firefox35:
          --- → verified
Flags: needinfo?(paul.silaghi)

    
    

    
  
UI issue for contacts with long names: http://i.imgur.com/9qrBn1j.png
Say if I should file.

    
    

    
  
Hi Paul, please file that issue. Thanks!
Depends on: 1077513

    
    

    
  
Comment on attachment 8492186 [details] [diff] [review]
Patch v1.8: Add a contacts list to the contacts tab

Approval Request Comment
Uplift request for patches staged and tested on Fig

        Attachment #8492186 -
        Flags: approval-mozilla-aurora?

    
    

    
  
Comment on attachment 8492186 [details] [diff] [review]
Patch v1.8: Add a contacts list to the contacts tab

I worked with Randell and Maire on uplifting a large number of Loop bugs at once. All of the bugs have been staged on Fig and tested by QE before uplift to Aurora. As well, all of the bugs are isolated to the Loop client. Randell handled the uplift with my approval. I am adding approval to the bug after the fact for bookkeeping.

        Attachment #8492186 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

    
    

    
  
I confirm that I'm able to view contacts with Aurora builds.

          status-firefox34:
          fixed → verified
Flags: in-moztrap?

    
  
Flags: in-moztrap+

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: