Issue 565363002: Implement support for fallback update check urls in the component updater

Issue 565363002: Implement support for fallback update check urls in the component updater (Closed)

Created:
6 years, 3 months ago by Sorin Jianu

Modified:
6 years, 3 months ago

Reviewers:
waffles, erikwright (departed)

CC:
chromium-reviews, cpu_(ooo_6.6-7.5)

Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr

Project:
chromium

Visibility:
Public.

More Reviews

Description

Implement support for fallback update check urls in the component updater This allows for specifying more than one urls for update checks and pings. The urls are then tried in the order they are specified. BUG=413879 Committed: https://crrev.com/395c2ac57fc8f5c2b5175692873a1702538cb84a Cr-Commit-Position: refs/heads/master@{#295146}

Patch Set 1 #

Total comments: 8

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Total comments: 41

Patch Set 5 : #

Patch Set 6 : #

Patch Set 7 : #

Created: 6 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+632 lines, -198 lines)			Patch
M	chrome/browser/component_updater/chrome_component_updater_configurator.cc	View	1	5 chunks	+21 lines, -15 lines	0 comments	Download
M	components/component_updater.gypi	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	components/component_updater/BUILD.gn	View		2 chunks	+3 lines, -0 lines	0 comments	Download
M	components/component_updater/component_patcher.cc	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	components/component_updater/component_updater_configurator.h	View	1 2 3 4	2 chunks	+7 lines, -5 lines	0 comments	Download
M	components/component_updater/component_updater_ping_manager.cc	View	1 2 3 4 5 6	5 chunks	+93 lines, -86 lines	0 comments	Download
M	components/component_updater/component_updater_service.cc	View	1 2 3 4	4 chunks	+11 lines, -7 lines	0 comments	Download
M	components/component_updater/default_component_installer.cc	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
A	components/component_updater/request_sender.h	View	1 2 3 4	1 chunk	+61 lines, -0 lines	0 comments	Download
A	components/component_updater/request_sender.cc	View	1 2 3 4 5 6	1 chunk	+65 lines, -0 lines	0 comments	Download
M	components/component_updater/test/component_patcher_unittest.cc	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	components/component_updater/test/crx_downloader_unittest.cc	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
A	components/component_updater/test/request_sender_unittest.cc	View	1	1 chunk	+210 lines, -0 lines	0 comments	Download
M	components/component_updater/test/test_configurator.h	View	1 2 3 4	4 chunks	+5 lines, -3 lines	0 comments	Download
M	components/component_updater/test/test_configurator.cc	View	1 2 3 4	2 chunks	+15 lines, -6 lines	0 comments	Download
M	components/component_updater/test/update_checker_unittest.cc	View	1 2 3 4 5 6	9 chunks	+29 lines, -21 lines	0 comments	Download
M	components/component_updater/test/url_request_post_interceptor.h	View		4 chunks	+20 lines, -3 lines	0 comments	Download
M	components/component_updater/test/url_request_post_interceptor.cc	View		7 chunks	+32 lines, -11 lines	0 comments	Download
M	components/component_updater/update_checker.h	View	1 2 3 4	3 chunks	+6 lines, -5 lines	0 comments	Download
M	components/component_updater/update_checker.cc	View	1 2 3 4	4 chunks	+46 lines, -35 lines	0 comments	Download
M	components/components_tests.gyp	View	1 2 3 4 5 6	2 chunks	+2 lines, -1 line	0 comments	Download

Messages

Total messages: 16 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

waffles

https://codereview.chromium.org/565363002/diff/1/chrome/browser/component_updater/chrome_component_updater_configurator.cc File chrome/browser/component_updater/chrome_component_updater_configurator.cc (right): https://codereview.chromium.org/565363002/diff/1/chrome/browser/component_updater/chrome_component_updater_configurator.cc#newcode48 chrome/browser/component_updater/chrome_component_updater_configurator.cc:48: "//clients2.google.com/service/update2" Is there any point in having this #define ...

6 years, 3 months ago (2014-09-12 22:38:26 UTC) #2

Sorin Jianu

Thank you, PTAL. https://codereview.chromium.org/565363002/diff/1/chrome/browser/component_updater/chrome_component_updater_configurator.cc File chrome/browser/component_updater/chrome_component_updater_configurator.cc (right): https://codereview.chromium.org/565363002/diff/1/chrome/browser/component_updater/chrome_component_updater_configurator.cc#newcode48 chrome/browser/component_updater/chrome_component_updater_configurator.cc:48: "//clients2.google.com/service/update2" On 2014/09/12 22:38:26, waffles wrote: ...

6 years, 3 months ago (2014-09-12 23:24:00 UTC) #3

Sorin Jianu

Erik, may I please have an owner's approval for components/component_updater.gypi Thank you!

6 years, 3 months ago (2014-09-15 18:08:16 UTC) #6

erikwright (departed)

First round of feedback covering everything outside test/. https://codereview.chromium.org/565363002/diff/60001/components/component_updater/component_updater_configurator.h File components/component_updater/component_updater_configurator.h (right): https://codereview.chromium.org/565363002/diff/60001/components/component_updater/component_updater_configurator.h#newcode57 components/component_updater/component_updater_configurator.h:57: // ...

6 years, 3 months ago (2014-09-15 18:37:12 UTC) #7

erikwright (departed)

https://codereview.chromium.org/565363002/diff/60001/components/component_updater/test/test_configurator.cc File components/component_updater/test/test_configurator.cc (right): https://codereview.chromium.org/565363002/diff/60001/components/component_updater/test/test_configurator.cc#newcode5 components/component_updater/test/test_configurator.cc:5: #include <string> not required (included in the header) https://codereview.chromium.org/565363002/diff/60001/components/component_updater/test/test_configurator.cc#newcode10 ...

6 years, 3 months ago (2014-09-15 18:46:55 UTC) #8

Sorin Jianu

Thank you Erik, PTAL. https://codereview.chromium.org/565363002/diff/60001/components/component_updater/component_updater_configurator.h File components/component_updater/component_updater_configurator.h (right): https://codereview.chromium.org/565363002/diff/60001/components/component_updater/component_updater_configurator.h#newcode57 components/component_updater/component_updater_configurator.h:57: // The urls for the ...

6 years, 3 months ago (2014-09-15 22:17:58 UTC) #9

erikwright (departed)

LGTM with a few nits outstanding. https://codereview.chromium.org/565363002/diff/60001/components/component_updater/component_updater_ping_manager.cc File components/component_updater/component_updater_ping_manager.cc (right): https://codereview.chromium.org/565363002/diff/60001/components/component_updater/component_updater_ping_manager.cc#newcode151 components/component_updater/component_updater_ping_manager.cc:151: void OnRequestSenderComplete(const net::URLFetcher* ...

6 years, 3 months ago (2014-09-16 17:38:11 UTC) #11

LGTM with a few nits outstanding.

https://codereview.chromium.org/565363002/diff/60001/components/component_upd...
File components/component_updater/component_updater_ping_manager.cc (right):

https://codereview.chromium.org/565363002/diff/60001/components/component_upd...
components/component_updater/component_updater_ping_manager.cc:151: void
OnRequestSenderComplete(const net::URLFetcher* source);
On 2014/09/15 22:17:57, Sorin Jianu wrote:
> On 2014/09/15 18:37:11, erikwright wrote:
> > forward-decl required?
> 
> This function is part of the public interface for the RequestSender, whose
> header is included fully.

Sure, but the only case where you are not required to IWYU would be if the type
only appeared in the declaration of an OVERRIDE of a virtual parent class
method.

In this case, indirectly, we know that this must already be forward-decl'd
because it corresponds to a callback type referenced in a class that is fully
defined and used.

In most cases we can rationalize that a type must already be fully defined, but
then we have to decide if, by definition, the usage of the type would disappear
if the rational later becomes invalid. To make things simpler, we don't do this
rationalization and instead use very simple rules.

Please do the forward-decl or include depending on whether this compilation unit
requires a full or partial type.

https://codereview.chromium.org/565363002/diff/60001/components/component_upd...
File components/component_updater/request_sender.cc (right):

https://codereview.chromium.org/565363002/diff/60001/components/component_upd...
components/component_updater/request_sender.cc:55:
base::Bind(&RequestSender::SendInternal, base::Unretained(this)))) {
On 2014/09/15 18:37:11, erikwright wrote:
> include base/bind.h and base/bind_helpers.h

ping.

https://codereview.chromium.org/565363002/diff/60001/components/component_upd...
File components/component_updater/request_sender.h (right):

https://codereview.chromium.org/565363002/diff/60001/components/component_upd...
components/component_updater/request_sender.h:11: #include "base/basictypes.h"
On 2014/09/15 22:17:57, Sorin Jianu wrote:
> On 2014/09/15 18:37:11, erikwright wrote:
> > I believe "base/macros.h" is preferred over "base/basictypes.h"
> 
> Thank you. Fixed here and I will fix the rest of the module in a future CL.

I wouldn't worry about fixing files you aren't already touching.

https://codereview.chromium.org/565363002/diff/60001/components/component_upd...
File components/component_updater/test/update_checker_unittest.cc (right):

https://codereview.chromium.org/565363002/diff/60001/components/component_upd...
components/component_updater/test/update_checker_unittest.cc:63: GURL
original_url_;
On 2014/09/15 22:17:58, Sorin Jianu wrote:
> On 2014/09/15 18:46:55, erikwright wrote:
> > include url/gurl.h
> 
> Done.

Are you sure?

https://codereview.chromium.org/565363002/diff/60001/components/component_upd...
components/component_updater/test/update_checker_unittest.cc:173:
base::Bind(&UpdateCheckerTest::UpdateCheckComplete,
On 2014/09/15 22:17:58, Sorin Jianu wrote:
> On 2014/09/15 18:46:54, erikwright wrote:
> > include base/bind.h and base/bind_helpers.h
> 
> Done.

Are you sure?

Sorin Jianu

Thank you Erik. I feel bad I did not catch these beforehand, I will think ...

6 years, 3 months ago (2014-09-16 19:53:31 UTC) #12

Thank you Erik. I feel bad I did not catch these beforehand, I  will think my
workflow over.

https://codereview.chromium.org/565363002/diff/60001/components/component_upd...
File components/component_updater/component_updater_ping_manager.cc (right):

https://codereview.chromium.org/565363002/diff/60001/components/component_upd...
components/component_updater/component_updater_ping_manager.cc:151: void
OnRequestSenderComplete(const net::URLFetcher* source);
On 2014/09/16 17:38:11, erikwright wrote:
> On 2014/09/15 22:17:57, Sorin Jianu wrote:
> > On 2014/09/15 18:37:11, erikwright wrote:
> > > forward-decl required?
> > 
> > This function is part of the public interface for the RequestSender, whose
> > header is included fully.
> 
> Sure, but the only case where you are not required to IWYU would be if the
type
> only appeared in the declaration of an OVERRIDE of a virtual parent class
> method.
> 
> In this case, indirectly, we know that this must already be forward-decl'd
> because it corresponds to a callback type referenced in a class that is fully
> defined and used.
> 
> In most cases we can rationalize that a type must already be fully defined,
but
> then we have to decide if, by definition, the usage of the type would
disappear
> if the rational later becomes invalid. To make things simpler, we don't do
this
> rationalization and instead use very simple rules.
> 
> Please do the forward-decl or include depending on whether this compilation
unit
> requires a full or partial type.

Thank you. I declared the forward type.

https://codereview.chromium.org/565363002/diff/60001/components/component_upd...
File components/component_updater/request_sender.cc (right):

https://codereview.chromium.org/565363002/diff/60001/components/component_upd...
components/component_updater/request_sender.cc:55:
base::Bind(&RequestSender::SendInternal, base::Unretained(this)))) {
On 2014/09/16 17:38:11, erikwright wrote:
> On 2014/09/15 18:37:11, erikwright wrote:
> > include base/bind.h and base/bind_helpers.h
> 
> ping.

Done.

https://codereview.chromium.org/565363002/diff/60001/components/component_upd...
File components/component_updater/request_sender.h (right):

https://codereview.chromium.org/565363002/diff/60001/components/component_upd...
components/component_updater/request_sender.h:11: #include "base/basictypes.h"
On 2014/09/16 17:38:11, erikwright wrote:
> On 2014/09/15 22:17:57, Sorin Jianu wrote:
> > On 2014/09/15 18:37:11, erikwright wrote:
> > > I believe "base/macros.h" is preferred over "base/basictypes.h"
> > 
> > Thank you. Fixed here and I will fix the rest of the module in a future CL.
> 
> I wouldn't worry about fixing files you aren't already touching.

Acknowledged.

https://codereview.chromium.org/565363002/diff/60001/components/component_upd...
File components/component_updater/test/update_checker_unittest.cc (right):

https://codereview.chromium.org/565363002/diff/60001/components/component_upd...
components/component_updater/test/update_checker_unittest.cc:63: GURL
original_url_;
On 2014/09/16 17:38:11, erikwright wrote:
> On 2014/09/15 22:17:58, Sorin Jianu wrote:
> > On 2014/09/15 18:46:55, erikwright wrote:
> > > include url/gurl.h
> > 
> > Done.
> 
> Are you sure?

Done.

https://codereview.chromium.org/565363002/diff/60001/components/component_upd...
components/component_updater/test/update_checker_unittest.cc:173:
base::Bind(&UpdateCheckerTest::UpdateCheckComplete,
On 2014/09/16 17:38:11, erikwright wrote:
> On 2014/09/15 22:17:58, Sorin Jianu wrote:
> > On 2014/09/15 18:46:54, erikwright wrote:
> > > include base/bind.h and base/bind_helpers.h
> > 
> > Done.
> 
> Are you sure?

Done.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/565363002/120001

6 years, 3 months ago (2014-09-16 19:54:31 UTC) #14

commit-bot: I haz the power

Committed patchset #7 (id:120001) as 8052d5ecd3e86d811bc6ed1483e31151ff4c5f30

6 years, 3 months ago (2014-09-16 21:31:15 UTC) #15

commit-bot: I haz the power

6 years, 3 months ago (2014-09-16 21:32:04 UTC) #16

Message was sent while issue was closed.

Patchset 7 (id:??) landed as
https://crrev.com/395c2ac57fc8f5c2b5175692873a1702538cb84a
Cr-Commit-Position: refs/heads/master@{#295146}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages