Bug 174797 - Web Automation: add support for uploading files
Summary: Web Automation: add support for uploading files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks: 174803
  Show dependency treegraph
 
Reported: 2017-07-24 13:58 PDT by BJ Burg
Modified: 2017-07-25 11:03 PDT (History)
11 users (show)

See Also:


Attachments
Patch (40.31 KB, patch)
2017-07-24 15:04 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
For Landing (45.87 KB, patch)
2017-07-24 16:51 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
For Landing (47.40 KB, patch)
2017-07-24 20:57 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Fix CMake builds (46.78 KB, patch)
2017-07-25 04:59 PDT, Carlos Garcia Campos
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2017-07-24 13:58:53 PDT
placeholder text.
Comment 1 BJ Burg 2017-07-24 13:59:35 PDT
<rdar://problem/28485063>
Comment 2 BJ Burg 2017-07-24 15:04:46 PDT
Created attachment 316320 [details]
Patch
Comment 3 Joseph Pecoraro 2017-07-24 15:29:52 PDT
Comment on attachment 316320 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:507
> +    // Per §14.3.10.5 in the W3C spec, if at least one file no longer exists, the command should fail.

"no longer exists" => "does not exist"

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:510
> +        if (!WebCore::fileExists(filename)) {

Did you also want to check that the file is readable by this process?

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:517
> +    // FIXME: validate filenames against allowed MIME types before choosing them.

Bugzilla bug before landing.

> Source/WebKit/UIProcess/WebPageProxy.cpp:4125
> +    if (m_controlledByAutomation) {
> +        if (auto* automationSession = process().processPool().automationSession()) {
> +            automationSession->handleRunOpenPanel(*this, *frame, parameters.get(), *m_openPanelResultListener);
> +            return;
> +        }
> +    }
> +
>      // Since runOpenPanel() can spin a nested run loop we need to turn off the responsiveness timer.
>      m_process->responsivenessTimer().stop();

Should we always return while controlled by automation even if there was no automation session?

Likewise we should probably let the responsiveness timer stop before returning above.

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:9981
> +				99249AD51F1F1E5600B62FBB /* AutomationFrontendDispatchers.cpp in Sources */,

Nit: You could run `sort-Xcode-project-file WebKit.xcodeproj/project.pbxproj` and only add your new bits, but then they would be in the correct locations.
Comment 4 BJ Burg 2017-07-24 16:29:03 PDT
(In reply to Joseph Pecoraro from comment #3)
> Comment on attachment 316320 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316320&action=review
> 
> r=me
> 
> > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:507
> > +    // Per §14.3.10.5 in the W3C spec, if at least one file no longer exists, the command should fail.
> 
> "no longer exists" => "does not exist"
> 
> > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:510
> > +        if (!WebCore::fileExists(filename)) {
> 
> Did you also want to check that the file is readable by this process?

This check will fail if the file is not readable by the process. Additionally, safaridriver will check for world-readable permissions before starting off the command, since that's not easy to do in cross-platform code. Other drivers may want a looser policy, as well.

> > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:517
> > +    // FIXME: validate filenames against allowed MIME types before choosing them.
> 
> Bugzilla bug before landing.

Ok.

> > Source/WebKit/UIProcess/WebPageProxy.cpp:4125
> > +    if (m_controlledByAutomation) {
> > +        if (auto* automationSession = process().processPool().automationSession()) {
> > +            automationSession->handleRunOpenPanel(*this, *frame, parameters.get(), *m_openPanelResultListener);
> > +            return;
> > +        }
> > +    }
> > +
> >      // Since runOpenPanel() can spin a nested run loop we need to turn off the responsiveness timer.
> >      m_process->responsivenessTimer().stop();
> 
> Should we always return while controlled by automation even if there was no
> automation session?

I can't see how that would happen, but sure.

> Likewise we should probably let the responsiveness timer stop before
> returning above.

OK.

> > Source/WebKit/WebKit.xcodeproj/project.pbxproj:9981
> > +				99249AD51F1F1E5600B62FBB /* AutomationFrontendDispatchers.cpp in Sources */,
> 
> Nit: You could run `sort-Xcode-project-file
> WebKit.xcodeproj/project.pbxproj` and only add your new bits, but then they
> would be in the correct locations.

I will, was waiting to rebase first.
Comment 5 BJ Burg 2017-07-24 16:32:43 PDT
(In reply to Joseph Pecoraro from comment #3)
> Comment on attachment 316320 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316320&action=review
> 
> r=me
> 
> > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:507
> > +    // Per §14.3.10.5 in the W3C spec, if at least one file no longer exists, the command should fail.
> 
> "no longer exists" => "does not exist"
> 
> > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:510
> > +        if (!WebCore::fileExists(filename)) {
> 
> Did you also want to check that the file is readable by this process?
> 
> > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:517
> > +    // FIXME: validate filenames against allowed MIME types before choosing them.
> 
> Bugzilla bug before landing.
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:4125
> > +    if (m_controlledByAutomation) {
> > +        if (auto* automationSession = process().processPool().automationSession()) {
> > +            automationSession->handleRunOpenPanel(*this, *frame, parameters.get(), *m_openPanelResultListener);
> > +            return;
> > +        }
> > +    }
> > +
> >      // Since runOpenPanel() can spin a nested run loop we need to turn off the responsiveness timer.
> >      m_process->responsivenessTimer().stop();
> 
> Should we always return while controlled by automation even if there was no
> automation session?
> 
> Likewise we should probably let the responsiveness timer stop before
> returning above.

Actually, no, this isn't needed because WebAutomationSession::handleRunOpenPanel does not have a nested run loop. It just picks whatever files were set last, if anything. The fact that the completion handler is possibly called asynchronously should not matter.
Comment 6 BJ Burg 2017-07-24 16:51:08 PDT
Created attachment 316328 [details]
For Landing
Comment 7 Build Bot 2017-07-24 16:54:03 PDT
This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)
Comment 8 Build Bot 2017-07-24 16:54:14 PDT
Attachment 316328 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/scripts/generate-inspector-protocol-bindings.py:173:  [generate_from_specification] Undefined variable 'CppFrontendDispatcherHeaderGenerator'  [pylint/E0602] [5]
Total errors found: 1 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 BJ Burg 2017-07-24 20:57:10 PDT
Created attachment 316348 [details]
For Landing
Comment 10 Build Bot 2017-07-24 21:00:36 PDT
Attachment 316348 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/scripts/generate-inspector-protocol-bindings.py:173:  [generate_from_specification] Undefined variable 'CppFrontendDispatcherHeaderGenerator'  [pylint/E0602] [5]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Carlos Garcia Campos 2017-07-25 04:59:41 PDT
Created attachment 316364 [details]
Fix CMake builds
Comment 12 Build Bot 2017-07-25 05:01:49 PDT
Attachment 316364 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/scripts/generate-inspector-protocol-bindings.py:173:  [generate_from_specification] Undefined variable 'CppFrontendDispatcherHeaderGenerator'  [pylint/E0602] [5]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 WebKit Commit Bot 2017-07-25 09:32:58 PDT
Comment on attachment 316364 [details]
Fix CMake builds

Rejecting attachment 316364 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 316364, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/JavaScriptCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/4185103
Comment 14 BJ Burg 2017-07-25 09:38:55 PDT
Commit queue is awful.
Comment 15 Carlos Garcia Campos 2017-07-25 09:42:35 PDT
Reviewer was not filled in the changelog in the patch, I didn't notice it. Joe, could you please r+, cq+ it? That should just work.
Comment 16 BJ Burg 2017-07-25 09:45:01 PDT
(In reply to Carlos Garcia Campos from comment #15)
> Reviewer was not filled in the changelog in the patch, I didn't notice it.
> Joe, could you please r+, cq+ it? That should just work.

It should have found it from the earlier reviewed patch. In any case, Joe won't be in for an hour or so, I'm going to land it manually.
Comment 17 BJ Burg 2017-07-25 11:03:09 PDT
Committed r219874: <http://trac.webkit.org/changeset/219874>