All users were logged out of Bugzilla on October 13th, 2018

file module should allow instantiation

RESOLVED INCOMPLETE

Status

P3
normal
RESOLVED INCOMPLETE
8 years ago
7 years ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

unspecified
Future

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

8 years ago
Currently, file module operates as a class class methods.
It means that when you operate on one file, like:

if (file.exists(path)) {
  file.read(path)
  file.remove(path)
}

we execute createInstance(Ci.nsILocalFile) and initWithPath three times. One per command.

My suggestion is to keep that behavior, while adding a new one, where a developer creates an instance of a file and operate on that:

let fh = file(path)
if (fh.exists()) {
  fh.read()
  fh.remove()
}

Additional benefit here is that things like storage require a handler to the file, which currently is not available, but with such an instance could be a property:

let fh = file(path)
if (fh.exists()) {
  var mDBConn = storageService.openDatabase(fh);
  fh.remove();
}

Thoughts? Comments?
I haven't given thought to keeping the current behavior, but I certainly like the idea of having a way to construct a file object on which one can access properties and methods and which one can pass to APIs that expect to receive files.  I would call the API for doing this "File" and make it a standard constructor function.
(Assignee)

Comment 2

8 years ago
So, there are three approaches I can go:

1) I can create File class with methods that match what exports object has. con: code duplication

2) I can remove static functions and provide File class only. cons: breaking API and removing the ability to check for the file with a oneliner.

3) I can create internal functions that take file handler as a first argument and then write wrapper functions for exports and for File. In former case it'll call MozFile first, in latter it will pass local filehandler.
Status: NEW → ASSIGNED
We're still at a point along the development of the APIs where we can break functionality.  However, it's also OK to duplicate functionality when appropriate, or pending further review of an API being duplicated.

In other words, even if we do want to eventually remove the static functions (which seems reasonable at first glance), we don't necessarily have to do so at the same time we implement the File constructor.  We can implement File, then at some point (perhaps quite soon afterwards) deprecate and eventually remove the static functions.

I would also suggest that a File constructor is almost as good as the static functions wrt. code conciseness.  Under the current API, code for acting on the basis of a file's existing looks like:

  let file = require("file");
  if (file.exists(path)) { ... }

If you don't care about saving a reference to the file, a constructor-based mechanism would be similarly short:

  let File = require("file").File;
  if (File(path).exists()) { ... }

If you do care about saving a reference to the file, it's one line longer, but then you would have a reference you can reuse, and the initial cost in lines of code would be amortized over the code using the reference:

  let File = require("file").File;
  let file = File(path);
  if (file.exists()) {
    file.this();
    file.that();
    file.theOther();
  }

So my suggestion would be to implement the File constructor now but defer deprecating/removing the static functions pending further analysis regarding their utility.
(Assignee)

Comment 4

8 years ago
Created attachment 456553 [details] [diff] [review]
patch v1

patch v1
Assignee: nobody → gandalf
Attachment #456553 - Flags: feedback?
Attachment #456553 - Flags: feedback? → feedback?(myk)
(Assignee)

Comment 5

8 years ago
it may make sense to target it for 0.6 in order to start rewriting calls to the API in 0.7 cycle.
(Assignee)

Comment 6

8 years ago
Created attachment 457168 [details]
perf test

Hmm... I have a problem with proving the case here (except of this being cleaner).

For some reason File(aDir).exists() is ~4 times slower than file.exists(aDir) which is not good.

Only after 5 operations on instantiated object (test case attached) I get comparable performance and then each additional operation is a win for the new code.

It may be sandbox specific.
Comment on attachment 456553 [details] [diff] [review]
patch v1

Overall, this looks good.  The only issue I note is that the module exposes the "fh" MozFile filehandle to callers, which provides unintended functionality to callers, and it uses "this" in methods, which gives callers the ability to access the global scope of the module.  It would be better to hide "fh" via the closure technique used in other modules and avoid use of "this" in methods so callers can't access the module's global scope.
Attachment #456553 - Flags: feedback?(myk) → feedback+
The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
OS: Mac OS X → All
Priority: -- → P3
Hardware: x86 → All
Target Milestone: --- → Future
Closing this for now. Gandalf, if you can get this finished, we can reopen it. :)
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.