Add AttachmentDownloader interface, change signature of AttachmentStore::Read

Add AttachmentDownloader interface, change signature of AttachmentStore::Read

- AttachmentDownloader interface is similar to AttachmentUploader interface.
  No implementation yet.
- AttachmentStore::Read guarantee should be stronger. It should attempt
  to read all attachments and return list of attachment ids that can't
  be loaded locally. Those need to be downloaded from server.

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

[maniscalco, 2014-05-22 20:28:09]

lgtm with a few nits

https://codereview.chromium.org/293143002/diff/1/sync/api/attachments/attachm...
File sync/api/attachments/attachment_downloader.h (right):

https://codereview.chromium.org/293143002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_downloader.h:33: // |callback| will receive an
DownloadResult code and an Attachment object. If
nit: "an DownloadResult" -> "a DownloadResult"

https://codereview.chromium.org/293143002/diff/1/sync/internal_api/attachment...
File sync/internal_api/attachments/fake_attachment_store_unittest.cc (right):

https://codereview.chromium.org/293143002/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/fake_attachment_store_unittest.cc:63:
attachments.reset();
This would be a natural place to .clear() failed_attachment_ids.

https://codereview.chromium.org/293143002/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/fake_attachment_store_unittest.cc:112:
EXPECT_EQ(attachments->size(), 1U);
Good place to EXPECT failed_attachment_ids is empty.

https://codereview.chromium.org/293143002/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/fake_attachment_store_unittest.cc:136:
EXPECT_EQ(attachments->size(), 2U);
Good place to EXPECT failed_attachment_ids is empty.

https://codereview.chromium.org/293143002/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/fake_attachment_store_unittest.cc:196:
EXPECT_EQ(attachments->size(), 0U);
Good place to EXPECT failed_attachment_ids to contain the ids from "ids".

https://codereview.chromium.org/293143002/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/fake_attachment_store_unittest.cc:212:
EXPECT_EQ(attachments->size(), 0U);
Good place to EXPECT failed_attachment_ids to contain the ids from "ids".

https://codereview.chromium.org/293143002/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/fake_attachment_store_unittest.cc:236:
EXPECT_EQ(attachments->size(), 0U);
Good place to EXPECT failed_attachment_ids to contain the ids from "ids".

[pavely, 2014-05-22 21:13:32]

https://codereview.chromium.org/293143002/diff/1/sync/api/attachments/attachm...
File sync/api/attachments/attachment_downloader.h (right):

https://codereview.chromium.org/293143002/diff/1/sync/api/attachments/attachm...
sync/api/attachments/attachment_downloader.h:33: // |callback| will receive an
DownloadResult code and an Attachment object. If
On 2014/05/22 20:28:09, maniscalco wrote:
> nit: "an DownloadResult" -> "a DownloadResult"

Done.

https://codereview.chromium.org/293143002/diff/1/sync/internal_api/attachment...
File sync/internal_api/attachments/fake_attachment_store_unittest.cc (right):

https://codereview.chromium.org/293143002/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/fake_attachment_store_unittest.cc:63:
attachments.reset();
On 2014/05/22 20:28:09, maniscalco wrote:
> This would be a natural place to .clear() failed_attachment_ids.

Done.

https://codereview.chromium.org/293143002/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/fake_attachment_store_unittest.cc:112:
EXPECT_EQ(attachments->size(), 1U);
On 2014/05/22 20:28:09, maniscalco wrote:
> Good place to EXPECT failed_attachment_ids is empty.

Done.

https://codereview.chromium.org/293143002/diff/1/sync/internal_api/attachment...
sync/internal_api/attachments/fake_attachment_store_unittest.cc:136:
EXPECT_EQ(attachments->size(), 2U);
On 2014/05/22 20:28:09, maniscalco wrote:
> Good place to EXPECT failed_attachment_ids is empty.

Done.

[pavely, 2014-05-22 21:13:37]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2014-05-22 21:14:04]

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

[commit-bot: I haz the power, 2014-05-23 07:19:48]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-23 09:15:55]

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

[commit-bot: I haz the power, 2014-05-23 09:15:55]

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

[pavely, 2014-05-23 17:24:14]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2014-05-23 17:24:33]

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

[commit-bot: I haz the power, 2014-05-23 18:07:36]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-23 20:20:37]

Change committed as 272585