[ios] Support forced fullscreen in FullscreenRefactoring [chromium/src : main]
Scott Yoder (Gerrit)
Scott Yoder voted and added 1 comment![]( "Open in Gerrit")
Votes added by Scott Yoder
| Commit-Queue | +1 |
1 comment
Please take a look. I could have stored a count like FullscreenModel (force_fullscreen_mode_counter_), but then I would have also had to store whatever the last trigger was. I felt like conceptually a stack is easier to understand, but can switch it back if that is preferred.
Also, if you are interested, I had Jetski document the use cases where fullscreen is entered: https://crbug.com/500669310#comment4
My open questions are: What if we try to force fullscreen when fullscreen is disabled? What if we try to disable fullscreen when fullscreen is forced?
Related details
- Aliona Dangla
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gauthier Ambard (Gerrit)
Gauthier Ambard added 1 comment![]( "Open in Gerrit")
if (trigger == FullscreenModeTransitionTrigger::kNavigation) {I don't understand this logic.
It is worth adding a comment at least
Related details
- Aliona Dangla
- Scott Yoder
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. |
Aliona Dangla (Gerrit)
Aliona Dangla added 1 comment![]( "Open in Gerrit")
enterFullscreenWithTrigger:FullscreenModeTransitionTrigger::
kForcedByCodeuse `trigger`
Related details
- Scott Yoder
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. |
Scott Yoder (Gerrit)
Scott Yoder added 2 comments![]( "Open in Gerrit")
enterFullscreenWithTrigger:FullscreenModeTransitionTrigger::
kForcedByCodeScott Yoderuse `trigger`
Done
if (trigger == FullscreenModeTransitionTrigger::kNavigation) {I don't understand this logic.
It is worth adding a comment at least
I've added a comment. The existing code (in FullscreenModel::ResetForNavigation), exits fullscreen unless it was forced by the user. This logic attempts to do the same thing.
However, I think there is a bug in that FullscreenModel method when Fullscreen is forced but not by the user - it decrements force_fullscreen_mode_counter_ and then exits fullscreen by setting the progress to 1. But what if the count is still greater than zero? Should it reset the count to zero? Here, it pops the stack to empty it unless it encounters a kForcedByUser. I think this is what it should do, but happy to discuss it.
Related details
- Aliona Dangla
- Gauthier Ambard
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. |
Gauthier Ambard (Gerrit)
Gauthier Ambard added 2 comments![]( "Open in Gerrit")
if (trigger == FullscreenModeTransitionTrigger::kNavigation) {Scott YoderI don't understand this logic.
It is worth adding a comment at least
I've added a comment. The existing code (in FullscreenModel::ResetForNavigation), exits fullscreen unless it was forced by the user. This logic attempts to do the same thing.
However, I think there is a bug in that FullscreenModel method when Fullscreen is forced but not by the user - it decrements force_fullscreen_mode_counter_ and then exits fullscreen by setting the progress to 1. But what if the count is still greater than zero? Should it reset the count to zero? Here, it pops the stack to empty it unless it encounters a kForcedByUser. I think this is what it should do, but happy to discuss it.
Isn't exiting fullscreen on navigation a security requirement?
Can you give a step breakdown of when this could happen?
// Reported when exiting fullscreen mode by tapping on the toolbar.This only mentions "exiting" fullscreen, which is why the code is a bit weird. Is it also used when entering fullscreen?
In which case the user is force-entering fullscreen and we won't exit on navigations?
Related details
- Aliona Dangla
- Scott Yoder
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. |
Scott Yoder (Gerrit)
Scott Yoder added 2 comments![]( "Open in Gerrit")
if (trigger == FullscreenModeTransitionTrigger::kNavigation) {Scott YoderI don't understand this logic.
It is worth adding a comment at least
Gauthier AmbardI've added a comment. The existing code (in FullscreenModel::ResetForNavigation), exits fullscreen unless it was forced by the user. This logic attempts to do the same thing.
However, I think there is a bug in that FullscreenModel method when Fullscreen is forced but not by the user - it decrements force_fullscreen_mode_counter_ and then exits fullscreen by setting the progress to 1. But what if the count is still greater than zero? Should it reset the count to zero? Here, it pops the stack to empty it unless it encounters a kForcedByUser. I think this is what it should do, but happy to discuss it.
Isn't exiting fullscreen on navigation a security requirement?
Can you give a step breakdown of when this could happen?
The only place that calls EnterFullscreen with a trigger of `kForcedByUser` is the new Overflow Menu item "Hide Toolbars" that manually enters fullscreen. This is a feature that @aliona...@chromium.org just finished. This change carries over that functionality where it does not exit fullscreen except by tapping the toolbar.
There really are not that many places that programmatically enter fullscreen - I had jetski document them here: http://crbug.com/500669310#comment4
// Reported when exiting fullscreen mode by tapping on the toolbar.This only mentions "exiting" fullscreen, which is why the code is a bit weird. Is it also used when entering fullscreen?
In which case the user is force-entering fullscreen and we won't exit on navigations?
I've updated the comment here. The new "Hide Toolbars" item in the Overflow Menu uses `kForcedByUser` when entering fullscreen.
Related details
- Aliona Dangla
- Gauthier Ambard
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. |
Aliona Dangla (Gerrit)
Aliona Dangla added 1 comment![]( "Open in Gerrit")
if (trigger == FullscreenModeTransitionTrigger::kNavigation) {Scott YoderI don't understand this logic.
It is worth adding a comment at least
Gauthier AmbardI've added a comment. The existing code (in FullscreenModel::ResetForNavigation), exits fullscreen unless it was forced by the user. This logic attempts to do the same thing.
However, I think there is a bug in that FullscreenModel method when Fullscreen is forced but not by the user - it decrements force_fullscreen_mode_counter_ and then exits fullscreen by setting the progress to 1. But what if the count is still greater than zero? Should it reset the count to zero? Here, it pops the stack to empty it unless it encounters a kForcedByUser. I think this is what it should do, but happy to discuss it.
Scott YoderIsn't exiting fullscreen on navigation a security requirement?
Can you give a step breakdown of when this could happen?
The only place that calls EnterFullscreen with a trigger of `kForcedByUser` is the new Overflow Menu item "Hide Toolbars" that manually enters fullscreen. This is a feature that @aliona...@chromium.org just finished. This change carries over that functionality where it does not exit fullscreen except by tapping the toolbar.
There really are not that many places that programmatically enter fullscreen - I had jetski document them here: http://crbug.com/500669310#comment4
Note for "Hide Toolbars" you can exit that mode by tap on the collapsed toolbar but also by quit/relaunch the app ; background/foreground the app ; When tapping in a link ; Going back or forward ; See https://b.corp.google.com/issues/444174990/dependencies
Related details
- Gauthier Ambard
- Scott Yoder
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. |
Scott Yoder (Gerrit)
Scott Yoder voted and added 1 comment![]( "Open in Gerrit")
Votes added by Scott Yoder
| Commit-Queue | +1 |
1 comment
if (trigger == FullscreenModeTransitionTrigger::kNavigation) {Scott YoderI don't understand this logic.
It is worth adding a comment at least
Gauthier AmbardI've added a comment. The existing code (in FullscreenModel::ResetForNavigation), exits fullscreen unless it was forced by the user. This logic attempts to do the same thing.
However, I think there is a bug in that FullscreenModel method when Fullscreen is forced but not by the user - it decrements force_fullscreen_mode_counter_ and then exits fullscreen by setting the progress to 1. But what if the count is still greater than zero? Should it reset the count to zero? Here, it pops the stack to empty it unless it encounters a kForcedByUser. I think this is what it should do, but happy to discuss it.
Scott YoderIsn't exiting fullscreen on navigation a security requirement?
Can you give a step breakdown of when this could happen?
Aliona DanglaThe only place that calls EnterFullscreen with a trigger of `kForcedByUser` is the new Overflow Menu item "Hide Toolbars" that manually enters fullscreen. This is a feature that @aliona...@chromium.org just finished. This change carries over that functionality where it does not exit fullscreen except by tapping the toolbar.
There really are not that many places that programmatically enter fullscreen - I had jetski document them here: http://crbug.com/500669310#comment4
Note for "Hide Toolbars" you can exit that mode by tap on the collapsed toolbar but also by quit/relaunch the app ; background/foreground the app ; When tapping in a link ; Going back or forward ; See https://b.corp.google.com/issues/444174990/dependencies
I had the logic backwards for navigation with "Hide Toolbars" - it should always exit Fullscreen on a navigation. In the legacy implementation, it exits if forced by user (i.e. Hide Toolbars), but otherwise does not exit fullscreen. After chatting with Aliona, we've agreed that it should always exit on a navigation regardless of the trigger, so I've updated the CL to implement this for the new implementation.
Related details
- Aliona Dangla
- Gauthier Ambard
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. |
Aliona Dangla (Gerrit)
Aliona Dangla added 3 comments![]( "Open in Gerrit")
std::stack<FullscreenModeTransitionTrigger> forced_triggers_;I am not sure we want to keep this stack. Because it does not verify that the trigger being exited actually matches the one at the top of the stack. If multiple features overlap (which should be currently impossible now but we never know), one feature could accidentally pop the other's state, leading to an inconsistent UI state where fullscreen is exited prematurely or incorrectly maintained.
// Reported when exiting fullscreen mode by tapping on the toolbar, or whenor when ENTERING fullscreen mode via a tap on the "Hide Toolbars" item in the overflow menu
Related details
- Gauthier Ambard
- Scott Yoder
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. |
Scott Yoder (Gerrit)
Scott Yoder voted and added 3 comments![]( "Open in Gerrit")
Votes added by Scott Yoder
| Commit-Queue | +1 |
3 comments
std::stack<FullscreenModeTransitionTrigger> forced_triggers_;I am not sure we want to keep this stack. Because it does not verify that the trigger being exited actually matches the one at the top of the stack. If multiple features overlap (which should be currently impossible now but we never know), one feature could accidentally pop the other's state, leading to an inconsistent UI state where fullscreen is exited prematurely or incorrectly maintained.
I think this is fair criticism - the stack isn't really behaving like a stack normally would and isn't adding a lot of value here. I've switched it to using a count, like the implementation in FullscreenModel.
// Reported when exiting fullscreen mode by tapping on the toolbar, or whenor when ENTERING fullscreen mode via a tap on the "Hide Toolbars" item in the overflow menu
- Aliona Dangla
- Gauthier Ambard
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. |
Aliona Dangla (Gerrit)
Aliona Dangla added 2 comments![]( "Open in Gerrit")
void FullscreenBrowserAgent::ExitFullscreen(
PassKey pass_key,
FullscreenModeTransitionTrigger trigger,
bool animated) {
switch (trigger) {
case FullscreenModeTransitionTrigger::kNavigation:
// On a navigation, fullscreen is always exited.
forced_count_ = 0;
break;
default:
if (forced_count_ > 0) {
forced_count_--;
if (forced_count_ > 0) {
return;
}
}
break;
}With this approach as enter/exit fullscreen is now the same as the forced one, how can we check that we do not call exit too many times for example
I am expecting that if your forced_count is already == 0 and you force exit, we should hit a CHECK
kForcedByCode = 1,Should we add `kForcedByFindInPage` and `kForcedByLens` (that enter and exit and we can check that we entered at some point with X or Y feture) so we now where it is coming from and we are 100% that these 2 want a forced mode? to make a difference with a regular exit and a forced one? WDYT
Related details
- Gauthier Ambard
- Scott Yoder
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. |