Closed Bug 459693 Opened 11 years ago Closed 10 years ago

Eliminate nsFileSpec and nsIFileSpec (references) from MailNews

Categories

(MailNews Core :: Backend, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.1a1

People

(Reporter: sgautherie, Assigned: sgautherie)

References

()

Details

Attachments

(10 files, 1 obsolete file)

7.13 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
1.67 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
7.78 KB, patch
neil
: review+
Details | Diff | Splinter Review
2.99 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
1.87 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
1.19 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
3.50 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
6.31 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
1.80 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
2.16 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
No description provided.
Depends on: 33451
Depends on: 355008
Attached patch (Cv1) <amUtils.js> (obsolete) — Splinter Review
Update quite old code.
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #342938 - Flags: superreview?(neil)
Attachment #342938 - Flags: review?(neil)
Attachment #342939 - Flags: superreview?(bienvenu)
Attachment #342939 - Flags: review?(bienvenu)
Attachment #342940 - Flags: superreview?(bienvenu)
Attachment #342940 - Flags: review?(bienvenu)
Attachment #342939 - Flags: superreview?(bienvenu)
Attachment #342939 - Flags: superreview+
Attachment #342939 - Flags: review?(bienvenu)
Attachment #342939 - Flags: review+
Attachment #342940 - Flags: superreview?(bienvenu)
Attachment #342940 - Flags: superreview+
Attachment #342940 - Flags: review?(bienvenu)
Attachment #342940 - Flags: review+
Comment on attachment 342939 [details] [diff] [review]
(Av1) Bug 33451 followup
[Checkin: Comment 4]

http://hg.mozilla.org/comm-central/rev/cbb98a8d6e8d
Attachment #342939 - Attachment description: (Av1) Bug 33451 followup → (Av1) Bug 33451 followup [Checkin: Comment 4]
Comment on attachment 342940 [details] [diff] [review]
(Bv1) Bug 355008 followup
[Checkin: Comment 5]

http://hg.mozilla.org/comm-central/rev/c5f2947a86f1
Attachment #342940 - Attachment description: (Bv1) Bug 355008 followup → (Bv1) Bug 355008 followup [Checkin: Comment 5]
Comment on attachment 342938 [details] [diff] [review]
(Cv1) <amUtils.js>

>   var ret = fp.show();
> 
>-  if (ret == nsIFilePicker.returnOK) 
>+  if (ret == nsIFilePicker.returnOK)
Nit: could inline ret

>+      var currentServer = allServers.GetElementAt(i)
>+                                    .QueryInterface(Components.interfaces.nsIMsgIncomingServer);
Nit: should use QueryElementAt

>+          currentServer.localPath.nativePath == folderPath)
This should use .localPath.equals(fp.file)
(It doesn't work as-is, because nativePath isn't scriptable on nsIFile)
Attachment #342938 - Flags: superreview?(neil)
Attachment #342938 - Flags: superreview-
Attachment #342938 - Flags: review?(neil)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081014 SeaMonkey/2.0a2pre] (nightly) (W2Ksp4)

Cv1, with comment 6 suggestion(s),
plus a more detailed error message.
Attachment #342938 - Attachment is obsolete: true
Attachment #343326 - Flags: superreview?(neil)
Attachment #343326 - Flags: review?(neil)
David, could you fix
{
/mailnews/import/outlook/src/nsOutlookMail.cpp
    * line 714 -- nsCOMPtr<nsIFileSpec> pSpec;
}
which is a leftover from bug 33451 ?
(Update the code or maybe remove this commented out function !?)
Thanks.
Attachment #343326 - Flags: superreview?(neil)
Attachment #343326 - Flags: superreview+
Attachment #343326 - Flags: review?(neil)
Attachment #343326 - Flags: review+
Attachment #343326 - Attachment description: (Cv2) <amUtils.js> → (Cv2) <amUtils.js> [Checkin: Comment 9]
(compiled, not run, but quite obvious)
Attachment #344214 - Flags: superreview?(bienvenu)
Attachment #344214 - Flags: review?(bienvenu)
Attachment #344214 - Flags: superreview?(bienvenu)
Attachment #344214 - Flags: superreview+
Attachment #344214 - Flags: review?(bienvenu)
Attachment #344214 - Flags: review+
Comment on attachment 344214 [details] [diff] [review]
(Dv1) Bug 33451 <nsMsgMailNewsUrl.cpp> followup ++
[Checkin: Comment 12]

r/sr=me, if you move the decl of nsresult down to where it's used:

 nsresult nsMsgSaveAsListener::SetupMsgWriteStream(nsIFile *aFile, PRBool addDummyEnvelope)
 {
-  nsresult rv = NS_ERROR_FAILURE;
+  nsresult rv;
Attachment #344214 - Attachment description: (Dv1) Bug 33451 <nsMsgMailNewsUrl.cpp> followup ++ → (Dv1) Bug 33451 <nsMsgMailNewsUrl.cpp> followup ++ [Checkin: Comment 12]
Attachment #344326 - Flags: superreview?(bienvenu)
Attachment #344326 - Flags: review?(bienvenu)
Attachment #344326 - Flags: superreview?(bienvenu)
Attachment #344326 - Flags: superreview+
Attachment #344326 - Flags: review?(bienvenu)
Attachment #344326 - Flags: review+
Comment on attachment 344326 [details] [diff] [review]
(Ev1) Bug 33451 <nsImapMailFolder.cpp> followup
[Checkin: Comment 15]

http://hg.mozilla.org/comm-central/rev/13727f26b4a9
Attachment #344326 - Attachment description: (Ev1) Bug 33451 <nsImapMailFolder.cpp> followup → (Ev1) Bug 33451 <nsImapMailFolder.cpp> followup [Checkin: Comment 15]
I don't know if using moveTo() would be "better" or not. (as a followup)

See
http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/io/nsIFile.idl?mark=195-196#163
Attachment #344419 - Flags: superreview?(bienvenu)
Attachment #344419 - Flags: review?(bienvenu)
Attachment #344419 - Flags: superreview?(bienvenu)
Attachment #344419 - Flags: superreview+
Attachment #344419 - Flags: review?(bienvenu)
Attachment #344419 - Flags: review+
Comment on attachment 344419 [details] [diff] [review]
(Fv1) Bug 98391 then bug 33451 <nsLocalMailFolder.cpp> followup
[Checkin: Comment 17]

http://hg.mozilla.org/comm-central/rev/9c1554e52ae8
Attachment #344419 - Attachment description: (Fv1) Bug 98391 then bug 33451 <nsLocalMailFolder.cpp> followup → (Fv1) Bug 98391 then bug 33451 <nsLocalMailFolder.cpp> followup [Checkin: Comment 17]
David, could you fix
{
/mailnews/local/src/nsMailboxUrl.cpp
    * line 174 -- // mURI isn't set due to nsFileSpec/nsIFile fun.
}
which seems to be a leftover from bug 33451 ?
Thanks.

See
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/mailnews/local/src/nsMailboxUrl.h&mark=1.44
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/mailnews/local/src/nsMailboxUrl.cpp&mark=1.102#1.106
when |m_filePath| and |m_messageFile| changed from nsFileSpec to nsILocalFile.
David, could you fix
{
/mailnews/import/eudora/src/nsEudoraMailbox.cpp
    * line 447 -- // to reopen it because the nsIFileSpec implementation will open
}
which seems to be a "missed update" from bug 368347 3rd patch ?
Thanks.

See
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/mailnews/import/eudora/src/nsEudoraMailbox.cpp&mark=1.31
when this function (with |nsIOutputStream *pDst|) and comment were added.
Depends on: 368347
Flags: wanted-thunderbird3?
Keywords: helpwanted
Target Milestone: --- → mozilla1.9.1b2
Whiteboard: [ToDo: comments 8, 18 and 19]
Attachment #345911 - Flags: superreview?(bienvenu)
Attachment #345911 - Flags: superreview+
Attachment #345911 - Flags: review?(bienvenu)
Attachment #345911 - Flags: review+
Comment on attachment 345911 [details] [diff] [review]
(Gv1) Bug 33451 <nsMsgDatabase.*> followup
[Checkin: Comment 21]

http://hg.mozilla.org/comm-central/rev/2fd2d404bcd3
Attachment #345911 - Attachment description: (Gv1) Bug 33451 <nsMsgDatabase.*> followup → (Gv1) Bug 33451 <nsMsgDatabase.*> followup [Checkin: Comment 21]
wanted‑thunderbird3+
Serge, are you working on these? Only tree references to go now!

The commented out outlook function should just be removed. For eudora, the matching line and a couple of uncommented lines after it. For the last comment ref, I think it should just be "isn't set due to nsIFile fun"
Flags: wanted-thunderbird3? → wanted-thunderbird3+
Per comment 8 and comment 22.
Attachment #406385 - Flags: superreview?(bienvenu)
Attachment #406385 - Flags: review?(bienvenu)
Attachment #406385 - Flags: approval-thunderbird3?
Comment on attachment 406385 [details] [diff] [review]
(Hv1) Bug 33451 <nsOutlookMail.cpp> followup
[Checkin: Comment 29]

Serge, please don't request approval until you have a reviewed patch - this way we can track requests better and spend less time working out what has/hasn't been reviewed.

Please also include risk statements when requesting approval as to what the patch does and could affect if it is wrong - even if brief.
Attachment #406385 - Flags: approval-thunderbird3?
Comment on attachment 406385 [details] [diff] [review]
(Hv1) Bug 33451 <nsOutlookMail.cpp> followup
[Checkin: Comment 29]

I see no risk but no reward either for landing this in 3.0 since it's merely removing commented out code.
Attachment #406385 - Flags: superreview?(bienvenu)
Attachment #406385 - Flags: superreview+
Attachment #406385 - Flags: review?(bienvenu)
Attachment #406385 - Flags: review+
Comment on attachment 406385 [details] [diff] [review]
(Hv1) Bug 33451 <nsOutlookMail.cpp> followup
[Checkin: Comment 29]


(In reply to comment #25)
> I see no risk but no reward either for landing this in 3.0 since it's merely
> removing commented out code.

100% agreed, but it would let me(/us) move forward.
Attachment #406385 - Flags: approval-thunderbird3?
Per comment 18 and comment 22.
(Note that I have no idea what the comment should be or whether it even still applies.)
Attachment #406634 - Flags: superreview?(bienvenu)
Attachment #406634 - Flags: review?(bienvenu)
Comment on attachment 406385 [details] [diff] [review]
(Hv1) Bug 33451 <nsOutlookMail.cpp> followup
[Checkin: Comment 29]

a=Standard8 as this is comment-only removals, and only if this lands before we branch (once we branch it will be very unlikely that tidy up patches will land, but as this is comment only, then I see low risk).
Attachment #406385 - Flags: approval-thunderbird3? → approval-thunderbird3+
Comment on attachment 406385 [details] [diff] [review]
(Hv1) Bug 33451 <nsOutlookMail.cpp> followup
[Checkin: Comment 29]


http://hg.mozilla.org/comm-central/rev/25fe4f5e7702
Attachment #406385 - Attachment description: (Hv1) Bug 33451 <nsOutlookMail.cpp> followup → (Hv1) Bug 33451 <nsOutlookMail.cpp> followup [Checkin: Comment 29]
Attachment #406634 - Flags: superreview?(bienvenu)
Attachment #406634 - Flags: superreview+
Attachment #406634 - Flags: review?(bienvenu)
Attachment #406634 - Flags: review+
Comment on attachment 406634 [details] [diff] [review]
(Iv1) Bug 33451 <nsMailboxUrl.cpp> followup ++
[Checkin: Comment 30]


http://hg.mozilla.org/comm-central/rev/02601e5f7bb2
Attachment #406634 - Attachment description: (Iv1) Bug 33451 <nsMailboxUrl.cpp> followup ++ → (Iv1) Bug 33451 <nsMailboxUrl.cpp> followup ++ [Checkin: Comment 30]
Keywords: helpwanted
Whiteboard: [ToDo: comments 8, 18 and 19]
Attachment #412334 - Flags: superreview?(bienvenu)
Attachment #412334 - Flags: superreview+
Attachment #412334 - Flags: review?(bienvenu)
Attachment #412334 - Flags: review+
Comment on attachment 412334 [details] [diff] [review]
(Jv1) Bug 368347 <nsEudoraMailbox.cpp> followup
[Checkin: Comment 32]


http://hg.mozilla.org/comm-central/rev/6ea33184f759
Attachment #412334 - Attachment description: (Jv1) Bug 368347 <nsEudoraMailbox.cpp> followup → (Jv1) Bug 368347 <nsEudoraMailbox.cpp> followup [Checkin: Comment 32]
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9.1b2 → Thunderbird 3.1a1
You need to log in before you can comment on or make changes to this bug.