devtools: Add Browser.setFileChooserBehavior for file chooser automation [chromium/src : main]
Eriko Kurimoto (Gerrit)
Eriko Kurimoto added 6 comments![]( "Open in Gerrit")
scoped_select_file_dialog_factory_;Since BrowserHandler is instantiated per DevToolsSession (e.g. per window/page target in wptrunner), multiple instances can exist concurrently. What happens if two sessions set and reset scoped_select_file_dialog_factory_ out of LIFO order, or if one session sets a behavior and another session calls 'default' to reset it? (See also how SetGlobalPrivacyControl stores process-global override state.)
listener_->FileSelectionCanceled();In the WPT RFC (rfcs#246), providing multiple paths to a single-select picker is described as causing the test harness to fail at dialog invocation, whereas FileSelectionCanceled() here rejects the picker promise with a normal DOM AbortError (same as user cancellation). Is AbortError the intended behavior here (and should the RFC be updated to match), or how do you plan to distinguish this from user cancellation?
experimental command setFileChooserBehaviorThe WPT RFC (rfcs#246) includes a context parameter so the behavior applies only to the next picker invoked in that browsing context, whereas Browser.setFileChooserBehavior and ui::SelectFileDialog::SetFactory are process-global. How do you plan to handle the context parameter in test_driver.set_file_picker_behavior?
acceptnit: Does this still need to say absolute file paths after PS6, and should it also mention directory paths? (Same for the commit message.)
Could we also add a test case that overwrites an unconsumed behavior with another accept or cancel before calling showOpenFilePicker?
Related details
- Anna Sato
- Ayu Goto
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ayu Goto (Gerrit)
Ayu Goto added 5 comments![]( "Open in Gerrit")
Since BrowserHandler is instantiated per DevToolsSession (e.g. per window/page target in wptrunner), multiple instances can exist concurrently. What happens if two sessions set and reset scoped_select_file_dialog_factory_ out of LIFO order, or if one session sets a behavior and another session calls 'default' to reset it? (See also how SetGlobalPrivacyControl stores process-global override state.)
Moved ScopedSetSelectFileDialogFactory to a process-global static in browser_handler.cc (tracking the owning BrowserHandler for cleanup in Disable()), so all BrowserHandler sessions share a single override instance without LIFO ordering issues and 'default' resets the override across sessions. Also added testMultiSessionOverride in set-file-chooser-behavior.js.
In the WPT RFC (rfcs#246), providing multiple paths to a single-select picker is described as causing the test harness to fail at dialog invocation, whereas FileSelectionCanceled() here rejects the picker promise with a normal DOM AbortError (same as user cancellation). Is AbortError the intended behavior here (and should the RFC be updated to match), or how do you plan to distinguish this from user cancellation?
Since the behavior is configured ahead of time (before the picker opens and its SELECT_OPEN_FILE vs SELECT_OPEN_MULTI_FILE type is known), setFileChooserBehavior itself cannot reject the call upfront, and ui::SelectFileDialog::Listener only provides FileSelected and FileSelectionCanceled callbacks. Calling FileSelectionCanceled() rejects the picker promise with AbortError and fails the test assertion when a test mistakenly configures multiple paths for a single-file picker. I'll also clarify this wording in the WPT RFC.
The WPT RFC (rfcs#246) includes a context parameter so the behavior applies only to the next picker invoked in that browsing context, whereas Browser.setFileChooserBehavior and ui::SelectFileDialog::SetFactory are process-global. How do you plan to handle the context parameter in test_driver.set_file_picker_behavior?
We decided to remove the context parameter from both the WPT RFC (rfcs#246) and test_driver.set_file_picker_behavior, so the API is browser-global across the board (matching WebDriver BiDi's proposed browser.setFileDialogBehavior, Browser.setFileChooserBehavior, and ui::SelectFileDialog::SetFactory).
nit: Does this still need to say absolute file paths after PS6, and should it also mention directory paths? (Same for the commit message.)
Done. Updated Browser.pdl and the commit message to say "file or directory paths".
Could we also add a test case that overwrites an unconsumed behavior with another accept or cancel before calling showOpenFilePicker?
Done. Added testOverwriteBeforeConsumption (covering both accept -> accept and accept -> cancel) as well as testMultiSessionOverride.
Related details
- Anna Sato
- Eriko Kurimoto
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anna Sato (Gerrit)
Anna Sato added 2 comments![]( "Open in Gerrit")
acceptAyu Gotonit: Does this still need to say absolute file paths after PS6, and should it also mention directory paths? (Same for the commit message.)
Done. Updated Browser.pdl and the commit message to say "file or directory paths".
Shouldn't we keep the `IsAbsolute()` check? In Chrome, it seems the selected paths go through `ShouldBlockAccessToPath()`, which has [CHECK(path.IsAbsolute()](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/file_system_access/chrome_file_system_access_permission_context.cc;l=618?q=ShouldBlockAccessToPath()).
Even if WPT tests pass relative paths to `test_driver.set_file_picker_behavior()`, I'd expect wptrunner to resolve them to absolute paths (e.g. under a temp directory) before calling this CDP command. Is there a case where a relative path actually reaches here?
Related details
- Ayu Goto
- Eriko Kurimoto
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eriko Kurimoto (Gerrit)
Eriko Kurimoto added 3 comments![]( "Open in Gerrit")
lgtm, but not +1-ing yet since we should wait for w3c discussion to finalize our design.
Thanks for you quick work!
nit: Since scoped_select_file_dialog_factory_ moved to browser_handler.cc, this include is no longer needed here.
acceptAyu Gotonit: Does this still need to say absolute file paths after PS6, and should it also mention directory paths? (Same for the commit message.)
Anna SatoDone. Updated Browser.pdl and the commit message to say "file or directory paths".
Shouldn't we keep the `IsAbsolute()` check? In Chrome, it seems the selected paths go through `ShouldBlockAccessToPath()`, which has [CHECK(path.IsAbsolute()](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/file_system_access/chrome_file_system_access_permission_context.cc;l=618?q=ShouldBlockAccessToPath()).
Even if WPT tests pass relative paths to `test_driver.set_file_picker_behavior()`, I'd expect wptrunner to resolve them to absolute paths (e.g. under a temp directory) before calling this CDP command. Is there a case where a relative path actually reaches here?
+1 to Anna's point: since Browser.setFileChooserBehavior is browser-global and has no base directory to resolve relative paths against, restoring the IsAbsolute() check (and using the platform basePath in the test like in PS5) will return a clean InvalidParams error instead of hitting CHECK(path.IsAbsolute()) in Chrome.
Related details
- Ayu Goto
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ayu Goto (Gerrit)
Ayu Goto voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ayu Goto <ayu...@google.com>
| Commit-Queue | +0 |
2 comments
nit: Since scoped_select_file_dialog_factory_ moved to browser_handler.cc, this include is no longer needed here.
Done
acceptAyu Gotonit: Does this still need to say absolute file paths after PS6, and should it also mention directory paths? (Same for the commit message.)
Anna SatoDone. Updated Browser.pdl and the commit message to say "file or directory paths".
Eriko KurimotoShouldn't we keep the `IsAbsolute()` check? In Chrome, it seems the selected paths go through `ShouldBlockAccessToPath()`, which has [CHECK(path.IsAbsolute()](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/file_system_access/chrome_file_system_access_permission_context.cc;l=618?q=ShouldBlockAccessToPath()).
Even if WPT tests pass relative paths to `test_driver.set_file_picker_behavior()`, I'd expect wptrunner to resolve them to absolute paths (e.g. under a temp directory) before calling this CDP command. Is there a case where a relative path actually reaches here?
+1 to Anna's point: since Browser.setFileChooserBehavior is browser-global and has no base directory to resolve relative paths against, restoring the IsAbsolute() check (and using the platform basePath in the test like in PS5) will return a clean InvalidParams error instead of hitting CHECK(path.IsAbsolute()) in Chrome.
Yes, wptrunner will always resolve relative paths to absolute paths before calling this CDP command, so relative paths shouldn't reach here. Restored the path.IsAbsolute() check in browser_handler.cc (and basePath in set-file-chooser-behavior.js), and updated Browser.pdl and the commit message to say "absolute file or directory paths".
Related details
- Anna Sato
- Eriko Kurimoto
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anna Sato (Gerrit)
Anna Sato voted and added 2 comments on change 8602432![]( "Open in Gerrit")
Votes added by Anna Sato <anna...@chromium.org>
| Code-Review | +1 |
2 comments
void TearDown() override { ui::SelectFileDialog::SetFactory(nullptr); }nit: Each test already uses ui::ScopedSetSelectFileDialogFactory so this isn't needed?
Related details
- Ayu Goto
- Eriko Kurimoto
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ayu Goto (Gerrit)
Ayu Goto added 1 comment on change 8602432![]( "Open in Gerrit")
void TearDown() override { ui::SelectFileDialog::SetFactory(nullptr); }nit: Each test already uses ui::ScopedSetSelectFileDialogFactory so this isn't needed?
- Anna Sato
- Eriko Kurimoto
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anna Sato (Gerrit)
Anna Sato voted Code-Review+1 on change 8602432![]( "Open in Gerrit")
- Ayu Goto
- Eriko Kurimoto
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |