[iOS] Upstream //ios/chrome/browser/ui

[iOS] Upstream //ios/chrome/browser/ui

Committed: https://crrev.com/56312cdf58df182ce892e333c99c54090edd8593
Cr-Commit-Position: refs/heads/master@{#324230}

[droger, 2015-04-02 16:45:50]

Patchset #1 (id:1) has been deleted

[droger, 2015-04-02 16:46:05]

droger@chromium.org changed reviewers:
+ sdefresne@chromium.org

[sdefresne, 2015-04-02 17:20:29]

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/O...
File ios/chrome/browser/ui/OWNERS (right):

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/O...
ios/chrome/browser/ui/OWNERS:1: bdibello@chromium.org
bdibello: are you okay reviewing this going forward?

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/background_generator.h (right):

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/background_generator.h:10: namespace background_generator
{
style: why the namespace?

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/background_generator.h:15: // at the end of |radius|.
|tileImage| is tiled over the entire
nit: I think that "image" would fit on this line. Can you relayout this comment?

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/background_generator.mm (right):

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/background_generator.mm:19: // TODO(blundell): This
probably belongs somewhere else now.
nit: I'd prefer if we could resolve or remove this TODO before upstreaming (my
hunch would be to just delete it).

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/background_generator.mm:30: CGFloat
gradient_colors[kColorCount * sizeof(CGFloat)] = {
This does not make any sense. This is initializing an array of 8 / 16 CGFloats
but only 4 are initialized.

According to the documentation of CGGradientCreateWithColorComponents,
gradient_colors size should be "the product of count and the number of
components in the color space". Since color space if grey color space, it has
two components, so I think the correct code is:

CGFloat gradient_colors[4] = { centerColor, 1.0, outsideColor, 1.0 };

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/f...
File ios/chrome/browser/ui/file_locations.h (right):

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/f...
ios/chrome/browser/ui/file_locations.h:10: namespace file_locations {
style: why the namespace?

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/f...
File ios/chrome/browser/ui/file_locations.mm (right):

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/f...
ios/chrome/browser/ui/file_locations.mm:27: std::string iosLocaleMapping(const
std::string& locale);
style: this is a c++ function, so should be called ios_locale_mapping (or
iOS_locale_mapping, I don't really know what should be the spelling of iOS). Or
do not use iOS in the method name, I would recomment
system_to_chrome_locale_mapping().

nit: there is no need to forward-declare and then implement this function at the
bottom (it does not use the other function). Instead I would recommend putting
it in an anonymous namespace.

namespace {

// iOS uses certain locale ...
std::string system_to_chrome_locale_mapping(const std::string& locale) {
  ...
}

}

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/f...
ios/chrome/browser/ui/file_locations.mm:32: std::string
find_file_in_resource(const std::string& base_name,
nit: can go in the anonymous namespace.

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/f...
ios/chrome/browser/ui/file_locations.mm:53: std::string
localized_file_name(const std::string& base_name,
nit: can go in the anonymous namespace.

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/n...
File ios/chrome/browser/ui/native_content_controller.h (right):

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/n...
ios/chrome/browser/ui/native_content_controller.h:4: #ifndef
IOS_CHROME_BROWSER_UI_NATIVE_CONTENT_CONTROLLER_H_
style: empty line before #include guard

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/n...
ios/chrome/browser/ui/native_content_controller.h:18: @interface
NativeContentController : NSObject<CRWNativeContent> {
Can you use the occasion to move the ivar to the .mm file, and use the
_trailingPrefix convention we are supposed to use in Objective-C code?

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/show_privacy_settings_util.h (right):

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/show_privacy_settings_util.h:4: #ifndef
IOS_CHROME_BROWSER_UI_SHOW_PRIVACY_SETTINGS_UTIL_H_
style: empty line before #include guard

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/side_swipe_gesture_recognizer.h (right):

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/side_swipe_gesture_recognizer.h:16: @interface
SideSwipeGestureRecognizer : UIPanGestureRecognizer {
Can you use the occasion to move ivar definition to .mm file and to use
_trailingPrefix convention we should use in Objective-C files?

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/side_swipe_gesture_recognizer.mm (right):

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/side_swipe_gesture_recognizer.mm:14: CGFloat
MAX_SWIPE_Y_ANGLE = 65;
style: const CGFloat kMaxSwipeYAngleDegree = 65;

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/side_swipe_gesture_recognizer.mm:16: CGFloat
MIN_SWIPE_X_THRESHOLD = 4;
style: const CGFloat kMinSwipeXThresholdPoint = 4;

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/side_swipe_gesture_recognizer.mm:63: CGFloat degrees =
fabsf(atan2f(dy, dx) * 180 / float(M_PI));
nit: std::fabs(std::atan2(dy, dx) * 180 / CGFloat(M_PI))

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/side_swipe_gesture_recognizer.mm:79: if
(ABS(currentPoint.x - startPoint_.x) > MIN_SWIPE_X_THRESHOLD) {
nit: std::abs(currentPoint.x - startPoint.x)

[bdibello, 2015-04-02 17:34:06]

bdibello@chromium.org changed reviewers:
+ bdibello@chromium.org

[bdibello, 2015-04-02 17:34:06]

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/O...
File ios/chrome/browser/ui/OWNERS (right):

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/O...
ios/chrome/browser/ui/OWNERS:1: bdibello@chromium.org
On 2015/04/02 17:20:28, sdefresne wrote:
> bdibello: are you okay reviewing this going forward?

I'd prefer if another owner could. I was added as an owner of ui primarily for
Material changes.

[droger, 2015-04-03 08:58:43]

Patchset #2 (id:40001) has been deleted

[droger, 2015-04-03 08:59:07]

Applied most of the comments, except some below;

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/f...
File ios/chrome/browser/ui/file_locations.mm (right):

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/f...
ios/chrome/browser/ui/file_locations.mm:27: std::string iosLocaleMapping(const
std::string& locale);
On 2015/04/02 17:20:28, sdefresne wrote:
> style: this is a c++ function, so should be called ios_locale_mapping (or
> iOS_locale_mapping, I don't really know what should be the spelling of iOS).
Or
> do not use iOS in the method name, I would recomment
> system_to_chrome_locale_mapping().
> 
> nit: there is no need to forward-declare and then implement this function at
the
> bottom (it does not use the other function). Instead I would recommend putting
> it in an anonymous namespace.
> 
> namespace {
> 
> // iOS uses certain locale ...
> std::string system_to_chrome_locale_mapping(const std::string& locale) {
>   ...
> }
> 
> }

This function is used in tests, I can't put it in anonymous namespace.

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/f...
ios/chrome/browser/ui/file_locations.mm:53: std::string
localized_file_name(const std::string& base_name,
On 2015/04/02 17:20:28, sdefresne wrote:
> nit: can go in the anonymous namespace.

This function is used in tests, I can't put it in anonymous namespace.

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/n...
File ios/chrome/browser/ui/native_content_controller.h (right):

https://codereview.chromium.org/1058933002/diff/20001/ios/chrome/browser/ui/n...
ios/chrome/browser/ui/native_content_controller.h:18: @interface
NativeContentController : NSObject<CRWNativeContent> {
On 2015/04/02 17:20:28, sdefresne wrote:
> Can you use the occasion to move the ivar to the .mm file, and use the
> _trailingPrefix convention we are supposed to use in Objective-C code?

No. These are @protected and used by subclasses.
I converted to leading underscore though.

[sdefresne, 2015-04-03 09:08:04]

lgtm

[droger, 2015-04-08 14:46:51]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2015-04-08 14:47:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1058933002/60001

[commit-bot: I haz the power, 2015-04-08 15:15:51]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2015-04-08 16:05:39]

Patchset 2 (id:??) landed as
https://crrev.com/56312cdf58df182ce892e333c99c54090edd8593
Cr-Commit-Position: refs/heads/master@{#324230}