Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the FileSystemObserver API #165

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

nathanmemmott
Copy link
Contributor

@nathanmemmott nathanmemmott commented Jul 18, 2024

Add the FileSystemObserver API

Defines the FileSystemObserver API, an API to observe file system
change events.

See #123 and WICG/file-system-access#72

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

Nathan Memmott and others added 4 commits July 3, 2024 11:48
Gives each concept in the concepts section its own section.
Pulls out some implementation-defined concepts into a file system
concept.

This is so that the file system concept can be extended further in the
future.
Pulls out some implementation-defined concepts into a file system
concept.

This is so that the file system concept can be extended further in the
future.
@nathanmemmott nathanmemmott changed the title File system observer Add the FileSystemObserver API Jul 19, 2024
@nathanmemmott nathanmemmott force-pushed the file_system_observer branch from 320d58a to 8047a41 Compare July 19, 2024 16:10
index.bs Outdated Show resolved Hide resolved
Copy link
Collaborator

@mkruisselbrink mkruisselbrink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if we could somehow specify more precisely what exact change events are triggered by changes to the bucket file system, since that should be entirely under control of user agents, so should be possible to be fully compatible between implementations. Not sure how to be go about doing that. Left a bunch of other comments as well...

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@nathanmemmott nathanmemmott force-pushed the file_system_observer branch 4 times, most recently from 86ed1e4 to 42a22b9 Compare October 31, 2024 23:54
@nathanmemmott
Copy link
Contributor Author

The errors causing the build to fail were not introduced by this PR.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated
@@ -1721,13 +2066,10 @@ The <dfn method for=StorageManager>getDirectory()</dfn> method steps are:
1. Set |dir|'s [=directory entry/children=] to an empty [=/set=].
1. Set |map|["root"] to |dir|.

1. Let |root| be an [=implementation-defined=] opaque [=string=].
1. Let |fileSystem| be [=/bucket file system=]'s [=file system/root=].
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhere in here fileSystem needs to be linked to a specific storage bottle for the correct storage bucket/origin. As written now it reads like there is one shared file system that all the storage buckets access. The old spec didn't really do this rigorously either, so it's probably okay to keep it a little bit hand-wavy (i.e. you might not need to go the whole way towards calling "obtain a local storage bottle map", and making it explicit that all the data for the file system is stored in the returned map), but we certainly don't want the spec to read like there is one "the bucket file system". Each storage bucket does need its own file system.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if the update makes sense.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated
Exposed=(DedicatedWorker,SharedWorker,Window),
SecureContext,
RuntimeEnabled=FileSystemObserver
] interface FileSystemChangeRecord {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason this is a class and not just a dictionary? I think in retrospect DOM's MutationRecord could have just been a dictionary as well. It's also not entirely clear to me why this has a constructor.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked the same question, and we had some offline discussion about this. The question of dictionary vs class seems to be the oldest open tag guidance request, but the most recent thinking in the issue thread there (w3ctag/design-principles#11 (comment)) seems to lean arguably in favor of using a class here, or at least not arguing against it (but that guidance certainly has changed a couple of times as the make-up of the tag has changed). I'm not sure what the latest thinking in this area is, and it would be nice if the TAG would actually write up something about this other than in that very long issue thread...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that accurately reflects the upthread discussion though with input from me and @domenic. In particular @domenic made the point there that for observer-related cases where it's just about data a dictionary would be more idiomatic. Having both a class and a constructor for it that takes a dictionary seems quite bloated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per https://groups.google.com/a/chromium.org/g/blink-dev/c/6oOaFmia2dc/m/gx0KgpQqBQAJ, the latest plan seems to be to use a dictionary, especially since that is easier to upgrade to an interface later. I think that's a good plan, and updating the spec PR to match that would be good.

Copy link
Contributor Author

@nathanmemmott nathanmemmott Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to dictionary.

);

[
Exposed=(DedicatedWorker,SharedWorker,Window),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not (Worker, Window), FileSystemHandle is probably exposed in service worker contexts too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A separate API would be needed for service workers. The service worker becomes inactive after 30 seconds of no events. The change events received in the FileSystemObserver callback would not count as a service worker event. There are also privacy considerations around allowing a site to observe file changes without the user being aware.

index.bs Show resolved Hide resolved
"errored",
"modified",
"moved",
"unknown",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure when errored and unknown would happen.
I guess errored could happen if unmounting a disk for instance.
I have no clue for unknown.
If we do not have a good usecase, should we remove from the initial PR?
Giving a description explaining what these values are might be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added definitions for each with examples.

"appeared",
"disappeared",
"errored",
"modified",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is modified meaning?
If only touching the file, will it trigger a FileSystemChange?
If so, I would guess that this could trigger events where no information for the web page has changed (IIRC, access/modification dates are not exposed).
Or maybe it is restricted to changes in the content itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally this would only be content changes but we have limited information from the OS APIs so we cannot always filter out metadata changes. There will be some noisiness that a user agent can attempt to filter.


<xmp class=idl>
dictionary FileSystemObserverObserveOptions {
boolean recursive = false;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a folder being observed is removed, which callback will be called?
One for the folder, or one per each file in the folder, or depending on the OS?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For each observer, there is only one callback. Each observer registration always has their root as the handle that was passed to observe().

A folder may be moved to a recycling bin or deleted permanently. When moved to the recycling bin, this is interpreted as a move out of scope, so will be reported as one "disappeared" on the folder. When permanently deleted, the OS must delete its descendants recursively before deleting the folder. So a "disappeared" event for each descendant will be reported.

"disappeared",
"errored",
"modified",
"moved",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is moved also covering rename change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

@nathanmemmott
Copy link
Contributor Author

The latest patch adds some logic that we decided on recently. We no longer send the changed handle for "disappeared", "errored", and "unknown".

@nathanmemmott
Copy link
Contributor Author

@annevk @mkruisselbrink @domenic @youennf PTAL again and let me know if this is ready to merge.

@asutherland I don't have permission to add you but please take a look. If someone does have permission, please add him.

Defines the FileSystemObserver API, an API to observe file system
change events.

See whatwg#123 and WICG/file-system-access#72
@mkruisselbrink
Copy link
Collaborator

This looks good to me, but it would be good to get at least one other person to sign off on this before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants