157131 - [meta] nsIIOService need to be frozen

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect, P2)

Description

[Doug Turner (:dougt)]

or the functionality (like creating uri's and opening channels).

Comment 1

[Doug Turner (:dougt)]

darin

Comment 2

[Darin Fisher]

Attachment: v1 patch

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

http://makeashorterlink.com/?I3FC42C61

Comment 3

[Darin Fisher]

http://makeashorterlink.com/?U40D33C61 (try this instead)

Comment 4

[Doug Turner (:dougt)]

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]

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)]

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]

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]

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]

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]

Comment on attachment 93765 [details] [diff] [review]
v1 patch

r=cmanske
for changes in editorUtilities.js

Comment 11

[Darin Fisher]

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]

+ * 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]

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]

 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]

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]

- 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]

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]

will do.. thx alec.

Comment 19

[Aleksey Nogin]

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]

Attachment: patch - fix uninitialized variable regression

Aleksey: thanks for noticing the problem!!

Comment 21

[Doug Turner (:dougt)]

Comment on attachment 94632 [details] [diff] [review]
patch - fix uninitialized variable regression

r=dougt

Comment 22

[Alec Flett]

Comment on attachment 94632 [details] [diff] [review]
patch - fix uninitialized variable regression

sr=alecf

Comment 23

[Darin Fisher]

looks like the v1 patch caused a mac regression: bug 161921 :(

patch has been backed out... investigating the problem...

Comment 24

[Darin Fisher]

re-landed patch after fixing mac smoketest blocker.

Comment 25

[Darin Fisher]

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]

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)]

or as we discussed before, we could remove these methods from this interface.

Comment 28

[Bradley Baetz (:bbaetz)]

Comment on attachment 94632 [details] [diff] [review]
patch - fix uninitialized variable regression

r=bbaetz, too

Comment 29

[Bradley Baetz (:bbaetz)]

errr, wrong bug; ignore that

Comment 30

[Darin Fisher]

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]

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)]

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)]

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]

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]

-> 1.2final... most likely going to be marked frozen as is.

Comment 36

[Darin Fisher]

ok, last chance to make changes to this interface.  will mark this interface
frozen tomorrow.

Comment 37

[Darin Fisher]

nsIIOService.idl has been marked frozen on the trunk.  marking this bug FIXED.

Comment 38

[David Epstein]

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).