or the functionality (like creating uri's and opening channels).
Created attachment 93765 [details] [diff] [review] v1 patch see the n.p.m.netlib thread describing these changes: http://makeashorterlink.com/?I3FC42C61
http://makeashorterlink.com/?U40D33C61 (try this instead)
Comment on attachment 93765 [details] [diff] [review] v1 patch r=dougt lets define it upfront when this interface will be frozen. What are we waiting on?
hey, i had a feature i wanted in nsIOService. i'll look for the bug and see if i can get it in early in 1.2
timeless, what do you exactly mean? Do you have objection to freezing this interface as it is in the above patch and, if so, what exactly are the problems with it.
For the editor changes, I defer to Charley since I believe those are all in his code. They seem ok to me but strike me as horribly inefficient. Charley--please review the editor changes in the patch for this bug. Probably we'll need a new bug to optimize all of the uri creation.
brade: right, i looked into making editor store nsIURIs in place of URL strings, but the patch started to spiral out into /editor/ui/dialogs, so i decided it'd be best to save that work for another bug ;)
i should also have added that performance-wise, the only real difference between NewURI and ExtractUrlPart is the fact that NewURI parses to completion and stores its results in an object. otherwise, they are equivalent. (e.g., calling ExtractUrlPart twice is probably worse than calling NewURI once and then accessing the attributes on nsIURI.)
Comment on attachment 93765 [details] [diff] [review] v1 patch r=cmanske for changes in editorUtilities.js
timeless: extractAttributeValue doesn't belong on nsIIOService. read my n.p.m.netlib posting... nsIIOService has been a dumping ground for miscellaneous stuff for far too long. time to clean it up! :-)
+ * handler's NewURI should be invoked). nsIIOService also provides a set of + * URL parsing utility functions. These are provided to make the programmers + * job easier and in some cases to improve performance by eliminating + * intermediate data structures and interfaces. um.
timeless: sure you could argue that a common function for parsing attribute value strings like "?attr1=value1&attr2=value2" is needed instead of the current world of mass-duplication. i'd argue, however, that this function has nothing to do with nsIIOService. all of the functions on nsIIOService, including the URL parsing utilities, have something to do with loading an URL. ExtractScheme indicates the way in which the IOService parses an URL to determine the appropriate protocol handler. getFileFromURLSpec/getURLSpecFromFile indicates how the IOService transforms nsIFile to nsIURI::spec (and back again) without the creation of the intermediate nsIURI implementation (optimization). extractAttributeValue is much more specific... perhaps some alternate interface would be more appropriate.
43 dougt 1.44 interface nsIURLParser; seems like a much more appropriate place for all of the functions you mentioned. i'm going to take an hour to consider the offline state feature. offhand i think that it's buried too deep in the chain. actually. i know it's too deep in the chain. we have a bunch of requests for browser to be online while mail is offline or vice-versa. by sticking it into the api level for general networking you're dooming us. so i'm requesting that the following vacate nsIIOService before freezing: 108 /** 109 andreas.otte 1.47 * Returns true if networking is in "offline" mode. When in offline mode, 110 * attempts to access the network will fail (although this is not 111 * necessarily corrolated with whether there is actually a network 112 * available -- that's hard to detect without causing the dialer to 113 * come up). 114 warren 1.31 */ 115 attribute boolean offline; 127 warren 1.31 //////////////////////////////////////////////////////////////////////////// 128 // URL parsing utilities 129 130 /** 131 warren 1.22 * Utility for protocol implementors -- extracts the scheme from a URL 132 * string, consistently and according to spec. 133 darin 1.58 * 134 * @param urlString the URL string to parse 135 * @return scheme 136 * 137 * @throws NS_ERROR_MALFORMED_URI if urlString is not of the right form. 138 */ 139 ACString extractScheme(in AUTF8String urlString); 146 darin 1.58 /** 147 * @param urlString absolute URL path. 148 * @param flags combination of url_XXX flags. 149 * 150 * @throws NS_ERROR_MALFORMED_URI if urlString is not of the right form. 151 */ 152 AUTF8String extractUrlPart(in AUTF8String urlString, in short flags); 153 warren 1.27 154 /** 155 andreas.otte 1.47 * Constants for the mask in the call to extractUrlPart 156 darin 1.54 * 157 * XXX these should really be enumerated in left-to-right order! 158 warren 1.35 */ 159 const short url_Scheme = (1<<0); 160 const short url_Username = (1<<1); 161 const short url_Password = (1<<2); 162 const short url_Host = (1<<3); 163 const short url_Directory = (1<<4); 164 const short url_FileBaseName = (1<<5); 165 const short url_FileExtension = (1<<6); 166 const short url_Param = (1<<7); 167 const short url_Query = (1<<8); 168 const short url_Ref = (1<<9); 169 andreas.otte 1.49 const short url_Path = (1<<10); 170 morse 1.53 const short url_Port = (1<<11); 171 warren 1.31 172 /** 173 * Resolves a relative path string containing "." and ".." 174 * with respect to a base path (assumed to already be resolved). 175 * For example, resolving "../../foo/./bar/../baz.html" w.r.t. 176 * "/a/b/c/d/e/" yields "/a/b/c/foo/baz.html". Attempting to 177 * ascend above the base results in the NS_ERROR_MALFORMED_URI 178 * exception. If basePath is null, it treats it as "/". 179 andreas.otte 1.47 * 180 darin 1.58 * @param relativePath a relative URI 181 * @param basePath a base URI 182 * 183 * @return a new string, representing canonical uri 184 warren 1.31 */ 185 darin 1.58 AUTF8String resolveRelativePath(in AUTF8String relativePath, 186 in AUTF8String basePath); and i'm suggesting that the following also vacate it. 187 alecf 1.55 188 /** 189 * conversions between nsILocalFile and a file url string 190 */ 191 192 /** 193 * gets a file:// url out of an nsIFile 194 darin 1.58 * @param file the file to extract the path from 195 alecf 1.55 * @return a file:// style url string 196 */ 197 darin 1.58 AUTF8String getURLSpecFromFile(in nsIFile file); 198 alecf 1.55 199 /** 200 * Sets the native path of the file given the url string 201 darin 1.58 * @param file the file to initialize with the given spec 202 * @param url the url string which will be used to initialize the file 203 alecf 1.55 */ 204 darin 1.58 void initFileFromURLSpec(in nsIFile file, in AUTF8String url); 205 warren 1.1 }; to get the default urlparser to find out what a scheme is, someone can either instantiate the default urlparser or call getParserForScheme("").
good grief man!! - having offline on nsIIOService doesn't prevent mailnews from having a separate online/offline state. this attribute only indicates the state of the necko services that deal with physical connections (e.g., the socket transport service and the dns service). - why should the nsIFile <-> URLspec conversion routines "vacate" nsIIOService? some description of why you feel this way would be helpful.
- having offline on nsIIOService doesn't prevent mailnews from having a separate online/offline state. this attribute only indicates the state of the necko services that deal with physical connections (e.g., the socket transport service and the dns service). what's the point? the IOService should do IO, if someone doesn't want to do IO they shouldn't ask the IOService to do it. - why should the nsIFile <-> URLspec conversion routines "vacate" nsIIOService? some description of why you feel this way would be helpful. they don't relate to input/output, they relate to URIs so they should either live near nsIFile or nsIURI. Interfaces should be clean, make sense, and encourage good design. Unless we want them to be ugly and collect random things. Either way, you should not draw the line somewhere in the middle.
Comment on attachment 93765 [details] [diff] [review] v1 patch minor nit in nsCookies.cpp: + if (os) + os->NotifyObservers(nsnull, "cookieIcon", NS_ConvertASCIItoUCS2("on").get()); I know this was code from before, but if you're going to tweak this line, mind making this NS_LITERAL_STRING? other than that, looks great! Nice cleanup in cookies/wallet sr=alecf
will do.. thx alec.
This patch have added a "might be used uninitialized" warning (see also bug 59652) to brad TBox: +mailnews/compose/src/nsMsgAttachment.cpp:178 + `nsresult rv' might be used uninitialized in this function Indeed, now nsMsgAttachment::DeleteAttachment will always check NS_SUCCEEDED on an uninitialized nsresult!
Created attachment 94632 [details] [diff] [review] patch - fix uninitialized variable regression Aleksey: thanks for noticing the problem!!
Comment on attachment 94632 [details] [diff] [review] patch - fix uninitialized variable regression r=dougt
Comment on attachment 94632 [details] [diff] [review] patch - fix uninitialized variable regression sr=alecf
looks like the v1 patch caused a mac regression: bug 161921 :( patch has been backed out... investigating the problem...
re-landed patch after fixing mac smoketest blocker.
alecf and i spoke about adding: AUTF8String normalizeURISpec(in AUTF8String aSpec, in string aOriginCharset); global history uses an ad-hoc version of this already, so we probably want to add support for it to necko.
another potential issue to resolve... non-ASCII characters in a file:// URL are encoded using the native filesystem charset. non-ASCII bytes are URL escaped. this means that nsIIOService::GetURLSpecFromFile and GetFileFromURLSpec should perhaps use ACString instead of AUTF8String to indicate that the strings must be ASCII, and to avoid the unnecessary overhead of checking for UTF8 char sequences. alternatively, we might one day want to encode file:// URLs using UTF8, but i really think that is unlikely for compatibility reasons.
or as we discussed before, we could remove these methods from this interface.
Comment on attachment 94632 [details] [diff] [review] patch - fix uninitialized variable regression r=bbaetz, too
errr, wrong bug; ignore that
i know i originally favored keeping GetURLSpecFromFile/GetFileFromURLSpec on nsIIOService, but now that i'm having second thoughts... perhaps it makes better sense to move these onto nsIFileProtocolHandler. it does seem a lot more logical to move these there. it also wouldn't make too big of an impact on existing code because it is trivial and quick to access the file protocol handler via nsIIOService::GetProtocolHandler("file") [the file protocol handler is one of the protocol handlers cached by the io service]. monster patch coming up... please let me know if you think this isn't the right way to go!
one more thing to think about... should nsIIOService::getProtocolHandler(scheme) return the protocol handler registered for that URL scheme or should it return the protocol handler that would actually be used to load the URL? example being FTP proxied via HTTP, in which case the HTTP protocol handler would actually be invoked instead of the FTP protocol handler to load the FTP URL. this has implications for the interpretation of nsIProtocolHandler::protocolFlags. if we want to extend the protocolFlags to include transport specific attributes, then we are in a bind. thoughts?
If it is just a mapping to get the registered protocol handler, you would wonder why one wouldn't just go through the service manager to obtain the protocol handler.
GetProtocolHandler can _only_ return the given scheme's handler - think PAC, which is a per-url thing. Besides, who would want the other behaviour? The only places outside of netwerek/base (and test programs) to call this are: 1) The script security manager, for the default port, which definately wants the 'real' one 2) The GTK code, which appears to be doing severak horrible things to resolve a uri to a file. It gets the protocl handler from a scheme obtained by creating a new uri for a passed in string, then qis it to nsIResProtocolHandler, and.... Its probably buggy. The details of the protocol handler really shouldn't be important to anyone who isn't writing one.
dougt: nsIIOService::getProtocolHandler will return the default protocol handler if there is no protocol handler registered for the specified scheme, thus the fact that callers could access a protocol handler via the service manager is really just an implementation detail for protocol handler writers. consumers of protocol handler should not rely on this fact. bbaetz: right! my point (even though i failed to state it) is that the current API wouldn't suffice if we needed to return the protocol handler that would be used to fetch the URL, and so since this is an API bug.... we need to nail down the semantics of getProtocolHandler. i think we should just stick with the current API, but make it explicitly clear in the documentation that the protocol handler returned is not necessarily the same protocol handler that will be used to fetch the resource. in fact, we don't really have an API that allows one to determine the protocol handler that would be used to create the channel... should we?
-> 1.2final... most likely going to be marked frozen as is.
ok, last chance to make changes to this interface. will mark this interface frozen tomorrow.
nsIIOService.idl has been marked frozen on the trunk. marking this bug FIXED.
Verified patch checkins against Mozilla 1.2b Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.2b) Gecko/20021026 build. Wrt to the patch, getURLSpecFromFile() and getFileFromURLSpec() have since moved to nsIFileProtocolHandler. Other methods like getParserForScheme() and extractUrlPart() have been removed (though I spotted one instance of it used in nsGlobalHistory.cpp). OS specific IOService* implementations have been merged into URLHelper files (i.e. nsIOServiceMac.cpp -> nsURLHelperMac.cpp, same for OS2, Unix, and Win).