Comment on attachment 9057706 [details] [diff] [review] Bug-1542717_map-list.patch Review of attachment 9057706 [details] [diff] [review]: ----------------------------------------------------------------- Please here too expand on the commit comment (like the bug summary already does) ::: mailnews/addrbook/content/map-list.js @@ +8,5 @@ > +{ > + const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm"); > + > + /** > + * The MozMapList widget behaves as menu popups for available maps options as a popup menu showing available map options for an address. @@ +17,5 @@ > + class MozMapList extends MozElements.MozMenuPopup { > + connectedCallback() { > + if (this.delayConnectedCallback() || this.hasConnected) { > + return; > + } maybe set the "is" attribute here @@ +37,5 @@ > + } > + > + /** > + * Initializes the necessary address data from an addressbook card. > + * @param card A nsIAbCard to get the address data from. @param {nsIAbCard} card - the card to get the addess data from @@ +59,5 @@ > + > + /** > + * Initializes the necessary address data from passed in values. > + */ > + initMapAddress(addr1, addr2, city, state, zip, country) { seems silly to use this method to set attributes for internal usage. just store internallly this.address1 = this.address2 = this.city = ... and so on @@ +85,5 @@ > + > + /** > + * Returns the Map service URL from localized pref. Returns null if there > + * is none at the given index. > + * @param index The index of the service to return. 0 is the default service. @param integer [index=0] - the .... @@ +87,5 @@ > + * Returns the Map service URL from localized pref. Returns null if there > + * is none at the given index. > + * @param index The index of the service to return. 0 is the default service. > + */ > + _getMapURLPref(index) { getMapURLPref(index = 0) { @@ +124,5 @@ > + let item = document.createElement("menuitem"); > + item.setAttribute("url", url); > + item.setAttribute("label", name); > + item.setAttribute("type", "radio"); > + item.setAttribute("name", "mapit_service"); seems unused, remove. @@ +189,5 @@ > + Ci.nsIPrefLocalizedString, defaultUrl); > + } > + > + /** > + * Generate map URL in the href attribute. doesn't generate anything in any href. please fix the doc. also add a @return

Comment on attachment 9057821 [details] [diff] [review] Bug-1542717_map-list.patch Review of attachment 9057821 [details] [diff] [review]: ----------------------------------------------------------------- Looks ok, r=mkmelin with the below fixed ::: mailnews/addrbook/content/map-list.js @@ +39,5 @@ > + > + /** > + * Initializes the necessary address data from an addressbook card. > + * @param {nsIAbCard} card - the card to get the addess data from > + * @param addrPrefix A prefix of the card properties to use. Use "Home" or "Work". for this too update to the same format @param {string} addPrefix - card property prefix: "Home" or "Work", to make the map use either HomeAddress or WorkAddress @@ +185,5 @@ > + let address2 = this.map_address2; > + let city = this.map_city; > + let state = this.map_state; > + let zip = this.map_zip; > + let country = this.map_country; no need to use these temporaries. you can use the this values. but please name them without the map_