net/url_request/test_url_fetcher_factory.h - Issue 12211076: Refactored FakeURLFetcher to make it more flexible

Side by Side Diff: net/url_request/test_url_fetcher_factory.h

Issue 12211076: Refactored FakeURLFetcher to make it more flexible (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Fix linter errors Created 7 years, 10 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

OLD	NEW
1 // Copyright (c) 2012 The Chromium Authors. All rights reserved.	1 // Copyright (c) 2012 The Chromium Authors. All rights reserved.

2 // Use of this source code is governed by a BSD-style license that can be	2 // Use of this source code is governed by a BSD-style license that can be

3 // found in the LICENSE file.	3 // found in the LICENSE file.

4	4

5 #ifndef NET_URL_REQUEST_TEST_URL_FETCHER_FACTORY_H_	5 #ifndef NET_URL_REQUEST_TEST_URL_FETCHER_FACTORY_H_

6 #define NET_URL_REQUEST_TEST_URL_FETCHER_FACTORY_H_	6 #define NET_URL_REQUEST_TEST_URL_FETCHER_FACTORY_H_

7	7

8 #include <list>	8 #include <list>

9 #include <map>	9 #include <map>

10 #include <string>	10 #include <string>

11 #include <utility>	11 #include <utility>

12	12

	13
	akalin 2013/02/12 23:10:10 remove extra newline remove extra newline Noam Samuel (WRONG ACCOUNT) 2013/02/13 00:25:52 Done. Show quoted text On 2013/02/12 23:10:10, akalin wrote: > remove extra newline Done.
	14 #include "base/callback.h"

13 #include "base/compiler_specific.h"	15 #include "base/compiler_specific.h"

	16 #include "base/memory/weak_ptr.h"

14 #include "base/threading/non_thread_safe.h"	17 #include "base/threading/non_thread_safe.h"

15 #include "googleurl/src/gurl.h"	18 #include "googleurl/src/gurl.h"

16 #include "net/http/http_request_headers.h"	19 #include "net/http/http_request_headers.h"

17 #include "net/url_request/url_fetcher_factory.h"	20 #include "net/url_request/url_fetcher_factory.h"

18 #include "net/url_request/url_request_status.h"	21 #include "net/url_request/url_request_status.h"

19	22

	23
	akalin 2013/02/12 23:10:10 remove extra newline remove extra newline Noam Samuel (WRONG ACCOUNT) 2013/02/13 00:25:52 Done. Show quoted text On 2013/02/12 23:10:10, akalin wrote: > remove extra newline Done.
20 namespace net {	24 namespace net {

21	25

22 // Changes URLFetcher's Factory for the lifetime of the object.	26 // Changes URLFetcher's Factory for the lifetime of the object.

23 // Note that this scoper cannot be nested (to make it even harder to misuse).	27 // Note that this scoper cannot be nested (to make it even harder to misuse).

24 class ScopedURLFetcherFactory : public base::NonThreadSafe {	28 class ScopedURLFetcherFactory : public base::NonThreadSafe {

25 public:	29 public:

26 explicit ScopedURLFetcherFactory(URLFetcherFactory* factory);	30 explicit ScopedURLFetcherFactory(URLFetcherFactory* factory);

27 virtual ~ScopedURLFetcherFactory();	31 virtual ~ScopedURLFetcherFactory();

28	32

29 private:	33 private:

(...skipping 250 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
280 // factory.SetFakeResponse("http://a.com/", "", false);	284 // factory.SetFakeResponse("http://a.com/", "", false);

281 // // But if the service requests http://b.com/asdf you want to respond with	285 // // But if the service requests http://b.com/asdf you want to respond with

282 // // a simple html page and an HTTP/200 code.	286 // // a simple html page and an HTTP/200 code.

283 // factory.SetFakeResponse("http://b.com/asdf",	287 // factory.SetFakeResponse("http://b.com/asdf",

284 // "<html><body>hello world</body></html>",	288 // "<html><body>hello world</body></html>",

285 // true);	289 // true);

286 //	290 //

287 // SomeService service;	291 // SomeService service;

288 // service.Run(); // Will eventually request these two URLs.	292 // service.Run(); // Will eventually request these two URLs.

289	293

	294 class FakeURLFetcher;

290 class FakeURLFetcherFactory : public URLFetcherFactory,	295 class FakeURLFetcherFactory : public URLFetcherFactory,

291 public ScopedURLFetcherFactory {	296 public ScopedURLFetcherFactory {

292 public:	297 public:

	298 typedef base::Callback< FakeURLFetcher*(
	akalin 2013/02/12 23:10:10 no space after < no space after < akalin 2013/02/12 23:10:10 add comment describing what the parameters are (si add comment describing what the parameters are (since they don't have names) akalin 2013/02/12 23:10:10 since the caller should take ownership of the retu since the caller should take ownership of the returned FakeURLFetcher, perhaps have it return a scoped_ptr<FakeURLFetcher>? Noam Samuel (WRONG ACCOUNT)* 2013/02/13 00:25:52 Done. Show quoted text On 2013/02/12 23:10:10, akalin wrote: > no space after < Done. Noam Samuel (WRONG ACCOUNT) 2013/02/13 00:25:52 Since this callback is only called by the factory Show quoted text On 2013/02/12 23:10:10, akalin wrote: > since the caller should take ownership of the returned FakeURLFetcher, perhaps > have it return a scoped_ptr<FakeURLFetcher>? Since this callback is only called by the factory itself, which should return a URLFetcher (per its interface), I do not think this is the case. Noam Samuel (WRONG ACCOUNT) 2013/02/13 00:25:52 Done. Show quoted text On 2013/02/12 23:10:10, akalin wrote: > add comment describing what the parameters are (since they don't have names) Done. akalin 2013/02/13 20:44:01 I'd argue that the factory method should also retu Show quoted text On 2013/02/13 00:25:52, Noam Samuel wrote: > On 2013/02/12 23:10:10, akalin wrote: > > since the caller should take ownership of the returned FakeURLFetcher, > perhaps > > have it return a scoped_ptr<FakeURLFetcher>? > > Since this callback is only called by the factory itself, which should return a > URLFetcher (per its interface), I do not think this is the case. I'd argue that the factory method should also return a scoped_ptr<>. That might require touching too many files, though, so I'm okay with leaving the scoped_ptr<> in the callback and calling release.
	299 const GURL&,

	300 URLFetcherDelegate*,

	301 const std::string&,

	302 bool) > FakeURLFetcherCreator;
	akalin 2013/02/12 23:10:10 no space before > no space before > Noam Samuel (WRONG ACCOUNT) 2013/02/13 00:25:52 Done. Show quoted text On 2013/02/12 23:10:10, akalin wrote: > no space before > Done.
	303

293 FakeURLFetcherFactory();	304 FakeURLFetcherFactory();
	akalin 2013/02/12 23:10:10 see if you can get away with only having a single see if you can get away with only having a single constructor. There are only a few places that uses FakeURLFetcherFactory so it shouldn't be too hard. You may need to expose a "MakeFakeURLFetcher()" function that simply calls new FakeURLFetcher.... Noam Samuel (WRONG ACCOUNT) 2013/02/13 00:25:52 Since the callback has an obvious but nontrivial d Since the callback has an obvious but nontrivial default value, wouldn't it make sense to leave the option of not passing it? The creation of this class may be confusing otherwise. On 2013/02/12 23:10:10, akalin wrote: Show quoted text > see if you can get away with only having a single constructor. There are only a > few places that uses FakeURLFetcherFactory so it shouldn't be too hard. You may > need to expose a "MakeFakeURLFetcher()" function that simply calls new > FakeURLFetcher.... akalin 2013/02/13 20:44:01 I suppose. Although I would at least like to get r Show quoted text On 2013/02/13 00:25:52, Noam Samuel wrote: > Since the callback has an obvious but nontrivial default > value, wouldn't it make sense to leave the option of not > passing it? The creation of this class may be confusing > otherwise. I suppose. Although I would at least like to get rid of the default constructor (and have callers pass in NULL). Noam Samuel (WRONG ACCOUNT) 2013/02/13 21:37:46 Added to latest patch set. If it's too cumbersome Show quoted text On 2013/02/13 20:44:01, akalin wrote: > On 2013/02/13 00:25:52, Noam Samuel wrote: > > Since the callback has an obvious but nontrivial default > > value, wouldn't it make sense to leave the option of not > > passing it? The creation of this class may be confusing > > otherwise. > > I suppose. Although I would at least like to get rid of the default constructor > (and have callers pass in NULL). Added to latest patch set. If it's too cumbersome as part of this CL, it can also be factored into its own CL.
294 // FakeURLFetcherFactory that will delegate creating URLFetcher for unknown	305 // FakeURLFetcherFactory that will delegate creating URLFetcher for unknown

295 // url to the given factory.	306 // url to the given factory.

296 explicit FakeURLFetcherFactory(URLFetcherFactory* default_factory);	307 explicit FakeURLFetcherFactory(URLFetcherFactory* default_factory);

	308

	309 // FakeURLFetcherFactory that will delegate creating URLFetcher for unknown
	akalin 2013/02/12 23:10:10 this comment isn't a complete sentence and is hard this comment isn't a complete sentence and is hard to understand. Perhaps something like: // \|creator\| is a callback that ... // \|default_factory\|, which can be NULL, is a ... Noam Samuel (WRONG ACCOUNT) 2013/02/13 00:25:52 Done. Show quoted text On 2013/02/12 23:10:10, akalin wrote: > this comment isn't a complete sentence and is hard to understand. Perhaps > something like: > > // \|creator\| is a callback that ... > // \|default_factory\|, which can be NULL, is a ... Done.
	310 // url to the given factory (can be NULL) and will delegate creating for

	311 // known url to given callback

	312 FakeURLFetcherFactory(const FakeURLFetcherCreator& creator,

	313 URLFetcherFactory* default_factory);

	314

297 virtual ~FakeURLFetcherFactory();	315 virtual ~FakeURLFetcherFactory();

298	316

299 // If no fake response is set for the given URL this method will delegate the	317 // If no fake response is set for the given URL this method will delegate the

300 // call to \|default_factory_\| if it is not NULL, or return NULL if it is	318 // call to \|default_factory_\| if it is not NULL, or return NULL if it is

301 // NULL.	319 // NULL.

302 // Otherwise, it will return a URLFetcher object which will respond with the	320 // Otherwise, it will return a URLFetcher object which will respond with the

303 // pre-baked response that the client has set by calling SetFakeResponse().	321 // pre-baked response that the client has set by calling SetFakeResponse().

304 virtual URLFetcher* CreateURLFetcher(	322 virtual URLFetcher* CreateURLFetcher(

305 int id,	323 int id,

306 const GURL& url,	324 const GURL& url,

307 URLFetcher::RequestType request_type,	325 URLFetcher::RequestType request_type,

308 URLFetcherDelegate* d) OVERRIDE;	326 URLFetcherDelegate* d) OVERRIDE;

309	327

310 // Sets the fake response for a given URL. If success is true we will serve	328 // Sets the fake response for a given URL. If success is true we will serve

311 // an HTTP/200 and an HTTP/500 otherwise. The \|response_data\| may be empty.	329 // an HTTP/200 and an HTTP/500 otherwise. The \|response_data\| may be empty.

312 void SetFakeResponse(const std::string& url,	330 void SetFakeResponse(const std::string& url,

313 const std::string& response_data,	331 const std::string& response_data,

314 bool success);	332 bool success);

315	333

	334
	akalin 2013/02/12 23:10:10 remove extra newline remove extra newline Noam Samuel (WRONG ACCOUNT) 2013/02/13 00:25:52 Done. Show quoted text On 2013/02/12 23:10:10, akalin wrote: > remove extra newline Done.
316 // Clear all the fake responses that were previously set via	335 // Clear all the fake responses that were previously set via

317 // SetFakeResponse().	336 // SetFakeResponse().

318 void ClearFakeResponses();	337 void ClearFakeResponses();

319	338

320 private:	339 private:

	340 FakeURLFetcherCreator creator_;
	akalin 2013/02/12 23:10:10 make it const make it const Noam Samuel (WRONG ACCOUNT) 2013/02/13 00:25:52 Done. Show quoted text On 2013/02/12 23:10:10, akalin wrote: > make it const Done.
321 typedef std::map<GURL, std::pair<std::string, bool> > FakeResponseMap;	341 typedef std::map<GURL, std::pair<std::string, bool> > FakeResponseMap;

322 FakeResponseMap fake_responses_;	342 FakeResponseMap fake_responses_;

323 URLFetcherFactory* default_factory_;	343 URLFetcherFactory* default_factory_;
	akalin 2013/02/12 23:10:10 make this const, like URLFetcherFactory* const make this const, like URLFetcherFactory* const Noam Samuel (WRONG ACCOUNT) 2013/02/13 00:25:52 Done. Show quoted text On 2013/02/12 23:10:10, akalin wrote: > make this const, like > > URLFetcherFactory* const Done.
324	344

	345

	346 static FakeURLFetcher* DefaultFakeURLFetcherCreator(

	347 const GURL& url,

	348 URLFetcherDelegate* delegate,

	349 const std::string& response,

	350 bool success);

325 DISALLOW_COPY_AND_ASSIGN(FakeURLFetcherFactory);	351 DISALLOW_COPY_AND_ASSIGN(FakeURLFetcherFactory);

326 };	352 };

327	353

	354

	355

	356 // This class is used by the FakeURLFetcherFactory above.
	akalin 2013/02/12 23:10:10 Try to describe what the FakeURLFetcher does witho Try to describe what the FakeURLFetcher does without referring to FakeURLFetcherFactory. Noam Samuel (WRONG ACCOUNT) 2013/02/13 00:25:52 Done. Show quoted text On 2013/02/12 23:10:10, akalin wrote: > Try to describe what the FakeURLFetcher does without referring to > FakeURLFetcherFactory. Done.
	357 class FakeURLFetcher : public TestURLFetcher {
	akalin 2013/02/12 23:10:10 you can move this declaration up above FakeURLFetc you can move this declaration up above FakeURLFetcherFactory instead of just forward-declaring. Noam Samuel (WRONG ACCOUNT) 2013/02/13 00:25:52 Done. Show quoted text On 2013/02/12 23:10:10, akalin wrote: > you can move this declaration up above FakeURLFetcherFactory instead of just > forward-declaring. Done.
	358 public:

	359 // Normal URL fetcher constructor but also takes in a pre-baked response.

	360 FakeURLFetcher(const GURL& url,

	361 URLFetcherDelegate* d,

	362 const std::string& response_data, bool success);

	363

	364 // Start the request. This will call the given delegate asynchronously

	365 // with the pre-baked response as parameter.

	366 virtual void Start() OVERRIDE;

	367

	368 virtual const GURL& GetURL() const OVERRIDE;

	369

	370 private:

	371 virtual ~FakeURLFetcher();

	372

	373 // This is the method which actually calls the delegate that is passed in the

	374 // constructor.

	375 void RunDelegate();

	376

	377 base::WeakPtrFactory<FakeURLFetcher> weak_factory_;

	378

	379 DISALLOW_COPY_AND_ASSIGN(FakeURLFetcher);

	380 };

	381

	382

328 // This is an implementation of URLFetcherFactory that will create a	383 // This is an implementation of URLFetcherFactory that will create a

329 // URLFetcherImpl. It can be use in conjunction with a FakeURLFetcherFactory in	384 // URLFetcherImpl. It can be use in conjunction with a FakeURLFetcherFactory in

330 // integration tests to control the behavior of some requests but execute	385 // integration tests to control the behavior of some requests but execute

331 // all the other ones.	386 // all the other ones.

332 class URLFetcherImplFactory : public URLFetcherFactory {	387 class URLFetcherImplFactory : public URLFetcherFactory {

333 public:	388 public:

334 URLFetcherImplFactory();	389 URLFetcherImplFactory();

335 virtual ~URLFetcherImplFactory();	390 virtual ~URLFetcherImplFactory();

336	391

337 // This method will create a real URLFetcher.	392 // This method will create a real URLFetcher.

338 virtual URLFetcher* CreateURLFetcher(	393 virtual URLFetcher* CreateURLFetcher(

339 int id,	394 int id,

340 const GURL& url,	395 const GURL& url,

341 URLFetcher::RequestType request_type,	396 URLFetcher::RequestType request_type,

342 URLFetcherDelegate* d) OVERRIDE;	397 URLFetcherDelegate* d) OVERRIDE;

343	398

344 };	399 };

345	400

346 } // namespace net	401 } // namespace net

347	402

348 #endif // NET_URL_REQUEST_TEST_URL_FETCHER_FACTORY_H_	403 #endif // NET_URL_REQUEST_TEST_URL_FETCHER_FACTORY_H_

OLD	NEW

« no previous file with comments | « no previous file | net/url_request/test_url_fetcher_factory.cc » ('j') | no next file with comments »