Add File interface #181

andreiltd · 2024-12-04T14:35:09Z

https://w3c.github.io/FileAPI/#file-section

Closes: #155

tschneidereit

While I left a few inline comments on the code as written, after getting far enough in the review I realized that I find it very difficult to reason through how the code implements the specification.

For example, when trying to understand the File constructor, I'm not sure how it implements the log of (simplifying here!) iterating through the list of fileBits and appending them to the blob underlying the File. If I understand the code correctly, it has a special case for a sequence whose first entry is a File instance. If that case is hit, it copies that File's contents and ignores the rest of the sequence?

But I might misunderstand the code here—that's very plausible!

Which is why I'd like to ask for something that's unfortunately a bunch of additional work: could you go through the code and add comments for which parts of the spec are implemented where? You can see examples of that in the various other builtins, in particular the ones for the fetch API. One that I think is quite decent is Response::redirect.

I used to have a script to turn the spec's source into C++ comments, but unfortunately seem to have lost it :( In the meantime, perhaps the easiest way to quickly get decent comments to use is roughly this:

find the spec algorithm you want to implement/get comments for
select the algorithms text in your browser
select "view source" (this works well at least in Firefox, I haven't tried other browsers)
copy the source
paste it into an html-to-markdown converter, such as https://htmlmarkdown.com/
prefix all lines in the output with //

That should give you something that's at least not too far away from usable.

As I said, I know that I'm asking for a whole bunch of extra work here, and I'm sorry for that. I don't really know of a better way to ensure that we're actually implementing the spec as written: the WPT suite gives us decent assurances, but it's not complete and doesn't cover all corner cases.

builtins/web/file.cpp

builtins/web/blob.cpp

builtins/web/file.cpp

andreiltd · 2024-12-11T15:00:16Z

Hey @tschneidereit ! Thanks for the feedback! I can definitely add comments with the steps from the spec - that’s a good point.

I've been thinking about this implementation, and I feel like part of the issue here is that I don’t have a clear understanding of how to model inheritance using jsapi. According to the spec, the File API extends Blob. This patch models it using composition: embedding the Blob into a File slot and delegating methods and properties. Maybe that's a correct way, but...

I noticed that the JS_InitClass function allows you to pass a prototype of the parent class, which seems like something I should use here. But I couldn’t find any documentation on how to properly implement this. For example, how do I call the parent constructor from a subclass? How do I control which constructor parameters are passed to the parent?

We shouldn’t need to add a special case for something like File([new File(["TEST"]). This should already be covered by the Blob implementation since it’s equivalent to File([new Blob(["TEST"]). In other words, Blob::is_instance() should return true for both Blob and File objects.

BTW, I don't mind redoing this PR from scratch as long as we can come up with better and more idiomatic code :)

andreiltd · 2024-12-12T17:07:24Z

Hey @tschneidereit I refactored the code so that File is now a subclass of Blob. This simplifies the implementation significantly and, hopefully doesn’t rely on too much "black magic." I’ve also added the steps from the spec in the comments.

I will prepare a separate PR for Blob, adding steps from spec to the documentation.

guybedford · 2024-12-30T17:31:16Z

builtins/web/blob.h

@@ -51,13 +51,19 @@ class Blob : public TraceableBuiltinImpl<Blob> {
  static const JSPropertySpec properties[];

  static constexpr unsigned ctor_length = 0;
-  enum Slots { Data, Type, Endings, Readers, Count };
+  enum Slots { Data, Type, Endings, Readers, Reserved1, Reserved2, Count };


Since these are being reserved by the FIle API, it may just make sense to directly call these MaybeFileName and MaybeFileModified, since those are their only meanings. Alternatively explicitly note they are reserved by the File API - FileAPISlot1 and FileAPISlot2.

Also I'm curious why not just have a File slot and have it point to a file instance here, where the File builtin has the Name and Modified slots?

My original goal was to use construct

JS::Construct(cx, blob_ctor_val, file_ctor_val, blob_args, &self);

so that internally it behaved like:

Reflect.construct(Blob, blobArgs, File);

and automatically allocated an object with the File class (allowing to set up extra slots such as name and lastModified). But for some reason JS_NewObjectForConstructor in the Blob constructor always produced a Blob object regardless of the newTarget.

I now switched to a simpler manual approach:

Create a File-class object directly,

Call Blob’s initialization routine on it (for shared functionality),

Then add the File-specific slots in the File constructor.

This keeps the “subclass” logic clean, avoids forcing Blob to know about File, and ensures we get a proper File object with all its extra slots.

Interestingly it also looks like using subclassing is now discouraged:

Warning: The standard committee now holds the position that the built-in subclassing mechanism in previous spec versions is over-engineered and causes non-negligible performance and security impacts. New built-in methods consider less about subclasses, and engine implementers are investigating whether to remove certain subclassing mechanisms. Consider using composition instead of inheritance when enhancing built-ins.

Note that this recommendation is to spec authors, and doesn't apply to implementations of existing specs. Since File is specified as extending Blob, we unfortunately don't get to decide that it shouldn't do so.

builtins/web/file.cpp

guybedford · 2024-12-30T17:35:02Z

builtins/web/file.cpp

+
+bool File::name_get(JSContext *cx, unsigned argc, JS::Value *vp) {
+  METHOD_HEADER(0);
+  // TODO: Change this class so that its prototype isn't an instance of the class


Can you clarify what is meant by this? Is the instance prototype not supposed to be an instance of the class, or the File.prototype constructor "prototype" property?

Sorry, no idea. I've been carrying this comment from other classes without giving it much thought.

For reasons I forgot, our builtin initialization system can't easily make it so that the prototype of instances isn't also an instance of the builtin. Which for some builtins it needs to, while for others it's not supposed to be. Hence, this TODO is strewn all over the place :/

andreiltd · 2025-01-08T13:09:46Z

host-apis/wasi-0.2.0/host_api.cpp

+  wasi_clocks_wall_clock_datetime_t time_now;
+  wasi_clocks_wall_clock_now(&time_now);
+
+  uint64_t total_nanoseconds = time_now.seconds * 1000000000ULL + time_now.nanoseconds;


This will overflow around year 2554. ¯\_(ツ)_/¯

tschneidereit

Despite the whole host of comments and suggestions I left, I think this is very close, and will just need one more short round!

builtins/web/blob.h

tschneidereit · 2024-12-12T17:36:26Z

builtins/web/file.h

+  static bool lastModified_get(JSContext *cx, unsigned argc, JS::Value *vp);
+
+public:
+  enum Slots { Count };


If you ensure that the slots used exclusively by File start after the last one used by Blob, you can continue using the same setup for all slots:

Suggested change

enum Slots { Count };

enum Slots {

Name = Blob::Slots::Count,

LastModified,

Count

};

builtins/web/file.cpp

tschneidereit · 2024-12-12T17:41:33Z

builtins/web/file.cpp

+    return api::throw_error(cx, api::Errors::WrongReceiver, "name get", "File");
+  }
+
+  auto name = JS::GetReservedSlot(self, static_cast<size_t>(ParentSlots::Name)).toString();


Suggested change

auto name = JS::GetReservedSlot(self, static_cast<size_t>(ParentSlots::Name)).toString();

auto name = JS::GetReservedSlot(self, static_cast<size_t>(Slots::Name)).toString();

builtins/web/file.cpp

tschneidereit · 2025-01-10T13:26:47Z

builtins/web/file.cpp

+
+bool File::name_get(JSContext *cx, unsigned argc, JS::Value *vp) {
+  METHOD_HEADER(0);
+  // TODO: Change this class so that its prototype isn't an instance of the class


For reasons I forgot, our builtin initialization system can't easily make it so that the prototype of instances isn't also an instance of the builtin. Which for some builtins it needs to, while for others it's not supposed to be. Hence, this TODO is strewn all over the place :/

tschneidereit · 2025-01-10T13:27:40Z

builtins/web/file.cpp

-
-  if (!opts) {
-    return true;
+bool File::init(JSContext *cx, HandleObject self, HandleValue fileBits, HandleValue fileName, HandleValue opts) {


Can you add a doc comment with a link to the algorithm implemented here? See some of the other builtins for how I usually format those.

tschneidereit · 2025-01-10T13:29:22Z

builtins/web/file.cpp

+  RootedValue lastModified(cx);
+  if (!init_last_modified(cx, opts, &lastModified)) {
+    return false;
  }


To adjust to the changes suggested above, this should become

Suggested change

RootedValue lastModified(cx);

if (!init_last_modified(cx, opts, &lastModified)) {

return false;

}

int64_t last_modified;

if (!read_last_modified(cx, opts, &last_modified)) {

return false;

}

tschneidereit · 2025-01-10T13:31:40Z

builtins/web/file.cpp

+  //  Steps 2, 3 and 5 are handled by Blob. We extend the Blob by adding a `name`
+  //  and the `lastModified` properties.
+  SetReservedSlot(self, static_cast<uint32_t>(Slots::Name), JS::StringValue(name));
+  SetReservedSlot(self, static_cast<uint32_t>(Slots::LastModified), lastModified);


This part is a bit unfortunate: we can't store lastModified as a uint32_t, because that'd overflow very quickly. (Way way before 2038, because it includes milliseconds.)

I think the best thing to do so we don't have to step away from using a Value encoding is to use a Number: 53 bits of precision should still be plenty for our purposes here.

tschneidereit · 2025-01-10T13:33:15Z

host-apis/wasi-0.2.0/host_api.cpp

@@ -107,6 +107,22 @@ void MonotonicClock::unsubscribe(const int32_t handle_id) {
  wasi_io_poll_pollable_drop_own(own_pollable_t{handle_id});
 }

+uint64_t WallClock::now() {


I think we'll not need this addition after all, given that we can use JS_Now as described above.

andreiltd · 2025-01-10T14:13:09Z

Hey @tschneidereit thanks for the review! Are you sure looked at the most recent version of the code? It looks like there are a bunch of comments that refer to outdated code, like Slots handling - there are now defined on File class, or using Int32 for storing lastModified - which uses Number now.

andreiltd marked this pull request as draft December 4, 2024 14:35

andreiltd force-pushed the file-builtin branch 4 times, most recently from 94de20c to 72b5f33 Compare December 9, 2024 14:25

Implement File API

5cdc538

andreiltd force-pushed the file-builtin branch from 72b5f33 to 5cdc538 Compare December 9, 2024 14:30

andreiltd added 2 commits December 9, 2024 17:52

Add public blob API

e7be7c3

Add comment about copying blob content

58e2d05

andreiltd marked this pull request as ready for review December 9, 2024 17:02

Rename res to rval

923eb3d

tschneidereit requested changes Dec 11, 2024

View reviewed changes

builtins/web/file.cpp Outdated Show resolved Hide resolved

builtins/web/blob.cpp Show resolved Hide resolved

builtins/web/file.cpp Outdated Show resolved Hide resolved

andreiltd added 3 commits December 12, 2024 17:52

Refactor File builtin

537903d

Remove unused function

e1d7095

Remove unused Blob::create function

89f9350

Run clang-format on file.h and file.cpp

9b26b51

andreiltd mentioned this pull request Dec 19, 2024

Implement URL.createObjectURL and URL.revokeObjectURL methods #187

Draft

andreiltd and others added 2 commits December 23, 2024 21:12

Add File::create function

eeab147

Merge branch 'main' into file-builtin

59b2055

guybedford reviewed Dec 30, 2024

View reviewed changes

Move File API slots to File class

12f720c

andreiltd commented Jan 8, 2025

View reviewed changes

Parse Date object

15f709f

andreiltd mentioned this pull request Jan 9, 2025

Add implementation of FormData builtin #191

Draft

tschneidereit requested changes Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add File interface #181

Add File interface #181

andreiltd commented Dec 4, 2024 •

edited

Loading

tschneidereit left a comment

andreiltd commented Dec 11, 2024 •

edited

Loading

andreiltd commented Dec 12, 2024 •

edited

Loading

guybedford Dec 30, 2024

guybedford Dec 30, 2024

andreiltd Jan 8, 2025

andreiltd Jan 8, 2025

tschneidereit Jan 10, 2025

guybedford Dec 30, 2024

andreiltd Jan 8, 2025

tschneidereit Jan 10, 2025

andreiltd Jan 8, 2025

tschneidereit left a comment

tschneidereit Dec 12, 2024

tschneidereit Dec 12, 2024

tschneidereit Jan 10, 2025

tschneidereit Jan 10, 2025

tschneidereit Jan 10, 2025

tschneidereit Jan 10, 2025

tschneidereit Jan 10, 2025

andreiltd commented Jan 10, 2025

	auto name = JS::GetReservedSlot(self, static_cast<size_t>(ParentSlots::Name)).toString();
	auto name = JS::GetReservedSlot(self, static_cast<size_t>(Slots::Name)).toString();

Add File interface #181

Are you sure you want to change the base?

Add File interface #181

Conversation

andreiltd commented Dec 4, 2024 • edited Loading

tschneidereit left a comment

Choose a reason for hiding this comment

andreiltd commented Dec 11, 2024 • edited Loading

andreiltd commented Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tschneidereit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreiltd commented Jan 10, 2025

andreiltd commented Dec 4, 2024 •

edited

Loading

andreiltd commented Dec 11, 2024 •

edited

Loading

andreiltd commented Dec 12, 2024 •

edited

Loading