Ewann Pellé (Gerrit)

unread,

Mar 5, 2026, 8:11:31 AMMar 5

to Anis Chara, AyeAye, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Anis Chara

Ewann Pellé voted and added 1 comment

Votes added by Ewann Pellé

Code-Review

+1

1 comment

File ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/create_tab_group_view_controller.mm

Line 926, Patchset 3:

}
#pragma mark - TabGroupCreationConsumer

Ewann Pellé . unresolved

```suggestion
}

#pragma mark - TabGroupCreationConsumer
```

Open in Gerrit

Related details

Attention is currently required from:

Anis Chara

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Anis Chara (Gerrit)

unread,

Mar 5, 2026, 8:13:58 AMMar 5

to Ewann Pellé, AyeAye, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Anis Chara added 1 comment

File ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/create_tab_group_view_controller.mm

Line 926, Patchset 3:

}
#pragma mark - TabGroupCreationConsumer

Ewann Pellé . resolved

```suggestion
}
#pragma mark - TabGroupCreationConsumer
```

Anis Chara

Fix applied.

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Gauthier Ambard (Gerrit)

unread,

Mar 5, 2026, 8:53:37 AMMar 5

to Anis Chara, Chromium LUCI CQ, Ewann Pellé, AyeAye, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Anis Chara and Ewann Pellé

Gauthier Ambard added 1 comment

File ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/create_tab_group_view_controller.mm

Line 919, Patchset 5 (Latest): _gradientBackground.locations = @[ @0.0, @0.15, @1.0 ];

Gauthier Ambard . unresolved

If this is the same as the other one, should they be a shared constant?

Open in Gerrit

Related details

Attention is currently required from:

Anis Chara
Ewann Pellé

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Anis Chara (Gerrit)

unread,

Mar 6, 2026, 9:37:08 AMMar 6

to Chromium LUCI CQ, Gauthier Ambard, Ewann Pellé, AyeAye, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Gauthier Ambard

Anis Chara added 1 comment

File ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/create_tab_group_view_controller.mm

Line 919, Patchset 5: _gradientBackground.locations = @[ @0.0, @0.15, @1.0 ];

Gauthier Ambard . resolved

If this is the same as the other one, should they be a shared constant?

Anis Chara

Moved it in the TabGroupColorPalette class.

Open in Gerrit

Related details

Attention is currently required from:

Gauthier Ambard

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Anis Chara (Gerrit)

unread,

Mar 6, 2026, 10:17:49 AMMar 6

to Chromium LUCI CQ, Gauthier Ambard, Ewann Pellé, AyeAye, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Ewann Pellé and Gauthier Ambard

Anis Chara added 1 comment

File ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/create_tab_group_view_controller.mm

Line 897, Patchset 8 (Latest): static_cast<tab_groups::TabGroupColorId>(_selectedButton.tag);

Anis Chara . unresolved

Since this method is called four times throughout the view, should I store the result in an ivar instead? @ewa...@chromium.org

Open in Gerrit

Related details

Attention is currently required from:

Ewann Pellé
Gauthier Ambard

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Ewann Pellé (Gerrit)

unread,

Mar 6, 2026, 10:21:11 AMMar 6

to Anis Chara, Chromium LUCI CQ, Gauthier Ambard, AyeAye, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Anis Chara and Gauthier Ambard

Ewann Pellé added 4 comments

File ios/chrome/browser/shared/ui/util/color_palette/tab_group_color_palette.h

Line 39, Patchset 8 (Latest):// The tabGroupColorID used to generate the variants.

Ewann Pellé . unresolved

TabGroupColorId

File ios/chrome/browser/shared/ui/util/color_palette/tab_group_color_palette.mm

Line 173, Patchset 8 (Latest): _colorID = tabGroupColorID;

Ewann Pellé . unresolved

this doesnt crash ? I think it should be `_tabGroupColorID` ?

File ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/create_tab_group_view_controller.mm

Line 46, Patchset 8 (Latest):const CGFloat kViewBackgroundAlphaWithGradient = 0.6;

Ewann Pellé . unresolved

this is used?

Line 222, Patchset 8 (Latest): _gradientBackground.frame = _containerBackground.bounds;

Ewann Pellé . unresolved

To avoid managing the layer you can also create a new class:

``` @interface TabGroupGradientView : UIView // update gradient colors.

(void)setGradientColors:(NSArray*)colors;
@end

@implementation TabGroupGradientView {
    CAGradientLayer* _gradientLayer;
}

- (instancetype)initWithFrame:(CGRect)frame {
    if (self = [super initWithFrame:frame]) {
        _gradientLayer = [CAGradientLayer layer];
        // Set fixed locations once
        _gradientLayer.locations = @[ @0.0, @0.15, @1.0 ];
        [self.layer addSublayer:_gradientLayer];
    }
    return self;
}

- (void)layoutSubviews {
    [super layoutSubviews];
    _gradientLayer.frame = self.bounds;
}

- (void)setGradientColors:(NSArray*)colors {
    _gradientLayer.colors = colors;
}
@end```

Open in Gerrit

Related details

Attention is currently required from:

Anis Chara
Gauthier Ambard

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Ewann Pellé (Gerrit)

unread,

Mar 6, 2026, 10:23:39 AMMar 6

to Anis Chara, Chromium LUCI CQ, Gauthier Ambard, AyeAye, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Anis Chara and Gauthier Ambard

Ewann Pellé added 1 comment

File ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/create_tab_group_view_controller.mm

Line 897, Patchset 8 (Latest): static_cast<tab_groups::TabGroupColorId>(_selectedButton.tag);

Anis Chara . unresolved

Since this method is called four times throughout the view, should I store the result in an ivar instead? @ewa...@chromium.org

Ewann Pellé

It returns the current selected color, this is not a static value. It could be moved in a helper method but shouldnt be stored in an ivar.

satisfied_requirement

unsatisfied_requirement

open

diffy

Gauthier Ambard (Gerrit)

unread,

Mar 9, 2026, 5:14:19 AMMar 9

to Anis Chara, Chromium LUCI CQ, Ewann Pellé, AyeAye, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Anis Chara

Gauthier Ambard added 1 comment

File ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/create_tab_group_view_controller.mm

Line 222, Patchset 8 (Latest): _gradientBackground.frame = _containerBackground.bounds;

Ewann Pellé . unresolved

To avoid managing the layer you can also create a new class:

``` @interface TabGroupGradientView : UIView // update gradient colors.

(void)setGradientColors:(NSArray*)colors;
@end

@implementation TabGroupGradientView {
    CAGradientLayer* _gradientLayer;
}

- (instancetype)initWithFrame:(CGRect)frame {
    if (self = [super initWithFrame:frame]) {
        _gradientLayer = [CAGradientLayer layer];
        // Set fixed locations once
        _gradientLayer.locations = @[ @0.0, @0.15, @1.0 ];
        [self.layer addSublayer:_gradientLayer];
    }
    return self;
}

- (void)layoutSubviews {
    [super layoutSubviews];
    _gradientLayer.frame = self.bounds;
}

- (void)setGradientColors:(NSArray*)colors {
    _gradientLayer.colors = colors;
}
@end```

Gauthier Ambard

If you do that, create a class that takes several stops and colors and put it in ios/chrome/browser/shared/ui/elements/gradient/

Open in Gerrit

Related details

Attention is currently required from:

Anis Chara

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Anis Chara (Gerrit)

unread,

Mar 19, 2026, 5:12:53 AM (4 days ago) Mar 19

to Chromium LUCI CQ, Gauthier Ambard, Ewann Pellé, AyeAye, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Ewann Pellé and Gauthier Ambard

Anis Chara added 5 comments

File ios/chrome/browser/shared/ui/util/color_palette/tab_group_color_palette.h

Line 39, Patchset 8:// The tabGroupColorID used to generate the variants.

Ewann Pellé . resolved

TabGroupColorId

Anis Chara

Done

File ios/chrome/browser/shared/ui/util/color_palette/tab_group_color_palette.mm

Line 173, Patchset 8: _colorID = tabGroupColorID;

Ewann Pellé . resolved

this doesnt crash ? I think it should be `_tabGroupColorID` ?

Anis Chara

Done

File ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/create_tab_group_view_controller.mm

Line 46, Patchset 8:const CGFloat kViewBackgroundAlphaWithGradient = 0.6;

Ewann Pellé . resolved

this is used?

Anis Chara

Done

Line 222, Patchset 8: _gradientBackground.frame = _containerBackground.bounds;

Ewann Pellé . resolved

To avoid managing the layer you can also create a new class:

``` @interface TabGroupGradientView : UIView // update gradient colors.

(void)setGradientColors:(NSArray*)colors;
@end

@implementation TabGroupGradientView {
    CAGradientLayer* _gradientLayer;
}

- (instancetype)initWithFrame:(CGRect)frame {
    if (self = [super initWithFrame:frame]) {
        _gradientLayer = [CAGradientLayer layer];
        // Set fixed locations once
        _gradientLayer.locations = @[ @0.0, @0.15, @1.0 ];
        [self.layer addSublayer:_gradientLayer];
    }
    return self;
}

- (void)layoutSubviews {
    [super layoutSubviews];
    _gradientLayer.frame = self.bounds;
}

- (void)setGradientColors:(NSArray*)colors {
    _gradientLayer.colors = colors;
}
@end```

Gauthier Ambard

If you do that, create a class that takes several stops and colors and put it in ios/chrome/browser/shared/ui/elements/gradient/

Anis Chara

Done

Line 897, Patchset 8: static_cast<tab_groups::TabGroupColorId>(_selectedButton.tag);

Anis Chara . resolved

Since this method is called four times throughout the view, should I store the result in an ivar instead? @ewa...@chromium.org

Ewann Pellé

It returns the current selected color, this is not a static value. It could be moved in a helper method but shouldnt be stored in an ivar.

Anis Chara

Aknowledged

Open in Gerrit

Related details

Attention is currently required from:

Ewann Pellé
Gauthier Ambard

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Ewann Pellé (Gerrit)

unread,

Mar 19, 2026, 6:25:49 AM (4 days ago) Mar 19

to Anis Chara, Chromium LUCI CQ, Gauthier Ambard, AyeAye, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Anis Chara and Gauthier Ambard

Ewann Pellé added 10 comments

Commit Message

Line 9, Patchset 10 (Latest):

This CL applies a gradient background to the CreateTabGroup view and
adds specific coloring to the title TextField and the snapshot
background.

It also refactors the TabGroupColorPalette class to allow retrieving the
gradient background without instantiating the entire palette. This is
necessary for the CreateTabGroup view, which requires a new gradient
background each time a color button is tapped.

Ewann Pellé . unresolved

What about the gradient view update ?

File ios/chrome/browser/shared/ui/elements/gradient/gradient_view.h

Line 42, Patchset 10 (Latest):// than 2 colors, colors must be CGColors.

Ewann Pellé . unresolved

why are you not passing UIColors ?

Line 29, Patchset 10 (Latest):// Initializes the view with a vertical gradient and more than 2 colors.

Ewann Pellé . unresolved

this is not clear.

File ios/chrome/browser/shared/ui/elements/gradient/gradient_view.mm

Line 43, Patchset 10 (Latest): return [self initWithStartColor:topColor

Ewann Pellé . unresolved

ni: call `initWithColors:` directly.

Line 66, Patchset 10 (Latest):- (instancetype)initWithColors:(NSArray*)colors

Ewann Pellé . unresolved

this instancetype seems to be private, I'll suggest to move it at the beginning of the file, also add a comment explaining its purpose.

Line 73, Patchset 10 (Latest): if (colors.count > 2 || locations.count > 2) {

Ewann Pellé . unresolved

Add a comment explaining why this check is necessary.

Line 109, Patchset 10 (Latest):- (void)setColors:(NSArray<UIColor*>*)colors {

Ewann Pellé . unresolved

Looks like you are using UIColor here.

Line 110, Patchset 10 (Latest):

  NSMutableArray* cgColors = [NSMutableArray arrayWithCapacity:colors.count];

  for (UIColor* color in colors) {
    [cgColors addObject:(__bridge id)color.CGColor];
  }

Ewann Pellé . unresolved

nit `self.gradientLayer.colors = [colors valueForKeyPath:@"CGColor"];`

File ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/tab_group_gradient_view.mm

Line 31, Patchset 10 (Latest): self.translatesAutoresizingMaskIntoConstraints = NO;

Ewann Pellé . unresolved

In general this is set by the caller.

File ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/tab_group_view_controller.mm

Line 1055, Patchset 10 (Latest): [(TabGroupGradientView*)_containerBackground

Ewann Pellé . unresolved

this is needed ?

Open in Gerrit

Related details

Attention is currently required from:

Anis Chara
Gauthier Ambard

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Anis Chara (Gerrit)

unread,

9:04 AM (7 hours ago) 9:04 AM

to Code Review Nudger, Chromium LUCI CQ, Gauthier Ambard, Ewann Pellé, AyeAye, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Ewann Pellé and Gauthier Ambard

Anis Chara added 10 comments

Commit Message

Line 9, Patchset 10:This CL applies a gradient background to the CreateTabGroup view and


adds specific coloring to the title TextField and the snapshot
background.

It also refactors the TabGroupColorPalette class to allow retrieving the
gradient background without instantiating the entire palette. This is
necessary for the CreateTabGroup view, which requires a new gradient
background each time a color button is tapped.

Ewann Pellé . resolved

What about the gradient view update ?

Anis Chara

Done

File ios/chrome/browser/shared/ui/elements/gradient/gradient_view.h

Line 42, Patchset 10:// than 2 colors, colors must be CGColors.

Ewann Pellé . resolved

why are you not passing UIColors ?

Anis Chara

Done

Line 29, Patchset 10:// Initializes the view with a vertical gradient and more than 2 colors.

Ewann Pellé . resolved

this is not clear.

Anis Chara

Done

File ios/chrome/browser/shared/ui/elements/gradient/gradient_view.mm

Line 43, Patchset 10: return [self initWithStartColor:topColor

Ewann Pellé . resolved

ni: call `initWithColors:` directly.

Anis Chara

Done

Line 66, Patchset 10:- (instancetype)initWithColors:(NSArray*)colors

Ewann Pellé . resolved

this instancetype seems to be private, I'll suggest to move it at the beginning of the file, also add a comment explaining its purpose.

Anis Chara

Done

Line 73, Patchset 10: if (colors.count > 2 || locations.count > 2) {

Ewann Pellé . resolved

Add a comment explaining why this check is necessary.

Anis Chara

Done

Line 109, Patchset 10:- (void)setColors:(NSArray<UIColor*>*)colors {

Ewann Pellé . resolved

Looks like you are using UIColor here.

Anis Chara

Done

Line 110, Patchset 10: NSMutableArray* cgColors = [NSMutableArray arrayWithCapacity:colors.count];



  for (UIColor* color in colors) {
    [cgColors addObject:(__bridge id)color.CGColor];
  }

Ewann Pellé . unresolved

nit `self.gradientLayer.colors = [colors valueForKeyPath:@"CGColor"];`

Anis Chara

Does not work due to CGColors not being an objective-C object.

File ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/tab_group_gradient_view.mm

Line 31, Patchset 10: self.translatesAutoresizingMaskIntoConstraints = NO;

Ewann Pellé . resolved

In general this is set by the caller.

Anis Chara

Done

File ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/tab_group_view_controller.mm

Line 1055, Patchset 10: [(TabGroupGradientView*)_containerBackground

Ewann Pellé . unresolved

this is needed ?

Anis Chara

Since _containerBackground is declared as a UIView to support the default background, this cast is required to call setColors, which is a method specific to TabGroupGradientView.

Open in Gerrit

Related details

Attention is currently required from:

Ewann Pellé
Gauthier Ambard

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Ewann Pellé (Gerrit)

unread,

9:27 AM (7 hours ago) 9:27 AM

to Anis Chara, Code Review Nudger, Chromium LUCI CQ, Gauthier Ambard, AyeAye, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Anis Chara and Gauthier Ambard

Ewann Pellé added 4 comments

File ios/chrome/browser/shared/ui/elements/gradient/gradient_view.h

Line 29, Patchset 12 (Latest):// Initializes the view with a vertical gradient, intended for use when more

Ewann Pellé . unresolved

still not very clear.

File ios/chrome/browser/shared/ui/elements/gradient/gradient_view.mm

Line 17, Patchset 12 (Latest):#pragma mark - Private

Ewann Pellé . unresolved

you should only have one Private pragma mark.

nit: remove it.

Line 110, Patchset 10:

  NSMutableArray* cgColors = [NSMutableArray arrayWithCapacity:colors.count];

  for (UIColor* color in colors) {
    [cgColors addObject:(__bridge id)color.CGColor];
  }

Ewann Pellé . resolved

nit `self.gradientLayer.colors = [colors valueForKeyPath:@"CGColor"];`

Anis Chara

Does not work due to CGColors not being an objective-C object.

Ewann Pellé

Acknowledged

File ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/tab_group_view_controller.mm

Line 1055, Patchset 10: [(TabGroupGradientView*)_containerBackground

Ewann Pellé . unresolved

this is needed ?

Anis Chara

Since _containerBackground is declared as a UIView to support the default background, this cast is required to call setColors, which is a method specific to TabGroupGradientView.

Ewann Pellé

In that case you should do:
`TabGroupGradientView* gradientBackground = base::apple::ObjCCast<TabGroupGradientView>(_containerBackground);`

Open in Gerrit

Related details

Attention is currently required from:

Anis Chara
Gauthier Ambard

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Anis Chara (Gerrit)

unread,

9:55 AM (7 hours ago) 9:55 AM

to Code Review Nudger, Chromium LUCI CQ, Gauthier Ambard, Ewann Pellé, AyeAye, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Ewann Pellé and Gauthier Ambard

Anis Chara added 3 comments

File ios/chrome/browser/shared/ui/elements/gradient/gradient_view.h

Line 29, Patchset 12:// Initializes the view with a vertical gradient, intended for use when more

Ewann Pellé . resolved

still not very clear.

Anis Chara

Done

File ios/chrome/browser/shared/ui/elements/gradient/gradient_view.mm

Line 17, Patchset 12:#pragma mark - Private

Ewann Pellé . resolved

you should only have one Private pragma mark.
nit: remove it.

Anis Chara

Done

File ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/tab_group_view_controller.mm

Line 1055, Patchset 10: [(TabGroupGradientView*)_containerBackground

Ewann Pellé . resolved

this is needed ?

Anis Chara

Since _containerBackground is declared as a UIView to support the default background, this cast is required to call setColors, which is a method specific to TabGroupGradientView.

Ewann Pellé

In that case you should do:
`TabGroupGradientView* gradientBackground = base::apple::ObjCCast<TabGroupGradientView>(_containerBackground);`

Anis Chara

Done

Open in Gerrit

Related details

Attention is currently required from:

Ewann Pellé
Gauthier Ambard

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Ewann Pellé (Gerrit)

unread,

10:00 AM (6 hours ago) 10:00 AM

to Anis Chara, Code Review Nudger, Chromium LUCI CQ, Gauthier Ambard, AyeAye, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Anis Chara and Gauthier Ambard

Ewann Pellé voted and added 2 comments

Votes added by Ewann Pellé

Code-Review

+1

2 comments

File ios/chrome/browser/shared/ui/elements/gradient/gradient_view.h

Line 14, Patchset 13 (Latest):@interface GradientView : UIView

Ewann Pellé . unresolved

Please fix order:
go/bling-best-practices#declaration-order-in-header-files

File ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/tab_group_gradient_view.h

Line 12, Patchset 13 (Latest):@interface TabGroupGradientView : GradientView

Ewann Pellé . unresolved

Add a description.

Open in Gerrit

Related details

Attention is currently required from:

Anis Chara
Gauthier Ambard

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Gauthier Ambard (Gerrit)

unread,

10:08 AM (6 hours ago) 10:08 AM

to Anis Chara, Ewann Pellé, Code Review Nudger, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Anis Chara

Gauthier Ambard added 5 comments

File ios/chrome/browser/shared/ui/elements/gradient/gradient_view.mm

Line 118, Patchset 13 (Latest): [cgColors addObject:(__bridge id)color.CGColor];

Gauthier Ambard . unresolved

You don't need the __bridge I think

File ios/chrome/browser/shared/ui/util/color_palette/tab_group_color_palette.mm

Line 132, Patchset 13 (Latest):// Returns the colors used for the gradient Background.

Gauthier Ambard . unresolved

Remove the comments here and above, those are public methods.

Line 140, Patchset 13 (Latest): (id)UIColorFromRGB(group.tone30),

Gauthier Ambard . unresolved

Why the cast to id here?

File ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/tab_group_gradient_view.h

Line 12, Patchset 13 (Latest):@interface TabGroupGradientView : GradientView

Gauthier Ambard . unresolved

Add comment

File ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/tab_group_view_controller.mm

Line 1059, Patchset 13 (Latest): base::apple::ObjCCastStrict<TabGroupGradientView>(_containerBackground);

Gauthier Ambard . unresolved

Rather than this, WDYT of adding another ivar for the same object but with the right type (i.e. in `configuredBackground` you set it), with a comment mentioning that it should be removed once shipped?

Open in Gerrit

Related details

Attention is currently required from:

Anis Chara

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Ewann Pellé (Gerrit)

unread,

10:11 AM (6 hours ago) 10:11 AM

to Anis Chara, Code Review Nudger, Chromium LUCI CQ, Gauthier Ambard, AyeAye, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Anis Chara

Ewann Pellé added 1 comment

File ios/chrome/browser/shared/ui/elements/gradient/gradient_view.h

Line 14, Patchset 13 (Latest):@interface GradientView : UIView

Ewann Pellé . resolved

Please fix order:
go/bling-best-practices#declaration-order-in-header-files

Ewann Pellé

Done

satisfied_requirement

unsatisfied_requirement

open

diffy

Anis Chara (Gerrit)

unread,

10:54 AM (6 hours ago) 10:54 AM

to Ewann Pellé, Code Review Nudger, Chromium LUCI CQ, Gauthier Ambard, AyeAye, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Ewann Pellé and Gauthier Ambard

Anis Chara added 6 comments

File ios/chrome/browser/shared/ui/elements/gradient/gradient_view.mm

Line 118, Patchset 13: [cgColors addObject:(__bridge id)color.CGColor];

Gauthier Ambard . resolved

You don't need the __bridge I think

Anis Chara

Done

File ios/chrome/browser/shared/ui/util/color_palette/tab_group_color_palette.mm

Line 132, Patchset 13:// Returns the colors used for the gradient Background.

Gauthier Ambard . resolved

Remove the comments here and above, those are public methods.

Anis Chara

Done

Line 140, Patchset 13: (id)UIColorFromRGB(group.tone30),

Gauthier Ambard . resolved

Why the cast to id here?

Anis Chara

removed

File ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/tab_group_gradient_view.h

Line 12, Patchset 13:@interface TabGroupGradientView : GradientView

Gauthier Ambard . resolved

Add comment

Anis Chara

Done

Line 12, Patchset 13:@interface TabGroupGradientView : GradientView

Ewann Pellé . resolved

Add a description.

Anis Chara

Done

File ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/tab_group_view_controller.mm

Line 1059, Patchset 13: base::apple::ObjCCastStrict<TabGroupGradientView>(_containerBackground);

Gauthier Ambard . resolved

Rather than this, WDYT of adding another ivar for the same object but with the right type (i.e. in `configuredBackground` you set it), with a comment mentioning that it should be removed once shipped?

Anis Chara

Done

Open in Gerrit

Related details

Attention is currently required from:

Ewann Pellé
Gauthier Ambard

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Gauthier Ambard (Gerrit)

unread,

11:36 AM (5 hours ago) 11:36 AM

to Anis Chara, Ewann Pellé, Code Review Nudger, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Anis Chara and Ewann Pellé

Gauthier Ambard voted and added 1 comment

Votes added by Gauthier Ambard

Code-Review

+1

1 comment

File ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/tab_group_view_controller.mm

Line 764, Patchset 15 (Latest): _containerGradientBackground = background;

Gauthier Ambard . unresolved

Add a TODO here to update the type.

Open in Gerrit

Related details

Attention is currently required from:

Anis Chara
Ewann Pellé

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Anis Chara (Gerrit)

unread,

11:51 AM (5 hours ago) 11:51 AM

to Gauthier Ambard, Ewann Pellé, Code Review Nudger, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Ewann Pellé and Gauthier Ambard

Anis Chara added 1 comment

File ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/tab_group_view_controller.mm

Line 764, Patchset 15: _containerGradientBackground = background;

Gauthier Ambard . resolved

Add a TODO here to update the type.

Anis Chara

Done

Open in Gerrit

Related details

Attention is currently required from:

Ewann Pellé
Gauthier Ambard

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Ewann Pellé (Gerrit)

unread,

11:59 AM (4 hours ago) 11:59 AM

to Anis Chara, Gauthier Ambard, Code Review Nudger, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Anis Chara and Gauthier Ambard

Ewann Pellé voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Anis Chara
Gauthier Ambard

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Gauthier Ambard (Gerrit)

unread,

12:59 PM (3 hours ago) 12:59 PM

to Anis Chara, Ewann Pellé, Code Review Nudger, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Anis Chara

Gauthier Ambard voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Anis Chara

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Anis Chara (Gerrit)

unread,

1:47 PM (3 hours ago) 1:47 PM

to Gauthier Ambard, Ewann Pellé, Code Review Nudger, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Anis Chara voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

2:54 PM (2 hours ago) 2:54 PM

to Anis Chara, Gauthier Ambard, Ewann Pellé, Code Review Nudger, AyeAye, chromium...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

[iOS] Add TabGroupPalette colors to the CreateTabGroup view

This CL applies a gradient background to the CreateTabGroupView and adds


specific coloring to the title TextField and the snapshot background.

It also refactors the TabGroupColorPalette class to allow retrieving the
gradient background without instantiating the entire palette. This is
necessary for the CreateTabGroup view, which requires a new gradient
background each time a color button is tapped.

Finally, it refactors GradientView by introducing a private initializer
to centralize configurations and a new initializer for gradients with
more than two colors. It also adds a TabGroupGradientView class to
handle the gradients in both CreateTabGroupView and TabGroupView.

screenshot :
https://drive.google.com/file/d/1dyuPCJ2PoHuoE_rl8UoFmYK5Akc9KcJJ/view?usp=sharing&resourcekey=0-GCXy9bNrxJFy1gRqxHo3JQ

record of the animations :
https://drive.google.com/file/d/1hOIkQkrbPoc1c3AgxNzohy89_MH9LiOs/view?usp=sharing&resourcekey=0-UeI-FaN9bfY3ewF_RpwS3Q

Bug: 445887468, 489639989

Change-Id: Ic63f2ab8e06b28d96b1dbbdb8d8b7dedceb16c94

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7633766

Commit-Queue: Anis Chara <ach...@google.com>

Reviewed-by: Ewann Pellé <ewa...@chromium.org>

Reviewed-by: Gauthier Ambard <gam...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1603623}

Files:

M ios/chrome/browser/shared/ui/elements/gradient/gradient_view.h
M ios/chrome/browser/shared/ui/elements/gradient/gradient_view.mm
M ios/chrome/browser/shared/ui/util/color_palette/tab_group_color_palette.h
M ios/chrome/browser/shared/ui/util/color_palette/tab_group_color_palette.mm
M ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/BUILD.gn
M ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/create_tab_group_view_controller.mm
A ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/tab_group_gradient_view.h
A ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/tab_group_gradient_view.mm
M ios/chrome/browser/tab_switcher/ui_bundled/tab_grid/tab_groups/tab_group_view_controller.mm

Change size: L

Delta: 9 files changed, 247 insertions(+), 89 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Gauthier Ambard, +1 by Ewann Pellé

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

[iOS] Add TabGroupPalette colors to the CreateTabGroup view [chromium/src : main]

Ewann Pellé (Gerrit)

Ewann Pellé voted and added 1 comment

Votes added by Ewann Pellé

1 comment

Related details

Anis Chara (Gerrit)

Anis Chara added 1 comment

Related details

Gauthier Ambard (Gerrit)

Gauthier Ambard added 1 comment

Related details

Anis Chara (Gerrit)

Anis Chara added 1 comment

Related details

Anis Chara (Gerrit)

Anis Chara added 1 comment

Related details

Ewann Pellé (Gerrit)

Ewann Pellé added 4 comments

Related details

Ewann Pellé (Gerrit)

Ewann Pellé added 1 comment

Gauthier Ambard (Gerrit)

Gauthier Ambard added 1 comment

Related details

Anis Chara (Gerrit)

Anis Chara added 5 comments

Related details

Ewann Pellé (Gerrit)

Ewann Pellé added 10 comments

Related details

Anis Chara (Gerrit)

Anis Chara added 10 comments

Related details

Ewann Pellé (Gerrit)

Ewann Pellé added 4 comments

Related details

Anis Chara (Gerrit)

Anis Chara added 3 comments

Related details

Ewann Pellé (Gerrit)

Ewann Pellé voted and added 2 comments

Votes added by Ewann Pellé

2 comments

Related details

Gauthier Ambard (Gerrit)

Gauthier Ambard added 5 comments

Related details

Ewann Pellé (Gerrit)

Ewann Pellé added 1 comment

Anis Chara (Gerrit)

Anis Chara added 6 comments

Related details

Gauthier Ambard (Gerrit)

Gauthier Ambard voted and added 1 comment

Votes added by Gauthier Ambard

1 comment

Related details

Anis Chara (Gerrit)

Anis Chara added 1 comment

Related details

Ewann Pellé (Gerrit)

Ewann Pellé voted Code-Review+1

Related details

Gauthier Ambard (Gerrit)

Gauthier Ambard voted Code-Review+1

Related details

Anis Chara (Gerrit)

Anis Chara voted Commit-Queue+2

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information