Settings: About: Fix cros channel info.

Settings: About: Fix cros channel info.

BUG=665705
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/9d01f7a426ca85db9b12a4db69f9bd63efb89dcd
Cr-Commit-Position: refs/heads/master@{#437125}

[stevenjb, 2016-12-02 22:00:45]

Description was changed from

==========
Settings: About: Fix cros channel info.

BUG=665705
==========

to

==========
Settings: About: Fix cros channel info.

BUG=665705
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[stevenjb, 2016-12-02 23:03:37]

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

[stevenjb, 2016-12-02 23:04:59]

https://codereview.chromium.org/2538043006/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/about_handler.cc (left):

https://codereview.chromium.org/2538043006/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/about_handler.cc:302:
html_source->AddBoolean("aboutCanChangeChannel", CanChangeChannel(profile));
The primary problem is that the result of CanChangeChannel is not accurate when
this is created, so we need to provide it via a query of some sort. (I checked
and none of the other properties set on Create appear to have that issue).

[dpapad, 2016-12-02 23:27:12]

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

https://codereview.chromium.org/2538043006/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/channel_switcher_dialog.js:128:
this.targetChannel_, selectedChannel)) {
I am confused by the currentChannel -> targetChannel change? Isn't the dialog
supposed to be pre-populated with the current channel?

https://codereview.chromium.org/2538043006/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/about_handler.cc (left):

https://codereview.chromium.org/2538043006/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/about_handler.cc:302:
html_source->AddBoolean("aboutCanChangeChannel", CanChangeChannel(profile));
On 2016/12/02 at 23:04:59, stevenjb wrote:
> The primary problem is that the result of CanChangeChannel is not accurate
when this is created, so we need to provide it via a query of some sort. (I
checked and none of the other properties set on Create appear to have that
issue).

I guess in the old C++ handler, we call CanChangeChannel() much later, see
"OnPageLoaded" at
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/help/help_handle....

https://codereview.chromium.org/2538043006/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/about_handler.cc (right):

https://codereview.chromium.org/2538043006/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/about_handler.cc:354: "getCanChangeChannel",
Nit: Maybe rename to just |canChangeChannel|? |getCan...| sounds odd, too many
verbs.

https://codereview.chromium.org/2538043006/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/about_handler.cc:569:
base::FundamentalValue(can_change_channel));
Nit (optional)

ResolveJavascriptCallback(
    base::StringValue(callback_id),
    base::FundamentalValue(CanChangeChannel(Profile::FromWebUI(web_ui())));

[dpapad, 2016-12-02 23:29:43]

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

https://codereview.chromium.org/2538043006/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/channel_switcher_dialog.js:128:
this.targetChannel_, selectedChannel)) {
On 2016/12/02 at 23:27:12, dpapad wrote:
> I am confused by the currentChannel -> targetChannel change? Isn't the dialog
supposed to be pre-populated with the current channel?

Also see equivalent logic in the old page,
https://cs.chromium.org/chromium/src/chrome/browser/resources/help/channel_ch...,
using currentChannel instead of targetChannel.

[stevenjb, 2016-12-02 23:54:39]

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

https://codereview.chromium.org/2538043006/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/channel_switcher_dialog.js:128:
this.targetChannel_, selectedChannel)) {
On 2016/12/02 23:29:43, dpapad wrote:
> On 2016/12/02 at 23:27:12, dpapad wrote:
> > I am confused by the currentChannel -> targetChannel change? Isn't the
dialog
> supposed to be pre-populated with the current channel?
> 
> Also see equivalent logic in the old page,
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/help/channel_ch...,
> using currentChannel instead of targetChannel.

We want to show the *target* channel as selected in the dialog, since that
corresponds to the "most recently selected" channel. (i.e. the user is actually
selecting the target channel, which won't become the current channel until an
update occurs).

But yeah, it looks like we need to involve currentChannel here at least. You'll
note that the rest of the old UI logic that you pointed to uses targetChannel.

I'll take a closer look at the code on Monday.



I'll look at this more carefully on Monday.

[dpapad, 2016-12-03 00:02:32]

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

https://codereview.chromium.org/2538043006/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/channel_switcher_dialog.js:128:
this.targetChannel_, selectedChannel)) {
On 2016/12/02 at 23:54:39, stevenjb wrote:
> On 2016/12/02 23:29:43, dpapad wrote:
> > On 2016/12/02 at 23:27:12, dpapad wrote:
> > > I am confused by the currentChannel -> targetChannel change? Isn't the
dialog
> > supposed to be pre-populated with the current channel?
> > 
> > Also see equivalent logic in the old page,
> >
https://cs.chromium.org/chromium/src/chrome/browser/resources/help/channel_ch...,
> > using currentChannel instead of targetChannel.
> 
> We want to show the *target* channel as selected in the dialog, since that
corresponds to the "most recently selected" channel. (i.e. the user is actually
selecting the target channel, which won't become the current channel until an
update occurs).
> 
> But yeah, it looks like we need to involve currentChannel here at least.
You'll note that the rest of the old UI logic that you pointed to uses
targetChannel.
> 
> I'll take a closer look at the code on Monday.
> 
> 
> 
> I'll look at this more carefully on Monday.

Ok. In case it helps, this is my understanding (which my be contradicting the
current state in this CL).

 - Current channel: This is what the dialog should be prepopulated with (which
means that the dialog always start with the "change channel" button disabled).
 - Target channel: Initially has the same value as the "current channel" and is
not reflected anywhere in the dialog.
 - Selected channel: The currently selected value in the dialog. If different
than "current channel" the "change channel" button is enabled. Once the user
clicks "change channel", this value becomes the "target channel" value.

[stevenjb, 2016-12-05 18:25:30]

On 2016/12/03 00:02:32, dpapad wrote:
>
https://codereview.chromium.org/2538043006/diff/40001/chrome/browser/resource...
> File chrome/browser/resources/settings/about_page/channel_switcher_dialog.js
> (right):
> 
>
https://codereview.chromium.org/2538043006/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/about_page/channel_switcher_dialog.js:128:
> this.targetChannel_, selectedChannel)) {
> On 2016/12/02 at 23:54:39, stevenjb wrote:
> > On 2016/12/02 23:29:43, dpapad wrote:
> > > On 2016/12/02 at 23:27:12, dpapad wrote:
> > > > I am confused by the currentChannel -> targetChannel change? Isn't the
> dialog
> > > supposed to be pre-populated with the current channel?
> > > 
> > > Also see equivalent logic in the old page,
> > >
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/help/channel_ch...,
> > > using currentChannel instead of targetChannel.
> > 
> > We want to show the *target* channel as selected in the dialog, since that
> corresponds to the "most recently selected" channel. (i.e. the user is
actually
> selecting the target channel, which won't become the current channel until an
> update occurs).
> > 
> > But yeah, it looks like we need to involve currentChannel here at least.
> You'll note that the rest of the old UI logic that you pointed to uses
> targetChannel.
> > 
> > I'll take a closer look at the code on Monday.
> > 
> > 
> > 
> > I'll look at this more carefully on Monday.
> 
> Ok. In case it helps, this is my understanding (which my be contradicting the
> current state in this CL).
> 
>  - Current channel: This is what the dialog should be prepopulated with (which
> means that the dialog always start with the "change channel" button disabled).

I do not believe that is correct. "Current channel" == the channel that Chrome
is currently updated to, *not* the channel that the user has most recently
selected. I believe that current channel should only affect the messaging in the
UI.

>  - Target channel: Initially has the same value as the "current channel" and
is
> not reflected anywhere in the dialog.

Not true. If a user selects a new channel then reloads the UI (or reboots)
before an update is applied, target channel != current channel. The dialog
should reflect the target channel state.

>  - Selected channel: The currently selected value in the dialog. If different
> than "current channel" the "change channel" button is enabled. Once the user
> clicks "change channel", this value becomes the "target channel" value.

This would just be the value of the radio button in the dialog.

I think the misunderstanding here is that "current channel" gets updated as soon
as the UI sends the request to Chrome. That does not appear to be the case.
Current channel reflects the channel that the user is currently running, and
target channel reflects the desired channel. They will be the same only once an
update is applied.

[dpapad, 2016-12-05 18:52:27]

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

https://codereview.chromium.org/2538043006/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/detailed_build_info.js:38:
this.getChannel();
It took me a second to realize that |getChannel| actually updates the state of
|this|. Can you rename to |updateChannel| instead?

https://codereview.chromium.org/2538043006/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/detailed_build_info.js:46:
getChannel: function() {
Private methods should end with "_" suffix.

https://codereview.chromium.org/2538043006/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/detailed_build_info.js:48: // We
actually display the target channel for the 'Currently on' message.
Nit: s/We actually d/D/

https://codereview.chromium.org/2538043006/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/about_page_tests.js (right):

https://codereview.chromium.org/2538043006/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/about_page_tests.js:42: /** @type {boolean} */
@private

https://codereview.chromium.org/2538043006/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/about_page_tests.js:612: *     changing channels
is allowed.
@return {!Promise} missing

https://codereview.chromium.org/2538043006/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/about_page_tests.js:619:
browserProxy.whenCalled('getCanChangeChannel'),
Why use Promise.all() for a single Promise?

[stevenjb, 2016-12-06 20:38:03]

PTAL

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

https://codereview.chromium.org/2538043006/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/channel_switcher_dialog.js:128:
this.targetChannel_, selectedChannel)) {
On 2016/12/03 00:02:32, dpapad wrote:
> On 2016/12/02 at 23:54:39, stevenjb wrote:
> > On 2016/12/02 23:29:43, dpapad wrote:
> > > On 2016/12/02 at 23:27:12, dpapad wrote:
> > > > I am confused by the currentChannel -> targetChannel change? Isn't the
> dialog
> > > supposed to be pre-populated with the current channel?
> > > 
> > > Also see equivalent logic in the old page,
> > >
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/help/channel_ch...,
> > > using currentChannel instead of targetChannel.
> > 
> > We want to show the *target* channel as selected in the dialog, since that
> corresponds to the "most recently selected" channel. (i.e. the user is
actually
> selecting the target channel, which won't become the current channel until an
> update occurs).
> > 
> > But yeah, it looks like we need to involve currentChannel here at least.
> You'll note that the rest of the old UI logic that you pointed to uses
> targetChannel.
> > 
> > I'll take a closer look at the code on Monday.
> > 
> > 
> > 
> > I'll look at this more carefully on Monday.
> 
> Ok. In case it helps, this is my understanding (which my be contradicting the
> current state in this CL).
> 
>  - Current channel: This is what the dialog should be prepopulated with (which
> means that the dialog always start with the "change channel" button disabled).
>  - Target channel: Initially has the same value as the "current channel" and
is
> not reflected anywhere in the dialog.
>  - Selected channel: The currently selected value in the dialog. If different
> than "current channel" the "change channel" button is enabled. Once the user
> clicks "change channel", this value becomes the "target channel" value.

OK, I think I have the logic correct now. We do need both the current and target
channel.

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

https://codereview.chromium.org/2538043006/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/detailed_build_info.js:38:
this.getChannel();
On 2016/12/05 18:52:27, dpapad wrote:
> It took me a second to realize that |getChannel| actually updates the state of
> |this|. Can you rename to |updateChannel| instead?

Done.

https://codereview.chromium.org/2538043006/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/detailed_build_info.js:46:
getChannel: function() {
On 2016/12/05 18:52:27, dpapad wrote:
> Private methods should end with "_" suffix.

Done.

https://codereview.chromium.org/2538043006/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/detailed_build_info.js:48: // We
actually display the target channel for the 'Currently on' message.
On 2016/12/05 18:52:27, dpapad wrote:
> Nit: s/We actually d/D/

Done.

https://codereview.chromium.org/2538043006/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/about_handler.cc (right):

https://codereview.chromium.org/2538043006/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/about_handler.cc:354: "getCanChangeChannel",
On 2016/12/02 23:27:12, dpapad wrote:
> Nit: Maybe rename to just |canChangeChannel|? |getCan...| sounds odd, too many
> verbs.

I decided this would all be a bit cleaner with a single getChannelInfo handler.
LMKWYT.

https://codereview.chromium.org/2538043006/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/about_handler.cc:569:
base::FundamentalValue(can_change_channel));
On 2016/12/02 23:27:12, dpapad wrote:
> Nit (optional)
> 
> ResolveJavascriptCallback(
>     base::StringValue(callback_id),
>     base::FundamentalValue(CanChangeChannel(Profile::FromWebUI(web_ui())));

Done.

https://codereview.chromium.org/2538043006/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/about_page_tests.js (right):

https://codereview.chromium.org/2538043006/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/about_page_tests.js:42: /** @type {boolean} */
On 2016/12/05 18:52:27, dpapad wrote:
> @private

Done.

https://codereview.chromium.org/2538043006/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/about_page_tests.js:619:
browserProxy.whenCalled('getCanChangeChannel'),
On 2016/12/05 18:52:27, dpapad wrote:
> Why use Promise.all() for a single Promise?

Done.

[dpapad, 2016-12-07 01:42:24]

LGTM with a question and a nit.

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

https://codereview.chromium.org/2538043006/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/detailed_build_info.js:45: //
Display the target channel for the 'Currently on' message.
Does this message reflect the fact that we are not yet on the "currently on"
channel, but still in the process of updating to it? Basically is
https://bugs.chromium.org/p/chromium/issues/detail?id=610529#c7 addressed as
part of this CL?

https://codereview.chromium.org/2538043006/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/about_page_tests.js (right):

https://codereview.chromium.org/2538043006/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/about_page_tests.js:92:
TestAboutPageBrowserProxy.prototype.setCanChangeChannel = function(
Nit: Can we combine the setCanChangeChannel() and setChannels() to a single
setChannelInfo() method?

[stevenjb, 2016-12-07 20:59:17]

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

https://codereview.chromium.org/2538043006/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/detailed_build_info.js:45: //
Display the target channel for the 'Currently on' message.
On 2016/12/07 01:42:23, dpapad wrote:
> Does this message reflect the fact that we are not yet on the "currently on"
> channel, but still in the process of updating to it? Basically is
> https://bugs.chromium.org/p/chromium/issues/detail?id=610529#c7 addressed as
> part of this CL?

Effectively. We could definitely improve the UI by showing the actual current
channel and the "updating to" channel here. For now at least this matches the
current experience.

https://codereview.chromium.org/2538043006/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/about_page_tests.js (right):

https://codereview.chromium.org/2538043006/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/about_page_tests.js:92:
TestAboutPageBrowserProxy.prototype.setCanChangeChannel = function(
On 2016/12/07 01:42:23, dpapad wrote:
> Nit: Can we combine the setCanChangeChannel() and setChannels() to a single
> setChannelInfo() method?

I did that initially but it actually ends up being more awkward, because we want
to, e.g. set just 'canChangeChannel' without changing the current/target
channel.

[stevenjb, 2016-12-07 20:59:20]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-12-07 20:59:20]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2538043006/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2016-12-07 20:59:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 21:42:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 21:42:45]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[stevenjb, 2016-12-07 23:01:34]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-12-07 23:01:34]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2538043006/#ps120001
(title: "Fix non cros tests")

[commit-bot: I haz the power, 2016-12-07 23:02:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 01:46:43]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481151694041780,
"parent_rev": "3922b4b143ed7c9c06e9b8fd1094a8fed701f35b", "commit_rev":
"206434b1fc423fc9957fe9e40759c99a9e308732"}

[commit-bot: I haz the power, 2016-12-08 01:47:33]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-12-08 01:50:18]

Description was changed from

==========
Settings: About: Fix cros channel info.

BUG=665705
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Settings: About: Fix cros channel info.

BUG=665705
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/9d01f7a426ca85db9b12a4db69f9bd63efb89dcd
Cr-Commit-Position: refs/heads/master@{#437125}
==========

[commit-bot: I haz the power, 2016-12-08 01:50:20]

Patchset 7 (id:??) landed as
https://crrev.com/9d01f7a426ca85db9b12a4db69f9bd63efb89dcd
Cr-Commit-Position: refs/heads/master@{#437125}