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