Bug 231676

Summary: Implement FileSystemHandle move()
Product: WebKit Reporter: Sihui Liu <sihui_liu>
Component: New BugsAssignee: Sihui Liu <sihui_liu>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, esprehn+autocc, ews-watchlist, kondapallykalyan, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 231706    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch for landing none

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].