Add extensions code to use common updater in components/update_client/

Add extensions code to use common updater in components/update_client/

This is another piece of work towards implementing differential
extensions update and share autoupdate code with the chrome components
system. It adds code to the extensions/browser directory that makes use
of the common autoupdate code via the update_client::UpdateClient class
to send update checks to the server, download a full .crx or
differential update, and hand back an unpacked directory with the
contents of the new extension version.

This CL does not yet add any code in chrome/ that actually uses these
new capabilities; that will be in a forthcoming CL.

BUG=490418
Committed: https://crrev.com/631a99a61cbe3e7e03e372b3d37c6a37fa700392
Cr-Commit-Position: refs/heads/master@{#354370}

[asargent_no_longer_on_chrome, 2015-09-23 23:18:21]

Patchset #1 (id:1) has been deleted

[asargent_no_longer_on_chrome, 2015-09-24 00:09:51]

Patchset #1 (id:20001) has been deleted

[asargent_no_longer_on_chrome, 2015-09-24 00:16:26]

asargent@chromium.org changed reviewers:
+ rockot@chromium.org, sorin@chromium.org

[asargent_no_longer_on_chrome, 2015-09-24 00:16:26]

rockot: please review. The most substantive changes are the following files; you
might want to look at them first:

extensions/browser/updater/update_service.{h,cc}
extensions/browser/updater/update_data_provider.{h,cc}
extensions/browser/updater/update_install_shim.{h,cc}


sorin: can you take also look at the files mentioned above, as well as
extensions/browser/updater/update_client_config.{h,cc} and
chrome/browser/extensions/updater/chrome_update_client_config.{h,cc}  to sanity
check that I'm not doing anything crazy?

[Ken Rockot(use gerrit already), 2015-09-25 16:19:19]

lgtm modulo a few nits

https://codereview.chromium.org/1362043005/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_extensions_browser_client.h (right):

https://codereview.chromium.org/1362043005/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/chrome_extensions_browser_client.h:9: #include
<string>
I would argue that these are unnecessary since the only references to
std::string and std::vector are for method signatures overridden from
ExtensionsBrowserClient, and it should be safe for a derived class to assume the
base has already provided necessary definitions (and extensions_browser_client.h
does in fact include these headers).

On a related note, it looks like <map> actually isn't used at all now, so that
could be deleted too!

https://codereview.chromium.org/1362043005/diff/40001/extensions/browser/upda...
File extensions/browser/updater/update_client_config.cc (right):

https://codereview.chromium.org/1362043005/diff/40001/extensions/browser/upda...
extensions/browser/updater/update_client_config.cc:19: }
nit: missing vertical whitespace between all these definitions

https://codereview.chromium.org/1362043005/diff/40001/extensions/browser/upda...
File extensions/browser/updater/update_install_shim.h (right):

https://codereview.chromium.org/1362043005/diff/40001/extensions/browser/upda...
extensions/browser/updater/update_install_shim.h:22: typedef
base::Callback<void(const std::string& /* extension_id */,
nit: new code should use type aliases instead of typedef

[asargent_no_longer_on_chrome, 2015-09-29 22:57:54]

Patchset #2 (id:60001) has been deleted

[asargent_no_longer_on_chrome, 2015-09-29 22:59:22]

responded to review comments

sorin: ping?

https://codereview.chromium.org/1362043005/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_extensions_browser_client.h (right):

https://codereview.chromium.org/1362043005/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/chrome_extensions_browser_client.h:9: #include
<string>
On 2015/09/25 16:19:19, Ken Rockot wrote:
> I would argue that these are unnecessary since the only references to
> std::string and std::vector are for method signatures overridden from
> ExtensionsBrowserClient, and it should be safe for a derived class to assume
the
> base has already provided necessary definitions (and
extensions_browser_client.h
> does in fact include these headers).
> 
> On a related note, it looks like <map> actually isn't used at all now, so that
> could be deleted too!

Removed <map>. As for the <string> and <vector> includes, I just added it to get
a clean run of 'git cl lint', where the IncludeWhatYouUse rule likely doesn't
know about inheritance.

https://codereview.chromium.org/1362043005/diff/40001/extensions/browser/upda...
File extensions/browser/updater/update_client_config.cc (right):

https://codereview.chromium.org/1362043005/diff/40001/extensions/browser/upda...
extensions/browser/updater/update_client_config.cc:19: }
On 2015/09/25 16:19:19, Ken Rockot wrote:
> nit: missing vertical whitespace between all these definitions

Done.

https://codereview.chromium.org/1362043005/diff/40001/extensions/browser/upda...
File extensions/browser/updater/update_install_shim.h (right):

https://codereview.chromium.org/1362043005/diff/40001/extensions/browser/upda...
extensions/browser/updater/update_install_shim.h:22: typedef
base::Callback<void(const std::string& /* extension_id */,
On 2015/09/25 16:19:19, Ken Rockot wrote:
> nit: new code should use type aliases instead of typedef

Thanks for the reminder. Done.

[Sorin Jianu, 2015-10-01 20:52:37]

sorin@chromium.org changed reviewers:
+ waffles@chromium.org

[Sorin Jianu, 2015-10-02 00:05:10]

Thank you Antony. I am impressed by this CL. Most of the comments below are
requests for clarification for my own understanding.

https://codereview.chromium.org/1362043005/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_extensions_browser_client.h (right):

https://codereview.chromium.org/1362043005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/chrome_extensions_browser_client.h:14: #include
"base/memory/scoped_ptr.h"
iwyu, memory/ref_counted.h

https://codereview.chromium.org/1362043005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/chrome_extensions_browser_client.h:114:
scoped_refptr<update_client::UpdateClient> CreateUpdateClient(
Is update_client::UpdateClient declared anywhere?

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/exte...
File extensions/browser/extensions_browser_client.cc (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/exte...
extensions/browser/extensions_browser_client.cc:23: return
scoped_refptr<update_client::UpdateClient>(nullptr);
I assume that the  UpdateClientFactory call is not hooked up at this time. Is
that true?
Also, a configurator instance would be needed at this call site.

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/test...
File extensions/browser/test_extensions_browser_client.cc (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/test...
extensions/browser/test_extensions_browser_client.cc:206: return
update_client_.Pass();
How does this work? I see that update_client_ instance has a setter. Will the
code guarantee that the instance is always set and we are not trying to create
too many instances of the update client by calling this function?

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/test...
File extensions/browser/test_extensions_browser_client.h (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/test...
extensions/browser/test_extensions_browser_client.h:37:
scoped_refptr<update_client::UpdateClient> update_client) {
include ref_counted.h ?

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
File extensions/browser/updater/update_client_config.cc (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
extensions/browser/updater/update_client_config.cc:17: int
UpdateClientConfig::InitialDelay() const {
Are the timing values for these functions correct?

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
File extensions/browser/updater/update_client_config.h (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
extensions/browser/updater/update_client_config.h:32: class UpdateClientConfig :
public update_client::Configurator {
The configurators are a sore point of our design (don't know how to make this
better though).
Would you like to take a look and see if you can reuse configurator_impl.cc th
way IOSConfigurator does?

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
File extensions/browser/updater/update_data_provider.cc (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
extensions/browser/updater/update_data_provider.cc:39: ExtensionRegistry*
registry = ExtensionRegistry::Get(context_);
can this be const?

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
extensions/browser/updater/update_data_provider.cc:45:
update_client::CrxComponent* info = &data->back();
is 44-45 an optimization to avoid a copy?

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
File extensions/browser/updater/update_install_shim.cc (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
extensions/browser/updater/update_install_shim.cc:37: if
(!base::DeleteFile(temp_dir.path(), true) ||
What is the purpose of the base::DeleteFile call?

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
File extensions/browser/updater/update_install_shim.h (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
extensions/browser/updater/update_install_shim.h:29: // doesn't have the notion
of extension ids, we use instances of this class to
Let's discuss more about this. What is update client missing specifically?

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
File extensions/browser/updater/update_service.h (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
extensions/browser/updater/update_service.h:32: // replace ExtensionUpdater in
Chrome.
I understand this is the new extension updater? This code is not in production
yet. Is my understanding correct?

[asargent_no_longer_on_chrome, 2015-10-06 21:22:01]

Patchset #3 (id:100001) has been deleted

[asargent_no_longer_on_chrome, 2015-10-06 21:31:18]

sorin: please take another look

https://codereview.chromium.org/1362043005/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_extensions_browser_client.h (right):

https://codereview.chromium.org/1362043005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/chrome_extensions_browser_client.h:14: #include
"base/memory/scoped_ptr.h"
On 2015/10/02 00:05:10, Sorin Jianu wrote:
> iwyu, memory/ref_counted.h

Done.

https://codereview.chromium.org/1362043005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/chrome_extensions_browser_client.h:114:
scoped_refptr<update_client::UpdateClient> CreateUpdateClient(
On 2015/10/02 00:05:10, Sorin Jianu wrote:
> Is update_client::UpdateClient declared anywhere?

It's forward-declared in extensions/browser/extensions_browser_client.h, which
is the abstract class that this one is a concrete implementation of.

So I didn't forward declare again in this file since this is an override method.

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/exte...
File extensions/browser/extensions_browser_client.cc (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/exte...
extensions/browser/extensions_browser_client.cc:23: return
scoped_refptr<update_client::UpdateClient>(nullptr);
On 2015/10/02 00:05:10, Sorin Jianu wrote:
> I assume that the  UpdateClientFactory call is not hooked up at this time. Is
> that true?
> Also, a configurator instance would be needed at this call site.

This is just a default implementation so that some other ExtensionsBrowserClient
subclasses (like app_shell and some testing ones) don't have to have the same
boilerplate. 

There is an override of this method in ChromeExtensionsBrowserClient
(chrome/browser/extensions/chrome_extensions_browser_client.cc) in this CL which
actually does create an UpdateClient using UpdateClientFactory

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/test...
File extensions/browser/test_extensions_browser_client.cc (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/test...
extensions/browser/test_extensions_browser_client.cc:206: return
update_client_.Pass();
On 2015/10/02 00:05:10, Sorin Jianu wrote:
> How does this work? I see that update_client_ instance has a setter. Will the
> code guarantee that the instance is always set and we are not trying to create
> too many instances of the update client by calling this function?

This is very similar to using .Pass with something like a scoped_ptr - it passes
off ownership from one thing to another. In this case, the caller gets returned
a scoped_refptr pointing to the same object that update_client_ did, and
update_client_ gets set to nullptr. 

(If you want to see the gory details of how .Pass works, it comes from the usage
of the TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03 macro on the scoped_refptr class;
that macro is defined in move.h)

So nothing is getting created here - this is just test code that relies on the
fact that some other test code previously created an UpdateClient that they want
returned by the next CreateUpdateClient call.

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/test...
File extensions/browser/test_extensions_browser_client.h (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/test...
extensions/browser/test_extensions_browser_client.h:37:
scoped_refptr<update_client::UpdateClient> update_client) {
On 2015/10/02 00:05:10, Sorin Jianu wrote:
> include ref_counted.h ?

Done.

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
File extensions/browser/updater/update_client_config.cc (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
extensions/browser/updater/update_client_config.cc:17: int
UpdateClientConfig::InitialDelay() const {
On 2015/10/02 00:05:10, Sorin Jianu wrote:
> Are the timing values for these functions correct?

Maybe? In my manual tests so far they seem to have worked ok, but I haven't
looked at the code very closely to understand exactly what each one does. 

My near term plan is to only use the UpdateClient in a sort of "on demand mode",
ie only when the ExtensionUpdater has determined it wants to do a check. So I
didn't want any sort of delays introduced when calling the UpdateClient::Update
method.

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
File extensions/browser/updater/update_client_config.h (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
extensions/browser/updater/update_client_config.h:32: class UpdateClientConfig :
public update_client::Configurator {
On 2015/10/02 00:05:10, Sorin Jianu wrote:
> The configurators are a sore point of our design (don't know how to make this
> better though).
> Would you like to take a look and see if you can reuse configurator_impl.cc th
> way IOSConfigurator does?

Oh, I've been meaning to touch base with you guys about this - I decided not to
use configurator_impl.cc because of the CanUseAltUrlSource() method that lets us
fallback to http. In conversations I had with the security team they wanted to
make sure that it was never possible to fallback to downloading the differential
update patches over http, and only ever use https. 

The right solution might be to get rid of the fallback entirely for all use
cases.

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
File extensions/browser/updater/update_data_provider.cc (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
extensions/browser/updater/update_data_provider.cc:39: ExtensionRegistry*
registry = ExtensionRegistry::Get(context_);
On 2015/10/02 00:05:10, Sorin Jianu wrote:
> can this be const?

Done.

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
extensions/browser/updater/update_data_provider.cc:45:
update_client::CrxComponent* info = &data->back();
On 2015/10/02 00:05:10, Sorin Jianu wrote:
> is 44-45 an optimization to avoid a copy?

Yes.

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
File extensions/browser/updater/update_install_shim.cc (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
extensions/browser/updater/update_install_shim.cc:37: if
(!base::DeleteFile(temp_dir.path(), true) ||
On 2015/10/02 00:05:10, Sorin Jianu wrote:
> What is the purpose of the base::DeleteFile call?

This is for safety, since the base::Move documentation does not guarantee that
it's safe to rename on top of a path that already exists.

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
File extensions/browser/updater/update_install_shim.h (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
extensions/browser/updater/update_install_shim.h:29: // doesn't have the notion
of extension ids, we use instances of this class to
On 2015/10/02 00:05:10, Sorin Jianu wrote:
> Let's discuss more about this. What is update client missing specifically?

The CrxInstaller::Install and CrxInstaller::GetInstalledFile methods don't get
passed the id of the extension, so I need to cache that so that I know which
extension is being referred to. In addition, for the GetInstalledFile method,
I'm caching the root directory of the current version of the extension at the
time the update check was started; to do away with that the GetInstalledFile
method would probably need to be passed the version. 

Also, one other minor point is that the assumption seems to be that subclasses
implementing the Install() method can complete its work and report
success/failure synchronously, but for extensions that isn't the case (we have
to hand off a bunch of work to the sandboxed utility process, etc.). I'm not
sure this matters too much, but if the UpdateClient really needs to know if the
install actually succeeded, we'd need to change to having a success/failure
callback that can be called later.

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
File extensions/browser/updater/update_service.h (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
extensions/browser/updater/update_service.h:32: // replace ExtensionUpdater in
Chrome.
On 2015/10/02 00:05:10, Sorin Jianu wrote:
> I understand this is the new extension updater? This code is not in production
> yet. Is my understanding correct?

Right, in subsequent CL's I'll actually be using the StartUpdateCheck method in
this class for real extension update checks, but this CL doesn't do that yet.

[Sorin Jianu, 2015-10-08 22:57:55]

Very nice, thank you Antony.

https://codereview.chromium.org/1362043005/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_extensions_browser_client.h (right):

https://codereview.chromium.org/1362043005/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/chrome_extensions_browser_client.h:114:
scoped_refptr<update_client::UpdateClient> CreateUpdateClient(
On 2015/10/06 21:31:17, Antony Sargent wrote:
> On 2015/10/02 00:05:10, Sorin Jianu wrote:
> > Is update_client::UpdateClient declared anywhere?
> 
> It's forward-declared in extensions/browser/extensions_browser_client.h, which
> is the abstract class that this one is a concrete implementation of.
> 
> So I didn't forward declare again in this file since this is an override
method.
> 

Acknowledged.

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/exte...
File extensions/browser/extensions_browser_client.cc (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/exte...
extensions/browser/extensions_browser_client.cc:23: return
scoped_refptr<update_client::UpdateClient>(nullptr);
On 2015/10/06 21:31:17, Antony Sargent wrote:
> On 2015/10/02 00:05:10, Sorin Jianu wrote:
> > I assume that the  UpdateClientFactory call is not hooked up at this time.
Is
> > that true?
> > Also, a configurator instance would be needed at this call site.
> 
> This is just a default implementation so that some other
ExtensionsBrowserClient
> subclasses (like app_shell and some testing ones) don't have to have the same
> boilerplate. 
> 
> There is an override of this method in ChromeExtensionsBrowserClient
> (chrome/browser/extensions/chrome_extensions_browser_client.cc) in this CL
which
> actually does create an UpdateClient using UpdateClientFactory 
> 

I see. In the general case is not desirable to have this kind of implementation
but I assume that you've considered using composition instead of inheritance,
and this implementation is better.

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/test...
File extensions/browser/test_extensions_browser_client.cc (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/test...
extensions/browser/test_extensions_browser_client.cc:206: return
update_client_.Pass();
On 2015/10/06 21:31:17, Antony Sargent wrote:
> On 2015/10/02 00:05:10, Sorin Jianu wrote:
> > How does this work? I see that update_client_ instance has a setter. Will
the
> > code guarantee that the instance is always set and we are not trying to
create
> > too many instances of the update client by calling this function?
> 
> This is very similar to using .Pass with something like a scoped_ptr - it
passes
> off ownership from one thing to another. In this case, the caller gets
returned
> a scoped_refptr pointing to the same object that update_client_ did, and
> update_client_ gets set to nullptr. 
> 
> (If you want to see the gory details of how .Pass works, it comes from the
usage
> of the TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03 macro on the scoped_refptr class;
> that macro is defined in move.h)
> 
> So nothing is getting created here - this is just test code that relies on the
> fact that some other test code previously created an UpdateClient that they
want
> returned by the next CreateUpdateClient call. 
> 

Correct. I am concerned that successive calls for
TestExtensionsBrowserClient::CreateUpdateClient don't create new instances of
UpdateClient. In other words, we see here a factory function named
CreateUpdateClient. We expect this function to return a new instance of the
object every time is it called.

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
File extensions/browser/updater/update_client_config.cc (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
extensions/browser/updater/update_client_config.cc:17: int
UpdateClientConfig::InitialDelay() const {
On 2015/10/06 21:31:17, Antony Sargent wrote:
> On 2015/10/02 00:05:10, Sorin Jianu wrote:
> > Are the timing values for these functions correct?
> 
> Maybe? In my manual tests so far they seem to have worked ok, but I haven't
> looked at the code very closely to understand exactly what each one does. 
> 
> My near term plan is to only use the UpdateClient in a sort of "on demand
mode",
> ie only when the ExtensionUpdater has determined it wants to do a check. So I
> didn't want any sort of delays introduced when calling the
UpdateClient::Update
> method. 

If this is the case, then InitialDelay and NextCheckDelay are not relevant to
you, since they determine when and how often the component updater runs.

Step delay is the interval between different discrete operations that comprise
an update check: such as downloads and installs. Not relevant for our case.

OnDemandDelay is used for a cooldown period for the callers of OnDemandUpdate in
the component updater. Not relevant.

UpdateDelay is the delay between applying successive updates for components.
Could be relevant to this case, if the intention is to update more than one CRX
at a time.

My advice would be to reuse ConfiguratorImpl if possible instead of redefining
these values.

The configurator code is in general confusing, since:
* it is used at runtime to provide parameters to the code
* it is also use to inject dependencies
* it is used across several software layers: update_client and component updater
for example

I'd like to refactor this but I don't have a clear idea how and I want to avoid
proliferation of too many Configurator classes.

That said, it would be desirable to reuse the ConfiguratorImpl by composition,
if it is possible at all.

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
File extensions/browser/updater/update_client_config.h (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
extensions/browser/updater/update_client_config.h:32: class UpdateClientConfig :
public update_client::Configurator {
On 2015/10/06 21:31:17, Antony Sargent wrote:
> On 2015/10/02 00:05:10, Sorin Jianu wrote:
> > The configurators are a sore point of our design (don't know how to make
this
> > better though).
> > Would you like to take a look and see if you can reuse configurator_impl.cc
th
> > way IOSConfigurator does?
> 
> Oh, I've been meaning to touch base with you guys about this - I decided not
to
> use configurator_impl.cc because of the CanUseAltUrlSource() method that lets
us
> fallback to http. In conversations I had with the security team they wanted to
> make sure that it was never possible to fallback to downloading the
differential
> update patches over http, and only ever use https. 
> 
> The right solution might be to get rid of the fallback entirely for all use
> cases.
> 
I think we can extend the design so this aspect is customizable by the client
reusing this code.
Right now the behavior is runtime-dependent on
base::win::MaybeHasSHA256Support(); but we can allow an override so other
implementations will always return 'false' if the fallback is not needed.

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
File extensions/browser/updater/update_install_shim.cc (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
extensions/browser/updater/update_install_shim.cc:37: if
(!base::DeleteFile(temp_dir.path(), true) ||
On 2015/10/06 21:31:17, Antony Sargent wrote:
> On 2015/10/02 00:05:10, Sorin Jianu wrote:
> > What is the purpose of the base::DeleteFile call?
> 
> This is for safety, since the base::Move documentation does not guarantee that
> it's safe to rename on top of a path that already exists.

Acknowledged.

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
File extensions/browser/updater/update_install_shim.h (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
extensions/browser/updater/update_install_shim.h:29: // doesn't have the notion
of extension ids, we use instances of this class to
On 2015/10/06 21:31:17, Antony Sargent wrote:
> On 2015/10/02 00:05:10, Sorin Jianu wrote:
> > Let's discuss more about this. What is update client missing specifically?
> 
> The CrxInstaller::Install and CrxInstaller::GetInstalledFile methods don't get
> passed the id of the extension, so I need to cache that so that I know which
> extension is being referred to. In addition, for the GetInstalledFile method,
> I'm caching the root directory of the current version of the extension at the
> time the update check was started; to do away with that the GetInstalledFile
> method would probably need to be passed the version. 
> 
> Also, one other minor point is that the assumption seems to be that subclasses
> implementing the Install() method can complete its work and report
> success/failure synchronously, but for extensions that isn't the case (we have
> to hand off a bunch of work to the sandboxed utility process, etc.). I'm not
> sure this matters too much, but if the UpdateClient really needs to know if
the
> install actually succeeded, we'd need to change to having a success/failure
> callback that can be called later. 
> 

Correct, we would have to change the interface to the installers so they are
non-blocking.

[asargent_no_longer_on_chrome, 2015-10-14 17:04:31]

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/exte...
File extensions/browser/extensions_browser_client.cc (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/exte...
extensions/browser/extensions_browser_client.cc:23: return
scoped_refptr<update_client::UpdateClient>(nullptr);
On 2015/10/08 22:57:54, Sorin Jianu wrote:
> On 2015/10/06 21:31:17, Antony Sargent wrote:
> > On 2015/10/02 00:05:10, Sorin Jianu wrote:
> > > I assume that the  UpdateClientFactory call is not hooked up at this time.
> Is
> > > that true?
> > > Also, a configurator instance would be needed at this call site.
> > 
> > This is just a default implementation so that some other
> ExtensionsBrowserClient
> > subclasses (like app_shell and some testing ones) don't have to have the
same
> > boilerplate. 
> > 
> > There is an override of this method in ChromeExtensionsBrowserClient
> > (chrome/browser/extensions/chrome_extensions_browser_client.cc) in this CL
> which
> > actually does create an UpdateClient using UpdateClientFactory 
> > 
> 
> I see. In the general case is not desirable to have this kind of
implementation
> but I assume that you've considered using composition instead of inheritance,
> and this implementation is better.

I think preferring composition to inheritance is a good general principle, but
most applicable to cases where the inherited class has interesting concrete
functionality in it. In this case the ExtensionsBrowserClient class is almost
entirely pure virtual, so it's pretty much just an abstract interface, and all
interesting concrete details are implemented in the concrete classes that
inherit from it, with the exception of a few uninteresting "do nothing" defaults
like this function and ExtensionsBrowserClient::ReportError below.

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/test...
File extensions/browser/test_extensions_browser_client.cc (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/test...
extensions/browser/test_extensions_browser_client.cc:206: return
update_client_.Pass();
On 2015/10/08 22:57:54, Sorin Jianu wrote:
> On 2015/10/06 21:31:17, Antony Sargent wrote:
> > On 2015/10/02 00:05:10, Sorin Jianu wrote:
> > > How does this work? I see that update_client_ instance has a setter. Will
> the
> > > code guarantee that the instance is always set and we are not trying to
> create
> > > too many instances of the update client by calling this function?
> > 
> > This is very similar to using .Pass with something like a scoped_ptr - it
> passes
> > off ownership from one thing to another. In this case, the caller gets
> returned
> > a scoped_refptr pointing to the same object that update_client_ did, and
> > update_client_ gets set to nullptr. 
> > 
> > (If you want to see the gory details of how .Pass works, it comes from the
> usage
> > of the TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03 macro on the scoped_refptr
class;
> > that macro is defined in move.h)
> > 
> > So nothing is getting created here - this is just test code that relies on
the
> > fact that some other test code previously created an UpdateClient that they
> want
> > returned by the next CreateUpdateClient call. 
> > 
> 
> Correct. I am concerned that successive calls for
> TestExtensionsBrowserClient::CreateUpdateClient don't create new instances of
> UpdateClient. In other words, we see here a factory function named
> CreateUpdateClient. We expect this function to return a new instance of the
> object every time is it called.

Ok, I've rewritten this so that TestExtensionsBrowserClient now takes a callback
to register a factory callback to vend UpdateClient instances.

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
File extensions/browser/updater/update_client_config.cc (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
extensions/browser/updater/update_client_config.cc:17: int
UpdateClientConfig::InitialDelay() const {
On 2015/10/08 22:57:54, Sorin Jianu wrote:
> On 2015/10/06 21:31:17, Antony Sargent wrote:
> > On 2015/10/02 00:05:10, Sorin Jianu wrote:
> > > Are the timing values for these functions correct?
> > 
> > Maybe? In my manual tests so far they seem to have worked ok, but I haven't
> > looked at the code very closely to understand exactly what each one does. 
> > 
> > My near term plan is to only use the UpdateClient in a sort of "on demand
> mode",
> > ie only when the ExtensionUpdater has determined it wants to do a check. So
I
> > didn't want any sort of delays introduced when calling the
> UpdateClient::Update
> > method. 
> 
> If this is the case, then InitialDelay and NextCheckDelay are not relevant to
> you, since they determine when and how often the component updater runs.
> 
> Step delay is the interval between different discrete operations that comprise
> an update check: such as downloads and installs. Not relevant for our case.
> 
> OnDemandDelay is used for a cooldown period for the callers of OnDemandUpdate
in
> the component updater. Not relevant.
> 
> UpdateDelay is the delay between applying successive updates for components.
> Could be relevant to this case, if the intention is to update more than one
CRX
> at a time.
> 
> My advice would be to reuse ConfiguratorImpl if possible instead of redefining
> these values.
> 
> The configurator code is in general confusing, since:
> * it is used at runtime to provide parameters to the code
> * it is also use to inject dependencies
> * it is used across several software layers: update_client and component
updater
> for example
> 
> I'd like to refactor this but I don't have a clear idea how and I want to
avoid
> proliferation of too many Configurator classes.
> 
> That said, it would be desirable to reuse the ConfiguratorImpl by composition,
> if it is possible at all.

Ok, I rewrote things to use a ConfiguratorImpl for most methods. However, it
wasn't clear to me that it was appropriate to include a dependency on
components/component_updater/ here in the generic extensions/ code, since
extensions in general probably should not now about the concept of binary
browser components, so I instead moved most of these methods (which mostly just
forward to ConfiguratorImpl) into the ChromeUpdateClient class. Ideally from my
perspective, ConfiguratorImpl would eventually move out of
components/component_updater/ into components/update_client/.

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
File extensions/browser/updater/update_client_config.h (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
extensions/browser/updater/update_client_config.h:32: class UpdateClientConfig :
public update_client::Configurator {
On 2015/10/08 22:57:54, Sorin Jianu wrote:
> On 2015/10/06 21:31:17, Antony Sargent wrote:
> > On 2015/10/02 00:05:10, Sorin Jianu wrote:
> > > The configurators are a sore point of our design (don't know how to make
> this
> > > better though).
> > > Would you like to take a look and see if you can reuse
configurator_impl.cc
> th
> > > way IOSConfigurator does?
> > 
> > Oh, I've been meaning to touch base with you guys about this - I decided not
> to
> > use configurator_impl.cc because of the CanUseAltUrlSource() method that
lets
> us
> > fallback to http. In conversations I had with the security team they wanted
to
> > make sure that it was never possible to fallback to downloading the
> differential
> > update patches over http, and only ever use https. 
> > 
> > The right solution might be to get rid of the fallback entirely for all use
> > cases.
> > 
> I think we can extend the design so this aspect is customizable by the client
> reusing this code.
> Right now the behavior is runtime-dependent on
> base::win::MaybeHasSHA256Support(); but we can allow an override so other
> implementations will always return 'false' if the fallback is not needed.

Ok, I've added a set_require_secure_transport() method to ConfiguratorImpl, and
am using it on the ConfiguratorImpl instance that I now am using in
ChromeUpdateClientConfig

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
File extensions/browser/updater/update_install_shim.h (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
extensions/browser/updater/update_install_shim.h:29: // doesn't have the notion
of extension ids, we use instances of this class to
On 2015/10/08 22:57:54, Sorin Jianu wrote:
> On 2015/10/06 21:31:17, Antony Sargent wrote:
> > On 2015/10/02 00:05:10, Sorin Jianu wrote:
> > > Let's discuss more about this. What is update client missing specifically?
> > 
> > The CrxInstaller::Install and CrxInstaller::GetInstalledFile methods don't
get
> > passed the id of the extension, so I need to cache that so that I know which
> > extension is being referred to. In addition, for the GetInstalledFile
method,
> > I'm caching the root directory of the current version of the extension at
the
> > time the update check was started; to do away with that the GetInstalledFile
> > method would probably need to be passed the version. 
> > 
> > Also, one other minor point is that the assumption seems to be that
subclasses
> > implementing the Install() method can complete its work and report
> > success/failure synchronously, but for extensions that isn't the case (we
have
> > to hand off a bunch of work to the sandboxed utility process, etc.). I'm not
> > sure this matters too much, but if the UpdateClient really needs to know if
> the
> > install actually succeeded, we'd need to change to having a success/failure
> > callback that can be called later. 
> > 
> 
> Correct, we would have to change the interface to the installers so they are
> non-blocking.

This seems like a bigger change, so in the interest of making incremental
progress and not getting stalled I'd like to move forward with this code as-is
for now if that's ok with you, and we can consider doing that as a separate
step.

[Sorin Jianu, 2015-10-14 20:37:09]

Thank you Antony, there might be a bug, please see the comments below.

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/exte...
File extensions/browser/extensions_browser_client.cc (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/exte...
extensions/browser/extensions_browser_client.cc:23: return
scoped_refptr<update_client::UpdateClient>(nullptr);
On 2015/10/14 17:04:31, Antony Sargent wrote:
> On 2015/10/08 22:57:54, Sorin Jianu wrote:
> > On 2015/10/06 21:31:17, Antony Sargent wrote:
> > > On 2015/10/02 00:05:10, Sorin Jianu wrote:
> > > > I assume that the  UpdateClientFactory call is not hooked up at this
time.
> > Is
> > > > that true?
> > > > Also, a configurator instance would be needed at this call site.
> > > 
> > > This is just a default implementation so that some other
> > ExtensionsBrowserClient
> > > subclasses (like app_shell and some testing ones) don't have to have the
> same
> > > boilerplate. 
> > > 
> > > There is an override of this method in ChromeExtensionsBrowserClient
> > > (chrome/browser/extensions/chrome_extensions_browser_client.cc) in this CL
> > which
> > > actually does create an UpdateClient using UpdateClientFactory 
> > > 
> > 
> > I see. In the general case is not desirable to have this kind of
> implementation
> > but I assume that you've considered using composition instead of
inheritance,
> > and this implementation is better.
> 
> I think preferring composition to inheritance is a good general principle, but
> most applicable to cases where the inherited class has interesting concrete
> functionality in it. In this case the ExtensionsBrowserClient class is almost
> entirely pure virtual, so it's pretty much just an abstract interface, and all
> interesting concrete details are implemented in the concrete classes that
> inherit from it, with the exception of a few uninteresting "do nothing"
defaults
> like this function and ExtensionsBrowserClient::ReportError below. 
> 
> 

Acknowledged.

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/test...
File extensions/browser/test_extensions_browser_client.cc (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/test...
extensions/browser/test_extensions_browser_client.cc:206: return
update_client_.Pass();
On 2015/10/14 17:04:31, Antony Sargent wrote:
> On 2015/10/08 22:57:54, Sorin Jianu wrote:
> > On 2015/10/06 21:31:17, Antony Sargent wrote:
> > > On 2015/10/02 00:05:10, Sorin Jianu wrote:
> > > > How does this work? I see that update_client_ instance has a setter.
Will
> > the
> > > > code guarantee that the instance is always set and we are not trying to
> > create
> > > > too many instances of the update client by calling this function?
> > > 
> > > This is very similar to using .Pass with something like a scoped_ptr - it
> > passes
> > > off ownership from one thing to another. In this case, the caller gets
> > returned
> > > a scoped_refptr pointing to the same object that update_client_ did, and
> > > update_client_ gets set to nullptr. 
> > > 
> > > (If you want to see the gory details of how .Pass works, it comes from the
> > usage
> > > of the TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03 macro on the scoped_refptr
> class;
> > > that macro is defined in move.h)
> > > 
> > > So nothing is getting created here - this is just test code that relies on
> the
> > > fact that some other test code previously created an UpdateClient that
they
> > want
> > > returned by the next CreateUpdateClient call. 
> > > 
> > 
> > Correct. I am concerned that successive calls for
> > TestExtensionsBrowserClient::CreateUpdateClient don't create new instances
of
> > UpdateClient. In other words, we see here a factory function named
> > CreateUpdateClient. We expect this function to return a new instance of the
> > object every time is it called.
> 
> Ok, I've rewritten this so that TestExtensionsBrowserClient now takes a
callback
> to register a factory callback to vend UpdateClient instances. 

Acknowledged.

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
File extensions/browser/updater/update_client_config.cc (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
extensions/browser/updater/update_client_config.cc:17: int
UpdateClientConfig::InitialDelay() const {
On 2015/10/14 17:04:31, Antony Sargent wrote:
> On 2015/10/08 22:57:54, Sorin Jianu wrote:
> > On 2015/10/06 21:31:17, Antony Sargent wrote:
> > > On 2015/10/02 00:05:10, Sorin Jianu wrote:
> > > > Are the timing values for these functions correct?
> > > 
> > > Maybe? In my manual tests so far they seem to have worked ok, but I
haven't
> > > looked at the code very closely to understand exactly what each one does. 
> > > 
> > > My near term plan is to only use the UpdateClient in a sort of "on demand
> > mode",
> > > ie only when the ExtensionUpdater has determined it wants to do a check.
So
> I
> > > didn't want any sort of delays introduced when calling the
> > UpdateClient::Update
> > > method. 
> > 
> > If this is the case, then InitialDelay and NextCheckDelay are not relevant
to
> > you, since they determine when and how often the component updater runs.
> > 
> > Step delay is the interval between different discrete operations that
comprise
> > an update check: such as downloads and installs. Not relevant for our case.
> > 
> > OnDemandDelay is used for a cooldown period for the callers of
OnDemandUpdate
> in
> > the component updater. Not relevant.
> > 
> > UpdateDelay is the delay between applying successive updates for components.
> > Could be relevant to this case, if the intention is to update more than one
> CRX
> > at a time.
> > 
> > My advice would be to reuse ConfiguratorImpl if possible instead of
redefining
> > these values.
> > 
> > The configurator code is in general confusing, since:
> > * it is used at runtime to provide parameters to the code
> > * it is also use to inject dependencies
> > * it is used across several software layers: update_client and component
> updater
> > for example
> > 
> > I'd like to refactor this but I don't have a clear idea how and I want to
> avoid
> > proliferation of too many Configurator classes.
> > 
> > That said, it would be desirable to reuse the ConfiguratorImpl by
composition,
> > if it is possible at all.
> 
> Ok, I rewrote things to use a ConfiguratorImpl for most methods. However, it
> wasn't clear to me that it was appropriate to include a dependency on
> components/component_updater/ here in the generic extensions/ code, since
> extensions in general probably should not now about the concept of binary
> browser components, so I instead moved most of these methods (which mostly
just
> forward to ConfiguratorImpl) into the ChromeUpdateClient class. Ideally from
my
> perspective, ConfiguratorImpl would eventually move out of
> components/component_updater/ into components/update_client/. 
> 
> 

Acknowledged.

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
File extensions/browser/updater/update_client_config.h (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
extensions/browser/updater/update_client_config.h:32: class UpdateClientConfig :
public update_client::Configurator {
On 2015/10/14 17:04:31, Antony Sargent wrote:
> On 2015/10/08 22:57:54, Sorin Jianu wrote:
> > On 2015/10/06 21:31:17, Antony Sargent wrote:
> > > On 2015/10/02 00:05:10, Sorin Jianu wrote:
> > > > The configurators are a sore point of our design (don't know how to make
> > this
> > > > better though).
> > > > Would you like to take a look and see if you can reuse
> configurator_impl.cc
> > th
> > > > way IOSConfigurator does?
> > > 
> > > Oh, I've been meaning to touch base with you guys about this - I decided
not
> > to
> > > use configurator_impl.cc because of the CanUseAltUrlSource() method that
> lets
> > us
> > > fallback to http. In conversations I had with the security team they
wanted
> to
> > > make sure that it was never possible to fallback to downloading the
> > differential
> > > update patches over http, and only ever use https. 
> > > 
> > > The right solution might be to get rid of the fallback entirely for all
use
> > > cases.
> > > 
> > I think we can extend the design so this aspect is customizable by the
client
> > reusing this code.
> > Right now the behavior is runtime-dependent on
> > base::win::MaybeHasSHA256Support(); but we can allow an override so other
> > implementations will always return 'false' if the fallback is not needed.
> 
> Ok, I've added a set_require_secure_transport() method to ConfiguratorImpl,
and
> am using it on the ConfiguratorImpl instance that I now am using in
> ChromeUpdateClientConfig
> 

Acknowledged.

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
File extensions/browser/updater/update_install_shim.h (right):

https://codereview.chromium.org/1362043005/diff/80001/extensions/browser/upda...
extensions/browser/updater/update_install_shim.h:29: // doesn't have the notion
of extension ids, we use instances of this class to
On 2015/10/14 17:04:31, Antony Sargent wrote:
> On 2015/10/08 22:57:54, Sorin Jianu wrote:
> > On 2015/10/06 21:31:17, Antony Sargent wrote:
> > > On 2015/10/02 00:05:10, Sorin Jianu wrote:
> > > > Let's discuss more about this. What is update client missing
specifically?
> > > 
> > > The CrxInstaller::Install and CrxInstaller::GetInstalledFile methods don't
> get
> > > passed the id of the extension, so I need to cache that so that I know
which
> > > extension is being referred to. In addition, for the GetInstalledFile
> method,
> > > I'm caching the root directory of the current version of the extension at
> the
> > > time the update check was started; to do away with that the
GetInstalledFile
> > > method would probably need to be passed the version. 
> > > 
> > > Also, one other minor point is that the assumption seems to be that
> subclasses
> > > implementing the Install() method can complete its work and report
> > > success/failure synchronously, but for extensions that isn't the case (we
> have
> > > to hand off a bunch of work to the sandboxed utility process, etc.). I'm
not
> > > sure this matters too much, but if the UpdateClient really needs to know
if
> > the
> > > install actually succeeded, we'd need to change to having a
success/failure
> > > callback that can be called later. 
> > > 
> > 
> > Correct, we would have to change the interface to the installers so they are
> > non-blocking.
> 
> This seems like a bigger change, so in the interest of making incremental
> progress and not getting stalled I'd like to move forward with this code as-is
> for now if that's ok with you, and we can consider doing that as a separate
> step. 

Indeed, I meant to say, we would do the change from blocking to non-blocking,
which is a monumental change, by how much code it is going to touch.

https://codereview.chromium.org/1362043005/diff/140001/components/component_u...
File components/component_updater/configurator_impl.cc (right):

https://codereview.chromium.org/1362043005/diff/140001/components/component_u...
components/component_updater/configurator_impl.cc:197:
fallback_to_alt_source_url_enabled_ = require_secure_transport;
is the logic correct here?

Let's assume require_secure_transport is |true|, to indicate that we want https
only. That implies that fallback_to_alt_source_url_enabled_ is |true|, and this
will make the code fall back to the unsecure kUpdaterAltUrl.

https://codereview.chromium.org/1362043005/diff/140001/components/component_u...
File components/component_updater/configurator_impl.h (right):

https://codereview.chromium.org/1362043005/diff/140001/components/component_u...
components/component_updater/configurator_impl.h:83: void
set_require_secure_transport(bool require_secure_transport);
Maybe we just need a setter: set_enable_alt_source_url(). Please see the comment
in the implementation section.

https://codereview.chromium.org/1362043005/diff/140001/extensions/browser/tes...
File extensions/browser/test_extensions_browser_client.cc (right):

https://codereview.chromium.org/1362043005/diff/140001/extensions/browser/tes...
extensions/browser/test_extensions_browser_client.cc:211: if
(update_client_factory_.is_null()) {
can use ?:

[asargent_no_longer_on_chrome, 2015-10-15 20:16:03]

https://codereview.chromium.org/1362043005/diff/140001/components/component_u...
File components/component_updater/configurator_impl.cc (right):

https://codereview.chromium.org/1362043005/diff/140001/components/component_u...
components/component_updater/configurator_impl.cc:197:
fallback_to_alt_source_url_enabled_ = require_secure_transport;
On 2015/10/14 20:37:09, Sorin Jianu wrote:
> is the logic correct here?
> 
> Let's assume require_secure_transport is |true|, to indicate that we want
https
> only. That implies that fallback_to_alt_source_url_enabled_ is |true|, and
this
> will make the code fall back to the unsecure kUpdaterAltUrl.

Oops, great catch. Fixed by switching to setter for
fallback_to_alt_source_url_enabled per your suggestion in .h file.

https://codereview.chromium.org/1362043005/diff/140001/components/component_u...
File components/component_updater/configurator_impl.h (right):

https://codereview.chromium.org/1362043005/diff/140001/components/component_u...
components/component_updater/configurator_impl.h:83: void
set_require_secure_transport(bool require_secure_transport);
On 2015/10/14 20:37:09, Sorin Jianu wrote:
> Maybe we just need a setter: set_enable_alt_source_url(). Please see the
comment
> in the implementation section.

Done.

https://codereview.chromium.org/1362043005/diff/140001/extensions/browser/tes...
File extensions/browser/test_extensions_browser_client.cc (right):

https://codereview.chromium.org/1362043005/diff/140001/extensions/browser/tes...
extensions/browser/test_extensions_browser_client.cc:211: if
(update_client_factory_.is_null()) {
On 2015/10/14 20:37:09, Sorin Jianu wrote:
> can use ?:

Done.

[Sorin Jianu, 2015-10-15 20:21:40]

lgtm

Nice, thank you Antony!

[asargent_no_longer_on_chrome, 2015-10-15 20:53:06]

The CQ bit was checked by asargent@chromium.org

[asargent_no_longer_on_chrome, 2015-10-15 20:53:06]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
sorin@chromium.org
Link to the patchset: https://codereview.chromium.org/1362043005/#ps180001
(title: "merged latest origin/master")

[commit-bot: I haz the power, 2015-10-15 20:53:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1362043005/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1362043005/180001

[commit-bot: I haz the power, 2015-10-15 21:51:57]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2015-10-15 21:52:55]

Patchset 6 (id:??) landed as
https://crrev.com/631a99a61cbe3e7e03e372b3d37c6a37fa700392
Cr-Commit-Position: refs/heads/master@{#354370}