AWC window.maximize, minimize and restore promises implemented [chromium/src : main]
Patryk Chodur (Gerrit)
Patryk Chodur added 1 comment![]( "Open in Gerrit")
std::move(callback).Run(/*minimized=*/false);Patryk ChodurYou're removing this branching in https://chromium-review.googlesource.com/c/chromium/src/+/7106319/1/third_party/blink/renderer/core/page/chrome_client_impl.h. Is it really necessary for the API to include true/false return values?
Patryk ChodurAfter I remove the `#ifdefs` I think no. I plan to add timeouts to the callbacks which will require that, but given that it will be done in a different CL I can remove them when rebasing.
Discussed in chat. Keeping if for now for the timeouts.
Related details
- Andrew Rayskiy
- Olga Korokhina
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. |
Olga Korokhina (Gerrit)
Olga Korokhina added 1 comment![]( "Open in Gerrit")
"Could not restore the window."));Patryk ChodurCan we have a bit more of a context/reason here, f.i. "Window could not be restored due to [reason]", or this is not needed @greengrape ?
Olga KorokhinaAs of now these callbacks can't really be rejected unless somebody breaks something in the code. The checks for whether the API is allowed and makes sense were earlier, for example in the `IsPermissionGranted` function.
Got it, so we don't have a reason here, just a fact. Thank you.
Related details
- Andrew Rayskiy
- Patryk Chodur
Andrew Rayskiy (Gerrit)
Andrew Rayskiy voted and added 4 comments![]( "Open in Gerrit")
Votes added by Andrew Rayskiy
| Code-Review | +1 |
4 comments
Vector<MinimizeCallback> minimize_callbacks_;Patryk ChodurMaybe use `base/callback_list.h` here? I'm not sure what the guidance here is.
Andrew RayskiyI don't see it being used in the blink code ([here](https://source.chromium.org/search?q=CallbackList%20path:blink&sq=)) whereas a `Vector` with callbacks is used in many places ([here](https://source.chromium.org/search?q=case:yes%20pcre:yes%20Vector%3C%5B%5Cw%3C%3E%5Cs:%5D%2B%5BcC%5Dallback%5Cw%2B;%20path:blink)). Besides, `CallbackList` is mostly useful for fixing lifetime issues (with the `CallbackListSubscription`), but the promise resolved in these callbacks is garbage collected, so _as far as I understand_ there's no problem.
Ack!
ui::mojom::blink::WindowShowState old_show_state = window_show_state_;nit: I'd do a func-local `using WindowShowState = ui::mojom::blink::WindowShowState;` -- this might simplify the switch too
ui::mojom::blink::WindowShowState::kMaximized;nit: `using ui::mojom::blink::WindowShowState` in the anon namespace
if (window->GetFrame()->GetWidgetForLocalRoot()->WindowShowState() ==Olga Korokhina(here and elsewhere) this condition is hard to read. How about an anon func?
```
bool IsMaximized(LocalDOMWindow* window) { return ... }
```and then...
```
if (IsMaximized(window)) {
resolver->Resolve();
return;
}// inline the callback here...
window->GetFrame()->GetChromeClient().Maximize(*window->GetFrame(),
std::move(callback));
```Is there any sense in allowing parallel requests btw? i.e. calling minimize() while a maximize() call is in progress?
Patryk Chodur+1, as long as there are more than 1 inclusions of this check this seems to be reasonable.
Andrew RayskiyAdded the anon function and inlined the callback.
As for whether it makes sense to issue parallel requests – in general it doesn't make much sense to do it, but it's likely going to happen because:
- this is a UI thing and we're resolving the promises when the OS changes the window state. In case of any lag the user will likely try to hit various buttons and the app will issue appropriate requests
- the OS can ignore the requests to change the window state. What will happen right now is we'll never resolve nor reject the promise. I plan to add some timeouts, but even then we can have 2 pending promises waiting to be resolved or rejected.
Acknowledged
Related details
- Olga Korokhina
- Patryk Chodur
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. |
Olga Korokhina (Gerrit)
Olga Korokhina voted Code-Review+1![]( "Open in Gerrit")
- Patryk Chodur
Olga Korokhina (Gerrit)
Olga Korokhina added 1 comment![]( "Open in Gerrit")
Patryk Chodur (Gerrit)
Patryk Chodur added 2 comments![]( "Open in Gerrit")
ui::mojom::blink::WindowShowState old_show_state = window_show_state_;nit: I'd do a func-local `using WindowShowState = ui::mojom::blink::WindowShowState;` -- this might simplify the switch too
Done
nit: `using ui::mojom::blink::WindowShowState` in the anon namespace
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike Wasserman (Gerrit)
Mike Wasserman added 6 comments![]( "Open in Gerrit")
const char restore_script[] =optional nit: extract shared script string literals or test helper methods
I suggest a helper that takes the API method name and expected state, e.g.:
```
EvalJsResult EvalDisplayStateChange(
const ToRenderFrameHost& execution_target,
std::string_view window_method,
std::string_view expected_state) {
const char script[] =
R"(new Promise((resolve, reject) => {
window.$1().then(() => {
if (window.matchMedia('(display-state: $2)').matches) {
resolve('window.$1() succeeded.');
} else {
reject('window.$1() resolved, but ' +
'`display-state: $2` not matched.');
}
}).catch(() => reject('window.$1() rejected.'));
});)";
return content::EvalJs(web_contents, content::JsReplace(script, window_method, expected_state));
```
void WasMinimized();Consider adding a single `OnWindowShowStateChanged(ui::mojom::blink::WindowShowState)` instead.
for (auto& callback : callbacks) {This is a great improvement over the initial impl, thank you.
Still, this won't work really well when a sequence of JS methods are invoked, then promises are resolved out of order e.g. `m1 = minimize(); r1 = restore(); m2 = minimize();` m2 will resolve when the first minimize() completes, potentially before the window is restored and minimized a second time.
Getting this right depends on how requests are issued to the WM (looks like we don't currently queue requests internally, we just issue them to the WM directly) and how the underlying WM handles our requests. If the WM treats requests in a FIFO queue, we could probably keep a single ordered list of all callbacks (with an associated type), and resolve everything at the front of the queue matching the state surfaced here.
Getting this correct wouldn't be easy, so API spec/docs should explain what behavior clients can expect.
Maybe rejecting requests when another request is in flight would yield a simpler and more reliable impl?
if (old_show_state != window_show_state_) {There's no clear request denial signal at this layer, right? If so, when the WM silently ignores a request, the JS promise will hang indefinitely. I wonder if the promises should reject with some timeout? (e.g. no window state change in 3s?). Again, we don't need a perfect impl, but having something well documented and reliable would be great.
if (old_show_state == WindowShowState::kMinimized ||
old_show_state == WindowShowState::kFullscreen) {
// We cannot reliably tell whether the change was caused by restoring
// the window or maximizing it.
View()->WasRestored();
}Clients like WebViewImpl would be better served be a single notification of window state change (as I suggested above). Then WebViewImpl can decide how to resolve its own callbacks from the clearest underlying signals, not from potentially ambiguous interpretations encapsulated here.
const String& error_message, bool maximized) {This represents success for any of the operations, not just maximized
```suggestion
const String& error_message, bool result) {
```
Related details
- Patryk Chodur
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. |
Patryk Chodur (Gerrit)
Patryk Chodur added 6 comments![]( "Open in Gerrit")
optional nit: extract shared script string literals or test helper methods
I suggest a helper that takes the API method name and expected state, e.g.:
```
EvalJsResult EvalDisplayStateChange(
const ToRenderFrameHost& execution_target,
std::string_view window_method,
std::string_view expected_state) {
const char script[] =
R"(new Promise((resolve, reject) => {
window.$1().then(() => {
if (window.matchMedia('(display-state: $2)').matches) {
resolve('window.$1() succeeded.');
} else {
reject('window.$1() resolved, but ' +
'`display-state: $2` not matched.');
}
}).catch(() => reject('window.$1() rejected.'));
});)";
return content::EvalJs(web_contents, content::JsReplace(script, window_method, expected_state));
```
Done. I had to use `base::ReplaceStringPlaceholders`, because otherwise the names were quoted, and it didn't work with `std::string_view`.
Consider adding a single `OnWindowShowStateChanged(ui::mojom::blink::WindowShowState)` instead.
Done
for (auto& callback : callbacks) {This is a great improvement over the initial impl, thank you.
Still, this won't work really well when a sequence of JS methods are invoked, then promises are resolved out of order e.g. `m1 = minimize(); r1 = restore(); m2 = minimize();` m2 will resolve when the first minimize() completes, potentially before the window is restored and minimized a second time.
Getting this right depends on how requests are issued to the WM (looks like we don't currently queue requests internally, we just issue them to the WM directly) and how the underlying WM handles our requests. If the WM treats requests in a FIFO queue, we could probably keep a single ordered list of all callbacks (with an associated type), and resolve everything at the front of the queue matching the state surfaced here.
Getting this correct wouldn't be easy, so API spec/docs should explain what behavior clients can expect.
Maybe rejecting requests when another request is in flight would yield a simpler and more reliable impl?
The behavior of issuing multiple requests is very platform specific. As far as I remember, issuing multiple calls at the same time can cause the state changes to be merged/skipped. For example, when I tried minimizing and restoring on Wayland, only the first action was performed.
Additional problem is what happens if the user minimizes the window manually just before the `window.minimize()` was called. It'll result in a single minimization of the window. Should we resolve the promise in such case?
If we were to support sequences of window changes, _I think_ we would need to queue them on our own and issue the next request when the previous is finished. The problem here, as well as with rejecting promises, is what should happen if WM refuses to change the state. It can be fixed with a timeout though.
My idea was to guarantee only issuing request to the WM and resolving promises when the state is changed, but that would imply the developer shouldn't issue multiple requests at the same time. I resolve all of them simply because it's doable, and could be easier for developers if the user spammed one button because of WM lag. That solution might be too undefined for the web though, so I'm open to suggestions.
What do you think?
if (old_show_state != window_show_state_) {There's no clear request denial signal at this layer, right? If so, when the WM silently ignores a request, the JS promise will hang indefinitely. I wonder if the promises should reject with some timeout? (e.g. no window state change in 3s?). Again, we don't need a perfect impl, but having something well documented and reliable would be great.
That's correct, the promise won't be resolved. I was also thinking about a timeout. I plan to add it in a follow up CL.
if (old_show_state == WindowShowState::kMinimized ||
old_show_state == WindowShowState::kFullscreen) {
// We cannot reliably tell whether the change was caused by restoring
// the window or maximizing it.
View()->WasRestored();
}Clients like WebViewImpl would be better served be a single notification of window state change (as I suggested above). Then WebViewImpl can decide how to resolve its own callbacks from the clearest underlying signals, not from potentially ambiguous interpretations encapsulated here.
Done
This represents success for any of the operations, not just maximized
```suggestion
const String& error_message, bool result) {
```
- Mike Wasserman
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. |
Mike Wasserman (Gerrit)
Mike Wasserman voted and added 5 comments![]( "Open in Gerrit")
Votes added by Mike Wasserman
| Code-Review | +1 |
5 comments
lgtm with some non-blocking comments, and I'm happy to look over updates. I hope we can actually just reject simultaneous requests for now, otherwise the API contract is quite strange.
EXPECT_FALSE(helper()->browser_view()->IsMaximized());I wonder if any of these might need RunUntil (ditto elsewhere exiting fullscreen)
// Enter fullscreenoptional nit: a helper method might be useful (this is done 3+ times), it's quite close to EvalDisplayStateChange, but creating a parallel fullscreen helper is likely better than generalizing a shared helper.
for (auto& callback : callbacks) {Patryk ChodurThis is a great improvement over the initial impl, thank you.
Still, this won't work really well when a sequence of JS methods are invoked, then promises are resolved out of order e.g. `m1 = minimize(); r1 = restore(); m2 = minimize();` m2 will resolve when the first minimize() completes, potentially before the window is restored and minimized a second time.
Getting this right depends on how requests are issued to the WM (looks like we don't currently queue requests internally, we just issue them to the WM directly) and how the underlying WM handles our requests. If the WM treats requests in a FIFO queue, we could probably keep a single ordered list of all callbacks (with an associated type), and resolve everything at the front of the queue matching the state surfaced here.
Getting this correct wouldn't be easy, so API spec/docs should explain what behavior clients can expect.
Maybe rejecting requests when another request is in flight would yield a simpler and more reliable impl?
The behavior of issuing multiple requests is very platform specific. As far as I remember, issuing multiple calls at the same time can cause the state changes to be merged/skipped. For example, when I tried minimizing and restoring on Wayland, only the first action was performed.
Additional problem is what happens if the user minimizes the window manually just before the `window.minimize()` was called. It'll result in a single minimization of the window. Should we resolve the promise in such case?
If we were to support sequences of window changes, _I think_ we would need to queue them on our own and issue the next request when the previous is finished. The problem here, as well as with rejecting promises, is what should happen if WM refuses to change the state. It can be fixed with a timeout though.
My idea was to guarantee only issuing request to the WM and resolving promises when the state is changed, but that would imply the developer shouldn't issue multiple requests at the same time. I resolve all of them simply because it's doable, and could be easier for developers if the user spammed one button because of WM lag. That solution might be too undefined for the web though, so I'm open to suggestions.
What do you think?
This is a thorny area for sure, perhaps we should start simpler by just rejecting simultaneous requests?
Ultimately, I think queueing/coalescing/dropping WM requests ourselves and keeping a FIFO queue of promise callbacks might better match dev expectations than resolving all pending requests of the same type as this CL currently does.
if (old_show_state != window_show_state_) {Patryk ChodurThere's no clear request denial signal at this layer, right? If so, when the WM silently ignores a request, the JS promise will hang indefinitely. I wonder if the promises should reject with some timeout? (e.g. no window state change in 3s?). Again, we don't need a perfect impl, but having something well documented and reliable would be great.
That's correct, the promise won't be resolved. I was also thinking about a timeout. I plan to add it in a follow up CL.
Rejecting after a timeout seems like a good near-term followup. Letting promises hang indefinitely is not a good API contract.
Related details
- Patryk Chodur
Patryk Chodur (Gerrit)
Patryk Chodur added 4 comments![]( "Open in Gerrit")
EXPECT_FALSE(helper()->browser_view()->IsMaximized());I wonder if any of these might need RunUntil (ditto elsewhere exiting fullscreen)
We don't need to, because we wait for a promise to resolve in `EvalDisplayStateChange`.
optional nit: a helper method might be useful (this is done 3+ times), it's quite close to EvalDisplayStateChange, but creating a parallel fullscreen helper is likely better than generalizing a shared helper.
Done.
for (auto& callback : callbacks) {Patryk ChodurThis is a great improvement over the initial impl, thank you.
Still, this won't work really well when a sequence of JS methods are invoked, then promises are resolved out of order e.g. `m1 = minimize(); r1 = restore(); m2 = minimize();` m2 will resolve when the first minimize() completes, potentially before the window is restored and minimized a second time.
Getting this right depends on how requests are issued to the WM (looks like we don't currently queue requests internally, we just issue them to the WM directly) and how the underlying WM handles our requests. If the WM treats requests in a FIFO queue, we could probably keep a single ordered list of all callbacks (with an associated type), and resolve everything at the front of the queue matching the state surfaced here.
Getting this correct wouldn't be easy, so API spec/docs should explain what behavior clients can expect.
Maybe rejecting requests when another request is in flight would yield a simpler and more reliable impl?
Mike WassermanThe behavior of issuing multiple requests is very platform specific. As far as I remember, issuing multiple calls at the same time can cause the state changes to be merged/skipped. For example, when I tried minimizing and restoring on Wayland, only the first action was performed.
Additional problem is what happens if the user minimizes the window manually just before the `window.minimize()` was called. It'll result in a single minimization of the window. Should we resolve the promise in such case?
If we were to support sequences of window changes, _I think_ we would need to queue them on our own and issue the next request when the previous is finished. The problem here, as well as with rejecting promises, is what should happen if WM refuses to change the state. It can be fixed with a timeout though.
My idea was to guarantee only issuing request to the WM and resolving promises when the state is changed, but that would imply the developer shouldn't issue multiple requests at the same time. I resolve all of them simply because it's doable, and could be easier for developers if the user spammed one button because of WM lag. That solution might be too undefined for the web though, so I'm open to suggestions.
What do you think?
This is a thorny area for sure, perhaps we should start simpler by just rejecting simultaneous requests?
Ultimately, I think queueing/coalescing/dropping WM requests ourselves and keeping a FIFO queue of promise callbacks might better match dev expectations than resolving all pending requests of the same type as this CL currently does.
Ok. I implemented rejecting simultaneous requests.
if (old_show_state != window_show_state_) {Patryk ChodurThere's no clear request denial signal at this layer, right? If so, when the WM silently ignores a request, the JS promise will hang indefinitely. I wonder if the promises should reject with some timeout? (e.g. no window state change in 3s?). Again, we don't need a perfect impl, but having something well documented and reliable would be great.
Mike WassermanThat's correct, the promise won't be resolved. I was also thinking about a timeout. I plan to add it in a follow up CL.
Rejecting after a timeout seems like a good near-term followup. Letting promises hang indefinitely is not a good API contract.
Great! I'll add it in a follow up CL and I added a `TODO` just for now.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike Wasserman (Gerrit)
Mike Wasserman voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mike Wasserman
| Code-Review | +1 |
1 comment
Related details
- Nate Chapin
- Patryk Chodur
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patryk Chodur (Gerrit)
Patryk Chodur added 1 comment![]( "Open in Gerrit")
I had to disable 2 tests on Mac after running them manually with utr. There are other problems with window state transitions on Mac that I plan to tackle in https://crbug.com/458599317
Related details
- Nate Chapin
Andrew Rayskiy (Gerrit)
Andrew Rayskiy voted Code-Review+1![]( "Open in Gerrit")
- Nate Chapin
- Patryk Chodur
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Chapin (Gerrit)
Nate Chapin voted and added 2 comments![]( "Open in Gerrit")
Votes added by Nate Chapin
| Code-Review | +1 |
2 comments
ScriptState::Scope scope(resolver->GetScriptState());This isn't needed, `Reject` puts a `ScriptState::Scope` on the stack.
resolver->Reject(V8ThrowDOMException::CreateOrEmpty(Shorthand: `resolver->RejectWithDOMException(DOMExceptionCode::kNotAllowedError, error_message));`
Related details
- Patryk Chodur
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. |
Patryk Chodur (Gerrit)
Patryk Chodur voted and added 2 comments![]( "Open in Gerrit")
Votes added by Patryk Chodur
| Commit-Queue | +1 |
2 comments
This isn't needed, `Reject` puts a `ScriptState::Scope` on the stack.
I needed it for the `V8ThrowDOMException::CreateOrEmpty` ([this](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/bindings/script_state.h;l=166;drc=e187d025a32045c6249b739190382367dfc85985) `CHECK` failed), but with the `RejectWithDOMException` it works like charm!
Shorthand: `resolver->RejectWithDOMException(DOMExceptionCode::kNotAllowedError, error_message));`
Done. Thank you!
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patryk Chodur (Gerrit)
Patryk Chodur voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Patryk Chodur (Gerrit)
Patryk Chodur voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
20 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/modules/awc/additional_windowing_controls.cc
Insertions: 4, Deletions: 8.
The diff is too large to show. Please review the diff.
```
Change information
AWC window.maximize, minimize and restore promises implemented
This commit implements promises for window.maximize(), window.minimize()
and window.restore() promises. Previously they were resolved just after
the browser requested the change without waiting for it to happen. Now
the promise will get resolved when an appropriate state change happens.
If another promise is waiting for the state change, the current promise
will be rejected. There's a plan to implement FIFO queue for these in
the following CLs.
- M chrome/browser/ui/views/web_apps/frame_toolbar/web_app_frame_toolbar_browsertest.cc
- M third_party/blink/renderer/core/exported/web_view_impl.cc
- M third_party/blink/renderer/core/exported/web_view_impl.h
- M third_party/blink/renderer/core/frame/web_frame_widget_impl.cc
- M third_party/blink/renderer/core/loader/empty_clients.h
- M third_party/blink/renderer/core/page/chrome_client.h
- M third_party/blink/renderer/core/page/chrome_client_impl.cc
- M third_party/blink/renderer/core/page/chrome_client_impl.h
- M third_party/blink/renderer/modules/awc/additional_windowing_controls.cc
Code-Review: +1 by Nate Chapin, +1 by Mike Wasserman, +1 by Olga Korokhina, +1 by Andrew Rayskiy
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |