[meta] nsIIOService need to be frozen

VERIFIED FIXED in mozilla1.2final



Embedding: APIs
15 years ago
15 years ago


(Reporter: dougt, Assigned: Darin Fisher)


({arch, meta, topembed+})

arch, meta, topembed+
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)



(2 attachments)



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


15 years ago
Blocks: 70229


15 years ago
Blocks: 157137

Comment 1

15 years ago
Assignee: dougt → darin


15 years ago
Priority: -- → P3
Target Milestone: --- → mozilla1.2beta


15 years ago
Keywords: mozilla1.2, topembed
Target Milestone: mozilla1.2beta → mozilla1.2alpha

Comment 2

15 years ago
Created attachment 93765 [details] [diff] [review]
v1 patch

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


Comment 3

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


15 years ago
Priority: P3 → P2


15 years ago
Attachment #93765 - Flags: review+

Comment 4

15 years ago
Comment on attachment 93765 [details] [diff] [review]
v1 patch


lets define it upfront when this interface will be frozen.  What are we waiting

Comment 5

15 years ago
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

15 years ago
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.  


15 years ago
Depends on: 119051


15 years ago
OS: Windows 2000 → All
Hardware: PC → All

Comment 7

15 years ago
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

15 years ago
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

15 years ago
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

15 years ago
Comment on attachment 93765 [details] [diff] [review]
v1 patch

for changes in editorUtilities.js

Comment 11

15 years ago
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

15 years ago
+ * 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.


Comment 13

15 years ago
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.


15 years ago
QA Contact: mdunn → depstein

Comment 14

15 years ago
 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
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
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
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
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                        */
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("").
QA Contact: depstein → mdunn


15 years ago
QA Contact: mdunn → depstein

Comment 15

15 years ago
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

15 years ago
- 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

15 years ago
Comment on attachment 93765 [details] [diff] [review]
v1 patch

minor nit in nsCookies.cpp:

+      if (os)
+	 os->NotifyObservers(nsnull, "cookieIcon",

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

Attachment #93765 - Flags: superreview+

Comment 18

15 years ago
will do.. thx alec.

Comment 19

15 years ago
This patch have added a "might be used uninitialized" warning (see also bug
59652) to brad TBox:

+ `nsresult rv' might be used uninitialized in this function

Indeed, now nsMsgAttachment::DeleteAttachment will always check NS_SUCCEEDED on
an uninitialized nsresult!

Comment 20

15 years ago
Created attachment 94632 [details] [diff] [review]
patch - fix uninitialized variable regression

Aleksey: thanks for noticing the problem!!

Comment 21

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

Attachment #94632 - Flags: review+

Comment 22

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

Attachment #94632 - Flags: superreview+

Comment 23

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

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


15 years ago
Keywords: patch

Comment 24

15 years ago
re-landed patch after fixing mac smoketest blocker.

Comment 25

15 years ago
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

15 years ago
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

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


15 years ago
Target Milestone: mozilla1.2alpha → mozilla1.2beta
Comment on attachment 94632 [details] [diff] [review]
patch - fix uninitialized variable regression

r=bbaetz, too
errr, wrong bug; ignore that

Comment 30

15 years ago
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!


15 years ago
Depends on: 166792


15 years ago
Summary: nsIIOService need to be frozen → [meta] nsIIOService need to be frozen


15 years ago
Severity: normal → major
Keywords: arch

Comment 31

15 years ago
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.


Comment 32

15 years ago
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.

Comment 34

15 years ago
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?


15 years ago
Keywords: topembed → meta, topembed+

Comment 35

15 years ago
-> 1.2final... most likely going to be marked frozen as is.
Target Milestone: mozilla1.2beta → mozilla1.2final

Comment 36

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

Comment 37

15 years ago
nsIIOService.idl has been marked frozen on the trunk.  marking this bug FIXED.
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 38

15 years ago
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).
You need to log in before you can comment on or make changes to this bug.