Fix resource leak on extension window closing.

Fix resource leak on extension window closing.

The ARC support extension should be unloaded on window
closed. However, it is not, because the port connected to
Chrome is kept. In such a case, background page is kept alive
so that the extension will not be unloaded.

This CL closes the port so that extension is unloaded.
Also, the "close" related messaging across ArcAuthService,
ArcSupportHost and extension's background.js is slightly
cleaned up to remove the unnecessary/unused code.
It could be as a preparation of fixing dependencies, too.

BUG=b/31079732, 651838
TEST=Ran on device. Ran unittests.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/824e668590c607e39ee4d8620de32016a920f2c2
Cr-Commit-Position: refs/heads/master@{#422196}

[hidehiko, 2016-09-30 08:50:28]

Description was changed from

==========
Fix resource leak on extension window closing.

The ARC support extension should be unloaded on window
closed. However, it is not, because the port connected to
Chrome is kept. In such a case, background page is kept alive
so that the extension will not be unloaded.

This CL closes the port so that extension is unloaded.
Also, the "close" related messaging across ArcAuthService,
ArcSupportHost and extension's background.js is slightly
cleaned up, as a preparation of fixing dependencies.

BUG=b/31079732
TEST=Ran on device. Ran unittests.
==========

to

==========
Fix resource leak on extension window closing.

The ARC support extension should be unloaded on window
closed. However, it is not, because the port connected to
Chrome is kept. In such a case, background page is kept alive
so that the extension will not be unloaded.

This CL closes the port so that extension is unloaded.
Also, the "close" related messaging across ArcAuthService,
ArcSupportHost and extension's background.js is slightly
cleaned up, as a preparation of fixing dependencies.

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

[hidehiko, 2016-09-30 08:53:23]

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

[commit-bot: I haz the power, 2016-09-30 08:53:33]

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-09-30 10:48:14]

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

[commit-bot: I haz the power, 2016-09-30 10:48:14]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[hidehiko, 2016-09-30 15:04:29]

Description was changed from

==========
Fix resource leak on extension window closing.

The ARC support extension should be unloaded on window
closed. However, it is not, because the port connected to
Chrome is kept. In such a case, background page is kept alive
so that the extension will not be unloaded.

This CL closes the port so that extension is unloaded.
Also, the "close" related messaging across ArcAuthService,
ArcSupportHost and extension's background.js is slightly
cleaned up, as a preparation of fixing dependencies.

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

to

==========
Fix resource leak on extension window closing.

The ARC support extension should be unloaded on window
closed. However, it is not, because the port connected to
Chrome is kept. In such a case, background page is kept alive
so that the extension will not be unloaded.

This CL closes the port so that extension is unloaded.
Also, the "close" related messaging across ArcAuthService,
ArcSupportHost and extension's background.js is slightly
cleaned up to remove the unnecessary/unused code.
It could be as a preparation of fixing dependencies, too.

BUG=b/31079732, 651838
TEST=Ran on device. Ran unittests.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[hidehiko, 2016-09-30 15:05:36]

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

[hidehiko, 2016-09-30 15:05:37]

PTAL.

[Luis Héctor Chávez, 2016-09-30 15:14:14]

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

https://codereview.chromium.org/2380683008/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_support_host.cc:302: void
ArcSupportHost::OnOptInUIClose() {
Given that this and Close() are now equivalent, can we get rid of one? Do we
need Close()? Can it not be OnOptInUIClose()?

[hidehiko, 2016-09-30 15:21:25]

Thank you for quick reply!

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

https://codereview.chromium.org/2380683008/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_support_host.cc:302: void
ArcSupportHost::OnOptInUIClose() {
On 2016/09/30 15:14:14, Luis Héctor Chávez wrote:
> Given that this and Close() are now equivalent, can we get rid of one? Do we
> need Close()? Can it not be OnOptInUIClose()?

I'm planning to remove OnOptInUIClose() as well as OnOptInUIShowPage(), because
these are actually not a observer, but delegate-like code. OnOptInUIClose() is
not an event that the window is closed, but just a request to close the window
from ArcAuthService to ArcSupportHost, which does not sound Observer and is
confusing.

Unfortunately, there is circular dependency. Cleaning the interface needs this
fix, otherwise multiple ArcSupportHost instances conflict to access
ArcAuthService. On the other hand, to simplify this CL, the dependency fix is
needed, as you pointed.

So, I'd like to fix the resource-leak issue first, and then fix the code
dependency.

[Luis Héctor Chávez, 2016-09-30 15:25:35]

lgtm with a TODO in that case :)

[xiyuan, 2016-09-30 16:56:52]

lgtm

[hidehiko, 2016-09-30 17:46:13]

Thank you for review. Done to add explicit TODO. Submitting.

[hidehiko, 2016-09-30 17:46:19]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2016-09-30 17:46:20]

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/2380683008/#ps20001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-09-30 17:46:52]

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-09-30 20:04:05]

Description was changed from

==========
Fix resource leak on extension window closing.

The ARC support extension should be unloaded on window
closed. However, it is not, because the port connected to
Chrome is kept. In such a case, background page is kept alive
so that the extension will not be unloaded.

This CL closes the port so that extension is unloaded.
Also, the "close" related messaging across ArcAuthService,
ArcSupportHost and extension's background.js is slightly
cleaned up to remove the unnecessary/unused code.
It could be as a preparation of fixing dependencies, too.

BUG=b/31079732, 651838
TEST=Ran on device. Ran unittests.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Fix resource leak on extension window closing.

The ARC support extension should be unloaded on window
closed. However, it is not, because the port connected to
Chrome is kept. In such a case, background page is kept alive
so that the extension will not be unloaded.

This CL closes the port so that extension is unloaded.
Also, the "close" related messaging across ArcAuthService,
ArcSupportHost and extension's background.js is slightly
cleaned up to remove the unnecessary/unused code.
It could be as a preparation of fixing dependencies, too.

BUG=b/31079732, 651838
TEST=Ran on device. Ran unittests.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-09-30 20:04:09]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-30 20:06:51]

Description was changed from

==========
Fix resource leak on extension window closing.

The ARC support extension should be unloaded on window
closed. However, it is not, because the port connected to
Chrome is kept. In such a case, background page is kept alive
so that the extension will not be unloaded.

This CL closes the port so that extension is unloaded.
Also, the "close" related messaging across ArcAuthService,
ArcSupportHost and extension's background.js is slightly
cleaned up to remove the unnecessary/unused code.
It could be as a preparation of fixing dependencies, too.

BUG=b/31079732, 651838
TEST=Ran on device. Ran unittests.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Fix resource leak on extension window closing.

The ARC support extension should be unloaded on window
closed. However, it is not, because the port connected to
Chrome is kept. In such a case, background page is kept alive
so that the extension will not be unloaded.

This CL closes the port so that extension is unloaded.
Also, the "close" related messaging across ArcAuthService,
ArcSupportHost and extension's background.js is slightly
cleaned up to remove the unnecessary/unused code.
It could be as a preparation of fixing dependencies, too.

BUG=b/31079732, 651838
TEST=Ran on device. Ran unittests.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/824e668590c607e39ee4d8620de32016a920f2c2
Cr-Commit-Position: refs/heads/master@{#422196}
==========

[commit-bot: I haz the power, 2016-09-30 20:06:55]

Patchset 2 (id:??) landed as
https://crrev.com/824e668590c607e39ee4d8620de32016a920f2c2
Cr-Commit-Position: refs/heads/master@{#422196}