Bug 174797 – Web Automation: add support for uploading files

• Component: WebKit2
• Status: RESOLVED FIXED

[Blaze Burg]

placeholder text.

[Blaze Burg]

<rdar://problem/28485063>

[Blaze Burg]

Created attachment 316320 [details]
Patch

[Joseph Pecoraro]

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.

[Blaze Burg]

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

[Blaze Burg]

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

[Blaze Burg]

Created attachment 316328 [details]
For Landing

[Build Bot]

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`)

[Build Bot]

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.

[Blaze Burg]

Created attachment 316348 [details]
For Landing

[Build Bot]

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.

[Carlos Garcia Campos]

Created attachment 316364 [details]
Fix CMake builds

[Build Bot]

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.

[WebKit Commit Bot]

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

[Blaze Burg]

Commit queue is awful.

[Carlos Garcia Campos]

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.

[Blaze Burg]

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

[Blaze Burg]

Committed r219874: <http://trac.webkit.org/changeset/219874>