1542717 - [de-xbl] convert the map-list binding to <menupopup is="map-list">

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Magnus Melin [:mkmelin]]

https://searchfox.org/comm-central/rev/304e74d67edad6b5a27bb62338a36805243d3851/mailnews/addrbook/content/addrbookWidgets.xml#299

The map-list binding can be converted to <menupopup is="map-list">

See bug 1531296 for examples.

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: Bug-1542717_map-list.patch

Comment 2

[Magnus Melin [:mkmelin]]

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 3

[Khushil Mistry [:khushil324]]

Attachment: Bug-1542717_map-list.patch

Comment 4

[Khushil Mistry [:khushil324]]

(In reply to Magnus Melin [:mkmelin] from comment #2)

seems unused, remove.

It is used to append menuitems with the required attributes. We are not inserting menuitems anywhere else.

Comment 5

[Magnus Melin [:mkmelin]]

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_

Comment 6

[Khushil Mistry [:khushil324]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c04f187d51edfb94bff28476ed9f000c56a54264

Comment 7

[Khushil Mistry [:khushil324]]

Attachment: Bug-1542717_map-list.patch

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/bddc7eeb6720
[de-xbl] convert map-list binding to <menupopup is='map-list'>. r=mkmelin