[iOS] Add banner to the toolbar [chromium/src : main]

0 views

Skip to first unread message

Eric Ekey (Gerrit)

unread,

1:30 PM (2 hours ago) 1:30 PM

to Gauthier Ambard, Chris Lu, chromium...@chromium.org, feature-me...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Chris Lu and Gauthier Ambard

Eric Ekey voted and added 9 comments

Votes added by Eric Ekey

Code-Review

9 comments

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Eric Ekey . unresolved

LGTM w/ some nits. Have you tested how the banner looks when resizing an iPad window?

File ios/chrome/browser/toolbar/coordinator/DEPS

Line 3, Patchset 6 (Latest): "+ios/chrome/browser/banner_promo/model/default_browser_banner_promo_app_agent.h",

Eric Ekey . unresolved

I think best practice is to include `model/` instead of the specific header?

File ios/chrome/browser/toolbar/coordinator/main_toolbar_coordinator.mm

Line 1256, Patchset 6 (Latest): if (topPosition && !self.profile->IsOffTheRecord()) {

Eric Ekey . unresolved

This is accessed a couple times in this method. Consider creating a variable to reuse.

File ios/chrome/browser/toolbar/coordinator/toolbar_mediator.h

Line 26, Patchset 6 (Latest):@interface ToolbarMediator : NSObject <ToolbarMutator, BannerPromoViewDelegate>

Eric Ekey . unresolved

nit: Alphabetical order

File ios/chrome/browser/toolbar/ui/toolbar_view_controller.h

Line 14, Patchset 6 (Latest):@protocol BannerPromoViewDelegate;

Eric Ekey . unresolved

nit: Alphabetical order.

File ios/chrome/browser/toolbar/ui/toolbar_view_controller.mm

Line 168, Patchset 6 (Latest):

  // Constraints for the banner promo in split toolbar mode.
  NSArray<NSLayoutConstraint*>* _bannerPromoAboveConstraints;
  // Constraints for the banner promo in non-split toolbar mode.
  NSArray<NSLayoutConstraint*>* _bannerPromoBelowConstraints;

Eric Ekey . unresolved

Can you specify in the comments that in split toolbar mode the banner is above the toolbar and vice versa?

Line 534, Patchset 6 (Latest):

// Helper method to actually do the animation to show the banner promo.
- (void)showBannerPromoAnimationBlock {
  _bannerPromoBackgroundHeightConstraint.constant =
      [self bannerPromoBackgroundHeightForFullscreenProgress:1];
  _locationBarBottomPaddingConstraint.constant =
      -[self locationBarBottomPaddingForFullscreenProgress:_fullscreenProgress];
  [self.toolbarHeightDelegate toolbarsHeightChanged];
  [self.view.superview layoutIfNeeded];
}

// Helper method for show completion.
- (void)showBannerPromoCompletionBlock {
  UIAccessibilityPostNotification(UIAccessibilityLayoutChangedNotification,
                                  _bannerPromo);
}

Eric Ekey . unresolved

Consider moving under private pragma

Line 568, Patchset 6 (Latest):

// Helper method to actually do the animation to hide the banner promo.
- (void)hideBannerPromoAnimationBlock {
  if ([self isBannerBelowToolbar]) {
    _bannerPromoBackgroundHeightConstraint.constant = 0;
  } else {
    _bannerPromoBackgroundTopConstraint.constant =
        -[self bannerPromoBackgroundHeightForFullscreenProgress:1];
  }

  _bannerPromoVisible = NO;

  _locationBarBottomPaddingConstraint.constant =
      -[self locationBarBottomPaddingForFullscreenProgress:_fullscreenProgress];

  [self.toolbarHeightDelegate toolbarsHeightChanged];
  [self.view.superview layoutIfNeeded];
}

// Helper method for hide completion.
- (void)hideBannerPromoCompletionBlock {
  [NSLayoutConstraint deactivateConstraints:_bannerPromoAboveConstraints];
  [NSLayoutConstraint deactivateConstraints:_bannerPromoBelowConstraints];
  [self.view.superview layoutIfNeeded];
}

Eric Ekey . unresolved

Same as above.

Line 870, Patchset 6 (Latest):- (void)setUpBannerPromo {

Eric Ekey . unresolved

Comment.

Open in Gerrit

Related details

Attention is currently required from:

Chris Lu
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

Reply all

Reply to author

Forward

0 new messages