[iOS] Moves Tab Strip in BVCContainerViewController [chromium/src : main]
Ewann Pellé (Gerrit)
Ewann Pellé added 2 comments![]( "Open in Gerrit")
- (UIViewController*)currentBVC {
return [self.childViewControllers firstObject];
}
Should this be updated ?
CGFloat progress = 1.0;
if (_bvcContainerContent && _bvcContainerContent.fullscreenController) {
progress = _bvcContainerContent.fullscreenController->GetProgress();
}
CGFloat offset =
AlignValueToPixel((1.0 - progress) * _fullscreenViewportInsetRange);
_tabStripTopConstraint = [_tabStripViewController.view.topAnchor
constraintEqualToAnchor:self.view.safeAreaLayoutGuide.topAnchor
constant:-offset];Will probably be merged with fullscreen APIs.
Related details
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. |
Ewann Pellé (Gerrit)
Ewann Pellé voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ewann Pellé
| Commit-Queue | +1 |
2 comments
- (UIViewController*)currentBVC {
return [self.childViewControllers firstObject];
}
Should this be updated ?
Done
CGFloat progress = 1.0;
if (_bvcContainerContent && _bvcContainerContent.fullscreenController) {
progress = _bvcContainerContent.fullscreenController->GetProgress();
}
CGFloat offset =
AlignValueToPixel((1.0 - progress) * _fullscreenViewportInsetRange);
_tabStripTopConstraint = [_tabStripViewController.view.topAnchor
constraintEqualToAnchor:self.view.safeAreaLayoutGuide.topAnchor
constant:-offset];Will probably be merged with fullscreen APIs.
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ewann Pellé (Gerrit)
Ewann Pellé voted Commit-Queue+1![]( "Open in Gerrit")
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 6 comments![]( "Open in Gerrit")
BrowserViewController to BVCContainerViewController.This feels like a good first step, but I wonder if it is the "right" approach.
In particular, in case we want to contain it, the primary toolbar isn't touching the TabStrip. There is some kind of containment of the BVC.
How do you plan to do it?
Should the BVC be inset by the BVCCVC and thus not be displayed below the TabStrip?
I don't think this is a good idea as I think it wouldn't work very well with fullscreen for the sites. But I am interested in our reasoning here.
In particular, I was more thinking along the lines of the BVCCVC adding an "inset" to the BVC, rather than the BVC being aware of the condition to display the TabStrip. That would remove all TabStrip-related logic out of BVC and provide a better encapsulation of the responsibility.
CanShowTabStrip(self) ? TabStripCollectionViewConstants.height : 0.0;Oooooh, interesting. You are completely replacing the use of the TabStrip in the headerViews property by this constant?
[self addConstraintsToPrimaryToolbar];Why is this no longer necessary?
Where are you re-updating the constraints for the toolbar?
@property(nonatomic, strong, readonly) TabStripCoordinator* tabStripCoordinator;Do you still need it inside the BVC?
What is it used for?
FullscreenController* fullscreenController;You can get it from the Browser, I don't think you need it here?
Consider passing it in init.
maxViewportInsets:(UIEdgeInsets)maxViewportInsets {Related details
- Aliona Dangla
- Ewann Pellé
- Mark Cogan
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. |
Ewann Pellé (Gerrit)
Ewann Pellé voted and added 5 comments![]( "Open in Gerrit")
Votes added by Ewann Pellé
| Commit-Queue | +1 |
5 comments
CanShowTabStrip(self) ? TabStripCollectionViewConstants.height : 0.0;Oooooh, interesting. You are completely replacing the use of the TabStrip in the headerViews property by this constant?
Yes, or by using `primaryToolbarTopPadding`
[self addConstraintsToPrimaryToolbar];Why is this no longer necessary?
Where are you re-updating the constraints for the toolbar?
TabStrip is removed from the view hierarchy when not visible. PTAL at `setTabStripViewController:` impl.
@property(nonatomic, strong, readonly) TabStripCoordinator* tabStripCoordinator;Do you still need it inside the BVC?
What is it used for?
No but.. BrowserViewControllerDependencies hands it to BrowserViewController.
That's why this protocol is a "temporary" solution.
FullscreenController* fullscreenController;You can get it from the Browser, I don't think you need it here?
Consider passing it in init.
Currently, the BrowserCoordinator sets up a massive struct (BrowserViewControllerDependencies) and hands it to BrowserViewController.
I think, to inject these directly into BVCContainerViewController during init, we would need to refactor BrowserCoordinator's init sequence to untangle the them before the BVC is created.
maxViewportInsets:(UIEdgeInsets)maxViewportInsets {To calculate the total vertical distance the top UI needs to travel to disappear off the top of the screen. See its usage in `updateForFullscreenProgress`.
Related details
- Aliona Dangla
- Gauthier Ambard
- Mark Cogan
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")
maxViewportInsets:(UIEdgeInsets)maxViewportInsets {`min` and `max` are used to compute `lvh` and `svh` css values correctly, it is currently implemented only when smooth scrolling is enabled
Related details
- Gauthier Ambard
- Mark Cogan
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. |
Ewann Pellé (Gerrit)
Ewann Pellé added 1 comment![]( "Open in Gerrit")
BrowserViewController to BVCContainerViewController.This feels like a good first step, but I wonder if it is the "right" approach.
In particular, in case we want to contain it, the primary toolbar isn't touching the TabStrip. There is some kind of containment of the BVC.
How do you plan to do it?Should the BVC be inset by the BVCCVC and thus not be displayed below the TabStrip?
I don't think this is a good idea as I think it wouldn't work very well with fullscreen for the sites. But I am interested in our reasoning here.In particular, I was more thinking along the lines of the BVCCVC adding an "inset" to the BVC, rather than the BVC being aware of the condition to display the TabStrip. That would remove all TabStrip-related logic out of BVC and provide a better encapsulation of the responsibility.
I think deleting tab strip refs from BVC should indeed be done.
Do you know if the tab strip VC will be impacted with fullscreen animations when the assistant sheet will be displayed ?
We can also have an "hybrid" approach, the BVCContainerViewController should observe the tab strip height (might be static when assistant sheet is visible) and push updates to the BVC. This data will contain the size of the strip + additional insets. This will create a one way data flow.
Ewann Pellé (Gerrit)
Ewann Pellé added 1 comment![]( "Open in Gerrit")
BrowserViewController to BVCContainerViewController.Ewann PelléThis feels like a good first step, but I wonder if it is the "right" approach.
In particular, in case we want to contain it, the primary toolbar isn't touching the TabStrip. There is some kind of containment of the BVC.
How do you plan to do it?Should the BVC be inset by the BVCCVC and thus not be displayed below the TabStrip?
I don't think this is a good idea as I think it wouldn't work very well with fullscreen for the sites. But I am interested in our reasoning here.In particular, I was more thinking along the lines of the BVCCVC adding an "inset" to the BVC, rather than the BVC being aware of the condition to display the TabStrip. That would remove all TabStrip-related logic out of BVC and provide a better encapsulation of the responsibility.
I think deleting tab strip refs from BVC should indeed be done.
Do you know if the tab strip VC will be impacted with fullscreen animations when the assistant sheet will be displayed ?
We can also have an "hybrid" approach, the BVCContainerViewController should observe the tab strip height (might be static when assistant sheet is visible) and push updates to the BVC. This data will contain the size of the strip + additional insets. This will create a one way data flow.
To make it clearer it will be an inset as suggested, but the BVCCVC will still manage tab strip logic.
Ewann Pellé (Gerrit)
Ewann Pellé added 1 comment![]( "Open in Gerrit")
- (void)updateStatusBarBackgroundViews {staticStatusBarView and fadingStatusBarView don't need to be anchored to the tab strip, the bottom anchor could be `self.safeArea.top` instead. staticStatusBarView should always be visible, fadingStatusBarView should be visible only if tab strip is visible. Will update this flow.
Related details
- Gauthier Ambard
- Mark Cogan
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. |
Ewann Pellé (Gerrit)
Ewann Pellé added 1 comment![]( "Open in Gerrit")
- (void)updateStatusBarBackgroundViews {staticStatusBarView and fadingStatusBarView don't need to be anchored to the tab strip, the bottom anchor could be `self.safeArea.top` instead. staticStatusBarView should always be visible, fadingStatusBarView should be visible only if tab strip is visible. Will update this flow.
Done
Mark Cogan (Gerrit)
Mark Cogan added 5 comments![]( "Open in Gerrit")
BrowserViewController to BVCContainerViewController.Ewann PelléThis feels like a good first step, but I wonder if it is the "right" approach.
In particular, in case we want to contain it, the primary toolbar isn't touching the TabStrip. There is some kind of containment of the BVC.
How do you plan to do it?Should the BVC be inset by the BVCCVC and thus not be displayed below the TabStrip?
I don't think this is a good idea as I think it wouldn't work very well with fullscreen for the sites. But I am interested in our reasoning here.In particular, I was more thinking along the lines of the BVCCVC adding an "inset" to the BVC, rather than the BVC being aware of the condition to display the TabStrip. That would remove all TabStrip-related logic out of BVC and provide a better encapsulation of the responsibility.
Ewann PelléI think deleting tab strip refs from BVC should indeed be done.
Do you know if the tab strip VC will be impacted with fullscreen animations when the assistant sheet will be displayed ?
We can also have an "hybrid" approach, the BVCContainerViewController should observe the tab strip height (might be static when assistant sheet is visible) and push updates to the BVC. This data will contain the size of the strip + additional insets. This will create a one way data flow.
To make it clearer it will be an inset as suggested, but the BVCCVC will still manage tab strip logic.
+1 to making BVC aware of less things.
@protocol BVCContainerContent <NSObject>I can maybe tolerate this name as it's temporary, but I really don't like directly referencing the (abbreviated) name of a view controller in other APIs. I guess here it's connected to BVCCVC, whose name I also don't like.
For protocols that temporarily expose details that will soon be refactored, we've used the name "Plumbing" in the past. BVCContainerPlumbing?
@property(nonatomic, strong, readonly) TabStripCoordinator* tabStripCoordinator;Ewann PelléDo you still need it inside the BVC?
What is it used for?
No but.. BrowserViewControllerDependencies hands it to BrowserViewController.
That's why this protocol is a "temporary" solution.
Obviously nothing in the UI layer should be depending on it.
FullscreenController* fullscreenController;Ewann PelléYou can get it from the Browser, I don't think you need it here?
Consider passing it in init.
Currently, the BrowserCoordinator sets up a massive struct (BrowserViewControllerDependencies) and hands it to BrowserViewController.
I think, to inject these directly into BVCContainerViewController during init, we would need to refactor BrowserCoordinator's init sequence to untangle the them before the BVC is created.
Not that the intent is that ongoing refactoring removes dependencies from BrowserViewControllerDependencies.
@interface BVCContainerViewController () <FullscreenUIElement>Consider finding a better name for this class.
Related details
- Ewann Pellé
- 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. |
Ewann Pellé (Gerrit)
Ewann Pellé added 4 comments![]( "Open in Gerrit")
BrowserViewController to BVCContainerViewController.Ewann PelléThis feels like a good first step, but I wonder if it is the "right" approach.
In particular, in case we want to contain it, the primary toolbar isn't touching the TabStrip. There is some kind of containment of the BVC.
How do you plan to do it?Should the BVC be inset by the BVCCVC and thus not be displayed below the TabStrip?
I don't think this is a good idea as I think it wouldn't work very well with fullscreen for the sites. But I am interested in our reasoning here.In particular, I was more thinking along the lines of the BVCCVC adding an "inset" to the BVC, rather than the BVC being aware of the condition to display the TabStrip. That would remove all TabStrip-related logic out of BVC and provide a better encapsulation of the responsibility.
Ewann PelléI think deleting tab strip refs from BVC should indeed be done.
Do you know if the tab strip VC will be impacted with fullscreen animations when the assistant sheet will be displayed ?
We can also have an "hybrid" approach, the BVCContainerViewController should observe the tab strip height (might be static when assistant sheet is visible) and push updates to the BVC. This data will contain the size of the strip + additional insets. This will create a one way data flow.
Mark CoganTo make it clearer it will be an inset as suggested, but the BVCCVC will still manage tab strip logic.
+1 to making BVC aware of less things.
Would it be fine if it's done in the following CL ?
@protocol BVCContainerContent <NSObject>I can maybe tolerate this name as it's temporary, but I really don't like directly referencing the (abbreviated) name of a view controller in other APIs. I guess here it's connected to BVCCVC, whose name I also don't like.
For protocols that temporarily expose details that will soon be refactored, we've used the name "Plumbing" in the past. BVCContainerPlumbing?
I'm fine renaming it to BVCContainerPlumbing, it will be removed in the following CL.
@property(nonatomic, strong, readonly) TabStripCoordinator* tabStripCoordinator;Ewann PelléDo you still need it inside the BVC?
What is it used for?
Mark CoganNo but.. BrowserViewControllerDependencies hands it to BrowserViewController.
That's why this protocol is a "temporary" solution.
Obviously nothing in the UI layer should be depending on it.
This protocol will be cleaned in the following CL.
@interface BVCContainerViewController () <FullscreenUIElement>Consider finding a better name for this class.
Will be renamed to BrowserLayoutViewController in a follow up.
Related details
- Gauthier Ambard
- Mark Cogan
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 3 comments![]( "Open in Gerrit")
@property(nonatomic, strong, readonly) TabStripCoordinator* tabStripCoordinator;Ewann PelléDo you still need it inside the BVC?
What is it used for?
Mark CoganNo but.. BrowserViewControllerDependencies hands it to BrowserViewController.
That's why this protocol is a "temporary" solution.
Ewann PelléObviously nothing in the UI layer should be depending on it.
This protocol will be cleaned in the following CL.
Can't you just remove it from the dependencies?
My question is: what is it used for? Is it necessary to have it in the dependencies?
FullscreenController* fullscreenController;Ewann PelléYou can get it from the Browser, I don't think you need it here?
Consider passing it in init.
Mark CoganCurrently, the BrowserCoordinator sets up a massive struct (BrowserViewControllerDependencies) and hands it to BrowserViewController.
I think, to inject these directly into BVCContainerViewController during init, we would need to refactor BrowserCoordinator's init sequence to untangle the them before the BVC is created.
Not that the intent is that ongoing refactoring removes dependencies from BrowserViewControllerDependencies.
I think we can pass it to the constructor. I know that the BVCdependency is there, but you want to remove this protocol long term, so why not doing it now?
maxViewportInsets:(UIEdgeInsets)maxViewportInsets {Aliona Dangla@aliona...@chromium.org what is this method used for?
`min` and `max` are used to compute `lvh` and `svh` css values correctly, it is currently implemented only when smooth scrolling is enabled
So do we need to update the `_fullscreenViewportInsetRange` here?
Related details
- Ewann Pellé
- Mark Cogan
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. |
Mark Cogan (Gerrit)
Mark Cogan voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mark Cogan
| Code-Review | +1 |
3 comments
BrowserViewController to BVCContainerViewController.Ewann PelléThis feels like a good first step, but I wonder if it is the "right" approach.
In particular, in case we want to contain it, the primary toolbar isn't touching the TabStrip. There is some kind of containment of the BVC.
How do you plan to do it?Should the BVC be inset by the BVCCVC and thus not be displayed below the TabStrip?
I don't think this is a good idea as I think it wouldn't work very well with fullscreen for the sites. But I am interested in our reasoning here.In particular, I was more thinking along the lines of the BVCCVC adding an "inset" to the BVC, rather than the BVC being aware of the condition to display the TabStrip. That would remove all TabStrip-related logic out of BVC and provide a better encapsulation of the responsibility.
Ewann PelléI think deleting tab strip refs from BVC should indeed be done.
Do you know if the tab strip VC will be impacted with fullscreen animations when the assistant sheet will be displayed ?
We can also have an "hybrid" approach, the BVCContainerViewController should observe the tab strip height (might be static when assistant sheet is visible) and push updates to the BVC. This data will contain the size of the strip + additional insets. This will create a one way data flow.
Mark CoganTo make it clearer it will be an inset as suggested, but the BVCCVC will still manage tab strip logic.
Ewann Pellé+1 to making BVC aware of less things.
Would it be fine if it's done in the following CL ?
Yes, I'd expect a big refactor like this to be incremental.
@protocol BVCContainerContent <NSObject>Ewann PelléI can maybe tolerate this name as it's temporary, but I really don't like directly referencing the (abbreviated) name of a view controller in other APIs. I guess here it's connected to BVCCVC, whose name I also don't like.
For protocols that temporarily expose details that will soon be refactored, we've used the name "Plumbing" in the past. BVCContainerPlumbing?
I'm fine renaming it to BVCContainerPlumbing, it will be removed in the following CL.
No need to rename if it's that temporary.
Related details
- Ewann Pellé
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. |
Ewann Pellé (Gerrit)
Ewann Pellé added 2 comments![]( "Open in Gerrit")
@property(nonatomic, strong, readonly) TabStripCoordinator* tabStripCoordinator;Ewann PelléDo you still need it inside the BVC?
What is it used for?
Mark CoganNo but.. BrowserViewControllerDependencies hands it to BrowserViewController.
That's why this protocol is a "temporary" solution.
Ewann PelléObviously nothing in the UI layer should be depending on it.
Gauthier AmbardThis protocol will be cleaned in the following CL.
Can't you just remove it from the dependencies?
My question is: what is it used for? Is it necessary to have it in the dependencies?
Will be improved in the following CL.
FullscreenController* fullscreenController;Ewann PelléYou can get it from the Browser, I don't think you need it here?
Consider passing it in init.
Mark CoganCurrently, the BrowserCoordinator sets up a massive struct (BrowserViewControllerDependencies) and hands it to BrowserViewController.
I think, to inject these directly into BVCContainerViewController during init, we would need to refactor BrowserCoordinator's init sequence to untangle the them before the BVC is created.
Gauthier AmbardNot that the intent is that ongoing refactoring removes dependencies from BrowserViewControllerDependencies.
I think we can pass it to the constructor. I know that the BVCdependency is there, but you want to remove this protocol long term, so why not doing it now?
- 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 voted Code-Review+1![]( "Open in Gerrit")
- Ewann Pellé
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. |