Closed
Bug 756316
Opened 12 years ago
Closed 12 years ago
Cleanup nsMsgLocalMailFolder::GetSubFolders
Categories
(MailNews Core :: Backend, defect)
MailNews Core
Backend
Tracking
(thunderbird14 fixed, thunderbird15 fixed)
RESOLVED
FIXED
Thunderbird 16.0
People
(Reporter: hiro, Assigned: hiro)
References
Details
Attachments
(1 file, 4 obsolete files)
11.63 KB,
patch
|
Bienvenu
:
review+
Bienvenu
:
approval-comm-aurora+
standard8
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
Found that the codes in GetSubFolders() are redundant because the most of the codes done in DiscoverSubFolders().
Attachment #624957 -
Flags: review?(dbienvenu)
Assignee | ||
Comment 1•12 years ago
|
||
I am sorry that the previous patch can not be compiled.
Assignee: nobody → hiikezoe
Attachment #624957 -
Attachment is obsolete: true
Attachment #624957 -
Flags: review?(dbienvenu)
Attachment #624967 -
Flags: review?(dbienvenu)
Assignee | ||
Comment 2•12 years ago
|
||
Comment on attachment 624967 [details] [diff] [review] fix This patch causes the problem that folder name is not translated.
Attachment #624967 -
Flags: review?(dbienvenu)
Comment 3•12 years ago
|
||
Comment on attachment 624967 [details] [diff] [review] fix As a general rule, I'd rather have less code duplicated code in the pluggable stores and more in the core classes, like nsMsgLocalMailFolder, to lessen the burden on new pluggable store implementors. So I'd prefer removing this cdoe from the pluggable stores, if possible.
Assignee | ||
Comment 4•12 years ago
|
||
(In reply to David :Bienvenu from comment #3) > Comment on attachment 624967 [details] [diff] [review] > fix > > As a general rule, I'd rather have less code duplicated code in the > pluggable stores and more in the core classes, like nsMsgLocalMailFolder, to > lessen the burden on new pluggable store implementors. So I'd prefer > removing this cdoe from the pluggable stores, if possible. At first, I tried to remove the codes in pluggable stores but I quited because directory structure depends on the pluggable store. Anyway you are absolutely right. There are still the codes independent from directory structure. Those codes should be in nsLocalMailFolder.cpp.
Assignee | ||
Comment 5•12 years ago
|
||
This patch needs the fix for bug 759237.
Attachment #624967 -
Attachment is obsolete: true
Attachment #627857 -
Flags: review?(dbienvenu)
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 6•12 years ago
|
||
The previous patch allowed to be more cleaned up.
Attachment #627857 -
Attachment is obsolete: true
Attachment #627857 -
Flags: review?(dbienvenu)
Attachment #627873 -
Flags: review?(dbienvenu)
Comment 7•12 years ago
|
||
Comment on attachment 627873 [details] [diff] [review] Fix this is slightly bit-rotted, perhaps by my slight tweaks to your addsubfolders patch. Since I probably bit-rotted you, I'll try to fix it myself and upload a de-bitrotted patch. This looks like a nice cleanup.
Comment 8•12 years ago
|
||
I'll try this patch...
Attachment #627873 -
Attachment is obsolete: true
Attachment #627873 -
Flags: review?(dbienvenu)
Attachment #628539 -
Flags: review?(dbienvenu)
Comment 9•12 years ago
|
||
Comment on attachment 628539 [details] [diff] [review] de-bitrotted patch looks good, thx very much for the patch!
Attachment #628539 -
Flags: review?(dbienvenu) → review+
Updated•12 years ago
|
Keywords: checkin-needed
Comment 10•12 years ago
|
||
https://hg.mozilla.org/comm-central/rev/9c0bd4891167
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 16.0
Comment 11•12 years ago
|
||
Comment on attachment 628539 [details] [diff] [review] de-bitrotted patch [Approval Request Comment] Regression caused by (bug #): User impact if declined: Testing completed (on c-c, etc.): Risk to taking this patch (and alternatives if risky): [Triage Comment] landing this on aurora and beta makes landing the fix for bug 751652 quite a bit easier. I'm going to land it on aurora and I think we should consider it for beta.
Attachment #628539 -
Flags: approval-comm-beta?
Attachment #628539 -
Flags: approval-comm-aurora+
Comment 12•12 years ago
|
||
(In reply to David :Bienvenu from comment #11) > landing this on aurora and beta makes landing the fix for bug 751652 quite a > bit easier. I'm going to land it on aurora and I think we should consider it > for beta. I think that's the wrong bug number.
Comment 13•12 years ago
|
||
sorry, bug 751562
Comment 14•12 years ago
|
||
http://hg.mozilla.org/releases/comm-aurora/rev/43254a94f3b1
status-thunderbird15:
--- → fixed
Comment 15•12 years ago
|
||
Comment on attachment 628539 [details] [diff] [review] de-bitrotted patch It looks lowish risk, so a=me.
Attachment #628539 -
Flags: approval-comm-beta? → approval-comm-beta+
Comment 16•12 years ago
|
||
fixed for comm-beta - http://hg.mozilla.org/releases/comm-beta/rev/d3988e6258ca
status-thunderbird14:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•