The default bug view has changed. See this FAQ.

Efficient JS File API - Windows back-end

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

unspecified
mozilla16
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 12 obsolete attachments)

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.
Blocks: 707676
Created attachment 581987 [details] [diff] [review]
Prev 1. Windows back-end.

Attaching an early preview of the Windows back-end.
Created attachment 617441 [details] [diff] [review]
Windows synchronous back-end

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)
Depends on: 747872
Blocks: 747876

Updated

5 years ago
Attachment #617441 - Flags: review?(doug.turner) → review+

Comment 3

5 years ago
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
Created attachment 619280 [details] [diff] [review]
Windows synchronous back-end

Applied feedback.
Attachment #617441 - Attachment is obsolete: true
Attachment #619280 - Flags: review?(taras.mozilla)

Comment 5

5 years ago
Comment on attachment 619280 [details] [diff] [review]
Windows synchronous back-end

wrong patch
Attachment #619280 - Flags: review?(taras.mozilla)
Created attachment 621523 [details] [diff] [review]
Windows synchronous back-end

Oops, here it is.
Attachment #619280 - Attachment is obsolete: true
Attachment #621523 - Flags: review?(taras.mozilla)

Updated

5 years ago
Attachment #621523 - Flags: review?(taras.mozilla) → review+
Created attachment 623148 [details] [diff] [review]
Windows synchronous back-end

Fixed a few issues, bound |MoveFileEx| (which I will be using) instead of |MoveFile| (which I won't).
Attachment #621523 - Attachment is obsolete: true
Created attachment 623150 [details] [diff] [review]
Windows synchronous back-end
Attachment #623148 - Attachment is obsolete: true
Attachment #623150 - Flags: review?(doug.turner)

Comment 9

5 years ago
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+
Created attachment 625596 [details] [diff] [review]
Windows synchronous back-end

Fixed, thanks.
Attachment #623150 - Attachment is obsolete: true
Created attachment 625599 [details] [diff] [review]
Windows synchronous back-end
Attachment #625596 - Attachment is obsolete: true
Created attachment 625600 [details] [diff] [review]
Companion testsuite
Created attachment 626444 [details] [diff] [review]
Windows synchronous back-end
Attachment #625599 - Attachment is obsolete: true
Created attachment 626445 [details] [diff] [review]
Companion testsuite
Attachment #625600 - Attachment is obsolete: true
Created attachment 627875 [details] [diff] [review]
Companion testsuite
Attachment #626445 - Attachment is obsolete: true
Created attachment 627876 [details] [diff] [review]
Windows synchronous back-end
Attachment #626444 - Attachment is obsolete: true
Depends on: 707679
Keywords: checkin-needed
Created attachment 630987 [details] [diff] [review]
Companion testsuite

Fixed typo.
Attachment #627875 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/f895c9b827b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a72bba62fea

Sorry for the delay...
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/f895c9b827b3
https://hg.mozilla.org/mozilla-central/rev/9a72bba62fea
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 790649
You need to log in before you can comment on or make changes to this bug.