Instantiate AttachmentDownloader and use it in AttachmentServiceImpl

Instantiate AttachmentDownloader and use it in AttachmentServiceImpl

GetOrDownloadAttachments creates state object that tracks set of attachments
for which requests are still pending. Once AttachmentStore::Read finishes 
AttachmentServiceImpl calls AttachmentDownloader to download attachments that 
were not found locally. 

Once all calls to Downloader finish consumer callback is called with results.

I used AttachmentIdSet in this code but didn't update AttachmentIdList to 
AttachmentIdSet in other places. I'd rather do it in separate change.

R=maniscalco@chromium.org
BUG=376073
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274164

[maniscalco, 2014-05-29 22:35:54]

Very nice!  Here's my feedback.  It's mostly nits.

https://codereview.chromium.org/307783002/diff/1/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_components_factory_impl.cc (right):

https://codereview.chromium.org/307783002/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_components_factory_impl.cc:574: //
AttachmentUpload instead of creating a new one per AttachmentService (bug
Can you update this comment to say we should do the same with
AttachmentDownloader?  I already updated the crbug.

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
File sync/api/attachments/attachment_service_impl.cc (right):

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl.cc:23: GetOrDownloadState(const
AttachmentIdList& attachment_ids,
Can you document the parameters?

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl.cc:25: virtual
~GetOrDownloadState();
Probably should make the dtor private.  Hmm, I thought we'd have received a
compiler warning/error about this... maybe that's only with
RefCountedThreadSafe?

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl.cc:28: void
AddAttachment(Attachment attachment);
attachment can be made const ref: const Attachment&.

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl.cc:42: // Requests for these
attachments are still in porgress.
s/porgress/progress/

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl.cc:43: AttachmentIdSet
incomplete_attachments_;
Consider renaming to in_progress_attachments_ for clarity.  I don't want anyone
to think "incomplete attachment" means "part of an attachment".

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl.cc:58:
std::inserter(incomplete_attachments_, incomplete_attachments_.end()));
Should this be std::back_inserter instead?  I can't remember if std::inserter
will end up inserting if AttachmentIdList is a vector.  Either way, using
back_inserter would make it clear what's going on.

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl.cc:68:
DCHECK(retrieved_attachments_->find(attachment.GetId()) ==
FYI, there is a DCHECK_EQ macro.  It may or may not work here.

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
File sync/api/attachments/attachment_service_impl_unittest.cc (right):

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl_unittest.cc:38: // Respond to Read
request. Attachments found in loca_attachments should be
s/loca_attachments/local_attachments/

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl_unittest.cc:59:
callback.Run(SUCCESS, attachments.Pass(), unavailable_attachments.Pass());
Do you need to invoke the callback here or could you PostTask instead?  If
possible, I suggest using PostTask because it makes it easier to see that there
are no re-entry bugs.

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl_unittest.cc:59:
callback.Run(SUCCESS, attachments.Pass(), unavailable_attachments.Pass());
Should the result code depend on unavailable_attachments.empty()?  If empty(),
it's UNSPECIFIED_ERROR.  If non-empty(), it's SUCCESS.  Right?

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl_unittest.cc:82: void
RespondToDownload(const AttachmentId& id, bool success) {
Instead of passing a bool, can you pass a DownloadResult?  That way, it's more
clear at the call site:

ResponseToDownload(id, false);

vs.

ResponseToDownload(id, DOWNLOAD_UNSPECIFIED_ERROR);

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl_unittest.cc:91:
download_requests[id].Run(result, attachment.Pass());
Similar to an earlier comment, can you use PostTask here instead of invoking Run
directly?

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl_unittest.cc:169: 
Consider adding a test for GetOrDownload with an empty id list.

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl_unittest.cc:181: EXPECT_EQ(1u,
last_download_attachments()->size());
s/1u/1U/

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl_unittest.cc:185: 
What happens if GetOrDownload is called with the same attachment listed multiple
times?  Will that DCHECK?  I guess it won't be an issue once we convert from
AttachmentIdList to AttachmentIdSet.

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl_unittest.cc:203: EXPECT_EQ(2u,
downloader()->download_requests.size());
s/2u/2U/

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl_unittest.cc:217: EXPECT_EQ(2u,
last_download_attachments()->size());
s/2u/2U/

[pavely, 2014-05-30 00:50:07]

PTAL

https://codereview.chromium.org/307783002/diff/1/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_components_factory_impl.cc (right):

https://codereview.chromium.org/307783002/diff/1/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_components_factory_impl.cc:574: //
AttachmentUpload instead of creating a new one per AttachmentService (bug
On 2014/05/29 22:35:54, maniscalco wrote:
> Can you update this comment to say we should do the same with
> AttachmentDownloader?  I already updated the crbug.

Done.

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
File sync/api/attachments/attachment_service_impl.cc (right):

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl.cc:23: GetOrDownloadState(const
AttachmentIdList& attachment_ids,
On 2014/05/29 22:35:54, maniscalco wrote:
> Can you document the parameters?

Done.

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl.cc:25: virtual
~GetOrDownloadState();
On 2014/05/29 22:35:54, maniscalco wrote:
> Probably should make the dtor private.  Hmm, I thought we'd have received a
> compiler warning/error about this... maybe that's only with
> RefCountedThreadSafe?

Done.

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl.cc:28: void
AddAttachment(Attachment attachment);
On 2014/05/29 22:35:54, maniscalco wrote:
> attachment can be made const ref: const Attachment&.

Done.

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl.cc:42: // Requests for these
attachments are still in porgress.
On 2014/05/29 22:35:54, maniscalco wrote:
> s/porgress/progress/

Done.

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl.cc:43: AttachmentIdSet
incomplete_attachments_;
in_progress_attachments_ is better.
I first renamed variables and then wrote comments. If I did it the other way I
would have come up with the same name.

Done.
On 2014/05/29 22:35:54, maniscalco wrote:
> Consider renaming to in_progress_attachments_ for clarity.  I don't want
anyone
> to think "incomplete attachment" means "part of an attachment".

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl.cc:58:
std::inserter(incomplete_attachments_, incomplete_attachments_.end()));
back_inserter needs push_back(). inserter needs insert() with hint.

inserter will work on both set and vector while back_inserter only works on
vector.
On 2014/05/29 22:35:54, maniscalco wrote:
> Should this be std::back_inserter instead?  I can't remember if std::inserter
> will end up inserting if AttachmentIdList is a vector.  Either way, using
> back_inserter would make it clear what's going on.

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl.cc:68:
DCHECK(retrieved_attachments_->find(attachment.GetId()) ==
The reason for DCHECK_EQ macro is that it will also print values of left and
right side. It helps when you not only know what assumption failed, but also can
learn actual values from log line.
Here both values are meaningless iterators.
On 2014/05/29 22:35:54, maniscalco wrote:
> FYI, there is a DCHECK_EQ macro.  It may or may not work here.

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
File sync/api/attachments/attachment_service_impl_unittest.cc (right):

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl_unittest.cc:38: // Respond to Read
request. Attachments found in loca_attachments should be
On 2014/05/29 22:35:54, maniscalco wrote:
> s/loca_attachments/local_attachments/

Done.

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl_unittest.cc:59:
callback.Run(SUCCESS, attachments.Pass(), unavailable_attachments.Pass());
On 2014/05/29 22:35:54, maniscalco wrote:
> Should the result code depend on unavailable_attachments.empty()?  If empty(),
> it's UNSPECIFIED_ERROR.  If non-empty(), it's SUCCESS.  Right?

Done.

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl_unittest.cc:59:
callback.Run(SUCCESS, attachments.Pass(), unavailable_attachments.Pass());
I thought about it when I was writing code. Decided not to because RespondTo...
is only invoked from test function.
I've changed the code to post, this way it is more bullet proof.
On 2014/05/29 22:35:54, maniscalco wrote:
> Do you need to invoke the callback here or could you PostTask instead?  If
> possible, I suggest using PostTask because it makes it easier to see that
there
> are no re-entry bugs.

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl_unittest.cc:82: void
RespondToDownload(const AttachmentId& id, bool success) {
On 2014/05/29 22:35:54, maniscalco wrote:
> Instead of passing a bool, can you pass a DownloadResult?  That way, it's more
> clear at the call site:
> 
> ResponseToDownload(id, false);
> 
> vs.
> 
> ResponseToDownload(id, DOWNLOAD_UNSPECIFIED_ERROR);
> 

Done.

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl_unittest.cc:91:
download_requests[id].Run(result, attachment.Pass());
On 2014/05/29 22:35:54, maniscalco wrote:
> Similar to an earlier comment, can you use PostTask here instead of invoking
Run
> directly?

Done.

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl_unittest.cc:169: 
On 2014/05/29 22:35:54, maniscalco wrote:
> Consider adding a test for GetOrDownload with an empty id list.

Done.

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl_unittest.cc:181: EXPECT_EQ(1u,
last_download_attachments()->size());
On 2014/05/29 22:35:54, maniscalco wrote:
> s/1u/1U/

Done.

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl_unittest.cc:185: 
It will silently work. 
I think it is better to move to AttachmentIdSet model.

On 2014/05/29 22:35:54, maniscalco wrote:
> What happens if GetOrDownload is called with the same attachment listed
multiple
> times?  Will that DCHECK?  I guess it won't be an issue once we convert from
> AttachmentIdList to AttachmentIdSet.

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl_unittest.cc:203: EXPECT_EQ(2u,
downloader()->download_requests.size());
On 2014/05/29 22:35:54, maniscalco wrote:
> s/2u/2U/

Done.

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl_unittest.cc:217: EXPECT_EQ(2u,
last_download_attachments()->size());
On 2014/05/29 22:35:54, maniscalco wrote:
> s/2u/2U/

Done.

[maniscalco, 2014-05-30 16:08:14]

lgtm

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
File sync/api/attachments/attachment_service_impl.cc (right):

https://codereview.chromium.org/307783002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_service_impl.cc:58:
std::inserter(incomplete_attachments_, incomplete_attachments_.end()));
SGTM, std::inserter seems like the way to go.

On 2014/05/30 00:50:07, pavely wrote:
> back_inserter needs push_back(). inserter needs insert() with hint.
> 
> inserter will work on both set and vector while back_inserter only works on
> vector.
> On 2014/05/29 22:35:54, maniscalco wrote:
> > Should this be std::back_inserter instead?  I can't remember if
std::inserter
> > will end up inserting if AttachmentIdList is a vector.  Either way, using
> > back_inserter would make it clear what's going on.
>

[pavely, 2014-05-30 16:30:11]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2014-05-30 16:32:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pavely@chromium.org/307783002/20001

[commit-bot: I haz the power, 2014-05-31 02:27:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-31 02:37:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-31 02:37:32]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[pavely, 2014-06-01 16:44:37]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2014-06-01 16:44:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pavely@chromium.org/307783002/20002

[commit-bot: I haz the power, 2014-06-02 05:33:17]

Change committed as 274164