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

RESOLVED FIXED in Thunderbird 68.0

Status

task
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: mkmelin, Assigned: khushil324)

Tracking

(Blocks 1 bug)

Trunk
Thunderbird 68.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Comment 1

2 months ago
Posted patch Bug-1542717_map-list.patch (obsolete) — Splinter Review
Attachment #9057706 - Flags: review?(mkmelin+mozilla)
Reporter

Comment 2

2 months ago
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
Attachment #9057706 - Flags: review?(mkmelin+mozilla)
Assignee

Comment 3

2 months ago
Posted patch Bug-1542717_map-list.patch (obsolete) — Splinter Review
Attachment #9057706 - Attachment is obsolete: true
Attachment #9057821 - Flags: review?(mkmelin+mozilla)
Assignee

Comment 4

2 months ago

(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.

Reporter

Comment 5

2 months ago
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_
Attachment #9057821 - Flags: review?(mkmelin+mozilla) → review+
Assignee

Comment 7

2 months ago
Attachment #9057821 - Attachment is obsolete: true
Attachment #9057869 - Flags: review+
Assignee

Updated

a month ago
Keywords: checkin-needed

Comment 8

a month ago

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

Status: NEW → RESOLVED
Last Resolved: a month ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

a month ago
Target Milestone: --- → Thunderbird 68.0
You need to log in before you can comment on or make changes to this bug.