Last Comment Bug 756316 - Cleanup nsMsgLocalMailFolder::GetSubFolders
: Cleanup nsMsgLocalMailFolder::GetSubFolders
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 16.0
Assigned To: Hiroyuki Ikezoe (:hiro)
:
Mentors:
Depends on: 759237
Blocks: 749574
  Show dependency treegraph
 
Reported: 2012-05-17 16:59 PDT by Hiroyuki Ikezoe (:hiro)
Modified: 2012-07-03 09:21 PDT (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed


Attachments
fix (3.15 KB, patch)
2012-05-17 16:59 PDT, Hiroyuki Ikezoe (:hiro)
no flags Details | Diff | Splinter Review
fix (3.20 KB, patch)
2012-05-17 17:19 PDT, Hiroyuki Ikezoe (:hiro)
no flags Details | Diff | Splinter Review
Fix (13.50 KB, patch)
2012-05-28 23:20 PDT, Hiroyuki Ikezoe (:hiro)
no flags Details | Diff | Splinter Review
Fix (13.87 KB, patch)
2012-05-29 01:13 PDT, Hiroyuki Ikezoe (:hiro)
no flags Details | Diff | Splinter Review
de-bitrotted patch (11.63 KB, patch)
2012-05-30 16:51 PDT, David :Bienvenu
mozilla: review+
mozilla: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Hiroyuki Ikezoe (:hiro) 2012-05-17 16:59:23 PDT
Created attachment 624957 [details] [diff] [review]
fix

Found that the codes in GetSubFolders() are redundant because the most of the codes done in DiscoverSubFolders().
Comment 1 Hiroyuki Ikezoe (:hiro) 2012-05-17 17:19:36 PDT
Created attachment 624967 [details] [diff] [review]
fix

I am sorry that the previous patch can not be compiled.
Comment 2 Hiroyuki Ikezoe (:hiro) 2012-05-17 17:54:03 PDT
Comment on attachment 624967 [details] [diff] [review]
fix

This patch causes the problem that folder name is not translated.
Comment 3 David :Bienvenu 2012-05-24 13:12:19 PDT
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.
Comment 4 Hiroyuki Ikezoe (:hiro) 2012-05-24 19:32:06 PDT
(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.
Comment 5 Hiroyuki Ikezoe (:hiro) 2012-05-28 23:20:08 PDT
Created attachment 627857 [details] [diff] [review]
Fix

This patch needs the fix for bug 759237.
Comment 6 Hiroyuki Ikezoe (:hiro) 2012-05-29 01:13:26 PDT
Created attachment 627873 [details] [diff] [review]
Fix

The previous patch allowed to be more cleaned up.
Comment 7 David :Bienvenu 2012-05-30 15:50:49 PDT
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 David :Bienvenu 2012-05-30 16:51:49 PDT
Created attachment 628539 [details] [diff] [review]
de-bitrotted patch

I'll try this patch...
Comment 9 David :Bienvenu 2012-05-31 08:37:23 PDT
Comment on attachment 628539 [details] [diff] [review]
de-bitrotted patch

looks good, thx very much for the patch!
Comment 10 Ryan VanderMeulen [:RyanVM] 2012-06-14 15:36:02 PDT
https://hg.mozilla.org/comm-central/rev/9c0bd4891167
Comment 11 David :Bienvenu 2012-06-29 13:58:08 PDT
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.
Comment 12 Mark Banner (:standard8) 2012-06-29 14:03:25 PDT
(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 David :Bienvenu 2012-06-29 14:16:58 PDT
sorry, bug 751562
Comment 15 Mark Banner (:standard8) 2012-07-03 04:29:05 PDT
Comment on attachment 628539 [details] [diff] [review]
de-bitrotted patch

It looks lowish risk, so a=me.
Comment 16 David :Bienvenu 2012-07-03 09:21:25 PDT
fixed for comm-beta - http://hg.mozilla.org/releases/comm-beta/rev/d3988e6258ca

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