Paul Rouget [:paul] Reporter
	Description • 16 years ago

The current treerow layout is a horizontally drawn set of cells (width based on the column width, and height based on the row height). Each cell could hold a text (cropped if needed, but using just one line) which could be styled as a all (no way to make one piece of the text green and another piece red).

Here, I propose a way to improve the current tree mechanism in order to make treerows richer without breaking the current API. The following does not mean we have to rewrite every nsITreeView implementation. It would be especially useful for TB and its message list's pane (see bug 213945).

Any comments about the relevancy of such improvement and about the following mechanism are welcome :)

Proposed new layout:

------------

Each row use the same height (needed for performance issues, like current behaviour) (CSS defined);
The Y offset and the height of each cell are independent (CSS defined), not based on the row height.
The width and the X offset of each cell is the same as the related column (like current behavior).
The text embedded in each cells is reachable as a set of piece of text ("items", reachable from the new treeview API).
Each piece of text is independently stylable (through properties set from on the new treeview API).
The text is drawn using all available vertical space.
A rect embedding text ("description") is drawn in the top of the cells. This block works like cells, but the X offset and the width is not restricted (CSS defined). Overlap with cells is assumed by the CSS rules.

CSS rules:
If the rows and the cells use this new mechanism:
Rows and cells have a new property: "multiline";
The new block ("description") is stylable from a new pseudo-element: -moz-tree-description, and description items from -moz-tree-description-item;
The cell items are stylable from a new pseudo-element: -moz-tree-cell-item;
Each item and description could have a set of properties set from the new treeview API;

nsITreeView2:
interface nsITreeView2: nsITreeView
{
  AString getCellItemText(in long row, in nsITreeColumn col, in long idxItem);
  long getCellItemCount(in long row, in nsITreeColumn col);
  void getCellItemProperties(in long row, in nsITreeView col, in long idxItem, in nsISupportsArray properties);

  AString getDescriptionItemText(in long row, in long idxItem);
  long getDescriptionItemCount(in long row);
  void getDescriptionItemProperties(in long row, in long idxItem, in nsISupportsArray properties);

  boolean isMultiline();
};

The "isMultiline" is useful if you want the TreeBodyFrame to use the multiline view or the classic view.
This could be useful if you want to let the user choose the desired view (you don't have to implement two treeview).

Know problems:
The main issue is about the description box coordinates.
If you want to display the description below the cells, you would set a top margin to avoid overlap. But you could want to make the last cell bigger (a cell using all the row height). Then, you must define a right margin in the description. But if your columns could be resized or could be moved, then, an overlap is still possible.
What should be the correct description behavior in a hierarchic tree ? Should it follow the indentation ? Could be optionnal.

P.S: used termes (description, items, multine, nsITreeView2) may be not very relevant. Feel free to propose alternatives :)

	Paul Rouget [:paul] Reporter
	Comment 3 • 16 years ago

More screenshots here: http://www.mozbox.org/pub/tb/multilineView/
Mainly TB related.

	David Ascher (:davida)
	Comment 4 • 16 years ago

It'd be really good to see something like this in 1.9.1 for Tb3.  

bz (out of a hat): any chance you could give this patch some quick feedback, so that Paul can know what's needed to make it review-ready?

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 16 years ago

It'd make sense to have someone sorta familiar with trees look at this instead of me...

	Neil Deakin
	Comment 6 • 16 years ago

I didn't look at the code in any detail. Some comments:

> interface nsITreeView2: nsITreeView

You should either just add the new methods to nsITreeView or use a more descriptive name like nsIMultilineTreeView.

'treeitem' is already used as a tagname, so calling the text pieces 'items' is confusing. Instead, I would suggest 'textpart' as in '-moz-tree-cell-text-part' or getCellTextPart.

> What should be the correct description behavior in a hierarchic tree ?

Each text part should just be drawn one underneath each other.

I'm not clear what 'description' is and how is differs from 'text parts'.

> +  nsCOMPtr<nsITreeView2> mView2;

Don't store this separately. Just get it again when needed.

What does 'multiline' get you? Multiple parts of text or one piece of text on multiple lines?

Is getItemText(0, 0) supposed to be equivalent to getCellText(0)? 

	Paul Rouget [:paul] Reporter
	Comment 7 • 16 years ago

(In reply to comment #6)
> I didn't look at the code in any detail. Some comments:
> 
> > interface nsITreeView2: nsITreeView
> 
> You should either just add the new methods to nsITreeView or use a more
> descriptive name like nsIMultilineTreeView.

Changing nsITreeView requires an update for each nsITreeView implementation.
nsIMultilineTreeVew sounds good.

> 'treeitem' is already used as a tagname, so calling the text pieces 'items' is
> confusing. Instead, I would suggest 'textpart' as in '-moz-tree-cell-text-part'
> or getCellTextPart.

Yes, it sounds more appropriate.

> > What should be the correct description behavior in a hierarchic tree ?
> 
> Each text part should just be drawn one underneath each other.

You mean descriptions boxes should not be impacted by the hierarchic behavior ? No left margin ?

> I'm not clear what 'description' is and how is differs from 'text parts'.

The description is a box containing a set of "textpart" (as a cell), but it's not anchored to any column. It's drawn directly in the row (the blue part in attachment 326398 [details]).

> > +  nsCOMPtr<nsITreeView2> mView2;
> 
> Don't store this separately. Just get it again when needed.
> 
> What does 'multiline' get you? Multiple parts of text or one piece of text on
> multiple lines?

Multiple parts of text on multiple lines:

a cell or a description box:
___________________
|part1part2part3pa|
|rt4part5part6    |
|_________________|


> 
> Is getItemText(0, 0) supposed to be equivalent to getCellText(0)? 
> 
No.

var foobar = getCellText(0);

is equivalent to:

var count = getCellItemCount(0);
var foobar = "";
for (var i = 0; i < count; i++) {
  foobar += getCellItemText(0, i);
}

	Neil Deakin
	Comment 8 • 16 years ago

I'm still a bit confused by item parts and descriptions. There are several 'blue parts' in that image. Also, the description api you mention in your first comment doesn't exist in the patch you provided.

Can you provide a example of what would be displayed in the following:

getCellText(x, y) returns "Cell Text"
getCellTextPart(x, y, 0) returns "Apples"
getCellTextPart(x, y, 1) returns "Oranges"
(and assuming you meant to implement getDescriptionText as well):
getDescriptionText(x, 0) returns "Lion"
getDescriptionText(x, 1) returns "Tiger"
All getXXXProperties return nothing of interest, and no stylesheet is used.

	Paul Rouget [:paul] Reporter
	Comment 9 • 16 years ago

(In reply to comment #8)
> I'm still a bit confused by item parts and descriptions. There are several
> 'blue parts' in that image.

Description boxes are boxes containing text like "Path: .......". One description box per row (only 3 are blue).

> Also, the description api you mention in your first
> comment doesn't exist in the patch you provided.

Yes, I'm sorry, the patch doesn't really match this proposed API.
In this patch, multipart concept was just about description, not about cells, so getItemText (in the patch) is the equivalent of getDescriptionText (in this proposed API) ... a bit confusing.

> Can you provide a example of what would be displayed in the following:
> 
> getCellText(x, y) returns "Cell Text"

If the component implements nsIMultilineTreeView, getCellText isn't used.

> getCellTextPart(x, y, 0) returns "Apples"
> getCellTextPart(x, y, 1) returns "Oranges"
> (and assuming you meant to implement getDescriptionText as well):
> getDescriptionText(x, 0) returns "Lion"
> getDescriptionText(x, 1) returns "Tiger"
> All getXXXProperties return nothing of interest, and no stylesheet is used.
> 

So the text in the cell x/y will be: "ApplesOranges" (which would be cropped and displayed using several lines if needed).

The text in the description box will be: "LionTiger" (which would be cropped and displayed using several lines if needed).


In short:
If a treeview implements nsITreeView and not nsIMultilineTreeView, it's drawn as the old way. If it implements nsIMultilineTreeView, getCellText isn't used, but getCellTextPart are. In addition, an other box is drawn, named "description", using the getDescriptiontext methods, and drawn relatively to the row.

I hope I'm more understandable.

	Neil Deakin
	Comment 10 • 16 years ago

No, as I don't understand what a description box is and how it relates to the cell text parts and the cell value, I want an actual screenshot or a diagram showing exactly what would be displayed in the example I gave.

getCellText needs to return a value for accessibility purposes.

	Neil Deakin
	Comment 11 • 16 years ago

(In reply to comment #9)

> Description boxes are boxes containing text like "Path: .......". One
> description box per row (only 3 are blue).

There are seven boxes in the image that are blue, all of which contain the text 'Path:'

	Paul Rouget [:paul] Reporter
	Comment 13 • 16 years ago

Blue boxes are description boxes. Red boxes are cells.

	Neil Deakin
	Comment 14 • 16 years ago

Ah, OK, now I understand. The code doesn't implement it this way I think, which is why I couldn't figure this out. So this looks pretty good then.

The text parts specify pieces of text to use instead of getCellText which specifies only one piece. In that case, it makes sense that implementors make getCellText return the text appended together, or some variation in case only certain parts of the text are useful.

The description applies to the entire row and is displayed underneath the column text. I think it should appear flush with the cell text of the primary column.

Both text parts and the description wrap right? Do text parts wrap only at part boundaries or in between a string when reaching the end of the line?

	Paul Rouget [:paul] Reporter
	Comment 15 • 16 years ago

(In reply to comment #14)
> Ah, OK, now I understand. The code doesn't implement it this way I think, which
> is why I couldn't figure this out. So this looks pretty good then.
> 
> The text parts specify pieces of text to use instead of getCellText which
> specifies only one piece. In that case, it makes sense that implementors make
> getCellText return the text appended together, or some variation in case only
> certain parts of the text are useful.

Yes.

> The description applies to the entire row and is displayed underneath the
> column text.

Cells could be displayed underneath the description box. It depends on the CSS rules. In the screenshot, description boxes have a "margin-top: 18px;" rule, and cells have a "height: 18px" rule.

But such behavior could be wrong. Coords are calculated from fixed rules (CSS), so if a column is moved, that could create an overlap.
Example:
In the last screenshot (attachment 327116 [details]), the last cell is bigger. No overlap because the description box have a right margin (defined in the css file). But, if the user resize the XXX column, the content of the cell could overlap the content of the description.
So dynamically calculate the description coords and size (in order to make the box use the maximum space available) could be the solution... I don't know.

> I think it should appear flush with the cell text of the primary
> column.

Ok.

> Both text parts and the description wrap right?
Yes.

> Do text parts wrap only at part boundaries or in between a string when reaching the end of the line?

Between a string, example:

4 text parts: "part1", "bigpart2", "part3", "part4"
___________
|part1bigpa|
|rt2part...|
|__________|

	Neil Deakin
	Comment 16 • 16 years ago

(In reply to comment #15)
> Cells could be displayed underneath the description box. It depends on the CSS
> rules. In the screenshot, description boxes have a "margin-top: 18px;" rule,
> and cells have a "height: 18px" rule.

Where are these rules specified? I'd like a screenshot of what it looks like where the developer has *no stylesheets defined*. All of the functionality should work as is and be displayed in an appropriate manner without the developer having to use any stylesheets.

	Paul Rouget [:paul] Reporter
	Comment 17 • 16 years ago

This patch is based on the proposed API.
Graphical elements works without any user defined style.

	Marco Zehe (:MarcoZ)
	Comment 22 • 16 years ago

What about the keyboard navigation model of this new widget? Does it match standard tree views, or is the behavior different when it's in multi-line mode?

	Paul Rouget [:paul] Reporter
	Comment 23 • 16 years ago

The keyboard navigation model is not affected.

	Paul Rouget [:paul] Reporter
	Comment 26 • 16 years ago

In order to make the patch review-ready, I need:

* some feedback about the code (which is currently pretty rough)
* a validation of the current behavior
* a validation of the current API/used names/CSS rules
* some information about accessibility issues (is there any stuff I should care about ?)
* someone who can test the patch on Windows and MacOS ?
* any advise you could give me :)

... and I must write some tests.

	Paul Rouget [:paul] Reporter
	Comment 27 • 16 years ago

Yet, Thunderbird team does not clearly define their needs about the new tree layout.

For instance, we're talking about rows with different sizes and a set of "description" like boxes which could host content like image/singlepart text/multipart text.

While we are not ok about what it's needed, I don't update my code.

	Andrew Sutherland [:asuth] (he/him)
	Comment 28 • 16 years ago

I think the ill-defined needs referred to are those of the 'experimental message view', which is going to go with a different approach, at least for prototyping.  So let's take that off the table.

These changes are still desired for bug 213945 (allowing a better vertical view configuration by having multi-line tree items through the use of descriptions) and bug 364090 (by allowing the preview to be styled differently and also be on its own line).  So I would say those are the use-cases to target, and we would still very much like this excellent functionality available in Thunderbird.

	Neil Deakin
	Comment 29 • 16 years ago

Paul, did you want someone to review this?

	Paul Rouget [:paul] Reporter
	Comment 30 • 16 years ago

(In reply to comment #29)
> Paul, did you want someone to review this?

Yes, for a feedback. Who should I ask to ?

	Dan Mosedale (:dmosedale, :dmose)
	Comment 31 • 16 years ago

Comment on attachment 329007 [details] [diff] [review]
updated patch: description box supports "display: none". Should work on Windows and Mac (not tested)

Neil's a good person to ask for review here, I think.  Doing so.  :-)

	Neil Deakin
	Comment 32 • 16 years ago

Comment on attachment 329007 [details] [diff] [review]
updated patch: description box supports "display: none". Should work on Windows and Mac (not tested)


+[scriptable, uuid(3c2a2d26-08d6-415b-a635-24f15983bde6)]
+interface nsIMultilineTreeView: nsITreeView
...
+  long getDescriptionTextPartCount(in long row);
+  AString getDescriptionTextPart(in long row, in long partIdx);
+  void getDescriptionTextPartProperties(in long row, in long partIdx, in nsISupportsArray properties);

I wonder if we should use nsIArray here. Not a big deal for now. Add documentation comments for this interface and methods.

+  boolean isMultiline();
+};
+
+

Remove extra blank lines
 
     // We don't want to consider any of the decorations that may be present
     // on the current row, so we have to deflate the rect by the border and 
     // padding and offset its left and top coordinates appropriately. 
+
     AdjustForBorderPadding(rowContext, cellRect);
 
+    NS_NAMED_LITERAL_CSTRING(cell, "cell");
     nsStyleContext* cellContext = GetPseudoStyleContext(nsCSSAnonBoxes::moztreecell);
-
-    NS_NAMED_LITERAL_CSTRING(cell, "cell");

This change doesn't seem necessary.

     if (currCol->IsCycler() || cell.Equals(aElement)) {
       // If the current Column is a Cycler, then the Rect is just the cell - the margins. 
       // Similarly, if we're just being asked for the cell rect, provide it. 
 
       theRect = cellRect;
       nsMargin cellMargin;
       cellContext->GetStyleMargin()->GetMargin(cellMargin);
       theRect.Deflate(cellMargin);
@@ -1933,16 +1942,21 @@ nsTreeBodyFrame::PrefillPropertyArray(PR
   if (mFocused)
     mScratchArray->AppendElement(nsGkAtoms::focus);
 
   // sort
   PRBool sorted = PR_FALSE;
   mView->IsSorted(&sorted);
   if (sorted)
     mScratchArray->AppendElement(nsGkAtoms::sorted);
+
+  // multiline
+  if (mIsMultiline)
+    mScratchArray->AppendElement(nsGkAtoms::multiline);
+

remove extra blank line
 
+PRInt32 nsTreeBodyFrame::GetCellHeight(PRInt32 aRow, nsITreeColumn* aColumn) {
...
+  if (cellHeight < cellMinHeight)
+    cellHeight = cellMinHeight;

cellHeight = PR_MIN(cellHeight, cellMinHeight);

+  nscoord height;
+  // As good a default as any.
+  if (mIsMultiline) {
+    height = 36;
+  } else {
+    height = 18;
+  }
+  return nsPresContext::CSSPixelsToAppUnits(height);
+}
+

Why not:
  return nsPresContext::CSSPixelsToAppUnits(mIsMultiline ? 36 : 18);

Also, remove the extra blank line.
 
+        PRInt32 xOffSet = 0;
+        nsTreeColumn* primaryCol = mColumns->GetPrimaryColumn();
+        if (primaryCol) {
+          PRInt32 cellX, cellY, cellWidth, cellHeight;
+          GetCoordsForCellItem(aRowIndex, primaryCol, NS_LITERAL_CSTRING("image"),
+              &cellX, &cellY, &cellWidth, &cellHeight);
+          xOffSet = nsPresContext::CSSPixelsToAppUnits(cellX); 

Hmmm. This seems an unoptimal way of calculating just the x coordinate. Not sure if this could be improved.
 
   // Now paint our element, but only if we aren't a cycler column.
   // XXX until we have the ability to load images, allow the view to 
   // insert text into cycler columns...
+
+

Remove extra blank lines

 }
+
+ 
+void
+nsTreeBodyFrame::PaintDescription(PRInt32              aRowIndex,

Remove extra blank line

+                                  const nsRect&        aDescRect,
+                                  nsPresContext*       aPresContext,
+                                  nsIRenderingContext& aRenderingContext,
+                                  const nsRect&        aDirtyRect,
+                                  nsPoint              aPt)

aPt is never used, so remove it.

+  // Adjust the rect for its border and padding.
+  nsMargin borderPadding(0, 0, 0, 0);
+  GetBorderPadding(descContext, borderPadding);
+
+  AdjustForBorderPadding(descContext, descRect);

borderPadding isn't used. I think you should remove those two lines.

+  nscoord currX = descRect.x;
+  nscoord remainingWidth = descRect.width;
+
+  nsRect elementRect(currX, descRect.y, remainingWidth, descRect.height);

elementRect will end up being equal to descRect. Just use descRect directly.

+}
+
+ 
+void
+nsTreeBodyFrame::PaintMultilineText(nsAutoString&        aText,

Remove extra blank line.

+  // Obtain the margins for the text and then deflate our rect by that 
+  // amount.  The text is assumed to be contained within the deflated rect.
+  nsRect textRect(aTextRect);

The comment doesn't apply here.

+
+  // ------

?

+          tmpText.Left(textToDraw, i);
+          textToDraw.Append(kEllipsis);
+          tmpText.Right(tmpText, tmpText.Length() - i);

No need to adjust tmpText right? It's the last line, so we should empty it.

+        }
+      } else {
+        nscoord cwidth;
+        nscoord twidth = 0;
+        int length = tmpText.Length();
+        int i;
+        for (i = 0; i < length; ++i) {
+          PRUnichar ch = tmpText[i];
+          // XXX this is horrible and doesn't handle clusters
+          aRenderingContext.GetWidth(ch,cwidth);
+          if (twidth + cwidth > width)
+            break;
+          twidth += cwidth;
+        }
+        tmpText.Left(textToDraw, i);
+        tmpText.Right(tmpText, tmpText.Length() - i);

Just 'length' can be used instead of 'tmpText.Length()'

+
+
 
 void
 nsTreeBodyFrame::PaintTwisty(PRInt32              aRowIndex,

Remove extra blank lines.


There are also a number of places where you've included extra blank lines such as this in the headers and css files. Remove those.

+  PRBool mIsMultiline;

This should be intialized in the constructor as well just to be safe.

Is there no way to share more code between PaintMultilineText and PaintText or AdjustForCellText? Maybe some extra functions perhaps?

Also, I notice that text doesn't word-wrap. That doesn't sound particularly as useful. Maybe ask roc if there is a convenient text drawing api available that takes a string(s) and wraps and paints them into a bounding box.

In the example you provided, on Mac, the description text doesn't repaint properly when the window is resized smaller and then larger again.

The actual feature looks great though. For accessibility, we can assume that getCellText() should return all of the text concatenated.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 33 • 16 years ago

> Maybe ask roc if there is a convenient text drawing api available that
> takes a string(s) and wraps and paints them into a bounding box.

There isn't. Watch out for bidi! nsCanvasRenderingContext2D has some reasonably smart text drawing you might be able to get ideas from.

	alexander :surkov (:asurkov)
	Comment 34 • 16 years ago

(In reply to comment #32)

> The actual feature looks great though. For accessibility, we can assume that
> getCellText() should return all of the text concatenated.

That's the way for unaccessible content. But since we need to expose style changes to AT as well then we should depend on new interface. Once we get this patch landed I'll put the patch for a11y issue.

	David :Bienvenu
	Comment 35 • 16 years ago

This patch seems to have stalled a bit - we would love to be able to use this in TB 3.0...

	Paul Rouget [:paul] Reporter
	Comment 36 • 16 years ago

I'm looking for some help about word wrapping and cropping mechanism :

I have a set of text part.
I have the largest box which will host the text.
I need to compute the text bounding box for a given text.
There are two context where I need to know the bounding box :
* to know which item is at a given coordinate (function GetItemWithinCellAt)
* where to draw the text : needed *before* drawing the text since we first have to know the height of the bounding box for the center vertical align.

So I can't wrap and crop the text only during the draw process.

Two choice :
* wrapping and cropping two times (compute the bounding box and draw the text) -> perf issues
* wrapping and cropping one time, but we have to "save" the result of a such computation -> dynamic allocation -> perf issues

A solution could be :
* no vertical align : start at the top of the cell/description box (so crop and wrap during the draw process)
* For GetItemWithingCellAt, base the result on the cell box and not on the text bounding box (no wrap/crop process needed). Rough result.

Comments ?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 37 • 16 years ago

I don't quite understand. All these text parts are on the same line?

	Paul Rouget [:paul] Reporter
	Comment 38 • 16 years ago

No. According to the size of the complete text and the size of the cell or description box, these parts would be drawn on several lines.

Please see attachment 327116 [details] :

The description box of the 4th row (the blue box under the "syncCVS-1.8.sh" cell) is wrapped and cropped. There are 3 part (a green one, a red one and a grey one).

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 39 • 16 years ago

What I mean is, the different text items always place horizontally (like a XUL hbox), not stacking vertically (like a XUL vbox)?

	Neil Deakin
	Comment 40 • 16 years ago

What we actually want here is to have a bounding box (which we know the size of beforehand, right?) and render a set of strings into it, with wrapping.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 41 • 16 years ago

That's hugely painful if you want to handle mixed-direction text.

	Neil Deakin
	Comment 42 • 16 years ago

(In reply to comment #41)
> That's hugely painful if you want to handle mixed-direction text.

Why? The implementation is complex? What complexity arises that doesn't for say, a normal block with some inline text strings in it?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 43 • 16 years ago

None. A normal block with inline text strings is really complicated, if you want to handle mixed-direction bidi text, and especially if you want text runs to be given different styles.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 44 • 16 years ago

You might want to look at nsCanvasRenderingContext2D for some clues about displaying mixed-direction text; it also handles measurement, although it doesn't actually handle line-breaking.

	Paul Rouget [:paul] Reporter
	Comment 45 • 16 years ago

Neil, we know the size of the cell (or description) box, but we don't know the
text bounding box :

After deflating (margin border and padding), we have a box where the text will
be paint. This is not the final text bounding box.

Beforehand, we don't know the number of lines needed for drawing the text and
the width of the largest line which will be paint.
In order to know the number of lines and the width of the largest line, we need
to collect all text parts and then apply the word-wrap/crop mechanism.
Once we know that, we know the height and the width of the bounding box.
Then, we can draw the text at the correct x/y position (according to the align
properties).

All those last operations will be needed again for drawing the text.

So I'm looking for a way to make such mechanism more efficient.

See my current solution in comment 36.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 46 • 16 years ago

If you gather all the strings, do the bidi splitting, then generate a list of textruns, you've done most of the work. Measuring and painting a textrun after you've created it is really cheap.

	Peter Lairo
	Comment 47 • 16 years ago

This bug would also finally make Thunderbird's "Vertical View" usable!?

+------------------------------------------------------------------------------+
| All Folders | Long Subject #1 of an e-mail that is so  | This is the body of |
| Account1    | long that it takes up two lines (bold!)  | the selected e-mail.|
|  - Inbox    | From: Name1@domain1.com Date: dd.mm.yyyy | Bla bla bla. Bla bla|
|  - Mail     |------------------------------------------| bla bla bla.        |
|    - IT     | Long Subject #2 of an e-mail that is so  | Bla bla bla. Bla bla|
|    - Politic| long that it takes up two lines (bold!)  | Bla bla bla. Bla    |
|    - Persona| From: Name2@domain2.com Date: dd.mm.yyyy | Bla bla bla. Bla bla|
|    - Work   |------------------------------------------| Bla bla bla. Bla bla|
|             | Long Subject #3 of an e-mail that is so  | Bla bla bla.        |
|             | long that it takes up two lines (bold!)  |                     |
|             | From: Name3@domain3.com Date: dd.mm.yyyy |                     |
+------------------------------------------------------------------------------+

	Peter Lairo
	Comment 48 • 16 years ago

Sorry about the broken ASCII art (was too wide by one column). :-[

+----------------------------------------------------------------------------+
|             | Long Subject #1 of an e-mail that is so | This is the body of|
| AccountName | long that it takes up two lines (bold!) | the selected e-mail|
|  -*Inbox*   | From: Name1@domain1.com Date: dd.mm.yyyy| Bla bla bla. Bla   |
|  - Mail     |-----------------------------------------| bla bla bla.       |
|   - IT      | *Long Subject #2 of an e-mail that is*  | Bla bla bla. Bla   |
|   - Politics| *long. It takes up two lines (bold!)*   | Bla bla bla. Bla   |
|   - Personal| *From: Name@domain.com Date: dd.mm.yy*  | Bla bla bla. Bla   |
|   - Work    |-----------------------------------------| Bla bla bla. Bla   |
|             | Long Subject #3 of an e-mail that is so | Bla bla.           |
|             | long that it takes up two lines (bold!) |                    |
|             | From: Name3@domain3.com Date: dd.mm.yyyy|                    |
+----------------------------------------------------------------------------+

	arno renevier
	Comment 50 • 14 years ago

(In reply to comment #32)
> (From update of attachment 329007 [details] [diff] [review])
> 
> +[scriptable, uuid(3c2a2d26-08d6-415b-a635-24f15983bde6)]
> +interface nsIMultilineTreeView: nsITreeView
> ...
> +  long getDescriptionTextPartCount(in long row);
> +  AString getDescriptionTextPart(in long row, in long partIdx);
> +  void getDescriptionTextPartProperties(in long row, in long partIdx, in
> nsISupportsArray properties);
> 
> I wonder if we should use nsIArray here. Not a big deal for now. Add
> documentation comments for this interface and methods.

Currently, all methods in nsITreeView use nsISupportsArray. So is it better to
use nsISupportsArray and use another bug to use nsISupportsArray in
nsITreeView/nsIMultilineTreeView ?

> 
> +        PRInt32 xOffSet = 0;
> +        nsTreeColumn* primaryCol = mColumns->GetPrimaryColumn();
> +        if (primaryCol) {
> +          PRInt32 cellX, cellY, cellWidth, cellHeight;
> +          GetCoordsForCellItem(aRowIndex, primaryCol,
> NS_LITERAL_CSTRING("image"),
> +              &cellX, &cellY, &cellWidth, &cellHeight);
> +          xOffSet = nsPresContext::CSSPixelsToAppUnits(cellX); 
> 
> Hmmm. This seems an unoptimal way of calculating just the x coordinate. Not
> sure if this could be improved.

I'm not sure we really want to use x coordinate as offset. For example, presentation would be very strang if primaryCol is second column of the tree. What about just computing padding, margin, indentation and moztreetwisty for primary row ?

I'll attach a patch that uses this solution, and fixes most of the problem you point in your comment

	arno renevier
	Comment 51 • 14 years ago

currently attached example (xulrunner app) works only with very first version of the patch. As nsIMultiTreeView api has changed in paul's second patch, here is an update xulrunner app.

	arno renevier
	Comment 52 • 14 years ago

I tried to work on the problem reported in comment 36 but did not have a lot of success.

In nsTreeBodyFrame::PaintDescription, once I have the description text, I  create a textRun with following code:



      const nsStyleFont* fontData = textContext->GetStyleFont();
      const nsFont& font = fontData->mFont;
      nsCAutoString langGroup;
      nsIAtom *langGroupAtom = aPresContext->GetLangGroup();
      if (langGroupAtom) {
        const char* lg;
        langGroupAtom->GetUTF8String(&lg);
        langGroup.Assign(lg);
      }
      gfxFontStyle fontStyle(font.style, font.weight, font.stretch, font.size,
                           langGroup, font.sizeAdjust, font.systemFont,
                           font.familyNameQuirks,
                           PR_FALSE);
      nsRefPtr<gfxFontGroup> fontGroup =
        gfxPlatform::GetPlatform()->CreateFontGroup(font.name, &fontStyle, PresContext()->GetUserFontSet());
      PRUint32 flags = gfxTextRunFactory::TEXT_NEED_BOUNDING_BOX |
        nsLayoutUtils::GetTextRunFlagsForStyle(textContext, GetStyleText(), fontData);
      nsRefPtr<gfxContext> ctx = aRenderingContext.ThebesContext();
      gfxTextRunFactory::Parameters params = {
        ctx, nsnull, nsnull, nsnull, 0, nsIDeviceContext::AppUnitsPerCSSPixel()
      };
      gfxTextRun* textRun = gfxTextRunCache::MakeTextRun(text.get(), text.Length(), fontGroup, &params, flags);



Then, instead of calling PaintMultilineText, I try to display it with



      gfxRect dirtyGfxRect ( gfxFloat(aDirtyRect.x) / aPresContext->DevPixelsToAppUnits(1),
                    gfxFloat(aDirtyRect.y) / aPresContext->DevPixelsToAppUnits(1),
                    gfxFloat(aDirtyRect.width) / aPresContext->DevPixelsToAppUnits(1),
                    gfxFloat(aDirtyRect.height) / aPresContext->DevPixelsToAppUnits(1)
                    );
      gfxPoint startPoint(NSAppUnitsToFloatPixels(aDirtyRect.x, PresContext()->AppUnitsPerDevPixel()),
                      NSAppUnitsToFloatPixels(aDirtyRect.y, PresContext()->AppUnitsPerDevPixel()));
      textRun->Draw(ctx.get(), startPoint, 0, textRun->GetLength(), &dirtyGfxRect, nsnull, nsnull);


but it does not work.

Do you have any clue to make this work ?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 53 • 14 years ago

        ctx, nsnull, nsnull, nsnull, 0, nsIDeviceContext::AppUnitsPerCSSPixel()

That should be AppUnitsPerDevPixel, but that won't fix your problem...

I think your problem is that the startPoint should be the baseline of the text --- i.e. the bottom of the letter, but you're setting it to the top-left of the dirty rect, so only the descenders (e.g. the tail of 'g') will be visible in the dirtyrect...

	Jim Porter (:squib)
	Comment 54 • 14 years ago

So, I know I'm probably going to regret asking this, but other than the stuff in comment 36, what remains to be cone here? I can probably work on this (with some guidance), but I'm not sure if this is "mostly done" or "barely started".

	Neil Deakin
	Comment 55 • 14 years ago

The main issue is supporting wrapped text. This work has been described above as "hugely painful" and "really complicated".

	Jim Porter (:squib)
	Comment 56 • 14 years ago

nsCanvasRenderingContext2D was mentioned above as something that does what needs to be done here. Is this function what I should be looking at? http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/nsCanvasRenderingContext2D.cpp#2606

I'm assuming (perhaps naively) here that while this is painful to do, it's already been done elsewhere (right?), so I can just take advantage of other people's pain in getting this working.

	ub.test
	Comment 57 • 13 years ago

(In reply to Jim Porter (:squib) from comment #56)
> nsCanvasRenderingContext2D was mentioned above as something that does what
> needs to be done here. Is this function what I should be looking at?
> http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/
> nsCanvasRenderingContext2D.cpp#2606
> 
> I'm assuming (perhaps naively) here that while this is painful to do, it's
> already been done elsewhere (right?), so I can just take advantage of other
> people's pain in getting this working.

Hi Jim,
Any luck on this?

	Jakob Kobberholm
	Comment 58 • 13 years ago

In an attempt to give a little incentive to anyone capable of solving this, I added this bug to FossFactory.org and donated $100 to it.

	Jakob Kobberholm
	Comment 59 • 13 years ago

Oh and a link to the project of course:
http://www.fossfactory.org/project/p294

	Ronan Jouchet
	Comment 60 • 13 years ago

Good idea, Jakob. I added $50 (actually $48.25 after deducing PayPal theft) to the sponsorship.

	mlissner@michaeljaylissner.com
	Comment 61 • 13 years ago

I put another $5, making this bug worth $165.

	ubuntu
	Comment 62 • 13 years ago

i put some 9,62€ in, perhaps we could get this long awaited message view in Thunderbird with the help of some money ;-)

	hugh
	Comment 63 • 13 years ago

The lack of 2-line headers in vertical view is what finally drove me to the paid Postbox version of Thunderbird (which has a decent implementation of this feature, as well as a polished version of the Conversations plugin). Perhaps I should have spent the money at FossFactory to get it into Thunderbird proper?

	Robert Augustin
	Comment 64 • 13 years ago

Added $50 to the Fossfactory fund. 

Come on you guys, join in if you haven't already. Even $5 or $10 will help.

	:aceman
	Comment 65 • 13 years ago

Looks like the reward is already at US$372.
But looks like a hard problem.

	Alfred Kayser
	Comment 66 • 12 years ago

Will a richlist do the trick, instead of coding a new structure?

	Jim Porter (:squib)
	Comment 67 • 12 years ago

(In reply to Alfred Kayser from comment #66)
> Will a richlist do the trick, instead of coding a new structure?

That all depends on how much you want to support. Even just pure HTML could "do the trick" if you limit the functionality in some ways. However, supporting customization is the hard part.

	Jonathan Protzenko [:protz]
	Comment 68 • 12 years ago

Also we have to keep in mind that:
- what makes Thunderbird good is the fact that scrolling the message list is really fast and efficient, 
- and that's because we're implementing a custom tree view from C++.

Moving away from a XUL tree would probably require significant rework of our code (I think).

	Colby Russell :crussell (no longer Mozilla-ing)
	Comment 69 • 12 years ago

I don't mean to be a jerk, but much of the discussion in this bug seems to be misplaced.

From what I can see, there are two things at play here:
- This is a bug with a low-level focus: adding to nsITreeView's bag of tricks
- There are people with a higher-level interest: the mail thread pane should offer a view like outlook, gmail, et cetera.  We have bug 213945 and bug 364090 for this (one of which may need to be duped against the other).

Whether the second thing happens as a direct result of the first thing isn't really the focus of those with the second interest.  It's possible that someone will manage do the second thing using a rich list, HTML, et cetera.  But that's the concern of bug 213945 or bug 364090, not this one.

	David Rees
	Comment 72 • 11 years ago

PostBox has made enhancements to TreeView that enable the view that folks want for Thunderbird bug 213945. That code is available due to MPL. See https://bugzilla.mozilla.org/show_bug.cgi?id=213945#c107.

	Yonggang Luo
	Comment 73 • 10 years ago

How did PostBox enhancements to TreeView?

	Paul Rouget [:paul] Reporter
	Comment 74 • 10 years ago

(In reply to Yonggang Luo from comment #73)
> How did PostBox enhancements to TreeView?

Apparently, they used the patch attached to this bug.

	Jim Porter (:squib)
	Comment 75 • 10 years ago

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #74)
> (In reply to Yonggang Luo from comment #73)
> > How did PostBox enhancements to TreeView?
> 
> Apparently, they used the patch attached to this bug.

How close would you say the patch here is to being completed? If it's nearly done, I could probably work on it.

	Yonggang Luo
	Comment 76 • 10 years ago

I am confused, if I use the patch, did this support for vbox/hbox in treecell?

	Yonggang Luo
	Comment 77 • 10 years ago

Does anybody know which version of Mozilla that PostBox are based on?
So I can do easier merge.

	Neil Deakin
	Comment 78 • 10 years ago

The patch in the bug adds support for multiline text. Getting text to wrap properly is difficult and is not done by this patch. Also, this patch is very old.

	Yonggang Luo
	Comment 79 • 10 years ago

I know, but I am working on this issue now, based on mozilla 31, so I need all kinds of suggestion to apply this patch to getting treecell to support multiline text, or even more, support for vbox/hbox in trecell, but I need help, at the first stage, I will getting mozilla 31 to support for multiline, that means apply this patch to mozilla 31.

	Magnus Melin [:mkmelin]
	Comment 80 • 10 years ago

(In reply to Yonggang Luo from comment #77)
> Does anybody know which version of Mozilla that PostBox are based on?

I think Gecko 7.0.1 (yes, anchient, thunderbird 7 something)

	Paul Rouget [:paul] Reporter
	Comment 81 • 10 years ago

(In reply to Yonggang Luo from comment #79)
> I know, but I am working on this issue now, based on mozilla 31, so I need
> all kinds of suggestion to apply this patch to getting treecell to support
> multiline text, or even more, support for vbox/hbox in trecell, but I need
> help, at the first stage

What kind of help do you need? You might want to start with https://developer.mozilla.org/en-US/docs/Introduction and https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide - for general help (not directly related to the multiline implementation), IRC or mailing lists are better than this bug.

Try to rebase attachment 428204 [details] [diff] [review] first (on top of mozilla central, not Firefox 31).

	Yonggang Luo
	Comment 82 • 10 years ago

(In reply to Magnus Melin from comment #80)
> (In reply to Yonggang Luo from comment #77)
> > Does anybody know which version of Mozilla that PostBox are based on?
> 
> I think Gecko 7.0.1 (yes, anchient, thunderbird 7 something)
Thanks a lot, I am rebasing it now.

	Wayne Mery (:wsmwk)
	Comment 83 • 10 years ago

(In reply to Yonggang Luo from comment #82)
> (In reply to Magnus Melin from comment #80)
> > (In reply to Yonggang Luo from comment #77)
> > > Does anybody know which version of Mozilla that PostBox are based on?
> > 
> > I think Gecko 7.0.1 (yes, anchient, thunderbird 7 something)
> Thanks a lot, I am rebasing it now.

I think it's more recent than version 7.  Matt has some insight into their use of Thunderbird code.

Their code is at http://www.postbox-inc.com/coveredcode

	xunxun
	Comment 84 • 10 years ago

(In reply to Wayne Mery (:wsmwk) from comment #83)
> (In reply to Yonggang Luo from comment #82)
> > (In reply to Magnus Melin from comment #80)
> > > (In reply to Yonggang Luo from comment #77)
> > > > Does anybody know which version of Mozilla that PostBox are based on?
> > > 
> > > I think Gecko 7.0.1 (yes, anchient, thunderbird 7 something)
> > Thanks a lot, I am rebasing it now.
> 
> I think it's more recent than version 7.  Matt has some insight into their
> use of Thunderbird code.
> 
> Their code is at http://www.postbox-inc.com/coveredcode

I downloaded the code, though he hide the thunderbird version number, I found that his useragent is 7.0.1, so I think it is based on thunderbird 7.0.1

	Yonggang Luo
	Comment 85 • 10 years ago

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #81)
> (In reply to Yonggang Luo from comment #79)
> > I know, but I am working on this issue now, based on mozilla 31, so I need
> > all kinds of suggestion to apply this patch to getting treecell to support
> > multiline text, or even more, support for vbox/hbox in trecell, but I need
> > help, at the first stage
> 
> What kind of help do you need? You might want to start with
> https://developer.mozilla.org/en-US/docs/Introduction and
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide - for
> general help (not directly related to the multiline implementation), IRC or
> mailing lists are better than this bug.
> 
> Try to rebase attachment 428204 [details] [diff] [review] first (on top of
> mozilla central, not Firefox 31).
I am looking for incremental building instructions (for testing) and 
something help me to running tests, I need to verify my code,
I will try to base on Firefox 31 first because it's stable, and then I will rebase it on mozilla central.

	Yonggang Luo
	Comment 86 • 10 years ago

It's seems the multiline already been committed in nsGkAtomList.h
How to deal with this issue?

	Yonggang Luo
	Comment 87 • 10 years ago

I removed the orignal "multiline" defines, don't know if that is a good idea. 

 content/base/src/nsGkAtomList.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/content/base/src/nsGkAtomList.h b/content/base/src/nsGkAtomList.h
index a6cf9fb..5bc3512 100644
--- a/content/base/src/nsGkAtomList.h
+++ b/content/base/src/nsGkAtomList.h
@@ -2191,7 +2191,7 @@ GK_ATOM(marginTop, "margin-top")
 GK_ATOM(menuitemcheckbox, "menuitemcheckbox")
 GK_ATOM(menuitemradio, "menuitemradio")
 GK_ATOM(mixed, "mixed")
-GK_ATOM(multiline, "multiline")
+//GK_ATOM(multiline, "multiline")
 GK_ATOM(password, "password")
 GK_ATOM(posinset, "posinset")
 GK_ATOM(presentation, "presentation")

	xunxun
	Comment 88 • 10 years ago

(In reply to Yonggang Luo from comment #86)
> It's seems the multiline already been committed in nsGkAtomList.h
> How to deal with this issue?

It's changed in bug 481395

nsAccessibilityAtoms->nsGkAtoms

	Yonggang Luo
	Comment 89 • 10 years ago

(In reply to xunxun from comment #88)
> (In reply to Yonggang Luo from comment #86)
> > It's seems the multiline already been committed in nsGkAtomList.h
> > How to deal with this issue?
> 
> It's changed in bug 481395
> 
> nsAccessibilityAtoms->nsGkAtoms

Thanks for your quick response, and I am not using nsAccessibilityAtoms, just nsGkAtom, but the problem is I am using a existed nsGkAtom multiline that conflict with  Paul Rouget's patch
I don't know if I can still use this GkAtom

	Yonggang Luo
	Comment 90 • 10 years ago

What's the replacement in mozilla 31 for nsISupportsArray 

It's already removed in xul/tree, now it's use AtomArray instead of nsISupportsArray*

	Yonggang Luo
	Comment 91 • 10 years ago

I am trying to use GK_ATOM(multiline, "multiline") defined in nsGkAtomList.h, but it under the macro #ifdef ACCESSIBILITY, may I move it to the global place?

	Yonggang Luo
	Comment 92 • 10 years ago

This is the updated multiline patch that support for mozilla 31esr, should be easy to cherry-pick to master.

Need reviews

	Alex Vincent [:WeirdAl]
	Comment 93 • 10 years ago

There's no way this will land on 31 ESR.  That's a stable branch primarily for security fixes; they are definitely not taking new features.

	Yonggang Luo
	Comment 94 • 10 years ago

(In reply to Alex Vincent [:WeirdAl] from comment #93)
> There's no way this will land on 31 ESR.  That's a stable branch primarily
> for security fixes; they are definitely not taking new features.

I am not intent to land on 31 ESR. but base 31 ESR to implement this feature, can you do a review for me, after that, I will cherry-pick it to master

	Sylvestre Ledru [:Sylvestre]
	Comment 95 • 10 years ago

Comment on attachment 8472801 [details] [diff] [review]
multiline-tree.patch

Yonggang, if you want someone to review your patch, please use review and select a developer. approval-mozilla-esr31 is to get a patch cherry-picked from m-c to esr31

You should develop directly on mozilla-central:
* 31 code can be very different from the one in m-c (34)
* it will be harder to the reviewer
* it is not the correct way to write patch

	Yonggang Luo
	Comment 96 • 10 years ago

(In reply to Sylvestre Ledru [:sylvestre] from comment #95)
> Comment on attachment 8472801 [details] [diff] [review]
> multiline-tree.patch
> 
> Yonggang, if you want someone to review your patch, please use review and
> select a developer. approval-mozilla-esr31 is to get a patch cherry-picked
> from m-c to esr31
> 
> You should develop directly on mozilla-central:
> * 31 code can be very different from the one in m-c (34)
> * it will be harder to the reviewer
> * it is not the correct way to write patch
I know what's your concern, I want to apply this patch to my own application that use XUL runner, so I need some stable version of mozilla, at the current time mozilla 31 is a relative new and stable version, so I won't miss the opportunity to merge this patch to mainline, at the current time, I need someone to review my patch, and teach me how to make it product quality, such as writing testcase for it and minor fixes. After the functional and tests are ready, then I will submit the last patch that rebased on mainline, Thanks for your detail explain.
I want to specify  ` Paul Rouget` as the reviewer, because he is the original patch creator, but I don't know how.

	Jonathan Protzenko [:protz]
	Comment 97 • 10 years ago

Yonggang, if you need help understanding the (sometimes scary !) bugzilla, there's a nice video at http://blog.johnath.com/2010/02/04/bugzilla-for-humans/

Cheers,

~ jonathan

	Paul Rouget [:paul] Reporter
	Comment 98 • 10 years ago

(In reply to Yonggang Luo from comment #96)
> I want to specify  ` Paul Rouget` as the reviewer, because he is the
> original patch creator, but I don't know how.

I can't review this patch (I'm not a peer of this module).

But first, you need to rebase the patch on top of central (not 31), and make sure text properly wraps (iirc, that's why this patch never got finished).

	Yonggang Luo
	Comment 99 • 10 years ago

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #98)
> (In reply to Yonggang Luo from comment #96)
> > I want to specify  ` Paul Rouget` as the reviewer, because he is the
> > original patch creator, but I don't know how.
> 
> I can't review this patch (I'm not a peer of this module).
> 
> But first, you need to rebase the patch on top of central (not 31), and make
> sure text properly wraps (iirc, that's why this patch never got finished).
Text properly wraps? I would like you explain it a bit more.
Rebase on top of central is OK,

	Paul Rouget [:paul] Reporter
	Comment 100 • 10 years ago

See comment 45 and comment 46.

	Yonggang Luo
	Comment 101 • 10 years ago

About RichTree, I have two question,
1. How to setting different row with different height? I means by using callback like getRowProperties, we turn out have something like  getRowHeight()
2. How to turn a cell draw to be a canvas, so it can be draw outside, for this, 
  for example
   drawCell(view, row,column, mouseStatus, keybordStatus)
   The view means TreeView, 
   mouseStatus for the customize draw to respond mouse event such as onClick, onHover, onFocus, onUnfocus for the specific cell.

If we have these two hook, we can draw anything we want.

	Yonggang Luo
	Comment 102 • 10 years ago

(In reply to Yonggang Luo from comment #101)
> About RichTree, I have two question,
> 1. How to setting different row with different height? I means by using
> callback like getRowProperties, we turn out have something like 
> getRowHeight()
For this, we can through css to specify different row height for different rows have different properties.

> 2. How to turn a cell draw to be a canvas, so it can be draw outside, for
> this, 
>   for example
>    drawCell(view, row,column, mouseStatus, keybordStatus)
>    The view means TreeView, 
>    mouseStatus for the customize draw to respond mouse event such as
> onClick, onHover, onFocus, onUnfocus for the specific cell.
> 
> If we have these two hook, we can draw anything we want.

The canvas is the only thing we need to provide.

	Paul Rouget [:paul] Reporter
	Comment 103 • 10 years ago

(In reply to Yonggang Luo from comment #101)
> About RichTree, I have two question,
> 1. How to setting different row with different height? I means by using
> callback like getRowProperties, we turn out have something like 
> getRowHeight()

This is not possible. A tree is designed to be able to hold a large amount of items. And to do so, one of the requirement is for all the items to have the same geometry.

	Yonggang Luo
	Comment 104 • 10 years ago

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #103)
> (In reply to Yonggang Luo from comment #101)
> > About RichTree, I have two question,
> > 1. How to setting different row with different height? I means by using
> > callback like getRowProperties, we turn out have something like 
> > getRowHeight()
> 
> This is not possible. A tree is designed to be able to hold a large amount
Yes, I agreed,
> of items. And to do so, one of the requirement is for all the items to have
> the same geometry.
Agreed too,

Now my solution works fine, I write the following function to rendering the image background, and works fine for me:

  //AString render(in long row, in nsITreeColumn col, in long x, in long y, in long width, in long height, in nsIMutableArray props);
  render: function(row, col, x, y, width, height, props) {
    x = x / 60;
    y = y / 60;
    width = width / 60;
    height = height / 60;
    if (!cellRenderCanvas)
    {
      cellRenderCanvas = window.document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
    }
    cellRenderCanvas.width  = width;
    cellRenderCanvas.height = height;
    var ctx = cellRenderCanvas.getContext("2d");
    ctx.clearRect(0, 0, cellRenderCanvas.width, cellRenderCanvas.height);

    var bubbleTextStyle = "bold 11px Segoe UI";
    ctx.font = bubbleTextStyle;
    var txt = "Hello " + row.toString() + " 123 World!" + row.toString()
    ctx.fillText(txt,0,0);
    ctx.fillText(txt,0,30);
    return cellRenderCanvas.toDataURL("image/png");
  }


The only problem is that I am using  cellRenderCanvas.toDataURL("image/png");
to passing the render result as a image and passed to PaintCell to PaintImage


  mIsRichCell = aColumn->GetType() == nsITreeColumn::TYPE_RICH || mScratchArray.Contains(nsGkAtoms::rich);
  if (mIsRichCell) {
    nsCOMPtr<nsIMutableArray> scratchArray = AtomArrayToMutableArray(mScratchArray);
    nsCOMPtr<nsITreeCellCanvas> cellCanvas;
    GetCellCanvas(getter_AddRefs(cellCanvas));
    mRichCellImageSrc = EmptyString();
    if (cellCanvas)
    {
      cellCanvas->Render(aRowIndex, aColumn, cellRect.x, cellRect.y, cellRect.width, cellRect.height, scratchArray, mRichCellImageSrc);
    }
    PaintImage(aRowIndex, aColumn, cellRect, aPresContext, aRenderingContext, nsCSSAnonBoxes::moztreeimage, aDirtyRect, remainingWidth, currX);
    aCurrX = currX;
    return;
  }


So I am looking for faster way to handling this, I means no need the image wrapper.

and the patch is truely simple, also, I need some suggestion about the event handling, because I want to implement buttons in canvas to respond to mouse events.

	Yonggang Luo
	Comment 105 • 10 years ago

Now I am happy to announce this issue has a big progress,
```

22035320a5ec86fa7639836cd41a88278bdc4c0d
 content/base/src/nsGkAtomList.h      |  1 +
 layout/xul/tree/nsITreeBoxObject.idl | 18 ++++++++++
 layout/xul/tree/nsITreeColumns.idl   |  1 +
 layout/xul/tree/nsTreeBodyFrame.cpp  | 66 ++++++++++++++++++++++++++++++++++--
 layout/xul/tree/nsTreeBodyFrame.h    |  5 +++
 layout/xul/tree/nsTreeBoxObject.cpp  | 19 +++++++++++
 layout/xul/tree/nsTreeColumns.cpp    |  3 +-
 7 files changed, 109 insertions(+), 4 deletions(-)

diff --git a/content/base/src/nsGkAtomList.h b/content/base/src/nsGkAtomList.h
index 3b633f5..d8523f3 100644
--- a/content/base/src/nsGkAtomList.h
+++ b/content/base/src/nsGkAtomList.h
@@ -941,6 +941,7 @@ GK_ATOM(resultPrefix, "result-prefix")
 GK_ATOM(rev, "rev")
 GK_ATOM(reverse, "reverse")
 GK_ATOM(reversed, "reversed")
+GK_ATOM(rich, "rich")
 GK_ATOM(richlistbox, "richlistbox")
 GK_ATOM(richlistitem, "richlistitem")
 GK_ATOM(right, "right")
diff --git a/layout/xul/tree/nsITreeBoxObject.idl b/layout/xul/tree/nsITreeBoxObject.idl
index 7832c38..70fc0cc 100644
--- a/layout/xul/tree/nsITreeBoxObject.idl
+++ b/layout/xul/tree/nsITreeBoxObject.idl
@@ -4,14 +4,26 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "nsISupports.idl"
+#include "nsIDOMEventListener.idl"
 
 interface nsIDOMElement;
 interface nsITreeView;
 interface nsITreeSelection;
 interface nsITreeColumn;
 interface nsITreeColumns;
+interface nsIMutableArray;
 interface nsIScriptableRegion;
 
+[scriptable, uuid(ACEED5F8-E57A-4CDC-B859-CF864904AB1C)]
+interface nsITreeCellCanvas : nsIDOMEventListener
+{
+  /**
+  * To rendering a TreeCell as a canvas, we return the rendering result
+  * as a image, that can be used by PaintImage
+  */
+  AString render(in nsITreeView view, in long row, in nsITreeColumn col, in long x, in long y, in long width, in long height, in nsIMutableArray props);
+};
+
 [scriptable, uuid(64BA5199-C4F4-4498-BBDC-F8E4C369086C)]
 interface nsITreeBoxObject : nsISupports
 {
@@ -28,6 +40,12 @@ interface nsITreeBoxObject : nsISupports
   attribute nsITreeView view;
 
   /**
+  * The canvas handler for rich cell, to rendering anything that canvs
+  * support, we can use this to rendering beatiful tree cell.
+  */
+  attribute nsITreeCellCanvas cellCanvas;
+
+  /**
    * Whether or not we are currently focused.
    */
   attribute boolean focused;
diff --git a/layout/xul/tree/nsITreeColumns.idl b/layout/xul/tree/nsITreeColumns.idl
index 9b27ce2..cc7f159 100644
--- a/layout/xul/tree/nsITreeColumns.idl
+++ b/layout/xul/tree/nsITreeColumns.idl
@@ -32,6 +32,7 @@ interface nsITreeColumn : nsISupports
   const short TYPE_TEXT                = 1;
   const short TYPE_CHECKBOX            = 2;
   const short TYPE_PROGRESSMETER       = 3;
+  const short TYPE_RICH                = 4;
   readonly attribute short type;
 
   nsITreeColumn getNext();
diff --git a/layout/xul/tree/nsTreeBodyFrame.cpp b/layout/xul/tree/nsTreeBodyFrame.cpp
index 7f33ed2..add6fad 100644
--- a/layout/xul/tree/nsTreeBodyFrame.cpp
+++ b/layout/xul/tree/nsTreeBodyFrame.cpp
@@ -61,6 +61,7 @@
 #include "nsIScriptableRegion.h"
 #include <algorithm>
 #include "ScrollbarActivity.h"
+#include <nsIMutableArray.h>
 
 #ifdef ACCESSIBILITY
 #include "nsAccessibilityService.h"
@@ -88,6 +89,16 @@ CancelImageRequest(const nsAString& aKey,
   return PL_DHASH_NEXT;
 }
 
+static nsCOMPtr<nsIMutableArray> AtomArrayToMutableArray(AtomArray &atomArray)
+{
+  nsCOMPtr<nsIMutableArray> scratchArray = do_CreateInstance(NS_ARRAY_CONTRACTID);
+  NS_ENSURE_TRUE(scratchArray, nullptr);
+  for (uint32_t i = 0; i < atomArray.Length(); ++i) {
+    scratchArray->AppendElement(atomArray[i], true);
+  }
+  return scratchArray;
+}
+
 //
 // NS_NewTreeFrame
 //
@@ -129,7 +140,8 @@ nsTreeBodyFrame::nsTreeBodyFrame(nsIPresShell* aPresShell, nsStyleContext* aCont
  mVerticalOverflow(false),
  mHorizontalOverflow(false),
  mReflowCallbackPosted(false),
- mCheckingOverflow(false)
+ mCheckingOverflow(false),
+ mIsRichCell(false)
 {
   mColumns = new nsTreeColumns(this);
 }
@@ -2109,7 +2121,13 @@ nsTreeBodyFrame::GetImage(int32_t aRowIndex, nsTreeColumn* aCol, bool aUseContex
   *aResult = nullptr;
 
   nsAutoString imageSrc;
-  mView->GetImageSrc(aRowIndex, aCol, imageSrc);
+  if (mIsRichCell) {
+    imageSrc = mRichCellImageSrc;
+    if (!imageSrc.IsEmpty())
+      aUseContext = false;
+  } else {
+    mView->GetImageSrc(aRowIndex, aCol, imageSrc);
+  }
   nsRefPtr<imgRequestProxy> styleRequest;
   if (!aUseContext && !imageSrc.IsEmpty()) {
     aAllowImageRegions = false;
@@ -3268,7 +3286,11 @@ nsTreeBodyFrame::PaintCell(int32_t              aRowIndex,
     nsRect elementRect(currX, cellRect.y, remainingWidth, cellRect.height);
     nsRect dirtyRect;
     if (dirtyRect.IntersectRect(aDirtyRect, elementRect)) {
-      switch (aColumn->GetType()) {
+      int16_t cellType = aColumn->GetType();
+      if (mScratchArray.Contains(nsGkAtoms::rich)) {
+        cellType = nsITreeColumn::TYPE_RICH;
+      }
+      switch (cellType) {
         case nsITreeColumn::TYPE_TEXT:
           PaintText(aRowIndex, aColumn, elementRect, aPresContext, aRenderingContext, aDirtyRect, currX);
           break;
@@ -3289,6 +3311,26 @@ nsTreeBodyFrame::PaintCell(int32_t              aRowIndex,
               break;
           }
           break;
+        case nsITreeColumn::TYPE_RICH:
+          nsCOMPtr<nsITreeCellCanvas> cellCanvas;
+          GetCellCanvas(getter_AddRefs(cellCanvas));
+          if (cellCanvas) {
+            nsCOMPtr<nsIMutableArray> scratchArray = AtomArrayToMutableArray(mScratchArray);
+            mRichCellImageSrc = EmptyString();
+            cellCanvas->Render(mView,
+                aRowIndex,
+                aColumn,
+                elementRect.x,
+                elementRect.y,
+                elementRect.width,
+                elementRect.height,
+                scratchArray,
+                mRichCellImageSrc);
+            mIsRichCell = true;
+            PaintImage(aRowIndex, aColumn, elementRect, aPresContext, aRenderingContext, aDirtyRect, remainingWidth, currX);
+            mIsRichCell = false;
+          }
+          break;
       }
     }
   }
@@ -4685,3 +4727,21 @@ nsTreeBodyFrame::OnImageIsAnimated(imgIRequest* aRequest)
 
   return NS_OK;
 }
+
+nsresult
+nsTreeBodyFrame::GetCellCanvas(nsITreeCellCanvas ** aCellCanvas)
+{
+  *aCellCanvas = nullptr;
+  nsWeakFrame weakFrame(this);
+  NS_ENSURE_STATE(weakFrame.IsAlive());
+  NS_IF_ADDREF(*aCellCanvas = mCellCanvas);
+  return NS_OK;
+}
+
+nsresult
+nsTreeBodyFrame::SetCellCanvas(nsITreeCellCanvas * aCellCanvas)
+{
+  NS_ENSURE_ARG_POINTER(aCellCanvas);
+  mCellCanvas = aCellCanvas;
+  return NS_OK;
+}
diff --git a/layout/xul/tree/nsTreeBodyFrame.h b/layout/xul/tree/nsTreeBodyFrame.h
index fb1f8b7..4bd7b4e 100644
--- a/layout/xul/tree/nsTreeBodyFrame.h
+++ b/layout/xul/tree/nsTreeBodyFrame.h
@@ -119,6 +119,8 @@ public:
   nsresult BeginUpdateBatch();
   nsresult EndUpdateBatch();
   nsresult ClearStyleAndImageCaches();
+  nsresult GetCellCanvas(nsITreeCellCanvas ** aTwistyImageGenerator);
+  nsresult SetCellCanvas(nsITreeCellCanvas * aTwistyImageGenerator);
 
   void ManageReflowCallback(const nsRect& aRect, nscoord aHorzWidth);
 
@@ -629,6 +631,9 @@ protected: // Data Members
   // have pointers to us.
   nsTHashtable<nsPtrHashKey<nsTreeImageListener> > mCreatedListeners;
 
+  nsCOMPtr<nsITreeCellCanvas> mCellCanvas;
+  bool mIsRichCell;
+  nsAutoString mRichCellImageSrc;
 }; // class nsTreeBodyFrame
 
 #endif
diff --git a/layout/xul/tree/nsTreeBoxObject.cpp b/layout/xul/tree/nsTreeBoxObject.cpp
index 6f61cba..af2aba5 100644
--- a/layout/xul/tree/nsTreeBoxObject.cpp
+++ b/layout/xul/tree/nsTreeBoxObject.cpp
@@ -490,3 +490,22 @@ NS_NewTreeBoxObject(nsIBoxObject** aResult)
   return NS_OK;
 }
 
+NS_IMETHODIMP
+nsTreeBoxObject::GetCellCanvas(nsITreeCellCanvas ** aCellCanvas)
+{
+  nsTreeBodyFrame* body = GetTreeBody();
+  if (body)
+    return body->GetCellCanvas(aCellCanvas);
+  else
+    *aCellCanvas = nullptr;
+  return NS_OK;
+}
+
+NS_IMETHODIMP
+nsTreeBoxObject::SetCellCanvas(nsITreeCellCanvas * aCellCanvas)
+{
+  nsTreeBodyFrame* body = GetTreeBody();
+  if (body)
+    return body->SetCellCanvas(aCellCanvas);
+  return NS_OK;
+}
diff --git a/layout/xul/tree/nsTreeColumns.cpp b/layout/xul/tree/nsTreeColumns.cpp
index 945da14..4f46fa6 100644
--- a/layout/xul/tree/nsTreeColumns.cpp
+++ b/layout/xul/tree/nsTreeColumns.cpp
@@ -319,11 +319,12 @@ nsTreeColumn::Invalidate()
   // Figure out our column type. Default type is text.
   mType = nsITreeColumn::TYPE_TEXT;
   static nsIContent::AttrValuesArray typestrings[] =
-    {&nsGkAtoms::checkbox, &nsGkAtoms::progressmeter, nullptr};
+    {&nsGkAtoms::checkbox, &nsGkAtoms::progressmeter, &nsGkAtoms::rich, nullptr};
   switch (mContent->FindAttrValueIn(kNameSpaceID_None, nsGkAtoms::type,
                                     typestrings, eCaseMatters)) {
     case 0: mType = nsITreeColumn::TYPE_CHECKBOX; break;
     case 1: mType = nsITreeColumn::TYPE_PROGRESSMETER; break;
+    case 2: mType = nsITreeColumn::TYPE_RICH; break;
   }
 
   // Fetch the crop style.

```

	Yonggang Luo
	Comment 106 • 10 years ago

Now it can be rendering with canvas in a proper and simple way.

This is a tree demo

	Yonggang Luo
	Comment 107 • 10 years ago

This is a simple patch to add rich tree support, only add a rich atom to original tree layout

	Yonggang Luo
	Comment 108 • 10 years ago

For testing purpose.
It's have canvas rendering code in the test.js file
and it support for 1000,000 entries without performance loss.

	Paul Rouget [:paul] Reporter
	Comment 109 • 10 years ago

Comment on attachment 8474219 [details] [diff] [review]
richTree.patch

(In reply to Yonggang Luo from comment #107)
> Created attachment 8474219 [details] [diff] [review]
> richTree.patch
> 
> This is a simple patch to add rich tree support, only add a rich atom to
> original tree layout

Again, I can't review this patch. I'm not a peer of this module.

Also, you totally changed what the original patch was doing, and if you delegate drawing to canvas, what's the point of using a xul:tree? File a new bug if you still want to use a canvas with a xul:tree.

If you want to fix this bug, improve the current patch, and address comment 45 and comment 46.

	Yonggang Luo
	Comment 110 • 10 years ago

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #109)
> Comment on attachment 8474219 [details] [diff] [review]
> richTree.patch
> 
> (In reply to Yonggang Luo from comment #107)
> > Created attachment 8474219 [details] [diff] [review]
> > richTree.patch
> > 
> > This is a simple patch to add rich tree support, only add a rich atom to
> > original tree layout
> 
> Again, I can't review this patch. I'm not a peer of this module.
So, tell me, who is the peer of this module, I will inform he/she.
> 
> Also, you totally changed what the original patch was doing, and if you
> delegate drawing to canvas, what's the point of using a xul:tree? File a new
> bug if you still want to use a canvas with a xul:tree.
I am not following the original patch, I am creating a new way to solve this issue, and works fine.
Please beware of the bug title, `Treerows need a way to hold richer content`, that's the exactly my new patch solved this issue, hold "Richer content", canvas is contains much more content thatn
'multiline'. 

> 
> If you want to fix this bug, improve the current patch, and address comment
> 45 and comment 46.

	Seth Fowler [:seth] [:s2h]
	Comment 111 • 10 years ago

I can't comment about the use of canvas to solve this bug, but if you continue to use the approach of creating an imgIContainer from canvas data, please request review from me before checking this in.

	Yonggang Luo
	Comment 112 • 10 years ago

Add mozFetchStream API in HTML Canvas interface for retrieve the
canvas content as png stream synchronously.

	Yonggang Luo
	Comment 113 • 10 years ago

Add rich tree support by rendering a treecell as a canvas,
and that canvas can be rendered in the Javascript, so in theoretically,
It's will as rich as canvas can do.

Text wrapping is also can be handled properly in canvas, that's won't be
a problem.

	Yonggang Luo
	Comment 114 • 10 years ago

Minor changes.

	Seth Fowler [:seth] [:s2h]
	Comment 115 • 10 years ago

Comment on attachment 8477256 [details] [diff] [review]
0001-Add-API-mozFetchStream-to-synchronously-fetch-the-ns.patch

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

This isn't the way to go. The method you're using the get the content of the canvas is also extremely expensive, since you're encoding it to PNG and then immediately decoding it again. You also don't need to (and shouldn't) add any methods to nsIDOMHTMLCanvasElement; stick to internal, C++-only APIs for this.

If the best way to resolve this bug is to create an image from a canvas element (and I'm not reviewing that; you need another reviewer familiar with XUL) then what you need to do is:

(1) Call CanvasRenderingcontext2D::GetSurfaceSnapshot to get a SourceSurface.
(2) Construct a gfxSurfaceDrawable using that SourceSurface.
(3) Use ImageOps::CreateFromDrawable to create an imgIContainer from the gfxSurfaceDrawable.

	Seth Fowler [:seth] [:s2h]
	Comment 116 • 10 years ago

Comment on attachment 8477440 [details] [diff] [review]
0002-Add-rich-tree-support-draw-the-canvas-in-treeceel-in.patch

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

r-'ing this patch as well; see the previous review for why.

	Yonggang Luo
	Comment 117 • 10 years ago

(In reply to Seth Fowler [:seth] from comment #115)
> Comment on attachment 8477256 [details] [diff] [review]
> 0001-Add-API-mozFetchStream-to-synchronously-fetch-the-ns.patch
> 
> Review of attachment 8477256 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This isn't the way to go. The method you're using the get the content of the
> canvas is also extremely expensive, since you're encoding it to PNG and then
> immediately decoding it again. You also don't need to (and shouldn't) add
> any methods to nsIDOMHTMLCanvasElement; stick to internal, C++-only APIs for
> this.
Thanks a lot, I have no intention add a API to nsIDOMHTMLCanvasElement, it's just because 
this is the only I figure out.  

> 
> If the best way to resolve this bug is to create an image from a canvas
> element (and I'm not reviewing that; you need another reviewer familiar with
> XUL) then what you need to do is:
> 
> (1) Call CanvasRenderingcontext2D::GetSurfaceSnapshot to get a SourceSurface.
> (2) Construct a gfxSurfaceDrawable using that SourceSurface.
> (3) Use ImageOps::CreateFromDrawable to create an imgIContainer from the
> gfxSurfaceDrawable.

Thanks a lot, I also knows that image encoding and decoding is costing and not necessary, I will have a try about your solution.

	Yonggang Luo
	Comment 118 • 10 years ago

(In reply to Yonggang Luo from comment #117)
> (In reply to Seth Fowler [:seth] from comment #115)
> > Comment on attachment 8477256 [details] [diff] [review]
> > 0001-Add-API-mozFetchStream-to-synchronously-fetch-the-ns.patch
> > 
> > Review of attachment 8477256 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This isn't the way to go. The method you're using the get the content of the
> > canvas is also extremely expensive, since you're encoding it to PNG and then
> > immediately decoding it again. You also don't need to (and shouldn't) add
> > any methods to nsIDOMHTMLCanvasElement; stick to internal, C++-only APIs for
> > this.
> Thanks a lot, I have no intention add a API to nsIDOMHTMLCanvasElement, it's
> just because 
> this is the only I figure out.  
> 
> > 
> > If the best way to resolve this bug is to create an image from a canvas
> > element (and I'm not reviewing that; you need another reviewer familiar with
> > XUL) then what you need to do is:
> > 
> > (1) Call CanvasRenderingcontext2D::GetSurfaceSnapshot to get a SourceSurface.
> > (2) Construct a gfxSurfaceDrawable using that SourceSurface.
> > (3) Use ImageOps::CreateFromDrawable to create an imgIContainer from the
> > gfxSurfaceDrawable.

There is no such a API ImageOps::CreateFromDrawable, I am getting confused.
> 
> Thanks a lot, I also knows that image encoding and decoding is costing and
> not necessary, I will have a try about your solution.

	Yonggang Luo
	Comment 119 • 10 years ago

Updated version doesn't need the image encode decode.

	Seth Fowler [:seth] [:s2h]
	Comment 120 • 10 years ago

Comment on attachment 8477845 [details] [diff] [review]
001-Add-rich-treecell-functional-in-tree-element-by-rend.patch

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

This patch is a good start! It still has a way to go, but with some work you can get it ready to land. Here are the most important things you need to do:

1. Pull an up-to-date copy of mozilla-central, and switch to using ImageOps::CreateFromDrawable.
2. Fix the issues I point out below. You should do this before requesting review from anyone else, because without the style issues the patch will be much easier to review.
3. Find someone who's familiar with the XUL layout code to review this patch. (You can find a list of the peers at https://wiki.mozilla.org/Modules/All)
4. Once you get an r+ from them, ask me for a review again.

::: layout/xul/tree/nsITreeBoxObject.idl
@@ +20,5 @@
> +  /**
> +  * To rendering a TreeCell as a canvas, returning the canvas that
> +  * rendered outside.
> +  */
> +  nsISupports render(in nsITreeView view, in long row, in nsITreeColumn col, in nsIMutableArray props);

Why return an nsISupports? I'm not sure why this should return anything, in fact. If you need to get the canvas object this contains, you should probably have a separate getter or attribute that uses a canvas interface and not just nsISupports.

@@ +42,5 @@
>    /**
> +  * The canvas handler for rich cell, to rendering anything that canvs
> +  * support, we can use this to rendering beatiful tree cell.
> +  */
> +  attribute nsITreeCellCanvas cellCanvas;

I don't really deal with XUL at all (which is why you really need to get someone who does to review this patch), so this may be off base, but: why add this attribute? There isn't an attribute for the image case.

::: layout/xul/tree/nsTreeBodyFrame.cpp
@@ +91,4 @@
>    return PL_DHASH_NEXT;
>  }
>  
> +static nsCOMPtr<nsIMutableArray> AtomArrayToMutableArray(AtomArray &atomArray)

Nit: Mozilla style is to place the return type on a separate line. You should also start arguments' names with 'a' and group the '&' with the type. So write:

static nsCOMPtr<nsIMutableArray>
AtomArrayToMutableArray(AtomArray& aAtomArray)

For more information on Mozilla's recommended coding style, please read this page:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

@@ +2320,4 @@
>  nsSize
>  nsTreeBodyFrame::GetImageDestSize(nsStyleContext* aStyleContext,
>                                    bool useImageRegion,
> +                                  nsIntSize* sz)

Nit: Mozilla style is to start an argument's name with 'a', and spelling things out is preferred. Use |nsIntSize* aSize|.

@@ +2361,4 @@
>        // Use the width of the image region.
>        imageSize.width = myList->mImageRegion.width;
>      }
> +    else if (sz) {

Nit: place the |else if| on the same line as the |}|. There are some other cases in this patch, too.

@@ +2412,4 @@
>  nsRect
>  nsTreeBodyFrame::GetImageSourceRect(nsStyleContext* aStyleContext,
>                                      bool useImageRegion,
> +                                    nsIntSize* sz)

aSize here too.

@@ +3263,5 @@
>    nsRect iconRect(currX, cellRect.y, remainingWidth, cellRect.height);
>    nsRect dirtyRect;
>    if (dirtyRect.IntersectRect(aDirtyRect, iconRect))
>      PaintImage(aRowIndex, aColumn, iconRect, aPresContext, aRenderingContext, aDirtyRect,
> +               remainingWidth, currX, false);

To the reader, when you have a boolean like "false", it's not obvious what it does. You don't always have to do it, but I personally prefer to make it clear what the boolean means like this:

             remainingWidth, currX, /* aIsCanvas = */ false);

Consider doing this for both calls to PaintImage.

@@ +3410,5 @@
>                              nsRenderingContext& aRenderingContext,
>                              const nsRect&        aDirtyRect,
>                              nscoord&             aRemainingWidth,
> +                            nscoord&             aCurrX,
> +                            bool                 isCanvas)

Nit: please use |aIsCanvas|.

@@ +3422,5 @@
> +  if (isCanvas) {
> +    imageContext = GetPseudoStyleContext(nsCSSAnonBoxes::moztreecanvas);
> +  } else {
> +    imageContext = GetPseudoStyleContext(nsCSSAnonBoxes::moztreeimage);
> +  }

This whole chunk of code can be more concisely written as:

nsStyleContext* imageContext =
  GetPseudoStyleContext(isCanvas ? nsCSSAnonBoxes::moztreecanvas : nsCSSAnonBoxes::moztreeimage);

@@ +3439,5 @@
>    bool useImageRegion = true;
>    nsCOMPtr<imgIContainer> image;
> +  RefPtr<mozilla::gfx::SourceSurface> surface;
> +  nsIntSize contentSize(0, 0);
> +  nsIntSize *contentSizePtr = NULL;

Nit: it's Mozilla style to group the '*' with the type, and to use 'nullptr' instead of 'NULL', so write |nsIntSize* contentSizePtr = nullptr;|.

However, you don't need contentSizePtr at all, because |&contentSize| will work just as well in any context.

@@ +3447,5 @@
> +    if (image) {
> +      int32_t width;
> +      int32_t height;
> +      image->GetWidth(&width);
> +      image->GetHeight(&height);

You can just write |image->GetWidth(&contentSize.width);| and similar for height. You don't need the intermediate variables.

@@ +3462,5 @@
> +      cellCanvas->Render(mView,
> +        aRowIndex,
> +        aColumn,
> +        scratchArray,
> +        getter_AddRefs(canvasCtx));

Nit: You should put multiple arguments on a line so that this call to Render takes up fewer lines.

@@ +3467,5 @@
> +    }
> +
> +    nsICanvasRenderingContextInternal *ctxInternal = NULL;
> +    if (canvasCtx) {
> +      canvasCtx->QueryInterface(NS_GET_IID(nsICanvasRenderingContextInternal), (void**)&ctxInternal);

You should never call QueryInterface manually; always use helper functions like do_QueryInterface. Also, you should store the result in an nsCOMPtr or you'll leak memory. You can write the above as:

nsCOMPtr<nsICanvasRenderingContextInternal> ctxInternal = do_QueryInterface(canvasCtx);

You don't even need to check for null.

For more information, see here:

https://developer.mozilla.org/en-US/docs/Using_nsCOMPtr/Reference_Manual

@@ +3583,5 @@
> +          imgIContainer::FLAG_NONE);
> +    } else {
> +      nsRefPtr<gfxDrawable> drawable =
> +          new gfxSurfaceDrawable(surface, ThebesIntSize(surface->GetSize()));
> +      nsLayoutUtils::DrawPixelSnapped(&aRenderingContext,

You should pull, because your code is out of date. nsLayoutUtils::DrawPixelSnapped no longer exists. That also explains why you don't see ImageOps::CreateFromDrawable, which *does* exist and is what you should use instead. Once you're using ImageOps::CreateFromDrawable, you won't need two separate code paths either, because you'll have an imgIContainer in either case.

@@ +4753,5 @@
>    return NS_OK;
>  }
> +
> +nsresult
> +nsTreeBodyFrame::GetCellCanvas(nsITreeCellCanvas ** aCellCanvas)

Nit: Group the "**" with the type. Similar for SetCellCanvas below.

@@ +4757,5 @@
> +nsTreeBodyFrame::GetCellCanvas(nsITreeCellCanvas ** aCellCanvas)
> +{
> +  *aCellCanvas = nullptr;
> +  nsWeakFrame weakFrame(this);
> +  NS_ENSURE_STATE(weakFrame.IsAlive());

Why are you creating an nsWeakFrame here at all?

@@ +4758,5 @@
> +{
> +  *aCellCanvas = nullptr;
> +  nsWeakFrame weakFrame(this);
> +  NS_ENSURE_STATE(weakFrame.IsAlive());
> +  NS_IF_ADDREF(*aCellCanvas = mCellCanvas);

Don't use NS_IF_ADDREF. The proper pattern here is:

nsCOMPtr<nsITreeCellCanvas> canvas = mCellCanvas;
canvas.forget(aCellCanvas);

::: layout/xul/tree/nsTreeBoxObject.cpp
@@ +493,5 @@
> +NS_IMETHODIMP
> +nsTreeBoxObject::GetCellCanvas(nsITreeCellCanvas ** aCellCanvas)
> +{
> +  nsTreeBodyFrame* body = GetTreeBody();
> +  if (body)

Nit: Always use braces around 'if' and 'else' bodies, even if they're only one line.

	Alex Vincent [:WeirdAl]
	Comment 121 • 10 years ago

Comment on attachment 8477845 [details] [diff] [review]
001-Add-rich-treecell-functional-in-tree-element-by-rend.patch

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

I have some serious issues as well, from the standpoint of someone who crafts trees in XUL a fair bit:

::: layout/xul/tree/nsITreeBoxObject.idl
@@ +10,5 @@
>  interface nsITreeView;
>  interface nsITreeSelection;
>  interface nsITreeColumn;
>  interface nsITreeColumns;
> +interface nsIMutableArray;

Please, don't use an nsIMutableArray of nsIAtom objects.  Use AString instead, as in https://hg.mozilla.org/releases/mozilla-release/diff/bf88a316cf18/layout/xul/tree/nsITreeView.idl .

@@ +27,1 @@
>  [scriptable, uuid(64BA5199-C4F4-4498-BBDC-F8E4C369086C)]

I'm pretty sure you have to rev the UUID for any interface where you add a new attribute or method.

@@ +42,5 @@
>    /**
> +  * The canvas handler for rich cell, to rendering anything that canvs
> +  * support, we can use this to rendering beatiful tree cell.
> +  */
> +  attribute nsITreeCellCanvas cellCanvas;

The nsITreeView interface is probably a better place to add this property, because it's the tree view that we're supposed to implement to provide the contents of the tree.  The tree box object is native.

Why does nsITreeCellCanvas inherit from nsIDOMEventListener?

If it doesn't have to, please consider adding a renderCanvas method instead to nsITreeView.

To answer Seth's question:  nsITreeView has a getImageSrc() method.

::: layout/xul/tree/nsITreeColumns.idl
@@ +32,4 @@
>    const short TYPE_TEXT                = 1;
>    const short TYPE_CHECKBOX            = 2;
>    const short TYPE_PROGRESSMETER       = 3;
> +  const short TYPE_CANVAS              = 4;

I don't remember whether this is enough to require a uuid bump or not.

::: layout/xul/tree/nsTreeBodyFrame.cpp
@@ +99,5 @@
> +    scratchArray->AppendElement(atomArray[i], false);
> +  }
> +  return scratchArray;
> +}
> +

Again, a mutable array of nsIAtoms is less desirable.  See 
nsTreeUtils::TokenizeProperties.

::: layout/xul/tree/nsTreeBoxObject.cpp
@@ +495,5 @@
> +{
> +  nsTreeBodyFrame* body = GetTreeBody();
> +  if (body)
> +    return body->GetCellCanvas(aCellCanvas);
> +  else

else after return is considered evil.

	Colby Russell :crussell (no longer Mozilla-ing)
	Comment 122 • 10 years ago

nsITreeCellCanvas sounds like it should be an object that you could draw to, but really the canvas still needs to be created elsewhere and returned by this interface's |renderCanvas| method.  nsITreeCellRenderer instead?

(Side note: can you use "-U 8" for your next patch?  Three lines of context makes things kind of difficult to read.)

(In reply to Seth Fowler [:seth] from comment #120)
> why add this attribute? There isn't an attribute for the image case.

Could you be more specific about your question?  It seems like you're making an implicit suggestion here, but I'm not sure what it is.

(In reply to Alex Vincent [:WeirdAl] from comment #121)
> @@ +42,5 @@
> >    /**
> > +  * The canvas handler for rich cell, to rendering anything that canvs
> > +  * support, we can use this to rendering beatiful tree cell.
> > +  */
> > +  attribute nsITreeCellCanvas cellCanvas;
> 
> The nsITreeView interface is probably a better place to add this property,
> because it's the tree view that we're supposed to implement to provide the
> contents of the tree.  The tree box object is native.
> 
> Why does nsITreeCellCanvas inherit from nsIDOMEventListener?
> 
> If it doesn't have to, please consider adding a renderCanvas method instead
> to nsITreeView.

Requiring that every nsITreeView implementation have a |renderCanvas| method would be pretty terrible.  Consider the cascade of changes that this would entail for implementations that live in m-c.  Now consider the changes to all the implementations that live in extensions.  All of which are just going end up as this: yet another no-op method to satisfy the already cumbersome interface that is nsITreeView.

	Colby Russell :crussell (no longer Mozilla-ing)
	Comment 123 • 10 years ago

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #74)
> (In reply to Yonggang Luo from comment #73)
> > How did PostBox enhancements to TreeView?
> 
> Apparently, they used the patch attached to this bug.

Where are you seeing that?

	Alex Vincent [:WeirdAl]
	Comment 124 • 10 years ago

(In reply to Colby Russell :crussell from comment #122)
> Requiring that every nsITreeView implementation have a |renderCanvas| method
> would be pretty terrible.  Consider the cascade of changes that this would
> entail for implementations that live in m-c.  Now consider the changes to
> all the implementations that live in extensions.  All of which are just
> going end up as this: yet another no-op method to satisfy the already
> cumbersome interface that is nsITreeView.

Meh, I hadn't considered that impact.  However, a counterpoint:  only C++ implementations would *have* to change.  If the method isn't called, JS implementations don't care.  (Admittedly it'd be sloppy not to, but...)

Alternatively, we could define a nsITreeViewCanvas interface that inherits from nsITreeView, and then QI for that interface before calling the method.

	Colby Russell :crussell (no longer Mozilla-ing)
	Comment 125 • 10 years ago

(In reply to Alex Vincent [:WeirdAl] from comment #124)
> Alternatively, we could define a nsITreeViewCanvas interface that inherits
> from nsITreeView, and then QI for that interface before calling the method.

That's the right approach, but one I didn't mention because of how unwieldy the design for the tree views is currently; nsITreeView needs to be split up to progressively allow more advanced features for implementations that opt-in by supporting the extended interfaces.  I fear that starting off in that direction for this bug alone (and then necessarily quitting less than half way there) seems like it wouldn't actually make it that much easier to do if/when the time comes where someone decides to actually do it, and might even make it harder.  The whole thing really needs to have some good thought put into it and tackled in one go.

Given the way things are, setting the tree's |cellRenderer|[*] right before you set its |view| seems okay enough.  Your choice as the implementor whether you want the rvals in those assignments to be the same object or different ones; the |renderCanvas| call from the patch already gives you a parameter specifying the view it was called for.

* Not |cellCanvas|, please, for the reason above.

	Yonggang Luo
	Comment 126 • 10 years ago

(In reply to Colby Russell :crussell from comment #125)
> (In reply to Alex Vincent [:WeirdAl] from comment #124)
> > Alternatively, we could define a nsITreeViewCanvas interface that inherits
> > from nsITreeView, and then QI for that interface before calling the method.
> 
> That's the right approach, but one I didn't mention because of how unwieldy
> the design for the tree views is currently; nsITreeView needs to be split up
> to progressively allow more advanced features for implementations that
> opt-in by supporting the extended interfaces.  I fear that starting off in
> that direction for this bug alone (and then necessarily quitting less than
> half way there) seems like it wouldn't actually make it that much easier to
> do if/when the time comes where someone decides to actually do it, and might
> even make it harder.  The whole thing really needs to have some good thought
> put into it and tackled in one go.
Agreed, we may have a discuss about a better idea. I need more suggestion about how to design 
a better API to extend the tree cell render functional,
At the current time, we have TEXT, CHECKBOX, PROGRESSMETER, and now it's renderer, I am not so sure
that they have the same altitude, I treat the canvas as the same position like checkbox, progressmeter.

After that, is that nsIDOMEventListener need to be extend by the renderer? I need the canvas to response to external events.
> 
> Given the way things are, setting the tree's |cellRenderer|[*] right before
> you set its |view| seems okay enough.  Your choice as the implementor
> whether you want the rvals in those assignments to be the same object or
> different ones; the |renderCanvas| call from the patch already gives you a

> parameter specifying the view it was called for.
> 
> * Not |cellCanvas|, please, for the reason above.
I totally agreed.

	Yonggang Luo
	Comment 127 • 10 years ago

(In reply to Alex Vincent [:WeirdAl] from comment #121)
> Comment on attachment 8477845 [details] [diff] [review]
> 001-Add-rich-treecell-functional-in-tree-element-by-rend.patch
> 
> Review of attachment 8477845 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have some serious issues as well, from the standpoint of someone who
> crafts trees in XUL a fair bit:
> 
> ::: layout/xul/tree/nsITreeBoxObject.idl
> @@ +10,5 @@
> >  interface nsITreeView;
> >  interface nsITreeSelection;
> >  interface nsITreeColumn;
> >  interface nsITreeColumns;
> > +interface nsIMutableArray;
> 
> Please, don't use an nsIMutableArray of nsIAtom objects.  Use AString
> instead, as in
> https://hg.mozilla.org/releases/mozilla-release/diff/bf88a316cf18/layout/xul/
> tree/nsITreeView.idl .
> 
> @@ +27,1 @@
> >  [scriptable, uuid(64BA5199-C4F4-4498-BBDC-F8E4C369086C)]
> 
> I'm pretty sure you have to rev the UUID for any interface where you add a
> new attribute or method.
> 
> @@ +42,5 @@
> >    /**
> > +  * The canvas handler for rich cell, to rendering anything that canvs
> > +  * support, we can use this to rendering beatiful tree cell.
> > +  */
> > +  attribute nsITreeCellCanvas cellCanvas;
> 
> The nsITreeView interface is probably a better place to add this property,
> because it's the tree view that we're supposed to implement to provide the
> contents of the tree.  The tree box object is native.
> 
> Why does nsITreeCellCanvas inherit from nsIDOMEventListener?
I need the events for canvas, so canvas can draw the things sense to external events. 
> 
> If it doesn't have to, please consider adding a renderCanvas method instead
> to nsITreeView.
From my points of view, nsITreeView is a `dataProvider`, and nsITreeCellCanvas  is a `dataConsumer`,
The dataProvider are often to be stable, but the dataConsumer is vary time to time, so it's better not to be placed at the same place.

In other works. renderCanvas is like CSS, we can modify it frequently according to different environment.
> 
> To answer Seth's question:  nsITreeView has a getImageSrc() method.
Yeah, you are right about this, we have getImageSrc, the problem with getImageSrc is this method is returned the URL, that's the compressed data, that means we need to decompress latter, and that was time
consuming, Seth's now provider a way to rendering the canvas as the TreeCell, so that we doesn't need to 
compress/decompress the image that consuming time.
> 
> ::: layout/xul/tree/nsITreeColumns.idl
> @@ +32,4 @@
> >    const short TYPE_TEXT                = 1;
> >    const short TYPE_CHECKBOX            = 2;
> >    const short TYPE_PROGRESSMETER       = 3;
> > +  const short TYPE_CANVAS              = 4;
> 
> I don't remember whether this is enough to require a uuid bump or not.
Yeah, you are that, that's need a bump.
> 
> ::: layout/xul/tree/nsTreeBodyFrame.cpp
> @@ +99,5 @@
> > +    scratchArray->AppendElement(atomArray[i], false);
> > +  }
> > +  return scratchArray;
> > +}
> > +
> 
> Again, a mutable array of nsIAtoms is less desirable.  See 
> nsTreeUtils::TokenizeProperties.
> 
Thanks, hope this API would simplify my logic.
> ::: layout/xul/tree/nsTreeBoxObject.cpp
> @@ +495,5 @@
> > +{
> > +  nsTreeBodyFrame* body = GetTreeBody();
> > +  if (body)
> > +    return body->GetCellCanvas(aCellCanvas);
> > +  else
> 
> else after return is considered evil.
True: thanks.

	Yonggang Luo
	Comment 128 • 10 years ago

(In reply to Seth Fowler [:seth] from comment #120)
> Comment on attachment 8477845 [details] [diff] [review]
> 001-Add-rich-treecell-functional-in-tree-element-by-rend.patch
> 
> Review of attachment 8477845 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch is a good start! It still has a way to go, but with some work you
> can get it ready to land. Here are the most important things you need to do:
> 
> 1. Pull an up-to-date copy of mozilla-central, and switch to using
> ImageOps::CreateFromDrawable.
Thanks a lot, that's a totally new API, that's good:)
Maybe we need more direct API latter, so we can directly return a ImageContainer in the javascript code from canvas. Anyway that's fantastic, I am using thunderbird 31 because my product was based on a stable 
version of mozilla 31. now It's stable for my product, and I have the chance to getting the code up-to-date.
> 2. Fix the issues I point out below. You should do this before requesting
> review from anyone else, because without the style issues the patch will be
> much easier to review.
> 3. Find someone who's familiar with the XUL layout code to review this
> patch. (You can find a list of the peers at
> https://wiki.mozilla.org/Modules/All)
> 4. Once you get an r+ from them, ask me for a review again.
> 
> ::: layout/xul/tree/nsITreeBoxObject.idl
> @@ +20,5 @@
> > +  /**
> > +  * To rendering a TreeCell as a canvas, returning the canvas that
> > +  * rendered outside.
> > +  */
> > +  nsISupports render(in nsITreeView view, in long row, in nsITreeColumn col, in nsIMutableArray props);
> 
> Why return an nsISupports? I'm not sure why this should return anything, in
> fact. If you need to get the canvas object this contains, you should
> probably have a separate getter or attribute that uses a canvas interface
> and not just nsISupports.
> 
Indeed, I'd like to return ImageContainer directly, but we lack that API in Javascript, return the canvas
is also a good idea, but that means more C++ code-wrapping.


> @@ +42,5 @@
> >    /**
> > +  * The canvas handler for rich cell, to rendering anything that canvs
> > +  * support, we can use this to rendering beatiful tree cell.
> > +  */
> > +  attribute nsITreeCellCanvas cellCanvas;
> 
> I don't really deal with XUL at all (which is why you really need to get
> someone who does to review this patch), so this may be off base, but: why
> add this attribute? There isn't an attribute for the image case.
Yeap, we need a better design about this.
> 

> ::: layout/xul/tree/nsTreeBodyFrame.cpp
> @@ +91,4 @@
> >    return PL_DHASH_NEXT;
> >  }
> >  
> > +static nsCOMPtr<nsIMutableArray> AtomArrayToMutableArray(AtomArray &atomArray)
> 
> Nit: Mozilla style is to place the return type on a separate line. You
> should also start arguments' names with 'a' and group the '&' with the type.
Gocha
> So write:
> 
> static nsCOMPtr<nsIMutableArray>
> AtomArrayToMutableArray(AtomArray& aAtomArray)
> 
> For more information on Mozilla's recommended coding style, please read this
> page:
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
> 
> @@ +2320,4 @@
> >  nsSize
> >  nsTreeBodyFrame::GetImageDestSize(nsStyleContext* aStyleContext,
> >                                    bool useImageRegion,
> > +                                  nsIntSize* sz)
> 
> Nit: Mozilla style is to start an argument's name with 'a', and spelling
> things out is preferred. Use |nsIntSize* aSize|.
> 
ditto
> @@ +2361,4 @@
> >        // Use the width of the image region.
> >        imageSize.width = myList->mImageRegion.width;
> >      }
> > +    else if (sz) {
> 
> Nit: place the |else if| on the same line as the |}|. There are some other
> cases in this patch, too.
> 
ditto
> @@ +2412,4 @@
> >  nsRect
> >  nsTreeBodyFrame::GetImageSourceRect(nsStyleContext* aStyleContext,
> >                                      bool useImageRegion,
> > +                                    nsIntSize* sz)
> 
> aSize here too.
> 
ditto
> @@ +3263,5 @@
> >    nsRect iconRect(currX, cellRect.y, remainingWidth, cellRect.height);
> >    nsRect dirtyRect;
> >    if (dirtyRect.IntersectRect(aDirtyRect, iconRect))
> >      PaintImage(aRowIndex, aColumn, iconRect, aPresContext, aRenderingContext, aDirtyRect,
> > +               remainingWidth, currX, false);
> 
> To the reader, when you have a boolean like "false", it's not obvious what
> it does. You don't always have to do it, but I personally prefer to make it
> clear what the boolean means like this:
> 
>              remainingWidth, currX, /* aIsCanvas = */ false);
> 
ditto, maybe the better solution is doesn't pass a bool, but pass
the image directly.

> Consider doing this for both calls to PaintImage.
> 
> @@ +3410,5 @@
> >                              nsRenderingContext& aRenderingContext,
> >                              const nsRect&        aDirtyRect,
> >                              nscoord&             aRemainingWidth,
> > +                            nscoord&             aCurrX,
> > +                            bool                 isCanvas)
> 
> Nit: please use |aIsCanvas|.
> 
ditto
> @@ +3422,5 @@
> > +  if (isCanvas) {
> > +    imageContext = GetPseudoStyleContext(nsCSSAnonBoxes::moztreecanvas);
> > +  } else {
> > +    imageContext = GetPseudoStyleContext(nsCSSAnonBoxes::moztreeimage);
> > +  }
> 
> This whole chunk of code can be more concisely written as:
> 
> nsStyleContext* imageContext =
>   GetPseudoStyleContext(isCanvas ? nsCSSAnonBoxes::moztreecanvas :
> nsCSSAnonBoxes::moztreeimage);
> 
True, 
> @@ +3439,5 @@
> >    bool useImageRegion = true;
> >    nsCOMPtr<imgIContainer> image;
> > +  RefPtr<mozilla::gfx::SourceSurface> surface;
> > +  nsIntSize contentSize(0, 0);
> > +  nsIntSize *contentSizePtr = NULL;
> 
> Nit: it's Mozilla style to group the '*' with the type, and to use 'nullptr'
> instead of 'NULL', so write |nsIntSize* contentSizePtr = nullptr;|.
> 
> However, you don't need contentSizePtr at all, because |&contentSize| will
> work just as well in any context.
Yeap, with your API, this is not necessary anymore:)
> 
> @@ +3447,5 @@
> > +    if (image) {
> > +      int32_t width;
> > +      int32_t height;
> > +      image->GetWidth(&width);
> > +      image->GetHeight(&height);
> 
> You can just write |image->GetWidth(&contentSize.width);| and similar for
> height. You don't need the intermediate variables.
Nice:)
> 
> @@ +3462,5 @@
> > +      cellCanvas->Render(mView,
> > +        aRowIndex,
> > +        aColumn,
> > +        scratchArray,
> > +        getter_AddRefs(canvasCtx));
> 
> Nit: You should put multiple arguments on a line so that this call to Render
> takes up fewer lines.
Gotcha.
> 
> @@ +3467,5 @@
> > +    }
> > +
> > +    nsICanvasRenderingContextInternal *ctxInternal = NULL;
> > +    if (canvasCtx) {
> > +      canvasCtx->QueryInterface(NS_GET_IID(nsICanvasRenderingContextInternal), (void**)&ctxInternal);
> 
> You should never call QueryInterface manually; always use helper functions
> like do_QueryInterface. Also, you should store the result in an nsCOMPtr or
> you'll leak memory. You can write the above as:
Good
> 
> nsCOMPtr<nsICanvasRenderingContextInternal> ctxInternal =
> do_QueryInterface(canvasCtx);
> 
> You don't even need to check for null.
> 
> For more information, see here:
> 
> https://developer.mozilla.org/en-US/docs/Using_nsCOMPtr/Reference_Manual
> 
> @@ +3583,5 @@
> > +          imgIContainer::FLAG_NONE);
> > +    } else {
> > +      nsRefPtr<gfxDrawable> drawable =
> > +          new gfxSurfaceDrawable(surface, ThebesIntSize(surface->GetSize()));
> > +      nsLayoutUtils::DrawPixelSnapped(&aRenderingContext,
> 
> You should pull, because your code is out of date.
> nsLayoutUtils::DrawPixelSnapped no longer exists. That also explains why you
> don't see ImageOps::CreateFromDrawable, which *does* exist and is what you
> should use instead. Once you're using ImageOps::CreateFromDrawable, you
> won't need two separate code paths either, because you'll have an
> imgIContainer in either case.
> 
> @@ +4753,5 @@
> >    return NS_OK;
> >  }
> > +
> > +nsresult
> > +nsTreeBodyFrame::GetCellCanvas(nsITreeCellCanvas ** aCellCanvas)
> 
> Nit: Group the "**" with the type. Similar for SetCellCanvas below.
> 
> @@ +4757,5 @@
> > +nsTreeBodyFrame::GetCellCanvas(nsITreeCellCanvas ** aCellCanvas)
> > +{
> > +  *aCellCanvas = nullptr;
> > +  nsWeakFrame weakFrame(this);
> > +  NS_ENSURE_STATE(weakFrame.IsAlive());
> 
> Why are you creating an nsWeakFrame here at all?
I am copied the code about how nsITreeView is Get/Set, so I am not so sure about this.
> 
> @@ +4758,5 @@
> > +{
> > +  *aCellCanvas = nullptr;
> > +  nsWeakFrame weakFrame(this);
> > +  NS_ENSURE_STATE(weakFrame.IsAlive());
> > +  NS_IF_ADDREF(*aCellCanvas = mCellCanvas);
> 
> Don't use NS_IF_ADDREF. The proper pattern here is:
> 
> nsCOMPtr<nsITreeCellCanvas> canvas = mCellCanvas;
> canvas.forget(aCellCanvas);
thanks, maybe the treeView's Set Code also need this modification.
> 
> ::: layout/xul/tree/nsTreeBoxObject.cpp
> @@ +493,5 @@
> > +NS_IMETHODIMP
> > +nsTreeBoxObject::GetCellCanvas(nsITreeCellCanvas ** aCellCanvas)
> > +{
> > +  nsTreeBodyFrame* body = GetTreeBody();
> > +  if (body)
> 
> Nit: Always use braces around 'if' and 'else' bodies, even if they're only
> one line.
+1

	Yonggang Luo
	Comment 129 • 10 years ago

> 
> ::: layout/xul/tree/nsITreeColumns.idl
> @@ +32,4 @@
> >    const short TYPE_TEXT                = 1;
> >    const short TYPE_CHECKBOX            = 2;
> >    const short TYPE_PROGRESSMETER       = 3;
> > +  const short TYPE_CANVAS              = 4;
> 
> I don't remember whether this is enough to require a uuid bump or not.
> 
> ::: layout/xul/tree/nsTreeBodyFrame.cpp
> @@ +99,5 @@
> > +    scratchArray->AppendElement(atomArray[i], false);
> > +  }
> > +  return scratchArray;
> > +}
> > +
> 
> Again, a mutable array of nsIAtoms is less desirable.  See 
> nsTreeUtils::TokenizeProperties.
> 
Do you means I need to create a new API that like 
StringnizeProperties?

	Yonggang Luo
	Comment 130 • 10 years ago

(In reply to Colby Russell :crussell from comment #122)
> nsITreeCellCanvas sounds like it should be an object that you could draw to,
> but really the canvas still needs to be created elsewhere and returned by
> this interface's |renderCanvas| method.  nsITreeCellRenderer instead?
> 
> (Side note: can you use "-U 8" for your next patch?  Three lines of context
> makes things kind of difficult to read.)
> 
Can you tell me the full command, cause I don't know what's do you mean.

	Yonggang Luo
	Comment 131 • 10 years ago

This is a tentative patch about API. I am looking for suggestion first

	Colby Russell :crussell (no longer Mozilla-ing)
	Comment 132 • 10 years ago

(In reply to Yonggang Luo from comment #130)
> (In reply to Colby Russell :crussell from comment #122)
> > nsITreeCellCanvas sounds like it should be an object that you could draw to,
> > but really the canvas still needs to be created elsewhere and returned by
> > this interface's |renderCanvas| method.  nsITreeCellRenderer instead?
> > 
> > (Side note: can you use "-U 8" for your next patch?  Three lines of context
> > makes things kind of difficult to read.)
> > 
> Can you tell me the full command, cause I don't know what's do you mean.

See <https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F>.  Note the "unified = 8" part in the example .hgrc.  Alternatively, if you're creating your patches with hg diff, you can use hg diff -U 8.  If you're using mq, that won't work, though, so the .hgrc change is best, and more convenient anyway.

	Colby Russell :crussell (no longer Mozilla-ing)
	Comment 133 • 10 years ago

Comment on attachment 8482308 [details] [diff] [review]
0002-Reshape-The-API-to-looking-for-suggestion.patch

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

The review field is for people who have the authority to approve changes.  I can't review this.  feedback+ from me, though.

This patch is based on changes to your previous one.  When submitting patches, you'll want to submit a whole new patch reflecting all the changes you intend to make, and mark the old one obsolete.  Bugzilla already has a built-in interdiff feature for reviewers to show them what changed between patches.  (The exception is when you're working on a bug that makes big changes, and you decide ahead of time to split things up into discrete chunks to make them easier to work with and upload part 1/x, part 2/x, et cetera.  But this bug isn't one of them.)

Wait until Neil (or someone else) gives you a positive review before requesting one from Seth.

Thanks for working on this.

::: layout/xul/tree/nsITreeBoxObject.idl
@@ +14,4 @@
>  interface nsIScriptableRegion;
>  
> +[scriptable, uuid(aceed5f8-e57a-4cdc-b859-cf864904ab1c)]
> +interface nsITreeCellRender : nsISupports

nsITreeCellRenderer, not nsITreeCellRender.

@@ +42,4 @@
>    * The canvas handler for rich cell, to rendering anything that canvs
>    * support, we can use this to rendering beatiful tree cell.
>    */
> +  attribute nsITreeCellRender cellCanvas;

|cellRenderer| here, too, not |cellCanvas|.

	Colby Russell :crussell (no longer Mozilla-ing)
	Comment 134 • 10 years ago

I forgot to mention that I bigger fan of the approach in the previous patch where the |draw| method would be returning a canvas element (in which case the nsITreeCellRenderer IDL should say that, and not nsISupports, as Seth says).  imgIContainer is conceptually "purer", but for practical reasons nsIDOMHTMLCanvasElement would be easier to work with for people who would actually be looking into implementing this.

It does occur to me that the APIs don't exist for a scripted renderer to be able to get the correct styles to use to paint the cell...

(In reply to Seth Fowler [:seth] from comment #120)
> I'm not sure why this should return anything, in
> fact. If you need to get the canvas object this contains, you should
> probably have a separate getter or attribute that uses a canvas interface
> and not just nsISupports.

This effectively enforces an "immediate mode" approach, since the tree's one renderer would be limited to one canvas shared between all cells it's responsible for painting.  The renderer would have to redo every |draw| call, even for cells that haven't changed, instead of being able to hand back a canvas/imgIContainer with no changes.

	Yonggang Luo
	Comment 135 • 10 years ago

(In reply to Colby Russell :crussell from comment #134)
> I forgot to mention that I bigger fan of the approach in the previous patch
> where the |draw| method would be returning a canvas element (in which case
> the nsITreeCellRenderer IDL should say that, and not nsISupports, as Seth
> says).  imgIContainer is conceptually "purer", but for practical reasons
> nsIDOMHTMLCanvasElement would be easier to work with for people who would
> actually be looking into implementing this.
In my design, returning imgIContainer would not stop users to using nsIDOMHTMLCanvasElement 
to rendering things. Just to make sure the returning result is imgIContainer, we can expose the 
API that can convert from nsIDOMHTMLCanvasElement  to imgIContainer.
If we really want to return nsIDOMHTMLCanvasElement, then we would using a less generic 
method drawCanvas(...).


> 
> It does occur to me that the APIs don't exist for a scripted renderer to be
> able to get the correct styles to use to paint the cell...
> 
> (In reply to Seth Fowler [:seth] from comment #120)
> > I'm not sure why this should return anything, in
> > fact. If you need to get the canvas object this contains, you should
> > probably have a separate getter or attribute that uses a canvas interface
> > and not just nsISupports.
> 
> This effectively enforces an "immediate mode" approach, since the tree's one
> renderer would be limited to one canvas shared between all cells it's
> responsible for painting.  The renderer would have to redo every |draw|
> call, even for cells that haven't changed, instead of being able to hand
Well, this is not so sure, because tree is using invalidate to handling the re-draw,
so we can make sure everytime the renderer calling to draw, it's always can be sure that 
we really need to redraw the things totally.
> back a canvas/imgIContainer with no changes.

	Yonggang Luo
	Comment 136 • 10 years ago

:seth, does the following implementation are OK?

NS_IMETHODIMP
imgTools::CreateImageFromCanvas(nsIDOMHTMLCanvasElement *aCanvas,
                                const nsAString&        aContextName,
                                imgIContainer           **aContainer)
{
  *aContainer = nullptr;
  NS_ENSURE_ARG_POINTER(aCanvas);
  nsAutoString contextName(NS_LITERAL_STRING("2d"));
  if (aContextName != EmptyString()) {
    contextName = aContextName;
  }
  nsCOMPtr<nsISupports> canvasContext;
  aCanvas->MozGetIPCContext(contextName, getter_AddRefs(canvasContext));
  nsCOMPtr<nsICanvasRenderingContextInternal> ctxInternal =
        do_QueryInterface(canvasContext);
  if (!ctxInternal) {
    return NS_OK;
  }
  RefPtr<mozilla::gfx::SourceSurface> surface =
        ctxInternal->GetSurfaceSnapshot();
  if (!surface) {
    return NS_OK;
  }
  RefPtr<gfxDrawable> drawable =
      new gfxSurfaceDrawable(surface, ThebesIntSize(surface->GetSize()));
  nsCOMPtr<imgIContainer> container = ImageOps::CreateFromDrawable(drawable);
  *aContainer = container;
  return NS_OK;
}

	Yonggang Luo
	Comment 137 • 10 years ago

I still feels pass Context is better:

NS_IMETHODIMP
imgTools::CreateImageFromCanvasContext(nsISupports        *aCanvasContext,
                                       imgIContainer      **aContainer)
{
  *aContainer = nullptr;
  nsCOMPtr<nsICanvasRenderingContextInternal> ctxInternal =
      do_QueryInterface(aCanvasContext);

We doesn't need to handle the getContext things.

	Seth Fowler [:seth] [:s2h]
	Comment 138 • 10 years ago

(In reply to Colby Russell :crussell from comment #134)
> This effectively enforces an "immediate mode" approach, since the tree's one
> renderer would be limited to one canvas shared between all cells it's
> responsible for painting.  The renderer would have to redo every |draw|
> call, even for cells that haven't changed, instead of being able to hand
> back a canvas/imgIContainer with no changes.

Yeah, that doesn't sound desirable, though I still find the API strange. I'll have to rely on people more experienced with XUL to suggest a better alternative if there is one, because I don't know really know this part of the code at all.

	Seth Fowler [:seth] [:s2h]
	Comment 139 • 10 years ago

(In reply to Yonggang Luo from comment #136)
> :seth, does the following implementation are OK?

Almost.

>   nsCOMPtr<nsICanvasRenderingContextInternal> ctxInternal =
>         do_QueryInterface(canvasContext);
>   if (!ctxInternal) {
>     return NS_OK;
>   }
>   RefPtr<mozilla::gfx::SourceSurface> surface =
>         ctxInternal->GetSurfaceSnapshot();
>   if (!surface) {
>     return NS_OK;
>   }

This is all good.

>   RefPtr<gfxDrawable> drawable =
>       new gfxSurfaceDrawable(surface, ThebesIntSize(surface->GetSize()));

This is fine too, but you should use an nsRefPtr rather than a RefPtr here. You basically

>   nsCOMPtr<imgIContainer> container = ImageOps::CreateFromDrawable(drawable);
>   *aContainer = container;

The imgIContainer you're creating here will get destroyed as soon as this method returns. To return the imgIContainer correctly, you need to replace |*aContainer = container;| with |container.forget(aContainer);|.

By the way, I agree with you that it's more convenient to just pass in the canvas context if that will work for you.

	Seth Fowler [:seth] [:s2h]
	Comment 140 • 10 years ago

Whoops, I failed to finish a sentence above. What I meant to write was: "You basically always want to use nsRefPtr for concrete types and nsCOMPtr for interfaces. RefPtr is basically only used with Moz2D code, which is why it's used for SourceSurface."

	Yonggang Luo
	Comment 141 • 10 years ago

(In reply to Seth Fowler [:seth] from comment #140)
> Whoops, I failed to finish a sentence above. What I meant to write was: "You
> basically always want to use nsRefPtr for concrete types and nsCOMPtr for
> interfaces. RefPtr is basically only used with Moz2D code, which is why it's
> used for SourceSurface."
This is the update version of CreateImageFromCanvasContext

NS_IMETHODIMP
imgTools::CreateImageFromCanvasContext(nsISupports        *aCanvasContext,
                                       imgIContainer      **aContainer)
{
  *aContainer = nullptr;
  nsCOMPtr<nsICanvasRenderingContextInternal> contextInternal =
      do_QueryInterface(aCanvasContext);
  if (!contextInternal) {
    return NS_OK;
  }
  RefPtr<mozilla::gfx::SourceSurface> surface =
      contextInternal->GetSurfaceSnapshot();
  if (!surface) {
    return NS_OK;
  }
  nsRefPtr<gfxDrawable> drawable =
      new gfxSurfaceDrawable(surface, ThebesIntSize(surface->GetSize()));
  nsCOMPtr<imgIContainer> container = ImageOps::CreateFromDrawable(drawable);
  container.forget(aContainer);
  return NS_OK;
}

	Seth Fowler [:seth] [:s2h]
	Comment 142 • 10 years ago

(In reply to Yonggang Luo from comment #141)
> This is the update version of CreateImageFromCanvasContext

Looks good!

>   *aContainer = nullptr;

You don't need this line, though.

	Yonggang Luo
	Comment 143 • 10 years ago

I fount in bugzilla, it's very hard to submit patch series.

	Alex Vincent [:WeirdAl]
	Comment 144 • 10 years ago

I'll respond with my feedback comments this weekend.  I've been very busy lately, and for that I apologize.

	Yonggang Luo
	Comment 145 • 10 years ago

Upgrade the patch with all kinds of fixes. Now it's cellRenderer and return a more
generic ImageContainer.

	Alex Vincent [:WeirdAl]
	Comment 146 • 10 years ago

Comment on attachment 8484232 [details] [diff] [review]
cellRenderer.diff

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

I'd be much more comfortable with this if you were to include some sample XUL+JS that exercises this code.  (A reftest would be fantastic, but I wouldn't expect that.  A crashtest would be better than no test at all.)  The patch has been through a number of changes since the initial proposal, including the API.

In particular I want to see an example implementation of the nsITreeCellRenderer, and how to use it.  That will help me shape my thoughts on the API.

::: layout/xul/tree/nsITreeBoxObject.idl
@@ +39,5 @@
>    attribute nsITreeView view;
>  
>    /**
> +  * The canvas handler for rich cell, to rendering anything that canvs
> +  * support, we can use this to rendering beatiful tree cell.

Spelling typos:  canvas, beautiful.

::: layout/xul/tree/nsITreeColumns.idl
@@ +31,5 @@
>  
>    const short TYPE_TEXT                = 1;
>    const short TYPE_CHECKBOX            = 2;
>    const short TYPE_PROGRESSMETER       = 3;
> +  const short TYPE_RENDERER            = 4;

I think you should keep a blank line between the constants and the associated type.  But that's just my opinion; if others disagree, go with what they say.

::: layout/xul/tree/nsTreeBodyFrame.cpp
@@ +3305,5 @@
> +          nsStyleContext* rendererContext = GetPseudoStyleContext(nsCSSAnonBoxes::moztreerenderer);
> +          bool useImageRegion = true;
> +          nsCOMPtr<imgIContainer> image;
> +          if (mCellRenderer) {
> +            nsAutoString properties = nsTreeUtils::StringizeProperties(mScratchArray);

I really think this is unnecessary:  you already have properties and mScratchArray established in PaintCell.  See http://hg.mozilla.org/mozilla-central/annotate/2255d7d187b2/layout/xul/tree/nsTreeBodyFrame.cpp#l3137 .  Neither existing code nor your patch changes properties or mScratchArray.

@@ +3312,5 @@
> +          if (image) {
> +            useImageRegion = false;
> +          } else {
> +            GetImage(aRowIndex, aColumn, true, rendererContext, useImageRegion, getter_AddRefs(image));
> +          }

What happens if the Draw call throws an exception?  What happens if it doesn't set an image as you expect it to?  You're going to fall into the GetImage code, and it's not entirely clear that's what you want.

@@ +3434,5 @@
>    NS_PRECONDITION(aColumn && aColumn->GetFrame(), "invalid column passed");
>  
>    bool isRTL = StyleVisibility()->mDirection == NS_STYLE_DIRECTION_RTL;
>    nscoord rightEdge = aCurrX + aRemainingWidth;
> +  nsStyleContext* imageContext = aStyleContext;

I don't understand the purpose of this pointer, since you've already established the pointer in the function's arguments.

I think you should add some NS_PRECONDITION lines, in case we get a null pointer for arguments you think you have.  (But since we'd crash anyway on a null dereference, maybe not.)

@@ +3447,5 @@
>    imageContext->StyleMargin()->GetMargin(imageMargin);
>    imageRect.Deflate(imageMargin);
>  
> +  bool useImageRegion = aUseImageRegion;
> +  nsCOMPtr<imgIContainer> image = aImage;

I understand this one even less.  You certainly don't need a nsCOMPtr here if you've already got the image.  Are you trying to avoid renaming variables in the function?

@@ +4757,5 @@
> +{
> +  nsWeakFrame weakFrame(this);
> +  NS_ENSURE_STATE(weakFrame.IsAlive());
> +  nsCOMPtr<nsITreeCellRenderer> renderer = mCellRenderer;
> +  renderer.forget(aCellRenderer);

*aCellRenderer = mCellRenderer

Then you don't need the local nsCOMPtr.

::: layout/xul/tree/nsTreeBodyFrame.h
@@ +634,5 @@
>    nsTHashtable<nsPtrHashKey<nsTreeImageListener> > mCreatedListeners;
>  
> +  // This is a external tree cell render, it's will rendering the cell
> +  // content, and resulting imgIContainer, the rendering progress will
> +  // be canvas or other ways.

I don't understand this comment.

::: layout/xul/tree/nsTreeBoxObject.cpp
@@ +508,5 @@
> +  if (body) {
> +    return body->SetCellRenderer(aCellRenderer);
> +  }
> +  return NS_OK;
> +}

Are we missing NS_ENSURE_ARG_POINTER(aCellRenderer); on the getter?

::: layout/xul/tree/nsTreeColumns.cpp
@@ +317,5 @@
>  
>    // Figure out our column type. Default type is text.
>    mType = nsITreeColumn::TYPE_TEXT;
>    static nsIContent::AttrValuesArray typestrings[] =
> +    {&nsGkAtoms::checkbox, &nsGkAtoms::progressmeter, &nsGkAtoms::renderer, nullptr};

Consider breaking this up into multiple lines:

  {
    &nsGkAtoms::checkbox,
    &nsGkAtoms::progressmeter,
    &nsGkAtoms::renderer,
    nullptr
  };

That would make it easier to add new types later.

::: layout/xul/tree/nsTreeUtils.cpp
@@ +51,5 @@
>    return NS_OK;
>  }
>  
> +nsAutoString
> +nsTreeUtils::StringizeProperties(const AtomArray& aPropertiesArray)

As I mention above, I don't see how you need this method.

	Yonggang Luo
	Comment 147 • 10 years ago

(In reply to Alex Vincent [:WeirdAl] from comment #146)
> Comment on attachment 8484232 [details] [diff] [review]
> cellRenderer.diff
> 
> Review of attachment 8484232 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd be much more comfortable with this if you were to include some sample
> XUL+JS that exercises this code.  (A reftest would be fantastic, but I
> wouldn't expect that.  A crashtest would be better than no test at all.) 
> The patch has been through a number of changes since the initial proposal,
> including the API.
> 
> In particular I want to see an example implementation of the
> nsITreeCellRenderer, and how to use it.  That will help me shape my thoughts
> on the API.
> 
> ::: layout/xul/tree/nsITreeBoxObject.idl
> @@ +39,5 @@
> >    attribute nsITreeView view;
> >  
> >    /**
> > +  * The canvas handler for rich cell, to rendering anything that canvs
> > +  * support, we can use this to rendering beatiful tree cell.
> 
> Spelling typos:  canvas, beautiful.
> 
> ::: layout/xul/tree/nsITreeColumns.idl
> @@ +31,5 @@
> >  
> >    const short TYPE_TEXT                = 1;
> >    const short TYPE_CHECKBOX            = 2;
> >    const short TYPE_PROGRESSMETER       = 3;
> > +  const short TYPE_RENDERER            = 4;
> 
> I think you should keep a blank line between the constants and the
> associated type.  But that's just my opinion; if others disagree, go with
> what they say.
> 
> ::: layout/xul/tree/nsTreeBodyFrame.cpp
> @@ +3305,5 @@
> > +          nsStyleContext* rendererContext = GetPseudoStyleContext(nsCSSAnonBoxes::moztreerenderer);
> > +          bool useImageRegion = true;
> > +          nsCOMPtr<imgIContainer> image;
> > +          if (mCellRenderer) {
> > +            nsAutoString properties = nsTreeUtils::StringizeProperties(mScratchArray);
> 
> I really think this is unnecessary:  you already have properties and
> mScratchArray established in PaintCell.  See
> http://hg.mozilla.org/mozilla-central/annotate/2255d7d187b2/layout/xul/tree/
> nsTreeBodyFrame.cpp#l3137 .  Neither existing code nor your patch changes
> properties or mScratchArray.
I need to pass properties  to external render, so that the render can rendering things accoriding
to the properties .
> 
> @@ +3312,5 @@
> > +          if (image) {
> > +            useImageRegion = false;
> > +          } else {
> > +            GetImage(aRowIndex, aColumn, true, rendererContext, useImageRegion, getter_AddRefs(image));
> > +          }
> 
> What happens if the Draw call throws an exception?  What happens if it
> doesn't set an image as you expect it to?  You're going to fall into the
> GetImage code, and it's not entirely clear that's what you want.
If there is a exception, then would not handle it, because the exception will inform
the user/programmer that the draw code is implemented wrong. and the direct result will
cause the drawing to be blank or abnormal, that what we want.


> 
> @@ +3434,5 @@
> >    NS_PRECONDITION(aColumn && aColumn->GetFrame(), "invalid column passed");
> >  
> >    bool isRTL = StyleVisibility()->mDirection == NS_STYLE_DIRECTION_RTL;
> >    nscoord rightEdge = aCurrX + aRemainingWidth;
> > +  nsStyleContext* imageContext = aStyleContext;
> 
> I don't understand the purpose of this pointer, since you've already
> established the pointer in the function's arguments.
for the purpose don't rename at multiple place.

> 
> I think you should add some NS_PRECONDITION lines, in case we get a null
> pointer for arguments you think you have.  (But since we'd crash anyway on a
> null dereference, maybe not.)
> 
> @@ +3447,5 @@
> >    imageContext->StyleMargin()->GetMargin(imageMargin);
> >    imageRect.Deflate(imageMargin);
Yeap, we need to do a ASSERT, because we can be sure this would not be NULL.

> >  
> > +  bool useImageRegion = aUseImageRegion;
> > +  nsCOMPtr<imgIContainer> image = aImage;
> 
> I understand this one even less.  You certainly don't need a nsCOMPtr here
> if you've already got the image.  Are you trying to avoid renaming variables
> in the function?
for the purpose don't rename at multiple place.

> 
> @@ +4757,5 @@
> > +{
> > +  nsWeakFrame weakFrame(this);
> > +  NS_ENSURE_STATE(weakFrame.IsAlive());
> > +  nsCOMPtr<nsITreeCellRenderer> renderer = mCellRenderer;
> > +  renderer.forget(aCellRenderer);
> 
> *aCellRenderer = mCellRenderer
> 
> Then you don't need the local nsCOMPtr.
Are you sure there won't be memory leaks?

> 
> ::: layout/xul/tree/nsTreeBodyFrame.h
> @@ +634,5 @@
> >    nsTHashtable<nsPtrHashKey<nsTreeImageListener> > mCreatedListeners;
> >  
> > +  // This is a external tree cell render, it's will rendering the cell
> > +  // content, and resulting imgIContainer, the rendering progress will
> > +  // be canvas or other ways.
> 
> I don't understand this comment.
Yeap, we need a javascript demo for this.

> 
> ::: layout/xul/tree/nsTreeBoxObject.cpp
> @@ +508,5 @@
> > +  if (body) {
> > +    return body->SetCellRenderer(aCellRenderer);
> > +  }
> > +  return NS_OK;
> > +}
> 
> Are we missing NS_ENSURE_ARG_POINTER(aCellRenderer); on the getter?


> 
> ::: layout/xul/tree/nsTreeColumns.cpp
> @@ +317,5 @@
> >  
> >    // Figure out our column type. Default type is text.
> >    mType = nsITreeColumn::TYPE_TEXT;
> >    static nsIContent::AttrValuesArray typestrings[] =
> > +    {&nsGkAtoms::checkbox, &nsGkAtoms::progressmeter, &nsGkAtoms::renderer, nullptr};
> 
> Consider breaking this up into multiple lines:
> 
>   {
>     &nsGkAtoms::checkbox,
>     &nsGkAtoms::progressmeter,
>     &nsGkAtoms::renderer,
>     nullptr
>   };
> 
> That would make it easier to add new types later.
> 
> ::: layout/xul/tree/nsTreeUtils.cpp
> @@ +51,5 @@
> >    return NS_OK;
> >  }
> >  
> > +nsAutoString
> > +nsTreeUtils::StringizeProperties(const AtomArray& aPropertiesArray)
> 
> As I mention above, I don't see how you need this method.

	Yonggang Luo
	Comment 148 • 10 years ago

> 
> I really think this is unnecessary:  you already have properties and
> mScratchArray established in PaintCell.  See
> http://hg.mozilla.org/mozilla-central/annotate/2255d7d187b2/layout/xul/tree/
> nsTreeBodyFrame.cpp#l3137 .  Neither existing code nor your patch changes
> properties or mScratchArray.
> 
The mScratchArray is a join set of Cell/Row/Column, and the properties is just for the specific Cell
that's why I need to convert mScratchArray back to nsAutoString and pass it to the drawing code.

	Alex Vincent [:WeirdAl]
	Comment 149 • 10 years ago

If you're worried about the correct Get/Set pattern in C++ XPCOM, take a look at this:
http://mxr.mozilla.org/mozilla-release/source/parser/xml/src/nsSAXXMLReader.cpp#356

I don't think SAXXMLReader will be converted to WebIDl anytime soon... :)

	neil@parkwaycc.co.uk
	Comment 150 • 10 years ago

Comment on attachment 8482308 [details] [diff] [review]
0002-Reshape-The-API-to-looking-for-suggestion.patch

Sorry for the delay in responding to this review. Unfortunately I'm not sure what I'm supposed to be reviewing here because this diff is not against mozilla-central.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 151 • 10 years ago

Is there a description of what it is you're doing?  I'd suggest putting it in a wiki, since this bug is too long to find anything in.  (And how does it differ from what Paul was doing?)

Also, is this a patch against mozilla-central or against ESR31?  If the latter, please submit patches against central for review, not ESR31.

	Seth Fowler [:seth] [:s2h]
	Comment 152 • 10 years ago

(In reply to Alex Vincent [:WeirdAl] from comment #146)
> @@ +4757,5 @@
> > +{
> > +  nsWeakFrame weakFrame(this);
> > +  NS_ENSURE_STATE(weakFrame.IsAlive());
> > +  nsCOMPtr<nsITreeCellRenderer> renderer = mCellRenderer;
> > +  renderer.forget(aCellRenderer);
> 
> *aCellRenderer = mCellRenderer
> 
> Then you don't need the local nsCOMPtr.

I don't think that's correct, since your approach doesn't do an addref.

	Seth Fowler [:seth] [:s2h]
	Comment 153 • 10 years ago

Comment on attachment 8484232 [details] [diff] [review]
cellRenderer.diff

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

Thanks for continuing to push forward!

I'll have to r- this just because of the imgITools changes. I'm willing to reconsider if you explain why it's necessary, though.

Also, I'm frankly starting to get confused about what's happening here. I think all the reviewers will find this much easier to understand if you rebase the code you have on top of mozilla-central tip. Furthermore, it'd be helpful if you divide your code into smaller, logically related patches. This seems like it's just a diff from the previous patch, although I'm not sure, and that's pretty confusing to review in a bug as long as this.

::: image/public/imgITools.idl
@@ +90,5 @@
>                                       in long aWidth,
>                                       in long aHeight,
>                                       [optional] in AString outputOptions);
>  
> +    imgIContainer createImageFromCanvasContext(in nsISupports canvasContext);

Why are you adding this? If you don't have a very good reason to do this from JavaScript, then you should not be adding this to imgITools.

If you're only calling this from C++, then ImageOps would be the place to add it, but if there's only one caller you shouldn't even do that.

::: layout/xul/tree/nsITreeColumns.idl
@@ +31,5 @@
>  
>    const short TYPE_TEXT                = 1;
>    const short TYPE_CHECKBOX            = 2;
>    const short TYPE_PROGRESSMETER       = 3;
> +  const short TYPE_RENDERER            = 4;

I agree with Alex; restore the blank line that used to be here.

::: layout/xul/tree/nsTreeBodyFrame.h
@@ +117,5 @@
>    nsresult BeginUpdateBatch();
>    nsresult EndUpdateBatch();
>    nsresult ClearStyleAndImageCaches();
> +  nsresult GetCellRenderer(nsITreeCellRenderer ** aCellRenderer);
> +  nsresult SetCellRenderer(nsITreeCellRenderer * aCellRenderer);

This  doesn't match the style guide. Please write |nsITreeCellRenderer*|, with no space between the type name and the star.

::: layout/xul/tree/nsTreeBoxObject.cpp
@@ +491,5 @@
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +nsTreeBoxObject::GetCellRenderer(nsITreeCellRenderer ** aCellRenderer)

This violates the style guide; please use |nsITreeCellRender**|, with no space.

@@ +497,5 @@
> +  nsTreeBodyFrame* body = GetTreeBody();
> +  if (body) {
> +    return body->GetCellRenderer(aCellRenderer);
> +  }
> +  return NS_OK;

Should you return NS_OK if GetTreeBody() returns null? I'm not saying you shouldn't, but make sure you've thought about it.

(This and the previous comment also apply to SetCellRenderer.)

::: layout/xul/tree/nsTreeUtils.cpp
@@ +60,5 @@
> +      properties += NS_LITERAL_STRING(" ");
> +    }
> +    properties += aPropertiesArray[i]->GetUTF16String();
> +  }
> +  return std::move(properties);

Don't use |std::move| in Mozilla code. You need the |Move()| function from |mozilla/Move.h|. (You'd have found this out eventually anyway, because this won't build on all platforms.)

	Seth Fowler [:seth] [:s2h]
	Comment 154 • 10 years ago

(In reply to Alex Vincent [:WeirdAl] from comment #149)
> If you're worried about the correct Get/Set pattern in C++ XPCOM, take a
> look at this:
> http://mxr.mozilla.org/mozilla-release/source/parser/xml/src/nsSAXXMLReader.
> cpp#356
> 
> I don't think SAXXMLReader will be converted to WebIDl anytime soon... :)

Please don't suggest that people use NS_IF_ADDREF.

	Yonggang Luo
	Comment 155 • 10 years ago

Is there any easy way to submit a series of patches?
Bugzilla limit only to submit a patch.

	Yonggang Luo
	Comment 156 • 10 years ago

Update with all comments.
The xul demo is coming.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 157 • 10 years ago

Restoring needinfo? from comment 151.

(In reply to Yonggang Luo from comment #155)
> Is there any easy way to submit a series of patches?
> Bugzilla limit only to submit a patch.

You can either:
 (1) use the "Create an attachment" form multiple times
 (2) use the bzexport mercurial extension (easier)

	Yonggang Luo
	Comment 158 • 10 years ago

The richtree test case.

	Yonggang Luo
	Comment 159 • 10 years ago

Complete patches set with crashtest.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 166 • 10 years ago

I can't even start on reviewing this, or figuring out an appropriate alternate reviewer, until you answer comment 151.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 167 • 10 years ago

Also, you should generally not request review from 5 people per patch unless you have a very good reason.

	Yonggang Luo
	Comment 168 • 10 years ago

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #151)
> Is there a description of what it is you're doing?  I'd suggest putting it
> in a wiki, since this bug is too long to find anything in.  (And how does it
> differ from what Paul was doing?)
> 
> Also, is this a patch against mozilla-central or against ESR31?  If the
> latter, please submit patches against central for review, not ESR31.
Er. it's not against ESR31, it's against tip. I've state it before.

	Yonggang Luo
	Comment 169 • 10 years ago

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #167)
> Also, you should generally not request review from 5 people per patch unless
> you have a very good reason.

They reviewed my code before.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 170 • 10 years ago

Is there a description of what it is you're doing?  I'd suggest putting it in a wiki, since this bug is too long to find anything in.  (And how does it differ from what Paul was doing?)

	Yonggang Luo
	Comment 171 • 10 years ago

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #170)
> Is there a description of what it is you're doing?  I'd suggest putting it
> in a wiki, since this bug is too long to find anything in.  (And how does it
> differ from what Paul was doing?)
Gotcha, I will document it, can you give me a wiki page to document it?
At the current time, the richtree.xul demo is the best document

	Yonggang Luo
	Comment 172 • 10 years ago

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #170)
> Is there a description of what it is you're doing?  I'd suggest putting it
> in a wiki, since this bug is too long to find anything in.  (And how does it
> differ from what Paul was doing?)
The main difference is that Paul is trying to support multiline support in C++ code, and have little
customize possibility, and at that time, we do not have 'Canvas',
and now, I was trying to treat a Tree Cell as a canvas, and rendering it through Javascript
Code, and I can feed the Canvas with the Tree Cell/Row data, so we can rendering 
the Cell in any form we want.
The Core API is like following:

The renderer hook interface:
[scriptable, uuid(aceed5f8-e57a-4cdc-b859-cf864904ab1c)]
interface nsITreeCellRenderer : nsISupports
{
  /**
  * Rendering a tree cell's content at the position (row,col), returning
  * the imgIContainer of the drawn result.
  */
  imgIContainer draw(in nsITreeView view, in long row, in nsITreeColumn col, in AString props);
};

The renderer hook
[scriptable, uuid(64ba5199-c4f4-4498-bbdc-f8e4c369086c)]
interface nsITreeBoxObject : nsISupports
{
  /**
  * The canvas handler for rich cell, to rendering anything that canvs
  * support, we can use this to rendering beatiful tree cell.
  */
  attribute nsITreeCellRenderer cellRenderer;
}


In Javascript:
document.getElementById('tree').treeBoxObject.cellRenderer = {
  // nsISupports
  QueryInterface: function(aIID) {
    if (aIID.equals(Ci.nsITreeCellRenderer) ||
        aIID.equals(Ci.nsISupports) ||
        aIID.equals(Ci.nsISupportsWeakReference))
      return this;
    throw Components.results.NS_NOINTERFACE;
  },
  //imgIContainer (ctx) render(in nsITreeView view, in long row, in nsITreeColumn col, in AString props);
  draw: function(view, row, col, props) {
    let canvas = window.document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
    let ctx = this.canvas.getContext("2d");
    /* Drawing things with ctx */
    let imageTools = Cc["@mozilla.org/image/tools;1"].
         getService(Ci.imgITools);
    /* Putting the canvas result into Tree Cell by draw API */
    return imageTools.createImageFromCanvasContext(ctx);
  }
}

In C++:
We have alreayd have DrawImage API, so use it to 
draw the imgIContainer directly, little modifcation to C++ code.

	Yonggang Luo
	Comment 173 • 10 years ago

Forgot to update the UUID of nsITreeBoxObject

The main difference is:
Paul is trying to support multiline support in C++ code, and can not be customize use Javascript,
  and at that time, we do not have 'canvas',
I was trying to treat a `Tree Cell` as a `Canvas`, and rendering it through Javascript
Code, and I can feed the `Canvas` with the Tree Cell/Row data, so we can rendering 
the Cell in any form we want.

The Core API is like following:

The renderer hook interface:
[scriptable, uuid(aceed5f8-e57a-4cdc-b859-cf864904ab1c)]
interface nsITreeCellRenderer : nsISupports
{
  /**
  * Rendering a tree cell's content at the position (row,col), returning
  * the imgIContainer of the drawn result.
  */
  imgIContainer draw(in nsITreeView view, in long row, in nsITreeColumn col, in AString props);
};

The renderer hook:
[scriptable, uuid(64ba5199-c4f4-4498-bbdc-f8e4c369086c)]
interface nsITreeBoxObject : nsISupports
{
  /**
  * The canvas handler for rich cell, to rendering anything that canvs
  * support, we can use this to rendering beatiful tree cell.
  */
  attribute nsITreeCellRenderer cellRenderer;
}


In Javascript:
document.getElementById('tree').treeBoxObject.cellRenderer = {
  // nsISupports
  QueryInterface: function(aIID) {
    if (aIID.equals(Ci.nsITreeCellRenderer) ||
        aIID.equals(Ci.nsISupports) ||
        aIID.equals(Ci.nsISupportsWeakReference))
      return this;
    throw Components.results.NS_NOINTERFACE;
  },
  //imgIContainer (ctx) render(in nsITreeView view, in long row, in nsITreeColumn col, in AString props);
  draw: function(view, row, col, props) {
    let canvas = window.document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
    let ctx = this.canvas.getContext("2d");
    /* Drawing things with ctx */
    let imageTools = Cc["@mozilla.org/image/tools;1"].
         getService(Ci.imgITools);
    /* Putting the canvas result into Tree Cell by draw API */
    return imageTools.createImageFromCanvasContext(ctx);
  }
}

In C++:
The TreeBoxObject already have PaintImage api, because we return imgIContainer directly, so we can reuse this
API to rendering our Canvas context to the surface.

	neil@parkwaycc.co.uk
	Comment 174 • 10 years ago

Comment on attachment 8496482 [details] [diff] [review]
0001-Refinement-the-nsTreeBodyFrame-GetImage-to-reduce-th.patch

Seems reasonable to me.

	Colby Russell :crussell (no longer Mozilla-ing)
	Comment 175 • 10 years ago

Comment on attachment 8496482 [details] [diff] [review]
0001-Refinement-the-nsTreeBodyFrame-GetImage-to-reduce-th.patch

The review field is for asking module owners or module peers whether the patch is good for check-in.  <https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed>  The best I can give is some non-authoritative comments, but I'm not even all that involved with Mozilla nowadays, so...

I'll say this, though:

It looks like you are frustrated with Bugzilla based on your experience with this bug.  The bug has a lot of history and activity and an unclear focus. (See comment 69.)  If you get much more frustrated with it, I don't think anyone would be too upset with you if you created a new bug, then marked it as blocking this one.  If you do that, make sure you CC some appropriate people and leave a comment here pointing to the new bug.  In the new bug you'll want to:

a) summarize the problem in the first comment,
b) attach your patches there, and optionally
c) explain alternative approaches you are considering and why

You'll probably benefit from it, and I don't speak for anybody else, but I'll bet dbaron and a few others who have a similar perspective would have an easier time with it, too.

	Seth Fowler [:seth] [:s2h]
	Comment 176 • 10 years ago

Comment on attachment 8496482 [details] [diff] [review]
0001-Refinement-the-nsTreeBodyFrame-GetImage-to-reduce-th.patch

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

Looks good. I don't think you need review from 5 people for this particular patch; it's pretty simple. I've unset the review flags for everyone other than me and Neil.

	pieter kristensen
	Comment 177 • 10 years ago

I do hope that this patch will give an opening to an easy solution of bug 213945 https://bugzilla.mozilla.org/show_bug.cgi?id=213945 "Mail/message listing/thread pane needs more organization in 3 vertical pane view (like Outlook, Lotus Notes et al.)".
This bug was filed in 2003!

	Seth Fowler [:seth] [:s2h]
	Comment 178 • 10 years ago

Comment on attachment 8496483 [details] [diff] [review]
0002-Add-imgIContainer-createImageFromCanvasContext-in-ns.patch

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

I'm the only reviewer you need on an imagelib patch. I've unset the other review flags.

It seems that you're adding |createImageFromCanvasContext| in order to support the |nsITreeCellRenderer.draw| API you've added.

That seems reasonable to me, but I need to think a little about this API before I r+ this.

I've added some initial review comments, though.

::: image/public/imgITools.idl
@@ +90,5 @@
>                                       in long aWidth,
>                                       in long aHeight,
>                                       [optional] in AString outputOptions);
>  
> +    imgIContainer createImageFromCanvasContext(in nsISupports canvasContext);

This shouldn't take nsISupports. It needs to take a real canvas interface.

You also need a documentation comment here.

::: image/src/imgTools.cpp
@@ +311,5 @@
>  }
>  
>  NS_IMETHODIMP
> +imgTools::CreateImageFromCanvasContext(nsISupports        *aCanvasContext,
> +                                       imgIContainer      **aContainer)

Again, this violates the style guide. Please write |nsISupports*| and |imgIContainer**|, with no space between the type name and the '*'.

@@ +316,5 @@
> +{
> +  nsCOMPtr<nsICanvasRenderingContextInternal> contextInternal =
> +      do_QueryInterface(aCanvasContext);
> +  if (!contextInternal) {
> +    return NS_OK;

Should you really be returning NS_OK if the caller passed in an argument of the wrong type? (Though you won't need this check anyway if you fix this method to not take an nsISupports.)

@@ +318,5 @@
> +      do_QueryInterface(aCanvasContext);
> +  if (!contextInternal) {
> +    return NS_OK;
> +  }
> +  RefPtr<mozilla::gfx::SourceSurface> surface =

You don't need "mozilla::gfx::", as we have "using mozilla::gfx" at the top of this file. Just write "SourceSurface".

	Seth Fowler [:seth] [:s2h]
	Comment 179 • 10 years ago

Comment on attachment 8496484 [details] [diff] [review]
0003-Add-a-cell-column-row-type-TYPE_RENDERER-and-corresp.patch

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

Seems OK, though this absolutely needs an r+ from someone with more XUL knowledge.

	Seth Fowler [:seth] [:s2h]
	Comment 180 • 10 years ago

Comment on attachment 8496494 [details] [diff] [review]
0005-Fixes-for-bug-441414-to-add-rich-tree-support-crasht.patch

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

The image-related code in this patch looks good! I'm r+'ing that aspect of it. There are some minor issues, which I've pointed out below, but nothing that should be hard to fix.

I'm not reviewing the overall design you're using here and the tests. Someone more familiar with XUL code needs to do that.

If you end up having to make substantial changes to the design, I'll need to review again.

::: layout/xul/tree/nsITreeBoxObject.idl
@@ +19,5 @@
> +  /**
> +  * Rendering a tree cell's content at the position (row,col), returning
> +  * the imgIContainer of the drawn result.
> +  */
> +  imgIContainer draw(in nsITreeView view, in long row, in nsITreeColumn col, in AString props);

Is there seriously no better way to pass these properties than as a space-delimited string?

I'm hoping someone who works with this code more can suggest a better alternative.

Also, this comment could use rewording, and it definitely needs '@param' explanations for each parameter and an '@return' explanation of the return value. (Read up on Doxygen comments if you're not familiar with this.)

Also, a nit: it looks like the '*' on the lines you've added is aligned to the '/' in the '/*' on the first line. They need to be indented by one space; all the stars should be aligned vertically.

@@ +39,5 @@
>    attribute nsITreeView view;
>  
>    /**
> +  * The canvas handler for rich cell, to rendering anything that canvs
> +  * support, we can use this to rendering beatiful tree cell.

Same nits as the other Doxygen comment in this file: this needs rewording, and it looks like the '*'s are aligned wrong.

::: layout/xul/tree/nsTreeBodyFrame.cpp
@@ +3264,5 @@
> +    bool useImageRegion = true;
> +    nsCOMPtr<imgIContainer> image;
> +    GetImage(aRowIndex, aColumn, false, imageContext, useImageRegion, getter_AddRefs(image));
> +    PaintImage(image, aColumn, iconRect, aPresContext, aRenderingContext,
> +        imageContext, aDirtyRect, remainingWidth, currX, useImageRegion);

Nit: |imageContext| should be aligned with |image|.

@@ +3309,5 @@
> +            nsAutoString properties = nsTreeUtils::StringizeProperties(mScratchArray);
> +            mCellRenderer->Draw(mView, aRowIndex, aColumn, properties, getter_AddRefs(image));
> +          }
> +          if (image) {
> +            useImageRegion = false;

It seems like it'd make more sense to default |useImageRegion| to |false|. If you get an image from calling |mCellRenderer->Draw|, then it'll already be set to the correct value, and you won't need this line. If you don't, you'll call |GetImage()|, and that will set it to the correct value, because |GetImage()| always sets the value of |useImageRegion|.

@@ +3314,5 @@
> +          } else {
> +            GetImage(aRowIndex, aColumn, true, rendererContext, useImageRegion, getter_AddRefs(image));
> +          }
> +          PaintImage(image, aColumn, iconRect, aPresContext, aRenderingContext,
> +              rendererContext, aDirtyRect, remainingWidth, currX, useImageRegion);

Nit: It looks like |renderContext| needs to be aligned with |image| on this line.

@@ +3447,5 @@
>    imageContext->StyleMargin()->GetMargin(imageMargin);
>    imageRect.Deflate(imageMargin);
>  
> +  bool useImageRegion = aUseImageRegion;
> +  nsCOMPtr<imgIContainer> image = aImage;

Don't copy these argument values. Just replace |useImageRegion| by |aUseImageRegion| and |image| by |aImage| in the rest of the method.

::: layout/xul/tree/nsTreeBodyFrame.h
@@ +634,5 @@
>    nsTHashtable<nsPtrHashKey<nsTreeImageListener> > mCreatedListeners;
>  
> +  // This is a external tree cell render, it's will rendering the cell
> +  // content, and resulting imgIContainer, the rendering progress will
> +  // be canvas or other ways.

This is another comment that needs rewording. I hesitate to make suggestions as the XUL folks may know better, but maybe something like:

"This is an external tree cell renderer. If present, it renders the cell content into an imgIContainer, which is then painted using PaintImage."

	Colby Russell :crussell (no longer Mozilla-ing)
	Comment 181 • 10 years ago

(In reply to Seth Fowler [:seth] from comment #180)
> > +  imgIContainer draw(in nsITreeView view, in long row, in nsITreeColumn col, in AString props);
> 
> Is there seriously no better way to pass these properties than as a
> space-delimited string?
> 
> I'm hoping someone who works with this code more can suggest a better
> alternative.

The properties are communicated to the nsTreeBodyFrame from the view as a space-delimited string.  That's a recent development.  Trees used to use an nsISupportsArray, and was changed for bug 407956.  (See bug 407956, comment 6, in particular.)  The renderer implementor is almost definitely the same as the nsITreeView implementor, so they should be familiar with this, having been the one to provide the property string to begin with (minus the prefilled properties).  It's wonky, and something like DOMTokenList would be better, but only if nsITreeView.getCellProperties and friends used same data structure, which is starting to sound like it should live in another bug.

> > +  // This is a external tree cell render, it's will rendering the cell
> > +  // content, and resulting imgIContainer, the rendering progress will
> > +  // be canvas or other ways.
> 
> This is another comment that needs rewording. I hesitate to make suggestions
> as the XUL folks may know better, but maybe something like:
> 
> "This is an external tree cell renderer. If present, it renders the cell
> content into an imgIContainer, which is then painted using PaintImage."

That works.  Optionally, maybe emphasize that the rendered result is used in lieu of what nsTreeBodyFrame would have rendered itself based on what it learned from the view via |getCellText| and friends.

	Yonggang Luo
	Comment 182 • 10 years ago

(In reply to Seth Fowler [:seth] from comment #178)
> Comment on attachment 8496483 [details] [diff] [review]
> 0002-Add-imgIContainer-createImageFromCanvasContext-in-ns.patch
> 
> Review of attachment 8496483 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm the only reviewer you need on an imagelib patch. I've unset the other
> review flags.
> 
> It seems that you're adding |createImageFromCanvasContext| in order to
> support the |nsITreeCellRenderer.draw| API you've added.
> 
> That seems reasonable to me, but I need to think a little about this API
> before I r+ this.
> 
> I've added some initial review comments, though.
> 
> ::: image/public/imgITools.idl
> @@ +90,5 @@
> >                                       in long aWidth,
> >                                       in long aHeight,
> >                                       [optional] in AString outputOptions);
> >  
> > +    imgIContainer createImageFromCanvasContext(in nsISupports canvasContext);
> 
> This shouldn't take nsISupports. It needs to take a real canvas interface.
We discussed before, canvasContext is a better option,
Please take a look at comment 
https://bugzilla.mozilla.org/show_bug.cgi?id=441414#c142,
There is another reason not use canvas directly, if we use nsIDOMHTMLCanvasElement,
we also need to pass a annoy parameter contextName ('2d' or '3d'), and in fact, when
we drawing things on the context, the result thing(context) is what we need to paint to the
surface, not the Canvas.
Please check this carefully.
> 
> You also need a documentation comment here.
> 
> ::: image/src/imgTools.cpp
> @@ +311,5 @@
> >  }
> >  
> >  NS_IMETHODIMP
> > +imgTools::CreateImageFromCanvasContext(nsISupports        *aCanvasContext,
> > +                                       imgIContainer      **aContainer)
> 
> Again, this violates the style guide. Please write |nsISupports*| and
> |imgIContainer**|, with no space between the type name and the '*'.
> 
> @@ +316,5 @@
> > +{
> > +  nsCOMPtr<nsICanvasRenderingContextInternal> contextInternal =
> > +      do_QueryInterface(aCanvasContext);
> > +  if (!contextInternal) {
> > +    return NS_OK;
> 
> Should you really be returning NS_OK if the caller passed in an argument of
> the wrong type? (Though you won't need this check anyway if you fix this
> method to not take an nsISupports.)
If we pass the canvas interface, we still will got the nsISupports for nsICanvasRenderingContextInternal,
this won't bypassed, because the image is created from nsICanvasRenderingContextInternal directly, not from canvas.
> 
> @@ +318,5 @@
> > +      do_QueryInterface(aCanvasContext);
> > +  if (!contextInternal) {
> > +    return NS_OK;
> > +  }
> > +  RefPtr<mozilla::gfx::SourceSurface> surface =
> 
> You don't need "mozilla::gfx::", as we have "using mozilla::gfx" at the top
> of this file. Just write "SourceSurface".
OK

	Yonggang Luo
	Comment 183 • 10 years ago

(In reply to Seth Fowler [:seth] from comment #180)
> Comment on attachment 8496494 [details] [diff] [review]
> 0005-Fixes-for-bug-441414-to-add-rich-tree-support-crasht.patch
> 
> Review of attachment 8496494 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The image-related code in this patch looks good! I'm r+'ing that aspect of
> it. There are some minor issues, which I've pointed out below, but nothing
> that should be hard to fix.
> 
> I'm not reviewing the overall design you're using here and the tests.
> Someone more familiar with XUL code needs to do that.
> 
> If you end up having to make substantial changes to the design, I'll need to
> review again.
> 
> ::: layout/xul/tree/nsITreeBoxObject.idl
> @@ +19,5 @@
> > +  /**
> > +  * Rendering a tree cell's content at the position (row,col), returning
> > +  * the imgIContainer of the drawn result.
> > +  */
> > +  imgIContainer draw(in nsITreeView view, in long row, in nsITreeColumn col, in AString props);
> 
> Is there seriously no better way to pass these properties than as a
> space-delimited string?
As :crussell state, now TreeView are not using AtomArray to pass parameters. But using space delimited 
string instead.
> 
> I'm hoping someone who works with this code more can suggest a better
> alternative.
> 
> Also, this comment could use rewording, and it definitely needs '@param'
> explanations for each parameter and an '@return' explanation of the return
> value. (Read up on Doxygen comments if you're not familiar with this.)
> 
> Also, a nit: it looks like the '*' on the lines you've added is aligned to
> the '/' in the '/*' on the first line. They need to be indented by one
> space; all the stars should be aligned vertically.
> 
> @@ +39,5 @@
> >    attribute nsITreeView view;
> >  
> >    /**
> > +  * The canvas handler for rich cell, to rendering anything that canvs
> > +  * support, we can use this to rendering beatiful tree cell.
> 
> Same nits as the other Doxygen comment in this file: this needs rewording,
> and it looks like the '*'s are aligned wrong.
> 
> ::: layout/xul/tree/nsTreeBodyFrame.cpp
> @@ +3264,5 @@
> > +    bool useImageRegion = true;
> > +    nsCOMPtr<imgIContainer> image;
> > +    GetImage(aRowIndex, aColumn, false, imageContext, useImageRegion, getter_AddRefs(image));
> > +    PaintImage(image, aColumn, iconRect, aPresContext, aRenderingContext,
> > +        imageContext, aDirtyRect, remainingWidth, currX, useImageRegion);
> 
> Nit: |imageContext| should be aligned with |image|.
> 
> @@ +3309,5 @@
> > +            nsAutoString properties = nsTreeUtils::StringizeProperties(mScratchArray);
> > +            mCellRenderer->Draw(mView, aRowIndex, aColumn, properties, getter_AddRefs(image));
> > +          }
> > +          if (image) {
> > +            useImageRegion = false;
> 
> It seems like it'd make more sense to default |useImageRegion| to |false|.
> If you get an image from calling |mCellRenderer->Draw|, then it'll already
> be set to the correct value, and you won't need this line. If you don't,
Agreed, done.

> you'll call |GetImage()|, and that will set it to the correct value, because
> |GetImage()| always sets the value of |useImageRegion|.
> 
> @@ +3314,5 @@
> > +          } else {
> > +            GetImage(aRowIndex, aColumn, true, rendererContext, useImageRegion, getter_AddRefs(image));
> > +          }
> > +          PaintImage(image, aColumn, iconRect, aPresContext, aRenderingContext,
> > +              rendererContext, aDirtyRect, remainingWidth, currX, useImageRegion);
> 
> Nit: It looks like |renderContext| needs to be aligned with |image| on this
> line.
> 
> @@ +3447,5 @@
> >    imageContext->StyleMargin()->GetMargin(imageMargin);
> >    imageRect.Deflate(imageMargin);
> >  
> > +  bool useImageRegion = aUseImageRegion;
> > +  nsCOMPtr<imgIContainer> image = aImage;
> 
> Don't copy these argument values. Just replace |useImageRegion| by
> |aUseImageRegion| and |image| by |aImage| in the rest of the method.
Done
> 
> ::: layout/xul/tree/nsTreeBodyFrame.h
> @@ +634,5 @@
> >    nsTHashtable<nsPtrHashKey<nsTreeImageListener> > mCreatedListeners;
> >  
> > +  // This is a external tree cell render, it's will rendering the cell
> > +  // content, and resulting imgIContainer, the rendering progress will
> > +  // be canvas or other ways.
> 
> This is another comment that needs rewording. I hesitate to make suggestions
> as the XUL folks may know better, but maybe something like:
> 
> "This is an external tree cell renderer. If present, it renders the cell
> content into an imgIContainer, which is then painted using PaintImage."
done with minor tune.

	Yonggang Luo
	Comment 185 • 10 years ago

I need people familiar with XUL tree to review this patch.
Sorry for annoy so much people.

	Mats Palmgren (inactive)
	Comment 186 • 10 years ago

Comment on attachment 8497672 [details] [diff] [review]
0005-Fixes-for-bug-441414-to-add-rich-tree-support-crasht.patch

(I don't think I'm the right reviewer for this patch.)

	Alex Vincent [:WeirdAl]
	Comment 187 • 10 years ago

Comment on attachment 8497672 [details] [diff] [review]
0005-Fixes-for-bug-441414-to-add-rich-tree-support-crasht.patch

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

I think I'm going to have to compile this to see everything that's going on... there's a lot.  I don't yet feel comfortable granting feedback+, but the sample does help.

::: layout/xul/tree/crashtests/441414-richtree.xul
@@ +9,5 @@
> +var Cc = Components.classes;
> +var Ci = Components.interfaces;
> +var Cu = Components.utils;
> +
> +var gIdx=[{

Try to be consistent with braces and brackets.  Also, what does gIdx represent?  (Comments are your friend.)

@@ +30,5 @@
> +var treeView = {
> +  _rowCount: 0,
> +  updateRowCount: function() {
> +    this._rowCount = 19173966; //2147483647,19173966, 19173961.133928571428571428571429
> +  },

What is the significance of this number?  (Put it in a comment.)

@@ +38,5 @@
> +    if (aIID.equals(Ci.nsITreeView) ||
> +        aIID.equals(Ci.nsISupports))
> +      return this;
> +    throw Components.results.NS_NOINTERFACE;
> +  },

Import the XPCOMUtils module and use XPCOMUtils.generateQI here please.

@@ +52,5 @@
> +    if (idx <= 1) {
> +      node = gIdx[idx];
> +    } else {
> +      node = gIdx[0];
> +    }

Nit:  node = gIdx[idx <= 1 ? idx : 0];

@@ +100,5 @@
> +    if (node.isRead) {
> +      ret += "read ";
> +    } else {
> +      ret += "unread ";
> +    }

ret += node.isRead ? "read " : "unread ";

@@ +111,5 @@
> +};
> +
> +function onStart() {
> +  treeView.updateRowCount();
> +  var treeBoxObject= document.getElementById("threadTree");

Don't call this treeBoxObject.  Each tree has a treeBoxObject property, and it means something very different than the tree element.

@@ +120,5 @@
> +window.addEventListener("load", onStart, true);
> +
> +function initThreadTree(threadTreeId, threadTreeBodyId) {
> +  treeCellCanvas.initialize(document.getElementById(threadTreeId),
> +      document.getElementById(threadTreeBodyId));

Please put each argument on a different line, or all on the same line.  You might just want to say:

let tree = document.getElementById(threadTreeId),
    body = document.getElementById(threadTreeBodyId);
treeCellCanvas.initialize(tree, body);

@@ +176,5 @@
> +function jsdump(str) {
> +  Cc['@mozilla.org/consoleservice;1']
> +            .getService(Ci.nsIConsoleService)
> +            .logStringMessage(str);
> +}

You don't need this function.  Instead, 
Cu.import("resource://gre/modules/Services.jsm");

and then when you need to log,
Services.console.logStringMessage(str);

@@ +178,5 @@
> +            .getService(Ci.nsIConsoleService)
> +            .logStringMessage(str);
> +}
> +
> +function drawText(ctx, text, font, color, align, x, y, maxWidth, crop) {

This function needs some JavaDoc, I think.  Maybe it's obvious from reading the argument names, but:

Is align is a boolean, a string, etc...
Can x and y be decimal numbers, or must they be integers, etc.
Is ctx a 2-D canvas or a WebGL canvas?  

(Note I do *not* recommend JavaDoc on the treeview implementation above, because you've already referred to the nsITreeView interface you're implementing.)

@@ +194,5 @@
> +  for (var i = 0; i < text.length && i >= finalPos;) {
> +    var ch = text[i];
> +    //replace(/[\.,-\/#!$%\^&\*;:{}=\-_`~()]/g,"")
> +    //.,-/#!$%^&*;:{}=\-_\'~()]"
> +    //' \t\n\r\v'

I realize it's commented out, but what does it say?

@@ +224,5 @@
> +        aIID.equals(Ci.nsISupports) ||
> +        aIID.equals(Ci.nsISupportsWeakReference))
> +      return this;
> +    throw Components.results.NS_NOINTERFACE;
> +  },

Again, XPCOMUtils.generateQI is your friend.

@@ +267,5 @@
> +
> +    this.treeBoxObject.cellRenderer = this;
> +    this.treeBoxObject.invalidate();
> +  },
> +  invalidateRow: function(row) {

Please put a blank line between functions.

@@ +278,5 @@
> +    if (this.mousePos == null) {
> +      this.mousePos = {x:-1, y:-1};
> +    }
> +    switch (e.type) {
> +    default:

This switch statement is meaningless!

@@ +289,5 @@
> +
> +      var isSameColumn = posCol.value != null && posCol.value.id == this.richColumn.id;
> +
> +      let row = posRow.value;
> +      if (posCol.value == null) {

Why not just if (row.value === null)?  (Beware 0 == null, but 0 !== null.)

@@ +307,5 @@
> +    }
> +  },
> +  //nsISupports (ctx) render(in nsITreeView view, in long row, in nsITreeColumn col, in AString props);
> +  //The underlying code for view is comn\mailnews\base\src\nsMsgDBView.cpp
> +  draw: function(view, row, col, props) {

You've documented the nsITreeCellRenderer interface below.  Instead of the nsISupports... line, just say // nsITreeCellRenderer.
The reference to underlying code is fine.

Throughout this function, you mix commands to the context with gathering data from the view.  I recommend gathering all your data up front, and then send commands to the context.  Also, this method needs some comments, I think.

@@ +316,5 @@
> +    ctx.clearRect(0, 0, this.canvas.width, this.canvas.height);
> +
> +    let hasButton = this.relativeRow == row - this.treeBoxObject.getFirstVisibleRow();
> +    let hasHover = propertiesHasAtom(props, 'hover');
> +    var selected = propertiesHasAtom(props, 'selected');

Consider:

let propsArray = props.split(/\s+/);
let hasHover = props.indexOf("hover") != -1;
let selected = props.indexOf("selected") != -1;

@@ +357,5 @@
> +    var date = view.getCellText(row, this.dateColumn).split(' ')[0];
> +    var title = view.getCellText(row, this.subjectColumn).substring(0, 128);
> +
> +    ctx.beginPath();
> +    ctx.arc(500, 50, row % 10 + 10, 0, 2 * 3.14, false);

2 * Math.PI please.

@@ +404,5 @@
> +  <hbox flex="1">
> +    <tree id="threadTree" width="600" hidecolumnpicker="true">
> +      <treecols>
> +        <treecol hideheader="true" id="richCol" flex="1" type="renderer" ignoreincolumnpicker="true" />
> +        <treecol hidden="true" hideheader="true" id="senderCol" flex="1" type="rich" ignoreincolumnpicker="true" />

I forget what type="rich" means.  Most of these other attributes are new to me... interesting.

	Alex Vincent [:WeirdAl]
	Comment 188 • 10 years ago

Comment on attachment 8496484 [details] [diff] [review]
0003-Add-a-cell-column-row-type-TYPE_RENDERER-and-corresp.patch

:evilpie, feedback requested because of bug 1081692; I think your patch there might cause a conflict with this bug, which is getting some traction again thanks to Yonggang Luo.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 189 • 10 years ago

I think we should stop work on this bug. See https://groups.google.com/forum/#!topic/mozilla.dev.platform/HDJl1iBifB8. At least we should stop reviewing patches here until we have a consensus that adding features to XUL trees is a good thing.

	Tom S [:evilpie]
	Comment 190 • 10 years ago

Comment on attachment 8496484 [details] [diff] [review]
0003-Add-a-cell-column-row-type-TYPE_RENDERER-and-corresp.patch

I think you just have to add TYPE_RENDERER to the TreeColumn.webidl

	Seth Fowler [:seth] [:s2h]
	Comment 191 • 10 years ago

Comment on attachment 8497672 [details] [diff] [review]
0005-Fixes-for-bug-441414-to-add-rich-tree-support-crasht.patch

Unsetting the review flag since it seems like the consensus right now is not to do this. Feel free to rerequest review if that changes.