Migrate upgrade notifications to observer

Migrate upgrade notifications to observer

NotificationService makes it too easy for a random piece of code to hook
into another, so this CL migrates upgrade notification to observer.

BUG=724540, 268984
Review-Url: https://codereview.chromium.org/2925973002
Cr-Commit-Position: refs/heads/master@{#478127}
Committed: https://chromium.googlesource.com/chromium/src/+/0469523c2aafcfb307f7a9190140194d169cb72c

[weidongg, 2017-06-07 00:17:53]

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

[commit-bot: I haz the power, 2017-06-07 00:18:16]

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-07 00:23:53]

Description was changed from

==========
Migrate upgrade notifications to observer

NotificationService makes it too easy for a random piece of code to hook
into another, so this CL migrates upgrade notification to observer.

BUG=724540
==========

to

==========
Migrate upgrade notifications to observer

NotificationService makes it too easy for a random piece of code to hook
into another, so this CL migrates upgrade notification to observer.

BUG=724540,268984
==========

[commit-bot: I haz the power, 2017-06-07 00:46:31]

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

[commit-bot: I haz the power, 2017-06-07 00:46:32]

Dry run: 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_...)

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

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

[commit-bot: I haz the power, 2017-06-07 02:22:31]

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-06-07 03:16:24]

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

[commit-bot: I haz the power, 2017-06-07 03:16:25]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[weidongg, 2017-06-07 16:46:54]

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

[commit-bot: I haz the power, 2017-06-07 16:47:16]

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-06-07 17:31:51]

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

[commit-bot: I haz the power, 2017-06-07 17:31:52]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[weidongg, 2017-06-07 18:13:26]

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

[commit-bot: I haz the power, 2017-06-07 18:13:51]

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-06-07 19:38:29]

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

[commit-bot: I haz the power, 2017-06-07 19:38:30]

Dry run: This issue passed the CQ dry run.

[weidongg, 2017-06-07 20:12:41]

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

[sky, 2017-06-07 23:01:42]

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/service_...
File chrome/browser/service_process/service_process_control.cc (left):

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/service_...
chrome/browser/service_process/service_process_control.cc:278:
DCHECK_EQ(chrome::NOTIFICATION_UPGRADE_RECOMMENDED, type);
Previously the observer was only added in a certain cases. Don't you need to
protect against that? By that I mean either set some boolean around line 248
(old) or conditionally add the observer?

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_client.h (right):

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_client.h:104: // UpgradeObserver
implementation.
Generally we group overrides, and don't have newlines between them. In other
words remove lines 103 and 104 (see policy::CloudPolicyStore::Observer overrides
below for what I mean)

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_view.cc (right):

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_view.cc:448: if
(OutdatedUpgradeBubbleView::IsAvailable()) {
no {}

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_view.cc:454: if
(OutdatedUpgradeBubbleView::IsAvailable()) {
no {}

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_view.h (right):

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_view.h:137: // UpgradeObserver
implementation.
Similar comment about merging.

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_view.h:142: void
OnCriticalUpgradeInstalled() override;
How about always overriding this function, and move the ifdef to the
implementation? Less ifdefs that way.

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/help/help_handler.cc (right):

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/help/help_handler.cc:376:
UpgradeDetector::GetInstance()->AddObserver(this);
I don't see a corresponding remove?

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

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/about_handler.cc:398: void
AboutHandler::OnJavascriptDisallowed() {
Is there a guarantee that this is called before the destructor?

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/upgrade_...
File chrome/browser/upgrade_detector.h (right):

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/upgrade_...
chrome/browser/upgrade_detector.h:89: void NotifyOutdatedInstall();
Why do the notify functions need to be public?

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/upgrade_...
chrome/browser/upgrade_detector.h:91: // Notifies that the current install is
outdated and auto-update (AU) is
I realize it's long, but go with AutoUpdate. au is cryptic.

[weidongg, 2017-06-08 02:28:43]

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/service_...
File chrome/browser/service_process/service_process_control.cc (left):

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/service_...
chrome/browser/service_process/service_process_control.cc:278:
DCHECK_EQ(chrome::NOTIFICATION_UPGRADE_RECOMMENDED, type);
On 2017/06/07 23:01:41, sky wrote:
> Previously the observer was only added in a certain cases. Don't you need to
> protect against that? By that I mean either set some boolean around line 248
> (old) or conditionally add the observer?

This function will be called multiple times. So I added a
|enable_upgrade_observer_| to prevent duplicate adding observer.

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_client.h (right):

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_client.h:104: // UpgradeObserver
implementation.
On 2017/06/07 23:01:41, sky wrote:
> Generally we group overrides, and don't have newlines between them. In other
> words remove lines 103 and 104 (see policy::CloudPolicyStore::Observer
overrides
> below for what I mean)

Done.

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_view.cc (right):

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_view.cc:448: if
(OutdatedUpgradeBubbleView::IsAvailable()) {
On 2017/06/07 23:01:41, sky wrote:
> no {}

Done.

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_view.cc:454: if
(OutdatedUpgradeBubbleView::IsAvailable()) {
On 2017/06/07 23:01:41, sky wrote:
> no {}

Done.

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_view.h (right):

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_view.h:137: // UpgradeObserver
implementation.
On 2017/06/07 23:01:41, sky wrote:
> Similar comment about merging.

Done.

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_view.h:142: void
OnCriticalUpgradeInstalled() override;
On 2017/06/07 23:01:41, sky wrote:
> How about always overriding this function, and move the ifdef to the
> implementation? Less ifdefs that way.

Done.

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/help/help_handler.cc (right):

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/help/help_handler.cc:376:
UpgradeDetector::GetInstance()->AddObserver(this);
On 2017/06/07 23:01:41, sky wrote:
> I don't see a corresponding remove?

Added AddObserver and RemoveObserver in constructor and destructor.
Used a flag to control the real action.

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

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/about_handler.cc:398: void
AboutHandler::OnJavascriptDisallowed() {
On 2017/06/07 23:01:42, sky wrote:
> Is there a guarantee that this is called before the destructor?

I am not sure, but I moved AddObserver and RemoveObserver to constructor and
destructor, and use a flag to control real action.

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/upgrade_...
File chrome/browser/upgrade_detector.h (right):

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/upgrade_...
chrome/browser/upgrade_detector.h:89: void NotifyOutdatedInstall();
On 2017/06/07 23:01:42, sky wrote:
> Why do the notify functions need to be public?

browser_commands.cc needs to notify upgrade to the observers. Should I make them
protected and add BrowserCommand as a friend class?

https://codereview.chromium.org/2925973002/diff/40001/chrome/browser/upgrade_...
chrome/browser/upgrade_detector.h:91: // Notifies that the current install is
outdated and auto-update (AU) is
On 2017/06/07 23:01:42, sky wrote:
> I realize it's long, but go with AutoUpdate. au is cryptic.

Done.

[weidongg, 2017-06-08 02:29:55]

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

[commit-bot: I haz the power, 2017-06-08 02:30:14]

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-06-08 03:29:43]

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

[commit-bot: I haz the power, 2017-06-08 03:29:43]

Dry run: This issue passed the CQ dry run.

[sky, 2017-06-08 18:09:52]

LGTM with a better name/comments.

https://codereview.chromium.org/2925973002/diff/60001/chrome/browser/service_...
File chrome/browser/service_process/service_process_control.h (right):

https://codereview.chromium.org/2925973002/diff/60001/chrome/browser/service_...
chrome/browser/service_process/service_process_control.h:248: };
DISALLOW... (while you are here).

https://codereview.chromium.org/2925973002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/help/help_handler.h (right):

https://codereview.chromium.org/2925973002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/help/help_handler.h:127: // Whether to start listening
to upgrade detector.
How about: If true changes to UpgradeObserver are applied, if false they are
ignored.
Same comment in the other places you use this pattern.
Maybe rename to apply_changes_from_upgrade_observer_?

[weidongg, 2017-06-08 18:58:08]

https://codereview.chromium.org/2925973002/diff/60001/chrome/browser/service_...
File chrome/browser/service_process/service_process_control.h (right):

https://codereview.chromium.org/2925973002/diff/60001/chrome/browser/service_...
chrome/browser/service_process/service_process_control.h:248: };
On 2017/06/08 18:09:52, sky wrote:
> DISALLOW... (while you are here).

https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...
says DISALLOW_COPY_AND_ASSIGN is no longer permitted in new code. Should I add
this here?

https://codereview.chromium.org/2925973002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/help/help_handler.h (right):

https://codereview.chromium.org/2925973002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/help/help_handler.h:127: // Whether to start listening
to upgrade detector.
On 2017/06/08 18:09:52, sky wrote:
> How about: If true changes to UpgradeObserver are applied, if false they are
> ignored.
> Same comment in the other places you use this pattern.
> Maybe rename to apply_changes_from_upgrade_observer_?

Yes, that's better, thanks. Done.

[sky, 2017-06-08 21:28:01]

https://codereview.chromium.org/2925973002/diff/60001/chrome/browser/service_...
File chrome/browser/service_process/service_process_control.h (right):

https://codereview.chromium.org/2925973002/diff/60001/chrome/browser/service_...
chrome/browser/service_process/service_process_control.h:248: };
On 2017/06/08 18:58:07, weidongg wrote:
> On 2017/06/08 18:09:52, sky wrote:
> > DISALLOW... (while you are here).
> 
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...
> says DISALLOW_COPY_AND_ASSIGN is no longer permitted in new code. Should I add
> this here?

See chromium-dev discussions. I don't see this code deleting copy and assign
though.

[weidongg, 2017-06-08 22:18:25]

Thanks for the review.

https://codereview.chromium.org/2925973002/diff/60001/chrome/browser/service_...
File chrome/browser/service_process/service_process_control.h (right):

https://codereview.chromium.org/2925973002/diff/60001/chrome/browser/service_...
chrome/browser/service_process/service_process_control.h:248: };
On 2017/06/08 21:28:00, sky wrote:
> On 2017/06/08 18:58:07, weidongg wrote:
> > On 2017/06/08 18:09:52, sky wrote:
> > > DISALLOW... (while you are here).
> > 
> >
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...
> > says DISALLOW_COPY_AND_ASSIGN is no longer permitted in new code. Should I
add
> > this here?
> 
> See chromium-dev discussions. I don't see this code deleting copy and assign
> though.

Added. Thanks.

[weidongg, 2017-06-08 22:59:47]

The CQ bit was checked by weidongg@chromium.org

[weidongg, 2017-06-08 22:59:48]

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

[commit-bot: I haz the power, 2017-06-08 23:00:46]

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-09 00:01:10]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496962787741300,
"parent_rev": "e8d7f36c67e1fc0d5f4b37682bc72c5be565a3ce", "commit_rev":
"0469523c2aafcfb307f7a9190140194d169cb72c"}

[commit-bot: I haz the power, 2017-06-09 00:01:22]

Description was changed from

==========
Migrate upgrade notifications to observer

NotificationService makes it too easy for a random piece of code to hook
into another, so this CL migrates upgrade notification to observer.

BUG=724540,268984
==========

to

==========
Migrate upgrade notifications to observer

NotificationService makes it too easy for a random piece of code to hook
into another, so this CL migrates upgrade notification to observer.

BUG=724540,268984

Review-Url: https://codereview.chromium.org/2925973002
Cr-Commit-Position: refs/heads/master@{#478127}
Committed:
https://chromium.googlesource.com/chromium/src/+/0469523c2aafcfb307f7a9190140...
==========

[commit-bot: I haz the power, 2017-06-09 00:01:24]

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