[omnibox] Add new animation for Ai Mode button in Omnibox [chromium/src : main]
Aviv Kiss (Gerrit)
Aviv Kiss voted and added 4 comments![]( "Open in Gerrit")
Votes added by Aviv Kiss
| Code-Review | +1 |
4 comments
}Please fix this WARNING reported by autoreview issue finding: This looks like it is missing the `InkDrop` updates and `host->UpdateBackground()` that `StandardIconLabelBubbleAnimation::OnAnimationEnded` has.
Since `ShowAnimation()` and `HideAnimation()` in `IconLabelBubbleView` set `ShowHighlightOnHover` and `ShowHighlightOnFocus` to `false`, failing to restore them here will result in the AI Mode button permanently losing its hover and focus highlights after an animation ends.
Consider adding:
```cpp
views::InkDrop::Get(host)->GetInkDrop()->SetShowHighlightOnHover(true);
views::InkDrop::Get(host)->GetInkDrop()->SetShowHighlightOnFocus(
!views::FocusRing::Get(host));
host->UpdateBackground();
```
CHECK(animation_);I think these CHECK calls will result in crashes in production. Is that intended?
UpdateIconImage();Please fix this WARNING reported by autoreview issue finding: `UpdateIconImage()` is already called at the end of `UpdateAnimationState()` which is executed below on line 189. We can remove this call to avoid doing the same work twice.
// Drive transitions genericallyWhat does this mean in this context? Looks like the actual code block below this is reseting if not visible?
Related details
- Caitlin Chen
- Kaan Alsan
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. |
Caitlin Chen (Gerrit)
Caitlin Chen added 4 comments![]( "Open in Gerrit")
Please fix this WARNING reported by autoreview issue finding: This looks like it is missing the `InkDrop` updates and `host->UpdateBackground()` that `StandardIconLabelBubbleAnimation::OnAnimationEnded` has.
Since `ShowAnimation()` and `HideAnimation()` in `IconLabelBubbleView` set `ShowHighlightOnHover` and `ShowHighlightOnFocus` to `false`, failing to restore them here will result in the AI Mode button permanently losing its hover and focus highlights after an animation ends.
Consider adding:
```cpp
views::InkDrop::Get(host)->GetInkDrop()->SetShowHighlightOnHover(true);
views::InkDrop::Get(host)->GetInkDrop()->SetShowHighlightOnFocus(
!views::FocusRing::Get(host));
host->UpdateBackground();
```
Done
I think these CHECK calls will result in crashes in production. Is that intended?
Ah, yes I likely meant DCHECK. CHanged!
Please fix this WARNING reported by autoreview issue finding: `UpdateIconImage()` is already called at the end of `UpdateAnimationState()` which is executed below on line 189. We can remove this call to avoid doing the same work twice.
Done
What does this mean in this context? Looks like the actual code block below this is reseting if not visible?
Sorry that was a misplaced comment trying to describe the transitions were being handled by the animation.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kaan Alsan (Gerrit)
Kaan Alsan added 6 comments![]( "Open in Gerrit")
I haven't looked at icon_label_bubble_view implementation yet, will get to it tomorrow
virtual void SetAnimationStyle(actions::ActionId action_id,
PageActionAnimationStyle style) = 0;
virtual void SetTrailingImage(actions::ActionId action_id,
const ui::ImageModel& trailing_image) = 0;
virtual void ClearTrailingImage(actions::ActionId action_id) = 0;
virtual void SetShowTrailingIcon(actions::ActionId action_id, bool show) = 0;
Add comment documenting what these do.
CHECK(animation_);Caitlin ChenI think these CHECK calls will result in crashes in production. Is that intended?
Ah, yes I likely meant DCHECK. CHanged!
We should be using CHECKs instead of DCHECKs, unless there's a good reason (usually performance related): https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/checks.md#invariant_verification-mechanisms.
If `animation_` really is null and we don't crash via CHECK, then the next line will be a null deref, which will still crash _and_ be a security issue.
IconLabelBubbleView::AnimationStyle GetAnimationStyleForTesting() const {
return GetAnimationStyle();
}
views::ImageView* GetTrailingImageViewForTesting() const {
return GetTrailingImageView();
}
these aren't trivial implementations (i.e. directly returning a member), so shouldn't be defined inline.
views::ImageView* GetTrailingImageViewForTesting() const {This method isn't const-correct. It should either return `const views::ImageView*` or this shouldn't be a `const` method. Same comment applies within icon_label_bubble_view.cc/h.
https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/const.md
Related details
- Caitlin Chen
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. |
Caitlin Chen (Gerrit)
Caitlin Chen added 6 comments![]( "Open in Gerrit")
I haven't looked at icon_label_bubble_view implementation yet, will get to it tomorrow
No worries!! Thanks for being patient with me and all the guidance!!
virtual void SetAnimationStyle(actions::ActionId action_id,
PageActionAnimationStyle style) = 0;
virtual void SetTrailingImage(actions::ActionId action_id,
const ui::ImageModel& trailing_image) = 0;
virtual void ClearTrailingImage(actions::ActionId action_id) = 0;
virtual void SetShowTrailingIcon(actions::ActionId action_id, bool show) = 0;
Add comment documenting what these do.
Done
CHECK(animation_);Caitlin ChenI think these CHECK calls will result in crashes in production. Is that intended?
Kaan AlsanAh, yes I likely meant DCHECK. CHanged!
We should be using CHECKs instead of DCHECKs, unless there's a good reason (usually performance related): https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/checks.md#invariant_verification-mechanisms.
If `animation_` really is null and we don't crash via CHECK, then the next line will be a null deref, which will still crash _and_ be a security issue.
Done
IconLabelBubbleView::AnimationStyle GetAnimationStyleForTesting() const {
return GetAnimationStyle();
}
views::ImageView* GetTrailingImageViewForTesting() const {
return GetTrailingImageView();
}
these aren't trivial implementations (i.e. directly returning a member), so shouldn't be defined inline.
Done
views::ImageView* GetTrailingImageViewForTesting() const {This method isn't const-correct. It should either return `const views::ImageView*` or this shouldn't be a `const` method. Same comment applies within icon_label_bubble_view.cc/h.
https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/const.md
- Kaan Alsan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |