Bug 231676 - Implement FileSystemHandle move()
Summary: Implement FileSystemHandle move()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar
Depends on:
Blocks: 231706
  Show dependency treegraph
 
Reported: 2021-10-13 08:25 PDT by Sihui Liu
Modified: 2021-10-13 18:39 PDT (History)
6 users (show)

See Also:


Attachments
Patch (29.42 KB, patch)
2021-10-13 08:43 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (28.48 KB, patch)
2021-10-13 09:00 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (31.64 KB, patch)
2021-10-13 09:43 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (32.94 KB, patch)
2021-10-13 10:37 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (32.37 KB, patch)
2021-10-13 10:38 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sihui Liu 2021-10-13 08:25:27 PDT
...
Comment 1 Sihui Liu 2021-10-13 08:43:05 PDT
Created attachment 441084 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2021-10-13 08:43:35 PDT
<rdar://problem/84199823>
Comment 3 Sihui Liu 2021-10-13 09:00:27 PDT
Created attachment 441087 [details]
Patch
Comment 4 Sihui Liu 2021-10-13 09:43:06 PDT
Created attachment 441092 [details]
Patch
Comment 5 youenn fablet 2021-10-13 10:04:33 PDT
Comment on attachment 441092 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=441092&action=review

> Source/WebCore/ChangeLog:9
> +        Provide a way to move an existing file to a new place in OPFS.

APFS?

> Source/WebCore/Modules/filesystemaccess/FileSystemHandle.cpp:66
> +        promise.settle(WTFMove(result));

As a future refactoring, we should probably enqueue a task to resolve the promises. That will make sure we do this in the event loop.

> Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:77
> +    return !(name == "." || name == ".." || name.contains(pathSeparator));

Might be easier to read as: name != "." && name != ".." && !name.containts(pathSeparator).

> Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:108
> +    // Do not remove file if there is open access handle as web process may be performing oeprations.

s/oeprations/operations/

> Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:110
> +        return FileSystemStorageError::InvalidState;

Do we have a test for it?

> Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:273
> +        return FileSystemStorageError::InvalidState;

Do we have a test for it?

> Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:283
> +        return FileSystemStorageError::InvalidName;

Do we have a test for it?
Comment 6 Sihui Liu 2021-10-13 10:13:21 PDT
(In reply to youenn fablet from comment #5)
> Comment on attachment 441092 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441092&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Provide a way to move an existing file to a new place in OPFS.
> 
> APFS?

Will use Origin Private File System.

> 
> > Source/WebCore/Modules/filesystemaccess/FileSystemHandle.cpp:66
> > +        promise.settle(WTFMove(result));
> 
> As a future refactoring, we should probably enqueue a task to resolve the
> promises. That will make sure we do this in the event loop.

Sure.

> 
> > Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:77
> > +    return !(name == "." || name == ".." || name.contains(pathSeparator));
> 
> Might be easier to read as: name != "." && name != ".." &&
> !name.containts(pathSeparator).

Will change.

> 
> > Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:108
> > +    // Do not remove file if there is open access handle as web process may be performing oeprations.
> 
> s/oeprations/operations/

Will change.

> 
> > Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:110
> > +        return FileSystemStorageError::InvalidState;
> 
> Do we have a test for it?

No, I will update existing test.

> 
> > Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:273
> > +        return FileSystemStorageError::InvalidState;
> 
> Do we have a test for it?

Yes, it's in the new test.

> 
> > Source/WebKit/NetworkProcess/storage/FileSystemStorageHandle.cpp:283
> > +        return FileSystemStorageError::InvalidName;
> 
> Do we have a test for it?

No, will add it in the new test.
Comment 7 Sihui Liu 2021-10-13 10:37:08 PDT
Created attachment 441099 [details]
Patch for landing
Comment 8 Sihui Liu 2021-10-13 10:38:23 PDT
Created attachment 441100 [details]
Patch for landing
Comment 9 EWS 2021-10-13 13:46:56 PDT
Committed r284124 (242944@main): <https://commits.webkit.org/242944@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441100 [details].