Chrome OS Gaia: Set param 'chrometype' for MinuteMaid (chromebox, chromebase or chromebook)

Chrome OS Gaia: Set param 'chrometype' for MinuteMaid (chromebox, chromebase or chromebook)

BUG=459475
Committed: https://crrev.com/3906a6f9898330aae1dc86ffb6ad690e950034f1
Cr-Commit-Position: refs/heads/master@{#318032}

[Roman Sorokin (ftl), 2015-02-25 08:38:57]

rsorokin@chromium.org changed reviewers:
+ nkostylev@chromium.org

[Roman Sorokin (ftl), 2015-02-25 08:38:58]

Nikita, please review

[Nikita (slow), 2015-02-25 09:28:43]

https://codereview.chromium.org/957673003/diff/1/chrome/browser/chromeos/chro...
File chrome/browser/chromeos/chromeos_utils.cc (right):

https://codereview.chromium.org/957673003/diff/1/chrome/browser/chromeos/chro...
chrome/browser/chromeos/chromeos_utils.cc:53: return "chromebox";
nit: I suggest making these static constants and declare them in header.

https://codereview.chromium.org/957673003/diff/1/chrome/browser/chromeos/chro...
File chrome/browser/chromeos/chromeos_utils.h (right):

https://codereview.chromium.org/957673003/diff/1/chrome/browser/chromeos/chro...
chrome/browser/chromeos/chromeos_utils.h:21: std::string
GetChromeDeviceTypeForMinuteMaid();
Please rename to something like GetChromeDeviceTypeString()

https://codereview.chromium.org/957673003/diff/1/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://codereview.chromium.org/957673003/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:235:
params.SetString("chromeType", GetChromeDeviceTypeForMinuteMaid());
I suggest set this parameter only if device ID string is not empty.

[Roman Sorokin (ftl), 2015-02-25 10:28:39]

Nikita, please review

https://codereview.chromium.org/957673003/diff/1/chrome/browser/chromeos/chro...
File chrome/browser/chromeos/chromeos_utils.cc (right):

https://codereview.chromium.org/957673003/diff/1/chrome/browser/chromeos/chro...
chrome/browser/chromeos/chromeos_utils.cc:53: return "chromebox";
On 2015/02/25 09:28:43, Nikita wrote:
> nit: I suggest making these static constants and declare them in header.

Done.

https://codereview.chromium.org/957673003/diff/1/chrome/browser/chromeos/chro...
File chrome/browser/chromeos/chromeos_utils.h (right):

https://codereview.chromium.org/957673003/diff/1/chrome/browser/chromeos/chro...
chrome/browser/chromeos/chromeos_utils.h:21: std::string
GetChromeDeviceTypeForMinuteMaid();
On 2015/02/25 09:28:43, Nikita wrote:
> Please rename to something like GetChromeDeviceTypeString()

Done.

https://codereview.chromium.org/957673003/diff/1/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://codereview.chromium.org/957673003/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:235:
params.SetString("chromeType", GetChromeDeviceTypeForMinuteMaid());
On 2015/02/25 09:28:43, Nikita wrote:
> I suggest set this parameter only if device ID string is not empty.

Done.

[Nikita (slow), 2015-02-25 11:55:01]

lgtm

https://codereview.chromium.org/957673003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/chromeos_utils.cc (right):

https://codereview.chromium.org/957673003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/chromeos_utils.cc:67: NOTREACHED() << "Unknown Chrome
device type" << resource_id;
nit: you may move this to default: part of switch statement and return
chrome_device_types::kChromebook to be safe.

https://codereview.chromium.org/957673003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://codereview.chromium.org/957673003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:237: if
(device_type != chrome_device_types::kChromebook)
Not needed.

[Roman Sorokin (ftl), 2015-02-25 12:05:26]

https://codereview.chromium.org/957673003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/chromeos_utils.cc (right):

https://codereview.chromium.org/957673003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/chromeos_utils.cc:67: NOTREACHED() << "Unknown Chrome
device type" << resource_id;
On 2015/02/25 11:55:00, Nikita wrote:
> nit: you may move this to default: part of switch statement and return
> chrome_device_types::kChromebook to be safe.

Done.

https://codereview.chromium.org/957673003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://codereview.chromium.org/957673003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:237: if
(device_type != chrome_device_types::kChromebook)
On 2015/02/25 11:55:01, Nikita wrote:
> Not needed.

Done.

[Roman Sorokin (ftl), 2015-02-25 12:05:31]

The CQ bit was checked by rsorokin@chromium.org

[Roman Sorokin (ftl), 2015-02-25 12:05:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
nkostylev@chromium.org
Link to the patchset: https://codereview.chromium.org/957673003/#ps40001 (title:
"Update after review")

[commit-bot: I haz the power, 2015-02-25 12:05:53]

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

[commit-bot: I haz the power, 2015-02-25 12:58:34]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-25 12:59:12]

Patchset 3 (id:??) landed as
https://crrev.com/3906a6f9898330aae1dc86ffb6ad690e950034f1
Cr-Commit-Position: refs/heads/master@{#318032}