[AWC] window.maximize, minimize and restore promises implemented [chromium/src : main]
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 3 comments![]( "Open in Gerrit")
Vector<MinimizeCallback> callbacks(std::move(minimize_callbacks_));std::exchange?
if (window_show_state_ == ui::mojom::blink::WindowShowState::kMaximized) {switch (...)
if (window->GetFrame()->GetWidgetForLocalRoot()->WindowShowState() ==(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?
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 added 3 comments![]( "Open in Gerrit")
if (window_show_state_ == ui::mojom::blink::WindowShowState::kMaximized) {switch (...)
+1
if (window->GetFrame()->GetWidgetForLocalRoot()->WindowShowState() ==(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?
+1, as long as there are more than 1 inclusions of this check this seems to be reasonable.
"Could not restore the window."));Can 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 ?
Related details
- Patryk Chodur
Patryk Chodur (Gerrit)
Patryk Chodur added 4 comments![]( "Open in Gerrit")
Vector<MinimizeCallback> callbacks(std::move(minimize_callbacks_));Patryk Chodurstd::exchange?
Done
if (window_show_state_ == ui::mojom::blink::WindowShowState::kMaximized) {Olga Korokhinaswitch (...)
+1
Done
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?
+1, as long as there are more than 1 inclusions of this check this seems to be reasonable.
Added 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.
"Could not restore the window."));Can 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 ?
As 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.
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. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 3 comments![]( "Open in Gerrit")
Vector<MinimizeCallback> minimize_callbacks_;Maybe use `base/callback_list.h` here? I'm not sure what the guidance here is.
std::move(callback).Run(/*minimized=*/false);You'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?
[](ScriptPromiseResolver<IDLUndefined>* resolver, bool maximized) {this looks like something that could be shared between the three functions as the only thing that differs is the error param.
Related details
- Olga Korokhina
- Patryk Chodur
Patryk Chodur (Gerrit)
Patryk Chodur added 3 comments![]( "Open in Gerrit")
Vector<MinimizeCallback> minimize_callbacks_;Maybe use `base/callback_list.h` here? I'm not sure what the guidance here is.
I 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.
std::move(callback).Run(/*minimized=*/false);You'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?
After 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.
[](ScriptPromiseResolver<IDLUndefined>* resolver, bool maximized) {this looks like something that could be shared between the three functions as the only thing that differs is the error param.
- 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. |