Magnus Melin [:mkmelin] Reporter
	Comment 2 • 6 years ago

Comment on attachment 9048722 [details] [diff] [review]
de-xbl-mail-messageids-headerfield.patch

This patch is empty

	Magnus Melin [:mkmelin] Reporter
	Comment 10 • 6 years ago

Comment on attachment 9048949 [details] [diff] [review]
de-xbl-mail-messageids-headerfield.patch

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

::: mail/base/content/mailWidgets.js
@@ +171,4 @@
>    }
>  }
>  
> +class MozMailMessageidsHeaderfiled extends MozXULElement {

typo: it's field, not filed

Please as you go add jsdoc type documentation about elements. See an example from MozMenulistEditable

@@ +190,5 @@
> +    });
> +    this.iconNode.appendChild(this.toggleIcon);
> +    this.appendChild(this.iconNode);
> +
> +    this.headerNode = document.createElement("hbox");

XBL didn't use to be able to be it's own node, but now that this becomes a custom element you're likely able to remove this extra hbox internal element and let the CE itself be the element. Like MozMailNewsgroupsHeaderfield. (Some css then likely needd to be adjusted accordingly.)

@@ +205,5 @@
> +  toggleWrap() {
> +    const headerValue = this.headerValue,
> +      messageIdNodes = headerValue.childNodes,
> +      showFullMessageIds = !this.showFullMessageIds,
> +      messageIds = this.mMessageIds;

generally declare all variables separately, and preferably when first used (where that's possible)

doesn't look like there is any benefit of these intermediary variable in this case

@@ +228,5 @@
> +    const headerValue = this.headerValue,
> +      messageIdNodes = headerValue.childNodes,
> +      numMessageIds = this.mMessageIds.length;
> +
> +    while (messageIdNodes.length > numMessageIds * 2 - 1)

add braces for all loops

@@ +233,5 @@
> +      headerValue.lastChild.remove();
> +
> +    this.toggleIcon.hidden = numMessageIds <= 1;
> +
> +    for (let index = 0; index < numMessageIds; index++) {

while we're here, make it i instead of index

@@ +253,5 @@
> +      }
> +    }
> +  }
> +
> +  _updateMessageIdNode(aMessageIdNode, aIndex, aMessageId, aLastId) {

trying to get away from the a (meaning arg) convention, so at least for small usages, remove that while converting

@@ +565,4 @@
>  customElements.define("mail-newsgroup", MozMailNewsgroup);
>  customElements.define("mail-newsgroups-headerfield", MozMailNewsgroupsHeaderfield);
>  customElements.define("mail-messageid", MozMailMessageid);
> +customElements.define("mail-messageids-headerfield", MozMailMessageidsHeaderfiled);

you can put this right after the MozMailMessageidsHeaderfiled class body. planning to move them all after their classes shortly

	Magnus Melin [:mkmelin] Reporter
	Comment 12 • 6 years ago

Comment on attachment 9049305 [details] [diff] [review]
de-xbl-mail-messageids-headerfield.patch

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

Related css for this should live in messageHeader.css 
There is (as for much of our css) a shared messageHeader.css which contains platform independent styling. That get's imported by the per-platform specific messageHeader.css where you'd put anything that is differnt between platforms

https://searchfox.org/comm-central/search?q=messageheader.css&case=false&regexp=false&path=mail%2Ftheme

	Magnus Melin [:mkmelin] Reporter
	Comment 16 • 6 years ago

Comment on attachment 9049359 [details] [diff] [review]
de-xbl-mail-messageids-headerfield.patch

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

Looking pretty good

Tests still need to be adjusted. One is test-message-toolbar.js but there can be others too.

From the obj-dir, run
make mozmill-one SOLO_TEST=message-header/test-message-header.js

You'll see 

SUMMARY-PASS | test-message-header.js::test_clicking_star_opens_inline_contact_editor
SUMMARY-UNEXPECTED-FAIL | test-message-header.js | test-message-header.js::test_msg_id_context_menu
  EXCEPTION: anonNodes is null
    at: test-window-helpers.js line 915
       _get_anon_element_by_id_and_query test-window-helpers.js:915 29
       _get_anon_elementid test-window-helpers.js:935 33
       test_msg_id_context_menu test-message-header.js:437 20
       Runner.prototype.wrapper frame.jsm:556 7
       Runner.prototype._runTestModule frame.jsm:626 14
       Runner.prototype.runTestModule frame.jsm:665 8
       Runner.prototype.runTestFile frame.jsm:525 8
       runTestFile frame.jsm:677 10
       Bridge.prototype._execFunction server.js:190 15
       Bridge.prototype.execFunction server.js:195 21
       Session.prototype.receive server.js:313 6
       AsyncRead.prototype.onDataAvailable server.js:78 16

... failure to find things the same way here: https://searchfox.org/comm-central/rev/8a64171478a67b80775e23c37992cdf1dbd99e54/mail/test/mozmill/message-header/test-message-header.js#437

mc.aid means anonymous id (anonid, which is what XBL use for internal content)

You should be able to just change it to mc.eid to make it work

::: mail/base/content/mailWidgets.js
@@ +178,5 @@
> + * mailnews.headers.showReferences pref to true in the mailnews/mailnews.js file.
> + * @extends {MozXULElement}
> + */
> +class MozMailMessageidsHeaderfield extends MozXULElement {
> +  connectedCallback() {

probably good to call delayConnectedCallback() here 


note that connectedCallback() can be called more than once, so you need a

if (this.hasChildNodes()) {
 return;
}

@@ +242,5 @@
> +    }
> +  }
> +
> +  _updateMessageIdNode(messageIdNode, index, messageId, lastId) {
> +    const showFullMessageIds = this.showFullMessageIds;

you don't need this temp variable

	Magnus Melin [:mkmelin] Reporter
	Comment 17 • 6 years ago

Comment on attachment 9049359 [details] [diff] [review]
de-xbl-mail-messageids-headerfield.patch

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

::: mail/base/content/mailWidgets.js
@@ +200,5 @@
> +
> +  _toggleWrap() {
> +    for (let i = 0; i < this.headerValue.childNodes.length; i += 2) {
> +      if (!this.showFullMessageIds) {
> +        this.toggleIcon.setAttribute("open", "true");

forgot to add, it would be nice to while we touch this do the more webby thing and just add it as a class.
this.toggleIcon.classList.add("open")

::: mail/themes/shared/mail/messageHeader.css
@@ +273,5 @@
> +.emailToggleHeaderfield:-moz-locale-dir(rtl) {
> +  background: url("chrome://global/skin/icons/twisty-collapsed-rtl.svg");
> +}
> +
> +.emailToggleHeaderfield[open] {

... and here then

emailToggleHeaderfield.open {

	Magnus Melin [:mkmelin] Reporter
	Comment 25 • 6 years ago

Comment on attachment 9050147 [details] [diff] [review]
de-xbl-mail-messageids-headerfield.patch

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

Almost there I think. Please provide a proper commit message, like

Bug 1531901 - convert the mail-messageids-headerfield binding to custom element. r?mkmelin

::: mail/base/content/mailWidgets.js
@@ +174,5 @@
> +/**
> + * MozMailMessageidsHeaderfield is a widget used to show/link messages in the message header.
> + * Shown by default for nntp messages, not for regular emails.
> + * Make it visible for all the emails in case of Reply or Fwd by settting the
> + * mailnews.headers.showReferences pref to true in the mailnews/mailnews.js file.

wouldn't discuss how to set it, and people would usually do that through the config editor (found under preferences | advanced)

@@ +181,5 @@
> +class MozMailMessageidsHeaderfield extends MozXULElement {
> +  connectedCallback() {
> +    if (this.hasChildNodes()) {
> +      return;
> +    }

I think this should also do a 

if (this.delayConnectedCallback()) {
  return;
}

That should be used as general practice unless we know the element won't have children or if the connectedCallback doesn't do anything with children.

	Magnus Melin [:mkmelin] Reporter
	Comment 28 • 6 years ago

Comment on attachment 9050836 [details] [diff] [review]
de-xbl-mail-messageids-headerfield.patch

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

::: mail/base/content/mailWidgets.js
@@ +193,5 @@
> +      this._toggleWrap();
> +    });
> +    this.appendChild(this.toggleIcon);
> +
> +    this.headerValue = document.createElement("div");

sorry I didn't notice this before, but this is wrong. what you create here is a <xul:div> element (document.createElement uses the document namespace to create the element). 

But <xul:div> is not a thing. What you'd use is probably an hbox (like in the xbl binding originally)

::: mail/themes/linux/mail/messageHeader.css
@@ +249,4 @@
>    margin-inline-start: 3px !important;
>    border: none !important;
>    background-color: transparent;
> +  display: block;

these changes are now in the wrong file if they are needed. you need to put them in the shared messageHeader.css instead of the linux specific

	Magnus Melin [:mkmelin] Reporter
	Comment 32 • 6 years ago

Comment on attachment 9051075 [details] [diff] [review]
de-xbl-mail-messageids-headerfield.patch

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

Looks good, thx! r=mkmelin

Please send it off for a try run to see you didn't break any mozmill tests. Let's you test out your new hg privileges.
Post the try run link here in the bug. If it looks good (no new failures), you add the checkin-needed keyword