1265767 - Implement a subset of the Blink FileSystem API for compat reasons

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrea Marchesini [:baku]]

Edge is shipping a subset of the Blink FileSystem API. We should do the same.
I'm going to upload a patch to show what we want to implement.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - WebIDL

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Honestly, I think we should implement the full Chrome API and use it in both drag'n'drop and in <input>. No need to drag our feet in search for a better API any more.

I don't think we should implement the sandboxed filesystem API yet though. At least not in this bug. Mainly because it'll be a totally different backend and so a lot more work, which we should prioritize separately.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 2 - FileEntry and DirectoryEntry

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 2 - FileEntry and DirectoryEntry

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 3 - FileEntry methods

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 4 - DirectoryReader

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 5 - DOMFileSystem

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 6 - getFile and getDirectory

This is the last patch. I wrote all of these on top of the Directory Upload API but probably, with a bit of tuning, it can land on m-i directly.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 6 - getFile and getDirectory

Forgot to update the last patch.

Comment 10

[Jonathan Watt [:jwatt]]

I spoke to Jonas about this and, although he wasn't quite as confident as I am that we will succeed, he agreed that we can push the new API rather than this API.

Comment 11

[Andrea Marchesini [:baku]]

(In reply to Jonathan Watt [:jwatt] from comment #10)
> I spoke to Jonas about this and, although he wasn't quite as confident as I
> am that we will succeed, he agreed that we can push the new API rather than
> this API.

This is what I hope! Plus, this API is built on top of the new one.
To me, landing the new API is just matter of reviewing code. Would be nice if we can do it for this week.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That's not exactly what I said. What I said is that I'm happy to let others make the call here. But I still think we need to be realistic about that call.

There's a few things that I think we need to keep in mind.

1. The worst possible option here is that the web ends up with *two* filesystem APIs. I.e. if we end up with both a Directory-based API, and a FileEntry/DirectoryEntry API then that's the worst possible option.

This is true even if we use FileEntry/DirectoryEntry in the Drag'n'Drop API and Directory-based is used for <input type=file>

2. Web developers tend to care more about being able to support old browsers, than they care about what syntax an API uses.

So if old browsers support FileEntry/DirectoryEntry, and new browsers support both FileEntry/DirectoryEntry and Directory, then developers are likely to choose to use FileEntry/DirectoryEntry only.

3. While very few websites use these APIs today, we should expect the number to increase over time. And we should expect these websites to want to support as many users as they can.

While these websites are likely going to go through the trouble of supporting whatever API we implement, we should ask ourselves if the solution that we choose make their lives easier or harder.

Comment 13

[Johnny Stenback (:jst)]

I agree with Jonas here, since we're being forced to implement the Chrome API I don't think we're in a position to meaningfully influence the web platform away from that API if only we implement the better API in addition. I'd say let's ship the Chrome API only for now (as much as I don't like to), and if other major browsers push for deprecation of that API down the road, and ship the new API, then we can do the same at that time.

Comment 14

[Jonathan Watt [:jwatt]]

(In reply to Jonas Sicking (:sicking) from comment #12)
> That's not exactly what I said. What I said is that I'm happy to let others
> make the call here. But I still think we need to be realistic about that
> call.

Yeah, sorry, I had in mind your qualification would be discussed in the meeting that we should have soon. Good to have them here though.

> 1. The worst possible option here is that the web ends up with *two*
> filesystem APIs. I.e. if we end up with both a Directory-based API, and a
> FileEntry/DirectoryEntry API then that's the worst possible option.

I'd agree (the patches for this bug are well in excess of 100 KB, which seems like a non-trivial maintinance burden). If we decide not to ship both APIs then the approach here of building the FileEntry/DirectoryEntry stuff on top of the Directory-based API is unfortunately probably not what we want since that implies shipping them both.

Comment 15

[Jonathan Watt [:jwatt]]

(In reply to Johnny Stenback  (:jst, [e-mail]) from comment #13)
> since we're being forced to implement the Chrome API

In this particular case I'd question that, despite MS's actions. For any API seeing non-trivial usage I'd agree that our hand is forced. However, there seem to be virtually no users other than Google Drive and MS OneDrive so I don't think the situation is unsalvageable in this case (I expect they'd add support for the Directory-based API quickly). If we were to ship the Directory-based API and provide a polyfill to make old Chrome and new Edge behave like the new API I think we could head off content that uses the old API building up. Then again I seem to be the only person with any appetite for that fight despite the fact that the new API is significantly better.

Against the old API I'd note that Apple has stated they prefer Jonas and Arun's new API, and the Chrome guys removed their spec for the old API from w3.org and have stated that they're essentially just waiting for us to ship something before following suite.

Comment 16

[Johnny Stenback (:jst)]

We've already designed the new API but we're IMO in no position any longer to not implement the Chrome API. Given that, I see no reason for us to spend time and energy on trying to push for a better API here given the circumstances. If Chrome decides they really want to see this new API on the web, I'd be happy for Mozilla to follow suite.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Jonas Sicking (:sicking) from comment #2)
> Honestly, I think we should implement the full Chrome API and use it in both
> drag'n'drop and in <input>. No need to drag our feet in search for a better
> API any more.
> 
> I don't think we should implement the sandboxed filesystem API yet though.
So you don't mean full chrome API, but the same subset as what MS is implementing.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8742887 [details] [diff] [review]
part 1 - WebIDL

>+[NoInterfaceObject]
>+interface DirectoryEntry : Entry {
>+    [Throws]
>+    DirectoryReader createReader();
>+
>+    [Throws]
>+    void getFile(DOMString? path, optional FileSystemFlags options, optional EntryCallback successCallback, optional ErrorCallback errorCallback);
>+
>+    [Throws]
>+    void getDirectory(DOMString? path, optional FileSystemFlags options, optional EntryCallback successCallback, optional ErrorCallback errorCallback);
>+
Both methods here have TreatUndefinedAs=NullString in blink for path param. I *think* that is effectively 'optional DOMString? path = null'.
Please test what happens when no param is passed to those methods or first param is undefined.

>+callback interface ErrorCallback {
>+    // This should be FileError but we are implementing just a subset of this
>+    // API.
Nit, "API" should fit into the end of the previous line


>+partial interface HTMLInputElement {
>+  [Pref="dom.webkit.filesystem.enabled", Cached, Constant, GetterThrows]
>+  readonly attribute sequence<Entry> webkitEntries;
>+
>+  [Pref="dom.webkit.filesystem.enabled", BinaryName="WebkitdirectoryAttr", SetterThrows]
>+          attribute boolean webkitdirectory;

I think we want one pref to control webkitdirectory and webkitrelativepath, and then a separate one for
webkitEntries and stuff like that.
Why webkitdirectory needs a binary name? Please remove it, if possible.

Those fixed, r+.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8743255 [details] [diff] [review]
part 2 - FileEntry and DirectoryEntry


>+function test_entries() {
>+  var entries = document.getElementById('entries');
>+  ok("webkitEntries" in entries, "HTMLInputElement.webkitEntries");
>+  is(entries.webkitEntries.length, 2, "HTMLInputElement.webkitEntries.length == 2");
>+
>+  for (var i = 0; i < entries.webkitEntries.length; ++i) {
>+    if (entries.webkitEntries[i].isFile) {
>+      ok(!fileEntry, "We just want 1 fileEntry");
>+      fileEntry = entries.webkitEntries[i];
>+    } else {
>+      ok(entries.webkitEntries[i].isDirectory, "If not a file, we have a directory.");
>+      ok(!directoryEntry, "We just want 1 directoryEntry");
>+      directoryEntry = entries.webkitEntries[i];
>+    }
We must have tests also the ensure that input element's .files is populated properly, since
.files is what people seem to use usually even with dnd.

Chrome seems to behave rather oddly here. If one has webkitdirectory attribute and one does dnd, webkitEntries is empty.
If webkitdirectory is not set, but multiple attribute is, then webkitEntries is non-empty.

Could you please check chromium bugs about this issue, whether it is designed feature or considered as a bug.

We need some tests, at least manual, for dnd for <input type=file>, <input type=file multiple> and <input type=file webkitdirectory> 

I think I could give r+ too, but I'd prefer to see some more tests.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I mean the same subset as MS is implementing. But both for <input> and for drag'n'drop. I suspect this essentially comes down to exposing the read-only parts of the API, but I'm not entirely sure.

I'll also start an email thread on this so that we can discuss our options.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8743256 [details] [diff] [review]
part 3 - FileEntry methods

>+  Run() override
>+  {
>+    nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(mGlobal);
>+    if (NS_WARN_IF(!window)) {
>+      return NS_ERROR_FAILURE;
>+    }
>+
>+    RefPtr<DOMError> error = new DOMError(window, NS_ERROR_NOT_IMPLEMENTED);
I think NotSupportedError (NS_ERROR_DOM_NOT_SUPPORTED_ERR) would be better, given that it is something defined in specs.
NS_ERROR_NOT_IMPLEMENTED is Gecko internal.

>+++ b/dom/webidl/DOMFileSystem.webidl
>@@ -47,17 +47,16 @@ interface DirectoryEntry : Entry {
>     DirectoryReader createReader();
> 
>     [Throws]
>     void getFile(DOMString? path, optional FileSystemFlags options, optional EntryCallback successCallback, optional ErrorCallback errorCallback);
> 
>     [Throws]
>     void getDirectory(DOMString? path, optional FileSystemFlags options, optional EntryCallback successCallback, optional ErrorCallback errorCallback);
> 
>-    [Throws]
>     void removeRecursively(VoidCallback successCallback, optional ErrorCallback errorCallback);
Might be good to document in .webidl which methods we don't support, like this removeRecursively

> interface FileEntry : Entry {
>     // the successCallback should be a FileWriteCallback but this method is not
>     // implemented.
>-    [Throws]
>     void createWriter (VoidCallback successCallback, optional ErrorCallback errorCallback);
and looks like that is documented here already.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8743257 [details] [diff] [review]
part 4 - DirectoryReader

>+  ResolvedCallback(JSContext* aCx, JS::Handle<JS::Value> aValue) override
>+  {
>+    if(NS_WARN_IF(!aValue.isObject())) {
>+      return;
>+    }
>+
>+    JS::Rooted<JSObject*> obj(aCx, &aValue.toObject());
>+
>+    uint32_t length;
>+    if (NS_WARN_IF(!JS_GetArrayLength(aCx, obj, &length))) {
>+      return;
>+    }
Should we check that we're dealing with an array (isArray()) here, not just isObject() check.

>+
>+    Sequence<OwningNonNull<Entry>> sequence;
>+    if (NS_WARN_IF(!sequence.SetLength(length, fallible))) {
>+      return;
>+    }
>+
>+    for (uint32_t i = 0; i < length; ++i) {
>+      JS::Rooted<JS::Value> value(aCx);
>+      if (NS_WARN_IF(!JS_GetElement(aCx, obj, i, &value))) {
>+        return;
>+      }
>+
>+      if(NS_WARN_IF(!value.isObject())) {
>+        return;
>+      }
>+
>+      JS::Rooted<JSObject*> valueObj(aCx, &value.toObject());
>+
>+      RefPtr<File> file;
>+      if (NS_SUCCEEDED(UNWRAP_OBJECT(File, valueObj, file))) {
>+        RefPtr<FileEntry> entry = new FileEntry(mGlobal, file);
>+        sequence[i] = entry;
>+        continue;
>+      }
>+
>+      RefPtr<Directory> directory;
>+      if (NS_WARN_IF(NS_FAILED(UNWRAP_OBJECT(Directory, valueObj,
>+                                             directory)))) {
>+        return;
>+      }
>+
>+      RefPtr<DirectoryEntry> entry = new DirectoryEntry(mGlobal, directory);
>+      sequence[i] = entry;
>+    }
Would be so nice if bindings layer generated code for JSValue->sequence conversions to be used in C++ code. oh well...


>+DirectoryReader::ReadEntries(EntriesCallback& aSuccessCallback,
>+                             const Optional<OwningNonNull<ErrorCallback>>& aErrorCallback,
>+                             ErrorResult& aRv)
>+{
>+  if (mAlreadyRead) {
>+    RefPtr<EmptyEntriesCallbackRunnable> runnable =
>+      new EmptyEntriesCallbackRunnable(&aSuccessCallback);
>+    aRv = NS_DispatchToMainThread(runnable);
>+    NS_WARN_IF(aRv.Failed());
>+    return;
>+  }
>+
>+  // This object can be used only once.
might be worth to mention this also in .webidl. It is totally unclear from it that the method is effectively just a one time
thing.

+  RejectedCallback(JSContext* aCx, JS::Handle<JS::Value> aValue) override
+  {
+    if (mErrorCallback) {
+      RefPtr<ErrorCallbackRunnable> runnable =
+        new ErrorCallbackRunnable(mGlobal, mErrorCallback, NS_ERROR_FAILURE);
You don't want to pass Gecko expection names to web, use InvalidStateError or some such here.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8743258 [details] [diff] [review]
part 5 - DOMFileSystem

>-  explicit ErrorCallbackRunnable(nsIGlobalObject* aGlobalObject,
>-                                 ErrorCallback* aCallback,
>-                                 nsresult aError = NS_ERROR_NOT_IMPLEMENTED)
>+  ErrorCallbackRunnable(nsIGlobalObject* aGlobalObject,
>+                        ErrorCallback* aCallback,
>+                        nsresult aError = NS_ERROR_NOT_IMPLEMENTED)
make the default value for aError to be NS_ERROR_DOM_NOT_SUPPORTED_ERR or some such

>+DOMFileSystem::Create(nsIGlobalObject* aGlobalObject)
>+
>+{
>+  MOZ_ASSERT(aGlobalObject);
>+
>+  nsID id;
>+  nsresult rv = nsContentUtils::GenerateUUIDInPlace(id);
>+  if (NS_WARN_IF(NS_FAILED(rv))) {
>+    return nullptr;
>+  }
>+
>+  char chars[NSID_LENGTH];
>+  id.ToProvidedString(chars);
>+  nsAutoCString name(Substring(chars + 1, chars + NSID_LENGTH - 2));
Please add a comment why + 1 and - 2
And this whole UUDI magic could use some comment


>+RootDirectoryReader::ReadEntries(EntriesCallback& aSuccessCallback,
>+                                 const Optional<OwningNonNull<ErrorCallback>>& aErrorCallback,
>+                                 ErrorResult& aRv)
>+{
>+  if (mAlreadyRead) {
>+    RefPtr<EmptyEntriesCallbackRunnable> runnable =
>+      new EmptyEntriesCallbackRunnable(&aSuccessCallback);
>+    aRv = NS_DispatchToMainThread(runnable);
>+    NS_WARN_IF(aRv.Failed());
>+    return;
>+  }
>+
>+  // This object can be used only once.
>+  mAlreadyRead = true;
This mAlreadyRead stuff could use also some comment, like in the normal Directory case

>+function test_filesystem() {
>+  is(fileEntry.filesystem, directoryEntry.filesystem, "FileSystem object is shared.");
>+
>+  var fs = fileEntry.filesystem;
>+  ok(fs.name, "FileSystem.name exists.");
>+  ok(fs.root, "FileSystem has a root.");
>+
>+  is(fs.root.name, "", "FileSystem.root.name must be an empty string.");
>+  is(fs.root.fullPath, "/", "FileSystem.root.fullPath must be '/'");
Do we have tests the non-root directories have fullPath starting with '/'?

>+
>+  reader = fs.root.createReader();
>+  reader.readEntries(function(a) {
>+    ok(Array.isArray(a), "We want an array.");
>+    is(a.length, 2, "reader.readyEntries returns 2 elements.");
Could you actually test that the entries are the expected ones


I'm amazed how simple all this code is on top of the Directory API. Which is good. It might hint to MS folks too that
implementing the Directory upload proposal API and then making blink API to work on top of that is a possible solution.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8743260 [details] [diff] [review]
part 6 - getFile and getDirectory

+  /**
+   * Return true if this is valid DOMPath. It also splits the path in
+   * subdirectories and stores them in aParts.
+   */
if this is a valid


>+++ b/dom/filesystem/GetFileOrDirectoryTask.cpp
>@@ -267,20 +267,16 @@ GetFileOrDirectoryTaskParent::IOWork()
>     return rv;
>   }
> 
>   if (!isFile) {
>     // Neither directory or file.
>     return NS_ERROR_DOM_FILESYSTEM_TYPE_MISMATCH_ERR;
>   }
> 
>-  if (!mFileSystem->IsSafeFile(mTargetPath)) {
>-    return NS_ERROR_DOM_SECURITY_ERR;
>-  }
>-
Could you explain this?

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8743260 [details] [diff] [review]
part 6 - getFile and getDirectory

Waiting for explanation to the question. I thought we agreed last week that better to not do unrelated changes in this bug even though DeviceStorage might be going away soon.

Comment 26

[Andrea Marchesini [:baku]]

Attachment: part 6 - getFile and getDirectory

Comment 27

[Andrea Marchesini [:baku]]

Attachment: patch 2 - entries

I added a test for .files.
About chromium bugs for webkitdirectory + multiple, I didn't find anything.
Do we want to implement the same behavior here?

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

I think we should follow blink's behavior as much as possible, even though it is totally bizarre.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8760171 [details] [diff] [review]
patch 2 - entries

>+DirectoryEntry::GetName(nsAString& aName, ErrorResult& aRv) const
>+{
>+  mDirectory->GetName(aName, aRv);
>+}
>+
>+void
>+DirectoryEntry::GetFullPath(nsAString& aPath, ErrorResult& aRv) const
>+{
>+  mDirectory->GetPath(aPath, aRv);
>+}
You've ensured that we end up returning exactly the same values for .name and .fullpath as what blink does, right?


>+function test_entries() {
>+  var entries = document.getElementById('entries');
>+  ok("webkitEntries" in entries, "HTMLInputElement.webkitEntries");
so do we have tests that webkitEntries is empty array when dnd isn't used?


And ok, we aren't replicating blink's odd behavior when webkitdirectory is used and dnd happens. I guess that is ok.

So we need bugs filed/fixed to support dnd, and only after that we can enable the API.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0c48d9a4960
Subset of Blink FileSystem API - patch 1 - WebIDL, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4d2ec0d2be1
Subset of Blink FileSystem API - patch 2 - Entries, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/7af7af94985a
Subset of Blink FileSystem API - patch 3 - FileEntry methods, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d22217515a1e
Subset of Blink FileSystem API - patch 4 - DirectoryEntry methods, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/56fe0bfb503c
Subset of Blink FileSystem API - patch 5 - DOMFileSystem, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ff81864d002
Subset of Blink FileSystem API - patch 6 - getFile and getDirectory, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/088bc6c7f00f
Subset of Blink FileSystem API - patch 7 - DnD tests, r=smaug

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b0c48d9a4960
https://hg.mozilla.org/mozilla-central/rev/f4d2ec0d2be1
https://hg.mozilla.org/mozilla-central/rev/7af7af94985a
https://hg.mozilla.org/mozilla-central/rev/d22217515a1e
https://hg.mozilla.org/mozilla-central/rev/56fe0bfb503c
https://hg.mozilla.org/mozilla-central/rev/8ff81864d002
https://hg.mozilla.org/mozilla-central/rev/088bc6c7f00f

Comment 32

[Eric Shepherd [:sheppy]]

Google already wrote a lot of (most of) this documentation, but it’s somewhat out of date and of course includes stuff we don’t implement. I am working on doc updates now. Status:

DirectoryReader renamed to FileSystemDirectoryReader. Compatibility information updated, specification info (such as it is) added, and content cleaned up and revised into standard MDN form. Subpage created for readEntries() method also in MDN standard form. This interface is done.

FileSystem updated with compat and spec info. Content cleaned into standard MDN form with subpages created. This interface still needs samples for the root and name properties: https://developer.mozilla.org/en-US/docs/Web/API/FileSystem.name and https://developer.mozilla.org/en-US/docs/Web/API/FileSystem/root

All the others have been renamed and have had first passes of updates for Firefox 50 done, but there’s more left to do. Coming the next few days.

Comment 33

[Eric Shepherd [:sheppy]]

FileSystemFileEntry docs are finished: https://developer.mozilla.org/en-US/docs/Web/API/FileSystemFileEntry. Changes include compat info updates, spec info (such as it is, again), content cleanup and standardization.

FileSystemDirectoryEntry docs are finished: https://developer.mozilla.org/en-US/docs/Web/API/FileSystemDirectoryEntry; this includes all subpages, compat tables, spec info and so forth.

FileSystemEntry docs are done as well. https://developer.mozilla.org/en-US/docs/Web/API/FileSystemEntry.

Wrote doc for Metadata (even though we don’t implement it, because it only took a few minutes so why not be thorough?): https://developer.mozilla.org/en-US/docs/Web/API/Metadata and subpages.

Created doc for FileSystemFlags (https://developer.mozilla.org/en-US/docs/Web/API/FileSystemFlags). I’m working on the subpages now; will only take a few more minutes to wrap up.

Then I need to do the page detailing specifics of the Firefox implementation as opposed to Chrome’s.

There will at that point be some stuff left that ought to be done to make things fully finished, but that will get us to the point where we’re good enough for Firefox 50.

I’ll update and clear ddn when I’m finished, shortly.

Comment 34

[Eric Shepherd [:sheppy]]

FileSystemFlags.create and exclusive pages are in place:

https://developer.mozilla.org/en-US/docs/Web/API/FileSystemFlags/create
https://developer.mozilla.org/en-US/docs/Web/API/FileSystemFlags/exclusive

Moving on to the last document, about Firefox’s support specifically.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Sheppy, you want to take a look at also what Edge has. It should be shipping pretty much the same set of API as Firefox.
(Don't have Edge right now to test that.)

Comment 36

[Eric Shepherd [:sheppy]]

More stuff done:

https://developer.mozilla.org/en-US/docs/Web/API/File/webkitRelativePath

https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement/webkitdirectory
https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement/webkitEntries

Updated https://developer.mozilla.org/en-US/docs/Web/API/DataTransferItem to include webkitGetAsEntry(), then created:
https://developer.mozilla.org/en-US/docs/Web/API/DataTransferItem/webkitGetAsEntry (including a nice live sample)

Firefox 50 for developers is now up to date as well. This is done!