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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Mar 21 11:17:02 PDT 2019

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

Attachment 365563: Proposed Fix


--- 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
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
parameter 'fileContents' contains a non-string value.");

Ditto (>1252).

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


> 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
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 =
> +    NSURL *remoteFile = [NSURL fileURLWithPath:remoteFilePath
> +    NSString *localFilePath = [temporaryDirectory

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?

More information about the webkit-reviews mailing list