Add a ReadLine() function to nsIFileInputStream

VERIFIED FIXED in mozilla0.9.3

Status

()

Core
Networking: File
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla0.9.3
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: need to review. r=?, sr=?)

Attachments

(6 attachments)

Mail from dougt@netscape.com:

> Would it be worthwhile adding a readline() function to
> nsIFileInputStream?  There are a few places in the tree that use
> readline() on nsFileSpec-based streams...

Not a bad idea.  Keep in on the file stream interface.  If there is demand, we
could promote it to the nsIInputStream.
This implementation differs from the nsFileSpecImpl::ReadLine implementation in
two ways (both changes for the better, in my opinion):

1)  It does platform-specific line endings correctly (or at least tries to -- it
may not work completely right on some wacky platforms).

2)  It writes the data to an nsAWritableString instead of to a char*.  This
means that we don't have to guess at an upper bound for line-length when calling it.

This blocks bug 52441, since parsing of mailcap/mime.types files is very
line-oriented.  Implementing this would allow the use of nsIFile instead of
nsIFileSpec in the fix to 52441.

Reviews?  Doug, what do you think?
Blocks: 52441
Keywords: mozilla0.9.2, patch, review
I just looked at my patch after the requisite 1-day break and it's terrible.

1)  It breaks when we have a \r not followed by \n on windows -- we lose a
    character. Similarly on Mac.
2)  It's hard to read
3)  The return value is useless -- it says 0 for both end of file and for an
    empty line

Working on a better patch.
Keywords: patch, review
OK. This one's cleaner and more correct and usefully reports end-of-file.

It does _not_ report the amount of data read.  That can be gotten directly from
the string it's writing to, so it seemed superfluous.

Reviews?  For real this time.  :)
Keywords: patch, review
In that second page, kBufferSize should be something like 1024 -- "1" is left
from my last test with it...

Comment 7

17 years ago
darin, jud,

do you think that nsIInputStream should have a ReadLine() or just the file input
stream?  I know that in a couple of places we do similar read line logic.
Whiteboard: need to review.
Target Milestone: --- → mozilla0.9.2

Comment 8

17 years ago
IMO line hunting belongs in the consumer of the raw stream, rather than in the 
stream itself.

Comment 9

17 years ago
i don't think we want to add this to nsIInputStream, and not just because it
would touch too many consumers.  IMO this should be provided by some other
interface.  though, i'm not sure what this interface would be called.

Jud: it sounds like you are suggesting a utility function (since we'd like not
to duplicate code)?  unfortunately, that would add a link dependency.  why not
provide an interface with a ReadLine function... i'm not sure this interface
should be nsIFileInputStream, since other streams can obviously provide such
functionality.  the underlying implementation of ReadLine could be shared by
stream implementations that live in the same component.

Comment 10

17 years ago
yea, a util function (we could avoid link dep by stuffing it in a .h file as
netutil.h does). my 2nd choice would be stuffing it in another iface.

Comment 11

17 years ago
netutil.h type inlines just add bloat to mozilla.  i think we should really
discourage this sort of thing when the functions themselves are large/complex.

Comment 12

17 years ago
The methods in nsNetUtil.h should really be implemented in a separate library.

Comment 13

17 years ago
well, netutil is just inlining code that would be there anyway. it's really just 
XPCOM wrapping.

Comment 14

17 years ago
we used to impl them in a seperate library, but everyone hated the link time 
dep.

Comment 15

17 years ago
really?  why was the link time dep bad?

Comment 16

17 years ago
So, it is sounding like that instead of implementing this on the stream itself,
it would be better to have a helper function that does this.  A benefit is that
it could work on any nsIInputStream.

Boris, are you up for something like this?
Assignee: dougt → bzbarsky

Comment 17

17 years ago
darin wrote: "unfortunately, that would add a link dependency." :-) 

people don't like link deps.
I could probably do a helper function, though I'd need to figure out what file 
to put it in...  I would be more in favor of having _some_ interface provide 
this functionality, though, if only because there's no reason not to have the 
ReadLine() scriptable.
ccing Doug...

Comment 20

17 years ago
scriptablity is a good reason for making this exist on some interface. 
nsINetUtils.idl anyone?

Comment 21

17 years ago
No!  nsNetUtils.h is just for wrappers around COM instantiations and the like.
It is not for functions like this.  This helper function should exist as part
of xpcom/io.  If it must be scriptable, then I think QI'ing to 
nsILineInputStream (or something like that) would be best.  The implementation's
of nsILineInputStream::ReadLine() could make use of a shared helper function 
internal to libxpcom.  Necko streams that want to provide this interface could
also share a helper function internal to libnecko.

The code for the helper function could be added to some .h file in xpcom/io and
thereby used to implement all the different nsILineInputStream::ReadLine's.

While this isn't exactly elegant, I think it does solve the problem.  It makes
ReadLine scriptable, and it doesn't introduce any more link dependencies.

Comment 22

17 years ago
I should have used a :-)

Comment 23

17 years ago
ah.. right :-)
OK.  So do we want to go ahead and add an nsILineInputStream interface together
with this helper function?  Should I try to do this?  I'd really like to get
this working asap.  We would have two identical helper functions, but that can't
be helped I suppose...

Comment 25

17 years ago
why would there be two identical helper funtions?
One for libxpcom, one for libnecko.  Though at the moment we may only need this
for necko streams...

Comment 27

17 years ago
we only need one implementation of this function if it is defined in a special
header file.

Comment 28

17 years ago
of course, in reality there would be a copy of it in each component that needs it.
OK.  Let me see if I understand what needs to be done correctly:

1) create nsILineInputStream.idl which has a readLine method

2a) create a new header file in netwerk/public that implements a ReadLine helper
function  (since nsIFileInputStream is a necko stream, this is in netwerk)
or 
2b) add the ReadLine helper to an existing header in netwerk/public (I don't see
an appropriate header for this there, so 2a is probably better)

3) make nsIFileInputStream implement the nsILineInputStream interface using the
helper function

4) add all the various files to the build system appropriately

Does this sound right?  If so, I'll take a shot at doing it.

Comment 30

17 years ago
almost... i think nsILineInputStream should belong in xpcom/io along with
nsIInputStream.  it is perhaps more generic than necko.
OK.  Will do.  A helper function is still needed in necko if it's to be usable
by nsIFileInputStream, right?

What's a decent name for the new header with the helper function?

Comment 32

17 years ago
nsReadLine.h (perhaps?)
Created attachment 38371 [details] [diff] [review]
Patch using nsReadLine.h and nsILineInputStream.idl
Attachment 38371 [details] [diff] implements what we've been discussing.
pushing out to 0.9.3 since the tree has closed for 0.9.2....
Target Milestone: mozilla0.9.2 → mozilla0.9.3

Comment 36

17 years ago
some comments about the patch:

1. readLine returns a boolean... why "true" for EOF?

2. why is NS_ReadLine declared inline?  i think it should just be declared    
   static.

3. PRLineBuffer is wrong, nsLineBuffer would be better, and it should not be
   a typedef to a pointer.

4. calling NS_InitLineBuffer inside nsFileInputStream::Create is wrong.  it 
   should only be done when necessary, such as the first time ReadLine is      
   called.  that way your not making all clients of nsFileInputStream pay the 
   cost of allocating mLineBuffer.
5. why make NS_ReadLine OS specific?  it is possible for any OS to have a file
   created by an OS.  NS_ReadLine should dynamically handle any newline   
   sequence.  this is particularly important, since the data stream could be
   from any source and not just from a local file.
> 1. readLine returns a boolean... why "true" for EOF?

At the time I was thinking in terms of what I actually want to know (which is
whether I have reached the end of the file).  I guess in a scripting context it
would make more sense to return "false" for EOF.  Which should we do?

> 2. why is NS_ReadLine declared inline?

Because I'm silly.  It's just static now.

> 3. PRLineBuffer is wrong, nsLineBuffer would be better, and it should not be
>    a typedef to a pointer.

Ok.  Changed to nsLineBuffer which is the struct itself and changed the various
functions involved to take pointers to nsLineBuffer

> 4.  calling NS_InitLineBuffer inside nsFileInputStream::Create is wrong.

So it is.  Changed that to initialize the buffer in ReadLine if it's not
initialized already

> 5. why make NS_ReadLine OS specific?

As long as this only applied to local files making it OS specific made sense. 
Now that it doesn't I guess I'll just have to treat '\n' _or_ '\r' as ending
lines (and make sure to ignore the '\r' or '\n' following it).

Working on fixing #5 right now.  Now that I think about it, #1 should probably
be resolved in favor of returning true when the operation is successful.

New patch coming up.


Created attachment 40104 [details] [diff] [review]
Patch that's not OS-specific and returns "false" on end-of-file
Reviews for attachment 40104 [details] [diff] [review]?  I'd really like to get this fixed so I have a
chance of landing bug 52441 by 0.9.3
Status: NEW → ASSIGNED
Keywords: mozilla0.9.2 → mozilla0.9.3

Comment 40

17 years ago
+NS_IMETHODIMP
+nsFileInputStream::ReadLine(nsAWritableString & aLine, PRBool *_retval) {
+  nsresult rv;
+  if (!mLineBuffer) {
+    rv = NS_InitLineBuffer(&mLineBuffer);
+  }
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+  return NS_ReadLine(this, mLineBuffer, aLine, _retval);


this is wrong!  what if mLineBuffer is not null?  then rv will have a junk
value.  consider this instead:

NS_IMETHODIMP
nsFileInputStream::ReadLine(nsAWritableString & aLine, PRBool *_retval) {
  if (!mLineBuffer) {
    nsresult rv = NS_InitLineBuffer(&mLineBuffer);
    if (NS_FAILED(rv)) return rv;
  }
  return NS_ReadLine(this, mLineBuffer, aLine, _retval);
}
eek.  good catch, darin.  Fixed.
Created attachment 40164 [details] [diff] [review]
patch with a fix for that rv thing Darin caught
Created attachment 40175 [details] [diff] [review]
change nsFileInputStream::Close to avoid leaking an nsLineBuffer
Whiteboard: need to review. → need to review. r=?, sr=?

Comment 44

17 years ago
Looks good to me.

Comment 45

17 years ago
r/sr=darin

Comment 46

17 years ago
looks good to me == r=
Checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Updated

17 years ago
QA Contact: tever → bbaetz
the code was checked in -> VERIFIED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.