Last Comment Bug 407956 - Remove nsISupportsArray usage from nsITreeView
: Remove nsISupportsArray usage from nsITreeView
Status: RESOLVED FIXED
: addon-compat, dev-doc-needed
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla22
Assigned To: Trevor Saunders (:tbsaunde)
:
: Neil Deakin
Mentors:
scr
: 156387 (view as bug list)
Depends on: 871208 848265 849534 849752 866383 914055 965210
Blocks: 394167 448235 nuke-nsSupportsArray 835916 399753 840913 851834 876855
  Show dependency treegraph
 
Reported: 2007-12-11 12:59 PST by Mark Banner (:standard8, limited time in Dec)
Modified: 2014-08-13 13:49 PDT (History)
34 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP v1 (29.34 KB, patch)
2007-12-11 13:04 PST, Mark Banner (:standard8, limited time in Dec)
no flags Details | Diff | Splinter Review
make nsITreeView::GetRowProperties() not take a nsISupportsArray* (24.86 KB, patch)
2013-01-08 19:55 PST, Trevor Saunders (:tbsaunde)
bzbarsky: review+
neil: superreview+
Details | Diff | Splinter Review
bug 407956 - GetCellProperties() (42.15 KB, patch)
2013-01-16 04:05 PST, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
bug 407956 - convert GetColumnProperties (22.99 KB, patch)
2013-01-16 04:06 PST, Trevor Saunders (:tbsaunde)
neil: review+
Details | Diff | Splinter Review
bug 407956 - GetCellProperties() (46.73 KB, patch)
2013-01-16 14:44 PST, Trevor Saunders (:tbsaunde)
neil: review+
Details | Diff | Splinter Review
bug 407956 - GetCellProperties() (51.41 KB, patch)
2013-01-17 20:50 PST, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
bug 407956 - make nsITreeView not take a nsISupportsArray* (68.33 KB, patch)
2013-01-18 12:01 PST, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
WIP (16.68 KB, patch)
2013-01-18 17:18 PST, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
mailnews changes (32.66 KB, patch)
2013-01-19 13:12 PST, neil@parkwaycc.co.uk
mconley: review+
Details | Diff | Splinter Review
suite changes (29.02 KB, patch)
2013-01-19 16:33 PST, neil@parkwaycc.co.uk
iann_bugzilla: review+
Details | Diff | Splinter Review
suite changes (29.01 KB, patch)
2013-01-26 16:03 PST, neil@parkwaycc.co.uk
neil: review+
Details | Diff | Splinter Review
inspector changes (4.39 KB, patch)
2013-02-12 17:07 PST, neil@parkwaycc.co.uk
bugzilla: review+
surkov.alexander: feedback+
Details | Diff | Splinter Review
Thunderbird changes (24.05 KB, patch)
2013-02-13 13:42 PST, :aceman
mconley: feedback+
Details | Diff | Splinter Review
rebased m-c patch (69.00 KB, patch)
2013-02-19 14:01 PST, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
Thunderbird changes v2 (24.13 KB, patch)
2013-02-22 15:12 PST, :aceman
mconley: review+
Details | Diff | Splinter Review
Thunderbird changes v3 (25.46 KB, patch)
2013-03-02 04:08 PST, :aceman
acelists: review+
Details | Diff | Splinter Review

Description Mark Banner (:standard8, limited time in Dec) 2007-12-11 12:59:21 PST
nsISupportsArray is obsolete (see http://wiki.mozilla.org/Gecko:Obsolete_API), nsITreeView currently uses it in three functions:

void getRowProperties(in long index, in nsISupportsArray properties);
void getCellProperties(in long row, in nsITreeColumn col, in nsISupportsArray properties);
void getColumnProperties(in nsITreeColumn col, in nsISupportsArray properties);

Having looked at the functions, I think replacing this with nsIMutableArray is the best solution - many of the existing callers set up an array with some properites, call the relevant function and expect the data to be appended to the array.

So to keep things simple, this appears the best way to go.

Attached is a WIP patch for the c++ parts that cover FF & SeaMonkey (probably TB but I'm not sure). I need to do all the js parts yet.

If anyone has any comments, please do make them soon as I'd like to get this done as I believe it will benefit mailnews going over to frozen linkage (and even if it doesn't, it should have been done before now).

cc'ing various lead devs of different components/extensions to let you be aware. I think this should be fairly easy to detect for backwards compatibility (where appropriate).
Comment 1 Mark Banner (:standard8, limited time in Dec) 2007-12-11 13:04:28 PST
Created attachment 292652 [details] [diff] [review]
WIP v1

This is the WIP patch.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2007-12-13 14:37:54 PST
It might be good to just return the properties (as a return value, at that).  Callers who want to append these to some other array could then do so....

I'm not sure our IDL allows us to do that nicely, though.
Comment 3 neil@parkwaycc.co.uk 2007-12-13 15:08:32 PST
Both nsTreeContentView and nsXULTreeBuilder obtain a space-delimited list of properties which they then pass to a shared routine that appends the atoms.

Thus one solution is simply to return the string and let the nsTreeBodyFrame call that routine instead.
Comment 4 Mark Banner (:standard8, limited time in Dec) 2008-02-22 06:06:41 PST
I'm not working on this in the short term, especially I expect not before 1.9 is released.
Comment 5 Alex Vincent [:WeirdAl] 2009-01-15 09:23:33 PST
My two bits:  I think we need to take this a step further for Mozilla 2.  The nsISupportsArray objects here are supposed to contain nsIAtom objects.  However, tree views are scriptable, and so are nsIAtom objects.  The nsIAtom interface clearly states this is not necessarily a good thing:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpcom/ds/nsIAtom.idl&rev=&cvsroot=/cvsroot&mark=45-50#45

Why are we returning atoms through this interface anyway?

(If I should open up a new bug for this, please say so.)
Comment 6 Trevor Saunders (:tbsaunde) 2013-01-07 13:50:53 PST
(In reply to neil@parkwaycc.co.uk from comment #3)
> Both nsTreeContentView and nsXULTreeBuilder obtain a space-delimited list of
> properties which they then pass to a shared routine that appends the atoms.
> 
> Thus one solution is simply to return the string and let the nsTreeBodyFrame
> call that routine instead.

this seems reasonable to me, and I'm considering doing it. does anyone have objections?
Comment 7 Trevor Saunders (:tbsaunde) 2013-01-08 19:55:57 PST
Created attachment 699550 [details] [diff] [review]
make nsITreeView::GetRowProperties() not take a nsISupportsArray*

Hearing no objections I started doing the approach from comment 3.

I don't think there's much urgency to reviewing this, but we should probably change this GetCellProperties() and GetColumnProperties() in the same release so it would be nice if reviews happened in a time frame that allowed that.
Comment 8 :Mook 2013-01-08 21:20:35 PST
Sorry for drive-by comments; trying to think about how to support both interfaces (since there will likely be a short while where people running both will exist, possibly as a result of ESR/Thunderbird):

For JS:
Implementing: Fork behaviour depending on whether the second argument is supplied.
Calling: Pass in a nsISupportArray, and fork depending on whether a return value exists.

For C++:
Implementing: Either a tear-off or try to support both forms and hope the overloading works.
Calling: Check results of QI.

Python, because I'm crazy:
Same as JS, but implementers need a default value of None for the second arg.

Notes:
- The IDL _needs_ to change IIDs for the C++ case to work.
- The IDL probably wants an updated comment that doesn't reference atoms.

Again, apologies for the drive-by, and thanks for working to eradicate a hard-to-use interface :D
Comment 9 neil@parkwaycc.co.uk 2013-01-09 01:24:51 PST
Comment on attachment 699550 [details] [diff] [review]
make nsITreeView::GetRowProperties() not take a nsISupportsArray*

>+        if (aRow < grippyRow)
>+          return this._getRowProperties(aRow);
>+
>+          return this._getRowProperties(aRow - 1);
Nit: wrong indentation.

>         if (!raw.IsEmpty()) {
>             nsAutoString cooked;
>             SubstituteText(mRows[aIndex]->mMatch->mResult, raw, cooked);
> 
>-            nsTreeUtils::TokenizeProperties(cooked, aProperties);
>+            aProps.Append(cooked);
Just substitute text directly into aProps.

> NS_IMETHODIMP
>-inDOMView::GetRowProperties(int32_t index, nsISupportsArray *properties)
>+inDOMView::GetRowProperties(int32_t index, nsAString& aProps)
> {
>   return NS_OK;
> }
Technically I believe all C++ callers need to call aProps.Truncate(); to match the behaviour of the JS callers return ""; the exception being is if all the code paths already call Assign or pass the string to a helper function.

>   /** 
>    * An atomized list of properties for a given row.  Each property, x, that
>    * the view gives back will cause the pseudoclass :moz-tree-row-x
>    * to be matched on the pseudoelement ::moz-tree-row.
>    */
>-  void getRowProperties(in long index, in nsISupportsArray properties);
>+  AString getRowProperties(in long index);
Nit: fix the comment too!

>   if (realRow) {
>     nsAutoString properties;
>     realRow->GetAttr(kNameSpaceID_None, nsGkAtoms::properties, properties);
>     if (!properties.IsEmpty())
>-      nsTreeUtils::TokenizeProperties(properties, aProperties);
>+      aProps.Append(properties);
if (realRow)
  realRow->GetAttr(kNameSpaceID_None, nsGkAtoms::properties, aProps);
else
  aProps.Truncate();

>       if (checkMethod == "getRowProperties") {
>-        var properties = Components.classes["@mozilla.org/supports-array;1"].
>-                           createInstance(Components.interfaces.nsISupportsArray);
>-        view.getRowProperties(r, properties);
>-        actual = convertProperties(properties);
>+        actual = view.getRowProperties(r);
>       }
>       else if (checkMethod == "hasNextSibling") {
>         actual = view[checkMethod](r, r);
>       }
>       else {
>         actual = view[checkMethod](r);
>       }
Just remove the special case for getRowProperties; the default behaviour is now correct in this case.
Comment 10 neil@parkwaycc.co.uk 2013-01-09 01:47:19 PST
(In reply to Mook from comment #8)
> For JS:
> Implementing: Fork behaviour depending on whether the second argument is
> supplied.
> Calling: Pass in a nsISupportArray, and fork depending on whether a return
> value exists.
I saw one C++ caller and one JS caller, and that was a test, so... ;-)

> For C++:
> Implementing: Either a tear-off or try to support both forms and hope the
> overloading works.
Overloading should be fine, but of course you'll need to advertise both the old and new interfaces with their uuids.

> Calling: Check results of QI.
Again, I doubt that there will be any callers.

> Notes:
> - The IDL _needs_ to change IIDs for the C++ case to work.
Yeah, the patch should have had this change. Oops on me for overlooking that.
Comment 11 neil@parkwaycc.co.uk 2013-01-09 01:50:02 PST
(In reply to Trevor Saunders from comment #7)
> we should probably
> change this GetCellProperties() and GetColumnProperties() in the same release
In the same push, I hope! Unless you're volunteering to fix comm-central, DOM Inspector ChatZilla and Venkman too...
Comment 12 Trevor Saunders (:tbsaunde) 2013-01-11 07:59:04 PST
> Technically I believe all C++ callers need to call aProps.Truncate(); to
> match the behaviour of the JS callers return ""; the exception being is if
> all the code paths already call Assign or pass the string to a helper
> function.

Is that an xpcom technicality we care about though? since  you shouldn't assume the xpcom method is c++ and if its JS passing a non empty string will have no effect on what happens?
Comment 13 neil@parkwaycc.co.uk 2013-01-11 09:07:05 PST
(In reply to Trevor Saunders from comment #12)
> > Technically I believe all C++ callers need to call aProps.Truncate(); to
> > match the behaviour of the JS callers return ""; the exception being is if
> > all the code paths already call Assign or pass the string to a helper
> > function.
> 
> Is that an xpcom technicality we care about though? since you shouldn't
> assume the xpcom method is c++ and if its JS passing a non empty string will
> have no effect on what happens?

Sorry, I meant implementations, not callers, should truncate the parameter if they don't assign to it, to match the behaviour when JS returns "".
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2013-01-11 09:27:05 PST
Both XPConnect and WebIDL promise that the passed-in value will be empty to start with, for what it's worth.  So the only case where a difference is observable is C++ callers...
Comment 15 Trevor Saunders (:tbsaunde) 2013-01-11 13:40:38 PST
(In reply to neil@parkwaycc.co.uk from comment #13)
> (In reply to Trevor Saunders from comment #12)
> > > Technically I believe all C++ callers need to call aProps.Truncate(); to
> > > match the behaviour of the JS callers return ""; the exception being is if
> > > all the code paths already call Assign or pass the string to a helper
> > > function.
> > 
> > Is that an xpcom technicality we care about though? since you shouldn't
> > assume the xpcom method is c++ and if its JS passing a non empty string will
> > have no effect on what happens?
> 
> Sorry, I meant implementations, not callers, should truncate the parameter
> if they don't assign to it, to match the behaviour when JS returns "".

that's what I thought you meant, but its not really clear to me that's worth bothering with. Since we control all the C++ callers more or less, as well as it being crazy for a c++ caller to pass a non empty string.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2013-01-15 21:20:31 PST
Comment on attachment 699550 [details] [diff] [review]
make nsITreeView::GetRowProperties() not take a nsISupportsArray*

Please rev the IID and fix the comments in the IDL to reflect the new reality.

And yes, we should fix the other two consumers of nsISuportArray too, and push it all at once.

Please don't forget to mark addon-compat, and check comm-central.

r=me with that.
Comment 17 Trevor Saunders (:tbsaunde) 2013-01-16 04:05:35 PST
Created attachment 702761 [details] [diff] [review]
bug 407956 - GetCellProperties()
Comment 18 Trevor Saunders (:tbsaunde) 2013-01-16 04:06:13 PST
Created attachment 702763 [details] [diff] [review]
bug 407956 - convert GetColumnProperties
Comment 19 neil@parkwaycc.co.uk 2013-01-16 07:36:37 PST
Comment on attachment 702763 [details] [diff] [review]
bug 407956 - convert GetColumnProperties

Well that was straightforward ;-)
Comment 20 neil@parkwaycc.co.uk 2013-01-16 08:05:25 PST
Comment on attachment 702761 [details] [diff] [review]
bug 407956 - GetCellProperties()

At what point are you going to stop including nsISupports.idl in nsITreeView.idl?

Some of these included cleanup for the atoms that are no longer used, and some of these didn't. The worst case appears to be that you forgot to remove inDOMViewAtomList.h.

>         if (aRow === grippyRow)
>-          aProps.AppendElement(atomServ.getAtom("grippyRow"));
>+          return "grippyRow";
>         else if (aRow < grippyRow)
>-          this._getCellProperties(aRow, aCol, aProps);
>+          return this._getCellProperties(aRow, aCol);
>         else
>-          this._getCellProperties(aRow - 1, aCol, aProps);
>+          return this._getCellProperties(aRow - 1, aCol, aProps);
[Nit: forgot to remove aProps this time]
I know they're existing lines, but you can remove the else after return.

>             properties.push(this._getAtomFor("visited"));
>           }
>         }
>       }
> 
>       this._cellProperties.set(node, properties);
>     }
>     for (let property of properties) {
>-      aProperties.AppendElement(property);
>+      props += " " + property.toString();
It might be better to convert properties to a string and eliminate the atoms.

>         nsAutoString raw;
>         cell->GetAttr(kNameSpaceID_None, nsGkAtoms::properties, raw);
> 
>         if (!raw.IsEmpty()) {
>-            nsAutoString cooked;
>-            SubstituteText(mRows[aRow]->mMatch->mResult, raw, cooked);
>-
>-            nsTreeUtils::TokenizeProperties(cooked, aProperties);
>+            SubstituteText(mRows[aRow]->mMatch->mResult, raw, aProps);
The raw/cooked joke doesn't work so well now ;-)

>-  /**
>-   * An atomized list of properties for a given cell.  Each property, x, that
>-   * the view gives back will cause the pseudoclass :moz-tree-cell-x
>-   * to be matched on the ::moz-tree-cell pseudoelement.
>+  /** A whitespace delimited list of properties for a given cell.  Each
>+ * property, x, that the view gives back will cause the pseudoclass
>+ * :moz-tree-cell-x to be matched on the ::moz-tree-cell pseudoelement.
[Nit: indentation, also I don't like that /** comment starts on the same line]
[JFTR the pseudoclass is actually :moz-tree-cell(x) or :moz-tree-cell-text(x)]

>+  } else {
>+    aProps.Truncate();
[Nit: your other changes assume that aProps defaults to empty...]

>+      return gLockAtoms[gPrefView[index].lockCol].toString();
gLockAtoms should be turned back into strings.

>                 var className = this.mTextbox.getResultAt(aIndex).className;
>                   if ( className ) {
>-                    aProperties.AppendElement(this.createAtom(className));
>+                    return className;
Just return this.mTextbox.getResultAt(aIndex).className;

>+
>+return "";
[Nit: indentation!]
Comment 21 Trevor Saunders (:tbsaunde) 2013-01-16 14:44:43 PST
Created attachment 703046 [details] [diff] [review]
bug 407956 - GetCellProperties()
Comment 22 Trevor Saunders (:tbsaunde) 2013-01-16 14:47:56 PST
(In reply to neil@parkwaycc.co.uk from comment #20)
> Comment on attachment 702761 [details] [diff] [review]
> bug 407956 - GetCellProperties()
> 
> At what point are you going to stop including nsISupports.idl in
> nsITreeView.idl?

presumably I should in part 3 when nsISupportsArray isn't used there so I can avoid adding and removing the forward decl.  I can just fix that locally if you like.

> Some of these included cleanup for the atoms that are no longer used, and
> some of these didn't. The worst case appears to be that you forgot to remove
> inDOMViewAtomList.h.

yeah, somehow it founds its way into the previous commit. that should be sorted out now.
Comment 23 neil@parkwaycc.co.uk 2013-01-17 09:54:17 PST
Comment on attachment 703046 [details] [diff] [review]
bug 407956 - GetCellProperties()

>   mCyclerStates: [
>-    createAtom("cyclerState1"),
>-    createAtom("cyclerState2"),
>-    createAtom("cyclerState3")
>+    "cyclerState1",
>+    "cyclerState2",
>+    "cyclerState3"
[createAtom etc. cleanup?]

> var atomSvc = Components.classes["@mozilla.org/atom-service;1"]
>                         .getService(Components.interfaces.nsIAtomService);
> gImageView._ltrAtom = atomSvc.getAtom("ltr");
> gImageView._brokenAtom = atomSvc.getAtom("broken");
> 
>-gImageView.getCellProperties = function(row, col, props) {
>+gImageView.getCellProperties = function(row, col) {
>   var data = gImageView.data[row];
>   var item = gImageView.data[row][COL_IMAGE_NODE];
>+  var props = "";
>   if (!checkProtocol(data) ||
>       item instanceof HTMLEmbedElement ||
>       (item instanceof HTMLObjectElement && !item.type.startsWith("image/")))
>-    props.AppendElement(this._brokenAtom);
>+    props += "broken";
> 
>   if (col.element.id == "image-address")
>-    props.AppendElement(this._ltrAtom);
>+    props += " ltr";
>+
>+  return props;
> };
[atomSvc/_ltrAtom/_brokenAtom cleanup?]

>         var grippyRow = that.getGrippyRow();
>         if (aRow === grippyRow)
>-          aProps.AppendElement(atomServ.getAtom("grippyRow"));
>-        else if (aRow < grippyRow)
>-          this._getCellProperties(aRow, aCol, aProps);
>-        else
>-          this._getCellProperties(aRow - 1, aCol, aProps);
>+          return "grippyRow";
>+        if (aRow < grippyRow)
>+          return this._getCellProperties(aRow, aCol);
>+
>+          return this._getCellProperties(aRow - 1, aCol);
[Nit: indentation]
[atomServ cleanup?]

>-      aProperties.AppendElement(this._getAtomFor(columnType));
[_getAtomFor cleanup?]

>     let properties = this._cellProperties.get(node, null);
>     if (!properties) {
>-      properties = [];
>+      properties = "";
if (!properties) no longer works because (at least as far as I can tell) properties can validly be "" which fails the ! test.

As it happens, the code is wrong because .get only takes one argument. (It got overlooked when _cellProperties was changed from WeakMap to Map). Instead it should be using !this._cellProperties.has(node), although properties !== undefined should also work.

>-        prop.AppendElement(this._ltrAtom);
[_ltrAtom cleanup?]

>-      prop.AppendElement(this._getAtom("partial"));
[_getAtom cleanup?]

>-   * An atomized list of properties for a given cell.  Each property, x, that
>-   * the view gives back will cause the pseudoclass :moz-tree-cell-x
>-   * to be matched on the ::moz-tree-cell pseudoelement.
>+   * A whitespace delimited list of properties for a given cell.  Each
>+   * property, x, that the view gives back will cause the pseudoclass
>+   * :moz-tree-cellx and :moz-tree-cell-textx to be matched on the
>+   * ::moz-tree-cell pseudoelement.
Bah, this documentation business is harder than it looks, you should run it past bz in case I have misunderstood but I think the correct thing to say is that ::moz-tree-cell(x) and ::moz-tree-cell-text(x) are pseudoelements that are matched on the treechildren element.

>-      prop.AppendElement(kLTRAtom);
[kLTRAtom cleanup?]

>-      prop.AppendElement(kLTRAtom);
[Ditto]

>-var gLockAtoms = [gAtomService.getAtom("default"), gAtomService.getAtom("user"), gAtomService.getAtom("locked")];
>+var gLockStrings = ["default", "user", "locked"];
> // we get these from a string bundle
> var gLockStrs = [];
> var gTypeStrs = [];
Can we call those gLockProps to avoid confusion with the existing gLockStrs?
Comment 24 Marco Bonardo [::mak] 2013-01-17 09:58:27 PST
(In reply to neil@parkwaycc.co.uk from comment #23)
> >     let properties = this._cellProperties.get(node, null);
> >     if (!properties) {
> >-      properties = [];
> >+      properties = "";
> if (!properties) no longer works because (at least as far as I can tell)
> properties can validly be "" which fails the ! test.
> 
> As it happens, the code is wrong because .get only takes one argument. (It
> got overlooked when _cellProperties was changed from WeakMap to Map).
> Instead it should be using !this._cellProperties.has(node), although
> properties !== undefined should also work.

since we have to get in both cases, I think the undefined check may be faster than has+get.

also in this method you forgot to remove the aProperties argument.
Comment 25 neil@parkwaycc.co.uk 2013-01-17 10:43:10 PST
Comment on attachment 703046 [details] [diff] [review]
bug 407956 - GetCellProperties()

> /* void getCellProperties (in long row, in nsITreeColumn col,
>                            in nsISupportsArray properties); */
[Forgot to update comment]

> /* void getCellProperties (in long row, in nsITreeColumn col, 
>  *                         in nsISupportsArray properties); 
[Forgot to update comment]

>   getCellProperties : function(row,column,prop) {
[Forgot to remove prop]
Comment 26 Philipp Kewisch [:Fallen] 2013-01-17 10:58:15 PST
(In reply to neil@parkwaycc.co.uk from comment #23)
> if (!properties) no longer works because (at least as far as I can tell)
> properties can validly be "" which fails the ! test.

Actually !"" returns true in xpcshell. I've been relying on this quite often :)
Comment 27 neil@parkwaycc.co.uk 2013-01-17 12:08:15 PST
(In reply to Philipp Kewisch from comment #26)
> (In reply to neil@parkwaycc.co.uk from comment #23)
> > if (!properties) no longer works because (at least as far as I can tell)
> > properties can validly be "" which fails the ! test.
> Actually !"" returns true
Before, properties could only be an array or undefined, which ! can distinguish. But now, properties can be "", which ! can't distinguish, which is the failure of the test.
Comment 28 neil@parkwaycc.co.uk 2013-01-17 12:25:25 PST
Comment on attachment 703046 [details] [diff] [review]
bug 407956 - GetCellProperties()

r=me with above comments addressed, but I'd like to see a final patch.
Comment 29 Trevor Saunders (:tbsaunde) 2013-01-17 20:39:12 PST
> >-      aProperties.AppendElement(this._getAtomFor(columnType));
> [_getAtomFor cleanup?]

uhm, I can't tell if nodeAnnotationChanged is still using that or if we busted that here :S
Comment 30 Trevor Saunders (:tbsaunde) 2013-01-17 20:50:40 PST
Created attachment 703745 [details] [diff] [review]
bug 407956 - GetCellProperties()

Boris, can you check the comment about what getCellProperties() does is correct in the idl?
Comment 31 Boris Zbarsky [:bz] (still a bit busy) 2013-01-17 20:59:23 PST
>+   * A whitespace delimited list of properties for a given cell.  Each
>+   * property, x, that the view gives back will cause the pseudoclass
>+   * :moz-tree-cell(x) and :moz-tree-cell-text(x) to be matched on the
>+   * <treechildren> element for the tree.

That doesn't seem quite right, unless I'm misreading the code.

If I read it right, what it will do is that it will make the following pseudo-elements match for this cell: ::-moz-tree-cell(x), ::-moz-tree-row(x), ::-moz-tree-twisty(x), ::-moz-tree-image(x), ::-moz-tree-cell-text(x).

Neil, does that match what you understand of this stuff?
Comment 32 neil@parkwaycc.co.uk 2013-01-18 00:48:01 PST
(In reply to Trevor Saunders from comment #29)
> > >-      aProperties.AppendElement(this._getAtomFor(columnType));
> > [_getAtomFor cleanup?]
> 
> uhm, I can't tell if nodeAnnotationChanged is still using that or if we
> busted that here :S

We busted that here. It's looking to see whether we have cached properties and if so update them and trigger a repaint.
Comment 33 neil@parkwaycc.co.uk 2013-01-18 01:17:03 PST
(In reply to Boris Zbarsky from comment #31)
> If I read it right, what it will do is that it will make the following
> pseudo-elements match for this cell: ::-moz-tree-cell(x),
> ::-moz-tree-row(x), ::-moz-tree-twisty(x), ::-moz-tree-image(x),
> ::-moz-tree-cell-text(x).
> 
> Neil, does that match what you understand of this stuff?

Yes, except for ::-moz-tree-row(x) which is only affected by getRowProperties.
Comment 34 Marco Bonardo [::mak] 2013-01-18 03:02:17 PST
(In reply to neil@parkwaycc.co.uk from comment #32)
> (In reply to Trevor Saunders from comment #29)
> > > >-      aProperties.AppendElement(this._getAtomFor(columnType));
> > > [_getAtomFor cleanup?]
> > 
> > uhm, I can't tell if nodeAnnotationChanged is still using that or if we
> > busted that here :S
> 
> We busted that here. It's looking to see whether we have cached properties
> and if so update them and trigger a repaint.

yes, the livemark update is asynchronous, thus it has to add the "livemark" property at a later time.
Once that is fixed, _atoms, _getAtomFor and _makeAtom can go away.
Comment 35 neil@parkwaycc.co.uk 2013-01-18 07:04:09 PST
Comment on attachment 703745 [details] [diff] [review]
bug 407956 - GetCellProperties()

> /* void getRowProperties (in long index, in nsISupportsArray properties); */
> NS_IMETHODIMP 
> nsCertTree::GetRowProperties(int32_t index, nsAString& aProps)
> {
>   return NS_OK;
> }
> 
>-/* void getCellProperties (in long row, in nsITreeColumn col, 
>- *                         in nsISupportsArray properties); 
>- */
> NS_IMETHODIMP 
> nsCertTree::GetCellProperties(int32_t row, nsITreeColumn* col, 
>-                              nsISupportsArray* properties)
>+                              nsAString& aProps)
> {
>   return NS_OK;
> }
> 
> /* void getColumnProperties (in nsITreeColumn col, 
>  *                           in nsISupportsArray properties); 
>  */
Actually I was hoping for a folded patch of all three methods, including the removal of the nsISupportsArray include. That would then show up any anomalies between the various patches ;-)
Comment 36 Boris Zbarsky [:bz] (still a bit busy) 2013-01-18 07:58:55 PST
> Yes, except for ::-moz-tree-row(x) which is only affected by getRowProperties.

Maybe.  Some places that's true.  But in nsTreeBodyFrame::GetCoordsForCellItem the loop there calls GetPseudoStyleContext(nsCSSAnonBoxes::moztreerow) immediately after calling GetCellProperties, so that will be getting the -moz-tree-row pseudo with the properties set to those of the cel, afaict.  And then using some border/padding information from the result...
Comment 37 neil@parkwaycc.co.uk 2013-01-18 08:27:59 PST
(In reply to Boris Zbarsky from comment #36)
> in nsTreeBodyFrame::GetCoordsForCellItem the loop there calls
> GetPseudoStyleContext(nsCSSAnonBoxes::moztreerow) immediately after calling
> GetCellProperties, so that will be getting the -moz-tree-row pseudo with the
> properties set to those of the cell, afaict.  And then using some
> border/padding information from the result...

That smells like a bug to me...
Comment 38 neil@parkwaycc.co.uk 2013-01-18 08:39:34 PST
(In reply to comment #37)
> (In reply to Boris Zbarsky from comment #36)
> > in nsTreeBodyFrame::GetCoordsForCellItem the loop there calls
> > GetPseudoStyleContext(nsCSSAnonBoxes::moztreerow) immediately after calling
> > GetCellProperties, so that will be getting the -moz-tree-row pseudo with the
> > properties set to those of the cell, afaict.  And then using some
> > border/padding information from the result...
> 
> That smells like a bug to me...

Filed bug 832339.
Comment 39 Trevor Saunders (:tbsaunde) 2013-01-18 10:29:32 PST
(In reply to neil@parkwaycc.co.uk from comment #35)
> Comment on attachment 703745 [details] [diff] [review]
> bug 407956 - GetCellProperties()
> 
> > /* void getRowProperties (in long index, in nsISupportsArray properties); */
> > NS_IMETHODIMP 
> > nsCertTree::GetRowProperties(int32_t index, nsAString& aProps)
> > {
> >   return NS_OK;
> > }
> > 
> >-/* void getCellProperties (in long row, in nsITreeColumn col, 
> >- *                         in nsISupportsArray properties); 
> >- */
> > NS_IMETHODIMP 
> > nsCertTree::GetCellProperties(int32_t row, nsITreeColumn* col, 
> >-                              nsISupportsArray* properties)
> >+                              nsAString& aProps)
> > {
> >   return NS_OK;
> > }
> > 
> > /* void getColumnProperties (in nsITreeColumn col, 
> >  *                           in nsISupportsArray properties); 
> >  */
> Actually I was hoping for a folded patch of all three methods, including the
> removal of the nsISupportsArray include. That would then show up any
> anomalies between the various patches ;-)

yeah, I was sort of planning to post that anyway (but after I finished fixing the places thing and then looking over it myself)
Comment 40 Trevor Saunders (:tbsaunde) 2013-01-18 12:01:49 PST
Created attachment 704022 [details] [diff] [review]
bug 407956 - make nsITreeView not take a nsISupportsArray*
Comment 41 neil@parkwaycc.co.uk 2013-01-18 13:41:43 PST
Comment on attachment 704022 [details] [diff] [review]
bug 407956 - make nsITreeView not take a nsISupportsArray*

>-            let properties = this._cellProperties.get(aNode, null);
>-            if (properties)
>-              properties.push(this._getAtomFor("livemark"));
>+            let properties = this._cellProperties.get(aNode);
>+            this._cellProperties.set(aNode, properties += " livemark ");
Need to check that properties !== undefined first.

>   /** 
>-   * An atomized list of properties for a given row.  Each property, x, that
>-   * the view gives back will cause the pseudoclass :moz-tree-row-x
>-   * to be matched on the pseudoelement ::moz-tree-row.
>+   * A whitespace delimited list of properties.  For each property X the view
>+   * gives back will cause the pseudoclasses  ::-moz-tree-cell(x),
>+   * ::-moz-tree-row(x), ::-moz-tree-twisty(x), ::-moz-tree-image(x),
>+   * ::-moz-tree-cell-text(x).  to be matched on the pseudoelement
>+   * ::moz-tree-row.
>    */
>-  void getRowProperties(in long index, in nsISupportsArray properties);
>+  AString getRowProperties(in long index);
Oops, you copied the getCellProperties comment...

>-/* void getRowProperties (in long index, in nsISupportsArray properties); */
[Not that these comments were very informative, since they just duplicate the idl]
Comment 42 neil@parkwaycc.co.uk 2013-01-18 17:18:49 PST
Created attachment 704156 [details] [diff] [review]
WIP

some comm-central C++ fixes
Comment 43 neil@parkwaycc.co.uk 2013-01-19 13:12:09 PST
Created attachment 704261 [details] [diff] [review]
mailnews changes
Comment 44 neil@parkwaycc.co.uk 2013-01-19 13:25:39 PST
Comment on attachment 704022 [details] [diff] [review]
bug 407956 - make nsITreeView not take a nsISupportsArray*

inDOMView.h and nsAutoCompleteController.h both need #include "nsCOMPtr.h"
Comment 45 neil@parkwaycc.co.uk 2013-01-19 16:33:04 PST
Created attachment 704280 [details] [diff] [review]
suite changes
Comment 46 Ian Neal 2013-01-20 08:45:33 PST
what m-c bugs need to land (or be installed locally) before I can test this?
Comment 47 Ian Neal 2013-01-20 09:24:32 PST
With the 3 patches from here applied to current trunk I get the following when trying to build:
In file included from /home/gizmo/central/comm-central/mozilla/layout/inspector/src/inDOMView.cpp:8:
/home/gizmo/central/comm-central/mozilla/layout/inspector/src/inDOMView.h:41:30: error: implicit instantiation of undefined template 'nsCOMPtr<nsITreeBoxObject>'
  nsCOMPtr<nsITreeBoxObject> mTree;
                             ^
../../../dist/include/nsTArray.h:237:26: note: template is declared here
template <class T> class nsCOMPtr;
                         ^
In file included from /home/gizmo/central/comm-central/mozilla/layout/inspector/src/inDOMView.cpp:8:
/home/gizmo/central/comm-central/mozilla/layout/inspector/src/inDOMView.h:42:30: error: implicit instantiation of undefined template 'nsCOMPtr<nsITreeSelection>'
  nsCOMPtr<nsITreeSelection> mSelection;
                             ^
../../../dist/include/nsTArray.h:237:26: note: template is declared here
template <class T> class nsCOMPtr;
                         ^
In file included from /home/gizmo/central/comm-central/mozilla/layout/inspector/src/inDOMView.cpp:8:
/home/gizmo/central/comm-central/mozilla/layout/inspector/src/inDOMView.h:43:25: error: implicit instantiation of undefined template 'nsCOMPtr<inIDOMUtils>'
  nsCOMPtr<inIDOMUtils> mDOMUtils;
                        ^
../../../dist/include/nsTArray.h:237:26: note: template is declared here
template <class T> class nsCOMPtr;
                         ^
In file included from /home/gizmo/central/comm-central/mozilla/layout/inspector/src/inDOMView.cpp:8:
/home/gizmo/central/comm-central/mozilla/layout/inspector/src/inDOMView.h:51:24: error: implicit instantiation of undefined template 'nsCOMPtr<nsIDOMNode>'
  nsCOMPtr<nsIDOMNode> mRootNode;
                       ^
../../../dist/include/nsTArray.h:237:26: note: template is declared here
template <class T> class nsCOMPtr;
                         ^
In file included from /home/gizmo/central/comm-central/mozilla/layout/inspector/src/inDOMView.cpp:8:
/home/gizmo/central/comm-central/mozilla/layout/inspector/src/inDOMView.h:52:28: error: implicit instantiation of undefined template 'nsCOMPtr<nsIDOMDocument>'
  nsCOMPtr<nsIDOMDocument> mRootDocument;
                           ^
../../../dist/include/nsTArray.h:237:26: note: template is declared here
template <class T> class nsCOMPtr;
                         ^
5 errors generated.
Comment 48 neil@parkwaycc.co.uk 2013-01-20 09:51:59 PST
(In reply to Ian Neal from comment #46)
> what m-c bugs need to land (or be installed locally) before I can test this?

Attachments, not bugs, but as per comment #44, if you use 704022 then you need to tweak two files manually. Alternatively, try using 699550, 703745 and 702763 in that order.

You also need to apply 704261 before you can test 704280.
Comment 49 Ian Neal 2013-01-20 10:30:08 PST
(In reply to neil@parkwaycc.co.uk from comment #48)
> (In reply to Ian Neal from comment #46)
> > what m-c bugs need to land (or be installed locally) before I can test this?
> 
> Attachments, not bugs, but as per comment #44, if you use 704022 then you
> need to tweak two files manually. Alternatively, try using 699550, 703745
> and 702763 in that order.
> 
> You also need to apply 704261 before you can test 704280.

Yes, that fixed it - manually adding that line to the two files. The older, obsolete patches did not apply cleanly.
Comment 50 :aceman 2013-01-20 11:25:06 PST
Will probably need update of docs at https://developer.mozilla.org/en-US/docs/XUL/Tutorial/Styling_a_Tree and similar.
Comment 51 Ian Neal 2013-01-26 08:27:43 PST
Comment on attachment 704280 [details] [diff] [review]
suite changes

>+++ b/suite/browser/pageinfo/pageInfo.js
>+  getRowProperties: function(row) { return "" },
Nit: return "";
>+  getCellProperties: function(row, column) { return "" },
Nit: return "";
>+  getColumnProperties: function(column) { return "" },
Nit: return "";

>+++ b/suite/common/dataman/dataman.js
>+  getRowProperties: function(aRow) { return ""; },
>+  getColumnProperties: function(aColumn) { return ""; },
>+  getCellProperties: function(aRow, aColumn)  return ""; {}
Nit: getCellProperties: function(aRow, aColumn) { return ""; }

>+++ b/suite/mailnews/search/FilterListDialog.js

>+  getRowProperties: function getRowProperties(index) {
>+    return this.mFilterList.getFilterAt(index).enabled ? "Enabled-true" : "";
>   },
>+  getCellProperties: function getCellProperties(row, col) {
>+    return this.mFilterList.getFilterAt(index).enabled ? "Enabled-true" : "";
Where does index get set for this?

This is on code inspection only, testing still to be done.
Comment 52 neil@parkwaycc.co.uk 2013-01-26 09:24:50 PST
(In reply to Ian Neal from comment #51)
> >+  getCellProperties: function(aRow, aColumn)  return ""; {}
> Nit: getCellProperties: function(aRow, aColumn) { return ""; }
Oops. copy & paste error - didn't look carefully where I clicked before pasting.

> >+  getRowProperties: function getRowProperties(index) {
> >+    return this.mFilterList.getFilterAt(index).enabled ? "Enabled-true" : "";
> >   },
> >+  getCellProperties: function getCellProperties(row, col) {
> >+    return this.mFilterList.getFilterAt(index).enabled ? "Enabled-true" : "";
> Where does index get set for this?
Copy & paste fail again, I overlooked the different parameter names; I'll probably change them all from index to row to be consistent.
Comment 53 Ian Neal 2013-01-26 11:23:13 PST
Comment on attachment 704280 [details] [diff] [review]
suite changes

r=me with those things fixed.
Comment 54 neil@parkwaycc.co.uk 2013-01-26 16:03:34 PST
Created attachment 706814 [details] [diff] [review]
suite changes

Updated for review comments.
Comment 55 Trevor Saunders (:tbsaunde) 2013-02-12 13:56:28 PST
mconley any idea when you'll be able to review this? ideally I'd like to be able to land the m-c bits of this some time next week after the merge.
Comment 56 Mike Conley (:mconley) 2013-02-12 14:49:16 PST
Comment on attachment 704261 [details] [diff] [review]
mailnews changes

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

This looks mostly good (though I'm not in a position to try these patches). I'd certainly like to see an all-green try run before I r+ this. Also, I have a few questions. See below.

::: mailnews/addrbook/src/nsAbView.cpp
@@ +317,5 @@
>    nsresult rv = card->GetIsMailList(&isMailList);
>    NS_ENSURE_SUCCESS(rv,rv);
>  
> +  if (isMailList)
> +    properties.AssignLiteral("MailList");

This is one of several places in this patch where we've gone from additive (AppendElement) to a full-overwrite (AssignLiteral).

Are you sure this is the desired behaviour? Are there edge cases we're missing here where we'd want to *add* "MailList"?

::: mailnews/base/src/nsMsgDBView.cpp
@@ +1377,5 @@
>      ClearHdrCache();
>      return NS_MSG_INVALID_DBVIEW_INDEX;
>    }
>  
> +  const PRUnichar* colID;

So, we used to append our atoms, and *then* call custom column handlers. Now we're calling them before. Why?

::: mailnews/extensions/newsblog/content/feed-subscriptions.js
@@ +168,5 @@
>      getCellProperties: function (aRow, aColumn, aProperties) {
>  //      aProperties.AppendElement(this._getAtomFor("folderNameCol"));
>        let item = this.getItemAtIndex(aRow);
>        let folder = item && item.folder ? item.folder : null;
> +      return !folder ? "serverType-rss" :

Youch - not a big fan of nested ternary operations.

I'd prefer this:

if (!folder) {
  return "serverType-rss";
}

return folder.isServer ? "serverType=rss isServer-true" : "livemark";
Comment 57 neil@parkwaycc.co.uk 2013-02-12 15:58:19 PST
(In reply to Mike Conley from comment #56)
> I'd certainly like to see an all-green try run before I r+ this.
That's unlikely, as this is only the changes to shared code; you'd want some Thunderbird-only changes too.

> > +  if (isMailList)
> > +    properties.AssignLiteral("MailList");
> This is one of several places in this patch where we've gone from additive
> (AppendElement) to a full-overwrite (AssignLiteral).
The old code passed in a partly-filled array, and expected you to add your own atoms to it. The new code expects you to return a string, which it then atomises and adds to its internal array. The way this works in C++ is that you get given a string that you are expected to fill in; as it happens you are provided with an empty string although XPCOM doesn't guarantee this.

> So, we used to append our atoms, and *then* call custom column handlers. Now
> we're calling them before. Why?
Because our custom column handler returns a string, which we need to concatenate with our own keywords. It's easier to do this if we pass the original string to the custom column handler rather than mess about with temporary strings (which we're forced to do with rows, since we have to concatenate all of the column properties, sigh...)

> >        let item = this.getItemAtIndex(aRow);
> >        let folder = item && item.folder ? item.folder : null;
> > +      return !folder ? "serverType-rss" :
> Youch - not a big fan of nested ternary operations.
Splinter quoted this badly so I couldn't see what you were getting at. How about:
if (!item || !item.folder)
  return "serverType-rss";

return item.folder.isServer ? "serverType=rss isServer-true" : "livemark";
Comment 58 alexander :surkov 2013-02-12 16:42:53 PST
Can you fix DOMi please?
Comment 59 neil@parkwaycc.co.uk 2013-02-12 17:07:04 PST
Created attachment 713196 [details] [diff] [review]
inspector changes

I think this suffices. (Obviously there's some back compat code left behind.)
Comment 60 alexander :surkov 2013-02-12 19:24:39 PST
Comment on attachment 713196 [details] [diff] [review]
inspector changes

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

thank you!

::: resources/content/jsutil/xul/inBaseTreeView.js
@@ +21,5 @@
>    setTree: function(aTree) { this.mTree = aTree; },
>    getCellText: function(aRow, aCol) { return ""; },
> +  getRowProperties: function(aIndex, aProperties) { return ""; },
> +  getCellProperties: function(aIndex, aCol, aProperties) { return ""; },
> +  getColumnProperties: function(aCol, aProperties) { return ""; },

it's for backward compatibility, right?
Comment 61 neil@parkwaycc.co.uk 2013-02-13 00:30:33 PST
(In reply to alexander surkov from comment #60)
> (From update of attachment 713196 [details] [diff] [review])
> > +  getRowProperties: function(aIndex, aProperties) { return ""; },
> > +  getCellProperties: function(aIndex, aCol, aProperties) { return ""; },
> > +  getColumnProperties: function(aCol, aProperties) { return ""; },
> it's for backward compatibility, right?
No, forward compatibility; the current code doesn't need you to do anything, the new code expects you to return a string.
Comment 62 alexander :surkov 2013-02-13 00:41:54 PST
(In reply to neil@parkwaycc.co.uk from comment #61)
> (In reply to alexander surkov from comment #60)
> > (From update of attachment 713196 [details] [diff] [review])
> > > +  getRowProperties: function(aIndex, aProperties) { return ""; },
> > > +  getCellProperties: function(aIndex, aCol, aProperties) { return ""; },
> > > +  getColumnProperties: function(aCol, aProperties) { return ""; },
> > it's for backward compatibility, right?
> No, forward compatibility; 

ok :) perhaps it depends on the order of patch landing; if your patch goes before Trevor's one then it's forward compatibility, otherwise it sounds like backward compatibility.
Comment 63 :aceman 2013-02-13 01:07:10 PST
So who has the TB changes in queue? :)

What is the proper way now to query if a property (string) is in the list? properties.contains("property") may not be unambiguous. There is one example in the suite changes like this:
'/\bsubscribed\b/.test(properties)'. But that look ugly.
Comment 64 neil@parkwaycc.co.uk 2013-02-13 01:48:30 PST
(In reply to aceman from comment #63)
> So who has the TB changes in queue? :)
Congratulations, you just volunteered ;-)

> What is the proper way now to query if a property (string) is in the list?
> properties.contains("property") may not be unambiguous. There is one example
> in the suite changes like this:
> '/\bsubscribed\b/.test(properties)'. But that look ugly.
Well, maybe the existing code wasn't right either, and we should be using gSubscribableServer.isSubscribed(gSearchView.getCellText(row, gSearchTree.columns.nameColumn2)) instead.
Comment 65 :aceman 2013-02-13 02:57:47 PST
(In reply to neil@parkwaycc.co.uk from comment #64)
> (In reply to aceman from comment #63)
> > So who has the TB changes in queue? :)
> Congratulations, you just volunteered ;-)
Yes, I could try, just trying to make sure I'll not collide with anybody.
So I should apply the core patch, then mailnews patch and work on top of that?

> > What is the proper way now to query if a property (string) is in the list?
> > properties.contains("property") may not be unambiguous. There is one example
> > in the suite changes like this:
> > '/\bsubscribed\b/.test(properties)'. But that look ugly.
I'm asking for a universal way to query if a property is in the string. Something like element.classList is. But if this propertylist has no such accessor, what about properties.split(" ").indexOf("property") ?
Comment 66 neil@parkwaycc.co.uk 2013-02-13 05:27:30 PST
(In reply to aceman from comment #65)
> So I should apply the core patch, then mailnews patch and work on top of
> that?
Yes, that will get you started.

> what about properties.split(" ").indexOf("property") ?
Sure, that would work.
Comment 67 Mike Conley (:mconley) 2013-02-13 09:02:00 PST
(In reply to neil@parkwaycc.co.uk from comment #57)
> (In reply to Mike Conley from comment #56)
> > I'd certainly like to see an all-green try run before I r+ this.
> That's unlikely, as this is only the changes to shared code; you'd want some
> Thunderbird-only changes too.
> 

Is there someone assigned to take on this work?

> > > +  if (isMailList)
> > > +    properties.AssignLiteral("MailList");
> > This is one of several places in this patch where we've gone from additive
> > (AppendElement) to a full-overwrite (AssignLiteral).
> The old code passed in a partly-filled array, and expected you to add your
> own atoms to it. The new code expects you to return a string, which it then
> atomises and adds to its internal array. The way this works in C++ is that
> you get given a string that you are expected to fill in; as it happens you
> are provided with an empty string although XPCOM doesn't guarantee this.
> 

Ok, got it.

> > So, we used to append our atoms, and *then* call custom column handlers. Now
> > we're calling them before. Why?
> Because our custom column handler returns a string, which we need to
> concatenate with our own keywords. It's easier to do this if we pass the
> original string to the custom column handler rather than mess about with
> temporary strings (which we're forced to do with rows, since we have to
> concatenate all of the column properties, sigh...)
> 

Got it.

> > >        let item = this.getItemAtIndex(aRow);
> > >        let folder = item && item.folder ? item.folder : null;
> > > +      return !folder ? "serverType-rss" :
> > Youch - not a big fan of nested ternary operations.
> Splinter quoted this badly so I couldn't see what you were getting at. How
> about:
> if (!item || !item.folder)
>   return "serverType-rss";
> 
> return item.folder.isServer ? "serverType=rss isServer-true" : "livemark";

I'm fine with this.
Comment 68 Mike Conley (:mconley) 2013-02-13 09:02:54 PST
Comment on attachment 704261 [details] [diff] [review]
mailnews changes

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

These changes look fine, but I'll feel better once I see a corresponding TB patch. :)
Comment 69 Mike Conley (:mconley) 2013-02-13 09:03:20 PST
(In reply to Mike Conley (:mconley) from comment #68)
> Comment on attachment 704261 [details] [diff] [review]
> mailnews changes
> 
> Review of attachment 704261 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> These changes look fine

Modulo the nested ternary nit I pointed out.
Comment 70 :aceman 2013-02-13 10:26:05 PST
(In reply to Mike Conley (:mconley) from comment #67)
> > That's unlikely, as this is only the changes to shared code; you'd want some
> > Thunderbird-only changes too.
> Is there someone assigned to take on this work?

Looks like that's me! :) I'll tell you in some hours if I can do it.
Comment 71 :aceman 2013-02-13 10:44:04 PST
The core patch does not apply cleanly, the files nsITreeView.idl, nsTreeBodyFrame.cpp, nsTreeContentView.cpp have moved.
Comment 72 Alex Vincent [:WeirdAl] 2013-02-13 11:00:56 PST
(In reply to :aceman from comment #71)
> the files nsITreeView.idl, nsTreeBodyFrame.cpp, nsTreeContentView.cpp have moved.

To layout/xul/tree, in bug 833879.  It was a simple move, no actual code changes; my apologies for not notifying people in this bug.
Comment 73 :aceman 2013-02-13 11:21:05 PST
Why do I still get the breakage from comment 47? Are the tweaks from comment 48 still needed?
Comment 74 :aceman 2013-02-13 13:42:57 PST
Created attachment 713615 [details] [diff] [review]
Thunderbird changes

So this looks like the needed TB changes. However, when I run it manually, the folders in the folder pane do not get styled (with the special icons - Inbox, Draft, secure server, etc.). Do the special CSS selectors apply properly with the new properties?

Tests do pass normally for me (on Linux), I just visually see something is broken.
Comment 75 neil@parkwaycc.co.uk 2013-02-13 16:35:04 PST
(In reply to aceman from comment #74)
> So this looks like the needed TB changes. However, when I run it manually,
> the folders in the folder pane do not get styled (with the special icons -
> Inbox, Draft, secure server, etc.). Do the special CSS selectors apply
> properly with the new properties?
You shouldn't have to change the CSS at all. Is it just this one tree that is broken? (SeaMonkey has lots of trees in its UI so I got plenty of practice!)
Comment 76 :aceman 2013-02-14 00:07:26 PST
I do not change the CSS but the styles do not apply in the folder pane. In the account manager, the styling does apply fine (the default account is bold via a property and I haven't changed anything there in this patch). I didn't notice any other breakage so far. So I probably broke getProperties in folderPane.js or something like that.
Comment 77 Colby Russell :crussell (no longer Mozilla-ing) 2013-02-16 09:29:33 PST
Comment on attachment 713196 [details] [diff] [review]
inspector changes

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

::: resources/content/jsutil/xul/inBaseTreeView.js
@@ +21,5 @@
>    setTree: function(aTree) { this.mTree = aTree; },
>    getCellText: function(aRow, aCol) { return ""; },
> +  getRowProperties: function(aIndex, aProperties) { return ""; },
> +  getCellProperties: function(aIndex, aCol, aProperties) { return ""; },
> +  getColumnProperties: function(aCol, aProperties) { return ""; },

Please add a comment referencing this bug and explaining that |aProperties| is for versions of Gecko < whatever.0
Comment 78 Trevor Saunders (:tbsaunde) 2013-02-19 14:01:59 PST
Created attachment 715694 [details] [diff] [review]
rebased m-c patch

https://tbpl.mozilla.org/?tree=Try&rev=96af5f83866e
Comment 79 Mike Conley (:mconley) 2013-02-21 20:28:30 PST
Comment on attachment 713615 [details] [diff] [review]
Thunderbird changes

(In reply to :aceman from comment #74)
> Created attachment 713615 [details] [diff] [review]
> Thunderbird changes
> 
> So this looks like the needed TB changes. However, when I run it manually,
> the folders in the folder pane do not get styled (with the special icons -
> Inbox, Draft, secure server, etc.). Do the special CSS selectors apply
> properly with the new properties?
> 
> Tests do pass normally for me (on Linux), I just visually see something is
> broken.


aceman:

The icons are appearing in the folder pane view just fine for me - I think this patch is doing the job. Can you confirm? If so, feedback+.

-Mike
Comment 80 :aceman 2013-02-22 15:12:45 PST
Created attachment 717362 [details] [diff] [review]
Thunderbird changes v2
Comment 81 Mike Conley (:mconley) 2013-02-28 15:24:58 PST
Comment on attachment 717362 [details] [diff] [review]
Thunderbird changes v2

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

aceman:

Patch looks good. I'm a little disturbed with the icon breakage you reported, but if it occurs, we'll deal with it in a follow-up.

Great job,

-Mike

::: mail/components/im/content/chat-messenger-overlay.js
@@ +1059,5 @@
>    get id() this.log.title,
>    get open() false,
>    get level() this._level,
>    get children() [],
> +  getProperties: function() { return ""; }

I think Florian prefers if you do it like this:

getProperties: function() ""

::: mailnews/base/util/folderUtils.jsm
@@ +55,1 @@
>    function addAtom(aName) {

I don't think this function adds much value. Tear it out, and replace usage of it with properties.push("whichever").

::: mailnews/extensions/newsblog/content/feed-subscriptions.js
@@ +156,1 @@
>  //      aProperties.AppendElement(this._getAtomFor("folderNameCol"));

This commented out code makes even less sense with our change here. Please remove it while you're here.
Comment 82 :aceman 2013-03-02 04:08:56 PST
Created attachment 720267 [details] [diff] [review]
Thunderbird changes v3

OK, updated.
Comment 83 :aceman 2013-03-02 04:09:33 PST
Is this ready to check in?
Comment 84 Trevor Saunders (:tbsaunde) 2013-03-03 08:27:16 PST
(In reply to :aceman from comment #83)
> Is this ready to check in?

should be, my week got busy or something :/

do c-c folks care about landing at the same time as the m-c part?  If so I could probably watch the tree this evening or  tomorrow.
Comment 85 :aceman 2013-03-03 08:41:29 PST
I think this should land at the same time otherwise c-c will get broken. I think RyanVM can check into all necessary repos.
Comment 86 Trevor Saunders (:tbsaunde) 2013-03-04 11:19:25 PST
https://hg.mozilla.org/mozilla-central/rev/bf88a316cf18
Comment 87 Trevor Saunders (:tbsaunde) 2013-03-04 11:40:59 PST
thunderbird part seems to have bit rooted but I'll fix that up next, but figured I'd push these first
remote:   https://hg.mozilla.org/comm-central/rev/e17faa6751ab
remote:   https://hg.mozilla.org/comm-central/rev/1e654d550c90
Comment 88 Trevor Saunders (:tbsaunde) 2013-03-04 11:48:00 PST
actually I think it just had a dep on the mailnews/ change which I didn't realize https://hg.mozilla.org/comm-central/rev/9701dcb19f53
Comment 89 Trevor Saunders (:tbsaunde) 2013-03-04 11:52:34 PST
finally https://hg.mozilla.org/dom-inspector/rev/a66d89b48c7e
Comment 90 Nils Maier [:nmaier] 2013-03-08 07:04:31 PST
Any suggestion on how to best feature-detect this without always checking for the old props argument to be undefined?
Comment 91 Nils Maier [:nmaier] 2013-03-08 07:46:17 PST
(In reply to Nils Maier [:nmaier] from comment #90)
> Any suggestion on how to best feature-detect this without always checking
> for the old props argument to be undefined?

Nevermind that.

Doing the following will detect "legacy" nsITreeView at least from mozilla1.9.2 to this change (the interface didn't change in ages) and thus is good enough for me:
if (Components.interfacesByID["{C06DC4D3-63A2-4422-A0A3-5F2EDDECA8C1}"]) {
  // use legacy implementations
  this.getCellProperties = this.getCellProperties_legacy;
  this.getColumnProperties = this.getColumnProperties_legacy;
  this.getRowProperties = this.getRowProperties_legacy;
}

Jorge, you might want to consider mentioning that in your next compatibility post, seeing that addons mxr yields quite a few results:
https://mxr.mozilla.org/addons/ident?i=getCellProperties

Of course, checking !aProperties does work, too, although didn't profile that well in my code base (DownThemAll!). See domi changeset from comment #89
Comment 92 JoeG 2013-03-09 04:03:15 PST
Just for the record, this has found its way into the Fx Nightly and has broken the FireFTP addon.
--------------------------------
As of 05 March 2013 ...
http://edu-net.net/images/fire_ftp_problem.jpg

y-koron explained in the Fx Forum http://forums.mozillazine.org/viewtopic.php?f=23&t=2672981 that ...
---------------------
Sadly, https://bugzilla.mozilla.org/show_bug.cgi?id=407956 changed the spec of functions need to access treeview properties.

This change affects some add-ons using treeview, and in many cases, their appearance isn't  applied correctly.

The problem is code like 'props.AppendElement(gAtomService.getAtom("xxx"));' no longer works. To fix it, devs need to change it to 'return "xxx";'.
--------------------------------
I sort of understand what the issue here is and see its relevance for TBird. However, other than the generally good principle of getting rid of deprecated code, I'm not knowledgeable enough to understand why this change was necessary or desirable in the Fx code.

Wouldn't it be possible to to restrict it to TBird?
Comment 93 JoeG 2013-03-09 04:03:50 PST
Just for the record, this has found its way into the Fx Nightly and has broken the FireFTP addon.
--------------------------------
As of 05 March 2013 ...
http://edu-net.net/images/fire_ftp_problem.jpg

y-koron explained in the Fx Forum http://forums.mozillazine.org/viewtopic.php?f=23&t=2672981 that ...
---------------------
Sadly, https://bugzilla.mozilla.org/show_bug.cgi?id=407956 changed the spec of functions need to access treeview properties.

This change affects some add-ons using treeview, and in many cases, their appearance isn't  applied correctly.

The problem is code like 'props.AppendElement(gAtomService.getAtom("xxx"));' no longer works. To fix it, devs need to change it to 'return "xxx";'.
--------------------------------
I sort of understand what the issue here is and see its relevance for TBird. However, other than the generally good principle of getting rid of deprecated code, I'm not knowledgeable enough to understand why this change was necessary or desirable in the Fx code.

Wouldn't it be possible to restrict it to TBird?
Comment 94 JoeG 2013-03-09 04:06:55 PST
Sorry for posting twice. 

I know there's been a LOT of discussion about the issue, but this is just another example of why it's time to allow posters to edit or remove submissions.
Comment 95 Alex Vincent [:WeirdAl] 2013-03-09 08:09:42 PST
(In reply to Joe Greenman from comment #93)
> Just for the record, this has found its way into the Fx Nightly and has
> broken the FireFTP addon.
> --------------------------------
> As of 05 March 2013 ...
> http://edu-net.net/images/fire_ftp_problem.jpg

I noticed the same regression, but I think it's not tied to the nsISupportsArray.  I've been working on creating a TreeViews.jsm module for generic use, where I'd written a generic conversion algorithm from string to nsISupportsArray.  I removed that conversion, and it's still busted.

That said, this regression belongs on a new bug, I think.
Comment 96 JoeG 2013-03-09 09:11:27 PST
(In reply to Alex Vincent [:WeirdAl] from comment #95)
> I noticed the same regression, but I think it's not tied to the
> nsISupportsArray.  ...

> That said, this regression belongs on a new bug, I think.

I plead innocent; I simply took y-koron's word for it. I'm definitely not a programmer.

Just for the record, I've informed the dev of FireFTP of y-koron's assumption, and he subsequently  acknowledged my mail. 

OK, I'll file a bug.
Comment 97 JoeG 2013-03-09 10:17:17 PST
Filed Bug 849534
Comment 98 Boris Zbarsky [:bz] (still a bit busy) 2013-03-11 11:14:14 PDT
> Wouldn't it be possible to restrict it to TBird?

This is a core Gecko API, so not really, no.  Furthermore, this is not in any way Thunderbird-specific....

> I'm not knowledgeable enough to understand why this change was necessary or desirable

It allows us to move on with removing nsISupportsArray and hence simplifying a bunch of other code that has to deal with its existence, if nothing else.
Comment 99 neil@parkwaycc.co.uk 2013-07-05 02:36:58 PDT
*** Bug 156387 has been marked as a duplicate of this bug. ***
Comment 100 Axel Grude [:realRaven] 2014-03-12 01:55:45 PDT
I am using GetCellProperties for the Thunderbird (24.3.0) folder tree in the new way (2 parameters, returning properties and adding my custom property for adding a folder Icon) but it breaks the folder pane: special icons are unstyled and unread folders are not bold any more. Can somebody point out what is wrong with this code:

  if (QuickFolders.FolderTree.GetCellProperties) return; // already defined!
  QuickFolders.FolderTree.GetCellProperties = gFolderTreeView.getCellProperties;  
  gFolderTreeView.getCellProperties = function(row, col) {
    let props = QuickFolders.FolderTree.GetCellProperties(row, col);
    if (col.id == "folderNameCol") {
      let folder = gFolderTreeView.getFolderForIndex(row);
      let folderIcon;
      if ( folderIcon = folder.getStringProperty("folderIcon") ) {
        // save folder icon selector
        props += " " + folderIcon;
      }
    }
    return props;
  } // end of override
---
Strangely, if I install other addons that also do folderpane manipulation (such as color folders or extra folder columns) it fixes the issue. But I cannot see what the authors of these extensions do differently? I can attach a prerelease if necessary.
Comment 101 neil@parkwaycc.co.uk 2014-03-12 02:52:10 PDT
>   QuickFolders.FolderTree.GetCellProperties =
> gFolderTreeView.getCellProperties;  
>   gFolderTreeView.getCellProperties = function(row, col) {
>     let props = QuickFolders.FolderTree.GetCellProperties(row, col);
How do you expect that to work? getCellProperties only expects to be called as a function on gFolderTreeView.
Comment 102 Axel Grude [:realRaven] 2014-03-12 03:29:47 PDT
(In reply to neil@parkwaycc.co.uk from comment #101)
> >   QuickFolders.FolderTree.GetCellProperties =
> > gFolderTreeView.getCellProperties;  
> >   gFolderTreeView.getCellProperties = function(row, col) {
> >     let props = QuickFolders.FolderTree.GetCellProperties(row, col);
> How do you expect that to work? getCellProperties only expects to be called
> as a function on gFolderTreeView.

I am afraid I do not understand the question. I am overwriting the getCellProperties with my own callback, wrapping the original one, as described here:

https://developer.mozilla.org/en-US/docs/XUL/Tutorial/Custom_Tree_Views

what am I doing wrong?
Comment 103 Axel Grude [:realRaven] 2014-03-12 04:54:21 PDT
(In reply to Axel Grude [:realRaven] from comment #102)
> (In reply to neil@parkwaycc.co.uk from comment #101)
> > >   QuickFolders.FolderTree.GetCellProperties =
> > > gFolderTreeView.getCellProperties;  
> > >   gFolderTreeView.getCellProperties = function(row, col) {
> > >     let props = QuickFolders.FolderTree.GetCellProperties(row, col);
> > How do you expect that to work? getCellProperties only expects to be called
> > as a function on gFolderTreeView.
> 
> I am afraid I do not understand the question. I am overwriting the
> getCellProperties with my own callback, wrapping the original one, as
> described here:
> 
> https://developer.mozilla.org/en-US/docs/XUL/Tutorial/Custom_Tree_Views
> 
> what am I doing wrong?

Sorry, a better link describing this technique is here:

https://developer.mozilla.org/en-US/docs/XUL/Tutorial/Styling_a_Tree
Comment 104 Boris Zbarsky [:bz] (still a bit busy) 2014-03-12 06:31:37 PDT
> what am I doing wrong?

gFolderTreeView.getCellProperties() will pass gFolderTreeView as the "this" value.

Your code passes QuickFolders.FolderTree as the "this" value.  The function in question needs to get information from the "this" value, so of course it fails when passed the wrong object as "this".

You want 

  QuickFolders.FolderTree.GetCellProperties.call(this, row, col);

or

  QuickFolders.FolderTree.GetCellProperties =
    gFolderTreeView.getCellProperties.bind(gFolderTreeView);

or some other mechanism for making sure "this" ends up correct.

Neither of the two links you posted shows sticking a view's methods on some other random object and then trying to call them....
Comment 105 Axel Grude [:realRaven] 2014-03-12 07:38:10 PDT
(In reply to Boris Zbarsky [:bz] from comment #104)
> > what am I doing wrong?
> 
> gFolderTreeView.getCellProperties() will pass gFolderTreeView as the "this"
> value.
> 
> Your code passes QuickFolders.FolderTree as the "this" value.  The function
> in question needs to get information from the "this" value, so of course it
> fails when passed the wrong object as "this".
> 
> You want 
> 
>   QuickFolders.FolderTree.GetCellProperties.call(this, row, col);
> 
> or
> 
>   QuickFolders.FolderTree.GetCellProperties =
>     gFolderTreeView.getCellProperties.bind(gFolderTreeView);
> 
> or some other mechanism for making sure "this" ends up correct.
> 
> Neither of the two links you posted shows sticking a view's methods on some
> other random object and then trying to call them....

I was just trying to post my solution in which I attached the old function to the originating object gFolderTreeView instead of mine (QuickFolders); but I still need to digest what you said. I think what I hear is that code internal code of the original function references "this" and somehow ends up in the owning object (QuickFolders). I think it is generally bad practice to use "this" in an event procedure for precisely that reason.

The below code works, can you check if it looks ok to you? Apart from that I would like to use one of your methods (when I understand them) as AMO review frowns on sticking new properties on existing objects (which is the reason why used my own object QuickFolders in the first place) as there is always the possibility of name collisions.

Revised (working) code - this does not seem to affect the styling as described in the bug:

 if (gFolderTreeView.supportsIcons) return; // already defined!
  // if (QuickFolders.FolderTree.GetCellProperties) return; // already defined!
  gFolderTreeView.getCellPropsWithoutIcons = gFolderTreeView.getCellProperties;  
  gFolderTreeView.qfIconsEnabled = QuickFolders.Preferences.getBoolPref('folderTree.icons');
  gFolderTreeView.getCellProperties = function(row, col) {
    let props = gFolderTreeView.getCellPropsWithoutIcons(row, col);
    if (col.id == "folderNameCol") {
      let folder = gFolderTreeView.getFolderForIndex(row);
      let folderIcon;
      if (!gFolderTreeView.qfIconsEnabled) {
        return props;
      }
      
      try {
        if ( folderIcon = folder.getStringProperty("folderIcon") ) {
          // save folder icon selector
          props += " " + folderIcon;
        }
      }
      catch(ex) {
        if (QuickFolders)
          QuickFolders.Util.logException('QuickFolders.FolderTree.getCellProperties()',ex);
      }
    }
    return props;
  } // end of override    
  gFolderTreeView.supportsIcons = true;
Comment 106 Boris Zbarsky [:bz] (still a bit busy) 2014-03-12 07:50:50 PDT
> it is generally bad practice to use "this" in an event procedure

What's an "event procedure"?

In any case, both of the options I suggested should be quite straightforward and not require you to add anything new to gFolderTreeView.
Comment 107 Axel Grude [:realRaven] 2014-03-12 09:30:36 PDT
(In reply to Boris Zbarsky [:bz] from comment #106)
> > it is generally bad practice to use "this" in an event procedure
> 
> What's an "event procedure"?
> 
A Function that is called as result of an event handler or a setTimout - it usually isn't called from a parent's context so using "this" is unsafe. But I might be completely wrong this with, just my experience from my own development; whenever you use this in anything that's part of an event handler / setTimeout it usually fails, so I assume there is no "owning object".
Comment 108 Boris Zbarsky [:bz] (still a bit busy) 2014-03-12 09:34:02 PDT
Ah.  setTimeout callbacks are called with "this" set to the window.  Event handlers are called with "this" set to the event target.  In all cases, if you want a specific "this", you can bind() it.
Comment 109 Yonggang Luo 2014-08-13 13:26:58 PDT
I want to post the following code to review, doesn't know if there any migration
code are in the wrong sense.

```
 content/base/src/nsGkAtomList.h     |   2 +-
 layout/xul/tree/nsITreeView.idl     |   5 +-
 layout/xul/tree/nsTreeBodyFrame.cpp | 120 +++++++++++++++++++++++-------------
 layout/xul/tree/nsTreeBodyFrame.h   |  10 +--
 4 files changed, 86 insertions(+), 51 deletions(-)

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")
diff --git a/layout/xul/tree/nsITreeView.idl b/layout/xul/tree/nsITreeView.idl
index c7536f8..ba856a3 100644
--- a/layout/xul/tree/nsITreeView.idl
+++ b/layout/xul/tree/nsITreeView.idl
@@ -9,6 +9,7 @@ interface nsITreeBoxObject;
 interface nsITreeSelection;
 interface nsITreeColumn;
 interface nsIDOMDataTransfer;
+interface nsIMutableArray;
 
 [scriptable, uuid(091116f0-0bdc-4b32-b9c8-c8d5a37cb088)]
 interface nsITreeView : nsISupports
@@ -208,11 +209,11 @@ interface nsIMultilineTreeView: nsITreeView
 {
     long getCellTextPartCount(in long row, in nsITreeColumn col);
     AString getCellTextPart(in long row, in nsITreeColumn col, in long partIdx);
-    void getCellTextPartProperties(in long row, in nsITreeColumn col, in long partIdx, in nsISupportsArray properties);
+    void getCellTextPartProperties(in long row, in nsITreeColumn col, in long partIdx, in nsIMutableArray properties);
 
     long getDescriptionTextPartCount(in long row);
     AString getDescriptionTextPart(in long row, in long partIdx);
-    void getDescriptionTextPartProperties(in long row, in long partIdx, in nsISupportsArray properties);
+    void getDescriptionTextPartProperties(in long row, in long partIdx, in nsIMutableArray properties);
 
     boolean isMultiline();
 };
diff --git a/layout/xul/tree/nsTreeBodyFrame.cpp b/layout/xul/tree/nsTreeBodyFrame.cpp
index 06ab7a7..1d854ac 100644
--- a/layout/xul/tree/nsTreeBodyFrame.cpp
+++ b/layout/xul/tree/nsTreeBodyFrame.cpp
@@ -59,6 +59,7 @@
 #include "nsTreeBoxObject.h"
 #include "nsRenderingContext.h"
 #include "nsIScriptableRegion.h"
+#include "nsIMutableArray.h"
 #include <algorithm>
 #include "ScrollbarActivity.h"
 
@@ -1138,7 +1139,7 @@ nsTreeBodyFrame::GetCoordsForCellItem(int32_t aRow, nsITreeColumn* aCol, const n
       currCol->GetWidthInTwips(this, &colWidth);
     NS_ASSERTION(NS_SUCCEEDED(rv), "invalid column");
 
-    PRInt32 cellHeight = GetCellHeight(aRow, aCol);
+    int32_t cellHeight = GetCellHeight(aRow, aCol);
     nsRect cellRect(currX, mInnerBox.y + mRowHeight * (aRow - mTopRowIndex),
                     colWidth, cellHeight);
 
@@ -1947,7 +1948,7 @@ nsTreeBodyFrame::PrefillPropertyArray(int32_t aRowIndex, nsTreeColumn* aCol)
 
   // drag session
   if (mIsMultiline)
-    mScratchArray->AppendElement(nsGkAtoms::multiline);
+    mScratchArray.AppendElement(nsGkAtoms::multiline);
 
   // drag session
   if (mSlots && mSlots->mIsDragging)
@@ -2440,9 +2441,9 @@ nsTreeBodyFrame::GetImageSourceRect(nsStyleContext* aStyleContext,
   return r;
 }
 
-PRInt32 nsTreeBodyFrame::GetCellHeight(PRInt32 aRow, nsITreeColumn* aColumn) {
+int32_t nsTreeBodyFrame::GetCellHeight(int32_t aRow, nsITreeColumn* aColumn) {
   nsStyleContext* cellContext = GetPseudoStyleContext(nsCSSAnonBoxes::moztreecell);
-  const nsStylePosition* cellPosition = cellContext->GetStylePosition();
+  const nsStylePosition* cellPosition = cellContext->StylePosition();
 
   nscoord cellMinHeight = 0;
   if (cellPosition->mMinHeight.GetUnit() == eStyleUnit_Coord)
@@ -2452,7 +2453,7 @@ PRInt32 nsTreeBodyFrame::GetCellHeight(PRInt32 aRow, nsITreeColumn* aColumn) {
   if (cellPosition->mHeight.GetUnit() == eStyleUnit_Coord)
     cellHeight = cellPosition->mHeight.GetCoordValue();
 
-  cellHeight = PR_MIN(cellHeight, cellMinHeight);
+  cellHeight = std::min(cellHeight, cellMinHeight);
   if (cellHeight == 0 || cellHeight > mRowHeight)
     cellHeight = mRowHeight;
   return cellHeight;
@@ -2464,7 +2465,7 @@ int32_t nsTreeBodyFrame::GetRowHeight()
   // + the specified margins.
   mScratchArray.Clear();
   if (mIsMultiline)
-    mScratchArray->AppendElement(nsGkAtoms::multiline);
+    mScratchArray.AppendElement(nsGkAtoms::multiline);
   nsStyleContext* rowContext = GetPseudoStyleContext(nsCSSAnonBoxes::moztreerow);
   if (rowContext) {
     const nsStylePosition* myPosition = rowContext->StylePosition();
@@ -3074,23 +3075,26 @@ nsTreeBodyFrame::PaintRow(int32_t              aRowIndex,
   else {
     // Now loop over our cells. Only paint a cell if it intersects with our dirty rect.
     if (mIsMultiline) {
-      PRBool displayDescription = PR_TRUE;
-      PrefillPropertyArray(aRowIndex, nsnull);
-      mView->GetRowProperties(aRowIndex, mScratchArray);
+      bool displayDescription = true;
+      PrefillPropertyArray(aRowIndex, nullptr);
+
+      nsAutoString properties;
+      mView->GetRowProperties(aRowIndex, properties);
+      nsTreeUtils::TokenizeProperties(properties, mScratchArray);
       nsStyleContext* descContext = GetPseudoStyleContext(nsCSSAnonBoxes::moztreedescription);
       if (descContext) {
-        const nsStyleDisplay* display = descContext->GetStyleDisplay();
+        const nsStyleDisplay* display = descContext->StyleDisplay();
         if (display->mDisplay == NS_STYLE_DISPLAY_NONE) {
-          displayDescription = PR_FALSE;
+          displayDescription = false;
         }
       }
 
       if (displayDescription) {
-        PRInt32 xOffSet = 0;
+        int32_t xOffSet = 0;
         nsTreeColumn* primaryCol = mColumns->GetPrimaryColumn();
         if (primaryCol) {
 
-          PRBool isRTL = GetStyleVisibility()->mDirection == NS_STYLE_DIRECTION_RTL;
+          bool isRTL = StyleVisibility()->mDirection == NS_STYLE_DIRECTION_RTL;
 
           nsMargin borderPadding(0, 0, 0, 0);
           nsStyleContext* rowContext = GetPseudoStyleContext(nsCSSAnonBoxes::moztreerow);
@@ -3101,7 +3105,7 @@ nsTreeBodyFrame::PaintRow(int32_t              aRowIndex,
           xOffSet += borderPadding.left;
 
           if (!isRTL) {
-            PRInt32 level;
+            int32_t level;
             mView->GetLevel(aRowIndex, &level);
             xOffSet += mIndentation * level;
 
@@ -3111,7 +3115,7 @@ nsTreeBodyFrame::PaintRow(int32_t              aRowIndex,
             GetTwistyRect(aRowIndex, primaryCol, imageRect, twistyRect, PresContext(), aRenderingContext, twistyContext);
 
             nsMargin twistyMargin;
-            twistyContext->GetStyleMargin()->GetMargin(twistyMargin);
+            twistyContext->StyleMargin()->GetMargin(twistyMargin);
             twistyRect.Inflate(twistyMargin);
             xOffSet += twistyRect.width;
           }
@@ -3367,20 +3371,34 @@ nsTreeBodyFrame::PaintCell(int32_t              aRowIndex,
         case nsITreeColumn::TYPE_TEXT:
          if (mIsMultiline) {
            nscoord xOffSet = 0, yOffSet = 0;
-           PRInt32 partCount, partIdx;
+           int32_t partCount, partIdx;
            nsCOMPtr<nsIMultilineTreeView> multilineView = do_QueryInterface(mView);
            multilineView->GetCellTextPartCount(aRowIndex, aColumn, &partCount);
            for (partIdx = 0; partIdx < partCount; partIdx++) {
              nsAutoString text;
              PrefillPropertyArray(aRowIndex, aColumn);
-             multilineView->GetCellProperties(aRowIndex, aColumn, mScratchArray);
-             multilineView->GetCellTextPartProperties(aRowIndex, aColumn, partIdx, mScratchArray);
+             nsAutoString properties;
+             multilineView->GetCellProperties(aRowIndex, aColumn, properties);
+             nsTreeUtils::TokenizeProperties(properties, mScratchArray);
+             nsCOMPtr<nsIMutableArray> scratchArray = do_CreateInstance(NS_ARRAY_CONTRACTID);
+             NS_ENSURE_TRUE_VOID(scratchArray);
+             multilineView->GetCellTextPartProperties(aRowIndex, aColumn, partIdx, scratchArray);
+             uint32_t scratchArrayLength;
+             scratchArray->GetLength(&scratchArrayLength);
+             for (uint32_t idx = 0; idx < scratchArrayLength; ++idx) {
+                 nsCOMPtr<nsIAtom> atom;
+                 nsresult rv = scratchArray->QueryElementAt(idx, NS_GET_IID(nsIAtom),
+                     getter_AddRefs(atom));
+                 NS_ENSURE_SUCCESS_VOID(rv);
+                 mScratchArray.AppendElement(atom);
+             }
+             scratchArray->Release();
              nsStyleContext* textContext = GetPseudoStyleContext(nsCSSAnonBoxes::moztreecelltextpart);
              multilineView->GetCellTextPart(aRowIndex, aColumn, partIdx, text);
              PaintMultilineText(text, elementRect, aPresContext, aRenderingContext, aDirtyRect, textContext, xOffSet, yOffSet);
            }
          } else {
-           PaintText(aRowIndex, aColumn, elementRect, aPresContext, aRenderingContext, aDirtyRect, currX, textRTL);
+           PaintText(aRowIndex, aColumn, elementRect, aPresContext, aRenderingContext, aDirtyRect, currX);
          }
           break;
         case nsITreeColumn::TYPE_CHECKBOX:
@@ -3408,21 +3426,24 @@ nsTreeBodyFrame::PaintCell(int32_t              aRowIndex,
 }
 
 void
-nsTreeBodyFrame::PaintDescription(PRInt32              aRowIndex,
+nsTreeBodyFrame::PaintDescription(int32_t              aRowIndex,
                            const nsRect&        aDescRect,
                            nsPresContext*       aPresContext,
-                           nsIRenderingContext& aRenderingContext,
+                           nsRenderingContext& aRenderingContext,
                            const nsRect&        aDirtyRect)
 {
 
-  PrefillPropertyArray(aRowIndex, nsnull);
-  mView->GetRowProperties(aRowIndex, mScratchArray);
+  PrefillPropertyArray(aRowIndex, nullptr);
+
+  nsAutoString properties;
+  mView->GetRowProperties(aRowIndex, properties);
+  nsTreeUtils::TokenizeProperties(properties, mScratchArray);
 
   nsStyleContext* descContext = GetPseudoStyleContext(nsCSSAnonBoxes::moztreedescription);
 
   nsRect descRect(aDescRect);
   nsMargin descMargin;
-  descContext->GetStyleMargin()->GetMargin(descMargin);
+  descContext->StyleMargin()->GetMargin(descMargin);
   descRect.Deflate(descMargin);
 
   // Paint our borders and background for our row rect.
@@ -3434,13 +3455,26 @@ nsTreeBodyFrame::PaintDescription(PRInt32              aRowIndex,
   nsRect dirtyRect;
   if (dirtyRect.IntersectRect(aDirtyRect, descRect)) {
     nscoord xOffSet = 0, yOffSet = 0;
-    PRInt32 partCount, partIdx;
+    int32_t partCount, partIdx;
     nsCOMPtr<nsIMultilineTreeView> multilineView = do_QueryInterface(mView);
     multilineView->GetDescriptionTextPartCount(aRowIndex, &partCount); 
     for (partIdx = 0; partIdx < partCount; partIdx++) {
       nsAutoString text;                                                                                                                  
-      PrefillPropertyArray(aRowIndex, nsnull);
-      multilineView->GetDescriptionTextPartProperties(aRowIndex, partIdx, mScratchArray);
+      PrefillPropertyArray(aRowIndex, nullptr);
+      nsTreeUtils::TokenizeProperties(properties, mScratchArray);
+      nsCOMPtr<nsIMutableArray> scratchArray = do_CreateInstance(NS_ARRAY_CONTRACTID);
+      NS_ENSURE_TRUE_VOID(scratchArray);
+      multilineView->GetDescriptionTextPartProperties(aRowIndex, partIdx, scratchArray);
+      uint32_t scratchArrayLength;
+      scratchArray->GetLength(&scratchArrayLength);
+      for (uint32_t idx = 0; idx < scratchArrayLength; ++idx) {
+          nsCOMPtr<nsIAtom> atom;
+          nsresult rv = scratchArray->QueryElementAt(idx, NS_GET_IID(nsIAtom),
+              getter_AddRefs(atom));
+          NS_ENSURE_SUCCESS_VOID(rv);
+          mScratchArray.AppendElement(atom);
+      }
+      scratchArray->Release();
       nsStyleContext* textContext = GetPseudoStyleContext(nsCSSAnonBoxes::moztreedescriptiontextpart);
       multilineView->GetDescriptionTextPart(aRowIndex, partIdx, text);
       PaintMultilineText(text, descRect, aPresContext, aRenderingContext, aDirtyRect, textContext, xOffSet, yOffSet);
@@ -3801,7 +3835,7 @@ void
 nsTreeBodyFrame::PaintMultilineText(nsAutoString&        aText,
                                     const nsRect&        aTextRect,
                                     nsPresContext*       aPresContext,
-                                    nsIRenderingContext& aRenderingContext,
+                                    nsRenderingContext& aRenderingContext,
                                     const nsRect&        aDirtyRect,
                                     nsStyleContext*      aStyleContext,
                                     nscoord&             aCurrY,
@@ -3820,18 +3854,18 @@ nsTreeBodyFrame::PaintMultilineText(nsAutoString&        aText,
   nsRect textRect(aTextRect);
 
   // Set our color.
-  aRenderingContext.SetColor(textContext->GetStyleColor()->mColor);
+  aRenderingContext.SetColor(textContext->StyleColor()->mColor);
 
   // Compute our text size.
-  nsCOMPtr<nsIFontMetrics> fontMet;
+  nsRefPtr<nsFontMetrics> fontMet;
   nsLayoutUtils::GetFontMetricsForStyleContext(textContext,
                                                getter_AddRefs(fontMet));
 
   nscoord height, baseline;
-  fontMet->GetHeight(height);
-  fontMet->GetMaxAscent(baseline);
+  height = fontMet->MaxHeight();
+  baseline = fontMet->MaxAscent();
 
-  PRUint32 maxLines = (aTextRect.height - aCurrY) / height;
+  uint32_t maxLines = (aTextRect.height - aCurrY) / height;
   if (aCurrY == 0 && maxLines == 0) {
     maxLines = 1;
   }
@@ -3841,8 +3875,8 @@ nsTreeBodyFrame::PaintMultilineText(nsAutoString&        aText,
 
   nsAutoString tmpText(text);
 
-  for (PRUint32 lineIdx = 0; lineIdx < maxLines; lineIdx++) {
-    PRBool lineComplete  = PR_TRUE;
+  for (uint32_t lineIdx = 0; lineIdx < maxLines; lineIdx++) {
+    bool lineComplete  = true;
     nsAutoString textToDraw(tmpText);
 
     nscoord width = nsLayoutUtils::GetStringWidth(this, &aRenderingContext, tmpText.get(), tmpText.Length());
@@ -3853,12 +3887,12 @@ nsTreeBodyFrame::PaintMultilineText(nsAutoString&        aText,
     if (width > maxWidth) {
 
       width = maxWidth;
-      aRenderingContext.SetTextRunRTL(PR_FALSE);
+      aRenderingContext.SetTextRunRTL(false);
 
       const nsDependentString& kEllipsis = nsContentUtils::GetLocalizedEllipsis();
       if (lineIdx == maxLines - 1) {
         nscoord ellipsisWidth;
-        aRenderingContext.GetWidth(kEllipsis, ellipsisWidth);
+        ellipsisWidth = aRenderingContext.GetWidth(kEllipsis);
 
         if (ellipsisWidth > width) {
           textToDraw.SetLength(0);
@@ -3873,9 +3907,9 @@ nsTreeBodyFrame::PaintMultilineText(nsAutoString&        aText,
           int length = tmpText.Length();
           int i;
           for (i = 0; i < length; ++i) {
-            PRUnichar ch = tmpText[i];
+            char16_t ch = tmpText[i];
             // XXX this is horrible and doesn't handle clusters
-            aRenderingContext.GetWidth(ch,cwidth);
+            cwidth = aRenderingContext.GetWidth(ch);
             if (twidth + cwidth > width)
               break;
             twidth += cwidth;
@@ -3889,9 +3923,9 @@ nsTreeBodyFrame::PaintMultilineText(nsAutoString&        aText,
         int length = tmpText.Length();
         int i;
         for (i = 0; i < length; ++i) {
-          PRUnichar ch = tmpText[i];
+          char16_t ch = tmpText[i];
           // XXX this is horrible and doesn't handle clusters
-          aRenderingContext.GetWidth(ch,cwidth);
+          cwidth = aRenderingContext.GetWidth(ch);
           if (twidth + cwidth > width)
             break;
           twidth += cwidth;
@@ -3901,10 +3935,10 @@ nsTreeBodyFrame::PaintMultilineText(nsAutoString&        aText,
       }
     } else {
       tmpText.SetLength(0);
-      lineComplete = PR_FALSE;
+      lineComplete = false;
     }
     // Draw decorations.
-    PRUint8 decorations = textContext->GetStyleTextReset()->mTextDecoration;
+    uint8_t decorations = textContext->StyleTextReset()->mTextDecorationLine;
 
     nscoord offset;
     nscoord size;
diff --git a/layout/xul/tree/nsTreeBodyFrame.h b/layout/xul/tree/nsTreeBodyFrame.h
index a82ac14..2cac519 100644
--- a/layout/xul/tree/nsTreeBodyFrame.h
+++ b/layout/xul/tree/nsTreeBodyFrame.h
@@ -196,10 +196,10 @@ protected:
                    nsRenderingContext& aRenderingContext,
                    const nsRect&        aDirtyRect);
 
-  void PaintDescription(PRInt32              aRowIndex,
+  void PaintDescription(int32_t              aRowIndex,
       const nsRect&        aDescRect,
       nsPresContext*       aPresContext,
-      nsIRenderingContext& aRenderingContext,
+      nsRenderingContext& aRenderingContext,
       const nsRect&        aDirtyRect);
 
   // This method paints a single row in the tree.
@@ -213,7 +213,7 @@ protected:
   void PaintMultilineText(nsAutoString&        aText,
           const nsRect&        aTextRect,
           nsPresContext*       aPresContext,
-          nsIRenderingContext& aRenderingContext,
+          nsRenderingContext& aRenderingContext,
           const nsRect&        aDirtyRect,
           nsStyleContext*      aStyleContext,
           nscoord&             aCurrY,
@@ -346,7 +346,7 @@ protected:
   int32_t GetRowHeight();
 
   // Returns the height of a cell
-  PRInt32 GetCellHeight(PRInt32 aRowIndex, nsITreeColumn* aCol);
+  int32_t GetCellHeight(int32_t aRowIndex, nsITreeColumn* aCol);
 
   // Returns our indentation width.
   int32_t GetIndentation();
@@ -631,7 +631,7 @@ protected: // Data Members
   // Whether or not we're currently focused.
   bool mFocused;
 
-  PRBool mIsMultiline;
+  bool mIsMultiline;
 
   // Do we have a fixed number of onscreen rows?
   bool mHasFixedRowCount;

```
Comment 110 Alex Vincent [:WeirdAl] 2014-08-13 13:45:47 PDT
(In reply to Yonggang Luo from comment #109)
> I want to post the following code to review, doesn't know if there any
> migration
> code are in the wrong sense.

Generally speaking, you should attach patches (especially big ones like this) instead of posting them as comments.

> --- a/layout/xul/tree/nsITreeView.idl
> +++ b/layout/xul/tree/nsITreeView.idl
> @@ -9,6 +9,7 @@ interface nsITreeBoxObject;
>  [scriptable, uuid(091116f0-0bdc-4b32-b9c8-c8d5a37cb088)]

Because you're changing the arguments of a method, you'll need to change the UUID string above.  Use uuidgen or ask firebot on #developers for a UUID.

> -    void getCellTextPartProperties(in long row, in nsITreeColumn col, in
> long partIdx, in nsISupportsArray properties);
> +    void getCellTextPartProperties(in long row, in nsITreeColumn col, in
> long partIdx, in nsIMutableArray properties);
>  
>      long getDescriptionTextPartCount(in long row);
>      AString getDescriptionTextPart(in long row, in long partIdx);
> -    void getDescriptionTextPartProperties(in long row, in long partIdx, in
> nsISupportsArray properties);
> +    void getDescriptionTextPartProperties(in long row, in long partIdx, in
> nsIMutableArray properties);

Please don't do this.  We moved away from using a nsIArray in nsITreeView, mainly because they're really hard to work with in JavaScript.  Instead, please take a look at attachment 715694 [details] [diff] [review], which is what was checked into mozilla-central.  You'll note the interface returns a string, which I believe is comma-separated.
Comment 111 Alex Vincent [:WeirdAl] 2014-08-13 13:49:19 PDT
I was wrong, it's whitespace-separated.

Note You need to log in before you can comment on or make changes to this bug.