[rAc - libaddressinput] Chrome download impl

[rAc - libaddressinput] Chrome downloader impl

based on https://codereview.chromium.org/96323002/ by Dan Beam

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

[please use gerrit instead, 2014-01-08 01:36:25]

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
File third_party/libaddressinput/chromium/chrome_downloader_impl.cc (right):

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_downloader_impl.cc:30:
requests_[fetcher] = new Request(url, downloaded.Pass());
nit:

requests_.insert(std::make_pair(fetcher, new  new Request(url,
downloaded.Pass()));

-- is more efficient, because it omits one lookup before inserting the value.

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
File third_party/libaddressinput/chromium/chrome_downloader_impl_unittest.cc
(right):

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_downloader_impl_unittest.cc:19: :
success_(false),
+2 more spaces of indent.

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
File third_party/libaddressinput/libaddressinput.gyp (right):

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
third_party/libaddressinput/libaddressinput.gyp:119:
'chromium/chrome_downloader_impl.h',
No need to re-include the chrome_downloader_impl.{h,cc} files in
libaddressinput_unittests target, because it depends on libaddressinput target.

[Dan Beam, 2014-01-08 03:25:49]

lgtm

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
File third_party/libaddressinput/chromium/chrome_downloader_impl.h (right):

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_downloader_impl.h:8: #include <map>
#include <string>

#include "base/memory/scoped_ptr.h"

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_downloader_impl.h:46: std::map<const
net::URLFetcher*, Request*> requests_;
I'm surprised we don't have an all-purpose ScopedMap for something like this...

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
File third_party/libaddressinput/chromium/chrome_downloader_impl_unittest.cc
(right):

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_downloader_impl_unittest.cc:10:
#include "testing/gtest/include/gtest/gtest.h"
#include
"third_party/libaddressinput/chromium/cpp/include/libaddressinput/callback.h"

for BuildCallback()

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_downloader_impl_unittest.cc:12:
namespace {
nit: not sure we're supposed to anonymously namespace tests

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
File
third_party/libaddressinput/chromium/cpp/include/libaddressinput/downloader.h
(left):

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/downloader.h:47:
scoped_ptr<Callback> downloaded) const = 0;
w00t

[Evan Stade, 2014-01-09 00:05:11]

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
File third_party/libaddressinput/chromium/chrome_downloader_impl.cc (right):

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_downloader_impl.cc:30:
requests_[fetcher] = new Request(url, downloaded.Pass());
On 2014/01/08 01:36:26, Rouslan Solomakhin wrote:
> nit:
> 
> requests_.insert(std::make_pair(fetcher, new  new Request(url,
> downloaded.Pass()));
> 
> -- is more efficient, because it omits one lookup before inserting the value.

Do you have documentation for that assertion? insert fails if the key already
exists. How can it know whether to fail without doing a lookup?

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
File third_party/libaddressinput/chromium/chrome_downloader_impl.h (right):

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_downloader_impl.h:8: #include <map>
On 2014/01/08 03:25:49, Dan Beam wrote:
> #include <string>
> 
> #include "base/memory/scoped_ptr.h"

Done.

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_downloader_impl.h:46: std::map<const
net::URLFetcher*, Request*> requests_;
On 2014/01/08 03:25:49, Dan Beam wrote:
> I'm surprised we don't have an all-purpose ScopedMap for something like
this...

good thing you were sitting down when you reviewed this.

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
File third_party/libaddressinput/chromium/chrome_downloader_impl_unittest.cc
(right):

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_downloader_impl_unittest.cc:10:
#include "testing/gtest/include/gtest/gtest.h"
On 2014/01/08 03:25:49, Dan Beam wrote:
> #include
> "third_party/libaddressinput/chromium/cpp/include/libaddressinput/callback.h"
> 
> for BuildCallback()

Done.

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_downloader_impl_unittest.cc:12:
namespace {
On 2014/01/08 03:25:49, Dan Beam wrote:
> nit: not sure we're supposed to anonymously namespace tests

Done.

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_downloader_impl_unittest.cc:19: :
success_(false),
On 2014/01/08 01:36:26, Rouslan Solomakhin wrote:
> +2 more spaces of indent.

Done.

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
File third_party/libaddressinput/libaddressinput.gyp (right):

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
third_party/libaddressinput/libaddressinput.gyp:119:
'chromium/chrome_downloader_impl.h',
On 2014/01/08 01:36:26, Rouslan Solomakhin wrote:
> No need to re-include the chrome_downloader_impl.{h,cc} files in
> libaddressinput_unittests target, because it depends on libaddressinput
target.

Done.

[please use gerrit instead, 2014-01-09 00:21:13]

lgtm

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
File third_party/libaddressinput/chromium/chrome_downloader_impl.cc (right):

https://codereview.chromium.org/127223003/diff/30001/third_party/libaddressin...
third_party/libaddressinput/chromium/chrome_downloader_impl.cc:30:
requests_[fetcher] = new Request(url, downloaded.Pass());
On 2014/01/09 00:05:12, Evan Stade wrote:
> On 2014/01/08 01:36:26, Rouslan Solomakhin wrote:
> > nit:
> > 
> > requests_.insert(std::make_pair(fetcher, new  new Request(url,
> > downloaded.Pass()));
> > 
> > -- is more efficient, because it omits one lookup before inserting the
value.
> 
> Do you have documentation for that assertion? insert fails if the key already
> exists. How can it know whether to fail without doing a lookup?

Couldn't find any docs. Carry on.

[commit-bot: I haz the power, 2014-01-09 00:25:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/127223003/220002

[commit-bot: I haz the power, 2014-01-09 10:27:16]

Failed to apply patch for
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h
  Hunk #1 FAILED at 85.
  1 out of 1 hunk FAILED -- saving rejects to file
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h.rej

Patch:      
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h
Index:
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h
diff --git
a/third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h
b/third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h
index
b7ceb7f960dd7626d252413781badb4de14250d7..41fc5f562411b93d997e985c6a85c0384ac671d3
100644
---
a/third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h
+++
b/third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h
@@ -85,7 +85,7 @@ class AddressValidator {
   };
 
   // Does not take ownership of |load_rules_delegate|, which can be NULL.
-  AddressValidator(scoped_ptr<const Downloader> downloader,
+  AddressValidator(scoped_ptr<Downloader> downloader,
                    scoped_ptr<Storage> storage,
                    LoadRulesDelegate* load_rules_delegate);
   ~AddressValidator();

[commit-bot: I haz the power, 2014-01-09 19:54:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/127223003/540001

[commit-bot: I haz the power, 2014-01-09 22:39:06]

Change committed as 244003