Add a minimal AttachmentUploaderImpl.

Add a minimal AttachmentUploaderImpl.

This is a "dumbed down" implementation with several open TODOs.  Future
CLs will add auth support and wire it in to the rest of sync.

Wire protocol is in flux and will change once the server has been
implemented.


BUG=

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

[maniscalco, 2014-05-12 20:29:51]

Hi Pavel, would you please review this change?

[pavely, 2014-05-14 18:38:31]

There are three main issues. Let's address those and then I'll do one thorough
pass for nits.

https://codereview.chromium.org/278263003/diff/20001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_server_url_builder.cc (right):

https://codereview.chromium.org/278263003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_server_url_builder.cc:44: return
GURL(url_prefix_ + unique_id);
Currently unique_id is guid, but it wasn't the case and it might change in the
future. Let's URLEncode it.

https://codereview.chromium.org/278263003/diff/20001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_uploader_impl.cc (right):

https://codereview.chromium.org/278263003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl.cc:152:
state_map_.add(unique_id, upload_state.Pass());
You never remove entries from state_map_. What will happen if attachment
uploaded and later datatype decides to upload the same attachment again?

https://codereview.chromium.org/278263003/diff/20001/sync/internal_api/public...
File sync/internal_api/public/attachments/attachment_server_url_builder.h
(right):

https://codereview.chromium.org/278263003/diff/20001/sync/internal_api/public...
sync/internal_api/public/attachments/attachment_server_url_builder.h:26:
AttachmentServerURLBuilder(const Scheme& scheme,
Do you have a scenario where you'll need to pass all these parameters?
I think likely use of this class is to use one prefix for dev, another for
stable and being able to override it from command line for experiments.
I'd say just pass the whole prefix as string and if in the future we need to do
something more we'll change it.

[maniscalco, 2014-05-14 21:59:03]

Thanks for the feedback.  RFAL!

https://codereview.chromium.org/278263003/diff/20001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_server_url_builder.cc (right):

https://codereview.chromium.org/278263003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_server_url_builder.cc:44: return
GURL(url_prefix_ + unique_id);
On 2014/05/14 18:38:31, pavely wrote:
> Currently unique_id is guid, but it wasn't the case and it might change in the
> future. Let's URLEncode it.

Of course, we'd then need to decode it on the server side.  What do you think
about restricting unique_id to be "url safe" instead?  Seems like that would be
simpler.  Also, we can rely on the GUID format to never change and be "url
safe".

https://codereview.chromium.org/278263003/diff/20001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_uploader_impl.cc (right):

https://codereview.chromium.org/278263003/diff/20001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl.cc:152:
state_map_.add(unique_id, upload_state.Pass());
Good catch.  Entries should be removed immediately after an upload completes. 
Fixed and added a test case.
 
On 2014/05/14 18:38:31, pavely wrote:
> You never remove entries from state_map_. What will happen if attachment
> uploaded and later datatype decides to upload the same attachment again?

https://codereview.chromium.org/278263003/diff/20001/sync/internal_api/public...
File sync/internal_api/public/attachments/attachment_server_url_builder.h
(right):

https://codereview.chromium.org/278263003/diff/20001/sync/internal_api/public...
sync/internal_api/public/attachments/attachment_server_url_builder.h:26:
AttachmentServerURLBuilder(const Scheme& scheme,
On 2014/05/14 18:38:31, pavely wrote:
> Do you have a scenario where you'll need to pass all these parameters?
> I think likely use of this class is to use one prefix for dev, another for
> stable and being able to override it from command line for experiments.
> I'd say just pass the whole prefix as string and if in the future we need to
do
> something more we'll change it.

We could get away with using a string as a URL prefix.  Done.  I've updated the
patch to drop AttachmentServerURLBuilder and instead pass a url_prefix string. 
LMKWYT.

[pavely, 2014-05-15 23:27:28]

https://codereview.chromium.org/278263003/diff/40001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_uploader_impl.cc (right):

https://codereview.chromium.org/278263003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl.cc:135: // Destroy this
object and return immediately.
This is unfortunate gentlemen's agreement. I try to stay away from hidden
"delete this" patterns. 
One alternative is to use DeleteSoon, but that would require some special code
in AttachmentUploaderImpl::dtor for cleaning up requests that haven't finished
yet.

Let's keep it as it is.

https://codereview.chromium.org/278263003/diff/40001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_uploader_impl_unittest.cc (right):

https://codereview.chromium.org/278263003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl_unittest.cc:34: class
AttachmentUploaderImplTest;
Not needed

https://codereview.chromium.org/278263003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl_unittest.cc:84: class
RequestHandler : public base::NonThreadSafe {
Do you really need RequestHandler as separate class? It is essentially just one
function.

https://codereview.chromium.org/278263003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl_unittest.cc:133:
message_loop_.Run();
In some previous change Tim asked me not to use MessageLoop directly in tests
but instead use RunLoop.
The pattern is this:

Closure quit_callback;

WaitFunction() {
RunLoop run_loop;
quit_closure = run_loop.QuitClosure();
run_loop.Run();
}

EventFunction() {
quit_closure.Run();
}

[maniscalco, 2014-05-16 16:46:36]

Thanks for the feedback, RFAL!

https://codereview.chromium.org/278263003/diff/40001/sync/internal_api/attach...
File sync/internal_api/attachments/attachment_uploader_impl_unittest.cc (right):

https://codereview.chromium.org/278263003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl_unittest.cc:34: class
AttachmentUploaderImplTest;
On 2014/05/15 23:27:28, pavely wrote:
> Not needed

Done.

https://codereview.chromium.org/278263003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl_unittest.cc:84: class
RequestHandler : public base::NonThreadSafe {
Not strictly necessary, but I found it made it easier to keep things
thread-safe.  EmbeddedTestServer will invoke the registered callback in some
thread other than the one that owns ETS.  I figure it better to make
RequestHandler its own class than to make the test fixture thread-safe.

On 2014/05/15 23:27:28, pavely wrote:
> Do you really need RequestHandler as separate class? It is essentially just
one
> function.

https://codereview.chromium.org/278263003/diff/40001/sync/internal_api/attach...
sync/internal_api/attachments/attachment_uploader_impl_unittest.cc:133:
message_loop_.Run();
I like this pattern better.  Done.  Thanks for pointing it out!

On 2014/05/15 23:27:28, pavely wrote:
> In some previous change Tim asked me not to use MessageLoop directly in tests
> but instead use RunLoop.
> The pattern is this:
> 
> Closure quit_callback;
> 
> WaitFunction() {
> RunLoop run_loop;
> quit_closure = run_loop.QuitClosure();
> run_loop.Run();
> }
> 
> EventFunction() {
> quit_closure.Run();
> }

[pavely, 2014-05-16 18:34:57]

lgtm

[maniscalco, 2014-05-16 18:46:16]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-05-16 18:47:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/maniscalco@chromium.org/278263003/60001

[commit-bot: I haz the power, 2014-05-16 19:18:52]

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:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-17 15:52:13]

Change committed as 271212