Display "Restart to update Adobe Flash Player" for Flash updates.

Display "Restart to update Adobe Flash Player" for Flash updates.

This CL displays the text "Restart to update Adobe Flash Player" in the
system tray when a Flash player update is available.

BUG=711822
Review-Url: https://codereview.chromium.org/2816253002
Cr-Commit-Position: refs/heads/master@{#465818}
Committed: https://chromium.googlesource.com/chromium/src/+/ed2af8b429def7ee0de3d62efe1f07e7d61c1ed8

[Greg K, 2017-04-17 18:07:13]

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

[commit-bot: I haz the power, 2017-04-17 18:07:30]

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, 2017-04-17 18:28:41]

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

[commit-bot: I haz the power, 2017-04-17 18:28:42]

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

[Greg K, 2017-04-17 22:23:41]

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

[commit-bot: I haz the power, 2017-04-17 22:24:03]

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

[Greg K, 2017-04-17 22:28:44]

Description was changed from

==========
Display "Restart to update Adobe Flash Player" for Flash updates.

This CL displays the text "Restart to update Adobe Flash Player" in the
system tray when a Flash player update is available.

BUG=711822
==========

to

==========
Display "Restart to update Adobe Flash Player" for Flash updates.

This CL displays the text "Restart to update Adobe Flash Player" in the
system tray when a Flash player update is available.

BUG=711822
==========

[Greg K, 2017-04-17 22:28:44]

kerrnel@chromium.org changed reviewers:
+ jamescook@chromium.org

[Greg K, 2017-04-17 22:29:16]

On 2017/04/17 22:28:44, Greg K wrote:
> mailto:kerrnel@chromium.org changed reviewers:
> + mailto:jamescook@chromium.org

James,

PTAL. In particular, is there anyway to change the unit test to check the string
in the update menu so that I can make sure it says the right thing in the right
scenarios?

Regards,

Greg

[Greg K, 2017-04-17 22:29:45]

kerrnel@chromium.org changed reviewers:
+ ihf@chromium.org

[commit-bot: I haz the power, 2017-04-17 22:54:30]

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

[commit-bot: I haz the power, 2017-04-17 22:54:31]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-04-18 00:57:28]

Code looks good, suggestion for the test below.

https://codereview.chromium.org/2816253002/diff/40001/ash/public/interfaces/u...
File ash/public/interfaces/update.mojom (right):

https://codereview.chromium.org/2816253002/diff/40001/ash/public/interfaces/u...
ash/public/interfaces/update.mojom:21: NONE,
Is NONE used? If not, I would just provide SYSTEM and FLASH.

https://codereview.chromium.org/2816253002/diff/40001/ash/system/update/tray_...
File ash/system/update/tray_update_unittest.cc (right):

https://codereview.chromium.org/2816253002/diff/40001/ash/system/update/tray_...
ash/system/update/tray_update_unittest.cc:31: }
You should add a test for the Flash case. I would suggest adding a
TrayUpdate::GetLabelForTesting() and a
TrayUpdate::UpdateView::GetLabelForTesting(). Your other option is to crawl the
view hierarchy looking the expected label, like with a simple depth-first
search.

[Greg K, 2017-04-18 20:46:46]

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

[commit-bot: I haz the power, 2017-04-18 20:47:23]

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, 2017-04-18 21:18:28]

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

[commit-bot: I haz the power, 2017-04-18 21:18:29]

Dry run: This issue passed the CQ dry run.

[Greg K, 2017-04-18 22:46:50]

https://codereview.chromium.org/2816253002/diff/40001/ash/public/interfaces/u...
File ash/public/interfaces/update.mojom (right):

https://codereview.chromium.org/2816253002/diff/40001/ash/public/interfaces/u...
ash/public/interfaces/update.mojom:21: NONE,
On 2017/04/18 00:57:28, James Cook wrote:
> Is NONE used? If not, I would just provide SYSTEM and FLASH.

Done.

https://codereview.chromium.org/2816253002/diff/40001/ash/system/update/tray_...
File ash/system/update/tray_update_unittest.cc (right):

https://codereview.chromium.org/2816253002/diff/40001/ash/system/update/tray_...
ash/system/update/tray_update_unittest.cc:31: }
On 2017/04/18 00:57:28, James Cook wrote:
> You should add a test for the Flash case. I would suggest adding a
> TrayUpdate::GetLabelForTesting() and a
> TrayUpdate::UpdateView::GetLabelForTesting(). Your other option is to crawl
the
> view hierarchy looking the expected label, like with a simple depth-first
> search.

Done.

[James Cook, 2017-04-18 23:20:57]

One suggestion for the test.

You'll need a security reviewer for the mojom files. I suggest tsepez, he's most
familiar with what we're doing in ash.

https://codereview.chromium.org/2816253002/diff/60001/ash/public/interfaces/s...
File ash/public/interfaces/system_tray.mojom (right):

https://codereview.chromium.org/2816253002/diff/60001/ash/public/interfaces/s...
ash/public/interfaces/system_tray.mojom:29: ShowUpdateIcon(UpdateSeverity
severity, bool factory_reset_required, UpdateType update_type);
nit: wrap line

https://codereview.chromium.org/2816253002/diff/60001/ash/system/update/tray_...
File ash/system/update/tray_update.cc (right):

https://codereview.chromium.org/2816253002/diff/60001/ash/system/update/tray_...
ash/system/update/tray_update.cc:95: update_label_ = label;
update_label_ can point to deleted memory after the tray is closed. Instead of
making this a static, I would either:

* Make it a member of TrayUpdate and set it via |owner| and clear it in the
destructor, or better yet
* Make it a member of this class and make TrayUpdate have a non-static
update_view_ or default_view_ member. CreateDefaultView() can set the pointer
and return it. You'll need to add an override for DestroyDefaultView() that
resets the pointer to null. There should be lots of examples of this.

https://codereview.chromium.org/2816253002/diff/60001/ash/system/update/tray_...
ash/system/update/tray_update.cc:149: return update_label_ ?
update_label_->text() : base::string16();
Then this could become "return default_view_ ? default_view_->label_ : nullptr"
or similar.

[Greg K, 2017-04-18 23:44:48]

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

[Greg K, 2017-04-18 23:45:23]

https://codereview.chromium.org/2816253002/diff/60001/ash/public/interfaces/s...
File ash/public/interfaces/system_tray.mojom (right):

https://codereview.chromium.org/2816253002/diff/60001/ash/public/interfaces/s...
ash/public/interfaces/system_tray.mojom:29: ShowUpdateIcon(UpdateSeverity
severity, bool factory_reset_required, UpdateType update_type);
On 2017/04/18 23:20:56, James Cook wrote:
> nit: wrap line

Done.

https://codereview.chromium.org/2816253002/diff/60001/ash/system/update/tray_...
File ash/system/update/tray_update.cc (right):

https://codereview.chromium.org/2816253002/diff/60001/ash/system/update/tray_...
ash/system/update/tray_update.cc:95: update_label_ = label;
On 2017/04/18 23:20:57, James Cook wrote:
> update_label_ can point to deleted memory after the tray is closed. Instead of
> making this a static, I would either:
> 
> * Make it a member of TrayUpdate and set it via |owner| and clear it in the
> destructor, or better yet
> * Make it a member of this class and make TrayUpdate have a non-static
> update_view_ or default_view_ member. CreateDefaultView() can set the pointer
> and return it. You'll need to add an override for DestroyDefaultView() that
> resets the pointer to null. There should be lots of examples of this.

Done.

https://codereview.chromium.org/2816253002/diff/60001/ash/system/update/tray_...
ash/system/update/tray_update.cc:149: return update_label_ ?
update_label_->text() : base::string16();
On 2017/04/18 23:20:57, James Cook wrote:
> Then this could become "return default_view_ ? default_view_->label_ :
nullptr"
> or similar.

Done.

[commit-bot: I haz the power, 2017-04-18 23:45:36]

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

[Greg K, 2017-04-18 23:46:46]

kerrnel@chromium.org changed reviewers:
+ wfh@chromium.org

[Greg K, 2017-04-18 23:47:04]

wfh, can you PTAL at the .mojom changes? tsepez@ is OOO.

Thanks,

Greg

[commit-bot: I haz the power, 2017-04-19 00:18:42]

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

[commit-bot: I haz the power, 2017-04-19 00:18:44]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-04-19 00:27:28]

LGTM. Nice patch.

https://codereview.chromium.org/2816253002/diff/80001/ash/system/update/tray_...
File ash/system/update/tray_update.cc (right):

https://codereview.chromium.org/2816253002/diff/80001/ash/system/update/tray_...
ash/system/update/tray_update.cc:93: views::Label* label =
TrayPopupUtils::CreateDefaultLabel();
nit: just use update_label_ here and below

https://codereview.chromium.org/2816253002/diff/80001/ash/system/update/tray_...
File ash/system/update/tray_update.h (right):

https://codereview.chromium.org/2816253002/diff/80001/ash/system/update/tray_...
ash/system/update/tray_update.h:53: views::Label* GetLabelForTesting();
optional: This can be public. The name alone is enough. Then you might not need
the friends.

[ilja, 2017-04-19 07:50:33]

https://codereview.chromium.org/2816253002/diff/80001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_client.cc (right):

https://codereview.chromium.org/2816253002/diff/80001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_client.cc:383:
DCHECK(detector->notify_upgrade() || flash_update_available_);
I feel the logic here (and new code below) while correct is a bit
inverted/brittle, as by default we show FLASH update. I would feel better if
this function by default showed SYSTEM update *or* if the DCHECK leads to an
early exit. Maybe I am overly cautious, but feel we might confuse users in a
failure case with a FLASH notification more than the other way around.

[Greg K, 2017-04-19 21:41:48]

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

[Greg K, 2017-04-19 21:42:10]

https://codereview.chromium.org/2816253002/diff/80001/ash/system/update/tray_...
File ash/system/update/tray_update.cc (right):

https://codereview.chromium.org/2816253002/diff/80001/ash/system/update/tray_...
ash/system/update/tray_update.cc:93: views::Label* label =
TrayPopupUtils::CreateDefaultLabel();
On 2017/04/19 00:27:28, James Cook wrote:
> nit: just use update_label_ here and below

Done.

https://codereview.chromium.org/2816253002/diff/80001/ash/system/update/tray_...
File ash/system/update/tray_update.h (right):

https://codereview.chromium.org/2816253002/diff/80001/ash/system/update/tray_...
ash/system/update/tray_update.h:53: views::Label* GetLabelForTesting();
On 2017/04/19 00:27:28, James Cook wrote:
> optional: This can be public. The name alone is enough. Then you might not
need
> the friends.

If its OK with you, I would much rather leave any testing specific methods
private. It just feels "better."

https://codereview.chromium.org/2816253002/diff/80001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_client.cc (right):

https://codereview.chromium.org/2816253002/diff/80001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_client.cc:383:
DCHECK(detector->notify_upgrade() || flash_update_available_);
On 2017/04/19 07:50:33, ilja wrote:
> I feel the logic here (and new code below) while correct is a bit
> inverted/brittle, as by default we show FLASH update. I would feel better if
> this function by default showed SYSTEM update *or* if the DCHECK leads to an
> early exit. Maybe I am overly cautious, but feel we might confuse users in a
> failure case with a FLASH notification more than the other way around.

Good point, but I had written the code below to make sure that a system update
has priority over a flash update for the notification. I changed this to return
if there is no update available instead.

[commit-bot: I haz the power, 2017-04-19 21:42:27]

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

[Will Harris, 2017-04-19 21:55:42]

mojom rs lgtm

[commit-bot: I haz the power, 2017-04-19 22:12:11]

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

[commit-bot: I haz the power, 2017-04-19 22:12:12]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-04-19 23:02:40]

still lgtm

[Greg K, 2017-04-20 00:00:56]

On 2017/04/19 23:02:40, James Cook wrote:
> still lgtm

Thanks I just finished testing the final CL in the case where we first apply a
Flash update (it shows "Restart to update Adobe Flash Player") and then a system
update comes along. The message changes to "Restart to Update."

[Greg K, 2017-04-20 00:08:16]

The CQ bit was checked by kerrnel@chromium.org

[commit-bot: I haz the power, 2017-04-20 00:08:42]

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-04-20 00:14:51]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492646896694140,
"parent_rev": "76c1b7af9080fc7e1c14b14aef7d1b8f3b4ac828", "commit_rev":
"ed2af8b429def7ee0de3d62efe1f07e7d61c1ed8"}

[commit-bot: I haz the power, 2017-04-20 00:15:42]

Description was changed from

==========
Display "Restart to update Adobe Flash Player" for Flash updates.

This CL displays the text "Restart to update Adobe Flash Player" in the
system tray when a Flash player update is available.

BUG=711822
==========

to

==========
Display "Restart to update Adobe Flash Player" for Flash updates.

This CL displays the text "Restart to update Adobe Flash Player" in the
system tray when a Flash player update is available.

BUG=711822

Review-Url: https://codereview.chromium.org/2816253002
Cr-Commit-Position: refs/heads/master@{#465818}
Committed:
https://chromium.googlesource.com/chromium/src/+/ed2af8b429def7ee0de3d62efe1f...
==========

[commit-bot: I haz the power, 2017-04-20 00:15:44]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/ed2af8b429def7ee0de3d62efe1f...