Implement coarse "on demand" functionality for update checks.

Implement coarse "on demand" functionality for update checks.

After this changelist, first-installation of components initiated by
CheckForUpdateSoon will bear an "installsource=ondemand" flag in the
request. (The server honors this flag by serving updates at 100% rather
than a gated fraction.)

"On demand" checks do not have a higher priority than normal 
CheckForUpdateSoon() update checks: they are not promoted to be handled
first or preempt component downloads/updates/installations that are
already in progress.

NOTRY=true
BUG=245318
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208256

[waffles, 2013-06-21 00:13:25]

Hi sorin, cpu,

Please take a look at this changelist to add on-demand functionality to the
component updater.

Please do not CQ or try this changelist until
https://codereview.chromium.org/15908002/ has made it to LKGR, as the patch is
computed from those files. (I don't think there's actually a merge conflict, so
I could change that, but why risk it?)

Thanks!
-Josh

[cpu_(ooo_6.6-7.5), 2013-06-21 00:22:56]

https://codereview.chromium.org/17419011/diff/4001/chrome/browser/component_u...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/17419011/diff/4001/chrome/browser/component_u...
chrome/browser/component_updater/component_updater_service.cc:706:
item->component.version.Equals(Version("0"));
I don't think so. This is hacky and the on-demand is not just at v0 but in all
cases where the current component is incompatible with the action that the user
needs to do.

How about we check if the item is on the requested_work_items_ and set the flag
if so?

[waffles, 2013-06-21 01:52:08]

Thanks for the quick turnaround!

https://codereview.chromium.org/17419011/diff/4001/chrome/browser/component_u...
File chrome/browser/component_updater/component_updater_service.cc (right):

https://codereview.chromium.org/17419011/diff/4001/chrome/browser/component_u...
chrome/browser/component_updater/component_updater_service.cc:706:
item->component.version.Equals(Version("0"));
On 2013/06/21 00:22:56, cpu wrote:
> I don't think so. This is hacky and the on-demand is not just at v0 but in all
> cases where the current component is incompatible with the action that the
user
> needs to do.
> 
> How about we check if the item is on the requested_work_items_ and set the
flag
> if so?

Done.

I had the Version==0 check in response to mevissen@'s concerns about components
calling CheckForUpdateSoon too aggressively and triggering constant "on demand"
updatechecks that aren't on demand. Indeed, PNaCl calls CheckForUpdateSoon every
browser boot.

Closer inspection shows that there is a CL in the CQ that changes that, and
additionally only calls CheckForUpdateSoon if the version == 0; so I've removed
the version check here.

[waffles, 2013-06-22 00:07:37]

On 2013/06/21 00:13:25, waffles wrote:
> Hi sorin, cpu,
> 
> Please take a look at this changelist to add on-demand functionality to the
> component updater.
> 
> Please do not CQ or try this changelist until
> https://codereview.chromium.org/15908002/ has made it to LKGR, as the patch is
> computed from those files. (I don't think there's actually a merge conflict,
so
> I could change that, but why risk it?)
> 
> Thanks!
> -Josh

(This is now safe to commit, in terms of being blocked on 15908002. The blocking
issue is now part of LKGR.)

[Sorin Jianu, 2013-06-24 17:25:57]

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

https://codereview.chromium.org/17419011/diff/16001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:582:
requested_work_items_.count(item),
I would consider using a
const bool is_ondemand = requested_work_items_.count(item);

The code is efficient as before, with the added benefit of making explicit what
the value of the expression means. A brief code comment to explain how the
feature is intended to work would be useful as well.

[asargent_no_longer_on_chrome, 2013-06-24 17:34:51]

lgtm

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

https://codereview.chromium.org/17419011/diff/16001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:574: // The
component to add is |crxit| and the |query| string is modified with the
nit: should this be "|item|" instead of "|crxit|" ?

https://codereview.chromium.org/17419011/diff/16001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:575: // required
omaha compatible query. Returns false when the query strings
nit: "when the query strings" -> "when the query string"

https://codereview.chromium.org/17419011/diff/16001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:582:
requested_work_items_.count(item),
On 2013/06/24 17:25:57, Sorin Jianu wrote:
> I would consider using a
> const bool is_ondemand = requested_work_items_.count(item);
> 
> The code is efficient as before, with the added benefit of making explicit
what
> the value of the expression means. A brief code comment to explain how the
> feature is intended to work would be useful as well.

This is a good suggestion. A slightly more compact pattern we sometimes use as
well is to just include a comment inline with the argument you're passing, e.g.


requested_work_items_.count(item), // is_ondemand


Either way would be fine.

[waffles, 2013-06-24 17:39:30]

Thanks!

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

https://codereview.chromium.org/17419011/diff/16001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:574: // The
component to add is |crxit| and the |query| string is modified with the
On 2013/06/24 17:34:51, Antony Sargent wrote:
> nit: should this be "|item|" instead of "|crxit|" ?

Done.

https://codereview.chromium.org/17419011/diff/16001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:575: // required
omaha compatible query. Returns false when the query strings
On 2013/06/24 17:34:51, Antony Sargent wrote:
> nit: "when the query strings" -> "when the query string"

Done.

https://codereview.chromium.org/17419011/diff/16001/chrome/browser/component_...
chrome/browser/component_updater/component_updater_service.cc:582:
requested_work_items_.count(item),
On 2013/06/24 17:34:51, Antony Sargent wrote:
> On 2013/06/24 17:25:57, Sorin Jianu wrote:
> > I would consider using a
> > const bool is_ondemand = requested_work_items_.count(item);
> > 
> > The code is efficient as before, with the added benefit of making explicit
> what
> > the value of the expression means. A brief code comment to explain how the
> > feature is intended to work would be useful as well.
> 
> This is a good suggestion. A slightly more compact pattern we sometimes use as
> well is to just include a comment inline with the argument you're passing,
e.g.
> 
> 
> requested_work_items_.count(item), // is_ondemand
> 
> 
> Either way would be fine. 

Done.

[Sorin Jianu, 2013-06-24 17:46:43]

lgtm

LGTM

[commit-bot: I haz the power, 2013-06-24 18:02:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/waffles@chromium.org/17419011/23003

[commit-bot: I haz the power, 2013-06-24 20:59:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/waffles@chromium.org/17419011/70001

[commit-bot: I haz the power, 2013-06-24 20:59:48]

Change committed as 208256