Remove controller concept from the ARC support extension.

Remove controller concept from the ARC support extension.

Currently, the responsibility of the ARC auth code
control flow was distributed across ArcAuthService
ArcSupportHost and background.js etc. This CL removes
it from background.js, as a preparation to move all
of them into a manageable form.

Now, background.js has two types of messages. 1) Action,
which is sent from the native code, to do something in
extension, and 2) Event to notify to the native code
that something (e.g., user clieck etc.) is happend in the
extension

BUG=b/31079732
TEST=Ran on test device. Ran tests.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/e4d20a317bbc7193b54bb28cdf7cafc242630ac7
Cr-Commit-Position: refs/heads/master@{#422734}

[hidehiko, 2016-10-03 12:59:02]

Description was changed from

==========
Remove controller concept from the ARC support extension.

Currently, the responsibility of the ARC auth code
control flow was distributed across ArcAuthService
ArcSupportHost and background.js etc. This CL removes
it from background.js, as a preparation to move all
of them into a manageable form.

Now, background.js has two types of messages. 1) Action,
which is sent from the native code, to do something in
extension, and 2) Event to notify to the native code
that something (e.g., user clieck etc.) is happend in the
extension

BUG=b/31079732
TEST=Ran on test device. Ran tests.
==========

to

==========
Remove controller concept from the ARC support extension.

Currently, the responsibility of the ARC auth code
control flow was distributed across ArcAuthService
ArcSupportHost and background.js etc. This CL removes
it from background.js, as a preparation to move all
of them into a manageable form.

Now, background.js has two types of messages. 1) Action,
which is sent from the native code, to do something in
extension, and 2) Event to notify to the native code
that something (e.g., user clieck etc.) is happend in the
extension

BUG=b/31079732
TEST=Ran on test device. Ran tests.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[hidehiko, 2016-10-03 13:00:55]

The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 13:01:09]

Dry run: 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-10-03 13:40:24]

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

[commit-bot: I haz the power, 2016-10-03 13:40:25]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2016-10-03 15:45:13]

hidehiko@chromium.org changed reviewers:
+ lhchavez@chromium.org, xiyuan@chromium.org

[hidehiko, 2016-10-03 15:45:14]

PTAL.

[xiyuan, 2016-10-03 15:58:31]

lgtm

[hidehiko, 2016-10-03 16:58:36]

On 2016/10/03 15:58:31, xiyuan wrote:
> lgtm

Thank you for quick review.

PTAL, Luis.

[Luis Héctor Chávez, 2016-10-03 20:40:17]

https://codereview.chromium.org/2388763002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_support_host.cc (right):

https://codereview.chromium.org/2388763002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_support_host.cc:54: const char kEvent[] =
"event";
can you make all of these constexpr now that you're at it?

https://codereview.chromium.org/2388763002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_support_host.cc:380: if (event ==
kEventOnAuthSuccedded) {
any reason why you're not going with "} else if {"? Should be equivalent and is
more idiomatic.

https://codereview.chromium.org/2388763002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_support_host.cc:391: if
(message->GetBoolean(kIsMetricsEnabled, &is_enabled))
With the change I suggested, it's probably better to make all these NOTREACHED()
if GetBoolean fails.

https://codereview.chromium.org/2388763002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/arc_support/background.js (right):

https://codereview.chromium.org/2388763002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/background.js:472:
sendNativeMessage('onAuthSucceeded', {'code': authCode});
Google's JavaScript style guide indicate that object literals should not have
quotes, unless needed to disambiguate syntax:
https://google.github.io/styleguide/javascriptguide.xml?showone=Array_and_Obj...

I also found 4529 instances of non-quoted names vs. 483 instances of quoted
names in c/b/resources, so it seems to also be the convention.

https://codereview.chromium.org/2388763002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/background.js:535:
data['isMetricsEnabled'] = enableMetrics.checked;
I'm not _entirely_ sure that this particular rule applies here:
https://google.github.io/styleguide/javascriptguide.xml?showone=Associative_A...

but in any case object.prop is more idiomatic and occurs 99k times vs. 1k times
for object['prop'].

And actually, you can construct this object upfront and be even more compliant
with the style guide:

var data = {
  isMetricsEnabled: !enableMedtrics.hidden && enableMetrics.checked,
  ....
};

[hidehiko, 2016-10-04 04:48:03]

The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 04:48:15]

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

[hidehiko, 2016-10-04 04:48:19]

Thank you for review. PTAL.

https://codereview.chromium.org/2388763002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_support_host.cc (right):

https://codereview.chromium.org/2388763002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_support_host.cc:54: const char kEvent[] =
"event";
On 2016/10/03 20:40:16, Luis Héctor Chávez wrote:
> can you make all of these constexpr now that you're at it?

Sure. Done.

https://codereview.chromium.org/2388763002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_support_host.cc:380: if (event ==
kEventOnAuthSuccedded) {
On 2016/10/03 20:40:16, Luis Héctor Chávez wrote:
> any reason why you're not going with "} else if {"? Should be equivalent and
is
> more idiomatic.

if ( ... ) {
  ...
  return;
}

is also idiomatic, I think?
Anyway, the main reason I chose it is that we have early return on error (at
L383, and more in the new PS), so for consistency.

We can remove return, e.g.

if (message->GetString(kCode, &code)) {
  arc_auth_service->SetAuthCodeAndStartArc(code);
} else {
  NOTREACHED();
}

although the early return looks more Chrome style?

Another approach would be creating a function for each event type, if it doesn't
look overkill.

Any suggestions?

https://codereview.chromium.org/2388763002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_support_host.cc:391: if
(message->GetBoolean(kIsMetricsEnabled, &is_enabled))
On 2016/10/03 20:40:16, Luis Héctor Chávez wrote:
> With the change I suggested, it's probably better to make all these
NOTREACHED()
> if GetBoolean fails.

Done.

https://codereview.chromium.org/2388763002/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/arc_support/background.js (right):

https://codereview.chromium.org/2388763002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/background.js:472:
sendNativeMessage('onAuthSucceeded', {'code': authCode});
On 2016/10/03 20:40:16, Luis Héctor Chávez wrote:
> Google's JavaScript style guide indicate that object literals should not have
> quotes, unless needed to disambiguate syntax:
>
https://google.github.io/styleguide/javascriptguide.xml?showone=Array_and_Obj...
> 
> I also found 4529 instances of non-quoted names vs. 483 instances of quoted
> names in c/b/resources, so it seems to also be the convention.

Updated as you suggested.

https://codereview.chromium.org/2388763002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/arc_support/background.js:535:
data['isMetricsEnabled'] = enableMetrics.checked;
On 2016/10/03 20:40:16, Luis Héctor Chávez wrote:
> I'm not _entirely_ sure that this particular rule applies here:
>
https://google.github.io/styleguide/javascriptguide.xml?showone=Associative_A...
> 
> but in any case object.prop is more idiomatic and occurs 99k times vs. 1k
times
> for object['prop'].
> 
> And actually, you can construct this object upfront and be even more compliant
> with the style guide:
> 
> var data = {
>   isMetricsEnabled: !enableMedtrics.hidden && enableMetrics.checked,
>   ....
> };

Done.

[Luis Héctor Chávez, 2016-10-04 04:57:12]

lgtm to avoid another cross-timezone roundtrip.

https://codereview.chromium.org/2388763002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_support_host.cc (right):

https://codereview.chromium.org/2388763002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_support_host.cc:380: if (event ==
kEventOnAuthSuccedded) {
On 2016/10/04 04:48:19, hidehiko wrote:
> On 2016/10/03 20:40:16, Luis Héctor Chávez wrote:
> > any reason why you're not going with "} else if {"? Should be equivalent and
> is
> > more idiomatic.
> 
> if ( ... ) {
>   ...
>   return;
> }
> 
> is also idiomatic, I think?
> Anyway, the main reason I chose it is that we have early return on error (at
> L383, and more in the new PS), so for consistency.
> 
> We can remove return, e.g.
> 
> if (message->GetString(kCode, &code)) {
>   arc_auth_service->SetAuthCodeAndStartArc(code);
> } else {
>   NOTREACHED();
> }
> 
> although the early return looks more Chrome style?
> 
> Another approach would be creating a function for each event type, if it
doesn't
> look overkill.
> 
> Any suggestions?

I've mostly seen the early return style when the last remaining path is the
"success" path, in this case it's an error. Function per event type i agree is
definitely overkill.

I think I like the returnless version better. it looks a bit more like a
switch/case, which would be an even better fit for this if C++ supported it for
strings.

[hidehiko, 2016-10-04 06:13:08]

Thank you for review. Submitting.

https://codereview.chromium.org/2388763002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_support_host.cc (right):

https://codereview.chromium.org/2388763002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_support_host.cc:380: if (event ==
kEventOnAuthSuccedded) {
On 2016/10/04 04:57:12, Luis Héctor Chávez wrote:
> On 2016/10/04 04:48:19, hidehiko wrote:
> > On 2016/10/03 20:40:16, Luis Héctor Chávez wrote:
> > > any reason why you're not going with "} else if {"? Should be equivalent
and
> > is
> > > more idiomatic.
> > 
> > if ( ... ) {
> >   ...
> >   return;
> > }
> > 
> > is also idiomatic, I think?
> > Anyway, the main reason I chose it is that we have early return on error (at
> > L383, and more in the new PS), so for consistency.
> > 
> > We can remove return, e.g.
> > 
> > if (message->GetString(kCode, &code)) {
> >   arc_auth_service->SetAuthCodeAndStartArc(code);
> > } else {
> >   NOTREACHED();
> > }
> > 
> > although the early return looks more Chrome style?
> > 
> > Another approach would be creating a function for each event type, if it
> doesn't
> > look overkill.
> > 
> > Any suggestions?
> 
> I've mostly seen the early return style when the last remaining path is the
> "success" path, in this case it's an error. Function per event type i agree is
> definitely overkill.
> 
> I think I like the returnless version better. it looks a bit more like a
> switch/case, which would be an even better fit for this if C++ supported it
for
> strings.

Ok, done.

[hidehiko, 2016-10-04 06:13:21]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2016-10-04 06:13:21]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
lhchavez@chromium.org
Link to the patchset: https://codereview.chromium.org/2388763002/#ps60001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-10-04 06:13:30]

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-10-04 07:43:17]

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

[commit-bot: I haz the power, 2016-10-04 07:43:18]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[hidehiko, 2016-10-04 07:51:50]

Looks unrelated... Retrying.

[hidehiko, 2016-10-04 07:51:56]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2016-10-04 07:52:07]

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-10-04 08:36:36]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-04 08:38:13]

Description was changed from

==========
Remove controller concept from the ARC support extension.

Currently, the responsibility of the ARC auth code
control flow was distributed across ArcAuthService
ArcSupportHost and background.js etc. This CL removes
it from background.js, as a preparation to move all
of them into a manageable form.

Now, background.js has two types of messages. 1) Action,
which is sent from the native code, to do something in
extension, and 2) Event to notify to the native code
that something (e.g., user clieck etc.) is happend in the
extension

BUG=b/31079732
TEST=Ran on test device. Ran tests.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Remove controller concept from the ARC support extension.

Currently, the responsibility of the ARC auth code
control flow was distributed across ArcAuthService
ArcSupportHost and background.js etc. This CL removes
it from background.js, as a preparation to move all
of them into a manageable form.

Now, background.js has two types of messages. 1) Action,
which is sent from the native code, to do something in
extension, and 2) Event to notify to the native code
that something (e.g., user clieck etc.) is happend in the
extension

BUG=b/31079732
TEST=Ran on test device. Ran tests.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/e4d20a317bbc7193b54bb28cdf7cafc242630ac7
Cr-Commit-Position: refs/heads/master@{#422734}
==========

[commit-bot: I haz the power, 2016-10-04 08:38:14]

Patchset 4 (id:??) landed as
https://crrev.com/e4d20a317bbc7193b54bb28cdf7cafc242630ac7
Cr-Commit-Position: refs/heads/master@{#422734}