166792 - move nsIIOService::GetURLSpecFromFile, etc. to nsIFileProtocolHandler

Status: VERIFIED FIXED

(Core :: Networking, defect, P2)

Description

[Darin Fisher]

move nsIIOService::GetURLSpecFromFile, etc. to nsIFileProtocolHandler.  this
just makes sense.

Comment 1

[Darin Fisher]

Attachment: v1 patch

so, i ended up refactoring some necko internals now that nsIOService doesn't
own these methods.  also, i'm eliminating the whole URL parser category thingy
that wasn't used for anything but looking up the correct nsIURLParser to use
for file:// URLs.  well, that can be easily hard coded.  i'm also renaming
nsIOServiceXXX.cpp as nsURLHelperXXX.cpp since the former name doesn't make any
sense now.

Comment 2

[Darin Fisher]

Attachment: v1.1 patch

realized that i forgot to remove the cached URL parser list from the IO
service.  not used, not needed :)

Comment 3

[Roy Yokoyama]

darin: can you use nsIFile::GetPath() instead?
+    // construct URL spec from native file path
+    rv = aFile->GetNativePath(ePath);

If not, then you need IsDBCSLeadByte() stuff in SwapSlashColon() as
http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsIOServiceWin.cpp#62

Comment 4

[Darin Fisher]

roy: the code for nsURLHelperWin.cpp is an almost identical copy of the code
from nsIOServiceWin.cpp, so i don't see why it should be changed.  if there's a
bug in the impl of nsIOServiceWin.cpp, then let's make that another bug report.

Comment 5

[Doug Turner (:dougt)]

Comment on attachment 97922 [details] [diff] [review]
v1.1 patch

r=dougt


(how long until we freeze nsIFileProtocalHandler?)

Comment 6

[Darin Fisher]

let's not talk about freezing nsIFileProtocolHandler ;-)

Comment 7

[Alec Flett]

I'm hoping to get to this today..

Comment 8

[Alec Flett]

Comment on attachment 97922 [details] [diff] [review]
v1.1 patch

+LOCAL_INCLUDES =			 \
+	 -I$(topsrcdir)/netwerk/base/src \
+	 $(NULL)
+

my understanding is that we normally use relative directories here, i.e.
-I$(srcdir)/../base/src

so that the module doesn't need to know where it is in the tree.

other than that, this looks fine. 
sr=alecf

Comment 9

[Darin Fisher]

thx for the tip alec!

Comment 10

[Darin Fisher]

fixed-on-trunk

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The addBookmark.js change looks like it's only partly complete.

Comment 12

[Darin Fisher]

thx dbaron... i corrected that mistake.

Comment 13

[(not reading, please use [e-mail] instead)]

oops, you forgot one in MsgComposeCommands.js:

which broke attaching files in the compose window.

it's fixed now:

Index: compose/resources/content/MsgComposeCommands.js
===================================================================
RCS file: /cvsroot/mozilla/mailnews/compose/resources/content/MsgComposeCommands
.js,v
retrieving revision 1.290
diff -r1.290 MsgComposeCommands.js
2229c2229,2230
<     var currentAttachment = ioService.getURLSpecFromFile(currentFile);
---
>     var fileHandler = ioService.getProtocolHandler("file").QueryInterface(Comp
onents.interfaces.nsIFileProtocolHandler);
>     var currentAttachment = fileHandler.getURLSpecFromFile(currentFile);

Comment 14

[benc]

Darin: anything I can do to verify, besides run a URL functional test?

Comment 15

[Darin Fisher]

benc: should be sufficient to verify this API change using LXR.

Comment 16

[benc]

checked both files w/ lxr and the function has been moved.