Closed Bug 707681 Opened 8 years ago Closed 8 years ago

Efficient JS File API - Windows back-end

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(2 files, 12 obsolete files)

9.70 KB, patch
Details | Diff | Splinter Review
10.07 KB, patch
Details | Diff | Splinter Review
Implement the JS File API Windows backend.

A few considerations:
- by opposition to Unix, the notion of opening a directory is somewhat unclear;
- this back-end needs to handle Windows-style sharing attributes;
- for the moment, we do not handle Windows-style permissions;
- by opposition to Unix, information on a file is spread through a number of system calls;
- we need to determine which version of Windows we target – most recent changes to system APIs seems to date back to Windows XP.
Attached patch Prev 1. Windows back-end. (obsolete) — Splinter Review
Attaching an early preview of the Windows back-end.
Attached patch Windows synchronous back-end (obsolete) — Splinter Review
Here is a working version of the Windows back-end. Few features are supported at the moment (in particular not the security context when opening/creating a file), but it is a start.
Attachment #581987 - Attachment is obsolete: true
Attachment #617441 - Flags: review?(doug.turner)
Attachment #617441 - Flags: review?(doug.turner) → review+
Comment on attachment 617441 [details] [diff] [review]
Windows synchronous back-end

+     let libc_candidates = ["kernel32.dll"];
+     for (let i = 0; i < libc_candidates.length; ++i) {
+       try {
+         libc = ctypes.open(libc_candidates[i]);
+         break;
+       } catch (x) {
+         LOG("Could not open kernel "+libc_candidates[i]);
+       }
+     }

Do not include pointless code. Get rid of the try/catch, loop, etc.

Can post a second patch for me to review that applies on top of this one with these fixes.

+       WinFile.CloseHandle = function(fd) {
+         fd.dispose();
+         if (ctypes.winLastError) {
+           throw new OSWin.Error(ctypes.winLastError, "close");
+         }
+       };

Please don't change semantics of low level functions. This should do fd.forget() and then call close function directly + pass proper return code
Attached patch Windows synchronous back-end (obsolete) — Splinter Review
Applied feedback.
Attachment #617441 - Attachment is obsolete: true
Attachment #619280 - Flags: review?(taras.mozilla)
Comment on attachment 619280 [details] [diff] [review]
Windows synchronous back-end

wrong patch
Attachment #619280 - Flags: review?(taras.mozilla)
Attached patch Windows synchronous back-end (obsolete) — Splinter Review
Oops, here it is.
Attachment #619280 - Attachment is obsolete: true
Attachment #621523 - Flags: review?(taras.mozilla)
Attachment #621523 - Flags: review?(taras.mozilla) → review+
Attached patch Windows synchronous back-end (obsolete) — Splinter Review
Fixed a few issues, bound |MoveFileEx| (which I will be using) instead of |MoveFile| (which I won't).
Attachment #621523 - Attachment is obsolete: true
Attached patch Windows synchronous back-end (obsolete) — Splinter Review
Attachment #623148 - Attachment is obsolete: true
Attachment #623150 - Flags: review?(doug.turner)
Comment on attachment 623150 [details] [diff] [review]
Windows synchronous back-end

Review of attachment 623150 [details] [diff] [review]:
-----------------------------------------------------------------

fix or follow up please.

::: toolkit/components/osfile/osfile_win_back.jsm
@@ +44,5 @@
> +     exports.OS.Win.File = {};
> +
> +     let LOG = OS.Shared.LOG.bind(OS.Shared, "OS.Win.File");
> +
> +     let libc = ctypes.open("kernel32.dll");

we should throw if libc isn't assigned.
Attachment #623150 - Flags: review?(doug.turner) → review+
Attached patch Windows synchronous back-end (obsolete) — Splinter Review
Fixed, thanks.
Attachment #623150 - Attachment is obsolete: true
Attached patch Windows synchronous back-end (obsolete) — Splinter Review
Attachment #625596 - Attachment is obsolete: true
Attached patch Windows synchronous back-end (obsolete) — Splinter Review
Attachment #625599 - Attachment is obsolete: true
Attached patch Companion testsuite (obsolete) — Splinter Review
Attachment #625600 - Attachment is obsolete: true
Attached patch Companion testsuite (obsolete) — Splinter Review
Attachment #626445 - Attachment is obsolete: true
Fixed typo.
Attachment #627875 - Attachment is obsolete: true
Depends on: 790649
You need to log in before you can comment on or make changes to this bug.