Move frame-related methods out of GoogleLandingViewController

ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.mm

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#import "ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.h"
+
+#include "base/logging.h"
+#import "ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_item.h"
+#import "ios/chrome/browser/ui/content_suggestions/cells/content_suggestions_most_visited_tile.h"
+#include "ios/chrome/browser/ui/ui_util.h"
+
+#if !defined(__has_feature) || !__has_feature(objc_arc)
+#error "This file requires ARC support."
+#endif
+
+namespace {
+const CGFloat kMaxSearchFieldFrameMargin = 200;
+const CGFloat kDoodleTopMarginIPadPortrait = 82;
+const CGFloat kDoodleTopMarginIPadLandscape = 82;
+const CGFloat kNTPSearchFieldBottomPadding = 16;
+
+// Height for the doodle frame when Google is not the default search engine.
+const CGFloat kNonGoogleSearchDoodleHeight = 60;
+// Height for the header view on tablet when Google is not the default search
+// engine.
+const CGFloat kNonGoogleSearchHeaderHeightIPad = 10;
+
+enum InterfaceOrientation {
+  ALL,
+  IPHONE_LANDSCAPE,
+};
+}
+
+namespace content_suggestions {
+
+CGRect getOrientationFrame(const CGRect frames[], CGFloat width) {
+  UIInterfaceOrientation orient =

[marq (ping after 24h), 2017/05/04 08:15:00]

Rather than having to have two variables that should be named 'orientation',
maybe hoist the UIInterfaceOrientationIsPortrait() call out and have an
is_portrait BOOL variable instead.

[gambard, 2017/05/04 15:12:32]

Done.

+      [[UIApplication sharedApplication] statusBarOrientation];
+  InterfaceOrientation inter_orient =

[marq (ping after 24h), 2017/05/04 08:15:00]

inter_orient -> orientation

[gambard, 2017/05/04 15:12:32]

Done.

+      (IsIPadIdiom() || UIInterfaceOrientationIsPortrait(orient))
+          ? ALL
+          : IPHONE_LANDSCAPE;
+
+  // Calculate width based on screen width and origin x.
+  CGRect frame = frames[inter_orient];

[marq (ping after 24h), 2017/05/04 08:15:00]

Since this depends on ALL == 0, please add explicit values for the enum.

[gambard, 2017/05/04 15:12:31]

Done.

+  frame.size.width = fmax(width - 2 * frame.origin.x, 50);
+  return frame;
+}
+
+CGFloat leftMarginForWidth(CGFloat width) {
+  NSUInteger columns = [ContentSuggestionsMostVisitedCell
+      numberOfTilesForWidth:width -
+                            2 * [ContentSuggestionsMostVisitedCell spacing]];
+  CGFloat whitespace =
+      width - columns * [ContentSuggestionsMostVisitedTile width] -
+      (columns - 1) * [ContentSuggestionsMostVisitedCell spacing];
+  CGFloat margin = AlignValueToPixel(whitespace / 2);
+  DCHECK(margin >= [ContentSuggestionsMostVisitedCell spacing]);
+  return margin;
+}
+
+CGRect doodleFrame(CGFloat width, BOOL logoIsShowing) {
+  const CGRect kDoodleFrame[2] = {
+      {{0, 66}, {0, 120}}, {{0, 56}, {0, 120}},

[marq (ping after 24h), 2017/05/04 08:15:00]

Please use CGRectMake instead of bare struct assignment.

[gambard, 2017/05/04 15:12:32]

Done.

+  };
+  CGRect doodleFrame = getOrientationFrame(kDoodleFrame, width);
+  if (!IsIPadIdiom() && !logoIsShowing)
+    doodleFrame.size.height = kNonGoogleSearchDoodleHeight;
+  if (IsIPadIdiom()) {
+    doodleFrame.origin.y = IsPortrait() ? kDoodleTopMarginIPadPortrait
+                                        : kDoodleTopMarginIPadLandscape;
+  }
+  return doodleFrame;
+}
+
+CGRect searchFieldFrame(CGFloat width, BOOL logoIsShowing) {
+  CGFloat y = CGRectGetMaxY(doodleFrame(width, logoIsShowing));
+  CGFloat leftMargin = leftMarginForWidth(width);
+  if (leftMargin > kMaxSearchFieldFrameMargin)
+    leftMargin = kMaxSearchFieldFrameMargin;
+  const CGRect kSearchFieldFrame[2] = {
+      {{leftMargin, y + 32}, {0, 50}}, {{leftMargin, y + 16}, {0, 50}},

[marq (ping after 24h), 2017/05/04 08:15:00]

CGRectMake.

[gambard, 2017/05/04 15:12:32]

Done.

+  };

[marq (ping after 24h), 2017/05/04 08:15:00]

This seems like it's broken for RTL? If we don't care about that here, comments
to that effect should be added.

[gambard, 2017/05/04 15:12:32]

The "leftMargin" name was misleading. I corrected it.

+  CGRect searchFieldFrame = getOrientationFrame(kSearchFieldFrame, width);
+  if (IsIPadIdiom()) {
+    CGFloat iPadTopMargin = IsPortrait() ? kDoodleTopMarginIPadPortrait
+                                         : kDoodleTopMarginIPadLandscape;
+    searchFieldFrame.origin.y += iPadTopMargin - 32;
+  }
+  return searchFieldFrame;
+}
+
+CGFloat heightForLogoHeader(CGFloat width,
+                            BOOL logoIsShowing,
+                            BOOL promoCanShow) {
+  CGFloat headerHeight = CGRectGetMaxY(searchFieldFrame(width, logoIsShowing)) +
+                         kNTPSearchFieldBottomPadding;
+  if (IsIPadIdiom()) {
+    if (logoIsShowing) {
+      if (!promoCanShow) {
+        UIInterfaceOrientation orient =
+            [[UIApplication sharedApplication] statusBarOrientation];
+        const CGFloat kTopSpacingMaterialPortrait = 56;
+        const CGFloat kTopSpacingMaterialLandscape = 32;
+        headerHeight += UIInterfaceOrientationIsPortrait(orient)
+                            ? kTopSpacingMaterialPortrait
+                            : kTopSpacingMaterialLandscape;
+      }
+    } else {
+      headerHeight = kNonGoogleSearchHeaderHeightIPad;
+    }
+  }
+  return headerHeight;

[marq (ping after 24h), 2017/05/04 08:15:00]

Maybe un-nest some of the conditionals here for clarity?

Early return of headerHeight if !IsIPadIdiom()

Early return of kNonGoogleSearchHeaderHeightIPad if !logoIsShowing

Early return of headerHeight if promoCanShow

[gambard, 2017/05/04 15:12:32]

Done.

+}
+
+}  // namespace content_suggestions