MD Settings: About page, implementing channel switcher dialog.

MD Settings: About page, implementing channel switcher dialog.

This dialog allows the user to switch between stable, beta and dev channels.
Will be hooked up in follow up CL.

BUG=546841
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/289aedeff070843ff814f8b6571be4be7e11fba0
Cr-Commit-Position: refs/heads/master@{#396370}

[dpapad, 2016-05-25 19:31:00]

Description was changed from

==========
MD Settings: About page, implementing channel switcher dialog.

This dialog allows the user to switch between stable, beta and dev channels.
Will be hooked up in follow up CL.

BUG=546841
==========

to

==========
MD Settings: About page, implementing channel switcher dialog.

This dialog allows the user to switch between stable, beta and dev channels.
Will be hooked up in follow up CL.

BUG=546841
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-05-25 23:30:00]

https://codereview.chromium.org/2010653003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/detailed_build_info.html
(right):

https://codereview.chromium.org/2010653003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/detailed_build_info.html:3: <link
rel="import"
href="chrome://md-settings/about_page/channel_switcher_dialog.html">
Even though <settings-channel-switcher-dialog> is not hooked up yet, this is
necessary for the about_page_tests.js to be able to test the new element.

[dpapad, 2016-05-25 23:30:09]

Patchset #2 (id:20001) has been deleted

[dpapad, 2016-05-25 23:30:13]

Patchset #1 (id:1) has been deleted

[dpapad, 2016-05-26 00:14:32]

Patchset #1 (id:40001) has been deleted

[dpapad, 2016-05-26 00:14:37]

Patchset #1 (id:60001) has been deleted

[dpapad, 2016-05-26 00:18:02]

dpapad@chromium.org changed reviewers:
+ dschuyler@chromium.org

[dpapad, 2016-05-26 00:18:03]

Screenshots at http://imgur.com/a/7Bree.

https://codereview.chromium.org/2010653003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/detailed_build_info.html
(right):

https://codereview.chromium.org/2010653003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/detailed_build_info.html:3: <link
rel="import"
href="chrome://md-settings/about_page/channel_switcher_dialog.html">
Even though <settings-channel-switcher-dialog> is not hooked up yet, this is
necessary for the about_page_tests.js to be able to test the new element.

[dschuyler, 2016-05-26 01:35:50]

https://codereview.chromium.org/2010653003/diff/80001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2010653003/diff/80001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:47: <message
name="IDS_SETTINGS_ABOUT_PAGE_CHANGE_CHANNEL_AND_POWERWASH" desc="The label for
the button for channel switch and powerwash.">
optional:
Several of these descriptions could be 
less 'wordy'. E.g. instead of the above:
"Button label for channel switch and powerwash."

https://codereview.chromium.org/2010653003/diff/80001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:53: <message
name="IDS_SETTINGS_ABOUT_PAGE_DELAYED_WARNING_TITLE" desc="The title for the
message that notifies user that channel change will be applied later.">
optional:
"Title for the message that the channel change will be applied later."

https://codereview.chromium.org/2010653003/diff/80001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:65: <message
name="IDS_SETTINGS_ABOUT_PAGE_UNSTABLE_WARNING_MESSAGE" desc="Message that warns
user about switching to developer (unstable) channel.">
optional:
"Warning about switching to developer (unstable) channel."

https://codereview.chromium.org/2010653003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/channel_switcher_dialog.html
(right):

https://codereview.chromium.org/2010653003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/channel_switcher_dialog.html:7:
<link rel="import" href="chrome://md-settings/settings_dialog.html">
Since "settings-shared" is mentioned below,
we should import settings_shared_css.html.

https://codereview.chromium.org/2010653003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/channel_switcher_dialog.js
(right):

https://codereview.chromium.org/2010653003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/channel_switcher_dialog.js:109: }
I'm wondering if the above checks are needed. It seems like a good
thing to mention in the docs (that only one should be enabled at
a time). It could also make a good unit test.

Since the function is private and only used in a few places, with 
hard coded params, could we safely go without this check?

https://codereview.chromium.org/2010653003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/2010653003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:162:
IDS_SETTINGS_ABOUT_PAGE_CHANGE_CHANNEL},
nit: this could unwrap, if desired.

[dpapad, 2016-05-26 17:58:55]

https://codereview.chromium.org/2010653003/diff/80001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2010653003/diff/80001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:47: <message
name="IDS_SETTINGS_ABOUT_PAGE_CHANGE_CHANNEL_AND_POWERWASH" desc="The label for
the button for channel switch and powerwash.">
On 2016/05/26 at 01:35:50, dschuyler wrote:
> optional:
> Several of these descriptions could be 
> less 'wordy'. E.g. instead of the above:
> "Button label for channel switch and powerwash."

Done. I had simply copied all the descriptions from the old Options existing
strings.

https://codereview.chromium.org/2010653003/diff/80001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:53: <message
name="IDS_SETTINGS_ABOUT_PAGE_DELAYED_WARNING_TITLE" desc="The title for the
message that notifies user that channel change will be applied later.">
On 2016/05/26 at 01:35:49, dschuyler wrote:
> optional:
> "Title for the message that the channel change will be applied later."

Done.

https://codereview.chromium.org/2010653003/diff/80001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:65: <message
name="IDS_SETTINGS_ABOUT_PAGE_UNSTABLE_WARNING_MESSAGE" desc="Message that warns
user about switching to developer (unstable) channel.">
On 2016/05/26 at 01:35:50, dschuyler wrote:
> optional:
> "Warning about switching to developer (unstable) channel."

Done.

https://codereview.chromium.org/2010653003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/channel_switcher_dialog.html
(right):

https://codereview.chromium.org/2010653003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/channel_switcher_dialog.html:7:
<link rel="import" href="chrome://md-settings/settings_dialog.html">
On 2016/05/26 at 01:35:50, dschuyler wrote:
> Since "settings-shared" is mentioned below,
> we should import settings_shared_css.html.

Done.

https://codereview.chromium.org/2010653003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/channel_switcher_dialog.js
(right):

https://codereview.chromium.org/2010653003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/channel_switcher_dialog.js:109: }
On 2016/05/26 at 01:35:50, dschuyler wrote:
> I'm wondering if the above checks are needed. It seems like a good
> thing to mention in the docs (that only one should be enabled at
> a time). It could also make a good unit test.
> 
> Since the function is private and only used in a few places, with 
> hard coded params, could we safely go without this check?

I already added unit tests to ensure that current code only shows at most one
button at a time. This check here is not strictly needed, it is mostly here to
1) Defend against a future breakage (meaning a future codepath that is not
covered by existing unit tests).
2) Communicate clearly the original intent.

I am on the fence too about asserting VS commenting. My concern with documenting
an invariant in a comment, is that it relies on a future dev carefully reading
the comment. A failed assertion on the other hand draws attention to itself even
if the developer misses it. FWIW, the old code simply had a comment, see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res....

https://codereview.chromium.org/2010653003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/2010653003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:162:
IDS_SETTINGS_ABOUT_PAGE_CHANGE_CHANNEL},
On 2016/05/26 at 01:35:50, dschuyler wrote:
> nit: this could unwrap, if desired.

Done. I just ran clang-format on this file, instead of adjusting this one line
manually.

[dschuyler, 2016-05-26 23:32:09]

LGTM (with some import path changes).

https://codereview.chromium.org/2010653003/diff/100001/chrome/app/settings_st...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2010653003/diff/100001/chrome/app/settings_st...
chrome/app/settings_strings.grdp:797: <message name="IDS_SETTINGS_SITE_SETTINGS"
desc="Text of the button that takes a user to settings page thats allows users
to modify site settings. Also the title of that settings page.">
Optional: s/thats/that/
(do feel free to skip this suggestion).

https://codereview.chromium.org/2010653003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/about_page/channel_switcher_dialog.html
(right):

https://codereview.chromium.org/2010653003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/channel_switcher_dialog.html:8:
<link rel="import" href="chrome://md-settings/settings_shared_css.html">
These three imports should change to relative paths
(without chrome://md-settings).

https://codereview.chromium.org/2010653003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/about_page/detailed_build_info.html
(right):

https://codereview.chromium.org/2010653003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/detailed_build_info.html:3: <link
rel="import"
href="chrome://md-settings/about_page/channel_switcher_dialog.html">
Nit: this should change to remove the 
chrome://md-settings portion of the href.

[dpapad, 2016-05-27 01:26:18]

https://codereview.chromium.org/2010653003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/about_page/channel_switcher_dialog.html
(right):

https://codereview.chromium.org/2010653003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/channel_switcher_dialog.html:8:
<link rel="import" href="chrome://md-settings/settings_shared_css.html">
On 2016/05/26 at 23:32:09, dschuyler wrote:
> These three imports should change to relative paths
> (without chrome://md-settings).

Done.

https://codereview.chromium.org/2010653003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/about_page/detailed_build_info.html
(right):

https://codereview.chromium.org/2010653003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/detailed_build_info.html:3: <link
rel="import"
href="chrome://md-settings/about_page/channel_switcher_dialog.html">
On 2016/05/26 at 23:32:09, dschuyler wrote:
> Nit: this should change to remove the 
> chrome://md-settings portion of the href.

Done.

[dpapad, 2016-05-27 01:26:24]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-05-27 01:26:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/2010653003/#ps120001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-05-27 01:27:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2010653003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2010653003/120001

[commit-bot: I haz the power, 2016-05-27 03:04:51]

Committed patchset #3 (id:120001)

[commit-bot: I haz the power, 2016-05-27 03:06:08]

Description was changed from

==========
MD Settings: About page, implementing channel switcher dialog.

This dialog allows the user to switch between stable, beta and dev channels.
Will be hooked up in follow up CL.

BUG=546841
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: About page, implementing channel switcher dialog.

This dialog allows the user to switch between stable, beta and dev channels.
Will be hooked up in follow up CL.

BUG=546841
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/289aedeff070843ff814f8b6571be4be7e11fba0
Cr-Commit-Position: refs/heads/master@{#396370}
==========

[commit-bot: I haz the power, 2016-05-27 03:06:09]

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