Add cross-platform API to get the form factor of the device

Add cross-platform API to get the form factor of the device

This API returns whether the device is a phone or a tablet for Android and iOS
(for other platforms, it always returns the device as being desktop). The new
API is placed in //ui rather than //chrome in order to make it accessible to
components.

BUG=312903
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233697

[Alexei Svitkine (slow), 2013-10-29 15:26:21]

Looks good. Can you file a new bug at http://crbug.com to track this work and
set BUG= to it?

https://codereview.chromium.org/50603005/diff/1/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://codereview.chromium.org/50603005/diff/1/chrome/chrome_common.gypi#new...
chrome/chrome_common.gypi:293: 'common/form_factor_desktop.cc',
I think we don't have gypi magic for _desktop.cc files, so you'll need to
manually exclude those from the non-ios, non-android build.

See how we do it here using the "sources!" directive:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome_brow...

(Add the same sort of thing to this file.)

https://codereview.chromium.org/50603005/diff/1/chrome/common/form_factor.h
File chrome/common/form_factor.h (right):

https://codereview.chromium.org/50603005/diff/1/chrome/common/form_factor.h#n...
chrome/common/form_factor.h:1: // Copyright
Use the following copyright blob (in all the files):

// Copyright 2013 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.

https://codereview.chromium.org/50603005/diff/1/chrome/common/form_factor.h#n...
chrome/common/form_factor.h:6: enum Study_FormFactor {
This should be a different enum than the variations Study protobuf ones.

Let's use the following names:

enum DeviceFormFactor {
  DEVICE_FORM_FACTOR_DESKTOP,
  DEVICE_FORM_FACTOR_PHONE,
  DEVICE_FORM_FACTOR_TABLET,
};

DeviceFormFactor GetDeviceFormFactor();

Also, change the files to chrome/common/device_form_factor*

(Sorry for changing my mind here since I originally told you to just use
form_factor* names).

https://codereview.chromium.org/50603005/diff/1/chrome/common/form_factor.h#n...
chrome/common/form_factor.h:12: Study_FormFactor GetFormFactor();
Needs a comment. Mention that on non-Android, non-IOS platforms,
DEVICE_FORM_FACTOR_DESKTOP is always returned.

https://codereview.chromium.org/50603005/diff/1/chrome/common/form_factor_and...
File chrome/common/form_factor_android.cc (right):

https://codereview.chromium.org/50603005/diff/1/chrome/common/form_factor_and...
chrome/common/form_factor_android.cc:3: #include "form_factor.h"
Add a blank line after this.

https://codereview.chromium.org/50603005/diff/1/chrome/common/form_factor_ios.mm
File chrome/common/form_factor_ios.mm (right):

https://codereview.chromium.org/50603005/diff/1/chrome/common/form_factor_ios...
chrome/common/form_factor_ios.mm:6: UIUserInterfaceIdiom idiom = [[UIDevice
currentDevice] userInterfaceIdiom];
This probably needs some include. You can probably google the UIDevice API and
see what header it comes from. Use try jobs to see if it compiles.

[yao, 2013-10-29 19:21:57]

Please help me to test with the trybot, cause I don't have the right to do so.
Thanks :)

https://codereview.chromium.org/50603005/diff/1/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://codereview.chromium.org/50603005/diff/1/chrome/chrome_common.gypi#new...
chrome/chrome_common.gypi:293: 'common/form_factor_desktop.cc',
On 2013/10/29 15:26:21, Alexei Svitkine wrote:
> I think we don't have gypi magic for _desktop.cc files, so you'll need to
> manually exclude those from the non-ios, non-android build.
> 
> See how we do it here using the "sources!" directive:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome_brow...
> 
> (Add the same sort of thing to this file.)

Done.

https://codereview.chromium.org/50603005/diff/1/chrome/common/form_factor.h
File chrome/common/form_factor.h (right):

https://codereview.chromium.org/50603005/diff/1/chrome/common/form_factor.h#n...
chrome/common/form_factor.h:1: // Copyright
On 2013/10/29 15:26:21, Alexei Svitkine wrote:
> Use the following copyright blob (in all the files):
> 
> // Copyright 2013 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.

Done.

https://codereview.chromium.org/50603005/diff/1/chrome/common/form_factor.h#n...
chrome/common/form_factor.h:6: enum Study_FormFactor {
On 2013/10/29 15:26:21, Alexei Svitkine wrote:
> This should be a different enum than the variations Study protobuf ones.
> 
> Let's use the following names:
> 
> enum DeviceFormFactor {
>   DEVICE_FORM_FACTOR_DESKTOP,
>   DEVICE_FORM_FACTOR_PHONE,
>   DEVICE_FORM_FACTOR_TABLET,
> };
> 
> DeviceFormFactor GetDeviceFormFactor();
> 
> Also, change the files to chrome/common/device_form_factor*
> 
> (Sorry for changing my mind here since I originally told you to just use
> form_factor* names).

Done.

https://codereview.chromium.org/50603005/diff/1/chrome/common/form_factor.h#n...
chrome/common/form_factor.h:12: Study_FormFactor GetFormFactor();
On 2013/10/29 15:26:21, Alexei Svitkine wrote:
> Needs a comment. Mention that on non-Android, non-IOS platforms,
> DEVICE_FORM_FACTOR_DESKTOP is always returned.

Done.

https://codereview.chromium.org/50603005/diff/1/chrome/common/form_factor_and...
File chrome/common/form_factor_android.cc (right):

https://codereview.chromium.org/50603005/diff/1/chrome/common/form_factor_and...
chrome/common/form_factor_android.cc:3: #include "form_factor.h"
On 2013/10/29 15:26:21, Alexei Svitkine wrote:
> Add a blank line after this.

Done.

https://codereview.chromium.org/50603005/diff/1/chrome/common/form_factor_ios.mm
File chrome/common/form_factor_ios.mm (right):

https://codereview.chromium.org/50603005/diff/1/chrome/common/form_factor_ios...
chrome/common/form_factor_ios.mm:6: UIUserInterfaceIdiom idiom = [[UIDevice
currentDevice] userInterfaceIdiom];
On 2013/10/29 15:26:21, Alexei Svitkine wrote:
> This probably needs some include. You can probably google the UIDevice API and
> see what header it comes from. Use try jobs to see if it compiles.

Done.

[Alexei Svitkine (slow), 2013-10-29 20:25:05]

I sent some try jobs and they look green (yay).

Here's some comments below. There's also an unresolved issue over email that we
need to reach a consensus on where this API will live. Stay tuned for that.

In the mean while, you can try changing your other in-progress CL to use the new
API. To do that, you should change variations_service.cc to pass it as an arg to
CreateTrialsFromSeed() on the VariationSeedProcessor.

https://codereview.chromium.org/50603005/diff/80001/chrome/common/device_form...
File chrome/common/device_form_factor.h (right):

https://codereview.chromium.org/50603005/diff/80001/chrome/common/device_form...
chrome/common/device_form_factor.h:10: Device_Form_Factor_PHONE = 1,
Enums should be all caps (other than the ones generated from all protos).

Check out Chromium C++ style guidelines:
http://www.chromium.org/developers/coding-style

https://codereview.chromium.org/50603005/diff/80001/chrome/common/device_form...
File chrome/common/device_form_factor_android.cc (right):

https://codereview.chromium.org/50603005/diff/80001/chrome/common/device_form...
chrome/common/device_form_factor_android.cc:11: if
(CommandLine::ForCurrentProcess()->HasSwitch(switches::kTabletUI)) {
There are several places that use this logic directly. Can you update them to
use the new API you're adding?

https://codereview.chromium.org/50603005/diff/80001/chrome/common/device_form...
File chrome/common/device_form_factor_desktop.cc (right):

https://codereview.chromium.org/50603005/diff/80001/chrome/common/device_form...
chrome/common/device_form_factor_desktop.cc:5: #include "device_form_factor.h"
That's a weird diff. I think you can tweak this with some git flag (so that the
other file doesn't get picked up as matching... maybe Google it).

[yao, 2013-10-29 22:44:12]

https://codereview.chromium.org/50603005/diff/80001/chrome/common/device_form...
File chrome/common/device_form_factor.h (right):

https://codereview.chromium.org/50603005/diff/80001/chrome/common/device_form...
chrome/common/device_form_factor.h:10: Device_Form_Factor_PHONE = 1,
On 2013/10/29 20:25:05, Alexei Svitkine wrote:
> Enums should be all caps (other than the ones generated from all protos).
> 
> Check out Chromium C++ style guidelines:
> http://www.chromium.org/developers/coding-style

Done.

https://codereview.chromium.org/50603005/diff/80001/chrome/common/device_form...
File chrome/common/device_form_factor_android.cc (right):

https://codereview.chromium.org/50603005/diff/80001/chrome/common/device_form...
chrome/common/device_form_factor_android.cc:11: if
(CommandLine::ForCurrentProcess()->HasSwitch(switches::kTabletUI)) {
On 2013/10/29 20:25:05, Alexei Svitkine wrote:
> There are several places that use this logic directly. Can you update them to
> use the new API you're adding?

Done.

[Alexei Svitkine (slow), 2013-10-31 14:52:31]

Some more comments. Per the discussion on the other thread, the API will have to
move to either ui/ or components/.

https://codereview.chromium.org/50603005/diff/150001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/device_info.cc (right):

https://codereview.chromium.org/50603005/diff/150001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info.cc:15: #if defined(OS_ANDROID)
No need for the ifdef here, just add this include together with the rest. (Even
if the function is only used for android, the include won't be an issue for
other platforms).

https://codereview.chromium.org/50603005/diff/150001/chrome/browser/web_resou...
File chrome/browser/web_resource/notification_promo.cc (right):

https://codereview.chromium.org/50603005/diff/150001/chrome/browser/web_resou...
chrome/browser/web_resource/notification_promo.cc:29: #if defined(OS_ANDROID)
Same comment as for device_info.cc.

https://codereview.chromium.org/50603005/diff/150001/chrome/browser/web_resou...
chrome/browser/web_resource/notification_promo.cc:64: // TODO(noyau): add
iOS-specific implementation
Nit: Address this TODO using the new API.

https://codereview.chromium.org/50603005/diff/150001/chrome/browser/web_resou...
chrome/browser/web_resource/notification_promo.cc:74: DeviceFormFactor device =
GetDeviceFormFactor();
Nit: remove to |form_factor|.

https://codereview.chromium.org/50603005/diff/150001/chrome/browser/web_resou...
chrome/browser/web_resource/notification_promo.cc:76: (device ==
DEVICE_FORM_FACTOR_TABLET? "tablet" : "phone"));
Nit: Space before ?

https://codereview.chromium.org/50603005/diff/150001/chrome/common/device_for...
File chrome/common/device_form_factor_desktop.cc (left):

https://codereview.chromium.org/50603005/diff/150001/chrome/common/device_for...
chrome/common/device_form_factor_desktop.cc:5: #import
"ui/base/cocoa/flipped_view.h"
This diff still looks weird. Maybe you can tweak git config to not do this, e.g.
via advice from:

http://stackoverflow.com/questions/7938582/how-does-git-detect-similar-files-...

[yao, 2013-11-01 14:37:59]

Could anyone help me to send the trybots? I don't have the permission to do so.
Thanks:)

https://codereview.chromium.org/50603005/diff/150001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/device_info.cc (right):

https://codereview.chromium.org/50603005/diff/150001/chrome/browser/sync/glue...
chrome/browser/sync/glue/device_info.cc:15: #if defined(OS_ANDROID)
On 2013/10/31 14:52:32, Alexei Svitkine wrote:
> No need for the ifdef here, just add this include together with the rest.
(Even
> if the function is only used for android, the include won't be an issue for
> other platforms).

Done.

https://codereview.chromium.org/50603005/diff/150001/chrome/browser/web_resou...
File chrome/browser/web_resource/notification_promo.cc (right):

https://codereview.chromium.org/50603005/diff/150001/chrome/browser/web_resou...
chrome/browser/web_resource/notification_promo.cc:29: #if defined(OS_ANDROID)
On 2013/10/31 14:52:32, Alexei Svitkine wrote:
> Same comment as for device_info.cc.

Done.

https://codereview.chromium.org/50603005/diff/150001/chrome/browser/web_resou...
chrome/browser/web_resource/notification_promo.cc:64: // TODO(noyau): add
iOS-specific implementation
On 2013/10/31 14:52:32, Alexei Svitkine wrote:
> Nit: Address this TODO using the new API.

Done.

https://codereview.chromium.org/50603005/diff/150001/chrome/browser/web_resou...
chrome/browser/web_resource/notification_promo.cc:74: DeviceFormFactor device =
GetDeviceFormFactor();
On 2013/10/31 14:52:32, Alexei Svitkine wrote:
> Nit: remove to |form_factor|.

Done.

https://codereview.chromium.org/50603005/diff/150001/chrome/browser/web_resou...
chrome/browser/web_resource/notification_promo.cc:76: (device ==
DEVICE_FORM_FACTOR_TABLET? "tablet" : "phone"));
On 2013/10/31 14:52:32, Alexei Svitkine wrote:
> Nit: Space before ?

Done.

[Alexei Svitkine (slow), 2013-11-01 18:48:26]

Looks very good, I think we're almost ready to send this out for OWNERS review,
just a few more comments from my side.

https://codereview.chromium.org/50603005/diff/310002/ui/base/device_form_fact...
File ui/base/device_form_factor.h (right):

https://codereview.chromium.org/50603005/diff/310002/ui/base/device_form_fact...
ui/base/device_form_factor.h:7: 
Add "namespace ui {" and "}  // namespace ui" at the end. Here and in the
implementation files.

(Callers will then need to use that prefix - e.g. ui::GetDeviceFormFactor() ==
ui::DEVICE_FORM_FACTOR_DESKTOP).

https://codereview.chromium.org/50603005/diff/310002/ui/base/ui_base_switches.cc
File ui/base/ui_base_switches.cc (right):

https://codereview.chromium.org/50603005/diff/310002/ui/base/ui_base_switches...
ui/base/ui_base_switches.cc:54: const char kTabletUI[] = "tablet-ui";
This should be ifdef android.

https://codereview.chromium.org/50603005/diff/310002/ui/base/ui_base_switches.h
File ui/base/ui_base_switches.h (right):

https://codereview.chromium.org/50603005/diff/310002/ui/base/ui_base_switches...
ui/base/ui_base_switches.h:27: UI_EXPORT extern const char kTabletUI[];
ifdef android, maybe move below all the others.

[yao, 2013-11-04 15:56:19]

https://codereview.chromium.org/50603005/diff/310002/ui/base/device_form_fact...
File ui/base/device_form_factor.h (right):

https://codereview.chromium.org/50603005/diff/310002/ui/base/device_form_fact...
ui/base/device_form_factor.h:7: 
On 2013/11/01 18:48:27, Alexei Svitkine wrote:
> Add "namespace ui {" and "}  // namespace ui" at the end. Here and in the
> implementation files.
> 
> (Callers will then need to use that prefix - e.g. ui::GetDeviceFormFactor() ==
> ui::DEVICE_FORM_FACTOR_DESKTOP).

Done.

https://codereview.chromium.org/50603005/diff/310002/ui/base/ui_base_switches.cc
File ui/base/ui_base_switches.cc (right):

https://codereview.chromium.org/50603005/diff/310002/ui/base/ui_base_switches...
ui/base/ui_base_switches.cc:54: const char kTabletUI[] = "tablet-ui";
On 2013/11/01 18:48:27, Alexei Svitkine wrote:
> This should be ifdef android.

Done.

https://codereview.chromium.org/50603005/diff/310002/ui/base/ui_base_switches.h
File ui/base/ui_base_switches.h (right):

https://codereview.chromium.org/50603005/diff/310002/ui/base/ui_base_switches...
ui/base/ui_base_switches.h:27: UI_EXPORT extern const char kTabletUI[];
On 2013/11/01 18:48:27, Alexei Svitkine wrote:
> ifdef android, maybe move below all the others.

Done.

[Alexei Svitkine (slow), 2013-11-04 16:08:51]

Couple more comments.

https://codereview.chromium.org/50603005/diff/150001/chrome/common/device_for...
File chrome/common/device_form_factor_desktop.cc (left):

https://codereview.chromium.org/50603005/diff/150001/chrome/common/device_for...
chrome/common/device_form_factor_desktop.cc:5: #import
"ui/base/cocoa/flipped_view.h"
On 2013/10/31 14:52:32, Alexei Svitkine wrote:
> This diff still looks weird. Maybe you can tweak git config to not do this,
e.g.
> via advice from:
> 
>
http://stackoverflow.com/questions/7938582/how-does-git-detect-similar-files-...

Ping.

https://codereview.chromium.org/50603005/diff/490001/chrome/browser/web_resou...
File chrome/browser/web_resource/notification_promo.cc (right):

https://codereview.chromium.org/50603005/diff/490001/chrome/browser/web_resou...
chrome/browser/web_resource/notification_promo.cc:70: #elif defined(OS_ANDROID)
Can you move this block next to OS_IOS (how about before it)?

It occurred to me that if android builds also define OS_LINUX, then this block
never gets hit. (Maybe they don't so it's correct, but better to just move this
block to be sure. Also, having it next to the iOS block makes it easier to see
that they have the same logic.)

https://codereview.chromium.org/50603005/diff/490001/chrome/browser/web_resou...
chrome/browser/web_resource/notification_promo.cc:72: return
(std::string("android-") +
Nit: remove ( before std::string (and at the end), so that it's consistent with
iOS case above.

https://codereview.chromium.org/50603005/diff/490001/ui/base/device_form_fact...
File ui/base/device_form_factor.h (right):

https://codereview.chromium.org/50603005/diff/490001/ui/base/device_form_fact...
ui/base/device_form_factor.h:18: UI_EXPORT DeviceFormFactor
GetDeviceFormFactor();
Add a short comment - mention that for platforms other than Android and iOS
always return DEVICE_FORM_FACTOR_DESKTOP.

https://codereview.chromium.org/50603005/diff/490001/ui/base/device_form_fact...
File ui/base/device_form_factor_android.cc (right):

https://codereview.chromium.org/50603005/diff/490001/ui/base/device_form_fact...
ui/base/device_form_factor_android.cc:13: if
(CommandLine::ForCurrentProcess()->HasSwitch(switches::kTabletUI)) {
Nit: Remove {}'s, since this is a 1-liner.

https://codereview.chromium.org/50603005/diff/490001/ui/base/device_form_fact...
File ui/base/device_form_factor_ios.mm (right):

https://codereview.chromium.org/50603005/diff/490001/ui/base/device_form_fact...
ui/base/device_form_factor_ios.mm:13: if (idiom == UIUserInterfaceIdiomPad) {
Nit: Remove {}'s since this is a 1-liner.

https://codereview.chromium.org/50603005/diff/490001/ui/base/ui_base_switches.cc
File ui/base/ui_base_switches.cc (right):

https://codereview.chromium.org/50603005/diff/490001/ui/base/ui_base_switches...
ui/base/ui_base_switches.cc:53: 
Nit: Remove extra line you added here.

[yao, 2013-11-04 19:38:28]

https://codereview.chromium.org/50603005/diff/490001/chrome/browser/web_resou...
File chrome/browser/web_resource/notification_promo.cc (right):

https://codereview.chromium.org/50603005/diff/490001/chrome/browser/web_resou...
chrome/browser/web_resource/notification_promo.cc:70: #elif defined(OS_ANDROID)
On 2013/11/04 16:08:51, Alexei Svitkine wrote:
> Can you move this block next to OS_IOS (how about before it)?
> 
> It occurred to me that if android builds also define OS_LINUX, then this block
> never gets hit. (Maybe they don't so it's correct, but better to just move
this
> block to be sure. Also, having it next to the iOS block makes it easier to see
> that they have the same logic.)

Done.

https://codereview.chromium.org/50603005/diff/490001/chrome/browser/web_resou...
chrome/browser/web_resource/notification_promo.cc:72: return
(std::string("android-") +
On 2013/11/04 16:08:51, Alexei Svitkine wrote:
> Nit: remove ( before std::string (and at the end), so that it's consistent
with
> iOS case above.

Done.

https://codereview.chromium.org/50603005/diff/490001/ui/base/device_form_fact...
File ui/base/device_form_factor.h (right):

https://codereview.chromium.org/50603005/diff/490001/ui/base/device_form_fact...
ui/base/device_form_factor.h:18: UI_EXPORT DeviceFormFactor
GetDeviceFormFactor();
On 2013/11/04 16:08:51, Alexei Svitkine wrote:
> Add a short comment - mention that for platforms other than Android and iOS
> always return DEVICE_FORM_FACTOR_DESKTOP.

Done.

https://codereview.chromium.org/50603005/diff/490001/ui/base/device_form_fact...
File ui/base/device_form_factor_android.cc (right):

https://codereview.chromium.org/50603005/diff/490001/ui/base/device_form_fact...
ui/base/device_form_factor_android.cc:13: if
(CommandLine::ForCurrentProcess()->HasSwitch(switches::kTabletUI)) {
On 2013/11/04 16:08:51, Alexei Svitkine wrote:
> Nit: Remove {}'s, since this is a 1-liner.

Done.

https://codereview.chromium.org/50603005/diff/490001/ui/base/device_form_fact...
File ui/base/device_form_factor_ios.mm (right):

https://codereview.chromium.org/50603005/diff/490001/ui/base/device_form_fact...
ui/base/device_form_factor_ios.mm:13: if (idiom == UIUserInterfaceIdiomPad) {
On 2013/11/04 16:08:51, Alexei Svitkine wrote:
> Nit: Remove {}'s since this is a 1-liner.

Done.

https://codereview.chromium.org/50603005/diff/490001/ui/base/ui_base_switches.cc
File ui/base/ui_base_switches.cc (right):

https://codereview.chromium.org/50603005/diff/490001/ui/base/ui_base_switches...
ui/base/ui_base_switches.cc:53: 
On 2013/11/04 16:08:51, Alexei Svitkine wrote:
> Nit: Remove extra line you added here.

Done.

[Alexei Svitkine (slow), 2013-11-04 19:44:31]

LGTM with a few final nits.

Now, you should add some OWNERS for the directories this is changing code in
(sky@ for ui/ and find a person each for sync, ntp and promo).

https://codereview.chromium.org/50603005/diff/740001/ui/base/device_form_fact...
File ui/base/device_form_factor.h (right):

https://codereview.chromium.org/50603005/diff/740001/ui/base/device_form_fact...
ui/base/device_form_factor.h:22: } // namespace ui
Nit: 2 spaces before // (add an extra space)

Please fix throughout.

https://codereview.chromium.org/50603005/diff/740001/ui/base/ui_base_switches.cc
File ui/base/ui_base_switches.cc (right):

https://codereview.chromium.org/50603005/diff/740001/ui/base/ui_base_switches...
ui/base/ui_base_switches.cc:71: // Uses the tablet specific UI components when
available
Nit: add a .

[yao, 2013-11-04 20:23:26]

We are adding an API that returns the form factor (phone, table or desktop) of
current device. PTAL. Thanks!

tedchoc@chromium.org: ntp_resource_cache_android.cc
tim@chromium.org: device_info.cc
achuith@chromium.org: notification_promo.cc
sky@chromium.org: files under ui/base
colinc@chromium.org: device_form_factor_ios.mm

https://codereview.chromium.org/50603005/diff/740001/ui/base/device_form_fact...
File ui/base/device_form_factor.h (right):

https://codereview.chromium.org/50603005/diff/740001/ui/base/device_form_fact...
ui/base/device_form_factor.h:22: } // namespace ui
On 2013/11/04 19:44:32, Alexei Svitkine wrote:
> Nit: 2 spaces before // (add an extra space)
> 
> Please fix throughout.

Done.

https://codereview.chromium.org/50603005/diff/740001/ui/base/ui_base_switches.cc
File ui/base/ui_base_switches.cc (right):

https://codereview.chromium.org/50603005/diff/740001/ui/base/ui_base_switches...
ui/base/ui_base_switches.cc:71: // Uses the tablet specific UI components when
available
On 2013/11/04 19:44:32, Alexei Svitkine wrote:
> Nit: add a .

Done.

[achuithb, 2013-11-04 20:54:51]

Ruslan should review notification_promo.cc

[aruslan, 2013-11-04 20:59:38]

notification_promo.cc LGTM, thanks!

[Ted C, 2013-11-04 21:17:22]

On 2013/11/04 20:59:38, aruslan wrote:
> notification_promo.cc LGTM, thanks!

ntp_resource_cache_android.cc lgtm

[Yaron, 2013-11-04 21:25:14]

+blundell who had a recent thread on this for chrome on ios needs. He should
probably weigh in on whether this fits with other in this area.

[sky, 2013-11-04 21:45:53]

LGTM

[blundell, 2013-11-05 15:03:56]

LGTM with satisfactory resolution to the question I've posed to noyau@ (note
that if the change isn't safe, you can just undo that part of the CL).

Eric, is the change to notification_promo.cc safe?

I took the liberty of updating the CL description.

https://codereview.chromium.org/50603005/diff/970001/chrome/browser/web_resou...
File chrome/browser/web_resource/notification_promo.cc (right):

https://codereview.chromium.org/50603005/diff/970001/chrome/browser/web_resou...
chrome/browser/web_resource/notification_promo.cc:65: ui::DeviceFormFactor
form_factor = ui::GetDeviceFormFactor();
noyau: Is this a safe change to make at the current time?

[blundell, 2013-11-05 15:31:51]

rsesek:

noyau@ says that you would know whether the change to notification_promo is safe
(note that with this change, the notification promo would start getting platform
strings with "ios-phone", which it to date has not been). Could you comment?

Thanks,

Colin

[noyau (Ping after 24h), 2013-11-05 15:48:02]

https://codereview.chromium.org/50603005/diff/970001/chrome/browser/web_resou...
File chrome/browser/web_resource/notification_promo.cc (right):

https://codereview.chromium.org/50603005/diff/970001/chrome/browser/web_resou...
chrome/browser/web_resource/notification_promo.cc:65: ui::DeviceFormFactor
form_factor = ui::GetDeviceFormFactor();
On 2013/11/05 15:03:57, blundell wrote:
> noyau: Is this a safe change to make at the current time?

Probably safe. We need to check with the promo server folks that they can deal
with this and we're good to go.

https://codereview.chromium.org/50603005/diff/970001/ui/base/device_form_fact...
File ui/base/device_form_factor_ios.mm (right):

https://codereview.chromium.org/50603005/diff/970001/ui/base/device_form_fact...
ui/base/device_form_factor_ios.mm:12: UIUserInterfaceIdiom idiom = [[UIDevice
currentDevice] userInterfaceIdiom];
We currently use ios_ui_util::iPadIdiom() all over the place to to this check.

We should pick one or the other to contain the canonical implementation and have
the other one calling it. As the implementation in this file is cross-platform I
suggest changing ios_ui_util::iPadIdiom() to call it.

[Alexei Svitkine (slow), 2013-11-05 16:27:28]

https://codereview.chromium.org/50603005/diff/970001/ui/base/device_form_fact...
File ui/base/device_form_factor_ios.mm (right):

https://codereview.chromium.org/50603005/diff/970001/ui/base/device_form_fact...
ui/base/device_form_factor_ios.mm:12: UIUserInterfaceIdiom idiom = [[UIDevice
currentDevice] userInterfaceIdiom];
On 2013/11/05 15:48:03, noyau wrote:
> We currently use ios_ui_util::iPadIdiom() all over the place to to this check.
> 
> We should pick one or the other to contain the canonical implementation and
have
> the other one calling it. As the implementation in this file is cross-platform
I
> suggest changing ios_ui_util::iPadIdiom() to call it.

Agreed. It will need to be done separately once this lands, since
ios_ui_util::iPadIdiom() is not in the upstream repo.

[blundell, 2013-11-05 16:28:11]

On 2013/11/05 16:27:28, Alexei Svitkine wrote:
>
https://codereview.chromium.org/50603005/diff/970001/ui/base/device_form_fact...
> File ui/base/device_form_factor_ios.mm (right):
> 
>
https://codereview.chromium.org/50603005/diff/970001/ui/base/device_form_fact...
> ui/base/device_form_factor_ios.mm:12: UIUserInterfaceIdiom idiom = [[UIDevice
> currentDevice] userInterfaceIdiom];
> On 2013/11/05 15:48:03, noyau wrote:
> > We currently use ios_ui_util::iPadIdiom() all over the place to to this
check.
> > 
> > We should pick one or the other to contain the canonical implementation and
> have
> > the other one calling it. As the implementation in this file is
cross-platform
> I
> > suggest changing ios_ui_util::iPadIdiom() to call it.
> 
> Agreed. It will need to be done separately once this lands, since
> ios_ui_util::iPadIdiom() is not in the upstream repo.

Even better, we could just kill that one and use only this one.

[aruslan, 2013-11-05 16:35:36]

https://codereview.chromium.org/50603005/diff/970001/chrome/browser/web_resou...
File chrome/browser/web_resource/notification_promo.cc (right):

https://codereview.chromium.org/50603005/diff/970001/chrome/browser/web_resou...
chrome/browser/web_resource/notification_promo.cc:65: ui::DeviceFormFactor
form_factor = ui::GetDeviceFormFactor();
On 2013/11/05 15:48:03, noyau wrote:
> On 2013/11/05 15:03:57, blundell wrote:
> > noyau: Is this a safe change to make at the current time?
> 
> Probably safe. We need to check with the promo server folks that they can deal
> with this and we're good to go.

Server-wise it is safe, supported and would enable the promo on tablets.
ios-phone:
https://clients3.google.com/crsignal/client?hl=en-US&dist=stable&branding=31....
ios-tablet:
https://clients3.google.com/crsignal/client?hl=en-US&dist=stable&branding=31....

noyau@ -- if you want the promo to be disabled on iPads please let me know.

[noyau1, 2013-11-05 16:51:54]

On Tue, Nov 5, 2013 at 5:35 PM, <aruslan@chromium.org> wrote:

> [...]
> noyau@ -- if you want the promo to be disabled on iPads please let me
> know.
>
No, no need to. The code presenting the promo is platform agnostic.

-- Eric

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[blundell, 2013-11-05 16:53:13]

LGTM

[Robert Sesek, 2013-11-05 17:48:28]

On 2013/11/05 16:35:36, aruslan wrote:
>
https://codereview.chromium.org/50603005/diff/970001/chrome/browser/web_resou...
> File chrome/browser/web_resource/notification_promo.cc (right):
> 
>
https://codereview.chromium.org/50603005/diff/970001/chrome/browser/web_resou...
> chrome/browser/web_resource/notification_promo.cc:65: ui::DeviceFormFactor
> form_factor = ui::GetDeviceFormFactor();
> On 2013/11/05 15:48:03, noyau wrote:
> > On 2013/11/05 15:03:57, blundell wrote:
> > > noyau: Is this a safe change to make at the current time?
> > 
> > Probably safe. We need to check with the promo server folks that they can
deal
> > with this and we're good to go.
> 
> Server-wise it is safe, supported and would enable the promo on tablets.
> ios-phone:
>
https://clients3.google.com/crsignal/client?hl=en-US&dist=stable&branding=31....
> ios-tablet:
>
https://clients3.google.com/crsignal/client?hl=en-US&dist=stable&branding=31....
> 
> noyau@ -- if you want the promo to be disabled on iPads please let me know.

From the diff, it was already sending ios-{phone,tablet} and the promoserver
already has those targets. I think this is a noop from the promoserver
perspective.

[blundell, 2013-11-05 21:04:08]

On 2013/11/05 17:48:28, rsesek wrote:
> On 2013/11/05 16:35:36, aruslan wrote:
> >
>
https://codereview.chromium.org/50603005/diff/970001/chrome/browser/web_resou...
> > File chrome/browser/web_resource/notification_promo.cc (right):
> > 
> >
>
https://codereview.chromium.org/50603005/diff/970001/chrome/browser/web_resou...
> > chrome/browser/web_resource/notification_promo.cc:65: ui::DeviceFormFactor
> > form_factor = ui::GetDeviceFormFactor();
> > On 2013/11/05 15:48:03, noyau wrote:
> > > On 2013/11/05 15:03:57, blundell wrote:
> > > > noyau: Is this a safe change to make at the current time?
> > > 
> > > Probably safe. We need to check with the promo server folks that they can
> deal
> > > with this and we're good to go.
> > 
> > Server-wise it is safe, supported and would enable the promo on tablets.
> > ios-phone:
> >
>
https://clients3.google.com/crsignal/client?hl=en-US&dist=stable&branding=31....
> > ios-tablet:
> >
>
https://clients3.google.com/crsignal/client?hl=en-US&dist=stable&branding=31....
> > 
> > noyau@ -- if you want the promo to be disabled on iPads please let me know.
> 
> From the diff, it was already sending ios-{phone,tablet} and the promoserver
> already has those targets. I think this is a noop from the promoserver
> perspective.

It's currently never sending ios-tablet, since the |isTablet| bool is hardcoded
to false.

[Robert Sesek, 2013-11-05 21:08:46]

On 2013/11/05 21:04:08, blundell wrote:
> On 2013/11/05 17:48:28, rsesek wrote:
> > On 2013/11/05 16:35:36, aruslan wrote:
> > >
> >
>
https://codereview.chromium.org/50603005/diff/970001/chrome/browser/web_resou...
> > > File chrome/browser/web_resource/notification_promo.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/50603005/diff/970001/chrome/browser/web_resou...
> > > chrome/browser/web_resource/notification_promo.cc:65: ui::DeviceFormFactor
> > > form_factor = ui::GetDeviceFormFactor();
> > > On 2013/11/05 15:48:03, noyau wrote:
> > > > On 2013/11/05 15:03:57, blundell wrote:
> > > > > noyau: Is this a safe change to make at the current time?
> > > > 
> > > > Probably safe. We need to check with the promo server folks that they
can
> > deal
> > > > with this and we're good to go.
> > > 
> > > Server-wise it is safe, supported and would enable the promo on tablets.
> > > ios-phone:
> > >
> >
>
https://clients3.google.com/crsignal/client?hl=en-US&dist=stable&branding=31....
> > > ios-tablet:
> > >
> >
>
https://clients3.google.com/crsignal/client?hl=en-US&dist=stable&branding=31....
> > > 
> > > noyau@ -- if you want the promo to be disabled on iPads please let me
know.
> > 
> > From the diff, it was already sending ios-{phone,tablet} and the promoserver
> > already has those targets. I think this is a noop from the promoserver
> > perspective.
> 
> It's currently never sending ios-tablet, since the |isTablet| bool is
hardcoded
> to false.

OK. But the server is ready to accept that.

[tim (not reviewing), 2013-11-06 21:41:53]

On 2013/11/05 21:08:46, rsesek wrote:
> On 2013/11/05 21:04:08, blundell wrote:
> > On 2013/11/05 17:48:28, rsesek wrote:
> > > On 2013/11/05 16:35:36, aruslan wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/50603005/diff/970001/chrome/browser/web_resou...
> > > > File chrome/browser/web_resource/notification_promo.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/50603005/diff/970001/chrome/browser/web_resou...
> > > > chrome/browser/web_resource/notification_promo.cc:65:
ui::DeviceFormFactor
> > > > form_factor = ui::GetDeviceFormFactor();
> > > > On 2013/11/05 15:48:03, noyau wrote:
> > > > > On 2013/11/05 15:03:57, blundell wrote:
> > > > > > noyau: Is this a safe change to make at the current time?
> > > > > 
> > > > > Probably safe. We need to check with the promo server folks that they
> can
> > > deal
> > > > > with this and we're good to go.
> > > > 
> > > > Server-wise it is safe, supported and would enable the promo on tablets.
> > > > ios-phone:
> > > >
> > >
> >
>
https://clients3.google.com/crsignal/client?hl=en-US&dist=stable&branding=31....
> > > > ios-tablet:
> > > >
> > >
> >
>
https://clients3.google.com/crsignal/client?hl=en-US&dist=stable&branding=31....
> > > > 
> > > > noyau@ -- if you want the promo to be disabled on iPads please let me
> know.
> > > 
> > > From the diff, it was already sending ios-{phone,tablet} and the
promoserver
> > > already has those targets. I think this is a noop from the promoserver
> > > perspective.
> > 
> > It's currently never sending ios-tablet, since the |isTablet| bool is
> hardcoded
> > to false.
> 
> OK. But the server is ready to accept that.

Sync LGTM

[commit-bot: I haz the power, 2013-11-07 18:52:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yiyaoliu@chromium.org/50603005/970001

[commit-bot: I haz the power, 2013-11-07 21:52:12]

Change committed as 233697