Closed
Bug 1466782
Opened 6 years ago
Closed 6 years ago
Port bug 1466673 (replace use of nsITreeColumns) and bug 1466727 (replace use of nsITreeColumn)
Categories
(MailNews Core :: General, enhancement)
MailNews Core
General
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 62.0
People
(Reporter: jorgk-bmo, Assigned: jorgk-bmo)
Details
Attachments
(3 files, 12 obsolete files)
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
Comment 1•6 years ago
|
||
Ideal timing would be to land the treecolumn changes before you write the code for those C++ trees, right?
Assignee | ||
Comment 2•6 years ago
|
||
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.
Comment 3•6 years ago
|
||
Ah, yeah, if you have to backport across these changes that's annnoying. Not sure I can do much about that...
Assignee | ||
Comment 4•6 years ago
|
||
Delay your stuff by a week?
Comment 5•6 years ago
|
||
I already landed some of it... But also, it's blocking some other things I would really like to get to soonish, sorry.
Assignee | ||
Comment 6•6 years ago
|
||
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.
Assignee | ||
Comment 7•6 years ago
|
||
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).
Comment 8•6 years ago
|
||
> OK, then get it over and done with quickly and land bug 1466727 as well.
Yes, working on it.
Comment 9•6 years ago
|
||
Comment on attachment 8983552 [details] [diff] [review] 1466782-replace-nsITreeColumns.patch r=me
Attachment #8983552 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Comment 10•6 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/bfc783ee07fa Port bug 1466673: replace use of nsITreeColumns. r=bz
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Assignee | ||
Comment 11•6 years ago
|
||
Attachment #8983661 -
Flags: review?(bzbarsky)
Comment 12•6 years ago
|
||
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+
Assignee | ||
Comment 13•6 years ago
|
||
So here we have it with a lot of string changes.
Attachment #8983661 -
Attachment is obsolete: true
Assignee | ||
Comment 14•6 years ago
|
||
Comment on attachment 8983902 [details] [diff] [review] 1466782-replace-nsITreeColumn.patch (v2) Quite tedious to look at :-(
Attachment #8983902 -
Flags: review?(bzbarsky)
Comment 15•6 years ago
|
||
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+
Comment 16•6 years ago
|
||
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: 6 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 17•6 years ago
|
||
Boris, I fixed that comment. Thanks for the review.
Target Milestone: --- → Thunderbird 62.0
Assignee | ||
Comment 18•6 years ago
|
||
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 19•6 years ago
|
||
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)
Assignee | ||
Comment 20•6 years ago
|
||
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 21•6 years ago
|
||
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+
Assignee | ||
Comment 22•6 years ago
|
||
(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.
Comment 23•6 years ago
|
||
(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).
Assignee | ||
Comment 24•6 years ago
|
||
(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®exp=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.
Comment 25•6 years ago
|
||
> 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.
Assignee | ||
Comment 26•6 years ago
|
||
This compares the full column name. BTW, the column names can be found in: https://dxr.mozilla.org/comm-central/rev/51678c5b0b3edd827c57a70f1c5baf02d44acead/mail/base/content/messenger.xul#521 https://dxr.mozilla.org/comm-central/rev/51678c5b0b3edd827c57a70f1c5baf02d44acead/mail/base/content/folderDisplay.js#384
Attachment #8983970 -
Attachment is obsolete: true
Attachment #8983970 -
Flags: review?(acelists)
Attachment #8984811 -
Flags: review?(acelists)
Assignee | ||
Comment 27•6 years ago
|
||
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 28•6 years ago
|
||
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+
Assignee | ||
Comment 29•6 years ago
|
||
(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)
Comment 30•6 years ago
|
||
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.
Assignee | ||
Comment 31•6 years ago
|
||
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)
Assignee | ||
Comment 32•6 years ago
|
||
I think I covered all single-character compares now.
Attachment #8984883 -
Flags: review?(acelists)
Comment 33•6 years ago
|
||
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+
Assignee | ||
Comment 34•6 years ago
|
||
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)
Assignee | ||
Comment 35•6 years ago
|
||
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)
Assignee | ||
Comment 36•6 years ago
|
||
Attachment #8985864 -
Attachment is obsolete: true
Attachment #8985864 -
Flags: feedback?(alta88)
Attachment #8985866 -
Flags: feedback?(alta88)
Comment 37•6 years ago
|
||
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+
Assignee | ||
Comment 38•6 years ago
|
||
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)
Assignee | ||
Comment 39•6 years ago
|
||
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)
Comment 40•6 years ago
|
||
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.
Assignee | ||
Comment 41•6 years ago
|
||
(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 42•6 years ago
|
||
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)
Assignee | ||
Comment 43•6 years ago
|
||
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 44•6 years ago
|
||
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+
Assignee | ||
Comment 45•6 years ago
|
||
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)
Comment 46•6 years ago
|
||
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?
Assignee | ||
Comment 47•6 years ago
|
||
Now including fixes of the tests.
Attachment #8985979 -
Attachment is obsolete: true
Attachment #8986024 -
Flags: review?(alta88)
Attachment #8986024 -
Flags: review?(acelists)
Comment 48•6 years ago
|
||
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 49•6 years ago
|
||
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+
Comment 50•6 years ago
|
||
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
Comment 51•6 years ago
|
||
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.
Description
•