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


Attachments
v1 patch (114.21 KB, patch)
2002-09-15 13:39 PDT, Darin Fisher
no flags Details | Diff | Splinter Review
v2 patch (31.16 KB, patch)
2002-09-23 12:18 PDT, Darin Fisher
doug.turner: review+
bzbarsky: superreview+
Details | Diff | Splinter Review

Description User image Doug Turner (:dougt) 2002-07-12 08:39:12 PDT
Needed Functionality
    using nsIFileChannel to determine if channel is file.

maybe this can be on the nsIChannel?
Comment 1 User image Judson Valeski 2002-07-12 08:45:58 PDT
note: nsIChannel is frozen.
Comment 2 User image Bradley Baetz (:bbaetz) 2002-07-12 20:17:21 PDT
-> schemeis("file", &ret) will allow you "to determine if channel is file",
won't it?
Comment 3 User image Doug Turner (:dougt) 2002-07-12 20:35:42 PDT
if the answer to that is a definitive yes, then we can close this bug.  
Comment 4 User image Bradley Baetz (:bbaetz) 2002-07-12 21:15:59 PDT
IF you can give an example of why/where this is needed, then I can tell you if
it will.

nsIFileChannel gives you more functionality, though, which you may or may not need.
Comment 5 User image Doug Turner (:dougt) 2002-07-13 10:21:16 PDT
> IF you can give an example of why/where this is needed, then I can tell you if
it will

If i had a concrete example, I could *also* tell if this is a problem.  This bug
is mainly for me to figure out if it is needed.   :-)

Comment 6 User image Darin Fisher 2002-07-25 09:53:45 PDT
SchemeIs("file", &ret) tells you the protocol type of a nsIURI, which also tells
you that the URL is handled by the file protocol handler.  so, the SchemeIs test
is sufficient.

however, doug, i thought you mentioned that there was also need for nsIFileURL
to be frozen?  what i really wanted to do was add a new parameter to
nsIStandardURL::Init that would determine if the nsStandardURL object could QI
to nsIFileURL.  then the HTTP protocol handler for example could pass FALSE, and
the file protocol handler could pass TRUE.  this would allow people to QI a
nsIURI to nsIFileURL to determine the nsIFile without having to first call
SchemeIs("file").  this is important since the resource URLs also map to local
files exposed via nsIFileURL.
Comment 7 User image Doug Turner (:dougt) 2002-07-25 10:03:23 PDT
good plan.  I never like QI to nsIFileURL, have it succeed, then detecting the
failure when you call getFile().
Comment 8 User image Doug Turner (:dougt) 2002-07-25 10:54:25 PDT
darin
Comment 9 User image Darin Fisher 2002-09-11 13:38:03 PDT
ok, to summarize what needs to be done here: **freeze nsIFileURL only**

i'll work on the necessary changes to nsStandardURL and nsIStandardURL to
implement comment #6 paragraph 2.
Comment 10 User image Darin Fisher 2002-09-15 13:39:53 PDT
Created attachment 99291 [details] [diff] [review]
v1 patch

this patch breaks nsIFileURL out of nsStandardURL, and introduces a subclass of
nsStandardURL specific to the file protocol handler.  nsResURL, which has
always been a subclass of nsStandardURL, now explicitly implements nsIFileURL
(instead of just overriding the impl from nsStandardURL).

this cleans up the "QI to nsIFileURL" issue... now only file: and resource:
nsIURI's can QI to nsIFileURL.	i think this is way more inline with what folks
expect.

i've also gotten rid of a couple of create instance calls, and added in a
cleanup/optimization in the HTTP NewURI code (now it just calls new on a
nsStandardURL instead of talking to the component manager to get a
nsStandardURL object).
Comment 11 User image Darin Fisher 2002-09-15 13:49:10 PDT
oh, one more important thing... it seemed cleaner to make nsIFileURL inherit
from nsISupports instead of nsIURL.  this simplifies the inheritance, and i
found that it doesn't really hurt the usability of the interface since most
folks just QI to nsIFileURL to get/set the nsIFile... few actually keep a
nsIFileURL around in order to access the nsIURI/nsIURL methods.  FWIW,
nsIStandardURL also inherits from nsISupports.
Comment 12 User image Doug Turner (:dougt) 2002-09-17 11:31:21 PDT
Comment on attachment 99291 [details] [diff] [review]
v1 patch

fix the spacing in nsExternalHelperAppService.cpp
+      nsCOMPtr <nsIFile> file;

please replace the comments that you removed from nsIStandardURL.idl with
something that reflects reality.


net_GetFileFromURLSpec doesn't null the in result.  In nsFileURLImpl::GetFile,
we should null *result if there is a failur.  You can fix this either in
nsFileURLImpl::GetFile or in the net_ calls.

Do we really have to clone the file in SetFile()?  Shouldn't the caller just
not mess with the object it passes?

If nsIFileURL doesn't derive from nsIURL, why is it named nsXXXURL?
Comment 13 User image Darin Fisher 2002-09-23 12:18:39 PDT
Created attachment 100265 [details] [diff] [review]
v2 patch

dougt and i talked about this bug, and we came up with a much better solution. 
now, instead of subclassing nsStandardURL to add support for nsIFileURL,
nsStandardURL now provides a constructor, which allows support for nsIFileURL
to be conditionally allowed.  this simplifies the inheritance structure as well
as the patch :)

a lot of the changes that i made throughout the tree in the v1 patch are still
needed since the assumption that nsStandardURL always implements nsIFileURL is
no longer valid.
Comment 14 User image Darin Fisher 2002-09-23 12:19:46 PDT
darn, looks like i didn't address all of dougt's comments... new patch coming up.
Comment 15 User image Darin Fisher 2002-09-24 21:26:37 PDT
> net_GetFileFromURLSpec doesn't null the in result.  In nsFileURLImpl::GetFile,
> we should null *result if there is a failur.  You can fix this either in
> nsFileURLImpl::GetFile or in the net_ calls.

nsStandardURL::GetFile() never NULL'd out the return param on failure.  that's
not required by XPCOM.  consumers are supposed to check the nsresult returned
for failure.

otherwise, i just tweaked the patch to fix the "nsCOMPtr <nsIFile>" issue.
Comment 16 User image Doug Turner (:dougt) 2002-09-25 06:15:01 PDT
I could have swore that xpconnect had problems with not nulling the out var.
Comment 17 User image David Bradley 2002-09-25 07:00:51 PDT
If the function returns an error code XPConnect doesn't touch the out parameters.
Comment 18 User image Doug Turner (:dougt) 2002-09-25 07:17:54 PDT
Comment on attachment 100265 [details] [diff] [review]
v2 patch

r=dougt

Before landing this, please see:
http://www.mozilla.org/projects/embedding/HowToFreeze.html

You should send a mail announcing that this interface is frozen.
Comment 19 User image Boris Zbarsky [:bz] (still a bit busy) 2002-10-05 18:09:12 PDT
Comment on attachment 100265 [details] [diff] [review]
v2 patch

sr=bzbarsky if you file another bug on an NS_INTERFACE_MAP_ENTRY_CONDITIONAL
macro.
Comment 20 User image Darin Fisher 2002-10-05 19:29:55 PDT
fixed-on-trunk :)
Comment 21 User image David Epstein 2002-10-31 19:14:47 PST
Verified patch checkin against Mozilla 1.2b Mozilla/5.0 (Windows; U; WinNT4.0;
en-US; rv:1.2b) Gecko/20021026 build.

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