Closed
Bug 510991
Opened 15 years ago
Closed 15 years ago
NS_GetURLSpecFromFile does a stat()
Categories
(Core :: Networking, defect, P3)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a1
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta2-fixed |
People
(Reporter: taras.mozilla, Assigned: benedict)
References
Details
(Whiteboard: [ts])
Attachments
(1 file, 4 obsolete files)
17.75 KB,
patch
|
jst
:
approval1.9.2+
|
Details | Diff | Splinter Review |
This one is ridiculous in that it does a stat() to determine if the url should have a '/' on the end. I'm pretty sure all of the callers already know if the url should point at a file or a directory.
Reporter | ||
Updated•15 years ago
|
Whiteboard: [ts]
Reporter | ||
Comment 1•15 years ago
|
||
(In reply to comment #0) > This one is ridiculous in that it does a stat() to determine if the url should > have a '/' on the end. I'm pretty sure all of the callers already know if the > url should point at a file or a directory. it's a TODO, filed it in hopes of someone fixing it before I get to.
Comment 2•15 years ago
|
||
per startup triage discussion, this might need an additive apis for GetURLSpecFromDir/GetURLSpecFromCallerIsSureItsAFile.
OS: Linux → All
Priority: -- → P3
Hardware: x86 → All
Assignee | ||
Updated•15 years ago
|
Assignee: nobody → bhsieh
Assignee | ||
Comment 3•15 years ago
|
||
I only modified two callers, but this patch plus bug 511761 removes all stats of components during startup.
Reporter | ||
Comment 4•15 years ago
|
||
Comment on attachment 402227 [details] [diff] [review] creates getURLSpecFromDir and getURLSpecFromNonDirFile >diff --git a/js/src/xpconnect/loader/mozJSComponentLoader.cpp b/js/src/xpconnect/loader/mozJSComponentLoader.cpp >--- a/js/src/xpconnect/loader/mozJSComponentLoader.cpp >+++ b/js/src/xpconnect/loader/mozJSComponentLoader.cpp >@@ -1181,17 +1181,17 @@ mozJSComponentLoader::GlobalForLocation( > nsCAutoString nativePath; > // Quick hack to unbust XPCONNECT_STANDALONE. > // This leaves the jsdebugger with a non-URL pathname in the > // XPCONNECT_STANDALONE case - but at least it builds and runs otherwise. > // See: http://bugzilla.mozilla.org/show_bug.cgi?id=121438 > #ifdef XPCONNECT_STANDALONE > localFile->GetNativePath(nativePath); > #else >- NS_GetURLSpecFromFile(aComponent, nativePath); >+ NS_GetURLSpecFromNonDirFile(aComponent, nativePath); > #endif > > // Before compiling the script, first check to see if we have it in > // the fastload file. Note: as a rule, fastload errors are not fatal > // to loading the script, since we can always slow-load. > nsCOMPtr<nsIFastLoadService> flSvc = do_GetFastLoadService(&rv); > > // Save the old state and restore it upon return >diff --git a/netwerk/base/public/nsNetUtil.h b/netwerk/base/public/nsNetUtil.h >--- a/netwerk/base/public/nsNetUtil.h >+++ b/netwerk/base/public/nsNetUtil.h >@@ -742,16 +742,42 @@ NS_GetURLSpecFromFile(nsIFile *file > nsresult rv; > nsCOMPtr<nsIFileProtocolHandler> fileHandler; > rv = NS_GetFileProtocolHandler(getter_AddRefs(fileHandler), ioService); > if (NS_SUCCEEDED(rv)) > rv = fileHandler->GetURLSpecFromFile(file, url); > return rv; > } > >+inline nsresult >+NS_GetURLSpecFromNonDirFile(nsIFile *file, >+ nsACString &url, >+ nsIIOService *ioService = nsnull) >+{ >+ nsresult rv; >+ nsCOMPtr<nsIFileProtocolHandler> fileHandler; >+ rv = NS_GetFileProtocolHandler(getter_AddRefs(fileHandler), ioService); >+ if (NS_SUCCEEDED(rv)) >+ rv = fileHandler->GetURLSpecFromNonDirFile(file, url); >+ return rv; >+} >+ >+inline nsresult >+NS_GetURLSpecFromDir(nsIFile *file, >+ nsACString &url, >+ nsIIOService *ioService = nsnull) >+{ >+ nsresult rv; >+ nsCOMPtr<nsIFileProtocolHandler> fileHandler; >+ rv = NS_GetFileProtocolHandler(getter_AddRefs(fileHandler), ioService); >+ if (NS_SUCCEEDED(rv)) >+ rv = fileHandler->GetURLSpecFromDir(file, url); >+ return rv; >+} >+ > /** > * Obtains the referrer for a given channel. This first tries to obtain the > * referrer from the property docshell.internalReferrer, and if that doesn't > * work and the channel is an nsIHTTPChannel, we check it's referrer property. > * > * @returns NS_ERROR_NOT_AVAILABLE if no referrer is available. > */ > inline nsresult >diff --git a/netwerk/base/src/nsURLHelper.cpp b/netwerk/base/src/nsURLHelper.cpp >--- a/netwerk/base/src/nsURLHelper.cpp >+++ b/netwerk/base/src/nsURLHelper.cpp >@@ -36,16 +36,17 @@ > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > > #include "nsURLHelper.h" > #include "nsReadableUtils.h" > #include "nsIServiceManager.h" > #include "nsIIOService.h" >+#include "nsILocalFile.h" > #include "nsIURLParser.h" > #include "nsIURI.h" > #include "nsMemory.h" > #include "nsEscape.h" > #include "nsCOMPtr.h" > #include "nsCRT.h" > #include "nsNetCID.h" > #include "netCore.h" >@@ -124,16 +125,70 @@ net_GetNoAuthURLParser() > nsIURLParser * > net_GetStdURLParser() > { > if (!gInitialized) > InitGlobals(); > return gStdURLParser; > } > >+//--------------------------------------------------------------------------- >+// GetFileFromURLSpec implementations >+//--------------------------------------------------------------------------- >+nsresult >+net_GetURLSpecFromDir(nsIFile *aFile, nsACString &result) >+{ >+ nsCAutoString escPath; >+ nsresult rv = net_GetURLSpecFromFileHelper(aFile, escPath); >+ >+ if (NS_FAILED(rv)) >+ return rv; >+ >+ if (escPath.Last() != '/') { >+ escPath += '/'; >+ } >+ >+ result = escPath; >+ return NS_OK; >+} >+ >+nsresult >+net_GetURLSpecFromNonDirFile(nsIFile *aFile, nsACString &result) >+{ >+ return net_GetURLSpecFromFileHelper(aFile, result); >+} Naming could use some work in this patch. I'm not great at that either. How about s/NonDirFile/ActualFile/? Looks to me like GetURLSpecFromFileHelper should be called net_GetURLSpecFromNonDirFile, then we can skip having this extra forwarding function. Also, Benjamin/Brendan/others, could we maybe modify existing net_GetURLSpecFromFile() by adding a default parameter "bool aIpromiseThisIsAFile=false" so we could skip the stat() without adding too many alternative functions? >+ >+nsresult >+net_GetURLSpecFromFile(nsIFile *aFile, nsACString &result) >+{ >+ // This method does an extra stat(). >+ NS_WARNING("Callers of GetURLSpecFromFile should use \ >+ GetURLSpecFromDir or GetURLSpecFromNonDirFile instead."); that's not real c++. do NS_WARNING("blah " <newline> "blah2") Rest of the patch looks good to me. This is a nice API fix, we should clean this up and land it asap.
Assignee | ||
Comment 5•15 years ago
|
||
A few concerns about this patch: - I'm not sure the NS_WARNING should remain: there are legitimate callers of the undifferentiated function. However, it'd be nice to ask people to move away from that function when possible. - It would be nice not to have three versions of the same function. Taras suggests a bool, but we really need a flag for: ActualFile / Dir / Either. An enum kind of works, but there is code that calls NS_GetURLSpec, net_GetURLSpec, and fileHandler->GetURLSpec, and callers of all of these functions would need to know about the enum, which is also painful.
Attachment #402227 -
Attachment is obsolete: true
Assignee | ||
Comment 6•15 years ago
|
||
see above comments.
Attachment #402251 -
Attachment is obsolete: true
Reporter | ||
Updated•15 years ago
|
Attachment #402253 -
Flags: review?(benjamin)
Comment 7•15 years ago
|
||
Comment on attachment 402253 [details] [diff] [review] ack hg only got half of my last patch I am not a peer of any of this code except perhaps accidentally the xpconnect stuff.
Attachment #402253 -
Flags: review?(benjamin) → review?(cbiesinger)
Assignee | ||
Comment 8•15 years ago
|
||
More numbers. Small sample size, caveat emptor, ymmv, etc etc. "Patched" numbers are with both this patch AND patch in bug 511761 applied. cold, no patch: 10829 10929 10395 13340 9573 10253 average: 10886 -------- cold, patch: 9707 11376 10563 11323 9590 10834 average: 10565
Comment 9•15 years ago
|
||
Comment on attachment 402253 [details] [diff] [review] ack hg only got half of my last patch +++ b/netwerk/base/public/nsNetUtil.h +NS_GetURLSpecFromActualFile(nsIFile *file, + nsACString &url, + nsIIOService *ioService = nsnull) this method needs a comment describing it. also, the indentation for the parameter lines is wrong. the same goes for NS_GetURLSpecFromDir +++ b/netwerk/base/src/nsURLHelper.cpp + nsresult rv = net_GetURLSpecFromActualFile(aFile, escPath); + + if (NS_FAILED(rv)) please delete that newline (similar in the other function) +++ b/netwerk/protocol/file/public/nsIFileProtocolHandler.idl the IDL needs a new IID +++ b/netwerk/protocol/file/src/nsFileProtocolHandler.cpp +nsFileProtocolHandler::GetURLSpecFromActualFile(nsIFile *file, nsACString &result) line too long, please limit lines to 80 characters
Attachment #402253 -
Flags: review?(cbiesinger) → review-
Assignee | ||
Comment 10•15 years ago
|
||
Attachment #402253 -
Attachment is obsolete: true
Attachment #404326 -
Flags: review?
Updated•15 years ago
|
Attachment #404326 -
Flags: review? → review+
Comment 11•15 years ago
|
||
Comment on attachment 404326 [details] [diff] [review] addresses above comments sorry, missed this one before, but otherwise looks good: +++ b/netwerk/base/src/nsURLHelperOS2.cpp +net_GetURLSpecFromActualFile(nsIFile *aFile, nsACstring &result) nsACstring -> nsACString
Assignee | ||
Comment 12•15 years ago
|
||
Attachment #404326 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Comment 13•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/0b299d45734b
Status: NEW → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Comment 14•15 years ago
|
||
Comment on attachment 404336 [details] [diff] [review] fixes typo This could improve startup time for Fennec
Attachment #404336 -
Flags: approval1.9.2?
Comment 15•15 years ago
|
||
So... After this fix, my console is full of warning spew: WARNING: If possible, callers of GetURLSpecFromFile should use GetURLSpecFromDir or GetURLSpecFromActualFile instead.: file /Users/bzbarsky/mozilla/css-frameconst/mozilla/netwerk/base/src/nsURLHelper.cpp, line 157 Hundreds of those. In particular, every single nsIOService::NewFileURI call leads to such a warning, via nsFileProtocolHandler::NewFileURI and nsStandardURL::SetFile. This happens all over the place in the chrome registry, etc. Can we either remove the warning or change the caller or something? This makes actually using a debug build painful.
Comment 16•15 years ago
|
||
I filed bug 522025 on that.
Comment 17•15 years ago
|
||
Comment on attachment 404336 [details] [diff] [review] fixes typo a=jst, but we should take the followup debug spew fix bug 522025 as well at the same time.
Attachment #404336 -
Flags: approval1.9.2? → approval1.9.2+
Updated•15 years ago
|
Keywords: checkin-needed
Comment 18•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d2c5fc49dbb1
status1.9.2:
--- → beta2-fixed
Updated•15 years ago
|
Keywords: checkin-needed
Updated•15 years ago
|
Version: unspecified → Trunk
We actually hit this a lot as part of bug 890712, in particular every time we use Cu.import.
You need to log in
before you can comment on or make changes to this bug.
Description
•