[webkit-reviews] review granted: [Bug 196081] Web Automation: support uploading non-local file paths : [Attachment 365563] Proposed Fix

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 196081: Web Automation: support uploading non-local file paths
https://bugs.webkit.org/show_bug.cgi?id=196081

Attachment 365563: Proposed Fix

https://bugs.webkit.org/attachment.cgi?id=365563&action=review




--- Comment #3 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 365563
  --> https://bugs.webkit.org/attachment.cgi?id=365563
Proposed Fix

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

r=me, although you may want to get another look of someone more familiar with
ObjC :)

> Source/WebKit/UIProcess/Automation/Automation.json:623
> +		   { "name": "filenames", "type": "array", "items": { "$ref":
"string" }, "description": "Absolute paths to the files that should be
selected." },
> +		   { "name": "fileContents", "type": "array", "items": { "$ref"
: "string" }, "optional": true, "description": "An array of Base64-encoded
binary data for each file to be selected. If this property is provided, it is
assumed that 'filenames' are not real file paths on the session host's
filesystem, and this binary data will be used instead." }

Shouldn't these be `"type": "string"`?

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1252
> +	       SYNC_FAIL_WITH_PREDEFINED_ERROR_AND_DETAILS(InternalError, "The
parameter 'filenames' contains a non-string value.");

Should we add more detail, like "The parameter 'filenames' contains a
non-string value at index ${i}."?

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1261
> +	       SYNC_FAIL_WITH_PREDEFINED_ERROR_AND_DETAILS(InternalError, "The
parameter 'fileContents' contains a non-string value.");

Ditto (>1252).

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1263
> +	   Optional<String> localFilePath =
platformGenerateLocalFilePathForRemoteFile(filename, fileData);

`auto`?

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1264
> +	   if (!localFilePath.hasValue())

You can drop the `hasValue`.  It's the same as `operator bool`.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1265
> +	       SYNC_FAIL_WITH_PREDEFINED_ERROR_AND_DETAILS(InternalError, "The
remote file could not be saved to a local temporary directory.");

Ditto (>1252).

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:2068
> +Optional<String>
WebAutomationSession::platformGenerateLocalFilePathForRemoteFile(const String&
remoteFilePath, const String& base64EncodedFileContents)

Don't include the parameter names if they aren't used (or surround them in an
inline comment) (or use an `UNUSED_PARAM`).

> Source/WebKit/UIProcess/Automation/cocoa/WebAutomationSessionCocoa.mm:64
> +    NSString *temporaryDirectory =
FileSystem::createTemporaryDirectory(@"WebDriver");
> +    NSURL *remoteFile = [NSURL fileURLWithPath:remoteFilePath
isDirectory:NO];
> +    NSString *localFilePath = [temporaryDirectory
stringByAppendingPathComponent:remoteFile.lastPathComponent];

I'd move these below the `if (!fileContents)` check, as they aren't needed
before that.

> Source/WebKit/UIProcess/Automation/cocoa/WebAutomationSessionCocoa.mm:65
> +    NSData *fileContents = [[NSData alloc]
initWithBase64EncodedString:base64EncodedFileContents options:0];

Should we `adoptNS` or retain/release this?