[WebUI LLBC] Introduce PermissionChip Interfaces. [chromium/src : main]
Qingxin Wu (Gerrit)
Qingxin Wu voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Elias Klim
- Paul Jensen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maks Orlovich (Gerrit)
Maks Orlovich added 7 comments![]( "Open in Gerrit")
if (views::View* chip_view = chip_controller_->chip()->GetAsView()) {Can probably CHECK this, you won't expect the WebUI chips impl with LocationBarView?
virtual bool IsMouseHovered() const = 0;This is going to be approximate with WebUI.
virtual bool is_fully_collapsed() const = 0;Probably shouldn't use names like this for virtuals.
virtual void StopAnimationForTesting() = 0;Can one do that w/CSS?
virtual void ResetAnimation(double value) = 0;Is this implementable with CSS? I think it's only ever used with 0 or 1, though, which probably simplifies things (and suggests this should take an enum or a bool or such instead)
virtual void AnimateCollapse(base::TimeDelta duration) = 0;Are you sure you need to be passing durations? These go into CSS, so it would be easier if they're just constants in CSS files. Looking around, there seems to be some sort of guard on whether the animations are on or not. I wonder if we should be pushing that bool as yet another CSS var?
class BubbleOwnerDelegate {This is way too generic a name to not be namespaced (perhaps as a member type?)
Related details
- Elias Klim
- Paul Jensen
- Qingxin Wu
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. |
Elias Klim (Gerrit)
Elias Klim voted and added 3 comments![]( "Open in Gerrit")
Votes added by Elias Klim
| Code-Review | +1 |
3 comments
LGTM % comments.
(I will be OOO for the whole week, CC'ed andypaicu@ for further questions if needed)
if (quiet_on_event == QuitOnEvent::kExpand) {Please rename to `quit_on_event`
permission_dashboard_view_->GetViewAccessibility().AnnounceAlert(In `ChipController`, you delegated the announcement to the chip via `chip_->AnnounceText(text)`.
Here, the name is set on the chip, but the alert is still announced on the parent `permission_dashboard_view_`. To be consistent and ensure correct screen reader behavior as we decouple, should this also be delegated to the chip?
Related details
- Paul Jensen
- Qingxin Wu
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. |
Qingxin Wu (Gerrit)
Qingxin Wu added 9 comments![]( "Open in Gerrit")
if (views::View* chip_view = chip_controller_->chip()->GetAsView()) {Can probably CHECK this, you won't expect the WebUI chips impl with LocationBarView?
Done
if (quiet_on_event == QuitOnEvent::kExpand) {Qingxin WuPlease rename to `quit_on_event`
Done
This is going to be approximate with WebUI.
thanks for pointing that out. Added a comment for it.
Probably shouldn't use names like this for virtuals.
changed to IsFullyCollapsed and IsAnimating.
Can one do that w/CSS?
removed it, and changed tests to rely on PermissionChipView::StopAnimationForTesting() instead.
Is this implementable with CSS? I think it's only ever used with 0 or 1, though, which probably simplifies things (and suggests this should take an enum or a bool or such instead)
changed to an enum AnimationState
virtual void AnimateCollapse(base::TimeDelta duration) = 0;Are you sure you need to be passing durations? These go into CSS, so it would be easier if they're just constants in CSS files. Looking around, there seems to be some sort of guard on whether the animations are on or not. I wonder if we should be pushing that bool as yet another CSS var?
Great point! I removed the duration parameter from the interface entirely and move the constants (e.g. 350ms) down into the native PermissionChipView implementation.
This is way too generic a name to not be namespaced (perhaps as a member type?)
moved the definition inside PermissionChipInterface
permission_dashboard_view_->GetViewAccessibility().AnnounceAlert(In `ChipController`, you delegated the announcement to the chip via `chip_->AnnounceText(text)`.
Here, the name is set on the chip, but the alert is still announced on the parent `permission_dashboard_view_`. To be consistent and ensure correct screen reader behavior as we decouple, should this also be delegated to the chip?
- Maks Orlovich
- Paul Jensen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paul Jensen (Gerrit)
Paul Jensen added 2 comments![]( "Open in Gerrit")
virtual void AnimateCollapse(base::TimeDelta duration) = 0;Qingxin WuAre you sure you need to be passing durations? These go into CSS, so it would be easier if they're just constants in CSS files. Looking around, there seems to be some sort of guard on whether the animations are on or not. I wonder if we should be pushing that bool as yet another CSS var?
Great point! I removed the duration parameter from the interface entirely and move the constants (e.g. 350ms) down into the native PermissionChipView implementation.
So the collapse time is sometimes 250ms and sometimes 75ms, so this change is making it always 250ms. Doesn't seem like a huge change to me but I'm not an owner.
permission_dashboard_view_->UpdateDividerViewVisibility();This looks roughly correct, but have you looked through it carefully to make sure this isn't a functional change?
Related details
- Maks Orlovich
- Qingxin Wu
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. |
Qingxin Wu (Gerrit)
Qingxin Wu added 1 comment![]( "Open in Gerrit")
virtual void AnimateCollapse(base::TimeDelta duration) = 0;Qingxin WuAre you sure you need to be passing durations? These go into CSS, so it would be easier if they're just constants in CSS files. Looking around, there seems to be some sort of guard on whether the animations are on or not. I wonder if we should be pushing that bool as yet another CSS var?
Paul JensenGreat point! I removed the duration parameter from the interface entirely and move the constants (e.g. 350ms) down into the native PermissionChipView implementation.
So the collapse time is sometimes 250ms and sometimes 75ms, so this change is making it always 250ms. Doesn't seem like a huge change to me but I'm not an owner.
@andy...@chromium.org For PermissionChipView's AnimateCollapse() method, it was passed in a 75ms duration from CollapseConfirmation(), and 250ms in other places. Are we fine to change the 75ms duration case to the same 250ms, or we have to keep the values as they are?
Related details
- Andy Paicu
- Maks Orlovich
- Paul Jensen
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. |
Maks Orlovich (Gerrit)
Maks Orlovich added 2 comments![]( "Open in Gerrit")
LocationBarView::GetChipAnchor() {Might be a good time to put the same impl in WebUILocationBar?
// after being closed via losing focus....Is this comment actually accurate?
Related details
- Andy Paicu
- Paul Jensen
- Qingxin Wu
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. |
Qingxin Wu (Gerrit)
Qingxin Wu added 1 comment![]( "Open in Gerrit")
permission_dashboard_view_->UpdateDividerViewVisibility();This looks roughly correct, but have you looked through it carefully to make sure this isn't a functional change?
Patch 12 changed to not let the controller directly micromanage the divider's visibility (which also needed a UpdateForDividerVisibility in chip interface that we don't want), but instead let permission_dashboard_view handle it (which will be a dashboard interface in the next CL).
We set indicatorchip visible to false two lines above. So, the dashboard update method becomes:
```
const int arc_width = ...;
secondary_chip_->UpdateForDividerVisibility(is_visible, arc_width);
chip_divider_view_->SetVisible(is_visible);
```
(1) In the original code `...GetRequestChip()->UpdateForDividerVisibility(false)`, the controller didn't pass the arc_width parameter, to use the default value 0. Although the arc_width will be ignored in UpdateForDividerVisibility in our case so it's effectively 0, but it does look better to directly pass 0. Updated this.
(2) The original code called `permission_dashboard_view_->GetDividerView()->SetVisible(false)`, now we call `chip_divider_view_->SetVisible(is_visible)` in UpdateDividerViewVisibility.
Qingxin Wu (Gerrit)
Qingxin Wu added 1 comment![]( "Open in Gerrit")
permission_dashboard_view_->UpdateDividerViewVisibility();Qingxin WuThis looks roughly correct, but have you looked through it carefully to make sure this isn't a functional change?
Patch 12 changed to not let the controller directly micromanage the divider's visibility (which also needed a UpdateForDividerVisibility in chip interface that we don't want), but instead let permission_dashboard_view handle it (which will be a dashboard interface in the next CL).
We set indicatorchip visible to false two lines above. So, the dashboard update method becomes:
```
const int arc_width = ...;
secondary_chip_->UpdateForDividerVisibility(is_visible, arc_width);
chip_divider_view_->SetVisible(is_visible);
```
(1) In the original code `...GetRequestChip()->UpdateForDividerVisibility(false)`, the controller didn't pass the arc_width parameter, to use the default value 0. Although the arc_width will be ignored in UpdateForDividerVisibility in our case so it's effectively 0, but it does look better to directly pass 0. Updated this.
(2) The original code called `permission_dashboard_view_->GetDividerView()->SetVisible(false)`, now we call `chip_divider_view_->SetVisible(is_visible)` in UpdateDividerViewVisibility.
(3) One difference is the original code only calls `GetRequestChip()->UpdateForDividerVisibility` when request chip is visible, but now we're calling it unconditionally same as what ChipController::HideChip does (another calller of UpdateDividerViewVisibility()). It causes we call secondary_chip_->UpdateForDividerVisibility when chip is not visible. My understanding is it does not change anything visible. @andy...@chromium.org Do you know if there was any reason of the original code, and do you see problem in the change here?
Related details
- Andy Paicu
- Paul Jensen
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. |
Qingxin Wu (Gerrit)
Qingxin Wu added 2 comments![]( "Open in Gerrit")
Might be a good time to put the same impl in WebUILocationBar?
thanks for the reminder. Done.
// after being closed via losing focus....Is this comment actually accurate?
this was just copied over. https://crsrc.org/c/chrome/browser/ui/views/permissions/chip/chip_controller.h;drc=405f385dce2db578ff3b2301686d231ee8f0b042;l=33
Rewrote it.
Related details
- Andy Paicu
- Maks Orlovich
- Paul Jensen
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. |
Qingxin Wu (Gerrit)
Qingxin Wu added 1 comment![]( "Open in Gerrit")
chip_visibility_subscription_ = chip->AddChipVisibilityCallback(@paulj...@chromium.org I added AddChipVisibilityCallback to the chip interface, instead of GetAsView() previously which would fail for webui. PTAL.
Qingxin Wu (Gerrit)
Qingxin Wu added 1 comment![]( "Open in Gerrit")
@tl...@chromium.org Mind reviewing chrome/browser/ui/views/frame/browser_view.cc and chrome/browser/ui/views/frame/picture_in_picture_browser_frame_view_interactive_uitest.cc? Thanks
Related details
- Andy Paicu
- Maks Orlovich
- Paul Jensen
- Tom Lukaszewicz
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. |
Qingxin Wu (Gerrit)
Qingxin Wu added 1 comment![]( "Open in Gerrit")
@tl...@chromium.org Mind reviewing chrome/browser/ui/views/frame/browser_view.cc and chrome/browser/ui/views/frame/picture_in_picture_browser_frame_view_interactive_uitest.cc? Thanks
oh you're the owner of more files. Mind reviewing the first 5 files (the ones that's not in ui/views/permissions)? Feel free to review other files if you'd like to. Thanks
Tom Lukaszewicz (Gerrit)
Tom Lukaszewicz added 2 comments![]( "Open in Gerrit")
Qingxin Wu@tl...@chromium.org Mind reviewing chrome/browser/ui/views/frame/browser_view.cc and chrome/browser/ui/views/frame/picture_in_picture_browser_frame_view_interactive_uitest.cc? Thanks
oh you're the owner of more files. Mind reviewing the first 5 files (the ones that's not in ui/views/permissions)? Feel free to review other files if you'd like to. Thanks
First 5 files look reasonable to me! I'll circle back once permissions owners have left their +1s.
if (auto* chip = chip_controller->chip()) {
if (chip->GetVisible()) {nit: Avoid a layer of nesting
```
if (auto* chip = chip_controller->chip(); chip->GetVisible()) {
```
Related details
- Andy Paicu
- Maks Orlovich
- Paul Jensen
- Qingxin Wu
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. |
Andy Paicu (Gerrit)
Andy Paicu added 1 comment![]( "Open in Gerrit")
virtual void AnimateCollapse(base::TimeDelta duration) = 0;Qingxin WuAre you sure you need to be passing durations? These go into CSS, so it would be easier if they're just constants in CSS files. Looking around, there seems to be some sort of guard on whether the animations are on or not. I wonder if we should be pushing that bool as yet another CSS var?
Paul JensenGreat point! I removed the duration parameter from the interface entirely and move the constants (e.g. 350ms) down into the native PermissionChipView implementation.
Qingxin WuSo the collapse time is sometimes 250ms and sometimes 75ms, so this change is making it always 250ms. Doesn't seem like a huge change to me but I'm not an owner.
@andy...@chromium.org For PermissionChipView's AnimateCollapse() method, it was passed in a 75ms duration from CollapseConfirmation(), and 250ms in other places. Are we fine to change the 75ms duration case to the same 250ms, or we have to keep the values as they are?
Unfortunately the people that would best know are either OOO or have left the team, but it seems to me that the different animation speeds are for different chips:
1) The 75ms is for the confirmation chip which is a usage indicator
2) The 250ms is for the permission prompt chip which shows together with permission prompts (or sometimes alone).
Since the use cases are different I would like to keep the animation speeds the same as before. Is there any particular reason why you would want to change it? I haven't really gone through the CL yet.
Related details
Maks Orlovich (Gerrit)
Maks Orlovich added 1 comment![]( "Open in Gerrit")
virtual void AnimateCollapse(base::TimeDelta duration) = 0;Qingxin WuAre you sure you need to be passing durations? These go into CSS, so it would be easier if they're just constants in CSS files. Looking around, there seems to be some sort of guard on whether the animations are on or not. I wonder if we should be pushing that bool as yet another CSS var?
Paul JensenGreat point! I removed the duration parameter from the interface entirely and move the constants (e.g. 350ms) down into the native PermissionChipView implementation.
Qingxin WuSo the collapse time is sometimes 250ms and sometimes 75ms, so this change is making it always 250ms. Doesn't seem like a huge change to me but I'm not an owner.
Andy Paicu@andy...@chromium.org For PermissionChipView's AnimateCollapse() method, it was passed in a 75ms duration from CollapseConfirmation(), and 250ms in other places. Are we fine to change the 75ms duration case to the same 250ms, or we have to keep the values as they are?
Unfortunately the people that would best know are either OOO or have left the team, but it seems to me that the different animation speeds are for different chips:
1) The 75ms is for the confirmation chip which is a usage indicator
2) The 250ms is for the permission prompt chip which shows together with permission prompts (or sometimes alone).Since the use cases are different I would like to keep the animation speeds the same as before. Is there any particular reason why you would want to change it? I haven't really gone through the CL yet.
Oh, that means that PermissionChip view can just save Role in constructor, and AnimateCollapse can check it to get the right behavior?
Related details
- Paul Jensen
- Qingxin Wu