Last Comment Bug 157131 - [meta] nsIIOService need to be frozen
: [meta] nsIIOService need to be frozen
Status: VERIFIED FIXED
: arch, meta, topembed+
Product: Core
Classification: Components
Component: Embedding: APIs (show other bugs)
: Trunk
: All All
: P2 major (vote)
: mozilla1.2final
Assigned To: Darin Fisher
: David Epstein
: Myk Melez [:myk] [@mykmelez]
Mentors:
Depends on: 119051 166792
Blocks: 70229 157137
  Show dependency treegraph
 
Reported: 2002-07-12 08:37 PDT by Doug Turner (:dougt)
Modified: 2002-10-31 18:30 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 patch (75.89 KB, patch)
2002-08-02 15:10 PDT, Darin Fisher
doug.turner: review+
alecf: superreview+
Details | Diff | Splinter Review
patch - fix uninitialized variable regression (1.79 KB, patch)
2002-08-09 08:21 PDT, Darin Fisher
doug.turner: review+
alecf: superreview+
Details | Diff | Splinter Review

Description Doug Turner (:dougt) 2002-07-12 08:37:02 PDT
or the functionality (like creating uri's and opening channels).
Comment 1 Doug Turner (:dougt) 2002-07-25 09:59:36 PDT
darin
Comment 2 Darin Fisher 2002-08-02 15:10:50 PDT
Created attachment 93765 [details] [diff] [review]
v1 patch

see the n.p.m.netlib thread describing these changes:

http://makeashorterlink.com/?I3FC42C61
Comment 3 Darin Fisher 2002-08-02 15:11:56 PDT
http://makeashorterlink.com/?U40D33C61 (try this instead)
Comment 4 Doug Turner (:dougt) 2002-08-02 23:57:11 PDT
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?
Comment 5 timeless 2002-08-04 17:43:20 PDT
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
Comment 6 Doug Turner (:dougt) 2002-08-04 23:18:36 PDT
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.  
Comment 7 Kathleen Brade 2002-08-05 08:40:55 PDT
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.
Comment 8 Darin Fisher 2002-08-05 08:43:29 PDT
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 ;)
Comment 9 Darin Fisher 2002-08-05 08:45:14 PDT
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 10 Charles Manske 2002-08-05 10:45:29 PDT
Comment on attachment 93765 [details] [diff] [review]
v1 patch

r=cmanske
for changes in editorUtilities.js
Comment 11 Darin Fisher 2002-08-05 11:13:38 PDT
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! :-)
Comment 12 timeless 2002-08-05 12:02:04 PDT
+ * 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.
Comment 13 Darin Fisher 2002-08-05 12:12:13 PDT
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.
Comment 14 timeless 2002-08-05 12:26:18 PDT
 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("").
Comment 15 Darin Fisher 2002-08-05 12:54:33 PDT
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.
Comment 16 timeless 2002-08-05 13:08:26 PDT
- 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 17 Alec Flett 2002-08-07 14:56:01 PDT
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
Comment 18 Darin Fisher 2002-08-07 14:57:08 PDT
will do.. thx alec.
Comment 19 Aleksey Nogin 2002-08-08 18:30:29 PDT
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!
Comment 20 Darin Fisher 2002-08-09 08:21:54 PDT
Created attachment 94632 [details] [diff] [review]
patch - fix uninitialized variable regression

Aleksey: thanks for noticing the problem!!
Comment 21 Doug Turner (:dougt) 2002-08-09 08:51:40 PDT
Comment on attachment 94632 [details] [diff] [review]
patch - fix uninitialized variable regression

r=dougt
Comment 22 Alec Flett 2002-08-09 10:32:04 PDT
Comment on attachment 94632 [details] [diff] [review]
patch - fix uninitialized variable regression

sr=alecf
Comment 23 Darin Fisher 2002-08-09 17:18:28 PDT
looks like the v1 patch caused a mac regression: bug 161921 :(

patch has been backed out... investigating the problem...
Comment 24 Darin Fisher 2002-08-15 11:49:04 PDT
re-landed patch after fixing mac smoketest blocker.
Comment 25 Darin Fisher 2002-08-27 14:33:46 PDT
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.
Comment 26 Darin Fisher 2002-09-01 17:04:14 PDT
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.
Comment 27 Doug Turner (:dougt) 2002-09-01 17:05:55 PDT
or as we discussed before, we could remove these methods from this interface.
Comment 28 Bradley Baetz (:bbaetz) 2002-09-02 17:26:40 PDT
Comment on attachment 94632 [details] [diff] [review]
patch - fix uninitialized variable regression

r=bbaetz, too
Comment 29 Bradley Baetz (:bbaetz) 2002-09-02 17:48:13 PDT
errr, wrong bug; ignore that
Comment 30 Darin Fisher 2002-09-03 11:45:31 PDT
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!
Comment 31 Darin Fisher 2002-09-12 16:03:29 PDT
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?
Comment 32 Doug Turner (:dougt) 2002-09-12 19:23:06 PDT
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.
Comment 33 Bradley Baetz (:bbaetz) 2002-09-12 19:47:50 PDT
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.
Comment 34 Darin Fisher 2002-09-12 20:09:23 PDT
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?
Comment 35 Darin Fisher 2002-10-20 16:11:52 PDT
-> 1.2final... most likely going to be marked frozen as is.
Comment 36 Darin Fisher 2002-10-25 16:48:47 PDT
ok, last chance to make changes to this interface.  will mark this interface
frozen tomorrow.
Comment 37 Darin Fisher 2002-10-29 11:53:19 PST
nsIIOService.idl has been marked frozen on the trunk.  marking this bug FIXED.
Comment 38 David Epstein 2002-10-31 18:30:56 PST
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).

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