mozStorage should accept paths

RESOLVED DUPLICATE of bug 813833

Status

()

RESOLVED DUPLICATE of bug 813833
6 years ago
6 years ago

People

(Reporter: Yoric, Assigned: br.carrjr)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=Yoric][lang=c++])

At the moment, according to the documentation, mozIStorage::OpenDatabase requires an instance of nsIFile. It would be nicer for OS.File users to also work with a simple string path.
(Assignee)

Comment 1

6 years ago
Hi, I'd be happy to give this a shot if no one is working on it yet.
is this just as an utility? Cause likely we'll still want to make an nsIFile internally to properly convert for the OS and check validity... So there's no win. Plus it's really easy for anyone to make such an helper in js.
@Brian With pleasure, but let's not write any code until we have decided of what we want the API to look like.

@mak Currently, the usage scenario is the following:

«
Cu.import("resource://gre/modules/FileUtils.jsm");

let mozStorage = ...
let path = OS.Path.join(...)
let file = new FileUtils.File(path);
let connection = mozStorage.openDatabase(file);
...
»

This examples has users mixing both OS.File and nsIFile and has users importing FileUtils, which is quite awkward as OS.File is designed largely to replace nsIFile and FileUtils for JS users.

Normally, users should write something along the lines of:

«
let mozStorage = ...
let path = OS.Path.join(...)
let connection = mozStorage.openDatabase(path);
...
»

So, one way to do this is to change mozIStorage. Another way is to implement a StorageUtils.jsm with roughly the same API as mozIStorage but that also accepts strings. I would be fine with this second option. Would this be ok with you, mak?
Flags: needinfo?(mak77)
I feel like this is just hiding stuff, I personally don't see anything bad in the first example. Why not just adding a OS.Path.IFile (dumb and confusing naming, something better could be found) for all of those APIs requiring nsIFile? I'm not sure about the direction the OS module is taking, so this could not be the direction we want, but it looks a cleaner approach to me, than adding a further util, that dupes again functionality we have all around. I think we will live still a long time with nsIFile.
I know you would like a pure approach to OS.File, but I'm not sure that should be pursued adding "dirt" to other APIs or modules. This would just be the beginning, how many APIs are taking nsIFile and will have to change?
Flags: needinfo?(mak77)
(In reply to Marco Bonardo [:mak] from comment #4)
> I feel like this is just hiding stuff, I personally don't see anything bad
> in the first example. Why not just adding a OS.Path.IFile (dumb and
> confusing naming, something better could be found) for all of those APIs
> requiring nsIFile?

Well, because:
- this won't work off the main thread, which is what OS.File is all about;
- this will encourage race conditions between uses of nsIFile and uses of OS.File, which is scary;
- generally, this is a confusing message for users, because we want OS.File to replace all uses of nsIFile.

> I'm not sure about the direction the OS module is taking,
> so this could not be the direction we want, but it looks a cleaner approach
> to me, than adding a further util, that dupes again functionality we have
> all around. I think we will live still a long time with nsIFile.
> I know you would like a pure approach to OS.File, but I'm not sure that
> should be pursued adding "dirt" to other APIs or modules. This would just be
> the beginning, how many APIs are taking nsIFile and will have to change?

This can certainly be discussed, but the core idea is that, from JavaScript code, nsIFile is almost always the wrong API to use. So, any API that confronts developers to nsIFile sends the wrong message and encourages writing code with wrong behavior. Good API design therefore dictates that we should make sure that no new code should need going through nsIFile.

Furthermore, as you know (bug 804108), I believe that we will want some kind of StorageUtils.jsm for other reasons. So I believe that this is just one more reason to start working on one.
This might be duped by bug 813833.
@Brian, Ok, looks like bug 813833 is duping this. Sorry for the noise.
Assignee: nobody → br.carrjr
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 813833
(Assignee)

Comment 8

6 years ago
Ok no problem
You need to log in before you can comment on or make changes to this bug.