Make UpgradeDetector listen to VariationsService changes.

Make UpgradeDetector listen to VariationsService changes.

This allows badging the wrench menu and showing the "Update
Google Chrome" item in the menu when critical variations
updates are available.

BUG=394855
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288147

[Alexei Svitkine (slow), 2014-08-01 15:36:00]

MAD, PTAL. (Finnur is on vacation, so only set him as cc).

In the end, I decided to incorporate this into UpgradeDetectorImpl, rather than
putting it into a separate class, since this way I can re-use the notification
escalation timer logic (i.e. where the more time has elapsed, the more severe
the notification - including the icon badge).

[MAD, 2014-08-01 17:41:35]

Très cool... Thanks!

LGTM with a couple minor nits.

BYE
MAD

https://codereview.chromium.org/421643002/diff/70001/chrome/browser/upgrade_d...
File chrome/browser/upgrade_detector.h (right):

https://codereview.chromium.org/421643002/diff/70001/chrome/browser/upgrade_d...
chrome/browser/upgrade_detector.h:85: // idle state. Protected and virtual so
that it could overridden by tests.
could "be" overridden.

https://codereview.chromium.org/421643002/diff/70001/chrome/browser/upgrade_d...
File chrome/browser/upgrade_detector_impl.cc (right):

https://codereview.chromium.org/421643002/diff/70001/chrome/browser/upgrade_d...
chrome/browser/upgrade_detector_impl.cc:290: // since it outlines the
VariationsService.
outlines -> outlives

[sky, 2014-08-04 18:24:08]

https://codereview.chromium.org/421643002/diff/70001/chrome/browser/upgrade_d...
File chrome/browser/upgrade_detector.cc (right):

https://codereview.chromium.org/421643002/diff/70001/chrome/browser/upgrade_d...
chrome/browser/upgrade_detector.cc:81: critical_experiment_updates_available_) {
nit: align with ( on previous line.

https://codereview.chromium.org/421643002/diff/70001/chrome/browser/upgrade_d...
File chrome/browser/upgrade_detector.h (right):

https://codereview.chromium.org/421643002/diff/70001/chrome/browser/upgrade_d...
chrome/browser/upgrade_detector.h:86: virtual void TriggerCriticalUpdate();
How about an observer instead of this? The observer could handle both the
TriggerNotification and TriggerCriticalNotification. You could install a default
observer that converts the observer function into a
content::NotificationService::current()->Notify. At a later day you could remove
the content::NotificationService::current()->Notify and make consumers install
an observer.

https://codereview.chromium.org/421643002/diff/70001/chrome/browser/upgrade_d...
chrome/browser/upgrade_detector.h:110: // Whether "best effort" experiment
updates are available.
Style guide says no protected members.

https://codereview.chromium.org/421643002/diff/70001/chrome/browser/upgrade_d...
File chrome/browser/upgrade_detector_impl.cc (right):

https://codereview.chromium.org/421643002/diff/70001/chrome/browser/upgrade_d...
chrome/browser/upgrade_detector_impl.cc:513: base::Time::Now() -
upgrade_detected_time_;
Don't you want timeticks?

[Alexei Svitkine (slow), 2014-08-04 19:43:11]

Thanks, PTAL.

https://codereview.chromium.org/421643002/diff/70001/chrome/browser/upgrade_d...
File chrome/browser/upgrade_detector.cc (right):

https://codereview.chromium.org/421643002/diff/70001/chrome/browser/upgrade_d...
chrome/browser/upgrade_detector.cc:81: critical_experiment_updates_available_) {
On 2014/08/04 18:24:07, sky wrote:
> nit: align with ( on previous line.

Done.

https://codereview.chromium.org/421643002/diff/70001/chrome/browser/upgrade_d...
File chrome/browser/upgrade_detector.h (right):

https://codereview.chromium.org/421643002/diff/70001/chrome/browser/upgrade_d...
chrome/browser/upgrade_detector.h:86: virtual void TriggerCriticalUpdate();
On 2014/08/04 18:24:07, sky wrote:
> How about an observer instead of this? The observer could handle both the
> TriggerNotification and TriggerCriticalNotification. You could install a
default
> observer that converts the observer function into a
> content::NotificationService::current()->Notify. At a later day you could
remove
> the content::NotificationService::current()->Notify and make consumers install
> an observer.

I'm not sure it makes sense in this case. The TriggerCriticalUpdate()
implementation does the following:

  - Start a repeating timer that checks if the system is idle
  - The timer task checks if the system is idle and if it is, calls
IdleCallback()
  - IdleCallback() then checks idle state and other info (e.g. incognito session
active) to see if it's an appropriate time to trigger the notification to show
the countdown UI ... or if computer is locked, will actually restart Chrome.

So basically, the implementation of this method is not "tell observers that a
critical update is ready" ... but rather "do some things behind the scenes and
then later once we determined its a good time, tell observers that a critical
update is ready". Thus converting this specific method to an observer method
doesn't make sense.

(I understand your motivation for suggesting this - i.e. to make it easy to
eventually remove the use of notifications from this code - but I think such a
change would be orthogonal to me adding this virtual method and thus doesn't
make sense to include in this CL.)

https://codereview.chromium.org/421643002/diff/70001/chrome/browser/upgrade_d...
chrome/browser/upgrade_detector.h:110: // Whether "best effort" experiment
updates are available.
On 2014/08/04 18:24:07, sky wrote:
> Style guide says no protected members.

Indeed, done. There were existing protected members here as well, so I moved
them too. (Result is introducing some protected methods, but that's the right
thing to do according to the style guide.)

https://codereview.chromium.org/421643002/diff/70001/chrome/browser/upgrade_d...
File chrome/browser/upgrade_detector_impl.cc (right):

https://codereview.chromium.org/421643002/diff/70001/chrome/browser/upgrade_d...
chrome/browser/upgrade_detector_impl.cc:290: // since it outlines the
VariationsService.
On 2014/08/01 17:41:30, MAD wrote:
> outlines -> outlives

Done.

https://codereview.chromium.org/421643002/diff/70001/chrome/browser/upgrade_d...
chrome/browser/upgrade_detector_impl.cc:513: base::Time::Now() -
upgrade_detected_time_;
On 2014/08/04 18:24:07, sky wrote:
> Don't you want timeticks?

This CL doesn't actually change the logic around this (just refactors it).

I agree that time ticks makes more sense here. But such a change should be in a
separate CL, so I'll do it after this lands. I filed http://crbug.com/400472 for
that fix.

[sky, 2014-08-04 20:30:41]

Ok, makes sense, LGTM

[Alexei Svitkine (slow), 2014-08-05 17:06:53]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-08-05 17:07:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/421643002/130001

[Alexei Svitkine (slow), 2014-08-05 20:37:49]

The CQ bit was unchecked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2014-08-06 03:53:54]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-08-06 03:54:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/421643002/170001

[commit-bot: I haz the power, 2014-08-06 09:56:42]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-06 10:16:54]

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

[commit-bot: I haz the power, 2014-08-06 10:16:55]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexei Svitkine (slow), 2014-08-06 15:03:20]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-08-06 15:04:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/421643002/190001

[commit-bot: I haz the power, 2014-08-06 21:06:07]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[commit-bot: I haz the power, 2014-08-06 21:36:01]

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

[commit-bot: I haz the power, 2014-08-06 21:36:02]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)

[Alexei Svitkine (slow), 2014-08-07 02:09:49]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-08-07 02:13:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/421643002/190001

[Ken Russell (switch to Gerrit), 2014-08-07 07:00:58]

The tab_capture_end2end_tests failure appears both real and caused by this CL.
Please don't dismiss it but investigate it locally. Please contact me if you
have any trouble figuring out how to run the test locally.

[commit-bot: I haz the power, 2014-08-07 11:01:03]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[commit-bot: I haz the power, 2014-08-07 11:26:14]

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

[commit-bot: I haz the power, 2014-08-07 11:26:15]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[Alexei Svitkine (slow), 2014-08-07 12:20:46]

Thanks, will take a look. Very surprising that it would affect something
like that...


On Thu, Aug 7, 2014 at 7:26 AM, <commit-bot@chromium.org> wrote:

> Try jobs failed on following builders:
>   linux_gpu_triggered_tests on tryserver.chromium.gpu
> (http://build.chromium.org/p/tryserver.chromium.gpu/
> builders/linux_gpu_triggered_tests/builds/42797)
>
> https://codereview.chromium.org/421643002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexei Svitkine (slow), 2014-08-07 17:57:25]

Was able to repro locally and confirm the patch causes the failure. Will debug
more to see what's going on.

[Alexei Svitkine (slow), 2014-08-07 18:18:38]

On 2014/08/07 17:57:25, Alexei Svitkine wrote:
> Was able to repro locally and confirm the patch causes the failure. Will debug
> more to see what's going on.

Uploaded patchset that fixes the issue. It was caused by VariationsService being
NULL in Chromium builds (not sure why other browser tests didn't catch it?).

[Alexei Svitkine (slow), 2014-08-07 18:19:08]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-08-07 18:23:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/421643002/210001

[commit-bot: I haz the power, 2014-08-07 21:57:30]

Change committed as 288147

[Ken Russell (switch to Gerrit), 2014-08-07 22:01:26]

On 2014/08/07 18:18:38, Alexei Svitkine wrote:
> On 2014/08/07 17:57:25, Alexei Svitkine wrote:
> > Was able to repro locally and confirm the patch causes the failure. Will
debug
> > more to see what's going on.
> 
> Uploaded patchset that fixes the issue. It was caused by VariationsService
being
> NULL in Chromium builds (not sure why other browser tests didn't catch it?).

Could you please add a more targeted test for that problem?

[Alexei Svitkine (slow), 2014-08-08 04:15:18]

On 2014/08/07 22:01:26, Ken Russell wrote:
> On 2014/08/07 18:18:38, Alexei Svitkine wrote:
> > On 2014/08/07 17:57:25, Alexei Svitkine wrote:
> > > Was able to repro locally and confirm the patch causes the failure. Will
> debug
> > > more to see what's going on.
> > 
> > Uploaded patchset that fixes the issue. It was caused by VariationsService
> being
> > NULL in Chromium builds (not sure why other browser tests didn't catch it?).
> 
> Could you please add a more targeted test for that problem?

I'm not sure why the tests in this CL didn't catch it. They create an
UpgradeDetectorImpl, which uses VariationsService in its constructor. Which
should be NULL in Chromium builds. Which should presumably should hit the same
error. Will investigate.

[Alexei Svitkine (slow), 2014-08-08 17:14:02]

On 2014/08/08 04:15:18, Alexei Svitkine wrote:
> On 2014/08/07 22:01:26, Ken Russell wrote:
> > On 2014/08/07 18:18:38, Alexei Svitkine wrote:
> > > On 2014/08/07 17:57:25, Alexei Svitkine wrote:
> > > > Was able to repro locally and confirm the patch causes the failure. Will
> > debug
> > > > more to see what's going on.
> > > 
> > > Uploaded patchset that fixes the issue. It was caused by VariationsService
> > being
> > > NULL in Chromium builds (not sure why other browser tests didn't catch
it?).
> > 
> > Could you please add a more targeted test for that problem?
> 
> I'm not sure why the tests in this CL didn't catch it. They create an
> UpgradeDetectorImpl, which uses VariationsService in its constructor. Which
> should be NULL in Chromium builds. Which should presumably should hit the same
> error. Will investigate.

Indeed, the new tests fail without the fix from patch set 9 in a Chromium build
(I was testing in an official build when developing). I wonder why they didn't
fail on the bots. Perhaps they didn't get run? Checking...

[Alexei Svitkine (slow), 2014-08-08 17:18:33]

On 2014/08/08 17:14:02, Alexei Svitkine wrote:
> On 2014/08/08 04:15:18, Alexei Svitkine wrote:
> > On 2014/08/07 22:01:26, Ken Russell wrote:
> > > On 2014/08/07 18:18:38, Alexei Svitkine wrote:
> > > > On 2014/08/07 17:57:25, Alexei Svitkine wrote:
> > > > > Was able to repro locally and confirm the patch causes the failure.
Will
> > > debug
> > > > > more to see what's going on.
> > > > 
> > > > Uploaded patchset that fixes the issue. It was caused by
VariationsService
> > > being
> > > > NULL in Chromium builds (not sure why other browser tests didn't catch
> it?).
> > > 
> > > Could you please add a more targeted test for that problem?
> > 
> > I'm not sure why the tests in this CL didn't catch it. They create an
> > UpgradeDetectorImpl, which uses VariationsService in its constructor. Which
> > should be NULL in Chromium builds. Which should presumably should hit the
same
> > error. Will investigate.
> 
> Indeed, the new tests fail without the fix from patch set 9 in a Chromium
build
> (I was testing in an official build when developing). I wonder why they didn't
> fail on the bots. Perhaps they didn't get run? Checking...

And indeed the tests failed on the bots with the previous patch sets. However,
it looks like the cq didn't mark them as failed and instead those runs show up
as yellow instead of red. But drilling down shows the failures.

[Alexei Svitkine (slow), 2014-08-08 17:21:32]

+sergeyberezin, +phajdan.jr who I think work on the cq.

Any idea why the cq didn't mark the runs from patch set 8 such as
win_chromium_rel as red, given the failures inside them? (They appear as yellow
on codereview and the failed cq email didn't mention those failures.)

[chromium-reviews, 2014-08-12 23:03:46]

It looks like a failure of the buildbot master to communicate the build
status update to Rietveld. It happens occasionally (e.g. due to network
failure), but fortunately quite rare.

Sergey.


On Fri, Aug 8, 2014 at 10:21 AM, <asvitkine@chromium.org> wrote:

> +sergeyberezin, +phajdan.jr who I think work on the cq.
>
> Any idea why the cq didn't mark the runs from patch set 8 such as
> win_chromium_rel as red, given the failures inside them? (They appear as
> yellow
> on codereview and the failed cq email didn't mention those failures.)
>
> https://codereview.chromium.org/421643002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexei Svitkine (slow), 2014-08-13 14:46:32]

On 2014/08/12 23:03:46, chromium-reviews wrote:
> It looks like a failure of the buildbot master to communicate the build
> status update to Rietveld. It happens occasionally (e.g. due to network
> failure), but fortunately quite rare.
> 
> Sergey.

It seems like it was happening pretty consistently for each patchset in this CL
(look at all the yellow _rel bot runs on each patchset). Doesn't sound like an
intermittent network failure to me - any chance there's a bug parsing the
buildbot output in this case?