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

This is the WIP patch.

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.

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.

I'm not working on this in the short term, especially I expect not before 1.9 is released.

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

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

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.

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

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

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

> 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?

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

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

(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 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 on attachment 702763 [details] [diff] [review] bug 407956 - convert GetColumnProperties Well that was straightforward ;-)

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!]

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

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

(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 :)

(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 on attachment 703046 [details] [diff] [review] bug 407956 - GetCellProperties() r=me with above comments addressed, but I'd like to see a final patch.

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

Boris, can you check the comment about what getCellProperties() does is correct in the idl?

>+ * 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?

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

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

(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 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 ;-)

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

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

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

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

some comm-central C++ fixes

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"

what m-c bugs need to land (or be installed locally) before I can test this?

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.

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

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

Will probably need update of docs at https://developer.mozilla.org/en-US/docs/XUL/Tutorial/Styling_a_Tree and similar.

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.

(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 on attachment 704280 [details] [diff] [review] suite changes r=me with those things fixed.

Updated for review comments.

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 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";

(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";

Can you fix DOMi please?

I think this suffices. (Obviously there's some back compat code left behind.)

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?

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

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

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.

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

(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") ?

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

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

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

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

The core patch does not apply cleanly, the files nsITreeView.idl, nsTreeBodyFrame.cpp, nsTreeContentView.cpp have moved.

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

Why do I still get the breakage from comment 47? Are the tweaks from comment 48 still needed?

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.

(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!)

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

OK, updated.

Is this ready to check in?

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

I think this should land at the same time otherwise c-c will get broken. I think RyanVM can check into all necessary repos.

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

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

finally https://hg.mozilla.org/dom-inspector/rev/a66d89b48c7e

Any suggestion on how to best feature-detect this without always checking for the old props argument to be undefined?

(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

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?

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?

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.

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

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

Filed Bug 849534

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

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.

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

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

(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

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

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

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

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

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.

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; ```

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

I was wrong, it's whitespace-separated.

Pushed by frgrahl@gmx.net: https://hg.mozilla.org/comm-central/rev/6ed938b4d3f0 make nsITreeView not take a nsISupportsArray* r=neil, bz sr=neil