Move frame-related methods out of GoogleLandingViewController

Move frame-related methods out of GoogleLandingViewController

In order to be able to display the ContentSuggestions as NTP, some methods
should be moved in helpers.
This CL moves the methods related to the frame of the elements in the NTP.

BUG=700375, 717974
Review-Url: https://codereview.chromium.org/2857123003
Cr-Commit-Position: refs/heads/master@{#470549}
Committed: https://chromium.googlesource.com/chromium/src/+/2c54e2be976f7c4e8d8ac2438f5f1e97f6566a6c

[gambard, 2017-05-03 10:36:13]

gambard@chromium.org changed reviewers:
+ marq@chromium.org

[gambard, 2017-05-03 10:36:13]

PTAL.

[gambard, 2017-05-03 14:22:34]

Description was changed from

==========
Move frame-related methods out of GoogleLandingViewController

In order to be able to display the ContentSuggestions as NTP, some methods
should be moved in helpers.
This CL moves the methods related to the frame of the elements in the NTP.

BUG=700375
==========

to

==========
Move frame-related methods out of GoogleLandingViewController

In order to be able to display the ContentSuggestions as NTP, some methods
should be moved in helpers.
This CL moves the methods related to the frame of the elements in the NTP.

BUG=700375, 700375
==========

[gambard, 2017-05-03 14:22:45]

Description was changed from

==========
Move frame-related methods out of GoogleLandingViewController

In order to be able to display the ContentSuggestions as NTP, some methods
should be moved in helpers.
This CL moves the methods related to the frame of the elements in the NTP.

BUG=700375, 700375
==========

to

==========
Move frame-related methods out of GoogleLandingViewController

In order to be able to display the ContentSuggestions as NTP, some methods
should be moved in helpers.
This CL moves the methods related to the frame of the elements in the NTP.

BUG=700375, 717974
==========

[gambard, 2017-05-03 15:37:15]

The CQ bit was checked by gambard@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 15:37:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 16:48:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 16:48:59]

Dry run: This issue passed the CQ dry run.

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

Unit tests for the extracted functions?

https://codereview.chromium.org/2857123003/diff/40001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.h
(right):

https://codereview.chromium.org/2857123003/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.h:16:
// Returns the left margin in order to have the tiles centered in a view with a
Left, or leading?

https://codereview.chromium.org/2857123003/diff/40001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.mm
(right):

https://codereview.chromium.org/2857123003/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.mm:37:
UIInterfaceOrientation orient =
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.

https://codereview.chromium.org/2857123003/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.mm:39:
InterfaceOrientation inter_orient =
inter_orient -> orientation

https://codereview.chromium.org/2857123003/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.mm:45:
CGRect frame = frames[inter_orient];
Since this depends on ALL == 0, please add explicit values for the enum.

https://codereview.chromium.org/2857123003/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.mm:64:
{{0, 66}, {0, 120}}, {{0, 56}, {0, 120}},
Please use CGRectMake instead of bare struct assignment.

https://codereview.chromium.org/2857123003/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.mm:82:
{{leftMargin, y + 32}, {0, 50}}, {{leftMargin, y + 16}, {0, 50}},
CGRectMake.

https://codereview.chromium.org/2857123003/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.mm:83:
};
This seems like it's broken for RTL? If we don't care about that here, comments
to that effect should be added.

https://codereview.chromium.org/2857123003/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.mm:113:
return headerHeight;
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:31]

gambard@chromium.org changed reviewers:
+ rohitrao@google.com

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

Thanks, rohit@: PTAL as marq is OOO

https://codereview.chromium.org/2857123003/diff/40001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.h
(right):

https://codereview.chromium.org/2857123003/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.h:16:
// Returns the left margin in order to have the tiles centered in a view with a
On 2017/05/04 08:15:00, marq (OOO to May 9) wrote:
> Left, or leading?

It should be symmetrical.
Done.

https://codereview.chromium.org/2857123003/diff/40001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.mm
(right):

https://codereview.chromium.org/2857123003/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.mm:37:
UIInterfaceOrientation orient =
On 2017/05/04 08:15:00, marq (OOO to May 9) wrote:
> 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.

Done.

https://codereview.chromium.org/2857123003/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.mm:39:
InterfaceOrientation inter_orient =
On 2017/05/04 08:15:00, marq (OOO to May 9) wrote:
> inter_orient -> orientation

Done.

https://codereview.chromium.org/2857123003/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.mm:45:
CGRect frame = frames[inter_orient];
On 2017/05/04 08:15:00, marq (OOO to May 9) wrote:
> Since this depends on ALL == 0, please add explicit values for the enum.

Done.

https://codereview.chromium.org/2857123003/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.mm:64:
{{0, 66}, {0, 120}}, {{0, 56}, {0, 120}},
On 2017/05/04 08:15:00, marq (OOO to May 9) wrote:
> Please use CGRectMake instead of bare struct assignment.

Done.

https://codereview.chromium.org/2857123003/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.mm:82:
{{leftMargin, y + 32}, {0, 50}}, {{leftMargin, y + 16}, {0, 50}},
On 2017/05/04 08:15:00, marq (OOO to May 9) wrote:
> CGRectMake.

Done.

https://codereview.chromium.org/2857123003/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.mm:83:
};
On 2017/05/04 08:15:00, marq (OOO to May 9) wrote:
> This seems like it's broken for RTL? If we don't care about that here,
comments
> to that effect should be added.

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

https://codereview.chromium.org/2857123003/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.mm:113:
return headerHeight;
On 2017/05/04 08:15:00, marq (OOO to May 9) wrote:
> 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

Done.

[marq (ping after 24h), 2017-05-10 09:23:49]

https://codereview.chromium.org/2857123003/diff/60001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.h
(right):

https://codereview.chromium.org/2857123003/diff/60001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.h:16:
// Returns left margin in order to have the tiles centered in a view with a
For clarity, maybe 'x-offset' is clearer than 'left margin'.

https://codereview.chromium.org/2857123003/diff/60001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils_unittest.mm
(right):

https://codereview.chromium.org/2857123003/diff/60001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils_unittest.mm:38:
std::unique_ptr<ScopedBlockSwizzler> safe_mode_swizzler_ =
orientation_swizzler seems like a better name.

Also, this isn't a member variable, so no trailing underscore.

https://codereview.chromium.org/2857123003/diff/60001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils_unittest.mm:72:
if (!IsIPadIdiom())
Can you swizzle UIDevice's -userInterfaceIdiom to exercise the various
IsIPadIdiom code paths without putting conditionals in the tests?

(I'm fine for that to be in a follow-up CL).

[gambard, 2017-05-10 11:09:40]

Thanks, PTAL.

https://codereview.chromium.org/2857123003/diff/60001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.h
(right):

https://codereview.chromium.org/2857123003/diff/60001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils.h:16:
// Returns left margin in order to have the tiles centered in a view with a
On 2017/05/10 09:23:49, marq (ping after 24h) wrote:
> For clarity, maybe 'x-offset' is clearer than 'left margin'.

Done.

https://codereview.chromium.org/2857123003/diff/60001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils_unittest.mm
(right):

https://codereview.chromium.org/2857123003/diff/60001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils_unittest.mm:38:
std::unique_ptr<ScopedBlockSwizzler> safe_mode_swizzler_ =
On 2017/05/10 09:23:49, marq (ping after 24h) wrote:
> orientation_swizzler seems like a better name.
> 
> Also, this isn't a member variable, so no trailing underscore.

Done.

https://codereview.chromium.org/2857123003/diff/60001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils_unittest.mm:72:
if (!IsIPadIdiom())
On 2017/05/10 09:23:49, marq (ping after 24h) wrote:
> Can you swizzle UIDevice's -userInterfaceIdiom to exercise the various
> IsIPadIdiom code paths without putting conditionals in the tests?
> 
> (I'm fine for that to be in a follow-up CL).

Done.

[marq (ping after 24h), 2017-05-10 11:17:27]

https://codereview.chromium.org/2857123003/diff/100001/ios/chrome/browser/ui/...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils_unittest.mm
(right):

https://codereview.chromium.org/2857123003/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils_unittest.mm:72:
if (IsIPadIdiom()) {
Here (and elsewhere) -- for each of the tests with IsIPadIdiom() conditions,
make two tests and use SetAs* instead of having this conditional.

[gambard, 2017-05-10 11:48:46]

Thanks, PTAL.

https://codereview.chromium.org/2857123003/diff/100001/ios/chrome/browser/ui/...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils_unittest.mm
(right):

https://codereview.chromium.org/2857123003/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_utils_unittest.mm:72:
if (IsIPadIdiom()) {
On 2017/05/10 11:17:26, marq (ping after 24h) wrote:
> Here (and elsewhere) -- for each of the tests with IsIPadIdiom() conditions,
> make two tests and use SetAs* instead of having this conditional.

Done.

[marq (ping after 24h), 2017-05-10 11:58:02]

lgtm

[gambard, 2017-05-10 11:58:32]

The CQ bit was checked by gambard@chromium.org

[commit-bot: I haz the power, 2017-05-10 11:58:59]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 13:13:50]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494417512524190,
"parent_rev": "15fa40c5ef33b76a60a40661452716a822c357e7", "commit_rev":
"2c54e2be976f7c4e8d8ac2438f5f1e97f6566a6c"}

[commit-bot: I haz the power, 2017-05-10 13:14:02]

Description was changed from

==========
Move frame-related methods out of GoogleLandingViewController

In order to be able to display the ContentSuggestions as NTP, some methods
should be moved in helpers.
This CL moves the methods related to the frame of the elements in the NTP.

BUG=700375, 717974
==========

to

==========
Move frame-related methods out of GoogleLandingViewController

In order to be able to display the ContentSuggestions as NTP, some methods
should be moved in helpers.
This CL moves the methods related to the frame of the elements in the NTP.

BUG=700375, 717974

Review-Url: https://codereview.chromium.org/2857123003
Cr-Commit-Position: refs/heads/master@{#470549}
Committed:
https://chromium.googlesource.com/chromium/src/+/2c54e2be976f7c4e8d8ac2438f5f...
==========

[commit-bot: I haz the power, 2017-05-10 13:14:03]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/2c54e2be976f7c4e8d8ac2438f5f...