Magnus Melin [:mkmelin]
	Comment 2 • 5 years ago

Comment on attachment 9094168 [details] [diff] [review]
Bug-1582424_remove-grid-chat-tooltip.patch

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

::: chat/content/chat-tooltip.js
@@ +289,5 @@
>          label.setAttribute("value", aLabel);
> +        let td1 = document.createElementNS(
> +          "http://www.w3.org/1999/xhtml",
> +          "td"
> +        );

looks like the first one should be <th>

But don't add a <label> under them, just add the content (createTextNode)

	Magnus Melin [:mkmelin]
	Comment 6 • 5 years ago

Comment on attachment 9094213 [details] [diff] [review]
Bug-1582424_remove-grid-chat-tooltip.patch

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

::: chat/content/chat-tooltip.js
@@ +287,5 @@
> +        let th = document.createElementNS("http://www.w3.org/1999/xhtml", "th");
> +        th.textContent = aLabel;
> +        row.appendChild(th);
> +        let td = document.createElementNS("http://www.w3.org/1999/xhtml", "td");
> +        description = td;

seems a bit unnecesary to have the let td here. 
Just do descriptition = document.createElementNS("http://www.w3.org/1999/xhtml")

@@ +297,5 @@
>        }
>        description.textContent = aValue;
>      }
>  
>      addSeparator() {

Didn't spot this earlier, but the addSeparator() should be redone, instead of adding an "empty" row.
Like in the other bug, add a class to style as needed.

	Magnus Melin [:mkmelin]
	Comment 8 • 5 years ago

Comment on attachment 9094879 [details] [diff] [review]
Bug-1582424_remove-grid-chat-tooltip.patch

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

Looks good to me, r=mkmelin

::: chat/content/chat-tooltip.js
@@ +213,5 @@
>                  <description class="tooltipMessage"></description>
>                </stack>
>              </hbox>
> +            <table xmlns="http://www.w3.org/1999/xhtml" class="tooltipTable">
> +            </table>

maybe just
<html:table class="tooltipTable"></table>

Everything beneath it will be html sure, but since you're creating through script, it doesn't matter.

	Patrick Cloke [:clokep]
	Comment 10 • 5 years ago

Comment on attachment 9094892 [details] [diff] [review]
Bug-1582424_remove-grid-chat-tooltip.patch

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

::: chat/content/chat-tooltip.js
@@ +300,5 @@
>  
>      addSeparator() {
> +      if (this.table.hasChildNodes()) {
> +        let lastElement = this.table.lastChild;
> +        lastElement.querySelector("th").classList.add("chatTooltipSeparator");

Would it make sense to add this to the `tr` (which is likely `lastElement`), instead of the `th` and `td` separately?