Add an API to component_updater that asks to do an update check "now".

Add an API to component_updater that asks to do an update check "now".

With this API, users of component_updater can trigger an
update or install when deemed important.  E.g., if a
component is not yet installed, but is about to be used.

There is not a guarantee that the item will actually
be updated.  There is only a good chance.  It can
still be preempted by other items that may have an update
available as well, or if multiple components
use this interface one will trump the others.

Use this to kick off a check of PNaCl if flag enabled, but
not yet installed.

Sort of related to:
BUG=107438
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182399

[jvoung (off chromium), 2013-01-25 17:48:30]

https://codereview.chromium.org/12054003/diff/1/chrome/browser/component_upda...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/12054003/diff/1/chrome/browser/component_upda...
chrome/browser/component_updater/component_updater_service.h:143: virtual Status
PingUpdateCheck(const CrxComponent& component) = 0;
Would it make sense to add a callback parameter, or just have people register
against the NotificationService and listen for
NOTIFICATION_COMPONENT_UPDATE_READY.

If they listen for the notification... what if there is an error and the ping
does nothing?  Listen component updater going back to sleep, and use that as a
signal?

[cpu_(ooo_6.6-7.5), 2013-01-30 01:57:48]

one issue that worries me here is that there could be a manifest fetch in flight
or a component install in flight. 

Your change invalidates this assumption so I am thinking there are bugs there.

[jvoung (off chromium), 2013-01-30 02:20:21]

https://codereview.chromium.org/12054003/diff/23001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/12054003/diff/23001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:548: }
Good point about having fetches or installs in flight.  This ping could end with
scheduling a timer, and the in flight step will also schedule one when it is
completed, so that would be bad.


Hmm, if it's in the process of fetching a manifest or installing, then the timer
isn't running, right?  Can we detect that here, and instead of doing
ProcessWorkItems (which could end with scheduling a timer), queue up something
so that when the in-flight step finishes and calls "ScheduleNextRun", it
schedules with a step_delay instead of a long one?  That gets something going
"quickly" without actually scheduling a timer.

[cpu_(ooo_6.6-7.5), 2013-01-30 21:13:05]

explore this idea:

Instead of PingUpdateCheck calling ProcessWorkItems, simply have the function
re-schedule the timer sooner.

Also note that if there is no update, I don't see a way for that to be
communicated, I assume you are showing some kind of spinner UI while the
installer kicks in?

https://codereview.chromium.org/12054003/diff/23001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/12054003/diff/23001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.h:143: virtual Status
PingUpdateCheck(const CrxComponent& component) = 0;
Not liking the word ping in the name, something like CheckSoon or CheckNow might
be better

[jvoung (off chromium), 2013-01-31 19:51:43]

Something like this?

Is it okay to add more notifications for handling spinners or other UI?

https://codereview.chromium.org/12054003/diff/23001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_service.h (right):

https://codereview.chromium.org/12054003/diff/23001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.h:143: virtual Status
PingUpdateCheck(const CrxComponent& component) = 0;
On 2013/01/30 21:13:05, cpu wrote:
> Not liking the word ping in the name, something like CheckSoon or CheckNow
might
> be better

Done.

[jvoung (off chromium), 2013-02-04 17:27:45]

cleaned up a little bit more, ptal

[jvoung (off chromium), 2013-02-04 23:53:02]

On 2013/02/04 17:27:45, jvoung (cr) wrote:
> cleaned up a little bit more, ptal

Here is possible followup CL to avoid installing within 10 seconds of boot, and
instead install when pnacl is actually used:

https://codereview.chromium.org/12184029/

[cpu_(ooo_6.6-7.5), 2013-02-05 00:04:47]

yeah this is more in line to what I want.

https://codereview.chromium.org/12054003/diff/35001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/12054003/diff/35001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:459: // (not the
others).
this again in terms of fairness.

https://codereview.chromium.org/12054003/diff/35001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:481:
chrome::NOTIFICATION_COMPONENT_UPDATE_COMPLETE,
not sure we need this one, I mean the installer got called so the installer is
the one to know.

https://codereview.chromium.org/12054003/diff/35001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:637: }
hmm, we are trying to be fair here with all the components, I don't want that
one component can pre-empt all others.

https://codereview.chromium.org/12054003/diff/35001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:654: //
ProcessPendingItems, not ProcessRequestedItem.
assuming that the config_ object returns constant values, which is something I
don't want unless is impossible to do otherwise, that is why I keep calling
config_ methods all the time instead of calling them once.

[cpu_(ooo_6.6-7.5), 2013-02-05 00:15:50]

To be more precise, I think we should speed up the timer as you have it but run
over all the components, meaning that the one that gets installed soon might not
be yours

In order to make yours have a chance, the only thing it needs is its state
changed to kNew

How about this
1. Find component change state to kNew if
   a. not updating or about to update
   b. not checked in the last config->OnDemandDelay() minutes
2. Shorten the timer timeout

[jvoung (off chromium), 2013-02-05 00:17:56]

https://codereview.chromium.org/12054003/diff/35001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/12054003/diff/35001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:481:
chrome::NOTIFICATION_COMPONENT_UPDATE_COMPLETE,
On 2013/02/05 00:04:47, cpu wrote:
> not sure we need this one, I mean the installer got called so the installer is
> the one to know.

I imagine that it is the "thing" that called CheckForUpdateSoon(), isn't
necessarily the installer (see the other CL). It makes sense to notify whoever
called CheckForUpdateSoon().

Or, should everything go through the installer, even if the installer is the one
that notifies others?

https://codereview.chromium.org/12054003/diff/35001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:637: }
On 2013/02/05 00:04:47, cpu wrote:
> hmm, we are trying to be fair here with all the components, I don't want that
> one component can pre-empt all others.

Should the effect of the CL not be to "cut in line", but to keep doing short
step_delays, for everyone, until the requested item makes it to the front of the
line?

https://codereview.chromium.org/12054003/diff/35001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:654: //
ProcessPendingItems, not ProcessRequestedItem.
On 2013/02/05 00:04:47, cpu wrote:
> assuming that the config_ object returns constant values, which is something I
> don't want unless is impossible to do otherwise, that is why I keep calling
> config_ methods all the time instead of calling them once.

The step_delay that is saved to the variable is the one that was compared
against remaining_delay, though.  If config_ can return a different value, then
the comparison doesn't make sense?

Should I just always reset the timer and skip the comparison?

[jvoung (off chromium), 2013-02-05 00:54:15]

On 2013/02/05 00:15:50, cpu wrote:
> To be more precise, I think we should speed up the timer as you have it but
run
> over all the components, meaning that the one that gets installed soon might
not
> be yours
> 
> In order to make yours have a chance, the only thing it needs is its state
> changed to kNew
> 
> How about this
> 1. Find component change state to kNew if
>    a. not updating or about to update
>    b. not checked in the last config->OnDemandDelay() minutes


How many minutes should OnDemandDelay() minutes be (a new parameter right)?

It could be quite painful to try to get something installed on demand, to wait
many minutes before checking again?


> 2. Shorten the timer timeout

[jvoung (off chromium), 2013-02-05 17:06:18]

On 2013/02/05 00:54:15, jvoung (cr) wrote:
> On 2013/02/05 00:15:50, cpu wrote:
> > To be more precise, I think we should speed up the timer as you have it but
> run
> > over all the components, meaning that the one that gets installed soon might
> not
> > be yours
> > 
> > In order to make yours have a chance, the only thing it needs is its state
> > changed to kNew
> > 
> > How about this
> > 1. Find component change state to kNew if
> >    a. not updating or about to update
> >    b. not checked in the last config->OnDemandDelay() minutes
> 
> 
> How many minutes should OnDemandDelay() minutes be (a new parameter right)?
> 
> It could be quite painful to try to get something installed on demand, to wait
> many minutes before checking again?


Or what if 1b is added with OnDemandDelay() being pretty long delay to avoid
abuse, but there is still a guarantee that something will happen for the
requested component?  It doesn't delay updates for other components by much, but
it does cause more work to happen (which would happen with 2, shortening the
timer timeout anyway)?


> 
> 
> > 2. Shorten the timer timeout

[jvoung (off chromium), 2013-02-06 19:54:23]

On 2013/02/05 17:06:18, jvoung (cr) wrote:
> On 2013/02/05 00:54:15, jvoung (cr) wrote:
> > On 2013/02/05 00:15:50, cpu wrote:
> > > To be more precise, I think we should speed up the timer as you have it
but
> > run
> > > over all the components, meaning that the one that gets installed soon
might
> > not
> > > be yours
> > > 
> > > In order to make yours have a chance, the only thing it needs is its state
> > > changed to kNew
> > > 
> > > How about this
> > > 1. Find component change state to kNew if
> > >    a. not updating or about to update
> > >    b. not checked in the last config->OnDemandDelay() minutes
> > 
> > 
> > How many minutes should OnDemandDelay() minutes be (a new parameter right)?
> > 
> > It could be quite painful to try to get something installed on demand, to
wait
> > many minutes before checking again?
> 
> 
> Or what if 1b is added with OnDemandDelay() being pretty long delay to avoid
> abuse, but there is still a guarantee that something will happen for the
> requested component?  It doesn't delay updates for other components by much,
but
> it does cause more work to happen (which would happen with 2, shortening the
> timer timeout anyway)?
> 
> 
> > 
> > 
> > > 2. Shorten the timer timeout


Would that be okay, or ?

[cpu_(ooo_6.6-7.5), 2013-02-06 22:25:32]

the delay unit should be in seconds.

https://codereview.chromium.org/12054003/diff/35001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/12054003/diff/35001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:481:
chrome::NOTIFICATION_COMPONENT_UPDATE_COMPLETE,
That would be ideal, that way you don't have to have code that filters for your
notification.

In general the notifications that are in this code are not doing anything except
helping tests, probably I over designed that part.

https://codereview.chromium.org/12054003/diff/35001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:637: }
On 2013/02/05 00:17:56, jvoung (cr) wrote:
> On 2013/02/05 00:04:47, cpu wrote:
> > hmm, we are trying to be fair here with all the components, I don't want
that
> > one component can pre-empt all others.
> 
> Should the effect of the CL not be to "cut in line", but to keep doing short
> step_delays, for everyone, until the requested item makes it to the front of
the
> line?

Yeah, until the component changes from kNew

https://codereview.chromium.org/12054003/diff/35001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:654: //
ProcessPendingItems, not ProcessRequestedItem.
Yes, that would be best.

[jvoung (off chromium), 2013-02-11 21:04:26]

https://codereview.chromium.org/12054003/diff/35001/chrome/browser/component_...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/12054003/diff/35001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:481:
chrome::NOTIFICATION_COMPONENT_UPDATE_COMPLETE,
On 2013/02/06 22:25:32, cpu wrote:
> That would be ideal, that way you don't have to have code that filters for
your
> notification.
> 
> In general the notifications that are in this code are not doing anything
except
> helping tests, probably I over designed that part.


Alright, removed the use of notification service.  Will have to think of
something else to notify a user of errors during kDownloading/kUpdating, since
there is also not a guarantee that the requested items go beyond the
kChecking/kCanUpdate stages, since only one of the kCanUpdate items get updated
at once?

https://codereview.chromium.org/12054003/diff/35001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:637: }
On 2013/02/06 22:25:32, cpu wrote:
> On 2013/02/05 00:17:56, jvoung (cr) wrote:
> > On 2013/02/05 00:04:47, cpu wrote:
> > > hmm, we are trying to be fair here with all the components, I don't want
> that
> > > one component can pre-empt all others.
> > 
> > Should the effect of the CL not be to "cut in line", but to keep doing short
> > step_delays, for everyone, until the requested item makes it to the front of
> the
> > line?
> 
> Yeah, until the component changes from kNew

Done.

https://codereview.chromium.org/12054003/diff/35001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:654: //
ProcessPendingItems, not ProcessRequestedItem.
On 2013/02/06 22:25:32, cpu wrote:
> Yes, that would be best.

Done.

[jvoung (off chromium), 2013-02-11 23:41:59]

On 2013/02/04 23:53:02, jvoung (cr) wrote:
> On 2013/02/04 17:27:45, jvoung (cr) wrote:
> > cleaned up a little bit more, ptal
> 
> Here is possible followup CL to avoid installing within 10 seconds of boot,
and
> instead install when pnacl is actually used:
> 
> https://codereview.chromium.org/12184029/

^^^^

Also updated that followup/sketch CL to go through the particular installer for
callbacks, instead of using the notification service.

[cpu_(ooo_6.6-7.5), 2013-02-12 23:27:30]

lgtm

[cpu_(ooo_6.6-7.5), 2013-02-12 23:34:26]

https://codereview.chromium.org/12054003/diff/62001/chrome/browser/component_...
File chrome/browser/component_updater/pnacl/pnacl_component_installer.cc
(right):

https://codereview.chromium.org/12054003/diff/62001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_component_installer.cc:342: // we
want it to be available "soon", so kick off an update check
wait, where is the command line check?

[jvoung (off chromium), 2013-02-12 23:38:37]

https://codereview.chromium.org/12054003/diff/62001/chrome/browser/component_...
File chrome/browser/component_updater/pnacl/pnacl_component_installer.cc
(right):

https://codereview.chromium.org/12054003/diff/62001/chrome/browser/component_...
chrome/browser/component_updater/pnacl/pnacl_component_installer.cc:342: // we
want it to be available "soon", so kick off an update check
On 2013/02/12 23:34:26, cpu wrote:
> wait, where is the command line check?

In chrome/browser/chrome_browser_main.cc, PNaCl is not registered at all if
--enable-pnacl isn't set, so it's a bit indirect:


void RegisterComponentsForUpdate(const CommandLine& command_line) {
  ComponentUpdateService* cus = g_browser_process->component_updater();
  if (!cus)
    return;
  // Registration can be before of after cus->Start() so it is ok to post
  // a task to the UI thread to do registration once you done the necessary
  // file IO to know you existing component version.
  RegisterRecoveryComponent(cus, g_browser_process->local_state());

  // ... stuff

  if (command_line.HasSwitch(switches::kEnablePnacl))
    RegisterPnaclComponent(cus);

  cus->Start();
}

[cpu_(ooo_6.6-7.5), 2013-02-13 00:03:24]

great, please move the check into the pnacl_component_installer.cc

you can add that to this CL or do a separate CL.

[jvoung (off chromium), 2013-02-13 00:15:55]

On 2013/02/13 00:03:24, cpu wrote:
> great, please move the check into the pnacl_component_installer.cc
> 
> you can add that to this CL or do a separate CL.

Moved in this CL.

[cpu_(ooo_6.6-7.5), 2013-02-13 03:03:28]

lgtm

[commit-bot: I haz the power, 2013-02-13 16:03:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jvoung@chromium.org/12054003/58005

[commit-bot: I haz the power, 2013-02-13 16:03:35]

Presubmit check for 12054003-58005 failed and returned exit status 1.

INFO:root:Found 7 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 4.1s to calculate.

[jvoung (off chromium), 2013-02-13 16:06:10]

On 2013/02/13 16:03:35, I haz the power (commit-bot) wrote:
> Presubmit check for 12054003-58005 failed and returned exit status 1.
> 
> INFO:root:Found 7 file(s).
> 
> Running presubmit commit checks ...
> Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
> Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     chrome
> 
> Presubmit checks took 4.1s to calculate.

+jam for chrome/ OWNERS

[jam, 2013-02-13 22:06:05]

On 2013/02/13 16:06:10, jvoung (cr) wrote:
> On 2013/02/13 16:03:35, I haz the power (commit-bot) wrote:
> > Presubmit check for 12054003-58005 failed and returned exit status 1.
> > 
> > INFO:root:Found 7 file(s).
> > 
> > Running presubmit commit checks ...
> > Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
> > Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
> > 
> > ** Presubmit ERRORS **
> > Missing LGTM from an OWNER for files in these directories:
> >     chrome
> > 
> > Presubmit checks took 4.1s to calculate.
> 
> +jam for chrome/ OWNERS

lgtm

please see chrome/OWNERS next time, i only do reviews for areas that i'm an
active contributor to. for other areas, use the reviewers listed in the reviewer
section

[commit-bot: I haz the power, 2013-02-13 22:26:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jvoung@chromium.org/12054003/58005

[commit-bot: I haz the power, 2013-02-13 23:20:53]

Retried try job too often on linux_chromeos for step(s) aura_unittests,
base_unittests, browser_tests, cacheinvalidation_unittests, check_deps,
chromeos_unittests, components_unittests, content_browsertests,
content_unittests, crypto_unittests, dbus_unittests, device_unittests,
gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, net_unittests, ppapi_unittests, printing_unittests,
sandbox_linux_unittests, sql_unittests, sync_unit_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...