[Sync] Move HttpBridge to sync/

[Sync] Move HttpBridge to sync/

Remove all references to content in HttpBridge.

Clean up handling of HTTP user agent in HttpBridge.

BUG=133791
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=143575

[akalin, 2012-06-21 18:53:30]

+tim for review

[akalin, 2012-06-21 20:00:23]

Fixed DEPS, added TODO.  Basically I think we eventually want to get rid of
HttpPostProvider and instead use the URLFetcher interface directly.  We
currently just use HttpPostProvider to stub out the network calls for test, so
we can simply use TestURLFetcher instead.

[tim (not reviewing), 2012-06-21 20:19:17]

On 2012/06/21 20:00:23, akalin wrote:
> Fixed DEPS, added TODO.  Basically I think we eventually want to get rid of
> HttpPostProvider and instead use the URLFetcher interface directly.  We
> currently just use HttpPostProvider to stub out the network calls for test, so
> we can simply use TestURLFetcher instead.

Not sure I follow the "use URLFetcher directly" comment? The reason this class
exists isn't for tests, it's because we wrap the async URLFetcher api in a
synchronous one.  That and it is a very convenient choke point for all sync
related http traffic. 

However we should get rid of HttpPostProvider if the only implementation is now
actually in sync/.  Previously the idea was that the embedder environment
provides the actual transport.

[akalin, 2012-06-21 20:41:10]

On 2012/06/21 20:19:17, timsteele wrote:
> On 2012/06/21 20:00:23, akalin wrote:
> > Fixed DEPS, added TODO.  Basically I think we eventually want to get rid of
> > HttpPostProvider and instead use the URLFetcher interface directly.  We
> > currently just use HttpPostProvider to stub out the network calls for test,
so
> > we can simply use TestURLFetcher instead.
> 
> Not sure I follow the "use URLFetcher directly" comment? The reason this class
> exists isn't for tests, it's because we wrap the async URLFetcher api in a
> synchronous one.  That and it is a very convenient choke point for all sync
> related http traffic. 

Well, HttpBridge exists to wrap URLFetcher, but the interface HttpPostProvider
exists for the choke point reason, as well as for tests (which is what it's
currently being used for).  I'm saying that URLFetcher can serve the same
purpose; HttpBridge should still work if it wraps a TestURLFetcher.

> However we should get rid of HttpPostProvider if the only implementation is
now
> actually in sync/.  Previously the idea was that the embedder environment
> provides the actual transport.

Yeah, I think we're in agreement here.  Should I file a bug/add a TODO
somewhere?

[tim (not reviewing), 2012-06-21 20:56:13]

On 2012/06/21 20:41:10, akalin wrote:
> On 2012/06/21 20:19:17, timsteele wrote:
> > On 2012/06/21 20:00:23, akalin wrote:
> > > Fixed DEPS, added TODO.  Basically I think we eventually want to get rid
of
> > > HttpPostProvider and instead use the URLFetcher interface directly.  We
> > > currently just use HttpPostProvider to stub out the network calls for
test,
> so
> > > we can simply use TestURLFetcher instead.
> > 
> > Not sure I follow the "use URLFetcher directly" comment? The reason this
class
> > exists isn't for tests, it's because we wrap the async URLFetcher api in a
> > synchronous one.  That and it is a very convenient choke point for all sync
> > related http traffic. 
> 
> Well, HttpBridge exists to wrap URLFetcher, but the interface HttpPostProvider
> exists for the choke point reason, as well as for tests (which is what it's
> currently being used for).  I'm saying that URLFetcher can serve the same
> purpose; HttpBridge should still work if it wraps a TestURLFetcher.
> 
> > However we should get rid of HttpPostProvider if the only implementation is
> now
> > actually in sync/.  Previously the idea was that the embedder environment
> > provides the actual transport.
> 
> Yeah, I think we're in agreement here.  Should I file a bug/add a TODO
> somewhere?

Ok, yeah, makes sense. Lets file a bug to remove HttpPostProvider.  LGTM

[commit-bot: I haz the power, 2012-06-21 20:58:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/10645004/6001

[commit-bot: I haz the power, 2012-06-21 22:51:30]

Try job failure for 10645004-6001 (retry) (retry) on mac_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-06-22 06:03:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/10645004/6001

[commit-bot: I haz the power, 2012-06-22 09:01:35]

Failed to apply patch for sync/internal_api/public/http_bridge.h:
While running patch -p1 --forward --force;
patching file sync/internal_api/public/http_bridge.h
Hunk #5 FAILED at 99.
Hunk #7 succeeded at 208 with fuzz 2.
1 out of 8 hunks FAILED -- saving rejects to file
sync/internal_api/public/http_bridge.h.rej

[commit-bot: I haz the power, 2012-06-22 09:33:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/10645004/4002

[commit-bot: I haz the power, 2012-06-22 12:11:39]

Change committed as 143575

[nilesh, 2012-06-22 16:25:10]

On 2012/06/22 12:11:39, I haz the power (commit-bot) wrote:
> Change committed as 143575

FYI: This breaks sync_unittests for Android (I guess because of the lack of test
server support).
http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Linux%20Android/...

[nilesh, 2012-06-22 16:26:34]

On 2012/06/22 16:25:10, nileshagrawal1 wrote:
> On 2012/06/22 12:11:39, I haz the power (commit-bot) wrote:
> > Change committed as 143575
> 
> FYI: This breaks sync_unittests for Android (I guess because of the lack of
test
> server support).
>
http://build.chromium.org/p/chromium.fyi/builders/Chromium%2520Linux%2520Andr...
Trying the link again:
http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Linux%20Android

[akalin, 2012-06-22 18:30:58]

On 2012/06/22 16:26:34, nileshagrawal1 wrote:
> On 2012/06/22 16:25:10, nileshagrawal1 wrote:
> > On 2012/06/22 12:11:39, I haz the power (commit-bot) wrote:
> > > Change committed as 143575
> > 
> > FYI: This breaks sync_unittests for Android (I guess because of the lack of
> test
> > server support).
> >
>
http://build.chromium.org/p/chromium.fyi/builders/Chromium%252520Linux%252520...
> Trying the link again:
>
http://build.chromium.org/p/chromium.fyi/builders/Chromium%2520Linux%2520Android

Hmm, neither link works.  Are you saying it's failing because the HttpBridge
tests require a testserver?

[Agrawal, 2012-06-22 19:06:38]

On Fri, Jun 22, 2012 at 11:30 AM, <akalin@chromium.org> wrote:

> On 2012/06/22 16:26:34, nileshagrawal1 wrote:
>
>> On 2012/06/22 16:25:10, nileshagrawal1 wrote:
>> > On 2012/06/22 12:11:39, I haz the power (commit-bot) wrote:
>> > > Change committed as 143575
>> >
>> > FYI: This breaks sync_unittests for Android (I guess because of the
>> lack of
>> test
>> > server support).
>> >
>>
>
> http://build.chromium.org/p/**chromium.fyi/builders/**
>
Chromium%252520Linux%**252520Android/builds/10552<http://build.chromium.org/p/chromium.fyi/builders/Chromium%252520Linux%252520Android/builds/10552>
>
>> Trying the link again:
>>
>
> http://build.chromium.org/p/**chromium.fyi/builders/**
>
Chromium%2520Linux%2520Android<http://build.chromium.org/p/chromium.fyi/builders/Chromium%2520Linux%2520Android>
>
> Hmm, neither link works.  Are you saying it's failing because the
> HttpBridge
> tests require a testserver?
>

http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Linux%20Android/...

Cl to disable the tests for android is underway.
http://codereview.chromium.org/10639019/



>
>
https://chromiumcodereview.**appspot.com/10645004/<https://chromiumcodereview...
>