.
<rdar://problem/45819897>
Created attachment 365563 [details] Proposed Fix
Comment on attachment 365563 [details] 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?
Comment on attachment 365563 [details] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=365563&action=review > Source/WebKit/UIProcess/Automation/WebAutomationSession.h:266 > + // Save base64-encoded file contents to a local file path, and return the path. It may be worth mentioning that this uses the `lastPathComponent` of the `remoteFilePath` as the filename, just so it's a bit clearer why "/a/b/c.txt" might get moved to only be "WebDriver/c.txt".
Comment on attachment 365563 [details] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=365563&action=review r=me as well >> Source/WebKit/UIProcess/Automation/cocoa/WebAutomationSessionCocoa.mm:65 >> + NSData *fileContents = [[NSData alloc] initWithBase64EncodedString:base64EncodedFileContents options:0]; > > Should we `adoptNS` or retain/release this? Good catch!
(In reply to Devin Rousso from comment #4) > Comment on attachment 365563 [details] > Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=365563&action=review > > > Source/WebKit/UIProcess/Automation/WebAutomationSession.h:266 > > + // Save base64-encoded file contents to a local file path, and return the path. > > It may be worth mentioning that this uses the `lastPathComponent` of the > `remoteFilePath` as the filename, just so it's a bit clearer why > "/a/b/c.txt" might get moved to only be "WebDriver/c.txt". It'll be WebDriver-XXXXXXXX where the Xs are randomized.
Committed r243340: <https://trac.webkit.org/changeset/243340>