Remove update icon after user confirms download

Remove update icon after user confirms download

This CL removes update icon from system tray along with the
corresponding row in details view after user confirms download of the
update over cellular connections.

BUG=731222
Review-Url: https://codereview.chromium.org/2933923002
Cr-Commit-Position: refs/heads/master@{#479181}
Committed: https://chromium.googlesource.com/chromium/src/+/98258cced1d9ed158af9e98706225119b6765e2d

[weidongg, 2017-06-12 20:57:32]

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

[commit-bot: I haz the power, 2017-06-12 20:58:00]

Dry run: CQ is trying da patch.

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

[weidongg, 2017-06-12 21:07:21]

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

[weidongg, 2017-06-12 21:07:22]

stevenjb@, please review the changes.

[commit-bot: I haz the power, 2017-06-12 21:47:28]

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

[commit-bot: I haz the power, 2017-06-12 21:47:29]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2017-06-13 17:38:18]

https://codereview.chromium.org/2933923002/diff/1/ash/system/update/tray_upda...
File ash/system/update/tray_update.cc (right):

https://codereview.chromium.org/2933923002/diff/1/ash/system/update/tray_upda...
ash/system/update/tray_update.cc:169: if (success) {
if (!success) return;

https://codereview.chromium.org/2933923002/diff/1/chrome/browser/ui/ash/syste...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/2933923002/diff/1/chrome/browser/ui/ash/syste...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:142:
DBusThreadManager::Get()->GetUpdateEngineClient()->RemoveObserver(this);
It's good practice to test against DBusThreadManager::IsInitialized() in
destructors; I'm pretty sure DBusThreadManager outlives this, but sometimes
destruction order is less clear in tests.

https://codereview.chromium.org/2933923002/diff/1/chrome/browser/ui/ash/syste...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.h (right):

https://codereview.chromium.org/2933923002/diff/1/chrome/browser/ui/ash/syste...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:120: // Overridden from
public UpdateEngineClient::Observer.
s/public//

[weidongg, 2017-06-13 19:01:39]

Thanks for the review.

https://codereview.chromium.org/2933923002/diff/1/ash/system/update/tray_upda...
File ash/system/update/tray_update.cc (right):

https://codereview.chromium.org/2933923002/diff/1/ash/system/update/tray_upda...
ash/system/update/tray_update.cc:169: if (success) {
On 2017/06/13 17:38:18, stevenjb wrote:
> if (!success) return;

Done.

https://codereview.chromium.org/2933923002/diff/1/chrome/browser/ui/ash/syste...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/2933923002/diff/1/chrome/browser/ui/ash/syste...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:142:
DBusThreadManager::Get()->GetUpdateEngineClient()->RemoveObserver(this);
On 2017/06/13 17:38:18, stevenjb wrote:
> It's good practice to test against DBusThreadManager::IsInitialized() in
> destructors; I'm pretty sure DBusThreadManager outlives this, but sometimes
> destruction order is less clear in tests.

Ok, I added a flag to indicate whether Initialize() is called or not.

https://codereview.chromium.org/2933923002/diff/1/chrome/browser/ui/ash/syste...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.h (right):

https://codereview.chromium.org/2933923002/diff/1/chrome/browser/ui/ash/syste...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:120: // Overridden from
public UpdateEngineClient::Observer.
On 2017/06/13 17:38:18, stevenjb wrote:
> s/public//

Done.

[stevenjb, 2017-06-13 20:29:53]

https://codereview.chromium.org/2933923002/diff/20001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/2933923002/diff/20001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:139: if (initialized_) {
This is not actually what I was suggesting, although if it is possible for
Initialize() to not be called then that is also a good idea.

https://codereview.chromium.org/2933923002/diff/20001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:144:
DBusThreadManager::Get()->GetUpdateEngineClient()->RemoveObserver(this);
With or without |initiaizled_|, this should be:

if (DBusThreadManager::IsInitialized())
  DBusThreadManager::Get()->GetUpdateEngineClient()->RemoveObserver(this);

in case DBusThreadManager is destroyed before SystemTrayDelegateChromeOS.

[weidongg, 2017-06-13 20:51:25]

https://codereview.chromium.org/2933923002/diff/20001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/2933923002/diff/20001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:144:
DBusThreadManager::Get()->GetUpdateEngineClient()->RemoveObserver(this);
On 2017/06/13 20:29:53, stevenjb wrote:
> With or without |initiaizled_|, this should be:
> 
> if (DBusThreadManager::IsInitialized())
>   DBusThreadManager::Get()->GetUpdateEngineClient()->RemoveObserver(this);
> 
> in case DBusThreadManager is destroyed before SystemTrayDelegateChromeOS.

Aha, I see. Thanks for explanation. Done.

[stevenjb, 2017-06-13 21:25:41]

lgtm

[weidongg, 2017-06-13 21:36:57]

The CQ bit was checked by weidongg@chromium.org

[commit-bot: I haz the power, 2017-06-13 21:37:58]

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, 2017-06-13 22:35:52]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497389817947010,
"parent_rev": "df3595bb1db14d038879823a24b60e9e284c861c", "commit_rev":
"98258cced1d9ed158af9e98706225119b6765e2d"}

[commit-bot: I haz the power, 2017-06-13 22:36:02]

Description was changed from

==========
Remove update icon after user confirms download

This CL removes update icon from system tray along with the
corresponding row in details view after user confirms download of the
update over cellular connections.

BUG=731222
==========

to

==========
Remove update icon after user confirms download

This CL removes update icon from system tray along with the
corresponding row in details view after user confirms download of the
update over cellular connections.

BUG=731222

Review-Url: https://codereview.chromium.org/2933923002
Cr-Commit-Position: refs/heads/master@{#479181}
Committed:
https://chromium.googlesource.com/chromium/src/+/98258cced1d9ed158af9e9870622...
==========

[commit-bot: I haz the power, 2017-06-13 22:36:04]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/98258cced1d9ed158af9e9870622...