Port bug 1466673 (replace use of nsITreeColumns) and bug 1466727 (replace use of nsITreeColumn)

RESOLVED FIXED in Thunderbird 62.0

Status

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: jorgk, Assigned: jorgk)

Tracking

Trunk
Thunderbird 62.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 12 obsolete attachments)

1.49 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
38.50 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
20.92 KB, patch
aceman
: review+
alta88
: review+
Details | Diff | Splinter Review
https://dxr.mozilla.org/comm-central/search?q=nsITreeColumn&redirect=false

The timing isn't great here since we're just gearing up to create more C++ trees in bug 1466705 using an adaption of this patch:
https://hg.mozilla.org/users/Pidgeot18_gmail.com/patch-queues/file/tip/patches-derdf/subscribe-tree
Ideal timing would be to land the treecolumn changes before you write the code for those C++ trees, right?
Thanks for the consideration, but no. Ideal you be to do bug 1466705 (which we should have fixed at version 59), then backport/uplift to 60, then follow your changes in trunk 62.

If your changes go in before our patch, we'll have a hard time backporting since we usually don't build older version locally.
Ah, yeah, if you have to backport across these changes that's annnoying.  Not sure I can do much about that...
Delay your stuff by a week?
I already landed some of it...  But also, it's blocking some other things I would really like to get to soonish, sorry.
OK, then get it over and done with quickly and land bug 1466727 as well. I see there are three reviews missing at time of writing.

I might apply bug 1466673 locally and make a start.
This compiles.

nsITreeColumns.h is included in five more files:
mailnews/addrbook/src/nsAbView.cpp
mailnews/base/src/nsMsgDBView.cpp
mailnews/base/src/nsMsgGroupView.cpp
mailnews/base/src/nsMsgSearchDBView.cpp
mailnews/base/src/nsMsgXFVirtualFolderDBView.cpp
but these all use the nsITreeColumn (no "s") interface. The include file is also still there since nsITreeColumns.idl is also still there and still contains nsITreeColumn (which will be removed next).
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8983552 - Flags: review?(bzbarsky)
> OK, then get it over and done with quickly and land bug 1466727 as well.

Yes, working on it.
Comment on attachment 8983552 [details] [diff] [review]
1466782-replace-nsITreeColumns.patch

r=me
Attachment #8983552 - Flags: review?(bzbarsky) → review+
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/bfc783ee07fa
Port bug 1466673: replace use of nsITreeColumns. r=bz
Keywords: leave-open
Attachment #8983661 - Flags: review?(bzbarsky)
Comment on attachment 8983661 [details] [diff] [review]
1466782-replace-nsITreeColumn.patch

>+  nsAutoString colID;
>+  col->GetId(colID);

You could also use the 0-arg form of GetId():

  const nsAString& colID = col->getId();

but see below.

>+  if (colID.get()[0] != char16_t('G'))

This seems fairly broken conceptually, for cases when colId is the empty string.  I guess in the nsAutoString case you would always have the null-terminator there....

If that's the case, you need to very clearly document the assumptions this is making.  Better would be to not depend on that, of course.

>+  switch (colID.get()[0])

Same here, multiple places.

>+      colID.get()[0] == 'l' && colID.get()[1] == 'o')

And here.

>+  if (colID.get()[0] == 's') {
>+  else if (colID.get()[0] == 'n') {

And these.

>+  if (colID.get()[0] == 'n') {

And here, multiple places.

r=me modulo that.
Attachment #8983661 - Flags: review?(bzbarsky) → review+
So here we have it with a lot of string changes.
Attachment #8983661 - Attachment is obsolete: true
Comment on attachment 8983902 [details] [diff] [review]
1466782-replace-nsITreeColumn.patch (v2)

Quite tedious to look at :-(
Attachment #8983902 - Flags: review?(bzbarsky)
Comment on attachment 8983902 [details] [diff] [review]
1466782-replace-nsITreeColumn.patch (v2)

>+  nsAutoString colID;  // This is null-terminated which we need for the call below.

For the GetCellTextForColumn call?  Maybe say that explicitly?

r=me
Attachment #8983902 - Flags: review?(bzbarsky) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6c2b2979b2af
Port bug 1466673: replace use of nsITreeColumn. r=bz
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Boris, I fixed that comment. Thanks for the review.
Target Milestone: --- → Thunderbird 62.0
Posted patch 1466782-follow-up.patch (obsolete) — Splinter Review
I was in a bit of a rush to get the previous patch landed, so I didn't include some clean-up. What do you think of this?

I guess those single character checks are meant to be faster then string compares on, for example, "subject".
Attachment #8983970 - Flags: review?(bzbarsky)
Comment on attachment 8983970 [details] [diff] [review]
1466782-follow-up.patch

I don't know when I'll have a chance to review this, sorry.  Probably not for at least two weeks, given where things stand right now.

In general, this doesn't need my review; it needs review from someone on Thunderbird...
Attachment #8983970 - Flags: review?(bzbarsky)
Comment on attachment 8983970 [details] [diff] [review]
1466782-follow-up.patch

Thanks Boris, you're right.

Aceman, here's I'm trying to get rid of direct string access to avoid potential crashes.
Attachment #8983970 - Flags: review?(acelists)
Comment on attachment 8983970 [details] [diff] [review]
1466782-follow-up.patch

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

Good that we can move away from plain strings.

::: mailnews/base/src/nsMsgDBView.cpp
@@ +2154,5 @@
>  nsMsgDBView::GetCellText(int32_t aRow,
>                           nsTreeColumn* aCol,
>                           nsAString& aValue)
>  {
> +  const nsAString& colID = aCol->GetId();

I think this should be nsString.

@@ +2181,5 @@
>                                 nsAString &aValue)
>  {
> +  uint32_t colLength = aColumnName.Length();
> +  if (colLength == 0)
> +    return NS_OK;

Is it common to have columns without name? Would they be empty?
Otherwise this check may just be a slowdown.

@@ +2195,5 @@
>  
>    nsCOMPtr<nsIMsgThread> thread;
>  
> +// Helper macro to safely check a single character in string.
> +#define CHECKCOL(i, c) (colLength > i && aColumnName.CharAt(i) == c)

Does CharAt() really need the length check?

@@ +2202,3 @@
>    {
>      case 's':
> +      if (CHECKCOL(1, 'u')) // subject

The whole original idea seems fragile to me, checking just 2 chars to guess what the full col name is. This has assumptions there are no names being prefixes to other names. Also the original code seemed to assume the name is always longer than what position we were checking (at least 7 chars).
I think when we use the full nsString object already, checking the full name may not be much slower than checking one CharAt(). Can you try a version with that? You can still keep the main switch.
Attachment #8983970 - Flags: feedback+
(In reply to :aceman from comment #21)
> I think this should be nsString.
No, |const nsAString& colID = col->getId();| as per comment #12, already landed in various places:
https://hg.mozilla.org/comm-central/rev/6c2b2979b2af#l1.60
https://hg.mozilla.org/comm-central/rev/6c2b2979b2af#l1.159
and many more.

> Is it common to have columns without name? Would they be empty?
> Otherwise this check may just be a slowdown.
Well, we added a few checks in https://hg.mozilla.org/comm-central/rev/6c2b2979b2af to avoid for First() to release assert:
https://searchfox.org/mozilla-central/rev/edbf2c009992315d85eeb885e1b8edbbd43c84b7/xpcom/string/nsTSubstring.cpp#849
The same applies here since the switch moved from aColumnName[0] potentially hitting the null-terminator to using First(). And I'd hate to have release asserts just because we forgot a case.

> Does CharAt() really need the length check?
Yes, otherwise it will assert as well:
https://searchfox.org/mozilla-central/rev/edbf2c009992315d85eeb885e1b8edbbd43c84b7/xpcom/string/nsTStringRepr.h#208
https://searchfox.org/mozilla-central/rev/edbf2c009992315d85eeb885e1b8edbbd43c84b7/xpcom/string/nsTString.h#210

> The whole original idea seems fragile to me, checking just 2 chars to guess
> what the full col name is. This has assumptions there are no names being
> prefixes to other names. Also the original code seemed to assume the name is
> always longer than what position we were checking (at least 7 chars).
> I think when we use the full nsString object already, checking the full name
> may not be much slower than checking one CharAt(). Can you try a version
> with that? You can still keep the main switch.
As I said in comment #18:
  I guess those single character checks are meant to be faster then
  string compares on, for example, "subject".

So no, I won't change it to string compares. I don't know what you mean by "try a version with that". Sure, that will work, but I'd have to do expensive performance comparisons, like compare a folder with 100.000 messages to see whether the string comparison makes a difference.

This here is just a little refactoring since everywhere we use |const nsAString& colID = col->getId();| apart from this one call site.

In conclusion: No new patch, this one "as is" or nothing else.
(In reply to Jorg K (GMT+2) from comment #22)
> (In reply to :aceman from comment #21)
> > I think this should be nsString.
> No, |const nsAString& colID = col->getId();| as per comment #12, already
> landed in various places:
> https://hg.mozilla.org/comm-central/rev/6c2b2979b2af#l1.60
> https://hg.mozilla.org/comm-central/rev/6c2b2979b2af#l1.159
> and many more.

It is strange to instantiate an abstract class from which specific classes inherit and which is mostly used for function arguments to allow to pass any of the child classes. But it seems to be allowed per https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings .
 
> > Is it common to have columns without name? Would they be empty?
> > Otherwise this check may just be a slowdown.
> Well, we added a few checks in
> https://hg.mozilla.org/comm-central/rev/6c2b2979b2af to avoid for First() to
> release assert:
> https://searchfox.org/mozilla-central/rev/
> edbf2c009992315d85eeb885e1b8edbbd43c84b7/xpcom/string/nsTSubstring.cpp#849
> The same applies here since the switch moved from aColumnName[0] potentially
> hitting the null-terminator to using First(). And I'd hate to have release
> asserts just because we forgot a case.
> 
> > Does CharAt() really need the length check?
> Yes, otherwise it will assert as well:
> https://searchfox.org/mozilla-central/rev/
> edbf2c009992315d85eeb885e1b8edbbd43c84b7/xpcom/string/nsTStringRepr.h#208
> https://searchfox.org/mozilla-central/rev/
> edbf2c009992315d85eeb885e1b8edbbd43c84b7/xpcom/string/nsTString.h#210
> 
> > The whole original idea seems fragile to me, checking just 2 chars to guess
> > what the full col name is. This has assumptions there are no names being
> > prefixes to other names. Also the original code seemed to assume the name is
> > always longer than what position we were checking (at least 7 chars).
> > I think when we use the full nsString object already, checking the full name
> > may not be much slower than checking one CharAt(). Can you try a version
> > with that? You can still keep the main switch.
> As I said in comment #18:
>   I guess those single character checks are meant to be faster then
>   string compares on, for example, "subject".

I understand they could be faster with plain char* strings. But I'm not so sure with nsString.
Also, it is wrong to assume the col name is "subject" when you only checked for the first "su" characters.
It could be e.g. "summary". I just asked if we could fix this pre-existing logical problem when we are already touching this and the new way may not have any perf advantages just checking 2 chars.

> So no, I won't change it to string compares. I don't know what you mean by
> "try a version with that". Sure, that will work, but I'd have to do
> expensive performance comparisons, like compare a folder with 100.000
> messages to see whether the string comparison makes a difference.

I do have a 1 million message folder. Anyway, I don't think the number of messages is relevant. The functions are only called for the cells that are visible on screen (thus maybe 20 rows).
(In reply to :aceman from comment #23)
> It is strange to instantiate an abstract class from which specific classes
> inherit and which is mostly used for function arguments to allow to pass any
> of the child classes.
Commonly used in 'const' expressions:
https://searchfox.org/comm-central/search?q=const+nsAString%26.*%3D&case=true&regexp=true&path=

> I do have a 1 million message folder. Anyway, I don't think the number of
> messages is relevant. The functions are only called for the cells that are
> visible on screen (thus maybe 20 rows).
OK.
> It is strange to instantiate an abstract class

The line

 const nsAString& colID = col->getId();

is not instantiating an abstract class.  It's just declaring a reference to an instance of that class.  This is not a strange thing to do at all, just like it's not strange to have a "const nsAString&" function argument.  This is the same situation: we have no idea what the actual concrete string is that getId() returns a reference to, but we know it's definitely an nsAString subclass.
Posted patch 1466782-follow-up.patch v2b (obsolete) — Splinter Review
We don't need the column name length any more.
Attachment #8984811 - Attachment is obsolete: true
Attachment #8984811 - Flags: review?(acelists)
Attachment #8984812 - Flags: review?(acelists)
Comment on attachment 8984812 [details] [diff] [review]
1466782-follow-up.patch v2b

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

Thanks, I'll test it on my big folders.
I'm sure we discussed this in some of the string conversion patches.
So is there a reason for .Equals(u"") instead of .EqualsLiteral()?

::: mailnews/base/src/nsMsgDBView.cpp
@@ +2181,4 @@
>                                 nsAString &aValue)
>  {
> +  if (aColumnName.IsEmpty())
> +    return NS_OK;

In case of empty column name you do not touch aValue. Based on the NS_OK return the caller may use the aValue, assuming it was set to the right value. Which it wasn't. What would be a proper value in this case? Empty string?
But yes, it seems this problem was in the original code too. But now you try to handle the empty name explicitly, while the original code didn't, it just fell through the function.

@@ +2217,3 @@
>          rv = FetchDate(msgHdr, aValue, true);
>        break;
>      case 'd':  // date

What about these? We should check the full name too when we do the others.
Attachment #8984812 - Flags: feedback+
Posted patch 1466782-follow-up.patch v3 (obsolete) — Splinter Review
(In reply to :aceman from comment #28)
> So is there a reason for .Equals(u"") instead of .EqualsLiteral()?
I still had bug 870698 comment #16 in mind: "... there is no EqualLiteral() function overloaded for char16_t string literals ...". But apparently is works.

> But yes, it seems this problem was in the original code too. But now you try
> to handle the empty name explicitly, while the original code didn't, it just
> fell through the function.
I'll truncate it now.

> >      case 'd':  // date
> What about these? We should check the full name too when we do the others.
That's not what you said in comment #21: "You can still keep the main switch."
Attachment #8984812 - Attachment is obsolete: true
Attachment #8984812 - Flags: review?(acelists)
Attachment #8984816 - Flags: review?(acelists)
Yes, the main switch can stay, just inside e.g. "case 'd'" you add check if it really is 'date'.
Checking some names in full and some only partly is not a full solution.
Posted patch 1466782-follow-up.patch v4 (obsolete) — Splinter Review
OK, more string comparisons.

What do you want to do in the other function nsMsgDBView::GetCellValue() that does all those comparisons on the first and second character:

https://searchfox.org/comm-central/rev/739a41cbf1250210a0a664e668c912ab4fc57d5b/mailnews/base/src/nsMsgDBView.cpp#1902
https://searchfox.org/comm-central/rev/739a41cbf1250210a0a664e668c912ab4fc57d5b/mailnews/base/src/nsMsgDBView.cpp#1933
Attachment #8984816 - Attachment is obsolete: true
Attachment #8984816 - Flags: review?(acelists)
Attachment #8984826 - Flags: review?(acelists)
Posted patch 1466782-follow-up.patch v5 (obsolete) — Splinter Review
I think I covered all single-character compares now.
Attachment #8984883 - Flags: review?(acelists)
Comment on attachment 8984883 [details] [diff] [review]
1466782-follow-up.patch v5

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

Thanks, this is what I wanted. And I tested it on a million message folder and it runs fine, as only the visible cells are redrawn.

::: mailnews/base/src/nsMsgDBView.cpp
@@ +1898,5 @@
>    // Use empty string for the normal states "Read", "Not Starred",
>    // "No Attachment" and "Not Junk".
>    switch (colID.First())
>    {
> +    case 'a': //

Remove the comment

@@ +1916,5 @@
>          tmp_str.Adopt(GetString(u"messageHasFlag"));
>          aValue.Assign(tmp_str);
>        }
>        break;
>      case 'j': // junk column

Remove the comment

@@ +2204,4 @@
>    {
>      case 's':
> +      if (aColumnName.EqualsLiteral("subjectCol"))
> +      if (aColumnName.EqualsLiteral("subjectCol"))

Why is this twice?

@@ +2368,5 @@
>      return NS_OK;
>  
>    switch (colID.First())
>    {
>      case 'u': // unreadButtonColHeader

Remove the comment

@@ +2400,4 @@
>        break;
>      case 'j': // junkStatus column
>      {
> +      if (! colID.EqualsLiteral("junkStatusCol") || !JunkControlsEnabled(row))

surplus space

::: mailnews/base/src/nsMsgGroupView.cpp
@@ +842,5 @@
>    if (!IsValidIndex(aRow))
>      return NS_MSG_INVALID_DBVIEW_INDEX;
>  
> +  if (m_flags[aRow] & MSG_VIEW_FLAG_DUMMY &&
> +      !aColumnName.IsEmpty() && aColumnName.First() != 'u')

Inside the branch we only check subjectCol and totalCol
so anything except 's' and 't' could be skipped here. So why is 'u' singled out? Yes, this is preexistind and probably assumes the only other existing columns all start with 'u'. But we want to kill these silent assumptions.
Attachment #8984883 - Flags: review?(acelists) → review+
Posted patch 1466782-follow-up.patch (v6) (obsolete) — Splinter Review
I've addressed the nits from the previous comment. The double-up line was most likely caused by Notepad++ where Ctrl+D duplicates a line and D is right next to S :-(

I still don't understand the business with the 'u' check, so we need to clarify that before we change that.
Attachment #8984826 - Attachment is obsolete: true
Attachment #8984883 - Attachment is obsolete: true
Attachment #8984826 - Flags: review?(acelists)
Comment on attachment 8985864 [details] [diff] [review]
1466782-follow-up.patch (v6)

Alta88, could you please check this patch. What is this code about?

if (m_flags[aRow] & MSG_VIEW_FLAG_DUMMY && aColumnName[0] != 'u')
https://dxr.mozilla.org/comm-central/rev/51678c5b0b3edd827c57a70f1c5baf02d44acead/mailnews/base/src/nsMsgGroupView.cpp#845

In the if-block we only check for "subjectCol" and "totalCol". However, at the end of the if-block there is an early-return, so "unreadCol" will be passed to nsMsgDBView::CellTextForColumn() whereas all non-u columns take the early return.
Attachment #8985864 - Flags: feedback?(alta88)
Posted patch 1466782-follow-up.patch (v6b) (obsolete) — Splinter Review
Attachment #8985864 - Attachment is obsolete: true
Attachment #8985864 - Flags: feedback?(alta88)
Attachment #8985866 - Flags: feedback?(alta88)
Comment on attachment 8985866 [details] [diff] [review]
1466782-follow-up.patch (v6b)

Your change is fine but nsMsgGroupView::CellTextForColumn() is rather a mess and should be moved around. I would early return if not MSG_VIEW_FLAG_DUMMY or column name is not subjectCol and is not totalCol. It's probable the u cols were going to be meaningful but certain cycler columns in the dummy header row were fails and that was fixed in Bug 267663.  Bug 374448 addresses making other dummy header row columns really work for all children.
Attachment #8985866 - Flags: feedback?(alta88) → feedback+
Posted patch 1466782-follow-up.patch (v7) (obsolete) — Splinter Review
OK, I reshuffled it according to "I would early return if not MSG_VIEW_FLAG_DUMMY or column name is not subjectCol and is not totalCol.". Now one can see what's happening ;-)
Attachment #8985866 - Attachment is obsolete: true
Attachment #8985891 - Flags: review?(alta88)
Attachment #8985891 - Flags: review?(acelists)
Posted patch 1466782-follow-up.patch (v7b) (obsolete) — Splinter Review
Optimised version to save string comparisons.
Attachment #8985891 - Attachment is obsolete: true
Attachment #8985891 - Flags: review?(alta88)
Attachment #8985891 - Flags: review?(acelists)
Attachment #8985892 - Flags: review?(alta88)
Attachment #8985892 - Flags: review?(acelists)
I would like this, but it does not seem to fix the main issue. Where did the 'u' check go? Previously 'u'-starting columns would go to the superclass (whether they are dummy or not), now they don't.
(In reply to :aceman from comment #40)
> I would like this, but it does not seem to fix the main issue. Where did the
> 'u' check go? Previously 'u'-starting columns would go to the superclass
> (whether they are dummy or not), now they don't.
What is the main issue? The main issue is the access of strings by index which might be shorter, so that issue *is* fixed.

The 'u' check is gone, maybe I misunderstood comment #37. Yes, 'u'-starting columns previously called nsMsgDBView::CellTextForColumn() unconditionally, now only non-dummy ones get sent that way so the behaviour is changing for dummy 'u' columns. But does that case exist? Alta88?
Comment on attachment 8985892 [details] [diff] [review]
1466782-follow-up.patch (v7b)

unreadCol needs to be determined for the dummy row in nsMsgDBView, unreadButtonColHeader cycler does not exist in the dummy row and if at some point is implemented, it should be in group view.  only subject, total, and unread Cols have values in the dummy header row.

so i think 
if (!(m_flags[aRow] & MSG_VIEW_FLAG_DUMMY) || aColumnName.EqualsLiteral("unreadCol"))
Attachment #8985892 - Flags: review?(alta88)
Posted patch 1466782-follow-up.patch (v8) (obsolete) — Splinter Review
Let's hope that v8 is finally right ;-)
I removed the comment since there was no need stating the obvious.
Attachment #8985892 - Attachment is obsolete: true
Attachment #8985892 - Flags: review?(acelists)
Attachment #8985979 - Flags: review?(alta88)
Attachment #8985979 - Flags: review?(acelists)
Comment on attachment 8985979 [details] [diff] [review]
1466782-follow-up.patch (v8)

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

Ok, this does answer what to do with the 'u' check. But I'll have to trust alta that this is correct.
Attachment #8985979 - Flags: review?(acelists) → review+
Comment on attachment 8985979 [details] [diff] [review]
1466782-follow-up.patch (v8)

I accidentally did a try run with this:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=dddc76898880562148fda04b727a2c17842e2820
And, lo and behold:
TEST-UNEXPECTED-FAIL | comm/mailnews/base/test/unit/test_nsMsgDBView_headerValues.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/base/test/unit/test_nsMsgDBView_headerValues.js | real_test/< - [real_test/< : 63] "" == "John Doe"
TEST-UNEXPECTED-FAIL | comm/mailnews/base/test/unit/test_viewSortByAddresses.js | xpcshell return code: 0
ReferenceError: MSG_VIEW_FLAG_DUMMY is not defined at /builds/worker/workspace/build/tests/xpcshell/tests/comm/mailnews/base/test/unit/test_viewSortByAddresses.js:122

:-(
Attachment #8985979 - Flags: review?(alta88)
Nice, did we uncover an unusual path in the test, that has MSG_VIEW_FLAG_DUMMY undefined?
It seems there is no good place to import it from, so 'var MSG_VIEW_FLAG_DUMMY = 0x20000000;' .

Also it seems the tests may be cheating, like:
gDBView.cellTextForColumn(0, "sender");

The column names are missing the "Col" suffix. Can you try that?
Now including fixes of the tests.
Attachment #8985979 - Attachment is obsolete: true
Attachment #8986024 - Flags: review?(alta88)
Attachment #8986024 - Flags: review?(acelists)
Comment on attachment 8986024 [details] [diff] [review]
1466782-follow-up.patch (v9)

WFM applied/built in grouped by date view, ie total and unread Cols are fine and update correctly on message delete and undo delete.  There is an existing issue with undo delete (and maybe new message addition) where the dummy row totalCol is not invalidated sometimes (unless hovered).
Attachment #8986024 - Flags: review?(alta88) → review+
Comment on attachment 8986024 [details] [diff] [review]
1466782-follow-up.patch (v9)

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

Thanks.
Attachment #8986024 - Flags: review?(acelists) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/802e67be3e3e
Follow-up: Fix nsMsgDBView::CellTextForColumn() to use nsString parameters. r=aceman,alta88
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f88630928880
Follow-up: Remove unneeded block. r=me DONTBUILD
You need to log in before you can comment on or make changes to this bug.