Implemented new channel switcher UI.

Implemented new channel switcher UI.

Must be landed after:
https://gerrit.chromium.org/gerrit/#/c/59422/
https://gerrit.chromium.org/gerrit/#/c/59427/

BUG=219292
TEST=tested on Linux ChromiumOS and Lumpy

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

[James Hawkins, 2013-06-20 18:23:51]

Please attach screenshots somewhere.

[ygorshenin1, 2013-06-21 10:59:07]

+ Nikita

[Nikita (slow), 2013-06-21 12:21:32]

first round of comments

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
File chrome/browser/resources/help/channel_change_page.css (right):

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/channel_change_page.css:12:
.channel-change-page-title {
nit: "Do not create a class for only one element; use the element ID instead."
http://dev.chromium.org/developers/web-development-style-guide

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
File chrome/browser/resources/help/channel_change_page.html (right):

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/channel_change_page.html:43:
i18n-content="channelChangePagePowerwashButton">
nit: Add closing tag </button>

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
File chrome/browser/resources/help/help.css (right):

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/help.css:139:
url('chrome://theme/IDR_CONTROLLED_SETTING_MANDATORY_LARGE');
nit: 4 spaces indent I guess

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
File chrome/browser/resources/help/help.html (right):

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/help.html:6: <link rel="stylesheet"
href="chrome://resources/css/bubble.css">
As discussed, please move all new links to css/js resources under chromeos
ifdef.

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/ui/webui/he...
File chrome/browser/ui/webui/help/version_updater_chromeos.cc (right):

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/version_updater_chromeos.cc:131: bool
is_powerwash_allowed) {
nit: Please fix indentation

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/version_updater_chromeos.cc:132: // On enterprise
machines we can only use SetReleaseChannel to store the
nit: use SetChannel

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/version_updater_chromeos.cc:137:
DBusThreadManager::Get()->GetUpdateEngineClient()->SetChannel(
nit: Please move SetChannel(...) on the next line.

https://codereview.chromium.org/17437004/diff/8001/chromeos/dbus/update_engin...
File chromeos/dbus/update_engine_client.cc (right):

https://codereview.chromium.org/17437004/diff/8001/chromeos/dbus/update_engin...
chromeos/dbus/update_engine_client.cc:168: dbus::MethodCall method_call(
Check somewhere that target_channel value is valid. Switching to enum is even
better.

https://codereview.chromium.org/17437004/diff/8001/chromeos/dbus/update_engin...
chromeos/dbus/update_engine_client.cc:192: 
nit: drop empty line

https://codereview.chromium.org/17437004/diff/8001/chromeos/dbus/update_engin...
chromeos/dbus/update_engine_client.cc:389: LOG(ERROR) << "Requesting to set
channel: "
LOG(INFO)?

https://codereview.chromium.org/17437004/diff/8001/chromeos/dbus/update_engin...
chromeos/dbus/update_engine_client.cc:395: LOG(ERROR) << "Requesting to get
channel, get_current_channel="
LOG(INFO)?

https://codereview.chromium.org/17437004/diff/8001/chromeos/dbus/update_engin...
File chromeos/dbus/update_engine_client.h (right):

https://codereview.chromium.org/17437004/diff/8001/chromeos/dbus/update_engin...
chromeos/dbus/update_engine_client.h:90: // DEPRECATED: Use SetChannel instead.
What is the plan to cleanup [Set|Get]ReleaseTrack(...) call?

https://codereview.chromium.org/17437004/diff/8001/chromeos/dbus/update_engin...
chromeos/dbus/update_engine_client.h:125: virtual void SetChannel(const
std::string& target_channel,
How about using enum here and only map that to [dev|beta|stable]-channel when
calling dbus method.

[Nikita (slow), 2013-06-21 12:23:13]

On 2013/06/20 18:23:51, James Hawkins wrote:
> Please attach screenshots somewhere.

+1, I'd like to see screenshots as well.

[Nikita (slow), 2013-06-21 12:58:04]

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
File chrome/browser/resources/help/channel_change_page.js (right):

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/channel_change_page.js:10:
ChannelChangePage.prototype = {
nit: Short comment describing usage is missing.

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/channel_change_page.js:35: stabilityTable_:
['dev-channel', 'beta-channel', 'stable-channel'],
nit: channelsList_ ?

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/channel_change_page.js:64: var productName =
loadTimeData.getStringF('aboutProductTitle');
You could do 64..70 in C++ and set these strings via i18 template system.

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/channel_change_page.js:117: var newClass = null;
nit: rename to newOverlayClass

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/channel_change_page.js:140: newClass =
'selected-channel-good';
Should we also notify user when switching from stable to beta?

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/channel_change_page.js:144: for (var i = 0; i <
this.uiClassTable_.length; i++) {
nit: Add comment.

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
File chrome/browser/resources/help/help.js (right):

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/help.js:110: // Create the DOM tree
nit: drop comment

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/help.js:212: return
!loadTimeData.valueExists('disableNewChannelSwitcherUI');
Where is this flag defined, passed?

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/help.js:244: $('relaunch-and-powerwash').hidden =
nit: Please add comment to clarify login when we allow doing reboot+powerwash.

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/help.js:380:
$('channel-change-page-container').hidden = true;
Not clear what exactly is left for old UI.

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/ui/webui/he...
File chrome/browser/ui/webui/help/help_handler.cc (right):

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:344:
version_updater_->GetChannel(true,
nit: Please add comments for true/false value (1st parameter).

[ygorshenin1, 2013-06-21 17:24:58]

PTAL

Nikita, James, I've shared screenshots with you:
https://drive.google.com/a/chromium.org/folderview?id=0B5OeqdgxipJ8cW1tUm5hT1...

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
File chrome/browser/resources/help/channel_change_page.css (right):

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/channel_change_page.css:12:
.channel-change-page-title {
On 2013/06/21 12:21:33, Nikita Kostylev wrote:
> nit: "Do not create a class for only one element; use the element ID instead."
> http://dev.chromium.org/developers/web-development-style-guide

Done.

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
File chrome/browser/resources/help/channel_change_page.html (right):

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/channel_change_page.html:43:
i18n-content="channelChangePagePowerwashButton">
On 2013/06/21 12:21:33, Nikita Kostylev wrote:
> nit: Add closing tag </button>

Done.

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
File chrome/browser/resources/help/channel_change_page.js (right):

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/channel_change_page.js:10:
ChannelChangePage.prototype = {
On 2013/06/21 12:58:04, Nikita Kostylev wrote:
> nit: Short comment describing usage is missing.

Done.

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/channel_change_page.js:35: stabilityTable_:
['dev-channel', 'beta-channel', 'stable-channel'],
On 2013/06/21 12:58:04, Nikita Kostylev wrote:
> nit: channelsList_ ?

Done.

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/channel_change_page.js:64: var productName =
loadTimeData.getStringF('aboutProductTitle');
On 2013/06/21 12:58:04, Nikita Kostylev wrote:
> You could do 64..70 in C++ and set these strings via i18 template system.

Done.

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/channel_change_page.js:117: var newClass = null;
On 2013/06/21 12:58:04, Nikita Kostylev wrote:
> nit: rename to newOverlayClass

Done.

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/channel_change_page.js:140: newClass =
'selected-channel-good';
As I understood from mocks, no.

On 2013/06/21 12:58:04, Nikita Kostylev wrote:
> Should we also notify user when switching from stable to beta?

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/channel_change_page.js:144: for (var i = 0; i <
this.uiClassTable_.length; i++) {
On 2013/06/21 12:58:04, Nikita Kostylev wrote:
> nit: Add comment.

Done.

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
File chrome/browser/resources/help/help.css (right):

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/help.css:139:
url('chrome://theme/IDR_CONTROLLED_SETTING_MANDATORY_LARGE');
On 2013/06/21 12:21:33, Nikita Kostylev wrote:
> nit: 4 spaces indent I guess

Done.

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
File chrome/browser/resources/help/help.html (right):

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/help.html:6: <link rel="stylesheet"
href="chrome://resources/css/bubble.css">
On 2013/06/21 12:21:33, Nikita Kostylev wrote:
> As discussed, please move all new links to css/js resources under chromeos
> ifdef.

Done.

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
File chrome/browser/resources/help/help.js (right):

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/help.js:110: // Create the DOM tree
On 2013/06/21 12:58:04, Nikita Kostylev wrote:
> nit: drop comment

Done.

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/help.js:212: return
!loadTimeData.valueExists('disableNewChannelSwitcherUI');
The flag isn't defined. This code is needed because if we'll decide to switch
back to the old switcher, corresponding CL will be as short and as simple as
possible.

On 2013/06/21 12:58:04, Nikita Kostylev wrote:
> Where is this flag defined, passed?

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/help.js:244: $('relaunch-and-powerwash').hidden =
On 2013/06/21 12:58:04, Nikita Kostylev wrote:
> nit: Please add comment to clarify login when we allow doing reboot+powerwash.

Done.

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/resources/h...
chrome/browser/resources/help/help.js:380:
$('channel-change-page-container').hidden = true;
channel-changer div contains old channel switcher UI.

On 2013/06/21 12:58:04, Nikita Kostylev wrote:
> Not clear what exactly is left for old UI.

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/ui/webui/he...
File chrome/browser/ui/webui/help/help_handler.cc (right):

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:344:
version_updater_->GetChannel(true,
On 2013/06/21 12:58:04, Nikita Kostylev wrote:
> nit: Please add comments for true/false value (1st parameter).

Done.

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/ui/webui/he...
File chrome/browser/ui/webui/help/version_updater_chromeos.cc (right):

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/version_updater_chromeos.cc:131: bool
is_powerwash_allowed) {
On 2013/06/21 12:21:33, Nikita Kostylev wrote:
> nit: Please fix indentation

Done.

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/version_updater_chromeos.cc:132: // On enterprise
machines we can only use SetReleaseChannel to store the
On 2013/06/21 12:21:33, Nikita Kostylev wrote:
> nit: use SetChannel

Done.

https://codereview.chromium.org/17437004/diff/8001/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/version_updater_chromeos.cc:137:
DBusThreadManager::Get()->GetUpdateEngineClient()->SetChannel(
On 2013/06/21 12:21:33, Nikita Kostylev wrote:
> nit: Please move SetChannel(...) on the next line.

Done.

https://codereview.chromium.org/17437004/diff/8001/chromeos/dbus/update_engin...
File chromeos/dbus/update_engine_client.cc (right):

https://codereview.chromium.org/17437004/diff/8001/chromeos/dbus/update_engin...
chromeos/dbus/update_engine_client.cc:168: dbus::MethodCall method_call(
On 2013/06/21 12:21:33, Nikita Kostylev wrote:
> Check somewhere that target_channel value is valid. Switching to enum is even
> better.

Done, enum constants will be added in a separate CL.

https://codereview.chromium.org/17437004/diff/8001/chromeos/dbus/update_engin...
chromeos/dbus/update_engine_client.cc:192: 
All other methods in this file have an empty line before dbus call.

On 2013/06/21 12:21:33, Nikita Kostylev wrote:
> nit: drop empty line

https://codereview.chromium.org/17437004/diff/8001/chromeos/dbus/update_engin...
chromeos/dbus/update_engine_client.cc:389: LOG(ERROR) << "Requesting to set
channel: "
On 2013/06/21 12:21:33, Nikita Kostylev wrote:
> LOG(INFO)?

Done.

https://codereview.chromium.org/17437004/diff/8001/chromeos/dbus/update_engin...
chromeos/dbus/update_engine_client.cc:395: LOG(ERROR) << "Requesting to get
channel, get_current_channel="
On 2013/06/21 12:21:33, Nikita Kostylev wrote:
> LOG(INFO)?

Done.

https://codereview.chromium.org/17437004/diff/8001/chromeos/dbus/update_engin...
File chromeos/dbus/update_engine_client.h (right):

https://codereview.chromium.org/17437004/diff/8001/chromeos/dbus/update_engin...
chromeos/dbus/update_engine_client.h:90: // DEPRECATED: Use SetChannel instead.
From my side is to delete them from UpdateEngineClient.

On 2013/06/21 12:21:33, Nikita Kostylev wrote:
> What is the plan to cleanup [Set|Get]ReleaseTrack(...) call?

https://codereview.chromium.org/17437004/diff/8001/chromeos/dbus/update_engin...
chromeos/dbus/update_engine_client.h:125: virtual void SetChannel(const
std::string& target_channel,
As update engine SetChannel() method requires string, mapping to enum will
require inverse mapping before dbus call. As these strings are short and passed
by a constant reference, I doesn't see any performance/readability gain.

On 2013/06/21 12:21:33, Nikita Kostylev wrote:
> How about using enum here and only map that to [dev|beta|stable]-channel when
> calling dbus method.

[Nikita (slow), 2013-06-21 22:23:39]

lgtm

[Nikita (slow), 2013-06-24 12:38:24]

On 2013/06/21 17:24:58, ygorshenin1 wrote:
> PTAL
> 
> Nikita, James, I've shared screenshots with you:
>
https://drive.google.com/a/chromium.org/folderview?id=0B5OeqdgxipJ8cW1tUm5hT1...

lgtm, as discussed:

1. please fix dialogs css to use same padding as other dialogs do.
2. please add new string for Aura shell tray menu that mentions powerwash.
current one: IDS_ASH_STATUS_TRAY_UPDATE = Restart to update
3. (separate CL) change Ash tray menu "restart to update" action so that it is
capable of doing powerwash, not only reboot.

[Nikita (slow), 2013-06-24 16:03:06]

On 2013/06/24 12:38:24, Nikita Kostylev wrote:
> On 2013/06/21 17:24:58, ygorshenin1 wrote:
> > PTAL
> > 
> > Nikita, James, I've shared screenshots with you:
> >
>
https://drive.google.com/a/chromium.org/folderview?id=0B5OeqdgxipJ8cW1tUm5hT1...
> 
> lgtm, as discussed:
> 
> 1. please fix dialogs css to use same padding as other dialogs do.
> 2. please add new string for Aura shell tray menu that mentions powerwash.
> current one: IDS_ASH_STATUS_TRAY_UPDATE = Restart to update
> 3. (separate CL) change Ash tray menu "restart to update" action so that it is
> capable of doing powerwash, not only reboot.

Include new strings:

4. "Updating your device to [channel]"
https://folio.googleplex.com/mlchan/About/channel%20switch/#/04-channelswitch...

5. "Channel changed"
https://folio.googleplex.com/mlchan/About/channel%20switch/#/05-channelswitch...

[ygorshenin1, 2013-06-24 16:59:50]

PTAL

[ygorshenin1, 2013-06-24 17:05:49]

+ Satoru for chromeos/

[satorux1, 2013-06-24 17:21:11]

chromeos/... LGTM with nits.

https://codereview.chromium.org/17437004/diff/52001/chromeos/dbus/update_engi...
File chromeos/dbus/update_engine_client.cc (right):

https://codereview.chromium.org/17437004/diff/52001/chromeos/dbus/update_engi...
chromeos/dbus/update_engine_client.cc:315: LOG(ERROR) << "Succeeded to set
channel";
This is not an error. Make it a DVLOG?

https://codereview.chromium.org/17437004/diff/52001/chromeos/dbus/update_engi...
chromeos/dbus/update_engine_client.cc:333: VLOG(1) << "The channel received: "
<< channel;
DVLOG? We usually prefer DVLOG over VLOG, as the latter increases the size of
the production binary.  But VLOG() is widely used in this file, so VLOG is fine
for consistency.

https://codereview.chromium.org/17437004/diff/52001/chromeos/dbus/update_engi...
File chromeos/dbus/update_engine_client.h (right):

https://codereview.chromium.org/17437004/diff/52001/chromeos/dbus/update_engi...
chromeos/dbus/update_engine_client.h:101: typedef base::Callback<void(const
std::string&)> GetChannelCallback;
Please add a parameter name:

typedef base::Callback<void(const std::string& channel_name)>

https://codereview.chromium.org/17437004/diff/52001/chromeos/dbus/update_engi...
chromeos/dbus/update_engine_client.h:126: bool is_powerwash_allowed) = 0;
boolean parameters like this often make callers hard to read:

  SetChannel(channel, true);  // true of what?

You might want to define an enum like POWERWASH_NONE and POWERWASH_ALLOWED

However, looking at the caller, this parameter comes from
VersionUpdaterCros::SetChannel(), which also takes a boolean parameter. Then,
introducing an enum just for this function wouldn't make sense...

[ygorshenin1, 2013-06-24 17:50:34]

Thanks!

https://codereview.chromium.org/17437004/diff/52001/chromeos/dbus/update_engi...
File chromeos/dbus/update_engine_client.cc (right):

https://codereview.chromium.org/17437004/diff/52001/chromeos/dbus/update_engi...
chromeos/dbus/update_engine_client.cc:315: LOG(ERROR) << "Succeeded to set
channel";
Renamed to VLOG() for consistency.

On 2013/06/24 17:21:12, satorux1 wrote:
> This is not an error. Make it a DVLOG?

https://codereview.chromium.org/17437004/diff/52001/chromeos/dbus/update_engi...
chromeos/dbus/update_engine_client.cc:333: VLOG(1) << "The channel received: "
<< channel;
On 2013/06/24 17:21:12, satorux1 wrote:
> DVLOG? We usually prefer DVLOG over VLOG, as the latter increases the size of
> the production binary.  But VLOG() is widely used in this file, so VLOG is
fine
> for consistency.

Done.

https://codereview.chromium.org/17437004/diff/52001/chromeos/dbus/update_engi...
File chromeos/dbus/update_engine_client.h (right):

https://codereview.chromium.org/17437004/diff/52001/chromeos/dbus/update_engi...
chromeos/dbus/update_engine_client.h:101: typedef base::Callback<void(const
std::string&)> GetChannelCallback;
On 2013/06/24 17:21:12, satorux1 wrote:
> Please add a parameter name:
> 
> typedef base::Callback<void(const std::string& channel_name)>

Done.

https://codereview.chromium.org/17437004/diff/52001/chromeos/dbus/update_engi...
chromeos/dbus/update_engine_client.h:126: bool is_powerwash_allowed) = 0;
We're planning to introduce an enum instead of strings for channel names and,
possibly, symbolic constants instead of booleans. But as we're in hurry, this
will be done in a separate CL.

On 2013/06/24 17:21:12, satorux1 wrote:
> boolean parameters like this often make callers hard to read:
> 
>   SetChannel(channel, true);  // true of what?
> 
> You might want to define an enum like POWERWASH_NONE and POWERWASH_ALLOWED
> 
> However, looking at the caller, this parameter comes from
> VersionUpdaterCros::SetChannel(), which also takes a boolean parameter. Then,
> introducing an enum just for this function wouldn't make sense...

[James Hawkins, 2013-06-24 18:13:11]

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
File chrome/browser/resources/help/channel_change_page.html (right):

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
chrome/browser/resources/help/channel_change_page.html:6: <input id="stable"
type="radio" name="channel" value="stable-channel">
"stable" as an ID is kinda dangerous (it may conflict w/ IDs from other
components).  I suggest making it more unique and specific to this component
(same for similarly-named IDs here).

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
chrome/browser/resources/help/channel_change_page.html:19: <div
class="show-when-selected-channel-requires-powerwash">
Why is this a class instead of an ID?  It appears to only be used once.

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
chrome/browser/resources/help/channel_change_page.html:36:
i18n-content="channelChangePagePowerwashButton"></button>
nit: Closing tag of wrapped element must be on a new line.  Same for other
cases.

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
File chrome/browser/resources/help/channel_change_page.js (right):

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
chrome/browser/resources/help/channel_change_page.js:23: * Name of the channel
the device is supposed to be.
"be on."?

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
chrome/browser/resources/help/channel_change_page.js:29: * True if the device is
enterprise-managed, false otherwise.
Optional nit: ", false otherwise" is superfluous.

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
chrome/browser/resources/help/channel_change_page.js:120: // Do nothing in this
case.
Why have the case listed if nothing is to be done?

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
chrome/browser/resources/help/channel_change_page.js:142: if (newOverlayClass)
Optional nit: Blank lines between blocks to increase readability.

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
File chrome/browser/resources/help/help.css (right):

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
chrome/browser/resources/help/help.css:11: z-index: 5;
Why is this necessary?

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
chrome/browser/resources/help/help.css:129: margin-left: 4px;
RTL, i.e., use -webkit-margin-start.

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
chrome/browser/resources/help/help.css:141: margin-left: 4px;
RTL

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
chrome/browser/resources/help/help.css:147: margin-left: 4px;
RTL

[ygorshenin1, 2013-06-24 18:35:31]

PTAL

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
File chrome/browser/resources/help/channel_change_page.html (right):

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
chrome/browser/resources/help/channel_change_page.html:6: <input id="stable"
type="radio" name="channel" value="stable-channel">
On 2013/06/24 18:13:12, James Hawkins wrote:
> "stable" as an ID is kinda dangerous (it may conflict w/ IDs from other
> components).  I suggest making it more unique and specific to this component
> (same for similarly-named IDs here).

Done.

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
chrome/browser/resources/help/channel_change_page.html:19: <div
class="show-when-selected-channel-requires-powerwash">
channel-change-page-powerwash-button also is visible iff
selected-channel-requires-powerwash class is set.
Also for uniform and simple UI transformations.

On 2013/06/24 18:13:12, James Hawkins wrote:
> Why is this a class instead of an ID?  It appears to only be used once.

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
chrome/browser/resources/help/channel_change_page.html:36:
i18n-content="channelChangePagePowerwashButton"></button>
On 2013/06/24 18:13:12, James Hawkins wrote:
> nit: Closing tag of wrapped element must be on a new line.  Same for other
> cases.

Done.

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
File chrome/browser/resources/help/channel_change_page.js (right):

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
chrome/browser/resources/help/channel_change_page.js:23: * Name of the channel
the device is supposed to be.
On 2013/06/24 18:13:12, James Hawkins wrote:
> "be on."?

Done.

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
chrome/browser/resources/help/channel_change_page.js:29: * True if the device is
enterprise-managed, false otherwise.
On 2013/06/24 18:13:12, James Hawkins wrote:
> Optional nit: ", false otherwise" is superfluous.

Done.

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
chrome/browser/resources/help/channel_change_page.js:120: // Do nothing in this
case.
On 2013/06/24 18:13:12, James Hawkins wrote:
> Why have the case listed if nothing is to be done?

Done.

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
chrome/browser/resources/help/channel_change_page.js:142: if (newOverlayClass)
On 2013/06/24 18:13:12, James Hawkins wrote:
> Optional nit: Blank lines between blocks to increase readability.

Done.

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
File chrome/browser/resources/help/help.css (right):

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
chrome/browser/resources/help/help.css:11: z-index: 5;
On 2013/06/24 18:13:12, James Hawkins wrote:
> Why is this necessary?

Done.

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
chrome/browser/resources/help/help.css:129: margin-left: 4px;
On 2013/06/24 18:13:12, James Hawkins wrote:
> RTL, i.e., use -webkit-margin-start.

Done.

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
chrome/browser/resources/help/help.css:141: margin-left: 4px;
On 2013/06/24 18:13:12, James Hawkins wrote:
> RTL

Done.

https://codereview.chromium.org/17437004/diff/48002/chrome/browser/resources/...
chrome/browser/resources/help/help.css:147: margin-left: 4px;
On 2013/06/24 18:13:12, James Hawkins wrote:
> RTL

Done.

[Nikita (slow), 2013-06-25 11:53:38]

James, ping.

This CL was supposed to get into branch but it seems that merge will be needed.

[James Hawkins, 2013-06-25 15:11:57]

https://codereview.chromium.org/17437004/diff/76001/chrome/browser/resources/...
File chrome/browser/resources/help/channel_change_page.css (right):

https://codereview.chromium.org/17437004/diff/76001/chrome/browser/resources/...
chrome/browser/resources/help/channel_change_page.css:24: display: none
!important;
You should be using hidden attribute in HTML instead of display: none.  Why do
you need !important?

https://codereview.chromium.org/17437004/diff/76001/chromeos/dbus/update_engi...
File chromeos/dbus/update_engine_client.cc (right):

https://codereview.chromium.org/17437004/diff/76001/chromeos/dbus/update_engi...
chromeos/dbus/update_engine_client.cc:91: // UpdateEngineClient override.
Here and elsewhere: s/override/implementation/

Though I suggest only having the "// UpdateEngineClient implementation." comment
once, then having all the impl methods below it (instead of one comment per
method).

[ygorshenin1, 2013-06-25 15:55:22]

PTAL

https://codereview.chromium.org/17437004/diff/76001/chrome/browser/resources/...
File chrome/browser/resources/help/channel_change_page.css (right):

https://codereview.chromium.org/17437004/diff/76001/chrome/browser/resources/...
chrome/browser/resources/help/channel_change_page.css:24: display: none
!important;
I'm using "display: none" due to simplicity of control of UI controls visibility
when user selects different options on the page. Otherwise, corresponding
controlling code will be a sequence of if-then statements for each UI element.
But I need "!important" because otherwise these rules will be overridden by rule
from overlay.css:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...


On 2013/06/25 15:11:58, James Hawkins wrote:
> You should be using hidden attribute in HTML instead of display: none.  Why do
> you need !important?

https://codereview.chromium.org/17437004/diff/76001/chromeos/dbus/update_engi...
File chromeos/dbus/update_engine_client.cc (right):

https://codereview.chromium.org/17437004/diff/76001/chromeos/dbus/update_engi...
chromeos/dbus/update_engine_client.cc:91: // UpdateEngineClient override.
On 2013/06/25 15:11:58, James Hawkins wrote:
> Here and elsewhere: s/override/implementation/
> 
> Though I suggest only having the "// UpdateEngineClient implementation."
comment
> once, then having all the impl methods below it (instead of one comment per
> method).

Done.

[James Hawkins, 2013-06-25 17:25:44]

lgtm

[commit-bot: I haz the power, 2013-06-25 17:34:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ygorshenin@chromium.org/17437004/89001

[commit-bot: I haz the power, 2013-06-25 17:51:26]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-25 18:09:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ygorshenin@chromium.org/17437004/101001

[commit-bot: I haz the power, 2013-06-25 19:08:54]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-06-26 09:27:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ygorshenin@chromium.org/17437004/101001

[commit-bot: I haz the power, 2013-06-26 10:10:02]

Change committed as 208679

[Isaac (away), 2013-11-04 08:28:27]

https://chromiumcodereview.appspot.com/17437004/diff/101001/chrome/browser/ui...
File chrome/browser/ui/webui/help/help_handler.cc (right):

https://chromiumcodereview.appspot.com/17437004/diff/101001/chrome/browser/ui...
chrome/browser/ui/webui/help/help_handler.cc:161: { "relaunch",
IDS_RELAUNCH_BUTTON },
Duplicate string?